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

["Zhang, Helin" <helin.zhang@intel.com>]

> -----Original Message-----
> From: Zhang, Helin
> Sent: Friday, April 6, 2018 11:48 PM
> To: Jankowski, Konrad; Zhang, Qi Z; Dai, Wei; Xing, Beilei; Wu, Jingjing;
> dev@dpdk.org
> Subject: RE: [PATCH] net/i40evf: regression fix - reenable interrupts in handler
> 
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Jankowski, Konrad
> > Sent: Thursday, April 5, 2018 1:12 AM
> > To: Zhang, Qi Z; Dai, Wei; Xing, Beilei; Wu, Jingjing; dev@dpdk.org
> > Subject: Re: [dpdk-dev] [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.
> > 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).
> 
> Hi Konrad
> Please don't use uio_pci_generic, as it is not supported by X710/XL710/XXV710
> series, as below. With VFIO, do you see similar issue?
> http://www.dpdk.org/doc/guides/rel_notes/known_issues.html#uio-pci-
> generic-module-bind-failed-in-x710-xl710-xxv710
> 
> > 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
Better title might be needed, with start of 'fix'. Also note that 'Fixes' line is
needed, and 'Cc' line might be needed.
> > >
> > > 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.
Also please fix the checkpatch warnings on patchwork. Thanks!

/Helin
> > >
> > > 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.