linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2 v3] Add new uio device for PCI with dynamic memory allocation
@ 2017-10-06 13:31 Stahl, Manuel
  2017-10-06 13:33 ` [PATCH 2/2] uio: Prefer MSI(X) interrupts in PCI drivers Stahl, Manuel
  2020-04-16 16:38 ` [PATCH v4] Add new uio device for PCI with dynamic memory allocation Manuel Stahl
  0 siblings, 2 replies; 18+ messages in thread
From: Stahl, Manuel @ 2017-10-06 13:31 UTC (permalink / raw)
  To: linux-kernel; +Cc: hjk, devel, gregkh, sojkam1

This device combines the uio_pci_generic driver and the uio_dmem_genirq
driver since PCI uses a slightly different API for interrupts.
A fixed number of DMA capable memory regions can be defined using the
module parameter "dmem_sizes". The memory is not allocated until the uio
device file is opened for the first time. When the device file is closed,
the allocated memory block is freed. Physical (DMA) addresses for the
dynamic regions are provided to the userspace via
/sys/class/uio/uioX/maps/mapY/addr in the same way as static addresses are
when the uio device file is open, when no processes are holding the device
file open, the address returned to userspace is DMA_ERROR_CODE.

Signed-off-by: Manuel Stahl <manuel.stahl@iis.fraunhofer.de>
---
 MAINTAINERS                       |   6 +
 drivers/uio/Kconfig               |   9 +
 drivers/uio/Makefile              |   1 +
 drivers/uio/uio_pci_dmem_genirq.c | 352 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 368 insertions(+)
 create mode 100644 drivers/uio/uio_pci_dmem_genirq.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 65b0c88d5ee0..e135c171f88b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5801,6 +5801,12 @@ L:	kvm@vger.kernel.org
 S:	Supported
 F:	drivers/uio/uio_pci_generic.c
 
+GENERIC UIO DRIVER FOR PCI DEVICES WITH DMA
+M:	"Manuel Stahl" <manuel.stahl@iis.fraunhofer.de>
+L:	kvm@vger.kernel.org
+S:	Supported
+F:	drivers/uio/uio_pci_dmem_genirq.c
+
 GENWQE (IBM Generic Workqueue Card)
 M:	Frank Haverkamp <haver@linux.vnet.ibm.com>
 M:	Guilherme G. Piccoli <gpiccoli@linux.vnet.ibm.com>
diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig
index 7e8dc78a9796..30ca4d405378 100644
--- a/drivers/uio/Kconfig
+++ b/drivers/uio/Kconfig
@@ -93,6 +93,15 @@ config UIO_PCI_GENERIC
 	  primarily, for virtualization scenarios.
 	  If you compile this as a module, it will be called uio_pci_generic.
 
+config UIO_PCI_DMEM_GENIRQ
+	tristate "Generic driver for PCI 2.3 and PCI Express cards with DMA"
+	depends on PCI
+	help
+	  Generic driver that you can bind, dynamically, to any
+	  PCI 2.3 compliant and PCI Express card. It is useful
+          for FPGAs with DMA capability connected via PCI.
+	  If you compile this as a module, it will be called uio_pci_dmem_genirq.
+
 config UIO_NETX
 	tristate "Hilscher NetX Card driver"
 	depends on PCI
diff --git a/drivers/uio/Makefile b/drivers/uio/Makefile
index e9663bb8a4c7..6c010c26695d 100644
--- a/drivers/uio/Makefile
+++ b/drivers/uio/Makefile
@@ -5,6 +5,7 @@ obj-$(CONFIG_UIO_DMEM_GENIRQ)	+= uio_dmem_genirq.o
 obj-$(CONFIG_UIO_AEC)	+= uio_aec.o
 obj-$(CONFIG_UIO_SERCOS3)	+= uio_sercos3.o
 obj-$(CONFIG_UIO_PCI_GENERIC)	+= uio_pci_generic.o
+obj-$(CONFIG_UIO_PCI_DMEM_GENIRQ)	+= uio_pci_dmem_genirq.o
 obj-$(CONFIG_UIO_NETX)	+= uio_netx.o
 obj-$(CONFIG_UIO_PRUSS)         += uio_pruss.o
 obj-$(CONFIG_UIO_MF624)         += uio_mf624.o
diff --git a/drivers/uio/uio_pci_dmem_genirq.c b/drivers/uio/uio_pci_dmem_genirq.c
new file mode 100644
index 000000000000..d1dfd8002c6d
--- /dev/null
+++ b/drivers/uio/uio_pci_dmem_genirq.c
@@ -0,0 +1,352 @@
+/* uio_pci_generic - generic UIO driver for PCI 2.3 devices with DMA memory
+ *
+ * Copyright (C) 2016 Fraunhofer IIS
+ * Author: Manuel Stahl <manuel.stahl@iis.fraunhofer.de>
+ *
+ * Based on uio_pci_generic.c by Michael S. Tsirkin
+ * and uio_dmem_genirq.c by Damian Hobson-Garcia.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.
+ *
+ * Since the driver does not declare any device ids, you must allocate
+ * id and bind the device to the driver yourself.  For example:
+ *
+ * # echo "8086 10f5" > /sys/bus/pci/drivers/uio_pci_dmem_genirq/new_id
+ * # echo -n 0000:00:19.0 > /sys/bus/pci/drivers/e1000e/unbind
+ * # echo -n 0000:00:19.0 > /sys/bus/pci/drivers/uio_pci_dmem_genirq/bind
+ * # ls -l /sys/bus/pci/devices/0000:00:19.0/driver
+ * .../0000:00:19.0/driver -> ../../../bus/pci/drivers/uio_pci_dmem_genirq
+ *
+ * Or use a modprobe alias:
+ * # alias pci:v000010EEd00001000sv*sd*sc*i* uio_pci_dmem_genirq
+ *
+ * Driver won't bind to devices which do not support the Interrupt Disable Bit
+ * in the command register. All devices compliant to PCI 2.3 (circa 2002) and
+ * all compliant PCI Express devices should support this bit.
+ *
+ * The DMA mask bits and sizes of dynamic regions are derived from module
+ * parameters.
+ *
+ * The format for specifying dynamic region sizes in module parameters
+ * is as follows:
+ *
+ * uio_pci_dmem_genirq.dmem_sizes := <uio_dmem_sizes_def>[;<uio_dmem_sizes_def>]
+ * <uio_dmem_sizes_def>           := <pci_id>:<size>[,<size>]
+ * <pci_id>                       := <vendor>:<device>
+ * <size>                         := standard linux memsize
+ *
+ * Examples:
+ *
+ * 1) UIO dmem device with 3 dynamic regions:
+ * uio_pci_dmem_genirq.dmem_sizes=8086:10f5:4K,16K,4M
+ *
+ * 2) Two UIO dmem devices with different number of dynamic regions:
+ * uio_pci_dmem_genirq.dmem_sizes=8086:10f5:4K,16K,4M;1234:0001:8K
+ */
+
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/slab.h>
+#include <linux/uio_driver.h>
+#include <linux/bitops.h>
+#include <linux/interrupt.h>
+#include <linux/stringify.h>
+#include <linux/dma-mapping.h>
+
+#define DRIVER_VERSION  "0.01.0"
+#define DRIVER_AUTHOR   "Manuel Stahl <manuel.stahl@iis.fraunhofer.de>"
+#define DRIVER_DESC "Generic UIO driver for PCI 2.3 devices with DMA memory"
+#define DRIVER_NAME "uio_pci_dmem_genirq"
+#define DMEM_MAP_ERROR (~0)
+
+struct uio_pci_dmem_dev {
+	struct uio_info info;
+	struct pci_dev *pdev;
+	void *dmem_region_vaddr[MAX_UIO_MAPS];
+	unsigned int refcnt;
+	struct mutex alloc_lock; /* mutex for dmem_region_vaddr and refcnt */
+};
+
+static inline struct uio_pci_dmem_dev *
+to_uio_pci_dmem_dev(struct uio_info *info)
+{
+	return container_of(info, struct uio_pci_dmem_dev, info);
+}
+
+static int open(struct uio_info *info, struct inode *inode)
+{
+	struct uio_pci_dmem_dev *priv = to_uio_pci_dmem_dev(info);
+	struct uio_mem *uiomem;
+	int dmem_region = 0;
+
+	uiomem = &priv->info.mem[dmem_region];
+
+	mutex_lock(&priv->alloc_lock);
+	while (!priv->refcnt && uiomem < &priv->info.mem[MAX_UIO_MAPS]) {
+		void *addr;
+
+		if (!uiomem->size)
+			break;
+
+		addr = dma_alloc_coherent(&priv->pdev->dev, uiomem->size,
+					  (dma_addr_t *)&uiomem->addr,
+					  GFP_KERNEL);
+		if (!addr)
+			uiomem->addr = DMEM_MAP_ERROR;
+
+		priv->dmem_region_vaddr[dmem_region++] = addr;
+		++uiomem;
+	}
+	if (pci_check_and_mask_intx(priv->pdev))
+		dev_info(&priv->pdev->dev, "Found pending interrupt");
+
+	if (!priv->refcnt)
+		pci_set_master(priv->pdev);
+
+	priv->refcnt++;
+
+	mutex_unlock(&priv->alloc_lock);
+
+	return 0;
+}
+
+static int release(struct uio_info *info, struct inode *inode)
+{
+	struct uio_pci_dmem_dev *priv = to_uio_pci_dmem_dev(info);
+	struct uio_mem *uiomem;
+	int dmem_region = 0;
+
+	uiomem = &priv->info.mem[dmem_region];
+
+	mutex_lock(&priv->alloc_lock);
+
+	priv->refcnt--;
+	while (!priv->refcnt && uiomem < &priv->info.mem[MAX_UIO_MAPS]) {
+		if (!uiomem->size)
+			break;
+		if (priv->dmem_region_vaddr[dmem_region]) {
+			dma_free_coherent(&priv->pdev->dev, uiomem->size,
+					  priv->dmem_region_vaddr[dmem_region],
+					  uiomem->addr);
+		}
+		uiomem->addr = DMEM_MAP_ERROR;
+		++dmem_region;
+		++uiomem;
+	}
+	if (pci_check_and_mask_intx(priv->pdev))
+		dev_info(&priv->pdev->dev, "Found pending interrupt");
+
+	if (!priv->refcnt)
+		pci_clear_master(priv->pdev);
+
+	mutex_unlock(&priv->alloc_lock);
+	return 0;
+}
+
+static int dmem_mmap(struct uio_info *info, struct vm_area_struct *vma)
+{
+	struct uio_pci_dmem_dev *gdev = to_uio_pci_dmem_dev(info->priv);
+	struct uio_mem *uiomem;
+	int mi = vma->vm_pgoff;
+
+	if (mi >= MAX_UIO_MAPS)
+		return -EINVAL;
+
+	uiomem = &info->mem[mi];
+	if (uiomem->memtype != UIO_MEM_PHYS)
+		return -EINVAL;
+	if (!uiomem->size)
+		return -EINVAL;
+
+	/* DMA address */
+	vma->vm_pgoff = 0;
+	return dma_mmap_coherent(&gdev->pdev->dev, vma,
+				 gdev->dmem_region_vaddr[mi],
+				 uiomem->addr, uiomem->size);
+}
+
+/* Interrupt handler. Read/modify/write the command register to disable the
+ * interrupt.
+ */
+static irqreturn_t irqhandler(int irq, struct uio_info *info)
+{
+	struct uio_pci_dmem_dev *gdev = to_uio_pci_dmem_dev(info);
+
+	if (!pci_check_and_mask_intx(gdev->pdev))
+		return IRQ_NONE;
+
+	/* UIO core will signal the user process. */
+	return IRQ_HANDLED;
+}
+
+static unsigned int uio_dmem_dma_bits = 32;
+static char uio_dmem_sizes[256];
+
+static int parse_dmem_entries(struct pci_dev *pdev,
+			      const struct pci_device_id *id,
+			      struct uio_pci_dmem_dev *gdev)
+{
+	int ret;
+	u32 regions = 0;
+	u32 vendor, device;
+	char *s, *tok, *sizes = NULL;
+	unsigned long long size;
+	struct uio_mem *uiomem;
+	char * const buf = kstrdup(uio_dmem_sizes, GFP_KERNEL);
+
+	if (!buf)
+		return -ENOMEM;
+
+	/* Find-out start and end of sizes list */
+	s = buf;
+	while (*s != '\0') {
+		sizes = NULL;
+		tok = strsep(&s, ":");
+		if (!tok)
+			break;
+		ret = kstrtou32(tok, 16, &vendor);
+		if (ret)
+			break;
+		tok = strsep(&s, ":");
+		if (!tok)
+			break;
+		ret = kstrtou32(tok, 16, &device);
+		if (ret)
+			break;
+		sizes = strsep(&s, ";");
+		if (vendor == id->vendor && device == id->device)
+			break;
+	}
+
+	memset(gdev->info.mem, 0, sizeof(gdev->info.mem));
+	if (sizes) {
+		dev_info(&pdev->dev, "Regions: %s\n", sizes);
+
+		/* Parse dynamic regions from sizes list */
+		regions = 0;
+		size = 0;
+		s = sizes;
+		while (s && (regions < MAX_UIO_MAPS)) {
+			tok = strsep(&s, ",");
+			if (!tok)
+				break;
+
+			size = memparse(tok, NULL);
+			if (size) {
+				uiomem = &gdev->info.mem[regions];
+				uiomem->memtype = UIO_MEM_PHYS;
+				/* Will be allocated in open() call */
+				uiomem->addr = DMEM_MAP_ERROR;
+				uiomem->size = size;
+				regions++;
+			}
+		}
+		if (s)
+			dev_warn(&pdev->dev, "device has more than "
+					__stringify(MAX_UIO_MAPS)
+					" dynamic memory regions.\n");
+	}
+	dev_info(&pdev->dev, "Found %d regions\n", regions);
+
+	kfree(buf);
+	return ret;
+}
+
+static int probe(struct pci_dev *pdev, const struct pci_device_id *id)
+{
+	struct uio_pci_dmem_dev *gdev;
+	int err;
+
+	dev_info(&pdev->dev, "Probe %s for %04x:%04x\n", DRIVER_NAME,
+		 id->vendor, id->device);
+
+	err = pci_enable_device(pdev);
+	if (err) {
+		dev_err(&pdev->dev, "%s: pci_enable_device failed: %d\n",
+			__func__, err);
+		return err;
+	}
+	pci_set_master(pdev);
+
+	dev_info(&pdev->dev, "Legacy IRQ: %i", pdev->irq);
+	if (pdev->irq && !pci_intx_mask_supported(pdev)) {
+		err = -ENODEV;
+		goto err_disable_pci;
+	}
+
+	gdev = kzalloc(sizeof(*gdev), GFP_KERNEL);
+	if (!gdev) {
+		err = -ENOMEM;
+		goto err_disable_pci;
+	}
+
+	gdev->info.name = DRIVER_NAME;
+	gdev->info.version = DRIVER_VERSION;
+	gdev->info.irq = pdev->irq;
+	gdev->info.irq_flags = IRQF_SHARED;
+	gdev->info.handler = irqhandler;
+	gdev->info.open = open;
+	gdev->info.release = release;
+	gdev->info.mmap = dmem_mmap;
+	gdev->info.priv = gdev;
+	gdev->pdev = pdev;
+
+	/* Set DMA coherent mask */
+	if (uio_dmem_dma_bits > 64)
+		uio_dmem_dma_bits = 64;
+	err = dma_set_coherent_mask(&pdev->dev,
+				    DMA_BIT_MASK(uio_dmem_dma_bits));
+	if (err) {
+		dev_err(&pdev->dev, "Unable to dma_set_coherent_mask\n");
+		goto err_free_gdev;
+	}
+
+	err = parse_dmem_entries(pdev, id, gdev);
+	if (err)
+		goto err_free_gdev;
+
+	mutex_init(&gdev->alloc_lock);
+
+	err = uio_register_device(&pdev->dev, &gdev->info);
+	if (err)
+		goto err_free_gdev;
+	pci_set_drvdata(pdev, gdev);
+
+	return 0;
+err_free_gdev:
+	kfree(gdev);
+err_disable_pci:
+	pci_clear_master(pdev);
+	pci_disable_device(pdev);
+	return err;
+}
+
+static void remove(struct pci_dev *pdev)
+{
+	struct uio_pci_dmem_dev *gdev = pci_get_drvdata(pdev);
+
+	uio_unregister_device(&gdev->info);
+	pci_clear_master(pdev);
+	pci_disable_device(pdev);
+	kfree(gdev);
+}
+
+static struct pci_driver uio_pci_driver = {
+	.name = DRIVER_NAME,
+	.id_table = NULL, /* only dynamic id's */
+	.probe = probe,
+	.remove = remove,
+};
+
+module_pci_driver(uio_pci_driver);
+
+module_param_named(dmem_dma_bits, uio_dmem_dma_bits, uint, 0444);
+MODULE_PARM_DESC(dmem_dma_bits, "Number of bits in DMA mask");
+module_param_string(dmem_sizes, uio_dmem_sizes, 256, 0444);
+MODULE_PARM_DESC(dmem_sizes, "Comma separated dynamic region sizes; e.g. 8086:10f5:4K,16K,4M;1234:0001:8K");
+
+MODULE_VERSION(DRIVER_VERSION);
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR(DRIVER_AUTHOR);
+MODULE_DESCRIPTION(DRIVER_DESC);
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 2/2] uio: Prefer MSI(X) interrupts in PCI drivers
  2017-10-06 13:31 [PATCH 1/2 v3] Add new uio device for PCI with dynamic memory allocation Stahl, Manuel
@ 2017-10-06 13:33 ` Stahl, Manuel
  2017-10-06 13:45   ` gregkh
  2020-04-16 16:38 ` [PATCH v4] Add new uio device for PCI with dynamic memory allocation Manuel Stahl
  1 sibling, 1 reply; 18+ messages in thread
From: Stahl, Manuel @ 2017-10-06 13:33 UTC (permalink / raw)
  To: linux-kernel; +Cc: hjk, devel, gregkh, sojkam1


Signed-off-by: Manuel Stahl <manuel.stahl@iis.fraunhofer.de>
---
 drivers/uio/uio_pci_dmem_genirq.c | 27 ++++++++++++++++++++-------
 drivers/uio/uio_pci_generic.c     | 24 ++++++++++++++++++------
 2 files changed, 38 insertions(+), 13 deletions(-)

diff --git a/drivers/uio/uio_pci_dmem_genirq.c b/drivers/uio/uio_pci_dmem_genirq.c
index d1dfd8002c6d..ebb3f9012cd8 100644
--- a/drivers/uio/uio_pci_dmem_genirq.c
+++ b/drivers/uio/uio_pci_dmem_genirq.c
@@ -173,11 +173,13 @@ static irqreturn_t irqhandler(int irq, struct uio_info *info)
 {
 	struct uio_pci_dmem_dev *gdev = to_uio_pci_dmem_dev(info);
 
-	if (!pci_check_and_mask_intx(gdev->pdev))
-		return IRQ_NONE;
+	if (gdev->pdev->msi_enabled || gdev->pdev->msix_enabled)
+		return IRQ_HANDLED;
 
-	/* UIO core will signal the user process. */
-	return IRQ_HANDLED;
+	if (pci_check_and_mask_intx(gdev->pdev))
+		return IRQ_HANDLED;
+
+	return IRQ_NONE;
 }
 
 static unsigned int uio_dmem_dma_bits = 32;
@@ -269,11 +271,19 @@ static int probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	}
 	pci_set_master(pdev);
 
-	dev_info(&pdev->dev, "Legacy IRQ: %i", pdev->irq);
-	if (pdev->irq && !pci_intx_mask_supported(pdev)) {
-		err = -ENODEV;
+	/* Try to use MSI interrupts */
+	err = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES);
+	if (err) {
+		dev_err(&pdev->dev, "%s: pci_alloc_irq_vectors failed: %d\n",
+			__func__, err);
 		goto err_disable_pci;
 	}
+	if (!pdev->msi_enabled && !pdev->msix_enabled &&
+	    !pci_intx_mask_supported(pdev)) {
+		err = -ENODEV;
+		goto err_free_irq_vectors;
+	}
+	dev_info(&pdev->dev, "IRQ: %i", pdev->irq);
 
 	gdev = kzalloc(sizeof(*gdev), GFP_KERNEL);
 	if (!gdev) {
@@ -316,6 +326,8 @@ static int probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	return 0;
 err_free_gdev:
 	kfree(gdev);
+err_free_irq_vectors:
+	pci_free_irq_vectors(pdev);
 err_disable_pci:
 	pci_clear_master(pdev);
 	pci_disable_device(pdev);
@@ -327,6 +339,7 @@ static void remove(struct pci_dev *pdev)
 	struct uio_pci_dmem_dev *gdev = pci_get_drvdata(pdev);
 
 	uio_unregister_device(&gdev->info);
+	pci_free_irq_vectors(pdev);
 	pci_clear_master(pdev);
 	pci_disable_device(pdev);
 	kfree(gdev);
diff --git a/drivers/uio/uio_pci_generic.c b/drivers/uio/uio_pci_generic.c
index a56fdf972dbe..23a800e76303 100644
--- a/drivers/uio/uio_pci_generic.c
+++ b/drivers/uio/uio_pci_generic.c
@@ -46,11 +46,13 @@ static irqreturn_t irqhandler(int irq, struct uio_info *info)
 {
 	struct uio_pci_generic_dev *gdev = to_uio_pci_generic_dev(info);
 
-	if (!pci_check_and_mask_intx(gdev->pdev))
-		return IRQ_NONE;
+	if (gdev->pdev->msi_enabled || gdev->pdev->msix_enabled)
+		return IRQ_HANDLED;
 
-	/* UIO core will signal the user process. */
-	return IRQ_HANDLED;
+	if (pci_check_and_mask_intx(gdev->pdev))
+		return IRQ_HANDLED;
+
+	return IRQ_NONE;
 }
 
 static int probe(struct pci_dev *pdev,
@@ -66,10 +68,18 @@ static int probe(struct pci_dev *pdev,
 		return err;
 	}
 
-	if (pdev->irq && !pci_intx_mask_supported(pdev)) {
-		err = -ENODEV;
+	/* Try to use MSI interrupts */
+	err = pci_alloc_irq_vectors(pdev, 0, 1, PCI_IRQ_ALL_TYPES);
+	if (err) {
+		dev_err(&pdev->dev, "%s: pci_alloc_irq_vectors failed: %d\n",
+			__func__, err);
 		goto err_verify;
 	}
+	if (pdev->irq && !pdev->msi_enabled && !pdev->msix_enabled &&
+	    !pci_intx_mask_supported(pdev)) {
+		err = -ENODEV;
+		goto err_alloc;
+	}
 
 	gdev = kzalloc(sizeof(struct uio_pci_generic_dev), GFP_KERNEL);
 	if (!gdev) {
@@ -98,6 +108,7 @@ static int probe(struct pci_dev *pdev,
 err_register:
 	kfree(gdev);
 err_alloc:
+	pci_free_irq_vectors(pdev);
 err_verify:
 	pci_disable_device(pdev);
 	return err;
@@ -108,6 +119,7 @@ static void remove(struct pci_dev *pdev)
 	struct uio_pci_generic_dev *gdev = pci_get_drvdata(pdev);
 
 	uio_unregister_device(&gdev->info);
+	pci_free_irq_vectors(pdev);
 	pci_disable_device(pdev);
 	kfree(gdev);
 }
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/2] uio: Prefer MSI(X) interrupts in PCI drivers
  2017-10-06 13:33 ` [PATCH 2/2] uio: Prefer MSI(X) interrupts in PCI drivers Stahl, Manuel
@ 2017-10-06 13:45   ` gregkh
  2017-10-06 13:50     ` Stahl, Manuel
  0 siblings, 1 reply; 18+ messages in thread
From: gregkh @ 2017-10-06 13:45 UTC (permalink / raw)
  To: Stahl, Manuel; +Cc: linux-kernel, devel, hjk, sojkam1

On Fri, Oct 06, 2017 at 01:33:32PM +0000, Stahl, Manuel wrote:
> 
> Signed-off-by: Manuel Stahl <manuel.stahl@iis.fraunhofer.de>
> ---
>  drivers/uio/uio_pci_dmem_genirq.c | 27 ++++++++++++++++++++-------
>  drivers/uio/uio_pci_generic.c     | 24 ++++++++++++++++++------
>  2 files changed, 38 insertions(+), 13 deletions(-)

I can't take patches without any changelog text in them, sorry.

greg k-h

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/2] uio: Prefer MSI(X) interrupts in PCI drivers
  2017-10-06 13:45   ` gregkh
@ 2017-10-06 13:50     ` Stahl, Manuel
  2017-10-06 14:57       ` Stephen Hemminger
  0 siblings, 1 reply; 18+ messages in thread
From: Stahl, Manuel @ 2017-10-06 13:50 UTC (permalink / raw)
  To: gregkh; +Cc: hjk, linux-kernel, devel, sojkam1

MSI(X) interrupts are not shared between devices. So when available
those should be preferred over legacy interrupts.

Signed-off-by: Manuel Stahl <manuel.stahl@iis.fraunhofer.de>
---
 drivers/uio/uio_pci_dmem_genirq.c | 27 ++++++++++++++++++++-------
 drivers/uio/uio_pci_generic.c     | 24 ++++++++++++++++++------
 2 files changed, 38 insertions(+), 13 deletions(-)

diff --git a/drivers/uio/uio_pci_dmem_genirq.c b/drivers/uio/uio_pci_dmem_genirq.c
index d1dfd8002c6d..ebb3f9012cd8 100644
--- a/drivers/uio/uio_pci_dmem_genirq.c
+++ b/drivers/uio/uio_pci_dmem_genirq.c
@@ -173,11 +173,13 @@ static irqreturn_t irqhandler(int irq, struct uio_info *info)
 {
 	struct uio_pci_dmem_dev *gdev = to_uio_pci_dmem_dev(info);
 
-	if (!pci_check_and_mask_intx(gdev->pdev))
-		return IRQ_NONE;
+	if (gdev->pdev->msi_enabled || gdev->pdev->msix_enabled)
+		return IRQ_HANDLED;
 
-	/* UIO core will signal the user process. */
-	return IRQ_HANDLED;
+	if (pci_check_and_mask_intx(gdev->pdev))
+		return IRQ_HANDLED;
+
+	return IRQ_NONE;
 }
 
 static unsigned int uio_dmem_dma_bits = 32;
@@ -269,11 +271,19 @@ static int probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	}
 	pci_set_master(pdev);
 
-	dev_info(&pdev->dev, "Legacy IRQ: %i", pdev->irq);
-	if (pdev->irq && !pci_intx_mask_supported(pdev)) {
-		err = -ENODEV;
+	/* Try to use MSI interrupts */
+	err = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES);
+	if (err) {
+		dev_err(&pdev->dev, "%s: pci_alloc_irq_vectors failed: %d\n",
+			__func__, err);
 		goto err_disable_pci;
 	}
+	if (!pdev->msi_enabled && !pdev->msix_enabled &&
+	    !pci_intx_mask_supported(pdev)) {
+		err = -ENODEV;
+		goto err_free_irq_vectors;
+	}
+	dev_info(&pdev->dev, "IRQ: %i", pdev->irq);
 
 	gdev = kzalloc(sizeof(*gdev), GFP_KERNEL);
 	if (!gdev) {
@@ -316,6 +326,8 @@ static int probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	return 0;
 err_free_gdev:
 	kfree(gdev);
+err_free_irq_vectors:
+	pci_free_irq_vectors(pdev);
 err_disable_pci:
 	pci_clear_master(pdev);
 	pci_disable_device(pdev);
@@ -327,6 +339,7 @@ static void remove(struct pci_dev *pdev)
 	struct uio_pci_dmem_dev *gdev = pci_get_drvdata(pdev);
 
 	uio_unregister_device(&gdev->info);
+	pci_free_irq_vectors(pdev);
 	pci_clear_master(pdev);
 	pci_disable_device(pdev);
 	kfree(gdev);
diff --git a/drivers/uio/uio_pci_generic.c b/drivers/uio/uio_pci_generic.c
index a56fdf972dbe..23a800e76303 100644
--- a/drivers/uio/uio_pci_generic.c
+++ b/drivers/uio/uio_pci_generic.c
@@ -46,11 +46,13 @@ static irqreturn_t irqhandler(int irq, struct uio_info *info)
 {
 	struct uio_pci_generic_dev *gdev = to_uio_pci_generic_dev(info);
 
-	if (!pci_check_and_mask_intx(gdev->pdev))
-		return IRQ_NONE;
+	if (gdev->pdev->msi_enabled || gdev->pdev->msix_enabled)
+		return IRQ_HANDLED;
 
-	/* UIO core will signal the user process. */
-	return IRQ_HANDLED;
+	if (pci_check_and_mask_intx(gdev->pdev))
+		return IRQ_HANDLED;
+
+	return IRQ_NONE;
 }
 
 static int probe(struct pci_dev *pdev,
@@ -66,10 +68,18 @@ static int probe(struct pci_dev *pdev,
 		return err;
 	}
 
-	if (pdev->irq && !pci_intx_mask_supported(pdev)) {
-		err = -ENODEV;
+	/* Try to use MSI interrupts */
+	err = pci_alloc_irq_vectors(pdev, 0, 1, PCI_IRQ_ALL_TYPES);
+	if (err) {
+		dev_err(&pdev->dev, "%s: pci_alloc_irq_vectors failed: %d\n",
+			__func__, err);
 		goto err_verify;
 	}
+	if (pdev->irq && !pdev->msi_enabled && !pdev->msix_enabled &&
+	    !pci_intx_mask_supported(pdev)) {
+		err = -ENODEV;
+		goto err_alloc;
+	}
 
 	gdev = kzalloc(sizeof(struct uio_pci_generic_dev), GFP_KERNEL);
 	if (!gdev) {
@@ -98,6 +108,7 @@ static int probe(struct pci_dev *pdev,
 err_register:
 	kfree(gdev);
 err_alloc:
+	pci_free_irq_vectors(pdev);
 err_verify:
 	pci_disable_device(pdev);
 	return err;
@@ -108,6 +119,7 @@ static void remove(struct pci_dev *pdev)
 	struct uio_pci_generic_dev *gdev = pci_get_drvdata(pdev);
 
 	uio_unregister_device(&gdev->info);
+	pci_free_irq_vectors(pdev);
 	pci_disable_device(pdev);
 	kfree(gdev);
 }
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/2] uio: Prefer MSI(X) interrupts in PCI drivers
  2017-10-06 13:50     ` Stahl, Manuel
@ 2017-10-06 14:57       ` Stephen Hemminger
  2017-10-20 12:50         ` gregkh
  0 siblings, 1 reply; 18+ messages in thread
From: Stephen Hemminger @ 2017-10-06 14:57 UTC (permalink / raw)
  To: Stahl, Manuel; +Cc: gregkh, devel, hjk, linux-kernel, sojkam1

On Fri, 6 Oct 2017 13:50:44 +0000
"Stahl, Manuel" <manuel.stahl@iis-extern.fraunhofer.de> wrote:

> MSI(X) interrupts are not shared between devices. So when available
> those should be preferred over legacy interrupts.
> 
> Signed-off-by: Manuel Stahl <manuel.stahl@iis.fraunhofer.de>
> ---
>  drivers/uio/uio_pci_dmem_genirq.c | 27 ++++++++++++++++++++-------
>  drivers/uio/uio_pci_generic.c     | 24 ++++++++++++++++++------
>  2 files changed, 38 insertions(+), 13 deletions(-)

The last time I tried to do MSI-X with pci-generic it got rejected
by the maintainer.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/2] uio: Prefer MSI(X) interrupts in PCI drivers
  2017-10-06 14:57       ` Stephen Hemminger
@ 2017-10-20 12:50         ` gregkh
  2017-10-20 12:58           ` Stahl, Manuel
  2017-10-24 13:11           ` Stephen Hemminger
  0 siblings, 2 replies; 18+ messages in thread
From: gregkh @ 2017-10-20 12:50 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: Stahl, Manuel, devel, hjk, linux-kernel, sojkam1

On Fri, Oct 06, 2017 at 07:57:00AM -0700, Stephen Hemminger wrote:
> On Fri, 6 Oct 2017 13:50:44 +0000
> "Stahl, Manuel" <manuel.stahl@iis-extern.fraunhofer.de> wrote:
> 
> > MSI(X) interrupts are not shared between devices. So when available
> > those should be preferred over legacy interrupts.
> > 
> > Signed-off-by: Manuel Stahl <manuel.stahl@iis.fraunhofer.de>
> > ---
> >  drivers/uio/uio_pci_dmem_genirq.c | 27 ++++++++++++++++++++-------
> >  drivers/uio/uio_pci_generic.c     | 24 ++++++++++++++++++------
> >  2 files changed, 38 insertions(+), 13 deletions(-)
> 
> The last time I tried to do MSI-X with pci-generic it got rejected
> by the maintainer.

Hm, yeah, this would break users today that do not have msi-x, right?

Not good, Manuel, how well did you test this?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/2] uio: Prefer MSI(X) interrupts in PCI drivers
  2017-10-20 12:50         ` gregkh
@ 2017-10-20 12:58           ` Stahl, Manuel
  2020-03-04 15:19             ` Stahl, Manuel
  2017-10-24 13:11           ` Stephen Hemminger
  1 sibling, 1 reply; 18+ messages in thread
From: Stahl, Manuel @ 2017-10-20 12:58 UTC (permalink / raw)
  To: stephen, gregkh; +Cc: hjk, linux-kernel, devel, sojkam1

Hi Greg,

it just uses MSI-X or MSI when available and falls back to legacy IRQ otherwise.

Regards,
Manuel

On Fr, 2017-10-20 at 14:50 +0200, gregkh@linuxfoundation.org wrote:
> On Fri, Oct 06, 2017 at 07:57:00AM -0700, Stephen Hemminger wrote:
> > On Fri, 6 Oct 2017 13:50:44 +0000
> > "Stahl, Manuel" <manuel.stahl@iis-extern.fraunhofer.de> wrote:
> > 
> > > MSI(X) interrupts are not shared between devices. So when available
> > > those should be preferred over legacy interrupts.
> > > 
> > > Signed-off-by: Manuel Stahl <manuel.stahl@iis.fraunhofer.de>
> > > ---
> > >  drivers/uio/uio_pci_dmem_genirq.c | 27 ++++++++++++++++++++-------
> > >  drivers/uio/uio_pci_generic.c     | 24 ++++++++++++++++++------
> > >  2 files changed, 38 insertions(+), 13 deletions(-)
> > 
> > The last time I tried to do MSI-X with pci-generic it got rejected
> > by the maintainer.
> 
> Hm, yeah, this would break users today that do not have msi-x, right?
> 
> Not good, Manuel, how well did you test this?
> 
> thanks,
> 
> greg k-h

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/2] uio: Prefer MSI(X) interrupts in PCI drivers
  2017-10-20 12:50         ` gregkh
  2017-10-20 12:58           ` Stahl, Manuel
@ 2017-10-24 13:11           ` Stephen Hemminger
  1 sibling, 0 replies; 18+ messages in thread
From: Stephen Hemminger @ 2017-10-24 13:11 UTC (permalink / raw)
  To: gregkh; +Cc: Stahl, Manuel, devel, hjk, linux-kernel, sojkam1

On Fri, 20 Oct 2017 14:50:44 +0200
"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org> wrote:

> On Fri, Oct 06, 2017 at 07:57:00AM -0700, Stephen Hemminger wrote:
> > On Fri, 6 Oct 2017 13:50:44 +0000
> > "Stahl, Manuel" <manuel.stahl@iis-extern.fraunhofer.de> wrote:
> >   
> > > MSI(X) interrupts are not shared between devices. So when available
> > > those should be preferred over legacy interrupts.
> > > 
> > > Signed-off-by: Manuel Stahl <manuel.stahl@iis.fraunhofer.de>
> > > ---
> > >  drivers/uio/uio_pci_dmem_genirq.c | 27 ++++++++++++++++++++-------
> > >  drivers/uio/uio_pci_generic.c     | 24 ++++++++++++++++++------
> > >  2 files changed, 38 insertions(+), 13 deletions(-)  
> > 
> > The last time I tried to do MSI-X with pci-generic it got rejected
> > by the maintainer.  
> 
> Hm, yeah, this would break users today that do not have msi-x, right?
> 
> Not good, Manuel, how well did you test this?
> 
> thanks,
> 
> greg k-h

Look at https://patchwork.kernel.org/patch/7303021/

The objection was more that UIO developers did not like that
UIO was (already) being used for DMA without IOMMU, and MSI-x has DMA because
of vector table.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/2] uio: Prefer MSI(X) interrupts in PCI drivers
  2017-10-20 12:58           ` Stahl, Manuel
@ 2020-03-04 15:19             ` Stahl, Manuel
  2020-03-04 15:44               ` gregkh
  0 siblings, 1 reply; 18+ messages in thread
From: Stahl, Manuel @ 2020-03-04 15:19 UTC (permalink / raw)
  To: stephen, gregkh; +Cc: hjk, linux-kernel, devel, sojkam1

Hi Greg,

so somehow this discussion stopped without any instructions how to proceed.
I think this kind of driver helps every FPGA developer to interface his design via PCIe to a Linux PC.
So if there is any chance to get this code merged, I'm glad to rebase this onto the latest kernel release.

Best regards,
Manuel Stahl

On Fr, 2017-10-20 at 14:58 +0200, Manuel Stahl wrote:
> Hi Greg,
> 
> it just uses MSI-X or MSI when available and falls back to legacy IRQ otherwise.
> 
> Regards,
> Manuel
> 
> On Fr, 2017-10-20 at 14:50 +0200, gregkh@linuxfoundation.org wrote:
> > On Fri, Oct 06, 2017 at 07:57:00AM -0700, Stephen Hemminger wrote:
> > > On Fri, 6 Oct 2017 13:50:44 +0000
> > > "Stahl, Manuel" <manuel.stahl@iis-extern.fraunhofer.de> wrote:
> > > 
> > > > MSI(X) interrupts are not shared between devices. So when available
> > > > those should be preferred over legacy interrupts.
> > > > 
> > > > Signed-off-by: Manuel Stahl <manuel.stahl@iis.fraunhofer.de>
> > > > ---
> > > >  drivers/uio/uio_pci_dmem_genirq.c | 27 ++++++++++++++++++++-------
> > > >  drivers/uio/uio_pci_generic.c     | 24 ++++++++++++++++++------
> > > >  2 files changed, 38 insertions(+), 13 deletions(-)
> > > 
> > > The last time I tried to do MSI-X with pci-generic it got rejected
> > > by the maintainer.
> > 
> > Hm, yeah, this would break users today that do not have msi-x, right?
> > 
> > Not good, Manuel, how well did you test this?
> > 
> > thanks,
> > 
> > greg k-h

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/2] uio: Prefer MSI(X) interrupts in PCI drivers
  2020-03-04 15:19             ` Stahl, Manuel
@ 2020-03-04 15:44               ` gregkh
  0 siblings, 0 replies; 18+ messages in thread
From: gregkh @ 2020-03-04 15:44 UTC (permalink / raw)
  To: Stahl, Manuel; +Cc: stephen, hjk, linux-kernel, devel, sojkam1

On Wed, Mar 04, 2020 at 03:19:55PM +0000, Stahl, Manuel wrote:
> Hi Greg,
> 
> so somehow this discussion stopped without any instructions how to proceed.

What is "this discussion"?

> I think this kind of driver helps every FPGA developer to interface
> his design via PCIe to a Linux PC.
> So if there is any chance to get this code merged, I'm glad to rebase
> this onto the latest kernel release.

Please rebase and resubmit, it's a patch from 2 1/2 years ago, not much
I can even remember about patch sets sent last week...

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v4] Add new uio device for PCI with dynamic memory allocation
  2017-10-06 13:31 [PATCH 1/2 v3] Add new uio device for PCI with dynamic memory allocation Stahl, Manuel
  2017-10-06 13:33 ` [PATCH 2/2] uio: Prefer MSI(X) interrupts in PCI drivers Stahl, Manuel
@ 2020-04-16 16:38 ` Manuel Stahl
  2020-04-28 13:54   ` gregkh @ linuxfoundation . org
  1 sibling, 1 reply; 18+ messages in thread
From: Manuel Stahl @ 2020-04-16 16:38 UTC (permalink / raw)
  To: linux-kernel
  Cc: hjk @ linutronix . de, devel @ driverdev . osuosl . org,
	gregkh @ linuxfoundation . org, sojkam1 @ fel . cvut . cz,
	Manuel Stahl, Manuel Stahl

This device combines the uio_pci_generic driver and the uio_dmem_genirq
driver since PCI uses a slightly different API for interrupts.
A fixed number of DMA capable memory regions can be defined using the
module parameter "dmem_sizes". The memory is not allocated until the uio
device file is opened for the first time. When the device file is closed,
the allocated memory block is freed. Physical (DMA) addresses for the
dynamic regions are provided to the userspace via
/sys/class/uio/uioX/maps/mapY/addr
When no processes are holding the device file open, the address returned
to userspace is DMA_ERROR_CODE.

Signed-off-by: Manuel Stahl <manuel.stahl@iis.fraunhofer.de>
---
 MAINTAINERS                       |   6 +
 drivers/uio/Kconfig               |   9 +
 drivers/uio/Makefile              |   1 +
 drivers/uio/uio_pci_dmem_genirq.c | 351 ++++++++++++++++++++++++++++++
 4 files changed, 367 insertions(+)
 create mode 100644 drivers/uio/uio_pci_dmem_genirq.c

diff --git a/MAINTAINERS b/MAINTAINERS
index e64e5db31497..446931530dbc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7149,6 +7149,12 @@ L:	kvm@vger.kernel.org
 S:	Supported
 F:	drivers/uio/uio_pci_generic.c
 
+GENERIC UIO DRIVER FOR PCI DEVICES WITH DMA
+M:	"Manuel Stahl" <manuel.stahl@iis.fraunhofer.de>
+L:	kvm@vger.kernel.org
+S:	Supported
+F:	drivers/uio/uio_pci_dmem_genirq.c
+
 GENERIC VDSO LIBRARY
 M:	Andy Lutomirski <luto@kernel.org>
 M:	Thomas Gleixner <tglx@linutronix.de>
diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig
index 202ee81cfc2b..0d3f8a01ec74 100644
--- a/drivers/uio/Kconfig
+++ b/drivers/uio/Kconfig
@@ -94,6 +94,15 @@ config UIO_PCI_GENERIC
 	  primarily, for virtualization scenarios.
 	  If you compile this as a module, it will be called uio_pci_generic.
 
+config UIO_PCI_DMEM_GENIRQ
+	tristate "Generic driver for PCI 2.3 and PCI Express cards with DMA"
+	depends on PCI
+	help
+	  Generic driver that you can bind, dynamically, to any
+	  PCI 2.3 compliant and PCI Express card. It is useful
+	  for FPGAs with DMA capability connected via PCI.
+	  If you compile this as a module, it will be called uio_pci_dmem_genirq.
+
 config UIO_NETX
 	tristate "Hilscher NetX Card driver"
 	depends on PCI
diff --git a/drivers/uio/Makefile b/drivers/uio/Makefile
index c285dd2a4539..202d6bfdd5aa 100644
--- a/drivers/uio/Makefile
+++ b/drivers/uio/Makefile
@@ -6,6 +6,7 @@ obj-$(CONFIG_UIO_DMEM_GENIRQ)	+= uio_dmem_genirq.o
 obj-$(CONFIG_UIO_AEC)	+= uio_aec.o
 obj-$(CONFIG_UIO_SERCOS3)	+= uio_sercos3.o
 obj-$(CONFIG_UIO_PCI_GENERIC)	+= uio_pci_generic.o
+obj-$(CONFIG_UIO_PCI_DMEM_GENIRQ)	+= uio_pci_dmem_genirq.o
 obj-$(CONFIG_UIO_NETX)	+= uio_netx.o
 obj-$(CONFIG_UIO_PRUSS)         += uio_pruss.o
 obj-$(CONFIG_UIO_MF624)         += uio_mf624.o
diff --git a/drivers/uio/uio_pci_dmem_genirq.c b/drivers/uio/uio_pci_dmem_genirq.c
new file mode 100644
index 000000000000..be1bdcc552fe
--- /dev/null
+++ b/drivers/uio/uio_pci_dmem_genirq.c
@@ -0,0 +1,351 @@
+// SPDX-License-Identifier: GPL-2.0
+/* uio_pci_generic - generic UIO driver for PCI 2.3 devices with DMA memory
+ *
+ * Copyright (C) 2016 Fraunhofer IIS
+ * Author: Manuel Stahl <manuel.stahl@iis.fraunhofer.de>
+ *
+ * Based on uio_pci_generic.c by Michael S. Tsirkin
+ * and uio_dmem_genirq.c by Damian Hobson-Garcia.
+ *
+ * Since the driver does not declare any device ids, you must allocate
+ * id and bind the device to the driver yourself.  For example:
+ *
+ * # echo "8086 10f5" > /sys/bus/pci/drivers/uio_pci_dmem_genirq/new_id
+ * # echo -n 0000:00:19.0 > /sys/bus/pci/drivers/e1000e/unbind
+ * # echo -n 0000:00:19.0 > /sys/bus/pci/drivers/uio_pci_dmem_genirq/bind
+ * # ls -l /sys/bus/pci/devices/0000:00:19.0/driver
+ * .../0000:00:19.0/driver -> ../../../bus/pci/drivers/uio_pci_dmem_genirq
+ *
+ * Or use a modprobe alias:
+ * # alias pci:v000010EEd00001000sv*sd*sc*i* uio_pci_dmem_genirq
+ *
+ * Driver won't bind to devices which do not support the Interrupt Disable Bit
+ * in the command register. All devices compliant to PCI 2.3 (circa 2002) and
+ * all compliant PCI Express devices should support this bit.
+ *
+ * The DMA mask bits and sizes of dynamic regions are derived from module
+ * parameters.
+ *
+ * The format for specifying dynamic region sizes in module parameters
+ * is as follows:
+ *
+ * uio_pci_dmem_genirq.dmem_sizes := <uio_dmem_sizes_def>[;<uio_dmem_sizes_def>]
+ * <uio_dmem_sizes_def>           := <pci_id>:<size>[,<size>]
+ * <pci_id>                       := <vendor>:<device>
+ * <size>                         := standard linux memsize
+ *
+ * Examples:
+ *
+ * 1) UIO dmem device with 3 dynamic regions:
+ * uio_pci_dmem_genirq.dmem_sizes=8086:10f5:4K,16K,4M
+ *
+ * 2) Two UIO dmem devices with different number of dynamic regions:
+ * uio_pci_dmem_genirq.dmem_sizes=8086:10f5:4K,16K,4M;1234:0001:8K
+ */
+
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/slab.h>
+#include <linux/uio_driver.h>
+#include <linux/bitops.h>
+#include <linux/interrupt.h>
+#include <linux/stringify.h>
+#include <linux/dma-mapping.h>
+
+#define DRIVER_VERSION  "0.01.0"
+#define DRIVER_AUTHOR   "Manuel Stahl <manuel.stahl@iis.fraunhofer.de>"
+#define DRIVER_DESC "Generic UIO driver for PCI 2.3 devices with DMA memory"
+#define DRIVER_NAME "uio_pci_dmem_genirq"
+#define DMEM_MAP_ERROR (~0)
+
+struct uio_pci_dmem_dev {
+	struct uio_info info;
+	struct pci_dev *pdev;
+	void *dmem_region_vaddr[MAX_UIO_MAPS];
+	unsigned int refcnt;
+	struct mutex alloc_lock; /* mutex for dmem_region_vaddr and refcnt */
+};
+
+static inline struct uio_pci_dmem_dev *
+to_uio_pci_dmem_dev(struct uio_info *info)
+{
+	return container_of(info, struct uio_pci_dmem_dev, info);
+}
+
+static int open(struct uio_info *info, struct inode *inode)
+{
+	struct uio_pci_dmem_dev *priv = to_uio_pci_dmem_dev(info);
+	struct uio_mem *uiomem;
+	int dmem_region = 0;
+
+	uiomem = &priv->info.mem[dmem_region];
+
+	mutex_lock(&priv->alloc_lock);
+	while (!priv->refcnt && uiomem < &priv->info.mem[MAX_UIO_MAPS]) {
+		void *addr;
+
+		if (!uiomem->size)
+			break;
+
+		addr = dma_alloc_coherent(&priv->pdev->dev, uiomem->size,
+					  (dma_addr_t *)&uiomem->addr,
+					  GFP_KERNEL);
+		if (!addr)
+			uiomem->addr = DMEM_MAP_ERROR;
+
+		priv->dmem_region_vaddr[dmem_region++] = addr;
+		++uiomem;
+	}
+	if (pci_check_and_mask_intx(priv->pdev))
+		dev_info(&priv->pdev->dev, "Found pending interrupt");
+
+	if (!priv->refcnt)
+		pci_set_master(priv->pdev);
+
+	priv->refcnt++;
+
+	mutex_unlock(&priv->alloc_lock);
+
+	return 0;
+}
+
+static int release(struct uio_info *info, struct inode *inode)
+{
+	struct uio_pci_dmem_dev *priv = to_uio_pci_dmem_dev(info);
+	struct uio_mem *uiomem;
+	int dmem_region = 0;
+
+	uiomem = &priv->info.mem[dmem_region];
+
+	mutex_lock(&priv->alloc_lock);
+
+	priv->refcnt--;
+	while (!priv->refcnt && uiomem < &priv->info.mem[MAX_UIO_MAPS]) {
+		if (!uiomem->size)
+			break;
+		if (priv->dmem_region_vaddr[dmem_region]) {
+			dma_free_coherent(&priv->pdev->dev, uiomem->size,
+					  priv->dmem_region_vaddr[dmem_region],
+					  uiomem->addr);
+		}
+		uiomem->addr = DMEM_MAP_ERROR;
+		++dmem_region;
+		++uiomem;
+	}
+	if (pci_check_and_mask_intx(priv->pdev))
+		dev_info(&priv->pdev->dev, "Found pending interrupt");
+
+	if (!priv->refcnt)
+		pci_clear_master(priv->pdev);
+
+	mutex_unlock(&priv->alloc_lock);
+	return 0;
+}
+
+static int dmem_mmap(struct uio_info *info, struct vm_area_struct *vma)
+{
+	struct uio_pci_dmem_dev *gdev = to_uio_pci_dmem_dev(info->priv);
+	struct uio_mem *uiomem;
+	int mi = vma->vm_pgoff;
+
+	if (mi >= MAX_UIO_MAPS)
+		return -EINVAL;
+
+	uiomem = &info->mem[mi];
+	if (uiomem->memtype != UIO_MEM_PHYS)
+		return -EINVAL;
+	if (!uiomem->size)
+		return -EINVAL;
+
+	/* DMA address */
+	vma->vm_pgoff = 0;
+	return dma_mmap_coherent(&gdev->pdev->dev, vma,
+				 gdev->dmem_region_vaddr[mi],
+				 uiomem->addr, uiomem->size);
+}
+
+/* Interrupt handler. Read/modify/write the command register to disable the
+ * interrupt.
+ */
+static irqreturn_t irqhandler(int irq, struct uio_info *info)
+{
+	struct uio_pci_dmem_dev *gdev = to_uio_pci_dmem_dev(info);
+
+	if (!pci_check_and_mask_intx(gdev->pdev))
+		return IRQ_NONE;
+
+	/* UIO core will signal the user process. */
+	return IRQ_HANDLED;
+}
+
+static unsigned int uio_dmem_dma_bits = 32;
+static char uio_dmem_sizes[256];
+
+static int parse_dmem_entries(struct pci_dev *pdev,
+			      const struct pci_device_id *id,
+			      struct uio_pci_dmem_dev *gdev)
+{
+	int ret;
+	u32 regions = 0;
+	u32 vendor, device;
+	char *s, *tok, *sizes = NULL;
+	unsigned long long size;
+	struct uio_mem *uiomem;
+	char * const buf = kstrdup(uio_dmem_sizes, GFP_KERNEL);
+
+	if (!buf)
+		return -ENOMEM;
+
+	/* Find-out start and end of sizes list */
+	s = buf;
+	while (*s != '\0') {
+		sizes = NULL;
+		tok = strsep(&s, ":");
+		if (!tok)
+			break;
+		ret = kstrtou32(tok, 16, &vendor);
+		if (ret)
+			break;
+		tok = strsep(&s, ":");
+		if (!tok)
+			break;
+		ret = kstrtou32(tok, 16, &device);
+		if (ret)
+			break;
+		sizes = strsep(&s, ";");
+		if (vendor == id->vendor && device == id->device)
+			break;
+	}
+
+	memset(gdev->info.mem, 0, sizeof(gdev->info.mem));
+	if (sizes) {
+		dev_info(&pdev->dev, "Regions: %s\n", sizes);
+
+		/* Parse dynamic regions from sizes list */
+		regions = 0;
+		size = 0;
+		s = sizes;
+		while (s && (regions < MAX_UIO_MAPS)) {
+			tok = strsep(&s, ",");
+			if (!tok)
+				break;
+
+			size = memparse(tok, NULL);
+			if (size) {
+				uiomem = &gdev->info.mem[regions];
+				uiomem->memtype = UIO_MEM_PHYS;
+				/* Will be allocated in open() call */
+				uiomem->addr = DMEM_MAP_ERROR;
+				uiomem->size = size;
+				regions++;
+			}
+		}
+		if (s)
+			dev_warn(&pdev->dev, "device has more than "
+					__stringify(MAX_UIO_MAPS)
+					" dynamic memory regions.\n");
+	}
+	dev_info(&pdev->dev, "Found %d regions\n", regions);
+
+	kfree(buf);
+	return ret;
+}
+
+static int probe(struct pci_dev *pdev, const struct pci_device_id *id)
+{
+	struct uio_pci_dmem_dev *gdev;
+	int err;
+
+	dev_info(&pdev->dev, "Probe %s for %04x:%04x\n", DRIVER_NAME,
+		 id->vendor, id->device);
+
+	err = pci_enable_device(pdev);
+	if (err) {
+		dev_err(&pdev->dev, "%s: pci_enable_device failed: %d\n",
+			__func__, err);
+		return err;
+	}
+	pci_set_master(pdev);
+
+	dev_info(&pdev->dev, "Legacy IRQ: %i", pdev->irq);
+	if (pdev->irq && !pci_intx_mask_supported(pdev)) {
+		err = -ENODEV;
+		goto err_disable_pci;
+	}
+
+	gdev = kzalloc(sizeof(*gdev), GFP_KERNEL);
+	if (!gdev) {
+		err = -ENOMEM;
+		goto err_disable_pci;
+	}
+
+	gdev->info.name = DRIVER_NAME;
+	gdev->info.version = DRIVER_VERSION;
+	gdev->info.irq = pdev->irq;
+	gdev->info.irq_flags = IRQF_SHARED;
+	gdev->info.handler = irqhandler;
+	gdev->info.open = open;
+	gdev->info.release = release;
+	gdev->info.mmap = dmem_mmap;
+	gdev->info.priv = gdev;
+	gdev->pdev = pdev;
+
+	/* Set DMA coherent mask */
+	if (uio_dmem_dma_bits > 64)
+		uio_dmem_dma_bits = 64;
+	err = dma_set_coherent_mask(&pdev->dev,
+				    DMA_BIT_MASK(uio_dmem_dma_bits));
+	if (err) {
+		dev_err(&pdev->dev, "Unable to dma_set_coherent_mask\n");
+		goto err_free_gdev;
+	}
+
+	err = parse_dmem_entries(pdev, id, gdev);
+	if (err)
+		goto err_free_gdev;
+
+	mutex_init(&gdev->alloc_lock);
+
+	err = uio_register_device(&pdev->dev, &gdev->info);
+	if (err)
+		goto err_free_gdev;
+	pci_set_drvdata(pdev, gdev);
+
+	return 0;
+err_free_gdev:
+	kfree(gdev);
+err_disable_pci:
+	pci_clear_master(pdev);
+	pci_disable_device(pdev);
+	return err;
+}
+
+static void remove(struct pci_dev *pdev)
+{
+	struct uio_pci_dmem_dev *gdev = pci_get_drvdata(pdev);
+
+	uio_unregister_device(&gdev->info);
+	pci_clear_master(pdev);
+	pci_disable_device(pdev);
+	kfree(gdev);
+}
+
+static struct pci_driver uio_pci_driver = {
+	.name = DRIVER_NAME,
+	.id_table = NULL, /* only dynamic id's */
+	.probe = probe,
+	.remove = remove,
+};
+
+module_pci_driver(uio_pci_driver);
+
+module_param_named(dmem_dma_bits, uio_dmem_dma_bits, uint, 0444);
+MODULE_PARM_DESC(dmem_dma_bits, "Number of bits in DMA mask");
+module_param_string(dmem_sizes, uio_dmem_sizes, 256, 0444);
+MODULE_PARM_DESC(dmem_sizes, "Comma separated dynamic region sizes; e.g. 8086:10f5:4K,16K,4M;1234:0001:8K");
+
+MODULE_VERSION(DRIVER_VERSION);
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR(DRIVER_AUTHOR);
+MODULE_DESCRIPTION(DRIVER_DESC);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH v4] Add new uio device for PCI with dynamic memory allocation
  2020-04-16 16:38 ` [PATCH v4] Add new uio device for PCI with dynamic memory allocation Manuel Stahl
@ 2020-04-28 13:54   ` gregkh @ linuxfoundation . org
  2020-04-28 15:47     ` Stahl, Manuel
  2020-04-29  7:51     ` Stahl, Manuel
  0 siblings, 2 replies; 18+ messages in thread
From: gregkh @ linuxfoundation . org @ 2020-04-28 13:54 UTC (permalink / raw)
  To: Manuel Stahl
  Cc: linux-kernel, devel @ driverdev . osuosl . org,
	hjk @ linutronix . de, Manuel Stahl, sojkam1 @ fel . cvut . cz

On Thu, Apr 16, 2020 at 06:38:30PM +0200, Manuel Stahl wrote:
> This device combines the uio_pci_generic driver and the uio_dmem_genirq
> driver since PCI uses a slightly different API for interrupts.
> A fixed number of DMA capable memory regions can be defined using the
> module parameter "dmem_sizes". The memory is not allocated until the uio
> device file is opened for the first time. When the device file is closed,
> the allocated memory block is freed. Physical (DMA) addresses for the
> dynamic regions are provided to the userspace via
> /sys/class/uio/uioX/maps/mapY/addr
> When no processes are holding the device file open, the address returned
> to userspace is DMA_ERROR_CODE.
> 
> Signed-off-by: Manuel Stahl <manuel.stahl@iis.fraunhofer.de>
> ---
>  MAINTAINERS                       |   6 +
>  drivers/uio/Kconfig               |   9 +
>  drivers/uio/Makefile              |   1 +
>  drivers/uio/uio_pci_dmem_genirq.c | 351 ++++++++++++++++++++++++++++++
>  4 files changed, 367 insertions(+)
>  create mode 100644 drivers/uio/uio_pci_dmem_genirq.c

What changed from previous versions?  Always put that below the ---
line.


> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e64e5db31497..446931530dbc 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7149,6 +7149,12 @@ L:	kvm@vger.kernel.org
>  S:	Supported
>  F:	drivers/uio/uio_pci_generic.c
>  
> +GENERIC UIO DRIVER FOR PCI DEVICES WITH DMA
> +M:	"Manuel Stahl" <manuel.stahl@iis.fraunhofer.de>
> +L:	kvm@vger.kernel.org
> +S:	Supported
> +F:	drivers/uio/uio_pci_dmem_genirq.c
> +
>  GENERIC VDSO LIBRARY
>  M:	Andy Lutomirski <luto@kernel.org>
>  M:	Thomas Gleixner <tglx@linutronix.de>
> diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig
> index 202ee81cfc2b..0d3f8a01ec74 100644
> --- a/drivers/uio/Kconfig
> +++ b/drivers/uio/Kconfig
> @@ -94,6 +94,15 @@ config UIO_PCI_GENERIC
>  	  primarily, for virtualization scenarios.
>  	  If you compile this as a module, it will be called uio_pci_generic.
>  
> +config UIO_PCI_DMEM_GENIRQ
> +	tristate "Generic driver for PCI 2.3 and PCI Express cards with DMA"
> +	depends on PCI
> +	help
> +	  Generic driver that you can bind, dynamically, to any
> +	  PCI 2.3 compliant and PCI Express card. It is useful
> +	  for FPGAs with DMA capability connected via PCI.
> +	  If you compile this as a module, it will be called uio_pci_dmem_genirq.
> +
>  config UIO_NETX
>  	tristate "Hilscher NetX Card driver"
>  	depends on PCI
> diff --git a/drivers/uio/Makefile b/drivers/uio/Makefile
> index c285dd2a4539..202d6bfdd5aa 100644
> --- a/drivers/uio/Makefile
> +++ b/drivers/uio/Makefile
> @@ -6,6 +6,7 @@ obj-$(CONFIG_UIO_DMEM_GENIRQ)	+= uio_dmem_genirq.o
>  obj-$(CONFIG_UIO_AEC)	+= uio_aec.o
>  obj-$(CONFIG_UIO_SERCOS3)	+= uio_sercos3.o
>  obj-$(CONFIG_UIO_PCI_GENERIC)	+= uio_pci_generic.o
> +obj-$(CONFIG_UIO_PCI_DMEM_GENIRQ)	+= uio_pci_dmem_genirq.o
>  obj-$(CONFIG_UIO_NETX)	+= uio_netx.o
>  obj-$(CONFIG_UIO_PRUSS)         += uio_pruss.o
>  obj-$(CONFIG_UIO_MF624)         += uio_mf624.o
> diff --git a/drivers/uio/uio_pci_dmem_genirq.c b/drivers/uio/uio_pci_dmem_genirq.c
> new file mode 100644
> index 000000000000..be1bdcc552fe
> --- /dev/null
> +++ b/drivers/uio/uio_pci_dmem_genirq.c
> @@ -0,0 +1,351 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* uio_pci_generic - generic UIO driver for PCI 2.3 devices with DMA memory
> + *
> + * Copyright (C) 2016 Fraunhofer IIS
> + * Author: Manuel Stahl <manuel.stahl@iis.fraunhofer.de>
> + *
> + * Based on uio_pci_generic.c by Michael S. Tsirkin
> + * and uio_dmem_genirq.c by Damian Hobson-Garcia.
> + *
> + * Since the driver does not declare any device ids, you must allocate
> + * id and bind the device to the driver yourself.  For example:
> + *
> + * # echo "8086 10f5" > /sys/bus/pci/drivers/uio_pci_dmem_genirq/new_id
> + * # echo -n 0000:00:19.0 > /sys/bus/pci/drivers/e1000e/unbind
> + * # echo -n 0000:00:19.0 > /sys/bus/pci/drivers/uio_pci_dmem_genirq/bind
> + * # ls -l /sys/bus/pci/devices/0000:00:19.0/driver
> + * .../0000:00:19.0/driver -> ../../../bus/pci/drivers/uio_pci_dmem_genirq
> + *
> + * Or use a modprobe alias:
> + * # alias pci:v000010EEd00001000sv*sd*sc*i* uio_pci_dmem_genirq
> + *
> + * Driver won't bind to devices which do not support the Interrupt Disable Bit
> + * in the command register. All devices compliant to PCI 2.3 (circa 2002) and
> + * all compliant PCI Express devices should support this bit.
> + *
> + * The DMA mask bits and sizes of dynamic regions are derived from module
> + * parameters.
> + *
> + * The format for specifying dynamic region sizes in module parameters
> + * is as follows:
> + *
> + * uio_pci_dmem_genirq.dmem_sizes := <uio_dmem_sizes_def>[;<uio_dmem_sizes_def>]
> + * <uio_dmem_sizes_def>           := <pci_id>:<size>[,<size>]
> + * <pci_id>                       := <vendor>:<device>
> + * <size>                         := standard linux memsize
> + *
> + * Examples:
> + *
> + * 1) UIO dmem device with 3 dynamic regions:
> + * uio_pci_dmem_genirq.dmem_sizes=8086:10f5:4K,16K,4M
> + *
> + * 2) Two UIO dmem devices with different number of dynamic regions:
> + * uio_pci_dmem_genirq.dmem_sizes=8086:10f5:4K,16K,4M;1234:0001:8K

Module parameters are horrid, are you sure there is no other way?


> + */
> +
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/slab.h>
> +#include <linux/uio_driver.h>
> +#include <linux/bitops.h>
> +#include <linux/interrupt.h>
> +#include <linux/stringify.h>
> +#include <linux/dma-mapping.h>
> +
> +#define DRIVER_VERSION  "0.01.0"
> +#define DRIVER_AUTHOR   "Manuel Stahl <manuel.stahl@iis.fraunhofer.de>"
> +#define DRIVER_DESC "Generic UIO driver for PCI 2.3 devices with DMA memory"
> +#define DRIVER_NAME "uio_pci_dmem_genirq"
> +#define DMEM_MAP_ERROR (~0)
> +
> +struct uio_pci_dmem_dev {
> +	struct uio_info info;
> +	struct pci_dev *pdev;
> +	void *dmem_region_vaddr[MAX_UIO_MAPS];
> +	unsigned int refcnt;
> +	struct mutex alloc_lock; /* mutex for dmem_region_vaddr and refcnt */
> +};
> +
> +static inline struct uio_pci_dmem_dev *
> +to_uio_pci_dmem_dev(struct uio_info *info)
> +{
> +	return container_of(info, struct uio_pci_dmem_dev, info);
> +}
> +
> +static int open(struct uio_info *info, struct inode *inode)
> +{
> +	struct uio_pci_dmem_dev *priv = to_uio_pci_dmem_dev(info);
> +	struct uio_mem *uiomem;
> +	int dmem_region = 0;
> +
> +	uiomem = &priv->info.mem[dmem_region];
> +
> +	mutex_lock(&priv->alloc_lock);
> +	while (!priv->refcnt && uiomem < &priv->info.mem[MAX_UIO_MAPS]) {
> +		void *addr;
> +
> +		if (!uiomem->size)
> +			break;
> +
> +		addr = dma_alloc_coherent(&priv->pdev->dev, uiomem->size,
> +					  (dma_addr_t *)&uiomem->addr,
> +					  GFP_KERNEL);
> +		if (!addr)
> +			uiomem->addr = DMEM_MAP_ERROR;
> +
> +		priv->dmem_region_vaddr[dmem_region++] = addr;
> +		++uiomem;
> +	}
> +	if (pci_check_and_mask_intx(priv->pdev))
> +		dev_info(&priv->pdev->dev, "Found pending interrupt");
> +
> +	if (!priv->refcnt)
> +		pci_set_master(priv->pdev);
> +
> +	priv->refcnt++;
> +
> +	mutex_unlock(&priv->alloc_lock);
> +
> +	return 0;
> +}
> +
> +static int release(struct uio_info *info, struct inode *inode)
> +{
> +	struct uio_pci_dmem_dev *priv = to_uio_pci_dmem_dev(info);
> +	struct uio_mem *uiomem;
> +	int dmem_region = 0;
> +
> +	uiomem = &priv->info.mem[dmem_region];
> +
> +	mutex_lock(&priv->alloc_lock);
> +
> +	priv->refcnt--;
> +	while (!priv->refcnt && uiomem < &priv->info.mem[MAX_UIO_MAPS]) {
> +		if (!uiomem->size)
> +			break;
> +		if (priv->dmem_region_vaddr[dmem_region]) {
> +			dma_free_coherent(&priv->pdev->dev, uiomem->size,
> +					  priv->dmem_region_vaddr[dmem_region],
> +					  uiomem->addr);
> +		}
> +		uiomem->addr = DMEM_MAP_ERROR;
> +		++dmem_region;
> +		++uiomem;
> +	}
> +	if (pci_check_and_mask_intx(priv->pdev))
> +		dev_info(&priv->pdev->dev, "Found pending interrupt");
> +
> +	if (!priv->refcnt)
> +		pci_clear_master(priv->pdev);
> +
> +	mutex_unlock(&priv->alloc_lock);
> +	return 0;
> +}
> +
> +static int dmem_mmap(struct uio_info *info, struct vm_area_struct *vma)
> +{
> +	struct uio_pci_dmem_dev *gdev = to_uio_pci_dmem_dev(info->priv);
> +	struct uio_mem *uiomem;
> +	int mi = vma->vm_pgoff;
> +
> +	if (mi >= MAX_UIO_MAPS)
> +		return -EINVAL;
> +
> +	uiomem = &info->mem[mi];
> +	if (uiomem->memtype != UIO_MEM_PHYS)
> +		return -EINVAL;
> +	if (!uiomem->size)
> +		return -EINVAL;
> +
> +	/* DMA address */
> +	vma->vm_pgoff = 0;
> +	return dma_mmap_coherent(&gdev->pdev->dev, vma,
> +				 gdev->dmem_region_vaddr[mi],
> +				 uiomem->addr, uiomem->size);
> +}
> +
> +/* Interrupt handler. Read/modify/write the command register to disable the
> + * interrupt.
> + */
> +static irqreturn_t irqhandler(int irq, struct uio_info *info)
> +{
> +	struct uio_pci_dmem_dev *gdev = to_uio_pci_dmem_dev(info);
> +
> +	if (!pci_check_and_mask_intx(gdev->pdev))
> +		return IRQ_NONE;
> +
> +	/* UIO core will signal the user process. */
> +	return IRQ_HANDLED;
> +}
> +
> +static unsigned int uio_dmem_dma_bits = 32;
> +static char uio_dmem_sizes[256];
> +
> +static int parse_dmem_entries(struct pci_dev *pdev,
> +			      const struct pci_device_id *id,
> +			      struct uio_pci_dmem_dev *gdev)
> +{
> +	int ret;
> +	u32 regions = 0;
> +	u32 vendor, device;
> +	char *s, *tok, *sizes = NULL;
> +	unsigned long long size;
> +	struct uio_mem *uiomem;
> +	char * const buf = kstrdup(uio_dmem_sizes, GFP_KERNEL);
> +
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	/* Find-out start and end of sizes list */
> +	s = buf;
> +	while (*s != '\0') {
> +		sizes = NULL;
> +		tok = strsep(&s, ":");
> +		if (!tok)
> +			break;
> +		ret = kstrtou32(tok, 16, &vendor);
> +		if (ret)
> +			break;
> +		tok = strsep(&s, ":");
> +		if (!tok)
> +			break;
> +		ret = kstrtou32(tok, 16, &device);
> +		if (ret)
> +			break;
> +		sizes = strsep(&s, ";");
> +		if (vendor == id->vendor && device == id->device)
> +			break;
> +	}
> +
> +	memset(gdev->info.mem, 0, sizeof(gdev->info.mem));
> +	if (sizes) {
> +		dev_info(&pdev->dev, "Regions: %s\n", sizes);

When drivers are working, they should not spit out any messages, make
this, and the other dev_info() calls in here, dev_dbg() at the most.

> +
> +		/* Parse dynamic regions from sizes list */
> +		regions = 0;
> +		size = 0;
> +		s = sizes;
> +		while (s && (regions < MAX_UIO_MAPS)) {
> +			tok = strsep(&s, ",");
> +			if (!tok)
> +				break;
> +
> +			size = memparse(tok, NULL);
> +			if (size) {
> +				uiomem = &gdev->info.mem[regions];
> +				uiomem->memtype = UIO_MEM_PHYS;
> +				/* Will be allocated in open() call */
> +				uiomem->addr = DMEM_MAP_ERROR;
> +				uiomem->size = size;
> +				regions++;
> +			}
> +		}
> +		if (s)
> +			dev_warn(&pdev->dev, "device has more than "
> +					__stringify(MAX_UIO_MAPS)
> +					" dynamic memory regions.\n");
> +	}
> +	dev_info(&pdev->dev, "Found %d regions\n", regions);
> +
> +	kfree(buf);
> +	return ret;
> +}
> +
> +static int probe(struct pci_dev *pdev, const struct pci_device_id *id)
> +{
> +	struct uio_pci_dmem_dev *gdev;
> +	int err;
> +
> +	dev_info(&pdev->dev, "Probe %s for %04x:%04x\n", DRIVER_NAME,
> +		 id->vendor, id->device);
> +
> +	err = pci_enable_device(pdev);
> +	if (err) {
> +		dev_err(&pdev->dev, "%s: pci_enable_device failed: %d\n",
> +			__func__, err);

__func__ is not needed for any dev_* call.

> +		return err;
> +	}
> +	pci_set_master(pdev);
> +
> +	dev_info(&pdev->dev, "Legacy IRQ: %i", pdev->irq);

Again, remove, be quiet :)

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v4] Add new uio device for PCI with dynamic memory allocation
  2020-04-28 13:54   ` gregkh @ linuxfoundation . org
@ 2020-04-28 15:47     ` Stahl, Manuel
  2020-04-28 15:58       ` gregkh
  2020-04-29  7:51     ` Stahl, Manuel
  1 sibling, 1 reply; 18+ messages in thread
From: Stahl, Manuel @ 2020-04-28 15:47 UTC (permalink / raw)
  To: gregkh; +Cc: hjk, linux-kernel, devel, sojkam1

On Di, 2020-04-28 at 15:54 +0200, greg k-h wrote:
> On Thu, Apr 16, 2020 at 06:38:30PM +0200, Manuel Stahl wrote:
> > This device combines the uio_pci_generic driver and the uio_dmem_genirq
> > driver since PCI uses a slightly different API for interrupts.
> > A fixed number of DMA capable memory regions can be defined using the
> > module parameter "dmem_sizes". The memory is not allocated until the uio
> > device file is opened for the first time. When the device file is closed,
> > the allocated memory block is freed. Physical (DMA) addresses for the
> > dynamic regions are provided to the userspace via
> > /sys/class/uio/uioX/maps/mapY/addr
> > When no processes are holding the device file open, the address returned
> > to userspace is DMA_ERROR_CODE.
> > 
> > Signed-off-by: Manuel Stahl <manuel.stahl@iis.fraunhofer.de>
> > ---
> >  MAINTAINERS                       |   6 +
> >  drivers/uio/Kconfig               |   9 +
> >  drivers/uio/Makefile              |   1 +
> >  drivers/uio/uio_pci_dmem_genirq.c | 351 ++++++++++++++++++++++++++++++
> >  4 files changed, 367 insertions(+)
> >  create mode 100644 drivers/uio/uio_pci_dmem_genirq.c
> 
> What changed from previous versions?  Always put that below the ---
> line.

Only rebased to the latest kernel version and incorporated changes from uio_pci_generic and uio_dmem_genirq.
No recommendations were made about other required changes, the discussion stopped about the fact if such a driver (UIO with DMA) should be allowed at all. For me such a driver allows me to quickly
test different FPGA designs on computers that don't have specific security requirements.

> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index e64e5db31497..446931530dbc 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -7149,6 +7149,12 @@ L:	kvm@vger.kernel.org
> >  S:	Supported
> >  F:	drivers/uio/uio_pci_generic.c
> >  
> > +GENERIC UIO DRIVER FOR PCI DEVICES WITH DMA
> > +M:	"Manuel Stahl" <manuel.stahl@iis.fraunhofer.de>
> > +L:	kvm@vger.kernel.org
> > +S:	Supported
> > +F:	drivers/uio/uio_pci_dmem_genirq.c
> > +
> >  GENERIC VDSO LIBRARY
> >  M:	Andy Lutomirski <luto@kernel.org>
> >  M:	Thomas Gleixner <tglx@linutronix.de>
> > diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig
> > index 202ee81cfc2b..0d3f8a01ec74 100644
> > --- a/drivers/uio/Kconfig
> > +++ b/drivers/uio/Kconfig
> > @@ -94,6 +94,15 @@ config UIO_PCI_GENERIC
> >  	  primarily, for virtualization scenarios.
> >  	  If you compile this as a module, it will be called uio_pci_generic.
> >  
> > +config UIO_PCI_DMEM_GENIRQ
> > +	tristate "Generic driver for PCI 2.3 and PCI Express cards with DMA"
> > +	depends on PCI
> > +	help
> > +	  Generic driver that you can bind, dynamically, to any
> > +	  PCI 2.3 compliant and PCI Express card. It is useful
> > +	  for FPGAs with DMA capability connected via PCI.
> > +	  If you compile this as a module, it will be called uio_pci_dmem_genirq.
> > +
> >  config UIO_NETX
> >  	tristate "Hilscher NetX Card driver"
> >  	depends on PCI
> > diff --git a/drivers/uio/Makefile b/drivers/uio/Makefile
> > index c285dd2a4539..202d6bfdd5aa 100644
> > --- a/drivers/uio/Makefile
> > +++ b/drivers/uio/Makefile
> > @@ -6,6 +6,7 @@ obj-$(CONFIG_UIO_DMEM_GENIRQ)	+= uio_dmem_genirq.o
> >  obj-$(CONFIG_UIO_AEC)	+= uio_aec.o
> >  obj-$(CONFIG_UIO_SERCOS3)	+= uio_sercos3.o
> >  obj-$(CONFIG_UIO_PCI_GENERIC)	+= uio_pci_generic.o
> > +obj-$(CONFIG_UIO_PCI_DMEM_GENIRQ)	+= uio_pci_dmem_genirq.o
> >  obj-$(CONFIG_UIO_NETX)	+= uio_netx.o
> >  obj-$(CONFIG_UIO_PRUSS)         += uio_pruss.o
> >  obj-$(CONFIG_UIO_MF624)         += uio_mf624.o
> > diff --git a/drivers/uio/uio_pci_dmem_genirq.c b/drivers/uio/uio_pci_dmem_genirq.c
> > new file mode 100644
> > index 000000000000..be1bdcc552fe
> > --- /dev/null
> > +++ b/drivers/uio/uio_pci_dmem_genirq.c
> > @@ -0,0 +1,351 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* uio_pci_generic - generic UIO driver for PCI 2.3 devices with DMA memory
> > + *
> > + * Copyright (C) 2016 Fraunhofer IIS
> > + * Author: Manuel Stahl <manuel.stahl@iis.fraunhofer.de>
> > + *
> > + * Based on uio_pci_generic.c by Michael S. Tsirkin
> > + * and uio_dmem_genirq.c by Damian Hobson-Garcia.
> > + *
> > + * Since the driver does not declare any device ids, you must allocate
> > + * id and bind the device to the driver yourself.  For example:
> > + *
> > + * # echo "8086 10f5" > /sys/bus/pci/drivers/uio_pci_dmem_genirq/new_id
> > + * # echo -n 0000:00:19.0 > /sys/bus/pci/drivers/e1000e/unbind
> > + * # echo -n 0000:00:19.0 > /sys/bus/pci/drivers/uio_pci_dmem_genirq/bind
> > + * # ls -l /sys/bus/pci/devices/0000:00:19.0/driver
> > + * .../0000:00:19.0/driver -> ../../../bus/pci/drivers/uio_pci_dmem_genirq
> > + *
> > + * Or use a modprobe alias:
> > + * # alias pci:v000010EEd00001000sv*sd*sc*i* uio_pci_dmem_genirq
> > + *
> > + * Driver won't bind to devices which do not support the Interrupt Disable Bit
> > + * in the command register. All devices compliant to PCI 2.3 (circa 2002) and
> > + * all compliant PCI Express devices should support this bit.
> > + *
> > + * The DMA mask bits and sizes of dynamic regions are derived from module
> > + * parameters.
> > + *
> > + * The format for specifying dynamic region sizes in module parameters
> > + * is as follows:
> > + *
> > + * uio_pci_dmem_genirq.dmem_sizes := <uio_dmem_sizes_def>[;<uio_dmem_sizes_def>]
> > + * <uio_dmem_sizes_def>           := <pci_id>:<size>[,<size>]
> > + * <pci_id>                       := <vendor>:<device>
> > + * <size>                         := standard linux memsize
> > + *
> > + * Examples:
> > + *
> > + * 1) UIO dmem device with 3 dynamic regions:
> > + * uio_pci_dmem_genirq.dmem_sizes=8086:10f5:4K,16K,4M
> > + *
> > + * 2) Two UIO dmem devices with different number of dynamic regions:
> > + * uio_pci_dmem_genirq.dmem_sizes=8086:10f5:4K,16K,4M;1234:0001:8K
> 
> Module parameters are horrid, are you sure there is no other way?
> 
> 
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/module.h>
> > +#include <linux/pci.h>
> > +#include <linux/slab.h>
> > +#include <linux/uio_driver.h>
> > +#include <linux/bitops.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/stringify.h>
> > +#include <linux/dma-mapping.h>
> > +
> > +#define DRIVER_VERSION  "0.01.0"
> > +#define DRIVER_AUTHOR   "Manuel Stahl <manuel.stahl@iis.fraunhofer.de>"
> > +#define DRIVER_DESC "Generic UIO driver for PCI 2.3 devices with DMA memory"
> > +#define DRIVER_NAME "uio_pci_dmem_genirq"
> > +#define DMEM_MAP_ERROR (~0)
> > +
> > +struct uio_pci_dmem_dev {
> > +	struct uio_info info;
> > +	struct pci_dev *pdev;
> > +	void *dmem_region_vaddr[MAX_UIO_MAPS];
> > +	unsigned int refcnt;
> > +	struct mutex alloc_lock; /* mutex for dmem_region_vaddr and refcnt */
> > +};
> > +
> > +static inline struct uio_pci_dmem_dev *
> > +to_uio_pci_dmem_dev(struct uio_info *info)
> > +{
> > +	return container_of(info, struct uio_pci_dmem_dev, info);
> > +}
> > +
> > +static int open(struct uio_info *info, struct inode *inode)
> > +{
> > +	struct uio_pci_dmem_dev *priv = to_uio_pci_dmem_dev(info);
> > +	struct uio_mem *uiomem;
> > +	int dmem_region = 0;
> > +
> > +	uiomem = &priv->info.mem[dmem_region];
> > +
> > +	mutex_lock(&priv->alloc_lock);
> > +	while (!priv->refcnt && uiomem < &priv->info.mem[MAX_UIO_MAPS]) {
> > +		void *addr;
> > +
> > +		if (!uiomem->size)
> > +			break;
> > +
> > +		addr = dma_alloc_coherent(&priv->pdev->dev, uiomem->size,
> > +					  (dma_addr_t *)&uiomem->addr,
> > +					  GFP_KERNEL);
> > +		if (!addr)
> > +			uiomem->addr = DMEM_MAP_ERROR;
> > +
> > +		priv->dmem_region_vaddr[dmem_region++] = addr;
> > +		++uiomem;
> > +	}
> > +	if (pci_check_and_mask_intx(priv->pdev))
> > +		dev_info(&priv->pdev->dev, "Found pending interrupt");
> > +
> > +	if (!priv->refcnt)
> > +		pci_set_master(priv->pdev);
> > +
> > +	priv->refcnt++;
> > +
> > +	mutex_unlock(&priv->alloc_lock);
> > +
> > +	return 0;
> > +}
> > +
> > +static int release(struct uio_info *info, struct inode *inode)
> > +{
> > +	struct uio_pci_dmem_dev *priv = to_uio_pci_dmem_dev(info);
> > +	struct uio_mem *uiomem;
> > +	int dmem_region = 0;
> > +
> > +	uiomem = &priv->info.mem[dmem_region];
> > +
> > +	mutex_lock(&priv->alloc_lock);
> > +
> > +	priv->refcnt--;
> > +	while (!priv->refcnt && uiomem < &priv->info.mem[MAX_UIO_MAPS]) {
> > +		if (!uiomem->size)
> > +			break;
> > +		if (priv->dmem_region_vaddr[dmem_region]) {
> > +			dma_free_coherent(&priv->pdev->dev, uiomem->size,
> > +					  priv->dmem_region_vaddr[dmem_region],
> > +					  uiomem->addr);
> > +		}
> > +		uiomem->addr = DMEM_MAP_ERROR;
> > +		++dmem_region;
> > +		++uiomem;
> > +	}
> > +	if (pci_check_and_mask_intx(priv->pdev))
> > +		dev_info(&priv->pdev->dev, "Found pending interrupt");
> > +
> > +	if (!priv->refcnt)
> > +		pci_clear_master(priv->pdev);
> > +
> > +	mutex_unlock(&priv->alloc_lock);
> > +	return 0;
> > +}
> > +
> > +static int dmem_mmap(struct uio_info *info, struct vm_area_struct *vma)
> > +{
> > +	struct uio_pci_dmem_dev *gdev = to_uio_pci_dmem_dev(info->priv);
> > +	struct uio_mem *uiomem;
> > +	int mi = vma->vm_pgoff;
> > +
> > +	if (mi >= MAX_UIO_MAPS)
> > +		return -EINVAL;
> > +
> > +	uiomem = &info->mem[mi];
> > +	if (uiomem->memtype != UIO_MEM_PHYS)
> > +		return -EINVAL;
> > +	if (!uiomem->size)
> > +		return -EINVAL;
> > +
> > +	/* DMA address */
> > +	vma->vm_pgoff = 0;
> > +	return dma_mmap_coherent(&gdev->pdev->dev, vma,
> > +				 gdev->dmem_region_vaddr[mi],
> > +				 uiomem->addr, uiomem->size);
> > +}
> > +
> > +/* Interrupt handler. Read/modify/write the command register to disable the
> > + * interrupt.
> > + */
> > +static irqreturn_t irqhandler(int irq, struct uio_info *info)
> > +{
> > +	struct uio_pci_dmem_dev *gdev = to_uio_pci_dmem_dev(info);
> > +
> > +	if (!pci_check_and_mask_intx(gdev->pdev))
> > +		return IRQ_NONE;
> > +
> > +	/* UIO core will signal the user process. */
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static unsigned int uio_dmem_dma_bits = 32;
> > +static char uio_dmem_sizes[256];
> > +
> > +static int parse_dmem_entries(struct pci_dev *pdev,
> > +			      const struct pci_device_id *id,
> > +			      struct uio_pci_dmem_dev *gdev)
> > +{
> > +	int ret;
> > +	u32 regions = 0;
> > +	u32 vendor, device;
> > +	char *s, *tok, *sizes = NULL;
> > +	unsigned long long size;
> > +	struct uio_mem *uiomem;
> > +	char * const buf = kstrdup(uio_dmem_sizes, GFP_KERNEL);
> > +
> > +	if (!buf)
> > +		return -ENOMEM;
> > +
> > +	/* Find-out start and end of sizes list */
> > +	s = buf;
> > +	while (*s != '\0') {
> > +		sizes = NULL;
> > +		tok = strsep(&s, ":");
> > +		if (!tok)
> > +			break;
> > +		ret = kstrtou32(tok, 16, &vendor);
> > +		if (ret)
> > +			break;
> > +		tok = strsep(&s, ":");
> > +		if (!tok)
> > +			break;
> > +		ret = kstrtou32(tok, 16, &device);
> > +		if (ret)
> > +			break;
> > +		sizes = strsep(&s, ";");
> > +		if (vendor == id->vendor && device == id->device)
> > +			break;
> > +	}
> > +
> > +	memset(gdev->info.mem, 0, sizeof(gdev->info.mem));
> > +	if (sizes) {
> > +		dev_info(&pdev->dev, "Regions: %s\n", sizes);
> 
> When drivers are working, they should not spit out any messages, make
> this, and the other dev_info() calls in here, dev_dbg() at the most.
> 
> > +
> > +		/* Parse dynamic regions from sizes list */
> > +		regions = 0;
> > +		size = 0;
> > +		s = sizes;
> > +		while (s && (regions < MAX_UIO_MAPS)) {
> > +			tok = strsep(&s, ",");
> > +			if (!tok)
> > +				break;
> > +
> > +			size = memparse(tok, NULL);
> > +			if (size) {
> > +				uiomem = &gdev->info.mem[regions];
> > +				uiomem->memtype = UIO_MEM_PHYS;
> > +				/* Will be allocated in open() call */
> > +				uiomem->addr = DMEM_MAP_ERROR;
> > +				uiomem->size = size;
> > +				regions++;
> > +			}
> > +		}
> > +		if (s)
> > +			dev_warn(&pdev->dev, "device has more than "
> > +					__stringify(MAX_UIO_MAPS)
> > +					" dynamic memory regions.\n");
> > +	}
> > +	dev_info(&pdev->dev, "Found %d regions\n", regions);
> > +
> > +	kfree(buf);
> > +	return ret;
> > +}
> > +
> > +static int probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > +{
> > +	struct uio_pci_dmem_dev *gdev;
> > +	int err;
> > +
> > +	dev_info(&pdev->dev, "Probe %s for %04x:%04x\n", DRIVER_NAME,
> > +		 id->vendor, id->device);
> > +
> > +	err = pci_enable_device(pdev);
> > +	if (err) {
> > +		dev_err(&pdev->dev, "%s: pci_enable_device failed: %d\n",
> > +			__func__, err);
> 
> __func__ is not needed for any dev_* call.
> 
> > +		return err;
> > +	}
> > +	pci_set_master(pdev);
> > +
> > +	dev_info(&pdev->dev, "Legacy IRQ: %i", pdev->irq);
> 
> Again, remove, be quiet :)

Use dev_dbg() or remove completely?

Best regards,
Manuel Stahl

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v4] Add new uio device for PCI with dynamic memory allocation
  2020-04-28 15:47     ` Stahl, Manuel
@ 2020-04-28 15:58       ` gregkh
  0 siblings, 0 replies; 18+ messages in thread
From: gregkh @ 2020-04-28 15:58 UTC (permalink / raw)
  To: Stahl, Manuel; +Cc: hjk, linux-kernel, devel, sojkam1

On Tue, Apr 28, 2020 at 03:47:42PM +0000, Stahl, Manuel wrote:
> > 
> > > +		return err;
> > > +	}
> > > +	pci_set_master(pdev);
> > > +
> > > +	dev_info(&pdev->dev, "Legacy IRQ: %i", pdev->irq);
> > 
> > Again, remove, be quiet :)
> 
> Use dev_dbg() or remove completely?

If it helps in debugging, dev_dbg() is fine to use.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v4] Add new uio device for PCI with dynamic memory allocation
  2020-04-28 13:54   ` gregkh @ linuxfoundation . org
  2020-04-28 15:47     ` Stahl, Manuel
@ 2020-04-29  7:51     ` Stahl, Manuel
  2020-04-29  9:41       ` gregkh
  1 sibling, 1 reply; 18+ messages in thread
From: Stahl, Manuel @ 2020-04-29  7:51 UTC (permalink / raw)
  To: gregkh; +Cc: linux-kernel, devel, Stahl, Manuel, sojkam1

On Di, 2020-04-28 at 15:54 +0200, gregkh @ linuxfoundation . org wrote:
> On Thu, Apr 16, 2020 at 06:38:30PM +0200, Manuel Stahl wrote:
> > 
> > + *
> > + * Since the driver does not declare any device ids, you must allocate
> > + * id and bind the device to the driver yourself.  For example:
> > + *
> > + * # echo "8086 10f5" > /sys/bus/pci/drivers/uio_pci_dmem_genirq/new_id
> > + * # echo -n 0000:00:19.0 > /sys/bus/pci/drivers/e1000e/unbind
> > + * # echo -n 0000:00:19.0 > /sys/bus/pci/drivers/uio_pci_dmem_genirq/bind
> > + * # ls -l /sys/bus/pci/devices/0000:00:19.0/driver
> > + * .../0000:00:19.0/driver -> ../../../bus/pci/drivers/uio_pci_dmem_genirq
> > + *
> > + * Or use a modprobe alias:
> > + * # alias pci:v000010EEd00001000sv*sd*sc*i* uio_pci_dmem_genirq
> > + *
> > + * Driver won't bind to devices which do not support the Interrupt Disable Bit
> > + * in the command register. All devices compliant to PCI 2.3 (circa 2002) and
> > + * all compliant PCI Express devices should support this bit.
> > + *
> > + * The DMA mask bits and sizes of dynamic regions are derived from module
> > + * parameters.
> > + *
> > + * The format for specifying dynamic region sizes in module parameters
> > + * is as follows:
> > + *
> > + * uio_pci_dmem_genirq.dmem_sizes := <uio_dmem_sizes_def>[;<uio_dmem_sizes_def>]
> > + * <uio_dmem_sizes_def>           := <pci_id>:<size>[,<size>]
> > + * <pci_id>                       := <vendor>:<device>
> > + * <size>                         := standard linux memsize
> > + *
> > + * Examples:
> > + *
> > + * 1) UIO dmem device with 3 dynamic regions:
> > + * uio_pci_dmem_genirq.dmem_sizes=8086:10f5:4K,16K,4M
> > + *
> > + * 2) Two UIO dmem devices with different number of dynamic regions:
> > + * uio_pci_dmem_genirq.dmem_sizes=8086:10f5:4K,16K,4M;1234:0001:8K
> 
> Module parameters are horrid, are you sure there is no other way?

You're right, seemed to be the simplest solution back when we started developing this driver. I will try to change it to sysfs, so that one can add regions while the module is already loaded.

Regards,
Manuel

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v4] Add new uio device for PCI with dynamic memory allocation
  2020-04-29  7:51     ` Stahl, Manuel
@ 2020-04-29  9:41       ` gregkh
  2020-04-29 13:53         ` Stahl, Manuel
  0 siblings, 1 reply; 18+ messages in thread
From: gregkh @ 2020-04-29  9:41 UTC (permalink / raw)
  To: Stahl, Manuel; +Cc: devel, linux-kernel, sojkam1

On Wed, Apr 29, 2020 at 07:51:01AM +0000, Stahl, Manuel wrote:
> On Di, 2020-04-28 at 15:54 +0200, gregkh @ linuxfoundation . org wrote:
> > On Thu, Apr 16, 2020 at 06:38:30PM +0200, Manuel Stahl wrote:
> > > 
> > > + *
> > > + * Since the driver does not declare any device ids, you must allocate
> > > + * id and bind the device to the driver yourself.  For example:
> > > + *
> > > + * # echo "8086 10f5" > /sys/bus/pci/drivers/uio_pci_dmem_genirq/new_id
> > > + * # echo -n 0000:00:19.0 > /sys/bus/pci/drivers/e1000e/unbind
> > > + * # echo -n 0000:00:19.0 > /sys/bus/pci/drivers/uio_pci_dmem_genirq/bind
> > > + * # ls -l /sys/bus/pci/devices/0000:00:19.0/driver
> > > + * .../0000:00:19.0/driver -> ../../../bus/pci/drivers/uio_pci_dmem_genirq
> > > + *
> > > + * Or use a modprobe alias:
> > > + * # alias pci:v000010EEd00001000sv*sd*sc*i* uio_pci_dmem_genirq
> > > + *
> > > + * Driver won't bind to devices which do not support the Interrupt Disable Bit
> > > + * in the command register. All devices compliant to PCI 2.3 (circa 2002) and
> > > + * all compliant PCI Express devices should support this bit.
> > > + *
> > > + * The DMA mask bits and sizes of dynamic regions are derived from module
> > > + * parameters.
> > > + *
> > > + * The format for specifying dynamic region sizes in module parameters
> > > + * is as follows:
> > > + *
> > > + * uio_pci_dmem_genirq.dmem_sizes := <uio_dmem_sizes_def>[;<uio_dmem_sizes_def>]
> > > + * <uio_dmem_sizes_def>           := <pci_id>:<size>[,<size>]
> > > + * <pci_id>                       := <vendor>:<device>
> > > + * <size>                         := standard linux memsize
> > > + *
> > > + * Examples:
> > > + *
> > > + * 1) UIO dmem device with 3 dynamic regions:
> > > + * uio_pci_dmem_genirq.dmem_sizes=8086:10f5:4K,16K,4M
> > > + *
> > > + * 2) Two UIO dmem devices with different number of dynamic regions:
> > > + * uio_pci_dmem_genirq.dmem_sizes=8086:10f5:4K,16K,4M;1234:0001:8K
> > 
> > Module parameters are horrid, are you sure there is no other way?
> 
> You're right, seemed to be the simplest solution back when we started developing this driver. I will try to change it to sysfs, so that one can add regions while the module is already loaded.

/me hands you some \n characters...

Anyway, configfs is for configuring stuff, don't make a sysfs file that
you have to somehow "parse" please.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v4] Add new uio device for PCI with dynamic memory allocation
  2020-04-29  9:41       ` gregkh
@ 2020-04-29 13:53         ` Stahl, Manuel
  2023-05-18  4:48           ` Hongren Zheng
  0 siblings, 1 reply; 18+ messages in thread
From: Stahl, Manuel @ 2020-04-29 13:53 UTC (permalink / raw)
  To: gregkh; +Cc: linux-kernel, devel, sojkam1

On Mi, 2020-04-29 at 11:41 +0200, gregkh@linuxfoundation.org wrote:
> On Wed, Apr 29, 2020 at 07:51:01AM +0000, Stahl, Manuel wrote:
> > On Di, 2020-04-28 at 15:54 +0200, gregkh @ linuxfoundation . org wrote:
> > > On Thu, Apr 16, 2020 at 06:38:30PM +0200, Manuel Stahl wrote:
> > > > 
> > > > + *
> > > > + * Since the driver does not declare any device ids, you must allocate
> > > > + * id and bind the device to the driver yourself.  For example:
> > > > + *
> > > > + * # echo "8086 10f5" > /sys/bus/pci/drivers/uio_pci_dmem_genirq/new_id
> > > > + * # echo -n 0000:00:19.0 > /sys/bus/pci/drivers/e1000e/unbind
> > > > + * # echo -n 0000:00:19.0 > /sys/bus/pci/drivers/uio_pci_dmem_genirq/bind
> > > > + * # ls -l /sys/bus/pci/devices/0000:00:19.0/driver
> > > > + * .../0000:00:19.0/driver -> ../../../bus/pci/drivers/uio_pci_dmem_genirq
> > > > + *
> > > > + * Or use a modprobe alias:
> > > > + * # alias pci:v000010EEd00001000sv*sd*sc*i* uio_pci_dmem_genirq
> > > > + *
> > > > + * Driver won't bind to devices which do not support the Interrupt Disable Bit
> > > > + * in the command register. All devices compliant to PCI 2.3 (circa 2002) and
> > > > + * all compliant PCI Express devices should support this bit.
> > > > + *
> > > > + * The DMA mask bits and sizes of dynamic regions are derived from module
> > > > + * parameters.
> > > > + *
> > > > + * The format for specifying dynamic region sizes in module parameters
> > > > + * is as follows:
> > > > + *
> > > > + * uio_pci_dmem_genirq.dmem_sizes := <uio_dmem_sizes_def>[;<uio_dmem_sizes_def>]
> > > > + * <uio_dmem_sizes_def>           := <pci_id>:<size>[,<size>]
> > > > + * <pci_id>                       := <vendor>:<device>
> > > > + * <size>                         := standard linux memsize
> > > > + *
> > > > + * Examples:
> > > > + *
> > > > + * 1) UIO dmem device with 3 dynamic regions:
> > > > + * uio_pci_dmem_genirq.dmem_sizes=8086:10f5:4K,16K,4M
> > > > + *
> > > > + * 2) Two UIO dmem devices with different number of dynamic regions:
> > > > + * uio_pci_dmem_genirq.dmem_sizes=8086:10f5:4K,16K,4M;1234:0001:8K
> > > 
> > > Module parameters are horrid, are you sure there is no other way?
> > 
> > You're right, seemed to be the simplest solution back when we started developing this driver. I will try to change it to sysfs, so that one can add regions while the module is already loaded.
> 
> /me hands you some \n characters...
> 
> Anyway, configfs is for configuring stuff, don't make a sysfs file that
> you have to somehow "parse" please.

Looking back at this driver after some years I realized again the reason
for using kernel parameters:

The current UIO API needs the information about available memory maps when
registering a new UIO device with __uio_register_device(), which obviously
needs to be called during probe() in uio_pci_dmem_genirq. Otherwise there
is no device file in /dev to open for user space applications.

After that there is no function to update the uio_map info. So we can either
keep the module parameters and allocate the DMA memory during probe() or
allocate the DMA memory during mmap() and
  a) replicate parts of uio_dev_add_attributes() in this driver to update sysfs
  b) add a function in uio.c to allow updates to the uio_map

Which way would you go?

Best regards,
Manuel Stahl

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v4] Add new uio device for PCI with dynamic memory allocation
  2020-04-29 13:53         ` Stahl, Manuel
@ 2023-05-18  4:48           ` Hongren Zheng
  0 siblings, 0 replies; 18+ messages in thread
From: Hongren Zheng @ 2023-05-18  4:48 UTC (permalink / raw)
  To: Stahl, Manuel, gregkh; +Cc: linux-kernel, devel, sojkam1, Yangyu Chen

On Wed, Apr 29, 2020 at 01:53:02PM +0000, Stahl, Manuel wrote:
> On Mi, 2020-04-29 at 11:41 +0200, gregkh@linuxfoundation.org wrote:
> > On Wed, Apr 29, 2020 at 07:51:01AM +0000, Stahl, Manuel wrote:
> > > On Di, 2020-04-28 at 15:54 +0200, gregkh @ linuxfoundation . org wrote:
> > > > On Thu, Apr 16, 2020 at 06:38:30PM +0200, Manuel Stahl wrote:
> > > > > 
> > > > > + *
> > > > > + * Since the driver does not declare any device ids, you must allocate
> > > > > + * id and bind the device to the driver yourself.  For example:
> > > > > + *
> > > > > + * # echo "8086 10f5" > /sys/bus/pci/drivers/uio_pci_dmem_genirq/new_id
> > > > > + * # echo -n 0000:00:19.0 > /sys/bus/pci/drivers/e1000e/unbind
> > > > > + * # echo -n 0000:00:19.0 > /sys/bus/pci/drivers/uio_pci_dmem_genirq/bind
> > > > > + * # ls -l /sys/bus/pci/devices/0000:00:19.0/driver
> > > > > + * .../0000:00:19.0/driver -> ../../../bus/pci/drivers/uio_pci_dmem_genirq
> > > > > + *
> > > > > + * Or use a modprobe alias:
> > > > > + * # alias pci:v000010EEd00001000sv*sd*sc*i* uio_pci_dmem_genirq
> > > > > + *
> > > > > + * Driver won't bind to devices which do not support the Interrupt Disable Bit
> > > > > + * in the command register. All devices compliant to PCI 2.3 (circa 2002) and
> > > > > + * all compliant PCI Express devices should support this bit.
> > > > > + *
> > > > > + * The DMA mask bits and sizes of dynamic regions are derived from module
> > > > > + * parameters.
> > > > > + *
> > > > > + * The format for specifying dynamic region sizes in module parameters
> > > > > + * is as follows:
> > > > > + *
> > > > > + * uio_pci_dmem_genirq.dmem_sizes := <uio_dmem_sizes_def>[;<uio_dmem_sizes_def>]
> > > > > + * <uio_dmem_sizes_def>           := <pci_id>:<size>[,<size>]
> > > > > + * <pci_id>                       := <vendor>:<device>
> > > > > + * <size>                         := standard linux memsize
> > > > > + *
> > > > > + * Examples:
> > > > > + *
> > > > > + * 1) UIO dmem device with 3 dynamic regions:
> > > > > + * uio_pci_dmem_genirq.dmem_sizes=8086:10f5:4K,16K,4M
> > > > > + *
> > > > > + * 2) Two UIO dmem devices with different number of dynamic regions:
> > > > > + * uio_pci_dmem_genirq.dmem_sizes=8086:10f5:4K,16K,4M;1234:0001:8K
> > > > 
> > > > Module parameters are horrid, are you sure there is no other way?
> > > 
> > > You're right, seemed to be the simplest solution back when we started developing this driver. I will try to change it to sysfs, so that one can add regions while the module is already loaded.
> > 
> > /me hands you some \n characters...
> > 
> > Anyway, configfs is for configuring stuff, don't make a sysfs file that
> > you have to somehow "parse" please.
> 
> Looking back at this driver after some years I realized again the reason
> for using kernel parameters:
> 
> The current UIO API needs the information about available memory maps when
> registering a new UIO device with __uio_register_device(), which obviously
> needs to be called during probe() in uio_pci_dmem_genirq. Otherwise there
> is no device file in /dev to open for user space applications.
> 
> After that there is no function to update the uio_map info. So we can either
> keep the module parameters and allocate the DMA memory during probe() or
> allocate the DMA memory during mmap() and
>   a) replicate parts of uio_dev_add_attributes() in this driver to update sysfs
>   b) add a function in uio.c to allow updates to the uio_map
> 
> Which way would you go?
> 
> Best regards,
> Manuel Stahl

I have similar need for our FPGA project where DMA to
userspace is wanted, how could this be moved forward?

Cc'ed my collaborator here.

It seems that there is not enough maintainance bandwidth for the UIO driver.
I'm willing to be a reviewer, and I assume after handling some of the
patches I can manage to do it. I wonder what else should be done.

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2023-05-18  4:48 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-06 13:31 [PATCH 1/2 v3] Add new uio device for PCI with dynamic memory allocation Stahl, Manuel
2017-10-06 13:33 ` [PATCH 2/2] uio: Prefer MSI(X) interrupts in PCI drivers Stahl, Manuel
2017-10-06 13:45   ` gregkh
2017-10-06 13:50     ` Stahl, Manuel
2017-10-06 14:57       ` Stephen Hemminger
2017-10-20 12:50         ` gregkh
2017-10-20 12:58           ` Stahl, Manuel
2020-03-04 15:19             ` Stahl, Manuel
2020-03-04 15:44               ` gregkh
2017-10-24 13:11           ` Stephen Hemminger
2020-04-16 16:38 ` [PATCH v4] Add new uio device for PCI with dynamic memory allocation Manuel Stahl
2020-04-28 13:54   ` gregkh @ linuxfoundation . org
2020-04-28 15:47     ` Stahl, Manuel
2020-04-28 15:58       ` gregkh
2020-04-29  7:51     ` Stahl, Manuel
2020-04-29  9:41       ` gregkh
2020-04-29 13:53         ` Stahl, Manuel
2023-05-18  4:48           ` Hongren Zheng

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).