UIO pci-generic support broke igb_uio

PATCH New Stephen Hemminger 2 participants 6 messages Apr 15, 2015 1 message not in the archive

Subsystems (2)

Userspace I/O (UIO) Generic UIO driver for PCI devices

lore

Share & tools

lore

Stephen Hemminger

author Acked-by Declan Doherty

2015-04-15 01:06 UTC #0 lore raw

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 <[email protected]>
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 <[email protected]>

Signed-off-by Bruce Richardson <[email protected]>

Acked-by Declan Doherty <[email protected]>

Zhou, Danny

2015-04-15 07:19 UTC #1 lore raw

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.

Quoted 39 lines

-----Original Message-----
From: Stephen Hemminger [mailto:[email protected]]
Sent: Wednesday, April 15, 2015 9:06 AM
To: Zhou, Danny; Richardson, Bruce; Doherty, Declan
Cc: [email protected]
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 <[email protected]>
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 <[email protected]>
Signed-off-by Bruce Richardson <[email protected]>
Acked-by Declan Doherty <[email protected]>

A message that is not in the archive (sent off-list or not yet received): DFDF335405C17848924A094BC35766CF0AB5C943-0J0gbvR4kTg/[email protected]

Stephen Hemminger

author

2015-04-15 15:57 UTC #3 lore raw

Quoted 7 lines · 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.

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.

Stephen Hemminger

author

2015-04-15 16:34 UTC #4 lore raw

cover fix UIO support broken by 2.0

In reply to a message not in the archive

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.

Stephen Hemminger

author

2015-04-15 16:36 UTC #5 lore raw

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

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 <[email protected]>

---
Patch against latest 4.0 upstream kernel

drivers/uio/uio_pci_generic.c +13 -0

@@ -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);

Stephen Hemminger

author

2015-04-15 16:38 UTC #6 lore raw

PATCH 2/2 uio: fix pci generic driver breakage

In reply to 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 <[email protected]>

lib/librte_eal/linuxapp/eal/eal_interrupts.c +10 ~~-30~~

@@ -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){

lib/librte_eal/linuxapp/eal/eal_pci_uio.c +0 -1

@@ -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 */

lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h +1 -2

@@ -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 */