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

[Zhang, Qi Z]

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 at intel.com>; Dai, Wei <wei.dai at intel.com>; Xing,
> Beilei <beilei.xing at intel.com>; Wu, Jingjing <jingjing.wu at intel.com>;
> dev at 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 

> 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 at intel.com>; Dai, Wei
> <wei.dai at intel.com>; Xing, Beilei <beilei.xing at intel.com>; Wu, Jingjing
> <jingjing.wu at intel.com>; dev at 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 at intel.com>; Xing, Beilei
> > <beilei.xing at intel.com>; Zhang, Qi Z <qi.z.zhang at intel.com>; Wu,
> > Jingjing <jingjing.wu at intel.com>; dev at dpdk.org
> > Cc: Jankowski, KonradX <konradx.jankowski at 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 at 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