UIO pci-generic support broke igb_uio

UIO pci-generic support broke igb_uio

[Stephen Hemminger]

The addition of uio pci-generic broke use if igb_uio because
the wrong file descriptor is being used.

If I was a hard ass I would recommend uio pci-generic support
be reverted from 2.0 until/unless this fixed.

Failure mode is on startup:

EAL:  Error reading interrupts status for fd 0
PANIC in start_port()
rte_eth-dev_start: port=0 err=-5

The problem commit is:
commit 4a499c64959074ba6fa6a5a2b3a2a6aa10627fa1
Author: Danny Zhou <danny.zhou-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Date:   Fri Feb 20 16:59:15 2015 +0000

    eal/linux: enable uio_pci_generic support
    
    Change the EAL PCI code so that it can work with both the
    uio_pci_generic in-tree driver, as well as the igb_uio
    DPDK-specific driver.
    
    This involves changes to
    1) Modify method of retrieving BAR resource mapping information
    2) Mapping using resource files in /sys rather than /dev/uio*
    2) Setup bus master bit in NIC's PCIe configuration space for
    uio_pci_generic.
    
    Signed-off-by: Danny Zhou <danny.zhou-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
    Signed-off-by: Bruce Richardson <bruce.richardson-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
    Acked-by: Declan Doherty <declan.doherty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Re: UIO pci-generic support broke igb_uio

[Zhou, Danny]

Could you please send out the steps for us to reproduce it? I guess you have applied
v6 interrupt patches to perform interrupt related tests, right?

We cannot reproduce it now.

The support to in_kernel uio_pci_generic avoids using igb_uio this DPDK specific kernel module, 
so it will be much easier for any Linux distribution to package DPDK.

> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen-OTpzqLSitTUnbdJkjeBofR2eb7JE58TQ@public.gmane.org]
> Sent: Wednesday, April 15, 2015 9:06 AM
> To: Zhou, Danny; Richardson, Bruce; Doherty, Declan
> Cc: dev-VfR2kkLFssw@public.gmane.org
> Subject: UIO pci-generic support broke igb_uio
> 
> The addition of uio pci-generic broke use if igb_uio because
> the wrong file descriptor is being used.
> 
> If I was a hard ass I would recommend uio pci-generic support
> be reverted from 2.0 until/unless this fixed.
> 
> Failure mode is on startup:
> 
> EAL:  Error reading interrupts status for fd 0
> PANIC in start_port()
> rte_eth-dev_start: port=0 err=-5
> 
> The problem commit is:
> commit 4a499c64959074ba6fa6a5a2b3a2a6aa10627fa1
> Author: Danny Zhou <danny.zhou-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Date:   Fri Feb 20 16:59:15 2015 +0000
> 
>     eal/linux: enable uio_pci_generic support
> 
>     Change the EAL PCI code so that it can work with both the
>     uio_pci_generic in-tree driver, as well as the igb_uio
>     DPDK-specific driver.
> 
>     This involves changes to
>     1) Modify method of retrieving BAR resource mapping information
>     2) Mapping using resource files in /sys rather than /dev/uio*
>     2) Setup bus master bit in NIC's PCIe configuration space for
>     uio_pci_generic.
> 
>     Signed-off-by: Danny Zhou <danny.zhou-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>     Signed-off-by: Bruce Richardson <bruce.richardson-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>     Acked-by: Declan Doherty <declan.doherty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Re: UIO pci-generic support broke igb_uio

[Stephen Hemminger]

On Wed, 15 Apr 2015 07:19:39 +0000
"Zhou, Danny" <danny.zhou-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:

> Could you please send out the steps for us to reproduce it? I guess you have applied
> v6 interrupt patches to perform interrupt related tests, right?
> 
> We cannot reproduce it now.
> 
> The support to in_kernel uio_pci_generic avoids using igb_uio this DPDK specific kernel module, 
> so it will be much easier for any Linux distribution to package DPDK.


Very simple. Run with link state enabled and igb_uio driver.

Looking more closely, I think the right way to fix this is to fix the
upstream uio_pci_generic to support controlling interrupts properly, rather
than trying to do it directly in userspace.

The problem is that doing it userspace is inherently racy because
the userspace config access does not do proper locking.

It really is an upstream kernel bug. Will send patch upstream to do it
the right way. But that won't help distributions.

[PATCH 0/2] fix UIO support broken by 2.0

[Stephen Hemminger]

Here is the correct way to fix it (even if it maybe hard to accept).
I think the most technically correct way to handle this is to fix
UIO driver in kernel to do the IRQ management according to the
API that was already described in the kernel documentation.
Then have the DPDK EAL use the IRQ management API.

This maybe hard for Enterprise distributions to accept, but it is
the right technical solution.

[PATCH 1/2 kernel] uio: add irq control support to uio_pci_generic

[Stephen Hemminger]

The driver already supported INTX interrupts but had no in kernel
function to enable and disable them.

It is possible for userspace to do this by accessing PCI config
directly, but this racy and better handled by same mechanism
that already exists in kernel.

Signed-off-by: Stephen Hemminger <stephen-OTpzqLSitTUnbdJkjeBofR2eb7JE58TQ@public.gmane.org>

---
Patch against latest 4.0 upstream kernel

--- a/drivers/uio/uio_pci_generic.c	2015-04-15 08:50:15.543900681 -0700
+++ b/drivers/uio/uio_pci_generic.c	2015-04-15 09:00:01.658609786 -0700
@@ -53,6 +53,18 @@ static irqreturn_t irqhandler(int irq, s
 	return IRQ_HANDLED;
 }
 
+static int irqcontrol(struct uio_info *info, s32 irq_on)
+{
+	struct uio_pci_generic_dev *gdev = to_uio_pci_generic_dev(info);
+	struct pci_dev *pdev = gdev->pdev;
+
+	pci_cfg_access_lock(pdev);
+	pci_intx(pdev, irq_on);
+	pci_cfg_access_unlock(pdev);
+
+	return 0;
+}
+
 static int probe(struct pci_dev *pdev,
 			   const struct pci_device_id *id)
 {
@@ -89,6 +101,7 @@ static int probe(struct pci_dev *pdev,
 	gdev->info.irq = pdev->irq;
 	gdev->info.irq_flags = IRQF_SHARED;
 	gdev->info.handler = irqhandler;
+	gdev->info.irqcontrol = irqcontrol;
 	gdev->pdev = pdev;
 
 	err = uio_register_device(&pdev->dev, &gdev->info);

[PATCH 2/2 dpdk] uio: fix pci generic driver breakage

[Stephen Hemminger]

The integration of using uio_pci_generic broke use of UIO
by igb_uio and other drivers. Go back to using uio IRQ control
through interrupt fd.

This patch assumes that if uio_pci_generic is being used
that the kernel has been fixed to support this
by integrating patch to support IRQ control.

Signed-off-by: Stephen Hemminger <stephen-OTpzqLSitTUnbdJkjeBofR2eb7JE58TQ@public.gmane.org>

--- a/lib/librte_eal/linuxapp/eal/eal_interrupts.c
+++ b/lib/librte_eal/linuxapp/eal/eal_interrupts.c
@@ -363,48 +363,28 @@ vfio_disable_msix(struct rte_intr_handle
 static int
 uio_intr_disable(struct rte_intr_handle *intr_handle)
 {
-	unsigned char command_high;
+	const int value = 0;
 
-	/* use UIO config file descriptor for uio_pci_generic */
-	if (pread(intr_handle->uio_cfg_fd, &command_high, 1, 5) != 1) {
+	if (write(intr_handle->fd, &value, sizeof(value)) < 0){
 		RTE_LOG(ERR, EAL,
-			"Error reading interrupts status for fd %d\n",
-			intr_handle->uio_cfg_fd);
+			"Error disabling interrupts for fd %d (%s)\n",
+			intr_handle->fd, strerror(errno));
 		return -1;
 	}
-	/* disable interrupts */
-	command_high |= 0x4;
-	if (pwrite(intr_handle->uio_cfg_fd, &command_high, 1, 5) != 1) {
-		RTE_LOG(ERR, EAL,
-			"Error disabling interrupts for fd %d\n",
-			intr_handle->uio_cfg_fd);
-		return -1;
-	}
-
 	return 0;
 }
 
 static int
 uio_intr_enable(struct rte_intr_handle *intr_handle)
 {
-	unsigned char command_high;
+	const int value = 1;
 
-	/* use UIO config file descriptor for uio_pci_generic */
-	if (pread(intr_handle->uio_cfg_fd, &command_high, 1, 5) != 1) {
+	if (write(intr_handle->fd, &value, sizeof(value)) < 0) {
 		RTE_LOG(ERR, EAL,
-			"Error reading interrupts status for fd %d\n",
-			intr_handle->uio_cfg_fd);
+			"Error enabling interrupts for fd %d (%s)\n",
+			intr_handle->fd, strerror(errno));
 		return -1;
 	}
-	/* enable interrupts */
-	command_high &= ~0x4;
-	if (pwrite(intr_handle->uio_cfg_fd, &command_high, 1, 5) != 1) {
-		RTE_LOG(ERR, EAL,
-			"Error enabling interrupts for fd %d\n",
-			intr_handle->uio_cfg_fd);
-		return -1;
-	}
-
 	return 0;
 }
 
@@ -547,7 +527,7 @@ rte_intr_callback_unregister(struct rte_
 int
 rte_intr_enable(struct rte_intr_handle *intr_handle)
 {
-	if (!intr_handle || intr_handle->fd < 0 || intr_handle->uio_cfg_fd < 0)
+	if (!intr_handle || intr_handle->fd < 0)
 		return -1;
 
 	switch (intr_handle->type){
@@ -587,7 +567,7 @@ rte_intr_enable(struct rte_intr_handle *
 int
 rte_intr_disable(struct rte_intr_handle *intr_handle)
 {
-	if (!intr_handle || intr_handle->fd < 0 || intr_handle->uio_cfg_fd < 0)
+	if (!intr_handle || intr_handle->fd < 0)
 		return -1;
 
 	switch (intr_handle->type){
--- a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
@@ -276,7 +276,6 @@ pci_uio_map_resource(struct rte_pci_devi
 	struct pci_map *maps;
 
 	dev->intr_handle.fd = -1;
-	dev->intr_handle.uio_cfg_fd = -1;
 	dev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN;
 
 	/* secondary processes - use already recorded details */
--- a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h
+++ b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h
@@ -52,8 +52,7 @@ enum rte_intr_handle_type {
 struct rte_intr_handle {
 	union {
 		int vfio_dev_fd;  /**< VFIO device file descriptor */
-		int uio_cfg_fd;  /**< UIO config file descriptor
-					for uio_pci_generic */
+		int uio_cfg_fd;   /**< UIO config file descriptor */
 	};
 	int fd;	 /**< interrupt event file descriptor */
 	enum rte_intr_handle_type type;  /**< handle type */