diff mbox

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

Message ID 1497260285-27536-1-git-send-email-shijith.thotton@caviumnetworks.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Shijith Thotton June 12, 2017, 9:38 a.m. UTC 
  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(+)

Comments

Thomas Monjalon July 5, 2017, 11:42 p.m. UTC | #1 
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
Ferruh Yigit July 6, 2017, 4:41 p.m. UTC | #2 
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/
Gregory Etelson July 6, 2017, 5:27 p.m. UTC | #3 
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/
> 
>
Shijith Thotton July 7, 2017, 10:03 a.m. UTC | #4 
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
Ferruh Yigit July 7, 2017, 10:16 a.m. UTC | #5 
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
>
diff mbox

Patch

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