i40e igb_uio: reset pci on process exit

i40e igb_uio: reset pci on process exit

[Gregory Etelson]

Hello,

My tests show 
if DPDK process attached to i40e VF through igb_uio 
exists without calling rte_eth_dev_close() 
then new instance attached to that VF could experience IO failures.
I came up with the following patch to ensure igb_uio will always reset PCI on process exit.
However, I would like to hear experts opinion to be sure __pci_reset_function
resets i40e VF properly.

Thank you.
Regards,
Gregory 


diff --git a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
index b9d427c..7d7ff9d 100644
--- a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
+++ b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
@@ -170,6 +170,19 @@ struct rte_uio_pci_dev {
        return IRQ_HANDLED;
 }
 
+static int
+igbuio_pci_release(struct uio_info *info, struct inode *inode)
+{
+       int ret;
+       struct rte_uio_pci_dev *udev = info->priv;
+       struct pci_dev *dev = udev->pdev;
+       ret = __pci_reset_function(dev);
+       dev_info(&dev->dev, "pci_reset_function %s \n",
+                 ret == 0 ? "succeded" : "failed");
+       return 0;
+}
+
+
 #ifdef CONFIG_XEN_DOM0
 static int
 igbuio_dom0_mmap_phys(struct uio_info *info, struct vm_area_struct *vma)
@@ -372,6 +385,7 @@ struct rte_uio_pci_dev {
        udev->info.version = "0.1";
        udev->info.handler = igbuio_pci_irqhandler;
        udev->info.irqcontrol = igbuio_pci_irqcontrol;
+       udev->info.release = igbuio_pci_release;
 #ifdef CONFIG_XEN_DOM0
        /* check if the driver run on Xen Dom0 */
        if (xen_initial_domain())

Re: i40e igb_uio: reset pci on process exit

[Stephen Hemminger]

On Wed, 24 May 2017 14:22:11 +0300
Gregory Etelson <gregory@weka.io> wrote:

>  
> +static int
> +igbuio_pci_release(struct uio_info *info, struct inode *inode)
> +{
> +       int ret;
> +       struct rte_uio_pci_dev *udev = info->priv;
> +       struct pci_dev *dev = udev->pdev;
> +       ret = __pci_reset_function(dev);
> +       dev_info(&dev->dev, "pci_reset_function %s \n",
> +                 ret == 0 ? "succeded" : "failed");
> +       return 0;
> +}
> +
> +

Patch in general looks ok, but:
 * no Signed-off
 * whitespace issues
 * doesn't pass kernel coding style
 * don't log on success, why log at all??

Re: i40e igb_uio: reset pci on process exit

[Gregory Etelson]

Hello,

This patch introduces idea I would like to implement in igb_uio driver
However, I would like to get hardware experts confirmation.
If the procedure correct I'll submit proper patch 

Regards,
Gregory

On Thursday, 25 May 2017 21:42:42 IDT Stephen Hemminger wrote:
> On Wed, 24 May 2017 14:22:11 +0300
> Gregory Etelson <gregory@weka.io> wrote:
> 
> >  
> > +static int
> > +igbuio_pci_release(struct uio_info *info, struct inode *inode)
> > +{
> > +       int ret;
> > +       struct rte_uio_pci_dev *udev = info->priv;
> > +       struct pci_dev *dev = udev->pdev;
> > +       ret = __pci_reset_function(dev);
> > +       dev_info(&dev->dev, "pci_reset_function %s \n",
> > +                 ret == 0 ? "succeded" : "failed");
> > +       return 0;
> > +}
> > +
> > +
> 
> Patch in general looks ok, but:
>  * no Signed-off
>  * whitespace issues
>  * doesn't pass kernel coding style
>  * don't log on success, why log at all??
> 

Re: i40e igb_uio: reset pci on process exit

[Shijith Thotton]

On Fri, May 26, 2017 at 07:30:58AM +0300, Gregory Etelson wrote:

Hi Gregory,

The patch is useful for LiquidIO PMD as we can avoid VF FLR request to
PF. One comment inline..

[..]
> > >  
> > > +static int
> > > +igbuio_pci_release(struct uio_info *info, struct inode *inode)
> > > +{
> > > +       int ret;
> > > +       struct rte_uio_pci_dev *udev = info->priv;
> > > +       struct pci_dev *dev = udev->pdev;
> > > +       ret = __pci_reset_function(dev);

s/__pci_reset_function/pci_reset_function

> > > +       dev_info(&dev->dev, "pci_reset_function %s \n",
> > > +                 ret == 0 ? "succeded" : "failed");
> > > +       return 0;
> > > +}
[..]

Thanks,
Shijith

Re: i40e igb_uio: reset pci on process exit

[Gregory Etelson]

Thank you.

Regards,
Gregory

On Friday, 26 May 2017 09:05:11 IDT Shijith Thotton wrote:
> On Fri, May 26, 2017 at 07:30:58AM +0300, Gregory Etelson wrote:
> 
> Hi Gregory,
> 
> The patch is useful for LiquidIO PMD as we can avoid VF FLR request to
> PF. One comment inline..
> 
> [..]
> > > >  
> > > > +static int
> > > > +igbuio_pci_release(struct uio_info *info, struct inode *inode)
> > > > +{
> > > > +       int ret;
> > > > +       struct rte_uio_pci_dev *udev = info->priv;
> > > > +       struct pci_dev *dev = udev->pdev;
> > > > +       ret = __pci_reset_function(dev);
> 
> s/__pci_reset_function/pci_reset_function
> 
> > > > +       dev_info(&dev->dev, "pci_reset_function %s \n",
> > > > +                 ret == 0 ? "succeded" : "failed");
> > > > +       return 0;
> > > > +}
> [..]
> 
> Thanks,
> Shijith
> 

Re: i40e igb_uio: reset pci on process exit

[Stephen Hemminger]

On Fri, 26 May 2017 09:17:33 +0300
Gregory Etelson <gregory@weka.io> wrote:

> Thank you.
> 
> Regards,
> Gregory
> 
> On Friday, 26 May 2017 09:05:11 IDT Shijith Thotton wrote:
> > On Fri, May 26, 2017 at 07:30:58AM +0300, Gregory Etelson wrote:
> > 
> > Hi Gregory,
> > 
> > The patch is useful for LiquidIO PMD as we can avoid VF FLR request to
> > PF. One comment inline..
> > 
> > [..]
> > > > >  
> > > > > +static int
> > > > > +igbuio_pci_release(struct uio_info *info, struct inode *inode)
> > > > > +{
> > > > > +       int ret;
> > > > > +       struct rte_uio_pci_dev *udev = info->priv;
> > > > > +       struct pci_dev *dev = udev->pdev;
> > > > > +       ret = __pci_reset_function(dev);
> > 
> > s/__pci_reset_function/pci_reset_function
> > 
> > > > > +       dev_info(&dev->dev, "pci_reset_function %s \n",
> > > > > +                 ret == 0 ? "succeded" : "failed");
> > > > > +       return 0;
> > > > > +}
> > [..]
> > 
> > Thanks,
> > Shijith
> > 
> 

What does VFIO do?

It looks like in vfio case pci_enable is held off until open and pci_disable is done
on close. There are other things that may need to be done to make close work
correctly. Like turning of msix. Also reset may not always be possible.

Re: i40e igb_uio: reset pci on process exit

[Gregory Etelson]

I did not look into VFIO driver yet

Regards,
Gregory

On Friday, 26 May 2017 18:53:21 IDT Stephen Hemminger wrote:
> On Fri, 26 May 2017 09:17:33 +0300
> Gregory Etelson <gregory@weka.io> wrote:
> 
> > Thank you.
> > 
> > Regards,
> > Gregory
> > 
> > On Friday, 26 May 2017 09:05:11 IDT Shijith Thotton wrote:
> > > On Fri, May 26, 2017 at 07:30:58AM +0300, Gregory Etelson wrote:
> > > 
> > > Hi Gregory,
> > > 
> > > The patch is useful for LiquidIO PMD as we can avoid VF FLR request to
> > > PF. One comment inline..
> > > 
> > > [..]
> > > > > >  
> > > > > > +static int
> > > > > > +igbuio_pci_release(struct uio_info *info, struct inode *inode)
> > > > > > +{
> > > > > > +       int ret;
> > > > > > +       struct rte_uio_pci_dev *udev = info->priv;
> > > > > > +       struct pci_dev *dev = udev->pdev;
> > > > > > +       ret = __pci_reset_function(dev);
> > > 
> > > s/__pci_reset_function/pci_reset_function
> > > 
> > > > > > +       dev_info(&dev->dev, "pci_reset_function %s \n",
> > > > > > +                 ret == 0 ? "succeded" : "failed");
> > > > > > +       return 0;
> > > > > > +}
> > > [..]
> > > 
> > > Thanks,
> > > Shijith
> > > 
> > 
> 
> What does VFIO do?
> 
> It looks like in vfio case pci_enable is held off until open and pci_disable is done
> on close. There are other things that may need to be done to make close work
> correctly. Like turning of msix. Also reset may not always be possible.
> 

Re: i40e igb_uio: reset pci on process exit

[Shijith Thotton]

On Fri, May 26, 2017 at 07:14:55PM +0300, Gregory Etelson wrote:
>    I did not look into VFIO driver yet
> 
>     
> 
>    Regards,
> 
>    Gregory
> 
>     
> 
>    On Friday, 26 May 2017 18:53:21 IDT Stephen Hemminger wrote:
> 
>    > On Fri, 26 May 2017 09:17:33 +0300
> 
>    > Gregory Etelson <gregory@weka.io> wrote:
> 
>    >
> 
>    > > Thank you.
> 
>    > >
> 
>    > > Regards,
> 
>    > > Gregory
> 
>    > >
> 
>    > > On Friday, 26 May 2017 09:05:11 IDT Shijith Thotton wrote:
> 
>    > > > On Fri, May 26, 2017 at 07:30:58AM +0300, Gregory Etelson wrote:
> 
>    > > >
> 
>    > > > Hi Gregory,
> 
>    > > >
> 
>    > > > The patch is useful for LiquidIO PMD as we can avoid VF FLR request
>    to
> 
>    > > > PF. One comment inline..
> 
>    > > >
> 
>    > > > [..]
> 
>    > > > > > >
> 
>    > > > > > > +static int
> 
>    > > > > > > +igbuio_pci_release(struct uio_info *info, struct inode
>    *inode)
> 
>    > > > > > > +{
> 
>    > > > > > > + int ret;
> 
>    > > > > > > + struct rte_uio_pci_dev *udev = info->priv;
> 
>    > > > > > > + struct pci_dev *dev = udev->pdev;
> 
>    > > > > > > + ret = __pci_reset_function(dev);
> 
>    > > >
> 
>    > > > s/__pci_reset_function/pci_reset_function
> 
>    > > >
> 
>    > > > > > > + dev_info(&dev->dev, "pci_reset_function %s \n",
> 
>    > > > > > > + ret == 0 ? "succeded" : "failed");
> 
>    > > > > > > + return 0;
> 
>    > > > > > > +}
> 
>    > > > [..]
> 
>    > > >
> 
>    > > > Thanks,
> 
>    > > > Shijith
> 
>    > > >
> 
>    > >
> 
>    >
> 
>    > What does VFIO do?
> 
>    >
> 
>    > It looks like in vfio case pci_enable is held off until open and
>    pci_disable is done
> 
>    > on close. There are other things that may need to be done to make close
>    work
> 
>    > correctly. Like turning of msix. Also reset may not always be possible.
>

Better follow VFIO as Stephen advised. VFIO does pci reset inside open[1] and
tries to reset device during release[2]. 

1. elixir.free-electrons.com/linux/latest/source/drivers/vfio/pci/vfio_pci.c#L229
2. elixir.free-electrons.com/linux/latest/source/drivers/vfio/pci/vfio_pci.c#L361

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;

       return pci_reset_function(dev);
}

and.. 
	udev->info.open = igbuio_pci_open;

Re: i40e igb_uio: reset pci on process exit

[Gregory Etelson]

I still have to support Red Hat 6.x. These system do not have VFIO 
IGB_UIO is the only option there.

Also, there was a discussion that claimed IGB_UIO has better performance than VFIO.
http://dpdk.org/ml/archives/dev/2014-August/004609.html

Regards,
Gregory

 
On Monday, 29 May 2017 12:48:59 IDT Shijith Thotton wrote:
> On Fri, May 26, 2017 at 07:14:55PM +0300, Gregory Etelson wrote:
> >    I did not look into VFIO driver yet
> > 
> >     
> > 
> >    Regards,
> > 
> >    Gregory
> > 
> >     
> > 
> >    On Friday, 26 May 2017 18:53:21 IDT Stephen Hemminger wrote:
> > 
> >    > On Fri, 26 May 2017 09:17:33 +0300
> > 
> >    > Gregory Etelson <gregory@weka.io> wrote:
> > 
> >    >
> > 
> >    > > Thank you.
> > 
> >    > >
> > 
> >    > > Regards,
> > 
> >    > > Gregory
> > 
> >    > >
> > 
> >    > > On Friday, 26 May 2017 09:05:11 IDT Shijith Thotton wrote:
> > 
> >    > > > On Fri, May 26, 2017 at 07:30:58AM +0300, Gregory Etelson wrote:
> > 
> >    > > >
> > 
> >    > > > Hi Gregory,
> > 
> >    > > >
> > 
> >    > > > The patch is useful for LiquidIO PMD as we can avoid VF FLR request
> >    to
> > 
> >    > > > PF. One comment inline..
> > 
> >    > > >
> > 
> >    > > > [..]
> > 
> >    > > > > > >
> > 
> >    > > > > > > +static int
> > 
> >    > > > > > > +igbuio_pci_release(struct uio_info *info, struct inode
> >    *inode)
> > 
> >    > > > > > > +{
> > 
> >    > > > > > > + int ret;
> > 
> >    > > > > > > + struct rte_uio_pci_dev *udev = info->priv;
> > 
> >    > > > > > > + struct pci_dev *dev = udev->pdev;
> > 
> >    > > > > > > + ret = __pci_reset_function(dev);
> > 
> >    > > >
> > 
> >    > > > s/__pci_reset_function/pci_reset_function
> > 
> >    > > >
> > 
> >    > > > > > > + dev_info(&dev->dev, "pci_reset_function %s \n",
> > 
> >    > > > > > > + ret == 0 ? "succeded" : "failed");
> > 
> >    > > > > > > + return 0;
> > 
> >    > > > > > > +}
> > 
> >    > > > [..]
> > 
> >    > > >
> > 
> >    > > > Thanks,
> > 
> >    > > > Shijith
> > 
> >    > > >
> > 
> >    > >
> > 
> >    >
> > 
> >    > What does VFIO do?
> > 
> >    >
> > 
> >    > It looks like in vfio case pci_enable is held off until open and
> >    pci_disable is done
> > 
> >    > on close. There are other things that may need to be done to make close
> >    work
> > 
> >    > correctly. Like turning of msix. Also reset may not always be possible.
> >
> 
> Better follow VFIO as Stephen advised. VFIO does pci reset inside open[1] and
> tries to reset device during release[2]. 
> 
> 1. elixir.free-electrons.com/linux/latest/source/drivers/vfio/pci/vfio_pci.c#L229
> 2. elixir.free-electrons.com/linux/latest/source/drivers/vfio/pci/vfio_pci.c#L361
> 
> 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;
> 
>        return pci_reset_function(dev);
> }
> 
> and.. 
> 	udev->info.open = igbuio_pci_open;
> 
> 

Re: i40e igb_uio: reset pci on process exit

[Shijith Thotton]

On Mon, May 29, 2017 at 01:01:06PM +0300, Gregory Etelson wrote:
>    I still have to support Red Hat 6.x. These system do not have VFIO
> 
>    IGB_UIO is the only option there.
> 
>    Also, there was a discussion that claimed IGB_UIO has better performance
>    than VFIO.
> 
>    http://dpdk.org/ml/archives/dev/2014-August/004609.html
> 
>    Regards,
>    Gregory
> 

[..]
>> 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;
>>
>>	return pci_reset_function(dev);
>> }
>>
>> and..
>> udev->info.open = igbuio_pci_open;
>>

I was suggesting to make reset part of open. It should work on your setup.

-	udev->info.release = igbuio_pci_release;
+	udev->info.open = igbuio_pci_open;

Shijith

Re: i40e igb_uio: reset pci on process exit

[Gregory Etelson]

PMD already resets PCI during initialization.
In my patch, exiting process forced to release it's resources

On Monday, 29 May 2017 14:01:48 IDT Shijith Thotton wrote:
> On Mon, May 29, 2017 at 01:01:06PM +0300, Gregory Etelson wrote:
> >    I still have to support Red Hat 6.x. These system do not have VFIO
> > 
> >    IGB_UIO is the only option there.
> > 
> >    Also, there was a discussion that claimed IGB_UIO has better performance
> >    than VFIO.
> > 
> >    http://dpdk.org/ml/archives/dev/2014-August/004609.html
> > 
> >    Regards,
> >    Gregory
> > 
> 
> [..]
> >> 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;
> >>
> >>	return pci_reset_function(dev);
> >> }
> >>
> >> and..
> >> udev->info.open = igbuio_pci_open;
> >>
> 
> I was suggesting to make reset part of open. It should work on your setup.
> 
> -	udev->info.release = igbuio_pci_release;
> +	udev->info.open = igbuio_pci_open;
> 
> Shijith
> 

[RFC PATCH] igb_uio: issue FLR during open and release of device file

[Shijith Thotton]

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
change, it is not mandatory to issue FLR by PMD's during init and close.

Signed-off-by: Shijith Thotton <shijith.thotton@caviumnetworks.com>
---
 lib/librte_eal/linuxapp/igb_uio/igb_uio.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 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..5bc58d2 100644
--- a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
+++ b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
@@ -170,6 +170,34 @@ struct rte_uio_pci_dev {
 	return IRQ_HANDLED;
 }
 
+/**
+ * This gets called while opening uio device file. It clears any previous state
+ * associated with the pci device.
+ */
+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;
+
+	/* reset the pci device */
+	pci_reset_function(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;
+
+	/* try to reset the pci device */
+	pci_try_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 +400,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

Re: [RFC PATCH] igb_uio: issue FLR during open and release of device file

[Ferruh Yigit]

On 5/31/2017 12:09 PM, Shijith Thotton wrote:
> 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
> change, it is not mandatory to issue FLR by PMD's during init and close.

Cc: Jianfeng Tan <jianfeng.tan@intel.com>

Jianfeng also implemented following patch:
http://dpdk.org/dev/patchwork/patch/17495/

Which also implements release and open ops, for slightly different
reason (prevent DMA access after app exit), but mainly both are to
gracefully handle application exit status.

btw, for Jianfeng's case, can adding pci_clear_master() in release and
moving pci_set_master() to open help preventing unwanted DMA?


Gregory,

Can you please check if this patch fixes your issue?

Thanks,
ferruh

> 
> Signed-off-by: Shijith Thotton <shijith.thotton@caviumnetworks.com>
> ---
>  lib/librte_eal/linuxapp/igb_uio/igb_uio.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 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..5bc58d2 100644
> --- a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
> +++ b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
> @@ -170,6 +170,34 @@ struct rte_uio_pci_dev {
>  	return IRQ_HANDLED;
>  }
>  
> +/**
> + * This gets called while opening uio device file. It clears any previous state
> + * associated with the pci device.
> + */
> +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;
> +
> +	/* reset the pci device */
> +	pci_reset_function(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;
> +
> +	/* try to reset the pci device */
> +	pci_try_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 +400,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())
> 

Re: [RFC PATCH] igb_uio: issue FLR during open and release of device file

[Stephen Hemminger]

On Wed, 31 May 2017 13:20:08 +0100
Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 5/31/2017 12:09 PM, Shijith Thotton wrote:
> > 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
> > change, it is not mandatory to issue FLR by PMD's during init and close.  
> 
> Cc: Jianfeng Tan <jianfeng.tan@intel.com>
> 
> Jianfeng also implemented following patch:
> http://dpdk.org/dev/patchwork/patch/17495/
> 
> Which also implements release and open ops, for slightly different
> reason (prevent DMA access after app exit), but mainly both are to
> gracefully handle application exit status.
> 
> btw, for Jianfeng's case, can adding pci_clear_master() in release and
> moving pci_set_master() to open help preventing unwanted DMA?
> 
> 
> Gregory,
> 
> Can you please check if this patch fixes your issue?
> 
> Thanks,
> ferruh

pci_reset should stop all DMA. It also clears master status.

Re: [RFC PATCH] igb_uio: issue FLR during open and release of device file

[Ferruh Yigit]

On 5/31/2017 4:30 PM, Stephen Hemminger wrote:
> On Wed, 31 May 2017 13:20:08 +0100
> Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> 
>> On 5/31/2017 12:09 PM, Shijith Thotton wrote:
>>> 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
>>> change, it is not mandatory to issue FLR by PMD's during init and close.  
>>
>> Cc: Jianfeng Tan <jianfeng.tan@intel.com>
>>
>> Jianfeng also implemented following patch:
>> http://dpdk.org/dev/patchwork/patch/17495/
>>
>> Which also implements release and open ops, for slightly different
>> reason (prevent DMA access after app exit), but mainly both are to
>> gracefully handle application exit status.
>>
>> btw, for Jianfeng's case, can adding pci_clear_master() in release and
>> moving pci_set_master() to open help preventing unwanted DMA?
>>
>>
>> Gregory,
>>
>> Can you please check if this patch fixes your issue?
>>
>> Thanks,
>> ferruh
> 
> pci_reset should stop all DMA. It also clears master status.

If so, should open() call pci_set_master(), currently it has been only
called by igbuio_pci_probe() once?

Re: [RFC PATCH] igb_uio: issue FLR during open and release of device file

[Shijith Thotton]

On Wed, May 31, 2017 at 06:11:40PM +0100, Ferruh Yigit wrote:
> On 5/31/2017 4:30 PM, Stephen Hemminger wrote:
> > On Wed, 31 May 2017 13:20:08 +0100
> > Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> > 
> >> On 5/31/2017 12:09 PM, Shijith Thotton wrote:
> >>> 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
> >>> change, it is not mandatory to issue FLR by PMD's during init and close.  
> >>
> >> Cc: Jianfeng Tan <jianfeng.tan@intel.com>
> >>
> >> Jianfeng also implemented following patch:
> >> http://dpdk.org/dev/patchwork/patch/17495/
> >>
> >> Which also implements release and open ops, for slightly different
> >> reason (prevent DMA access after app exit), but mainly both are to
> >> gracefully handle application exit status.
> >>
> >> btw, for Jianfeng's case, can adding pci_clear_master() in release and
> >> moving pci_set_master() to open help preventing unwanted DMA?
> >>
> >>
> >> Gregory,
> >>
> >> Can you please check if this patch fixes your issue?
> >>
> >> Thanks,
> >> ferruh
> > 
> > pci_reset should stop all DMA. It also clears master status.

Per Alex Williamson[1], bus master will be disabled after FLR. But a disable is
preferred after reset, since all device won't behave as per spec.

1. http://www.spinics.net/lists/kvm/msg115715.html

> 
> If so, should open() call pci_set_master(), currently it has been only
> called by igbuio_pci_probe() once?

DPDK has pci_uio_set_bus_master/pci_vfio_set_bus_master to set bus master.
Can we leave it to the application ? vfio leaves the device with bus master
disabled after open.

Re: [RFC PATCH] igb_uio: issue FLR during open and release of device file

[Gregory Etelson]

On Wednesday, 31 May 2017 15:20:08 IDT Ferruh Yigit wrote:
> On 5/31/2017 12:09 PM, Shijith Thotton wrote:
> > 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
> > change, it is not mandatory to issue FLR by PMD's during init and close.
> 
> Cc: Jianfeng Tan <jianfeng.tan@intel.com>
> 
> Jianfeng also implemented following patch:
> http://dpdk.org/dev/patchwork/patch/17495/
> 
> Which also implements release and open ops, for slightly different
> reason (prevent DMA access after app exit), but mainly both are to
> gracefully handle application exit status.
> 
> btw, for Jianfeng's case, can adding pci_clear_master() in release and
> moving pci_set_master() to open help preventing unwanted DMA?
> 
> 
> Gregory,
> 
> Can you please check if this patch fixes your issue?
> 
> Thanks,
> ferruh

The tests are running.
I'll update you on completion.

Regards,
Gregory


> 
> > 
> > Signed-off-by: Shijith Thotton <shijith.thotton@caviumnetworks.com>
> > ---
> >  lib/librte_eal/linuxapp/igb_uio/igb_uio.c | 30 ++++++++++++++++++++++++++++++
> >  1 file changed, 30 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..5bc58d2 100644
> > --- a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
> > +++ b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
> > @@ -170,6 +170,34 @@ struct rte_uio_pci_dev {
> >  	return IRQ_HANDLED;
> >  }
> >  
> > +/**
> > + * This gets called while opening uio device file. It clears any previous state
> > + * associated with the pci device.
> > + */
> > +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;
> > +
> > +	/* reset the pci device */
> > +	pci_reset_function(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;
> > +
> > +	/* try to reset the pci device */
> > +	pci_try_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 +400,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())
> > 
> 
> 

Re: [RFC PATCH] igb_uio: issue FLR during open and release of device file

[Gregory Etelson]

In my environment I could reproduce server crash 
after applying the patch

Regards,
Gregory


On Wednesday, 31 May 2017 15:20:08 IDT Ferruh Yigit wrote:
> On 5/31/2017 12:09 PM, Shijith Thotton wrote:
> > 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
> > change, it is not mandatory to issue FLR by PMD's during init and close.
> 
> Cc: Jianfeng Tan <jianfeng.tan@intel.com>
> 
> Jianfeng also implemented following patch:
> http://dpdk.org/dev/patchwork/patch/17495/
> 
> Which also implements release and open ops, for slightly different
> reason (prevent DMA access after app exit), but mainly both are to
> gracefully handle application exit status.
> 
> btw, for Jianfeng's case, can adding pci_clear_master() in release and
> moving pci_set_master() to open help preventing unwanted DMA?
> 
> 
> Gregory,
> 
> Can you please check if this patch fixes your issue?
> 
> Thanks,
> ferruh
> 
> > 
> > Signed-off-by: Shijith Thotton <shijith.thotton@caviumnetworks.com>
> > ---
> >  lib/librte_eal/linuxapp/igb_uio/igb_uio.c | 30 ++++++++++++++++++++++++++++++
> >  1 file changed, 30 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..5bc58d2 100644
> > --- a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
> > +++ b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
> > @@ -170,6 +170,34 @@ struct rte_uio_pci_dev {
> >  	return IRQ_HANDLED;
> >  }
> >  
> > +/**
> > + * This gets called while opening uio device file. It clears any previous state
> > + * associated with the pci device.
> > + */
> > +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;
> > +
> > +	/* reset the pci device */
> > +	pci_reset_function(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;
> > +
> > +	/* try to reset the pci device */
> > +	pci_try_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 +400,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())
> > 
> 
> 

Re: [RFC PATCH] igb_uio: issue FLR during open and release of device file

[Tan, Jianfeng]

Hi Gregory,


On 6/4/2017 3:22 PM, Gregory Etelson wrote:
>
> In my environment I could reproduce server crash
>
> after applying the patch
>

Different from your patch, my patch calls 
pci_enable_device()/pci_disable_device() instead of 
pci_reset_function()/pci_try_reset_function() separately in open() and 
release(). Could you check if that's the reason?

vfio_pci actually calls both pci_try_reset_function() and 
pci_disable_device() in release().

Thanks,
Jianfeng

> Regards,
>
> Gregory
>
> On Wednesday, 31 May 2017 15:20:08 IDT Ferruh Yigit wrote:
>
> > On 5/31/2017 12:09 PM, Shijith Thotton wrote:
>
> > > 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
>
> > > change, it is not mandatory to issue FLR by PMD's during init and 
> close.
>
> >
>
> > Cc: Jianfeng Tan <jianfeng.tan@intel.com>
>
> >
>
> > Jianfeng also implemented following patch:
>
> > http://dpdk.org/dev/patchwork/patch/17495/
>
> >
>
> > Which also implements release and open ops, for slightly different
>
> > reason (prevent DMA access after app exit), but mainly both are to
>
> > gracefully handle application exit status.
>
> >
>
> > btw, for Jianfeng's case, can adding pci_clear_master() in release and
>
> > moving pci_set_master() to open help preventing unwanted DMA?
>
> >
>
> >
>
> > Gregory,
>
> >
>
> > Can you please check if this patch fixes your issue?
>
> >
>
> > Thanks,
>
> > ferruh
>
> >
>
> > >
>
> > > Signed-off-by: Shijith Thotton <shijith.thotton@caviumnetworks.com>
>
> > > ---
>
> > > lib/librte_eal/linuxapp/igb_uio/igb_uio.c | 30 
> ++++++++++++++++++++++++++++++
>
> > > 1 file changed, 30 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..5bc58d2 100644
>
> > > --- a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
>
> > > +++ b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
>
> > > @@ -170,6 +170,34 @@ struct rte_uio_pci_dev {
>
> > > return IRQ_HANDLED;
>
> > > }
>
> > >
>
> > > +/**
>
> > > + * This gets called while opening uio device file. It clears any 
> previous state
>
> > > + * associated with the pci device.
>
> > > + */
>
> > > +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;
>
> > > +
>
> > > + /* reset the pci device */
>
> > > + pci_reset_function(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;
>
> > > +
>
> > > + /* try to reset the pci device */
>
> > > + pci_try_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 +400,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())
>
> > >
>
> >
>
> >
>

Re: [RFC PATCH] igb_uio: issue FLR during open and release of device file

[Gregory Etelson]

Hello Jianfeng,

I was experimenting with pci_disable_device() in release
Tests running on Intel 82599 VF crashed server after process down,
before a new instance was started.
Tests running on Intel XL710 VF did not crash server, but could not send / receive Ethernet frames
although all DPDK initialization routines completed without errors

Regards,
Gregory



    
Hi Gregory,        
On 6/4/2017 3:22 PM, Gregory Etelson      wrote:        
                        
In my environment I could reproduce server crash       
after applying the patch            Different from your patch, my patch calls    pci_enable_device()/pci_disable_device() instead of    pci_reset_function()/pci_try_reset_function() separately in open()    and release(). Could you check if that's the reason?        vfio_pci actually calls both pci_try_reset_function() and    pci_disable_device() in release().        Thanks,    Jianfeng        
      
      
Regards,      
Gregory      
      
      
On Wednesday, 31 May 2017 15:20:08 IDT Ferruh Yigit wrote:      
> On 5/31/2017 12:09 PM, Shijith Thotton wrote:      
> > 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      
> > change, it is not mandatory to issue FLR by PMD's during init and close.      
>       
> Cc: Jianfeng Tan <jianfeng.tan@intel.com>[1]      
>       
> Jianfeng also implemented following patch:      
> http://dpdk.org/dev/patchwork/patch/17495/[2]      
>       
> Which also implements release and open ops, for slightly different      
> reason (prevent DMA access after app exit), but mainly both are to      
> gracefully handle application exit status.      
>       
> btw, for Jianfeng's case, can adding pci_clear_master() in release and      
> moving pci_set_master() to open help preventing unwanted DMA?      
>       
>       
> Gregory,      
>       
> Can you please check if this patch fixes your issue?      
>       
> Thanks,      
> ferruh      
>       
> >       
> > Signed-off-by: Shijith Thotton <shijith.thotton@caviumnetworks.com>[3]      
> > ---      
> >  lib/librte_eal/linuxapp/igb_uio/igb_uio.c | 30 ++++++++++++++++++++++++++++++      
> >  1 file changed, 30 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..5bc58d2 100644      
> > --- a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c      
> > +++ b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c      
> > @@ -170,6 +170,34 @@ struct rte_uio_pci_dev {      
> >  	return IRQ_HANDLED;      
> >  }      
> >        
> > +/**      
> > + * This gets called while opening uio device file. It clears any previous state      
> > + * associated with the pci device.      
> > + */      
> > +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;      
> > +      
> > +	/* reset the pci device */      
> > +	pci_reset_function(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;      
> > +      
> > +	/* try to reset the pci device */      
> > +	pci_try_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 +400,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] mailto:jianfeng.tan@intel.com
[2] http://dpdk.org/dev/patchwork/patch/17495/
[3] mailto:shijith.thotton@caviumnetworks.com

Re: [RFC PATCH] igb_uio: issue FLR during open and release of device file

[Stephen Hemminger]

On Wed, 31 May 2017 16:39:26 +0530
Shijith Thotton <shijith.thotton@caviumnetworks.com> wrote:

> +	/* reset the pci device */
> +	pci_reset_function(dev);

It is not considered best practice to comment the obvious.
This comment borders on being the classic /* add one to i */

[PATCH] igb_uio: issue FLR during open and release of device file

[Shijith Thotton]

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.

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>
---
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..7c04bb9 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_try_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

Re: [PATCH] igb_uio: issue FLR during open and release of device file

[Thomas Monjalon]

Any comment, please?

12/06/2017 11:38, Shijith Thotton:
> 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.
> 
> 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>
> ---
> 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

Re: [PATCH] igb_uio: issue FLR during open and release of device file

[Ferruh Yigit]

On 6/12/2017 10:38 AM, Shijith Thotton wrote:
> 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.
> 
> 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>

This patch, and Gregory's patch [1] are very similar and main target is
to leave device in a more proper state when DPDK application quits
unexpectedly.

Difference between two are, this one implements both .open and .release
ops, and sets / clears bus master accordingly.

Although main concern is .reset, I am OK to follow vfio_pci approach
here, and clearing bus master on .reset can prevent unwanted DMA access.

So, I am for this patch and I am testing it for a few days without a
problem.

But Gregory reported a crash with older version of this patch, without
more detail, we should clear that first. With Gregory's Tested-by, I am
OK with this patch.


Gregory,

Are you using your version, what are the results? And would you mind
testing this patch?

Thanks,
ferruh


[1]
http://dpdk.org/dev/patchwork/patch/25061/

Re: [PATCH] igb_uio: issue FLR during open and release of device file

[Gregory Etelson]

I could not reproduce server crash with http://dpdk.org/dev/patchwork/patch/25267/ [1]
However, pci_try_reset_function() API used in that patch is not defined in RedHat-6.x Linux-2.6.32 kernels 
Therefore I work with http://dpdk.org/dev/patchwork/patch/25061/  patch [2].
[2] was successfully tested with IXGBE & I40e VFs on RH 6.x, RH 7.x Ubuntu 14.04 and SLES-11.4 
 
Regards,
Gregory

On Thursday, 6 July 2017 19:41:40 IDT Ferruh Yigit wrote:
> On 6/12/2017 10:38 AM, Shijith Thotton wrote:
> > 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.
> > 
> > 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>
> 
> This patch, and Gregory's patch [1] are very similar and main target is
> to leave device in a more proper state when DPDK application quits
> unexpectedly.
> 
> Difference between two are, this one implements both .open and .release
> ops, and sets / clears bus master accordingly.
> 
> Although main concern is .reset, I am OK to follow vfio_pci approach
> here, and clearing bus master on .reset can prevent unwanted DMA access.
> 
> So, I am for this patch and I am testing it for a few days without a
> problem.
> 
> But Gregory reported a crash with older version of this patch, without
> more detail, we should clear that first. With Gregory's Tested-by, I am
> OK with this patch.
> 
> 
> Gregory,
> 
> Are you using your version, what are the results? And would you mind
> testing this patch?
> 
> Thanks,
> ferruh
> 
> 
> [1]
> http://dpdk.org/dev/patchwork/patch/25061/
> 
> 

Re: [PATCH] igb_uio: issue FLR during open and release of device file

[Shijith Thotton]

On Thu, Jul 06, 2017 at 08:27:17PM +0300, Gregory Etelson wrote:
> I could not reproduce server crash with http://dpdk.org/dev/patchwork/patch/25267/ [1]
> However, pci_try_reset_function() API used in that patch is not defined in RedHat-6.x Linux-2.6.32 kernels 
> Therefore I work with http://dpdk.org/dev/patchwork/patch/25061/  patch [2].
> [2] was successfully tested with IXGBE & I40e VFs on RH 6.x, RH 7.x Ubuntu 14.04 and SLES-11.4 
>  
> Regards,
> Gregory
> 
> On Thursday, 6 July 2017 19:41:40 IDT Ferruh Yigit wrote:
> > On 6/12/2017 10:38 AM, Shijith Thotton wrote:
> > > 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.
> > > 
> > > 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>
> > 
> > This patch, and Gregory's patch [1] are very similar and main target is
> > to leave device in a more proper state when DPDK application quits
> > unexpectedly.
> > 
> > Difference between two are, this one implements both .open and .release
> > ops, and sets / clears bus master accordingly.
> > 
> > Although main concern is .reset, I am OK to follow vfio_pci approach
> > here, and clearing bus master on .reset can prevent unwanted DMA access.
> > 
> > So, I am for this patch and I am testing it for a few days without a
> > problem.
> > 
> > But Gregory reported a crash with older version of this patch, without
> > more detail, we should clear that first. With Gregory's Tested-by, I am
> > OK with this patch.
> > 
> > 
> > Gregory,
> > 
> > Are you using your version, what are the results? And would you mind
> > testing this patch?
> > 
> > Thanks,
> > ferruh
> > 
> > 
> > [1]
> > http://dpdk.org/dev/patchwork/patch/25061/
> > 
> > 

Hi Gregory,

Please try the following change:
s/pci_try_reset_function/pci_reset_function/

pci_try_reset_function is same as pci_reset_function, except it returns -EAGAIN
if unable to lock the device[1].

If everyone agrees, I can submit v2 with this change.

1. http://elixir.free-electrons.com/linux/latest/source/drivers/pci/pci.c#L4293

Thanks,
Shijith

Re: [PATCH] igb_uio: issue FLR during open and release of device file

[Ferruh Yigit]

On 7/7/2017 11:03 AM, Shijith Thotton wrote:
> On Thu, Jul 06, 2017 at 08:27:17PM +0300, Gregory Etelson wrote:
>> I could not reproduce server crash with http://dpdk.org/dev/patchwork/patch/25267/ [1]
>> However, pci_try_reset_function() API used in that patch is not defined in RedHat-6.x Linux-2.6.32 kernels 
>> Therefore I work with http://dpdk.org/dev/patchwork/patch/25061/  patch [2].
>> [2] was successfully tested with IXGBE & I40e VFs on RH 6.x, RH 7.x Ubuntu 14.04 and SLES-11.4 
>>  
>> Regards,
>> Gregory
>>
>> On Thursday, 6 July 2017 19:41:40 IDT Ferruh Yigit wrote:
>>> On 6/12/2017 10:38 AM, Shijith Thotton wrote:
>>>> 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.
>>>>
>>>> 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>
>>>
>>> This patch, and Gregory's patch [1] are very similar and main target is
>>> to leave device in a more proper state when DPDK application quits
>>> unexpectedly.
>>>
>>> Difference between two are, this one implements both .open and .release
>>> ops, and sets / clears bus master accordingly.
>>>
>>> Although main concern is .reset, I am OK to follow vfio_pci approach
>>> here, and clearing bus master on .reset can prevent unwanted DMA access.
>>>
>>> So, I am for this patch and I am testing it for a few days without a
>>> problem.
>>>
>>> But Gregory reported a crash with older version of this patch, without
>>> more detail, we should clear that first. With Gregory's Tested-by, I am
>>> OK with this patch.
>>>
>>>
>>> Gregory,
>>>
>>> Are you using your version, what are the results? And would you mind
>>> testing this patch?
>>>
>>> Thanks,
>>> ferruh
>>>
>>>
>>> [1]
>>> http://dpdk.org/dev/patchwork/patch/25061/
>>>
>>>
> 
> Hi Gregory,
> 
> Please try the following change:
> s/pci_try_reset_function/pci_reset_function/
> 
> pci_try_reset_function is same as pci_reset_function, except it returns -EAGAIN
> if unable to lock the device[1].
> 
> If everyone agrees, I can submit v2 with this change.

pci_try_reset_function() not being available in older kernel versions
seems a problem and blocking Gregory.

To move forward, I would suggest sending the v2, and we can continue
discussion based on it.

Thanks,
ferruh

> 
> 1. http://elixir.free-electrons.com/linux/latest/source/drivers/pci/pci.c#L4293
> 
> Thanks,
> Shijith
> 

[PATCH v2] igb_uio: issue FLR during open and release of device file

[Shijith Thotton]

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.

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

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

[Ferruh Yigit]

On 7/7/2017 12:13 PM, Shijith Thotton wrote:
> 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.
> 
> 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>

Gregory,

Would you mind testing this one?

Thanks,
ferruh

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

[Gregory Etelson]

Hello Ferruh,

I could not reproduce server crash with the patch.
However, some tests report ixgbe_vf_pmd and i40e_vf_pmd 
do not receive and transmit frames after process restart,
although PMD initialization completed successfully
Is there a way to collect PF firmware dump for investigation ?

Regards,
Gregory

On Friday, 7 July 2017 18:10:40 IDT Ferruh Yigit wrote:
> On 7/7/2017 12:13 PM, Shijith Thotton wrote:
> > 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.
> > 
> > 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>
> 
> Gregory,
> 
> Would you mind testing this one?
> 
> Thanks,
> ferruh
> 
> 

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

[Gregory Etelson]

Hello Ferruh,

Both patches
[1] http://dpdk.org/dev/patchwork/patch/26633/ and 
[2] http://dpdk.org/dev/patchwork/patch/25061/ have failed the same test.
This is kind of strange because [2] has already passed that test numerous times.
I'll recalibrate my cluster and run the test again.
Besides that, [1] does the job

Regards,
Gregory

On Monday, 10 July 2017 06:07:45 IDT Gregory Etelson wrote:


Hello Ferruh,

I could not reproduce server crash with the patch.
However, some tests report ixgbe_vf_pmd and i40e_vf_pmd 
do not receive and transmit frames after process restart,
although PMD initialization completed successfully
Is there a way to collect PF firmware dump for investigation ?

Regards,
Gregory

On Friday, 7 July 2017 18:10:40 IDT Ferruh Yigit wrote:
> On 7/7/2017 12:13 PM, Shijith Thotton wrote:
> > 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.
> > 
> > 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>
> 
> Gregory,
> 
> Would you mind testing this one?
> 
> Thanks,
> ferruh
> 
> 

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

[Gregory Etelson]

Hello Ferruh,

All tests have passed successfully.


Regards,
Gregory


Hello Ferruh,

Both patches
[1] http://dpdk.org/dev/patchwork/patch/26633/ and 
[2] http://dpdk.org/dev/patchwork/patch/25061/ have failed the same test.
This is kind of strange because [2] has already passed that test numerous times.
I'll recalibrate my cluster and run the test again.
Besides that, [1] does the job

Regards,
Gregory

On Monday, 10 July 2017 06:07:45 IDT Gregory Etelson wrote:


Hello Ferruh,

I could not reproduce server crash with the patch.
However, some tests report ixgbe_vf_pmd and i40e_vf_pmd 
do not receive and transmit frames after process restart,
although PMD initialization completed successfully
Is there a way to collect PF firmware dump for investigation ?

Regards,
Gregory

On Friday, 7 July 2017 18:10:40 IDT Ferruh Yigit wrote:
> On 7/7/2017 12:13 PM, Shijith Thotton wrote:
> > 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.
> > 
> > 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>
> 
> Gregory,
> 
> Would you mind testing this one?
> 
> Thanks,
> ferruh
> 
> 

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

[Tan, Jianfeng]

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

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

[Shijith Thotton]

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.

Thanks,
Shijith

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

[Tan, Jianfeng]

> -----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?

Thanks,
Jianfeng

> 
> Thanks,
> Shijith

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

[Shijith Thotton]

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.
Please correct if I assumed it wrong.

Shijith

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

[Tan, Jianfeng]

> -----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

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

[Tan, Jianfeng]

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

Reviewed-by: Jianfeng Tan <jianfeng.tan@intel.com>

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

[Gregory Etelson]

Hello Shijith,

Please add the patch to uio_pci_generic.c file in Linux kernel
We experience similar faults with NVMe devices  

On Wednesday, 12 July 2017 06:40:55 IDT Tan, Jianfeng wrote:
> 
> > -----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.
> > 
> > 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>
> 
> Reviewed-by: Jianfeng Tan <jianfeng.tan@intel.com>

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

[Gregory Etelson]

[-- Attachment #1: Type: text/plain, Size: 1127 bytes --]

Hello Shijith,

Please add the patch to uio_pci_generic.c file in Linux kernel
We experience similar faults with NVMe devices  

On Wednesday, 12 July 2017 06:40:55 IDT Tan, Jianfeng wrote:
> 
> > -----Original Message-----
> > From: Shijith Thotton [mailto:shijith.thotton(a)caviumnetworks.com]
> > Sent: Friday, July 7, 2017 7:14 PM
> > To: dev(a)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.
> > 
> > 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(a)caviumnetworks.com>
> 
> Reviewed-by: Jianfeng Tan <jianfeng.tan(a)intel.com>


[-- Attachment #2: attachment.html --]
[-- Type: text/html, Size: 5386 bytes --]

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

[Ferruh Yigit]

On 7/12/2017 4:40 AM, Tan, Jianfeng wrote:
> 
> 
>> -----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.
>>
>> 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>
> 
> Reviewed-by: Jianfeng Tan <jianfeng.tan@intel.com>

Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>

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

[Gregory Etelson]

On Wednesday, 19 July 2017 16:32:34 IDT Ferruh Yigit wrote:
> On 7/12/2017 4:40 AM, Tan, Jianfeng wrote:
> > 
> > 
> >> -----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.
> >>
> >> 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>
> > 
> > Reviewed-by: Jianfeng Tan <jianfeng.tan@intel.com>
> 
> Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>
> 

Acked-by: Gregory Etelson <gregory@weka.io>

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

[Thomas Monjalon]

> > >> 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.
> > >>
> > >> 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>
> > > 
> > > Reviewed-by: Jianfeng Tan <jianfeng.tan@intel.com>
> > 
> > Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>
> 
> Acked-by: Gregory Etelson <gregory@weka.io>

Applied, thanks