Re: [dpdk-dev] [PATCH v2] igb_uio: issue FLR during open and release of device file

["Tan, Jianfeng" <jianfeng.tan@intel.com>]

> -----Original Message-----
> From: Shijith Thotton [mailto:shijith.thotton@caviumnetworks.com]
> Sent: Monday, July 10, 2017 6:43 PM
> To: Tan, Jianfeng
> Cc: dev@dpdk.org; Yigit, Ferruh; Gregory Etelson; Thomas Monjalon;
> Stephen Hemminger; Lu, Wenzhuo
> Subject: Re: [PATCH v2] igb_uio: issue FLR during open and release of device
> file
> 
> On Mon, Jul 10, 2017 at 09:00:38AM +0000, Tan, Jianfeng wrote:
> >
> >
> > > -----Original Message-----
> > > From: Shijith Thotton [mailto:shijith.thotton@caviumnetworks.com]
> > > Sent: Monday, July 10, 2017 3:11 PM
> > > To: Tan, Jianfeng
> > > Cc: dev@dpdk.org; Yigit, Ferruh; Gregory Etelson; Thomas Monjalon;
> > > Stephen Hemminger; Lu, Wenzhuo
> > > Subject: Re: [PATCH v2] igb_uio: issue FLR during open and release of
> device
> > > file
> > >
> > > On Mon, Jul 10, 2017 at 03:38:34AM +0000, Tan, Jianfeng wrote:
> > > > Hi Thotton,
> > > >
> > > > > -----Original Message-----
> > > > > From: Shijith Thotton [mailto:shijith.thotton@caviumnetworks.com]
> > > > > Sent: Friday, July 7, 2017 7:14 PM
> > > > > To: dev@dpdk.org
> > > > > Cc: Yigit, Ferruh; Gregory Etelson; Thomas Monjalon; Stephen
> > > Hemminger;
> > > > > Tan, Jianfeng; Lu, Wenzhuo
> > > > > Subject: [PATCH v2] igb_uio: issue FLR during open and release of
> device
> > > file
> > > > >
> > > > > Set UIO info device file operations open and release. Call pci reset
> > > > > function inside open and release to clear device state at start and end.
> > > > > Copied this behaviour from vfio_pci kernel module code. With this
> patch,
> > > > > it is not mandatory to issue FLR by PMD's during init and close.
> > > >
> > > > I'm afraid this will not work for restarted DPDK process. In current
> probe(),
> > > we set up the I/O mem and I/O port; and those sys files are used by EAL
> > > IGB_UIO initialization code to map I/O mem and port. After reset in
> release(),
> > > we will lose those sys files in next open().
> > > >
> > > > Thanks,
> > > > Jianfeng
> > > >
> > > > >
> > > > > Bus master enable and disable are added in open and release
> > > respectively
> > > > > to take care of device DMA.
> > > > >
> > > > > Signed-off-by: Shijith Thotton <shijith.thotton@caviumnetworks.com>
> > > > > ---
> > > > > v2 changes:
> > > > >  - Replaced pci_try_reset_function with pci_reset_function as it is not
> > > > >    available in older kernel versions.
> > > > >
> > > > > v1 changes:
> > > > >  - Added pci set master inside open and clear master inside release.
> > > > >  - Remove obvious comments.
> > > > >
> > > > > RFC: http://dpdk.org/ml/archives/dev/2017-May/066917.html
> > > > >
> > > > >  lib/librte_eal/linuxapp/igb_uio/igb_uio.c | 33
> > > > > +++++++++++++++++++++++++++++++
> > > > >  1 file changed, 33 insertions(+)
> > > > >
> > > > > diff --git a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
> > > > > b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
> > > > > index b9d427c..07a19a3 100644
> > > > > --- a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
> > > > > +++ b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
> > > > > @@ -170,6 +170,37 @@ struct rte_uio_pci_dev {
> > > > >  	return IRQ_HANDLED;
> > > > >  }
> > > > >
> > > > > +/**
> > > > > + * This gets called while opening uio device file.
> > > > > + */
> > > > > +static int
> > > > > +igbuio_pci_open(struct uio_info *info, struct inode *inode)
> > > > > +{
> > > > > +	struct rte_uio_pci_dev *udev = info->priv;
> > > > > +	struct pci_dev *dev = udev->pdev;
> > > > > +
> > > > > +	pci_reset_function(dev);
> > > > > +
> > > > > +	/* set bus master, which was cleared by the reset function */
> > > > > +	pci_set_master(dev);
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static int
> > > > > +igbuio_pci_release(struct uio_info *info, struct inode *inode)
> > > > > +{
> > > > > +	struct rte_uio_pci_dev *udev = info->priv;
> > > > > +	struct pci_dev *dev = udev->pdev;
> > > > > +
> > > > > +	/* stop the device from further DMA */
> > > > > +	pci_clear_master(dev);
> > > > > +
> > > > > +	pci_reset_function(dev);
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > >  #ifdef CONFIG_XEN_DOM0
> > > > >  static int
> > > > >  igbuio_dom0_mmap_phys(struct uio_info *info, struct
> vm_area_struct
> > > > > *vma)
> > > > > @@ -372,6 +403,8 @@ struct rte_uio_pci_dev {
> > > > >  	udev->info.version = "0.1";
> > > > >  	udev->info.handler = igbuio_pci_irqhandler;
> > > > >  	udev->info.irqcontrol = igbuio_pci_irqcontrol;
> > > > > +	udev->info.open = igbuio_pci_open;
> > > > > +	udev->info.release = igbuio_pci_release;
> > > > >  #ifdef CONFIG_XEN_DOM0
> > > > >  	/* check if the driver run on Xen Dom0 */
> > > > >  	if (xen_initial_domain())
> > > > > --
> > > > > 1.8.3.1
> > > >
> > >
> > > Hi Jianfeng,
> > >
> > > I have tested the patch with LiquidIO VFs in VM using testpmd and could
> not
> > > see
> > > any issue over multiple runs.
> >
> > I got that, you are using pci_reset_function() instead of pci_disable_device
> (the function I was trying). So only one question left, from the comment of
> pci_reset_function(), it "saves and restores device state over  the reset",
> then is __pci_reset_function() is more proper here?
> 
> Per comments of __pci_reset_function:
>  * Resetting the device will make the contents of PCI configuration space
>  * random, so any caller of this must be prepared to reinitialise the
>  * device including MSI, bus mastering, BARs, decoding IO and memory
> spaces,
>  * etc.
> 
> So thought, pci_reset_function would be proper as it saves and restores
> state.

Make sense. My was thinking the device will be re-initialized anyway and not necessary to restore the state. But we cannot leave BARs random as device cannot manage the physical memory layout.

Testing on virtio devices shows that function works well. And this avoids compatibility issue (as my patch).

Great work!

Thanks,
Jianfeng