Re: [dpdk-dev] [PATCH] net/i40evf: regression fix - reenable interrupts in handler

["Jankowski, Konrad" <konrad.jankowski@intel.com>]

Hi Qi,

> -----Original Message-----
> From: Zhang, Qi Z
> Sent: Tuesday, April 10, 2018 2:26 PM
> To: Jankowski, Konrad <konrad.jankowski@intel.com>; Dai, Wei
> <wei.dai@intel.com>; Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>; dev@dpdk.org
> Subject: RE: [PATCH] net/i40evf: regression fix - reenable interrupts in handler
> 
> Hi Konrad:
> 	Sorry for late reply.
> 
> > -----Original Message-----
> > From: Jankowski, Konrad
> > Sent: Thursday, April 5, 2018 1:12 AM
> > To: Zhang, Qi Z <qi.z.zhang@intel.com>; Dai, Wei <wei.dai@intel.com>;
> > Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing
> > <jingjing.wu@intel.com>; dev@dpdk.org
> > Subject: RE: [PATCH] net/i40evf: regression fix - reenable interrupts
> > in handler
> >
> > Hi Zhang,
> >
> > If you look at the source of interrupt handlers in both the igb_uio
> > and uio_pci_generic drivers (look for irqreturn_t type), you will see
> > they can disable interrupts immediately after receipt of one.
> 
> Do you mean
> 
> if (udev->mode == RTE_INTR_MODE_LEGACY &&
> 	    !pci_check_and_mask_intx(udev->pdev))
> 		return IRQ_NONE;
> 
> so you are using legacy interrupt mode?
> 
> According i40e datasheet (chapter 7.5.1), legacy interrupt mode is only
> supported by PF, So that's why we have rte_intr_enable in
> i40e_dev_interrupt_handler.
> But for VF, it only support msi-x interrupt mode.
> 
> Regards
> Qi

OK I see. Still the old version of igb_uio disables interrupts in both cases, MSI or legacy.
That version comes provided with windriver OVP6, and that's the only version that
actually works on this system (back from DPDK 1.6 accrording to rte_version.h).
If we're ok with not supporting this system, I'm ok with dropping this patch.
(I'll just use it locally myself)

Cheers,
Konrad

> > It's up to the user to make sure they're re-enabled. Also please
> > compare with code in i40e_ethdev.c, which still does this correctly -
> > there's an explicit
> > rte_intr_enable(dev->intr_handle) call at the end of
> > i40e_dev_interrupt_handler().
> > Probably a cleaner approach would be to leave them disabled as is, but
> > only enable them for a once-off receipt when sending an AdminQ
> > message, maybe that was the assumption here. However I've added some
> > tracing to
> > igbuio_pci_irqcontrol() and I'm sure this isn't happening on my system.
> > (nothing is enabling those interrupts post device init). Looks like
> > code path might be different with the newest igb_uio driver and MSI
> > enabled, but the current code will still not work for all cases (like with
> uio_pci_generic).
> > There can also be cases when you have and older igb_uio driver which
> > disables interrupts in all cases and running it with with a new DPDK.
> > (vendor provided compiled driver) I think for full compatibility we
> > need to keep re-enabling those interrupts.
> >
> > Regards,
> > Konrad
> >
> > -----Original Message-----
> > From: Zhang, Qi Z
> > Sent: Wednesday, March 28, 2018 4:37 AM
> > To: Jankowski, Konrad <konrad.jankowski@intel.com>; Dai, Wei
> > <wei.dai@intel.com>; Xing, Beilei <beilei.xing@intel.com>; Wu,
> > Jingjing <jingjing.wu@intel.com>; dev@dpdk.org
> > Subject: RE: [PATCH] net/i40evf: regression fix - reenable interrupts
> > in handler
> >
> > Hi Jankowski:
> >
> > > -----Original Message-----
> > > From: Jankowski, KonradX
> > > Sent: Thursday, February 15, 2018 2:33 AM
> > > To: Dai, Wei <wei.dai@intel.com>; Xing, Beilei
> > > <beilei.xing@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Wu,
> > > Jingjing <jingjing.wu@intel.com>; dev@dpdk.org
> > > Cc: Jankowski, KonradX <konradx.jankowski@intel.com>
> > > Subject: [PATCH] net/i40evf: regression fix - reenable interrupts in
> > > handler
> > >
> > > Commit 66b8304f removed the rte_intr_enable() call from
> > > i40evf_dev_interrupt_handler() as a "bonus". On one of my systems
> > > this causes the AdminQ messages to stop beeing delivered to the VF.
> > > This results in unability to initialize and use the port. With this
> > > patch it works
> > again.
> > >
> > > System in question:
> > > Wind River OVP6 running kernel
> > > 3.10.58-ovp-rt58-WR6.0.0.13_preempt-rt
> > >
> > > Signed-off-by: Konrad Jankowski <konrad.jankowski@intel.com>
> > > ---
> > >  drivers/net/i40e/i40e_ethdev_vf.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/net/i40e/i40e_ethdev_vf.c
> > > b/drivers/net/i40e/i40e_ethdev_vf.c
> > > index fd003fe..b927a35 100644
> > > --- a/drivers/net/i40e/i40e_ethdev_vf.c
> > > +++ b/drivers/net/i40e/i40e_ethdev_vf.c
> > > @@ -1404,6 +1404,7 @@ i40evf_dev_interrupt_handler(void *param)
> > >
> > >  done:
> > >  	i40evf_enable_irq0(hw);
> > > +	rte_intr_enable(dev->intr_handle);'
> >
> > Would you explain more about why the patch fix the issue?
> > Usually we will not accept a fix just because it work but not
> > understand the root cause.
> >
> > Regards
> > Qi
> >
> > >  }
> > >
> > >  static int
> > > --
> > > 2.5.5

--------------------------------------------------------------
Intel Research and Development Ireland Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263


This e-mail and any attachments may contain confidential material for the sole
use of the intended recipient(s). Any review or distribution by others is
strictly prohibited. If you are not the intended recipient, please contact the
sender and delete all copies.