[PATCH v6 0/2] UIO support for dfl devices

[PATCH v6 0/2] UIO support for dfl devices

[Xu Yilun]

This patchset supports some dfl device drivers written in userspace.

In the patchset v1, the "driver_override" interface should be used to bind
the DFL UIO driver to DFL devices. But there is concern that the
"driver_override" interface is not OK itself.

In v2, we use a new matching algorithem. The "driver_override" interface
is abandoned, the DFL UIO driver matches any DFL device which could not be
handled by other DFL drivers. So the DFL UIO driver could be used for new
DFL devices which are not supported by kernel. The concern is the UIO may
not be suitable as a default/generic driver for all dfl features, such as
features with multiple interrupts.

In v4, we specify each matching device in the id_table of the UIO driver,
just the same as other dfl drivers do. Now the UIO driver supports Ether
Group feature. To support more DFL features, their feature ids should be
added to the driver's id_table.


Main changes from v1:
- switch to the new matching algorithem. It matches DFL devices which could
  not be handled by other DFL drivers.
- refacor the code about device resources filling.
- add the documentation.

Main changes from v2:
- split the match ops changes in dfl.c to an independent patch.
- move the declarations needed for dfl-uio-pdev from include/linux/dfl.h
  to driver/fpga/dfl.h
- some minor fixes.

Main changes from v3:
- switch to specifying each matching device in the driver's id_table.
- refactor the irq handling code.

Main changes from v4:
- refactor the irq handling code.

Main changes from v5:
- fix the res[] zero initialization issue.
- improve the return code for probe().
- some doc improvement.


Xu Yilun (2):
  fpga: dfl: add the userspace I/O device support for DFL devices
  Documentation: fpga: dfl: Add description for DFL UIO support

 Documentation/fpga/dfl.rst  | 24 ++++++++++++
 drivers/fpga/Kconfig        | 10 +++++
 drivers/fpga/Makefile       |  1 +
 drivers/fpga/dfl-uio-pdev.c | 93 +++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 128 insertions(+)
 create mode 100644 drivers/fpga/dfl-uio-pdev.c

-- 
2.7.4

[PATCH v6 1/2] fpga: dfl: add the userspace I/O device support for DFL devices

[Xu Yilun]

This patch supports the DFL drivers be written in userspace. This is
realized by exposing the userspace I/O device interfaces.

The driver leverages the uio_pdrv_genirq, it adds the uio_pdrv_genirq
platform device with the DFL device's resources, and let the generic UIO
platform device driver provide support to userspace access to kernel
interrupts and memory locations.

The driver now supports the ether group feature. To support a new DFL
feature been directly accessed via UIO, its feature id should be added to
the driver's id_table.

Signed-off-by: Xu Yilun <yilun.xu@intel.com>
Reviewed-by: Tom Rix <trix@redhat.com>
Acked-by: Wu Hao <hao.wu@intel.com>
---
v2: switch to the new matching algorithem. It matches DFL devices which
     could not be handled by other DFL drivers.
    refacor the code about device resources filling.
    fix some comments.
v3: split the dfl.c changes out of this patch.
    some minor fixes
v4: drop the idea of a generic matching algorithem, instead we specify
     each matching device in id_table.
    to make clear that only one irq is supported, the irq handling code
     is refactored.
v5: refactor the irq resource code.
v6: fix the res[] zero initialization issue.
    improve the return code for probe().
---
 drivers/fpga/Kconfig        | 10 +++++
 drivers/fpga/Makefile       |  1 +
 drivers/fpga/dfl-uio-pdev.c | 93 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 104 insertions(+)
 create mode 100644 drivers/fpga/dfl-uio-pdev.c

diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index 5ff9438..61445be 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -203,6 +203,16 @@ config FPGA_DFL_NIOS_INTEL_PAC_N3000
 	  the card. It also instantiates the SPI master (spi-altera) for
 	  the card's BMC (Board Management Controller).
 
+config FPGA_DFL_UIO_PDEV
+	tristate "FPGA DFL Driver for Userspace I/O platform devices"
+	depends on FPGA_DFL && UIO_PDRV_GENIRQ
+	help
+	  Enable this to allow some DFL drivers be written in userspace. It
+	  adds the uio_pdrv_genirq platform device with the DFL feature's
+	  resources, and lets the generic UIO platform device driver provide
+	  support for userspace access to kernel interrupts and memory
+	  locations.
+
 config FPGA_DFL_PCI
 	tristate "FPGA DFL PCIe Device Driver"
 	depends on PCI && FPGA_DFL
diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
index 18dc9885..8847fe0 100644
--- a/drivers/fpga/Makefile
+++ b/drivers/fpga/Makefile
@@ -45,6 +45,7 @@ dfl-afu-objs := dfl-afu-main.o dfl-afu-region.o dfl-afu-dma-region.o
 dfl-afu-objs += dfl-afu-error.o
 
 obj-$(CONFIG_FPGA_DFL_NIOS_INTEL_PAC_N3000)	+= dfl-n3000-nios.o
+obj-$(CONFIG_FPGA_DFL_UIO_PDEV)		+= dfl-uio-pdev.o
 
 # Drivers for FPGAs which implement DFL
 obj-$(CONFIG_FPGA_DFL_PCI)		+= dfl-pci.o
diff --git a/drivers/fpga/dfl-uio-pdev.c b/drivers/fpga/dfl-uio-pdev.c
new file mode 100644
index 0000000..12b47bf
--- /dev/null
+++ b/drivers/fpga/dfl-uio-pdev.c
@@ -0,0 +1,93 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * DFL driver for Userspace I/O platform devices
+ *
+ * Copyright (C) 2020 Intel Corporation, Inc.
+ */
+#include <linux/dfl.h>
+#include <linux/errno.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/uio_driver.h>
+
+#define DRIVER_NAME "dfl-uio-pdev"
+
+static int dfl_uio_pdev_probe(struct dfl_device *ddev)
+{
+	struct platform_device_info pdevinfo = { 0 };
+	struct resource res[2] = { { 0 } };
+	struct uio_info uio_pdata = { 0 };
+	struct platform_device *uio_pdev;
+	struct device *dev = &ddev->dev;
+	unsigned int num_res = 1;
+
+	res[0].parent = &ddev->mmio_res;
+	res[0].flags = IORESOURCE_MEM;
+	res[0].start = ddev->mmio_res.start;
+	res[0].end = ddev->mmio_res.end;
+
+	if (ddev->num_irqs) {
+		if (ddev->num_irqs > 1)
+			dev_warn(&ddev->dev,
+				 "%d irqs for %s, but UIO only supports the first one\n",
+				 ddev->num_irqs, dev_name(&ddev->dev));
+
+		res[1].flags = IORESOURCE_IRQ;
+		res[1].start = ddev->irqs[0];
+		res[1].end = ddev->irqs[0];
+		num_res++;
+	}
+
+	uio_pdata.name = DRIVER_NAME;
+	uio_pdata.version = "0";
+
+	pdevinfo.name = "uio_pdrv_genirq";
+	pdevinfo.res = res;
+	pdevinfo.num_res = num_res;
+	pdevinfo.parent = &ddev->dev;
+	pdevinfo.id = PLATFORM_DEVID_AUTO;
+	pdevinfo.data = &uio_pdata;
+	pdevinfo.size_data = sizeof(uio_pdata);
+
+	uio_pdev = platform_device_register_full(&pdevinfo);
+	if (IS_ERR(uio_pdev))
+		return PTR_ERR(uio_pdev);
+
+	dev_set_drvdata(dev, uio_pdev);
+
+	return 0;
+}
+
+static void dfl_uio_pdev_remove(struct dfl_device *ddev)
+{
+	struct platform_device *uio_pdev = dev_get_drvdata(&ddev->dev);
+
+	platform_device_unregister(uio_pdev);
+}
+
+#define FME_FEATURE_ID_ETH_GROUP	0x10
+
+static const struct dfl_device_id dfl_uio_pdev_ids[] = {
+	{ FME_ID, FME_FEATURE_ID_ETH_GROUP },
+
+	/* Add your new id entries here to support uio for more dfl features */
+
+	{ }
+};
+MODULE_DEVICE_TABLE(dfl, dfl_uio_pdev_ids);
+
+static struct dfl_driver dfl_uio_pdev_driver = {
+	.drv	= {
+		.name       = DRIVER_NAME,
+	},
+	.id_table = dfl_uio_pdev_ids,
+	.probe	= dfl_uio_pdev_probe,
+	.remove	= dfl_uio_pdev_remove,
+};
+module_dfl_driver(dfl_uio_pdev_driver);
+
+MODULE_DESCRIPTION("DFL driver for Userspace I/O platform devices");
+MODULE_AUTHOR("Intel Corporation");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

[PATCH v6 2/2] Documentation: fpga: dfl: Add description for DFL UIO support

[Xu Yilun]

This patch adds description for UIO support for dfl devices on DFL
bus.

Signed-off-by: Xu Yilun <yilun.xu@intel.com>
---
v2: no doc in v1, add it for v2.
v3: some documentation fixes.
v4: documentation change since the driver matching is changed.
v5: no change.
v6: improve the title of the userspace driver support section.
    some word improvement.
---
 Documentation/fpga/dfl.rst | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
index 0404fe6..c33b6d0 100644
--- a/Documentation/fpga/dfl.rst
+++ b/Documentation/fpga/dfl.rst
@@ -7,6 +7,7 @@ Authors:
 - Enno Luebbers <enno.luebbers@intel.com>
 - Xiao Guangrong <guangrong.xiao@linux.intel.com>
 - Wu Hao <hao.wu@intel.com>
+- Xu Yilun <yilun.xu@intel.com>
 
 The Device Feature List (DFL) FPGA framework (and drivers according to
 this framework) hides the very details of low layer hardwares and provides
@@ -502,6 +503,29 @@ FME Partial Reconfiguration Sub Feature driver (see drivers/fpga/dfl-fme-pr.c)
 could be a reference.
 
 
+Userspace driver support for DFL devices
+========================================
+The purpose of an FPGA is to be reprogrammed with newly developed hardware
+components. New hardware can instantiate a new private feature in the DFL, and
+then get a DFL device in their system. In some cases users may need a userspace
+driver for the DFL device:
+
+* Users may need to run some diagnostic test for their hardwares.
+* Users may prototype the kernel driver in user space.
+* Some hardware is designed for specific purposes and does not fit into one of
+  the standard kernel subsystems.
+
+This requires direct access to MMIO space and interrupt handling from
+userspace. The dfl-uio-pdev module exposes the UIO device interfaces for this
+purpose. It adds the uio_pdrv_genirq platform device with the resources of
+the DFL feature, and lets the generic UIO platform device driver provide UIO
+support to userspace.
+
+FPGA_DFL_UIO_PDEV should be selected to enable the dfl-uio-pdev module driver.
+To support a new DFL feature been directly accessed via UIO, its feature id
+should be added to the driver's id_table.
+
+
 Open discussion
 ===============
 FME driver exports one ioctl (DFL_FPGA_FME_PORT_PR) for partial reconfiguration
-- 
2.7.4

Re: [PATCH v6 1/2] fpga: dfl: add the userspace I/O device support for DFL devices

[Moritz Fischer]

Hi Xu,

On Wed, Jan 13, 2021 at 09:54:07AM +0800, Xu Yilun wrote:
> This patch supports the DFL drivers be written in userspace. This is
> realized by exposing the userspace I/O device interfaces.
> 
> The driver leverages the uio_pdrv_genirq, it adds the uio_pdrv_genirq
> platform device with the DFL device's resources, and let the generic UIO
> platform device driver provide support to userspace access to kernel
> interrupts and memory locations.
> 
> The driver now supports the ether group feature. To support a new DFL
> feature been directly accessed via UIO, its feature id should be added to
> the driver's id_table.
> 
> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> Reviewed-by: Tom Rix <trix@redhat.com>
> Acked-by: Wu Hao <hao.wu@intel.com>
> ---
> v2: switch to the new matching algorithem. It matches DFL devices which
>      could not be handled by other DFL drivers.
>     refacor the code about device resources filling.
>     fix some comments.
> v3: split the dfl.c changes out of this patch.
>     some minor fixes
> v4: drop the idea of a generic matching algorithem, instead we specify
>      each matching device in id_table.
>     to make clear that only one irq is supported, the irq handling code
>      is refactored.
> v5: refactor the irq resource code.
> v6: fix the res[] zero initialization issue.
>     improve the return code for probe().
> ---
>  drivers/fpga/Kconfig        | 10 +++++
>  drivers/fpga/Makefile       |  1 +
>  drivers/fpga/dfl-uio-pdev.c | 93 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 104 insertions(+)
>  create mode 100644 drivers/fpga/dfl-uio-pdev.c
> 
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index 5ff9438..61445be 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -203,6 +203,16 @@ config FPGA_DFL_NIOS_INTEL_PAC_N3000
>  	  the card. It also instantiates the SPI master (spi-altera) for
>  	  the card's BMC (Board Management Controller).
>  
> +config FPGA_DFL_UIO_PDEV
> +	tristate "FPGA DFL Driver for Userspace I/O platform devices"
> +	depends on FPGA_DFL && UIO_PDRV_GENIRQ
> +	help
> +	  Enable this to allow some DFL drivers be written in userspace. It
Nit: Enable this to allow DFL drivers to be written in userspace.
> +	  adds the uio_pdrv_genirq platform device with the DFL feature's
> +	  resources, and lets the generic UIO platform device driver provide
> +	  support for userspace access to kernel interrupts and memory
> +	  locations.
> +
>  config FPGA_DFL_PCI
>  	tristate "FPGA DFL PCIe Device Driver"
>  	depends on PCI && FPGA_DFL
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index 18dc9885..8847fe0 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -45,6 +45,7 @@ dfl-afu-objs := dfl-afu-main.o dfl-afu-region.o dfl-afu-dma-region.o
>  dfl-afu-objs += dfl-afu-error.o
>  
>  obj-$(CONFIG_FPGA_DFL_NIOS_INTEL_PAC_N3000)	+= dfl-n3000-nios.o
> +obj-$(CONFIG_FPGA_DFL_UIO_PDEV)		+= dfl-uio-pdev.o
>  
>  # Drivers for FPGAs which implement DFL
>  obj-$(CONFIG_FPGA_DFL_PCI)		+= dfl-pci.o
> diff --git a/drivers/fpga/dfl-uio-pdev.c b/drivers/fpga/dfl-uio-pdev.c
> new file mode 100644
> index 0000000..12b47bf
> --- /dev/null
> +++ b/drivers/fpga/dfl-uio-pdev.c
> @@ -0,0 +1,93 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * DFL driver for Userspace I/O platform devices
> + *
> + * Copyright (C) 2020 Intel Corporation, Inc.
> + */
> +#include <linux/dfl.h>
> +#include <linux/errno.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/uio_driver.h>
> +
> +#define DRIVER_NAME "dfl-uio-pdev"
> +
> +static int dfl_uio_pdev_probe(struct dfl_device *ddev)
> +{
> +	struct platform_device_info pdevinfo = { 0 };
> +	struct resource res[2] = { { 0 } };
> +	struct uio_info uio_pdata = { 0 };
> +	struct platform_device *uio_pdev;
> +	struct device *dev = &ddev->dev;
> +	unsigned int num_res = 1;
> +
> +	res[0].parent = &ddev->mmio_res;
> +	res[0].flags = IORESOURCE_MEM;
> +	res[0].start = ddev->mmio_res.start;
> +	res[0].end = ddev->mmio_res.end;
> +
> +	if (ddev->num_irqs) {
> +		if (ddev->num_irqs > 1)
> +			dev_warn(&ddev->dev,
> +				 "%d irqs for %s, but UIO only supports the first one\n",
> +				 ddev->num_irqs, dev_name(&ddev->dev));
> +
> +		res[1].flags = IORESOURCE_IRQ;
> +		res[1].start = ddev->irqs[0];
> +		res[1].end = ddev->irqs[0];
> +		num_res++;
> +	}
> +
> +	uio_pdata.name = DRIVER_NAME;
> +	uio_pdata.version = "0";
> +
> +	pdevinfo.name = "uio_pdrv_genirq";
> +	pdevinfo.res = res;
> +	pdevinfo.num_res = num_res;
> +	pdevinfo.parent = &ddev->dev;
> +	pdevinfo.id = PLATFORM_DEVID_AUTO;
> +	pdevinfo.data = &uio_pdata;
> +	pdevinfo.size_data = sizeof(uio_pdata);
> +
> +	uio_pdev = platform_device_register_full(&pdevinfo);
It looks like:
	platform_device_register_resndata(&ddev->dev, "uio_pdrv_genirq",
					  PLATFORM_DEVID_AUTO, res, num_res,
					  &uio_pdata, sizeof(uio_pdata))

would work?


> +	if (IS_ERR(uio_pdev))
> +		return PTR_ERR(uio_pdev);
> +
> +	dev_set_drvdata(dev, uio_pdev);
> +
> +	return 0;
> +}
> +
> +static void dfl_uio_pdev_remove(struct dfl_device *ddev)
> +{
> +	struct platform_device *uio_pdev = dev_get_drvdata(&ddev->dev);
> +
> +	platform_device_unregister(uio_pdev);
> +}
> +
> +#define FME_FEATURE_ID_ETH_GROUP	0x10
> +
> +static const struct dfl_device_id dfl_uio_pdev_ids[] = {
> +	{ FME_ID, FME_FEATURE_ID_ETH_GROUP },
> +
> +	/* Add your new id entries here to support uio for more dfl features */
This is fairly common, we can maybe drop this comment?
> +
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(dfl, dfl_uio_pdev_ids);
> +
> +static struct dfl_driver dfl_uio_pdev_driver = {
> +	.drv	= {
> +		.name       = DRIVER_NAME,
> +	},
> +	.id_table = dfl_uio_pdev_ids,
> +	.probe	= dfl_uio_pdev_probe,
> +	.remove	= dfl_uio_pdev_remove,
> +};
> +module_dfl_driver(dfl_uio_pdev_driver);
> +
> +MODULE_DESCRIPTION("DFL driver for Userspace I/O platform devices");
> +MODULE_AUTHOR("Intel Corporation");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.7.4
> 

Otherwise, looks good to me.

- Moritz

Re: [PATCH v6 2/2] Documentation: fpga: dfl: Add description for DFL UIO support

[Moritz Fischer]

Hi Xu,

On Wed, Jan 13, 2021 at 09:54:08AM +0800, Xu Yilun wrote:
> This patch adds description for UIO support for dfl devices on DFL
> bus.
> 
> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> ---
> v2: no doc in v1, add it for v2.
> v3: some documentation fixes.
> v4: documentation change since the driver matching is changed.
> v5: no change.
> v6: improve the title of the userspace driver support section.
>     some word improvement.
> ---
>  Documentation/fpga/dfl.rst | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
> index 0404fe6..c33b6d0 100644
> --- a/Documentation/fpga/dfl.rst
> +++ b/Documentation/fpga/dfl.rst
> @@ -7,6 +7,7 @@ Authors:
>  - Enno Luebbers <enno.luebbers@intel.com>
>  - Xiao Guangrong <guangrong.xiao@linux.intel.com>
>  - Wu Hao <hao.wu@intel.com>
> +- Xu Yilun <yilun.xu@intel.com>
>  
>  The Device Feature List (DFL) FPGA framework (and drivers according to
>  this framework) hides the very details of low layer hardwares and provides
> @@ -502,6 +503,29 @@ FME Partial Reconfiguration Sub Feature driver (see drivers/fpga/dfl-fme-pr.c)
>  could be a reference.
>  
>  
> +Userspace driver support for DFL devices
> +========================================
> +The purpose of an FPGA is to be reprogrammed with newly developed hardware
> +components. New hardware can instantiate a new private feature in the DFL, and
> +then get a DFL device in their system. In some cases users may need a userspace
> +driver for the DFL device:
> +
> +* Users may need to run some diagnostic test for their hardwares.
> +* Users may prototype the kernel driver in user space.
> +* Some hardware is designed for specific purposes and does not fit into one of
> +  the standard kernel subsystems.
> +
> +This requires direct access to MMIO space and interrupt handling from
> +userspace. The dfl-uio-pdev module exposes the UIO device interfaces for this
> +purpose. It adds the uio_pdrv_genirq platform device with the resources of
> +the DFL feature, and lets the generic UIO platform device driver provide UIO
> +support to userspace.
> +
> +FPGA_DFL_UIO_PDEV should be selected to enable the dfl-uio-pdev module driver.
> +To support a new DFL feature been directly accessed via UIO, its feature id
> +should be added to the driver's id_table.
> +
> +
>  Open discussion
>  ===============
>  FME driver exports one ioctl (DFL_FPGA_FME_PORT_PR) for partial reconfiguration
> -- 
> 2.7.4
> 

Looks fine to me, can you resend with changes for the other patch?

- Moritz

Re: [PATCH v6 1/2] fpga: dfl: add the userspace I/O device support for DFL devices

[Greg KH]

On Wed, Jan 13, 2021 at 09:54:07AM +0800, Xu Yilun wrote:
> This patch supports the DFL drivers be written in userspace. This is
> realized by exposing the userspace I/O device interfaces.
> 
> The driver leverages the uio_pdrv_genirq, it adds the uio_pdrv_genirq
> platform device with the DFL device's resources, and let the generic UIO
> platform device driver provide support to userspace access to kernel
> interrupts and memory locations.

Why doesn't the existing uio driver work for this, why do you need a new
one?

> ---
>  drivers/fpga/Kconfig        | 10 +++++
>  drivers/fpga/Makefile       |  1 +
>  drivers/fpga/dfl-uio-pdev.c | 93 +++++++++++++++++++++++++++++++++++++++++++++

uio drivers traditionally go in drivers/uio/ and start with "uio", so
shouldn't this be drivers/uio/uio_dfl_pdev.c to match the same naming
scheme?

But again, you need to explain in detail, why the existing uio driver
doesn't work properly, or why you can't just add a few lines to an
existing one.

thanks,

greg k-h

Re: [PATCH v6 1/2] fpga: dfl: add the userspace I/O device support for DFL devices

[Moritz Fischer]

Greg,

On Sun, Jan 17, 2021 at 04:45:04PM +0100, Greg KH wrote:
> On Wed, Jan 13, 2021 at 09:54:07AM +0800, Xu Yilun wrote:
> > This patch supports the DFL drivers be written in userspace. This is
> > realized by exposing the userspace I/O device interfaces.
> > 
> > The driver leverages the uio_pdrv_genirq, it adds the uio_pdrv_genirq
> > platform device with the DFL device's resources, and let the generic UIO
> > platform device driver provide support to userspace access to kernel
> > interrupts and memory locations.
> 
> Why doesn't the existing uio driver work for this, why do you need a new
> one?
> 
> > ---
> >  drivers/fpga/Kconfig        | 10 +++++
> >  drivers/fpga/Makefile       |  1 +
> >  drivers/fpga/dfl-uio-pdev.c | 93 +++++++++++++++++++++++++++++++++++++++++++++
> 
> uio drivers traditionally go in drivers/uio/ and start with "uio", so
> shouldn't this be drivers/uio/uio_dfl_pdev.c to match the same naming
> scheme?

I had considered suggesting that, but ultimately this driver only
creates a 'uio_pdrv_genirq' platform device, so it didn't seem like a
good fit.
> 
> But again, you need to explain in detail, why the existing uio driver
> doesn't work properly, or why you can't just add a few lines to an
> existing one.

Ultimately there are three options I see:
1) Do what Xu does, which is re-use the 'uio_pdrv_genirq' uio driver by
  creating a platform device for it as sub-device of the dfl device that
  we bind to uio_pdrv_genirq
2) Add a module_dfl_driver part to drivers/uio/uio_pdrv_genirq.c and
  corresponding id table
3) Create a new uio_dfl_genirq kind of driver that uses the dfl bus and
  that would make sense to then put into drivers/uio. (This would
  duplicate code in uio_pdrv_genirq to some extend)

Overall I think in terms of code re-use I think Xu's choice might be
less new code as it simply wraps the uio platform device driver, and
allows for defining the resources passed to the UIO driver to be defined
by hardware through a DFL.

I've seen the pattern that Xu proposed used in other places like the
macb network driver where you'd have macb_main (the platform driver) and
macb_pci that wraps it for a pci usage.

- Moritz

Re: [PATCH v6 1/2] fpga: dfl: add the userspace I/O device support for DFL devices

[Xu Yilun]

On Sat, Jan 16, 2021 at 07:56:08PM -0800, Moritz Fischer wrote:
> Hi Xu,
> 
> On Wed, Jan 13, 2021 at 09:54:07AM +0800, Xu Yilun wrote:
> > This patch supports the DFL drivers be written in userspace. This is
> > realized by exposing the userspace I/O device interfaces.
> > 
> > The driver leverages the uio_pdrv_genirq, it adds the uio_pdrv_genirq
> > platform device with the DFL device's resources, and let the generic UIO
> > platform device driver provide support to userspace access to kernel
> > interrupts and memory locations.
> > 
> > The driver now supports the ether group feature. To support a new DFL
> > feature been directly accessed via UIO, its feature id should be added to
> > the driver's id_table.
> > 
> > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > Reviewed-by: Tom Rix <trix@redhat.com>
> > Acked-by: Wu Hao <hao.wu@intel.com>
> > ---
> > v2: switch to the new matching algorithem. It matches DFL devices which
> >      could not be handled by other DFL drivers.
> >     refacor the code about device resources filling.
> >     fix some comments.
> > v3: split the dfl.c changes out of this patch.
> >     some minor fixes
> > v4: drop the idea of a generic matching algorithem, instead we specify
> >      each matching device in id_table.
> >     to make clear that only one irq is supported, the irq handling code
> >      is refactored.
> > v5: refactor the irq resource code.
> > v6: fix the res[] zero initialization issue.
> >     improve the return code for probe().
> > ---
> >  drivers/fpga/Kconfig        | 10 +++++
> >  drivers/fpga/Makefile       |  1 +
> >  drivers/fpga/dfl-uio-pdev.c | 93 +++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 104 insertions(+)
> >  create mode 100644 drivers/fpga/dfl-uio-pdev.c
> > 
> > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> > index 5ff9438..61445be 100644
> > --- a/drivers/fpga/Kconfig
> > +++ b/drivers/fpga/Kconfig
> > @@ -203,6 +203,16 @@ config FPGA_DFL_NIOS_INTEL_PAC_N3000
> >  	  the card. It also instantiates the SPI master (spi-altera) for
> >  	  the card's BMC (Board Management Controller).
> >  
> > +config FPGA_DFL_UIO_PDEV
> > +	tristate "FPGA DFL Driver for Userspace I/O platform devices"
> > +	depends on FPGA_DFL && UIO_PDRV_GENIRQ
> > +	help
> > +	  Enable this to allow some DFL drivers be written in userspace. It
> Nit: Enable this to allow DFL drivers to be written in userspace.

Yes, will fix it.

> > +	  adds the uio_pdrv_genirq platform device with the DFL feature's
> > +	  resources, and lets the generic UIO platform device driver provide
> > +	  support for userspace access to kernel interrupts and memory
> > +	  locations.
> > +
> >  config FPGA_DFL_PCI
> >  	tristate "FPGA DFL PCIe Device Driver"
> >  	depends on PCI && FPGA_DFL
> > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> > index 18dc9885..8847fe0 100644
> > --- a/drivers/fpga/Makefile
> > +++ b/drivers/fpga/Makefile
> > @@ -45,6 +45,7 @@ dfl-afu-objs := dfl-afu-main.o dfl-afu-region.o dfl-afu-dma-region.o
> >  dfl-afu-objs += dfl-afu-error.o
> >  
> >  obj-$(CONFIG_FPGA_DFL_NIOS_INTEL_PAC_N3000)	+= dfl-n3000-nios.o
> > +obj-$(CONFIG_FPGA_DFL_UIO_PDEV)		+= dfl-uio-pdev.o
> >  
> >  # Drivers for FPGAs which implement DFL
> >  obj-$(CONFIG_FPGA_DFL_PCI)		+= dfl-pci.o
> > diff --git a/drivers/fpga/dfl-uio-pdev.c b/drivers/fpga/dfl-uio-pdev.c
> > new file mode 100644
> > index 0000000..12b47bf
> > --- /dev/null
> > +++ b/drivers/fpga/dfl-uio-pdev.c
> > @@ -0,0 +1,93 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * DFL driver for Userspace I/O platform devices
> > + *
> > + * Copyright (C) 2020 Intel Corporation, Inc.
> > + */
> > +#include <linux/dfl.h>
> > +#include <linux/errno.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +#include <linux/uio_driver.h>
> > +
> > +#define DRIVER_NAME "dfl-uio-pdev"
> > +
> > +static int dfl_uio_pdev_probe(struct dfl_device *ddev)
> > +{
> > +	struct platform_device_info pdevinfo = { 0 };
> > +	struct resource res[2] = { { 0 } };
> > +	struct uio_info uio_pdata = { 0 };
> > +	struct platform_device *uio_pdev;
> > +	struct device *dev = &ddev->dev;
> > +	unsigned int num_res = 1;
> > +
> > +	res[0].parent = &ddev->mmio_res;
> > +	res[0].flags = IORESOURCE_MEM;
> > +	res[0].start = ddev->mmio_res.start;
> > +	res[0].end = ddev->mmio_res.end;
> > +
> > +	if (ddev->num_irqs) {
> > +		if (ddev->num_irqs > 1)
> > +			dev_warn(&ddev->dev,
> > +				 "%d irqs for %s, but UIO only supports the first one\n",
> > +				 ddev->num_irqs, dev_name(&ddev->dev));
> > +
> > +		res[1].flags = IORESOURCE_IRQ;
> > +		res[1].start = ddev->irqs[0];
> > +		res[1].end = ddev->irqs[0];
> > +		num_res++;
> > +	}
> > +
> > +	uio_pdata.name = DRIVER_NAME;
> > +	uio_pdata.version = "0";
> > +
> > +	pdevinfo.name = "uio_pdrv_genirq";
> > +	pdevinfo.res = res;
> > +	pdevinfo.num_res = num_res;
> > +	pdevinfo.parent = &ddev->dev;
> > +	pdevinfo.id = PLATFORM_DEVID_AUTO;
> > +	pdevinfo.data = &uio_pdata;
> > +	pdevinfo.size_data = sizeof(uio_pdata);
> > +
> > +	uio_pdev = platform_device_register_full(&pdevinfo);
> It looks like:
> 	platform_device_register_resndata(&ddev->dev, "uio_pdrv_genirq",
> 					  PLATFORM_DEVID_AUTO, res, num_res,
> 					  &uio_pdata, sizeof(uio_pdata))
> 
> would work?

It works. I'll change it.

> 
> 
> > +	if (IS_ERR(uio_pdev))
> > +		return PTR_ERR(uio_pdev);
> > +
> > +	dev_set_drvdata(dev, uio_pdev);
> > +
> > +	return 0;
> > +}
> > +
> > +static void dfl_uio_pdev_remove(struct dfl_device *ddev)
> > +{
> > +	struct platform_device *uio_pdev = dev_get_drvdata(&ddev->dev);
> > +
> > +	platform_device_unregister(uio_pdev);
> > +}
> > +
> > +#define FME_FEATURE_ID_ETH_GROUP	0x10
> > +
> > +static const struct dfl_device_id dfl_uio_pdev_ids[] = {
> > +	{ FME_ID, FME_FEATURE_ID_ETH_GROUP },
> > +
> > +	/* Add your new id entries here to support uio for more dfl features */
> This is fairly common, we can maybe drop this comment?

Yes.

Thanks,
Yilun

Re: [PATCH v6 2/2] Documentation: fpga: dfl: Add description for DFL UIO support

[Xu Yilun]

On Sat, Jan 16, 2021 at 07:57:48PM -0800, Moritz Fischer wrote:
> Hi Xu,
> 
> On Wed, Jan 13, 2021 at 09:54:08AM +0800, Xu Yilun wrote:
> > This patch adds description for UIO support for dfl devices on DFL
> > bus.
> > 
> > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > ---
> > v2: no doc in v1, add it for v2.
> > v3: some documentation fixes.
> > v4: documentation change since the driver matching is changed.
> > v5: no change.
> > v6: improve the title of the userspace driver support section.
> >     some word improvement.
> > ---
> >  Documentation/fpga/dfl.rst | 24 ++++++++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> > 
> > diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
> > index 0404fe6..c33b6d0 100644
> > --- a/Documentation/fpga/dfl.rst
> > +++ b/Documentation/fpga/dfl.rst
> > @@ -7,6 +7,7 @@ Authors:
> >  - Enno Luebbers <enno.luebbers@intel.com>
> >  - Xiao Guangrong <guangrong.xiao@linux.intel.com>
> >  - Wu Hao <hao.wu@intel.com>
> > +- Xu Yilun <yilun.xu@intel.com>
> >  
> >  The Device Feature List (DFL) FPGA framework (and drivers according to
> >  this framework) hides the very details of low layer hardwares and provides
> > @@ -502,6 +503,29 @@ FME Partial Reconfiguration Sub Feature driver (see drivers/fpga/dfl-fme-pr.c)
> >  could be a reference.
> >  
> >  
> > +Userspace driver support for DFL devices
> > +========================================
> > +The purpose of an FPGA is to be reprogrammed with newly developed hardware
> > +components. New hardware can instantiate a new private feature in the DFL, and
> > +then get a DFL device in their system. In some cases users may need a userspace
> > +driver for the DFL device:
> > +
> > +* Users may need to run some diagnostic test for their hardwares.
> > +* Users may prototype the kernel driver in user space.
> > +* Some hardware is designed for specific purposes and does not fit into one of
> > +  the standard kernel subsystems.
> > +
> > +This requires direct access to MMIO space and interrupt handling from
> > +userspace. The dfl-uio-pdev module exposes the UIO device interfaces for this
> > +purpose. It adds the uio_pdrv_genirq platform device with the resources of
> > +the DFL feature, and lets the generic UIO platform device driver provide UIO
> > +support to userspace.
> > +
> > +FPGA_DFL_UIO_PDEV should be selected to enable the dfl-uio-pdev module driver.
> > +To support a new DFL feature been directly accessed via UIO, its feature id
> > +should be added to the driver's id_table.
> > +
> > +
> >  Open discussion
> >  ===============
> >  FME driver exports one ioctl (DFL_FPGA_FME_PORT_PR) for partial reconfiguration
> > -- 
> > 2.7.4
> > 
> 
> Looks fine to me, can you resend with changes for the other patch?

Yes, I will. I may wait for a while to check more comments from Greg.

Thanks,
Yilun

Re: [PATCH v6 1/2] fpga: dfl: add the userspace I/O device support for DFL devices

[Tom Rix]

On 1/17/21 8:22 AM, Moritz Fischer wrote:
> Greg,
>
> On Sun, Jan 17, 2021 at 04:45:04PM +0100, Greg KH wrote:
>> On Wed, Jan 13, 2021 at 09:54:07AM +0800, Xu Yilun wrote:
>>> This patch supports the DFL drivers be written in userspace. This is
>>> realized by exposing the userspace I/O device interfaces.
>>>
>>> The driver leverages the uio_pdrv_genirq, it adds the uio_pdrv_genirq
>>> platform device with the DFL device's resources, and let the generic UIO
>>> platform device driver provide support to userspace access to kernel
>>> interrupts and memory locations.
>> Why doesn't the existing uio driver work for this, why do you need a new
>> one?
>>
>>> ---
>>>  drivers/fpga/Kconfig        | 10 +++++
>>>  drivers/fpga/Makefile       |  1 +
>>>  drivers/fpga/dfl-uio-pdev.c | 93 +++++++++++++++++++++++++++++++++++++++++++++
>> uio drivers traditionally go in drivers/uio/ and start with "uio", so
>> shouldn't this be drivers/uio/uio_dfl_pdev.c to match the same naming
>> scheme?
> I had considered suggesting that, but ultimately this driver only
> creates a 'uio_pdrv_genirq' platform device, so it didn't seem like a
> good fit.
>> But again, you need to explain in detail, why the existing uio driver
>> doesn't work properly, or why you can't just add a few lines to an
>> existing one.
> Ultimately there are three options I see:
> 1) Do what Xu does, which is re-use the 'uio_pdrv_genirq' uio driver by
>   creating a platform device for it as sub-device of the dfl device that
>   we bind to uio_pdrv_genirq
> 2) Add a module_dfl_driver part to drivers/uio/uio_pdrv_genirq.c and
>   corresponding id table
> 3) Create a new uio_dfl_genirq kind of driver that uses the dfl bus and
>   that would make sense to then put into drivers/uio. (This would
>   duplicate code in uio_pdrv_genirq to some extend)
>
> Overall I think in terms of code re-use I think Xu's choice might be
> less new code as it simply wraps the uio platform device driver, and
> allows for defining the resources passed to the UIO driver to be defined
> by hardware through a DFL.
>
> I've seen the pattern that Xu proposed used in other places like the
> macb network driver where you'd have macb_main (the platform driver) and
> macb_pci that wraps it for a pci usage.
>
> - Moritz

Thinking of this problem more generally.

Every fpga will have a handful of sub devices.

Do we want to carry them in the fpga subsystem or carry them in the other subsystems ?

Consider the short term reviewing and long term maintenance of the sub devices by the subsystem maintainers.

It easier for them if the sub devices are in the other subsystems.


Applying this to specifically for dfl_uio.

No one from the uio subsystem reviewing this change is a problem.

I think this change needs to go to the uio subsystem.

And a new entry in the MAINTAINERS file to keep the fpga subsystem in the loop for reviews and acks

Tom

Re: [PATCH v6 1/2] fpga: dfl: add the userspace I/O device support for DFL devices

[Moritz Fischer]

Hi Tom,

On Thu, Jan 21, 2021 at 06:30:20AM -0800, Tom Rix wrote:
> 
> On 1/17/21 8:22 AM, Moritz Fischer wrote:
> > Greg,
> >
> > On Sun, Jan 17, 2021 at 04:45:04PM +0100, Greg KH wrote:
> >> On Wed, Jan 13, 2021 at 09:54:07AM +0800, Xu Yilun wrote:
> >>> This patch supports the DFL drivers be written in userspace. This is
> >>> realized by exposing the userspace I/O device interfaces.
> >>>
> >>> The driver leverages the uio_pdrv_genirq, it adds the uio_pdrv_genirq
> >>> platform device with the DFL device's resources, and let the generic UIO
> >>> platform device driver provide support to userspace access to kernel
> >>> interrupts and memory locations.
> >> Why doesn't the existing uio driver work for this, why do you need a new
> >> one?
> >>
> >>> ---
> >>>  drivers/fpga/Kconfig        | 10 +++++
> >>>  drivers/fpga/Makefile       |  1 +
> >>>  drivers/fpga/dfl-uio-pdev.c | 93 +++++++++++++++++++++++++++++++++++++++++++++
> >> uio drivers traditionally go in drivers/uio/ and start with "uio", so
> >> shouldn't this be drivers/uio/uio_dfl_pdev.c to match the same naming
> >> scheme?
> > I had considered suggesting that, but ultimately this driver only
> > creates a 'uio_pdrv_genirq' platform device, so it didn't seem like a
> > good fit.
> >> But again, you need to explain in detail, why the existing uio driver
> >> doesn't work properly, or why you can't just add a few lines to an
> >> existing one.
> > Ultimately there are three options I see:
> > 1) Do what Xu does, which is re-use the 'uio_pdrv_genirq' uio driver by
> >   creating a platform device for it as sub-device of the dfl device that
> >   we bind to uio_pdrv_genirq
> > 2) Add a module_dfl_driver part to drivers/uio/uio_pdrv_genirq.c and
> >   corresponding id table
> > 3) Create a new uio_dfl_genirq kind of driver that uses the dfl bus and
> >   that would make sense to then put into drivers/uio. (This would
> >   duplicate code in uio_pdrv_genirq to some extend)
> >
> > Overall I think in terms of code re-use I think Xu's choice might be
> > less new code as it simply wraps the uio platform device driver, and
> > allows for defining the resources passed to the UIO driver to be defined
> > by hardware through a DFL.
> >
> > I've seen the pattern that Xu proposed used in other places like the
> > macb network driver where you'd have macb_main (the platform driver) and
> > macb_pci that wraps it for a pci usage.
> >
> > - Moritz
> 
> Thinking of this problem more generally.
> 
> Every fpga will have a handful of sub devices.
> 
> Do we want to carry them in the fpga subsystem or carry them in the other subsystems ?

Yeah no we really don't. I think that was the point of the whole DFL
bus :)
> 
> Consider the short term reviewing and long term maintenance of the sub devices by the subsystem maintainers.
> 
> It easier for them if the sub devices are in the other subsystems.

Agreed.
> 
> 
> Applying this to specifically for dfl_uio.
> 
> No one from the uio subsystem reviewing this change is a problem.

Greg will.
> I think this change needs to go to the uio subsystem.

Yeah I've thought about this some for the last few days, maybe it's
easier that way.

Tbh, there's so little code here even if we went with option 3 above
it's probably fairly short. It would set a better prcedent.

Xu, how do you feel about giving that a spin? See if option 3 will be
way more code.

- Moritz

Re: [PATCH v6 1/2] fpga: dfl: add the userspace I/O device support for DFL devices

[Xu Yilun]

On Thu, Jan 21, 2021 at 12:03:47PM -0800, Moritz Fischer wrote:
> Hi Tom,
> 
> On Thu, Jan 21, 2021 at 06:30:20AM -0800, Tom Rix wrote:
> > 
> > On 1/17/21 8:22 AM, Moritz Fischer wrote:
> > > Greg,
> > >
> > > On Sun, Jan 17, 2021 at 04:45:04PM +0100, Greg KH wrote:
> > >> On Wed, Jan 13, 2021 at 09:54:07AM +0800, Xu Yilun wrote:
> > >>> This patch supports the DFL drivers be written in userspace. This is
> > >>> realized by exposing the userspace I/O device interfaces.
> > >>>
> > >>> The driver leverages the uio_pdrv_genirq, it adds the uio_pdrv_genirq
> > >>> platform device with the DFL device's resources, and let the generic UIO
> > >>> platform device driver provide support to userspace access to kernel
> > >>> interrupts and memory locations.
> > >> Why doesn't the existing uio driver work for this, why do you need a new
> > >> one?
> > >>
> > >>> ---
> > >>>  drivers/fpga/Kconfig        | 10 +++++
> > >>>  drivers/fpga/Makefile       |  1 +
> > >>>  drivers/fpga/dfl-uio-pdev.c | 93 +++++++++++++++++++++++++++++++++++++++++++++
> > >> uio drivers traditionally go in drivers/uio/ and start with "uio", so
> > >> shouldn't this be drivers/uio/uio_dfl_pdev.c to match the same naming
> > >> scheme?
> > > I had considered suggesting that, but ultimately this driver only
> > > creates a 'uio_pdrv_genirq' platform device, so it didn't seem like a
> > > good fit.
> > >> But again, you need to explain in detail, why the existing uio driver
> > >> doesn't work properly, or why you can't just add a few lines to an
> > >> existing one.
> > > Ultimately there are three options I see:
> > > 1) Do what Xu does, which is re-use the 'uio_pdrv_genirq' uio driver by
> > >   creating a platform device for it as sub-device of the dfl device that
> > >   we bind to uio_pdrv_genirq
> > > 2) Add a module_dfl_driver part to drivers/uio/uio_pdrv_genirq.c and
> > >   corresponding id table
> > > 3) Create a new uio_dfl_genirq kind of driver that uses the dfl bus and
> > >   that would make sense to then put into drivers/uio. (This would
> > >   duplicate code in uio_pdrv_genirq to some extend)
> > >
> > > Overall I think in terms of code re-use I think Xu's choice might be
> > > less new code as it simply wraps the uio platform device driver, and
> > > allows for defining the resources passed to the UIO driver to be defined
> > > by hardware through a DFL.
> > >
> > > I've seen the pattern that Xu proposed used in other places like the
> > > macb network driver where you'd have macb_main (the platform driver) and
> > > macb_pci that wraps it for a pci usage.
> > >
> > > - Moritz
> > 
> > Thinking of this problem more generally.
> > 
> > Every fpga will have a handful of sub devices.
> > 
> > Do we want to carry them in the fpga subsystem or carry them in the other subsystems ?
> 
> Yeah no we really don't. I think that was the point of the whole DFL
> bus :)
> > 
> > Consider the short term reviewing and long term maintenance of the sub devices by the subsystem maintainers.
> > 
> > It easier for them if the sub devices are in the other subsystems.
> 
> Agreed.
> > 
> > 
> > Applying this to specifically for dfl_uio.
> > 
> > No one from the uio subsystem reviewing this change is a problem.
> 
> Greg will.
> > I think this change needs to go to the uio subsystem.
> 
> Yeah I've thought about this some for the last few days, maybe it's
> easier that way.
> 
> Tbh, there's so little code here even if we went with option 3 above
> it's probably fairly short. It would set a better prcedent.
> 
> Xu, how do you feel about giving that a spin? See if option 3 will be
> way more code.

Yes, I'll try to put it to drivers/uio.

I see the implementation in vfio_platform.c/vfio_amba.c/vfio_platform_common.c.
I'm wondering if we could handle it in that way.

Thanks,
yilun