[PATCH 0/2] UIO support for dfl devices

[PATCH 0/2] UIO support for dfl devices

[Xu Yilun]

This patchset supports some dfl device drivers written in userspace.

The usage is like:

 # echo dfl_dev.1 > /sys/bus/dfl/drivers/<kernel driver>/unbind
 # echo dfl-uio-pdev > /sys/bus/dfl/devices/dfl_dev.1/driver_override
 # echo dfl_dev.1 > /sys/bus/dfl/drivers_probe


Xu Yilun (2):
  fpga: dfl: add driver_override support
  fpga: dfl: add the userspace I/O device support for DFL devices

 Documentation/ABI/testing/sysfs-bus-dfl | 28 +++++++++--
 drivers/fpga/Kconfig                    | 10 ++++
 drivers/fpga/Makefile                   |  1 +
 drivers/fpga/dfl-uio-pdev.c             | 83 +++++++++++++++++++++++++++++++++
 drivers/fpga/dfl.c                      | 54 ++++++++++++++++++++-
 include/linux/dfl.h                     |  2 +
 6 files changed, 173 insertions(+), 5 deletions(-)
 create mode 100644 drivers/fpga/dfl-uio-pdev.c

-- 
2.7.4

[PATCH 1/2] fpga: dfl: add driver_override support

[Xu Yilun]

Add support for overriding the default matching of a dfl device to a dfl
driver. It follows the same way that can be used for PCI and platform
devices. This patch adds the 'driver_override' sysfs file.

Signed-off-by: Xu Yilun <yilun.xu@intel.com>
---
 Documentation/ABI/testing/sysfs-bus-dfl | 28 ++++++++++++++---
 drivers/fpga/dfl.c                      | 54 ++++++++++++++++++++++++++++++++-
 include/linux/dfl.h                     |  2 ++
 3 files changed, 79 insertions(+), 5 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-dfl b/Documentation/ABI/testing/sysfs-bus-dfl
index 23543be..db7e8d3 100644
--- a/Documentation/ABI/testing/sysfs-bus-dfl
+++ b/Documentation/ABI/testing/sysfs-bus-dfl
@@ -1,15 +1,35 @@
 What:		/sys/bus/dfl/devices/dfl_dev.X/type
-Date:		Aug 2020
-KernelVersion:	5.10
+Date:		Oct 2020
+KernelVersion:	5.11
 Contact:	Xu Yilun <yilun.xu@intel.com>
 Description:	Read-only. It returns type of DFL FIU of the device. Now DFL
 		supports 2 FIU types, 0 for FME, 1 for PORT.
 		Format: 0x%x
 
 What:		/sys/bus/dfl/devices/dfl_dev.X/feature_id
-Date:		Aug 2020
-KernelVersion:	5.10
+Date:		Oct 2020
+KernelVersion:	5.11
 Contact:	Xu Yilun <yilun.xu@intel.com>
 Description:	Read-only. It returns feature identifier local to its DFL FIU
 		type.
 		Format: 0x%x
+
+What:           /sys/bus/dfl/devices/.../driver_override
+Date:           Oct 2020
+KernelVersion:  5.11
+Contact:        Xu Yilun <yilun.xu@intel.com>
+Description:    This file allows the driver for a device to be specified. When
+                specified, only a driver with a name matching the value written
+                to driver_override will have an opportunity to bind to the
+                device. The override is specified by writing a string to the
+                driver_override file (echo dfl-uio-pdev > driver_override) and
+                may be cleared with an empty string (echo > driver_override).
+                This returns the device to standard matching rules binding.
+                Writing to driver_override does not automatically unbind the
+                device from its current driver or make any attempt to
+                automatically load the specified driver.  If no driver with a
+                matching name is currently loaded in the kernel, the device
+                will not bind to any driver.  This also allows devices to
+                opt-out of driver binding using a driver_override name such as
+                "none".  Only a single driver may be specified in the override,
+                there is no support for parsing delimiters.
diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
index 511b20f..bc35750 100644
--- a/drivers/fpga/dfl.c
+++ b/drivers/fpga/dfl.c
@@ -262,6 +262,10 @@ static int dfl_bus_match(struct device *dev, struct device_driver *drv)
 	struct dfl_driver *ddrv = to_dfl_drv(drv);
 	const struct dfl_device_id *id_entry;
 
+	/* When driver_override is set, only bind to the matching driver */
+	if (ddev->driver_override)
+		return !strcmp(ddev->driver_override, drv->name);
+
 	id_entry = ddrv->id_table;
 	if (id_entry) {
 		while (id_entry->feature_id) {
@@ -303,6 +307,53 @@ static int dfl_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
 			      ddev->type, ddev->feature_id);
 }
 
+static ssize_t driver_override_show(struct device *dev,
+				    struct device_attribute *attr, char *buf)
+{
+	struct dfl_device *ddev = to_dfl_dev(dev);
+	ssize_t len;
+
+	device_lock(dev);
+	len = sprintf(buf, "%s\n", ddev->driver_override);
+	device_unlock(dev);
+	return len;
+}
+
+static ssize_t driver_override_store(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t count)
+{
+	struct dfl_device *ddev = to_dfl_dev(dev);
+	char *driver_override, *old, *cp;
+
+	/* We need to keep extra room for a newline */
+	if (count >= (PAGE_SIZE - 1))
+		return -EINVAL;
+
+	driver_override = kstrndup(buf, count, GFP_KERNEL);
+	if (!driver_override)
+		return -ENOMEM;
+
+	cp = strchr(driver_override, '\n');
+	if (cp)
+		*cp = '\0';
+
+	device_lock(dev);
+	old = ddev->driver_override;
+	if (strlen(driver_override)) {
+		ddev->driver_override = driver_override;
+	} else {
+		kfree(driver_override);
+		ddev->driver_override = NULL;
+	}
+	device_unlock(dev);
+
+	kfree(old);
+
+	return count;
+}
+static DEVICE_ATTR_RW(driver_override);
+
 static ssize_t
 type_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
@@ -324,6 +375,7 @@ static DEVICE_ATTR_RO(feature_id);
 static struct attribute *dfl_dev_attrs[] = {
 	&dev_attr_type.attr,
 	&dev_attr_feature_id.attr,
+	&dev_attr_driver_override.attr,
 	NULL,
 };
 ATTRIBUTE_GROUPS(dfl_dev);
@@ -469,7 +521,7 @@ static int dfl_devs_add(struct dfl_feature_platform_data *pdata)
 
 int __dfl_driver_register(struct dfl_driver *dfl_drv, struct module *owner)
 {
-	if (!dfl_drv || !dfl_drv->probe || !dfl_drv->id_table)
+	if (!dfl_drv || !dfl_drv->probe)
 		return -EINVAL;
 
 	dfl_drv->drv.owner = owner;
diff --git a/include/linux/dfl.h b/include/linux/dfl.h
index 7affba2f..e1b2471 100644
--- a/include/linux/dfl.h
+++ b/include/linux/dfl.h
@@ -32,6 +32,7 @@ enum dfl_id_type {
  * @num_irqs: number of IRQs supported by this dfl device.
  * @cdev: pointer to DFL FPGA container device this dfl device belongs to.
  * @id_entry: matched id entry in dfl driver's id table.
+ * @driver_override: driver name to force a match
  */
 struct dfl_device {
 	struct device dev;
@@ -43,6 +44,7 @@ struct dfl_device {
 	unsigned int num_irqs;
 	struct dfl_fpga_cdev *cdev;
 	const struct dfl_device_id *id_entry;
+	char *driver_override;
 };
 
 /**
-- 
2.7.4

[PATCH 2/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.

Signed-off-by: Xu Yilun <yilun.xu@intel.com>
---
 drivers/fpga/Kconfig        | 10 ++++++
 drivers/fpga/Makefile       |  1 +
 drivers/fpga/dfl-uio-pdev.c | 83 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 94 insertions(+)
 create mode 100644 drivers/fpga/dfl-uio-pdev.c

diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index 5d7f0ae..e054722 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -202,6 +202,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 device's
+	  resources, and let the generic UIO platform device driver provide
+	  support to 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..e07b3d5 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..d35b846
--- /dev/null
+++ b/drivers/fpga/dfl-uio-pdev.c
@@ -0,0 +1,83 @@
+// 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 device *dev = &ddev->dev;
+	struct platform_device_info pdevinfo = { 0 };
+	struct uio_info uio_pdata = { 0 };
+	struct platform_device *uio_pdev;
+	struct resource *res;
+	int i, idx = 0;
+
+	pdevinfo.name = "uio_pdrv_genirq";
+
+	res = kcalloc(ddev->num_irqs + 1, sizeof(*res), GFP_KERNEL);
+	if (!res)
+		return -ENOMEM;
+
+	res[idx].parent = &ddev->mmio_res;
+	res[idx].flags = IORESOURCE_MEM;
+	res[idx].start = ddev->mmio_res.start;
+	res[idx].end = ddev->mmio_res.end;
+	++idx;
+
+	/* then add irq resource */
+	for (i = 0; i < ddev->num_irqs; i++) {
+		res[idx].flags = IORESOURCE_IRQ;
+		res[idx].start = ddev->irqs[i];
+		res[idx].end = ddev->irqs[i];
+		++idx;
+	}
+
+	uio_pdata.name = DRIVER_NAME;
+	uio_pdata.version = "0";
+
+	pdevinfo.res = res;
+	pdevinfo.num_res = idx;
+	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))
+		dev_set_drvdata(dev, uio_pdev);
+
+	kfree(res);
+
+	return PTR_ERR_OR_ZERO(uio_pdev);
+}
+
+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);
+}
+
+static struct dfl_driver dfl_uio_pdev_driver = {
+	.drv	= {
+		.name       = DRIVER_NAME,
+	},
+	.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

Re: [PATCH 1/2] fpga: dfl: add driver_override support

[Tom Rix]

On 10/15/20 11:02 PM, Xu Yilun wrote:
> Add support for overriding the default matching of a dfl device to a dfl
> driver. It follows the same way that can be used for PCI and platform
> devices. This patch adds the 'driver_override' sysfs file.
>
> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> ---
>  Documentation/ABI/testing/sysfs-bus-dfl | 28 ++++++++++++++---
>  drivers/fpga/dfl.c                      | 54 ++++++++++++++++++++++++++++++++-
>  include/linux/dfl.h                     |  2 ++
>  3 files changed, 79 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-dfl b/Documentation/ABI/testing/sysfs-bus-dfl
> index 23543be..db7e8d3 100644
> --- a/Documentation/ABI/testing/sysfs-bus-dfl
> +++ b/Documentation/ABI/testing/sysfs-bus-dfl
> @@ -1,15 +1,35 @@
>  What:		/sys/bus/dfl/devices/dfl_dev.X/type
> -Date:		Aug 2020
> -KernelVersion:	5.10
> +Date:		Oct 2020
> +KernelVersion:	5.11
>  Contact:	Xu Yilun <yilun.xu@intel.com>
>  Description:	Read-only. It returns type of DFL FIU of the device. Now DFL
>  		supports 2 FIU types, 0 for FME, 1 for PORT.
>  		Format: 0x%x
>  
>  What:		/sys/bus/dfl/devices/dfl_dev.X/feature_id
> -Date:		Aug 2020
> -KernelVersion:	5.10
> +Date:		Oct 2020
> +KernelVersion:	5.11
>  Contact:	Xu Yilun <yilun.xu@intel.com>
>  Description:	Read-only. It returns feature identifier local to its DFL FIU
>  		type.
>  		Format: 0x%x

These updates, do not match the comment.

Consider splitting this out.

> +
> +What:           /sys/bus/dfl/devices/.../driver_override
> +Date:           Oct 2020
> +KernelVersion:  5.11
> +Contact:        Xu Yilun <yilun.xu@intel.com>
I am looking at description and trying to make it consistent with sysfs-bus-pci
> +Description:    This file allows the driver for a device to be specified.

'to be specified which will override the standard dfl bus feature id to driver mapping.'


>  When
> +                specified, only a driver with a name matching the value written
> +                to driver_override will have an opportunity to bind to the
> +                device. The override is specified by writing a string to the
> +                driver_override file (echo dfl-uio-pdev > driver_override) and
> +                may be cleared with an empty string (echo > driver_override).
> +                This returns the device to standard matching rules binding.
> +                Writing to driver_override does not automatically unbind the
> +                device from its current driver or make any attempt to
> +                automatically load the specified driver.  If no driver with a
> +                matching name is currently loaded in the kernel, the device
> +                will not bind to any driver.  This also allows devices to
> +                opt-out of driver binding using a driver_override name such as
> +                "none".  Only a single driver may be specified in the override,
> +                there is no support for parsing delimiters.
> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> index 511b20f..bc35750 100644
> --- a/drivers/fpga/dfl.c
> +++ b/drivers/fpga/dfl.c
> @@ -262,6 +262,10 @@ static int dfl_bus_match(struct device *dev, struct device_driver *drv)
>  	struct dfl_driver *ddrv = to_dfl_drv(drv);
>  	const struct dfl_device_id *id_entry;
>  
> +	/* When driver_override is set, only bind to the matching driver */
> +	if (ddev->driver_override)
> +		return !strcmp(ddev->driver_override, drv->name);
> +
>  	id_entry = ddrv->id_table;
>  	if (id_entry) {
>  		while (id_entry->feature_id) {
> @@ -303,6 +307,53 @@ static int dfl_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
>  			      ddev->type, ddev->feature_id);
>  }
>  

I am looking at other implementations of driver_override* and looking for consistency.

> +static ssize_t driver_override_show(struct device *dev,
> +				    struct device_attribute *attr, char *buf)
> +{
> +	struct dfl_device *ddev = to_dfl_dev(dev);
> +	ssize_t len;
> +
> +	device_lock(dev);
> +	len = sprintf(buf, "%s\n", ddev->driver_override);
len = snprintf(buf, PAGE_SIZE ...
> +	device_unlock(dev);
> +	return len;
> +}
> +
> +static ssize_t driver_override_store(struct device *dev,
> +				     struct device_attribute *attr,
> +				     const char *buf, size_t count)
> +{
> +	struct dfl_device *ddev = to_dfl_dev(dev);
> +	char *driver_override, *old, *cp;
> +
> +	/* We need to keep extra room for a newline */
> +	if (count >= (PAGE_SIZE - 1))
> +		return -EINVAL;
> +
> +	driver_override = kstrndup(buf, count, GFP_KERNEL);
> +	if (!driver_override)
> +		return -ENOMEM;
> +
> +	cp = strchr(driver_override, '\n');
> +	if (cp)
> +		*cp = '\0';
> +
> +	device_lock(dev);
> +	old = ddev->driver_override;
> +	if (strlen(driver_override)) {
> +		ddev->driver_override = driver_override;
> +	} else {
> +		kfree(driver_override);
> +		ddev->driver_override = NULL;
> +	}
> +	device_unlock(dev);
> +
> +	kfree(old);
> +
> +	return count;
> +}
> +static DEVICE_ATTR_RW(driver_override);
> +
>  static ssize_t
>  type_show(struct device *dev, struct device_attribute *attr, char *buf)
>  {
> @@ -324,6 +375,7 @@ static DEVICE_ATTR_RO(feature_id);
>  static struct attribute *dfl_dev_attrs[] = {
>  	&dev_attr_type.attr,
>  	&dev_attr_feature_id.attr,
> +	&dev_attr_driver_override.attr,
>  	NULL,
>  };
>  ATTRIBUTE_GROUPS(dfl_dev);
> @@ -469,7 +521,7 @@ static int dfl_devs_add(struct dfl_feature_platform_data *pdata)
>  
>  int __dfl_driver_register(struct dfl_driver *dfl_drv, struct module *owner)
>  {
> -	if (!dfl_drv || !dfl_drv->probe || !dfl_drv->id_table)
> +	if (!dfl_drv || !dfl_drv->probe)

id_table is still needed for the normal case.

Instead of removing this check, could you add something like

|| (!dfl_drv->is_override && !dfl_drv->id_table)

Tom

>  		return -EINVAL;
>  
>  	dfl_drv->drv.owner = owner;
> diff --git a/include/linux/dfl.h b/include/linux/dfl.h
> index 7affba2f..e1b2471 100644
> --- a/include/linux/dfl.h
> +++ b/include/linux/dfl.h
> @@ -32,6 +32,7 @@ enum dfl_id_type {
>   * @num_irqs: number of IRQs supported by this dfl device.
>   * @cdev: pointer to DFL FPGA container device this dfl device belongs to.
>   * @id_entry: matched id entry in dfl driver's id table.
> + * @driver_override: driver name to force a match
>   */
>  struct dfl_device {
>  	struct device dev;
> @@ -43,6 +44,7 @@ struct dfl_device {
>  	unsigned int num_irqs;
>  	struct dfl_fpga_cdev *cdev;
>  	const struct dfl_device_id *id_entry;
> +	char *driver_override;
>  };
>  
>  /**

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

[Tom Rix]

On 10/15/20 11:02 PM, 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.
>
> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> ---
>  drivers/fpga/Kconfig        | 10 ++++++
>  drivers/fpga/Makefile       |  1 +
>  drivers/fpga/dfl-uio-pdev.c | 83 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 94 insertions(+)
>  create mode 100644 drivers/fpga/dfl-uio-pdev.c
>
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index 5d7f0ae..e054722 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -202,6 +202,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 device's
> +	  resources, and let the generic UIO platform device driver provide
'and lets the'
> +	  support to 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..e07b3d5 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..d35b846
> --- /dev/null
> +++ b/drivers/fpga/dfl-uio-pdev.c
> @@ -0,0 +1,83 @@
> +// 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 device *dev = &ddev->dev;
> +	struct platform_device_info pdevinfo = { 0 };
> +	struct uio_info uio_pdata = { 0 };
> +	struct platform_device *uio_pdev;
> +	struct resource *res;
> +	int i, idx = 0;

idx is not needed.

> +
> +	pdevinfo.name = "uio_pdrv_genirq";
> +
> +	res = kcalloc(ddev->num_irqs + 1, sizeof(*res), GFP_KERNEL);
> +	if (!res)
> +		return -ENOMEM;
> +
> +	res[idx].parent = &ddev->mmio_res;
res[0].parent =
> +	res[idx].flags = IORESOURCE_MEM;
> +	res[idx].start = ddev->mmio_res.start;
> +	res[idx].end = ddev->mmio_res.end;
> +	++idx;
> +
> +	/* then add irq resource */
> +	for (i = 0; i < ddev->num_irqs; i++) {
> +		res[idx].flags = IORESOURCE_IRQ;

res[i+1].flags =

Tom

> +		res[idx].start = ddev->irqs[i];
> +		res[idx].end = ddev->irqs[i];
> +		++idx;
> +	}
> +
> +	uio_pdata.name = DRIVER_NAME;
> +	uio_pdata.version = "0";
> +
> +	pdevinfo.res = res;
> +	pdevinfo.num_res = idx;
> +	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))
> +		dev_set_drvdata(dev, uio_pdev);
> +
> +	kfree(res);
> +
> +	return PTR_ERR_OR_ZERO(uio_pdev);
> +}
> +
> +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);
> +}
> +
> +static struct dfl_driver dfl_uio_pdev_driver = {
> +	.drv	= {
> +		.name       = DRIVER_NAME,
> +	},
> +	.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");

Re: [PATCH 0/2] UIO support for dfl devices

[Tom Rix]

On 10/15/20 11:02 PM, Xu Yilun wrote:
> This patchset supports some dfl device drivers written in userspace.
>
> The usage is like:
>
>  # echo dfl_dev.1 > /sys/bus/dfl/drivers/<kernel driver>/unbind
>  # echo dfl-uio-pdev > /sys/bus/dfl/devices/dfl_dev.1/driver_override
>  # echo dfl_dev.1 > /sys/bus/dfl/drivers_probe
>
>
> Xu Yilun (2):
>   fpga: dfl: add driver_override support
>   fpga: dfl: add the userspace I/O device support for DFL devices
>
>  Documentation/ABI/testing/sysfs-bus-dfl | 28 +++++++++--
>  drivers/fpga/Kconfig                    | 10 ++++
>  drivers/fpga/Makefile                   |  1 +
>  drivers/fpga/dfl-uio-pdev.c             | 83 +++++++++++++++++++++++++++++++++
>  drivers/fpga/dfl.c                      | 54 ++++++++++++++++++++-
>  include/linux/dfl.h                     |  2 +
>  6 files changed, 173 insertions(+), 5 deletions(-)
>  create mode 100644 drivers/fpga/dfl-uio-pdev.c

This is a neat new feature.

Should something be added to Documentation/fpga/dfl.rst ?

Overall, patchset looks good.

Tom

Re: [PATCH 1/2] fpga: dfl: add driver_override support

[Xu Yilun]

On Fri, Oct 16, 2020 at 09:21:50AM -0700, Tom Rix wrote:
> 
> On 10/15/20 11:02 PM, Xu Yilun wrote:
> > Add support for overriding the default matching of a dfl device to a dfl
> > driver. It follows the same way that can be used for PCI and platform
> > devices. This patch adds the 'driver_override' sysfs file.
> >
> > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > ---
> >  Documentation/ABI/testing/sysfs-bus-dfl | 28 ++++++++++++++---
> >  drivers/fpga/dfl.c                      | 54 ++++++++++++++++++++++++++++++++-
> >  include/linux/dfl.h                     |  2 ++
> >  3 files changed, 79 insertions(+), 5 deletions(-)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-dfl b/Documentation/ABI/testing/sysfs-bus-dfl
> > index 23543be..db7e8d3 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-dfl
> > +++ b/Documentation/ABI/testing/sysfs-bus-dfl
> > @@ -1,15 +1,35 @@
> >  What:		/sys/bus/dfl/devices/dfl_dev.X/type
> > -Date:		Aug 2020
> > -KernelVersion:	5.10
> > +Date:		Oct 2020
> > +KernelVersion:	5.11
> >  Contact:	Xu Yilun <yilun.xu@intel.com>
> >  Description:	Read-only. It returns type of DFL FIU of the device. Now DFL
> >  		supports 2 FIU types, 0 for FME, 1 for PORT.
> >  		Format: 0x%x
> >  
> >  What:		/sys/bus/dfl/devices/dfl_dev.X/feature_id
> > -Date:		Aug 2020
> > -KernelVersion:	5.10
> > +Date:		Oct 2020
> > +KernelVersion:	5.11
> >  Contact:	Xu Yilun <yilun.xu@intel.com>
> >  Description:	Read-only. It returns feature identifier local to its DFL FIU
> >  		type.
> >  		Format: 0x%x
> 
> These updates, do not match the comment.
> 
> Consider splitting this out.

I'm sorry it's a typo. The above code should not be changed.

> 
> > +
> > +What:           /sys/bus/dfl/devices/.../driver_override
> > +Date:           Oct 2020
> > +KernelVersion:  5.11
> > +Contact:        Xu Yilun <yilun.xu@intel.com>
> I am looking at description and trying to make it consistent with sysfs-bus-pci
> > +Description:    This file allows the driver for a device to be specified.
> 
> 'to be specified which will override the standard dfl bus feature id to driver mapping.'

Yes, it could be improved.

Actually now it is the "type" and "feature id" matching, the 2 fields
are defined for dfl_driver.id_table. In future for dfl v1, it may be
GUID matching, which will be added to id_table. So how about we make it
more generic:

'to be specified which will override the standard ID table matching.'

> 
> 
> >  When
> > +                specified, only a driver with a name matching the value written
> > +                to driver_override will have an opportunity to bind to the
> > +                device. The override is specified by writing a string to the
> > +                driver_override file (echo dfl-uio-pdev > driver_override) and
> > +                may be cleared with an empty string (echo > driver_override).
> > +                This returns the device to standard matching rules binding.
> > +                Writing to driver_override does not automatically unbind the
> > +                device from its current driver or make any attempt to
> > +                automatically load the specified driver.  If no driver with a
> > +                matching name is currently loaded in the kernel, the device
> > +                will not bind to any driver.  This also allows devices to
> > +                opt-out of driver binding using a driver_override name such as
> > +                "none".  Only a single driver may be specified in the override,
> > +                there is no support for parsing delimiters.
> > diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> > index 511b20f..bc35750 100644
> > --- a/drivers/fpga/dfl.c
> > +++ b/drivers/fpga/dfl.c
> > @@ -262,6 +262,10 @@ static int dfl_bus_match(struct device *dev, struct device_driver *drv)
> >  	struct dfl_driver *ddrv = to_dfl_drv(drv);
> >  	const struct dfl_device_id *id_entry;
> >  
> > +	/* When driver_override is set, only bind to the matching driver */
> > +	if (ddev->driver_override)
> > +		return !strcmp(ddev->driver_override, drv->name);
> > +
> >  	id_entry = ddrv->id_table;
> >  	if (id_entry) {
> >  		while (id_entry->feature_id) {
> > @@ -303,6 +307,53 @@ static int dfl_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
> >  			      ddev->type, ddev->feature_id);
> >  }
> >  
> 
> I am looking at other implementations of driver_override* and looking for consistency.
> 
> > +static ssize_t driver_override_show(struct device *dev,
> > +				    struct device_attribute *attr, char *buf)
> > +{
> > +	struct dfl_device *ddev = to_dfl_dev(dev);
> > +	ssize_t len;
> > +
> > +	device_lock(dev);
> > +	len = sprintf(buf, "%s\n", ddev->driver_override);
> len = snprintf(buf, PAGE_SIZE ...

It is good to me.

Some bus drivers use snprintf, some use sprintf.

I think it is reasonable snprintf is used here, unlike %d, %u ... it is
uncertain for the output size of %s.

> > +	device_unlock(dev);
> > +	return len;
> > +}
> > +
> > +static ssize_t driver_override_store(struct device *dev,
> > +				     struct device_attribute *attr,
> > +				     const char *buf, size_t count)
> > +{
> > +	struct dfl_device *ddev = to_dfl_dev(dev);
> > +	char *driver_override, *old, *cp;
> > +
> > +	/* We need to keep extra room for a newline */
> > +	if (count >= (PAGE_SIZE - 1))
> > +		return -EINVAL;
> > +
> > +	driver_override = kstrndup(buf, count, GFP_KERNEL);
> > +	if (!driver_override)
> > +		return -ENOMEM;
> > +
> > +	cp = strchr(driver_override, '\n');
> > +	if (cp)
> > +		*cp = '\0';
> > +
> > +	device_lock(dev);
> > +	old = ddev->driver_override;
> > +	if (strlen(driver_override)) {
> > +		ddev->driver_override = driver_override;
> > +	} else {
> > +		kfree(driver_override);
> > +		ddev->driver_override = NULL;
> > +	}
> > +	device_unlock(dev);
> > +
> > +	kfree(old);
> > +
> > +	return count;
> > +}
> > +static DEVICE_ATTR_RW(driver_override);
> > +
> >  static ssize_t
> >  type_show(struct device *dev, struct device_attribute *attr, char *buf)
> >  {
> > @@ -324,6 +375,7 @@ static DEVICE_ATTR_RO(feature_id);
> >  static struct attribute *dfl_dev_attrs[] = {
> >  	&dev_attr_type.attr,
> >  	&dev_attr_feature_id.attr,
> > +	&dev_attr_driver_override.attr,
> >  	NULL,
> >  };
> >  ATTRIBUTE_GROUPS(dfl_dev);
> > @@ -469,7 +521,7 @@ static int dfl_devs_add(struct dfl_feature_platform_data *pdata)
> >  
> >  int __dfl_driver_register(struct dfl_driver *dfl_drv, struct module *owner)
> >  {
> > -	if (!dfl_drv || !dfl_drv->probe || !dfl_drv->id_table)
> > +	if (!dfl_drv || !dfl_drv->probe)
> 
> id_table is still needed for the normal case.
> 
> Instead of removing this check, could you add something like
> 
> || (!dfl_drv->is_override && !dfl_drv->id_table)

I don't think it is needed. Seems is_override and !id_table are duplicated
conditions for this implementation. And it may make confusing, e.g. could
a driver been force matched when is_override is not set?

I think we could make it simple, if the dfl driver didn't provide the
id_table, normally it could not match any device. I think it could be
easily understood by dfl driver developers.


Thanks,
Yilun

> 
> Tom
> 
> >  		return -EINVAL;
> >  
> >  	dfl_drv->drv.owner = owner;
> > diff --git a/include/linux/dfl.h b/include/linux/dfl.h
> > index 7affba2f..e1b2471 100644
> > --- a/include/linux/dfl.h
> > +++ b/include/linux/dfl.h
> > @@ -32,6 +32,7 @@ enum dfl_id_type {
> >   * @num_irqs: number of IRQs supported by this dfl device.
> >   * @cdev: pointer to DFL FPGA container device this dfl device belongs to.
> >   * @id_entry: matched id entry in dfl driver's id table.
> > + * @driver_override: driver name to force a match
> >   */
> >  struct dfl_device {
> >  	struct device dev;
> > @@ -43,6 +44,7 @@ struct dfl_device {
> >  	unsigned int num_irqs;
> >  	struct dfl_fpga_cdev *cdev;
> >  	const struct dfl_device_id *id_entry;
> > +	char *driver_override;
> >  };
> >  
> >  /**

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

[Xu Yilun]

On Fri, Oct 16, 2020 at 09:36:00AM -0700, Tom Rix wrote:
> 
> On 10/15/20 11:02 PM, 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.
> >
> > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > ---
> >  drivers/fpga/Kconfig        | 10 ++++++
> >  drivers/fpga/Makefile       |  1 +
> >  drivers/fpga/dfl-uio-pdev.c | 83 +++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 94 insertions(+)
> >  create mode 100644 drivers/fpga/dfl-uio-pdev.c
> >
> > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> > index 5d7f0ae..e054722 100644
> > --- a/drivers/fpga/Kconfig
> > +++ b/drivers/fpga/Kconfig
> > @@ -202,6 +202,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 device's
> > +	  resources, and let the generic UIO platform device driver provide
> 'and lets the'

Yes.

> > +	  support to 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..e07b3d5 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..d35b846
> > --- /dev/null
> > +++ b/drivers/fpga/dfl-uio-pdev.c
> > @@ -0,0 +1,83 @@
> > +// 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 device *dev = &ddev->dev;
> > +	struct platform_device_info pdevinfo = { 0 };
> > +	struct uio_info uio_pdata = { 0 };
> > +	struct platform_device *uio_pdev;
> > +	struct resource *res;
> > +	int i, idx = 0;
> 
> idx is not needed.

I could remove the idx. But I think I could ++res during the resource
filling.

Thanks,
Yilun

> 
> > +
> > +	pdevinfo.name = "uio_pdrv_genirq";
> > +
> > +	res = kcalloc(ddev->num_irqs + 1, sizeof(*res), GFP_KERNEL);
> > +	if (!res)
> > +		return -ENOMEM;
> > +
> > +	res[idx].parent = &ddev->mmio_res;
> res[0].parent =
> > +	res[idx].flags = IORESOURCE_MEM;
> > +	res[idx].start = ddev->mmio_res.start;
> > +	res[idx].end = ddev->mmio_res.end;
> > +	++idx;
> > +
> > +	/* then add irq resource */
> > +	for (i = 0; i < ddev->num_irqs; i++) {
> > +		res[idx].flags = IORESOURCE_IRQ;
> 
> res[i+1].flags =
> 
> Tom
> 
> > +		res[idx].start = ddev->irqs[i];
> > +		res[idx].end = ddev->irqs[i];
> > +		++idx;
> > +	}
> > +
> > +	uio_pdata.name = DRIVER_NAME;
> > +	uio_pdata.version = "0";
> > +
> > +	pdevinfo.res = res;
> > +	pdevinfo.num_res = idx;
> > +	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))
> > +		dev_set_drvdata(dev, uio_pdev);
> > +
> > +	kfree(res);
> > +
> > +	return PTR_ERR_OR_ZERO(uio_pdev);
> > +}
> > +
> > +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);
> > +}
> > +
> > +static struct dfl_driver dfl_uio_pdev_driver = {
> > +	.drv	= {
> > +		.name       = DRIVER_NAME,
> > +	},
> > +	.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");

Re: [PATCH 0/2] UIO support for dfl devices

[Xu Yilun]

On Fri, Oct 16, 2020 at 09:40:03AM -0700, Tom Rix wrote:
> 
> On 10/15/20 11:02 PM, Xu Yilun wrote:
> > This patchset supports some dfl device drivers written in userspace.
> >
> > The usage is like:
> >
> >  # echo dfl_dev.1 > /sys/bus/dfl/drivers/<kernel driver>/unbind
> >  # echo dfl-uio-pdev > /sys/bus/dfl/devices/dfl_dev.1/driver_override
> >  # echo dfl_dev.1 > /sys/bus/dfl/drivers_probe
> >
> >
> > Xu Yilun (2):
> >   fpga: dfl: add driver_override support
> >   fpga: dfl: add the userspace I/O device support for DFL devices
> >
> >  Documentation/ABI/testing/sysfs-bus-dfl | 28 +++++++++--
> >  drivers/fpga/Kconfig                    | 10 ++++
> >  drivers/fpga/Makefile                   |  1 +
> >  drivers/fpga/dfl-uio-pdev.c             | 83 +++++++++++++++++++++++++++++++++
> >  drivers/fpga/dfl.c                      | 54 ++++++++++++++++++++-
> >  include/linux/dfl.h                     |  2 +
> >  6 files changed, 173 insertions(+), 5 deletions(-)
> >  create mode 100644 drivers/fpga/dfl-uio-pdev.c
> 
> This is a neat new feature.
> 
> Should something be added to Documentation/fpga/dfl.rst ?

Yes, I could add the Doc.

> 
> Overall, patchset looks good.
> 
> Tom

RE: [PATCH 1/2] fpga: dfl: add driver_override support

[Wu, Hao]

> On Fri, Oct 16, 2020 at 09:21:50AM -0700, Tom Rix wrote:
> >
> > On 10/15/20 11:02 PM, Xu Yilun wrote:
> > > Add support for overriding the default matching of a dfl device to a dfl
> > > driver. It follows the same way that can be used for PCI and platform
> > > devices. This patch adds the 'driver_override' sysfs file.
> > >
> > > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > > ---
> > >  Documentation/ABI/testing/sysfs-bus-dfl | 28 ++++++++++++++---
> > >  drivers/fpga/dfl.c                      | 54
> ++++++++++++++++++++++++++++++++-
> > >  include/linux/dfl.h                     |  2 ++
> > >  3 files changed, 79 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/Documentation/ABI/testing/sysfs-bus-dfl
> b/Documentation/ABI/testing/sysfs-bus-dfl
> > > index 23543be..db7e8d3 100644
> > > --- a/Documentation/ABI/testing/sysfs-bus-dfl
> > > +++ b/Documentation/ABI/testing/sysfs-bus-dfl
> > > @@ -1,15 +1,35 @@
> > >  What:		/sys/bus/dfl/devices/dfl_dev.X/type
> > > -Date:		Aug 2020
> > > -KernelVersion:	5.10
> > > +Date:		Oct 2020
> > > +KernelVersion:	5.11
> > >  Contact:	Xu Yilun <yilun.xu@intel.com>
> > >  Description:	Read-only. It returns type of DFL FIU of the device.
> Now DFL
> > >  		supports 2 FIU types, 0 for FME, 1 for PORT.
> > >  		Format: 0x%x
> > >
> > >  What:		/sys/bus/dfl/devices/dfl_dev.X/feature_id
> > > -Date:		Aug 2020
> > > -KernelVersion:	5.10
> > > +Date:		Oct 2020
> > > +KernelVersion:	5.11
> > >  Contact:	Xu Yilun <yilun.xu@intel.com>
> > >  Description:	Read-only. It returns feature identifier local to its DFL
> FIU
> > >  		type.
> > >  		Format: 0x%x
> >
> > These updates, do not match the comment.
> >
> > Consider splitting this out.
> 
> I'm sorry it's a typo. The above code should not be changed.
> 
> >
> > > +
> > > +What:           /sys/bus/dfl/devices/.../driver_override
> > > +Date:           Oct 2020
> > > +KernelVersion:  5.11
> > > +Contact:        Xu Yilun <yilun.xu@intel.com>
> > I am looking at description and trying to make it consistent with sysfs-bus-
> pci
> > > +Description:    This file allows the driver for a device to be specified.
> >
> > 'to be specified which will override the standard dfl bus feature id to driver
> mapping.'
> 
> Yes, it could be improved.
> 
> Actually now it is the "type" and "feature id" matching, the 2 fields
> are defined for dfl_driver.id_table. In future for dfl v1, it may be
> GUID matching, which will be added to id_table. So how about we make it
> more generic:
> 
> 'to be specified which will override the standard ID table matching.'
> 
> >
> >
> > >  When
> > > +                specified, only a driver with a name matching the value written
> > > +                to driver_override will have an opportunity to bind to the
> > > +                device. The override is specified by writing a string to the
> > > +                driver_override file (echo dfl-uio-pdev > driver_override) and
> > > +                may be cleared with an empty string (echo > driver_override).
> > > +                This returns the device to standard matching rules binding.
> > > +                Writing to driver_override does not automatically unbind the
> > > +                device from its current driver or make any attempt to
> > > +                automatically load the specified driver.  If no driver with a
> > > +                matching name is currently loaded in the kernel, the device
> > > +                will not bind to any driver.  This also allows devices to
> > > +                opt-out of driver binding using a driver_override name such as
> > > +                "none".  Only a single driver may be specified in the override,
> > > +                there is no support for parsing delimiters.
> > > diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> > > index 511b20f..bc35750 100644
> > > --- a/drivers/fpga/dfl.c
> > > +++ b/drivers/fpga/dfl.c
> > > @@ -262,6 +262,10 @@ static int dfl_bus_match(struct device *dev,
> struct device_driver *drv)
> > >  	struct dfl_driver *ddrv = to_dfl_drv(drv);
> > >  	const struct dfl_device_id *id_entry;
> > >
> > > +	/* When driver_override is set, only bind to the matching driver */
> > > +	if (ddev->driver_override)
> > > +		return !strcmp(ddev->driver_override, drv->name);
> > > +
> > >  	id_entry = ddrv->id_table;
> > >  	if (id_entry) {
> > >  		while (id_entry->feature_id) {
> > > @@ -303,6 +307,53 @@ static int dfl_bus_uevent(struct device *dev,
> struct kobj_uevent_env *env)
> > >  			      ddev->type, ddev->feature_id);
> > >  }
> > >
> >
> > I am looking at other implementations of driver_override* and looking for
> consistency.
> >
> > > +static ssize_t driver_override_show(struct device *dev,
> > > +				    struct device_attribute *attr, char *buf)
> > > +{
> > > +	struct dfl_device *ddev = to_dfl_dev(dev);
> > > +	ssize_t len;
> > > +
> > > +	device_lock(dev);
> > > +	len = sprintf(buf, "%s\n", ddev->driver_override);
> > len = snprintf(buf, PAGE_SIZE ...
> 
> It is good to me.
> 
> Some bus drivers use snprintf, some use sprintf.
> 
> I think it is reasonable snprintf is used here, unlike %d, %u ... it is
> uncertain for the output size of %s.

you limited the size < a page in store function for driver_override?

Hao

> 
> > > +	device_unlock(dev);
> > > +	return len;
> > > +}
> > > +
> > > +static ssize_t driver_override_store(struct device *dev,
> > > +				     struct device_attribute *attr,
> > > +				     const char *buf, size_t count)
> > > +{
> > > +	struct dfl_device *ddev = to_dfl_dev(dev);
> > > +	char *driver_override, *old, *cp;
> > > +
> > > +	/* We need to keep extra room for a newline */
> > > +	if (count >= (PAGE_SIZE - 1))
> > > +		return -EINVAL;
> > > +
> > > +	driver_override = kstrndup(buf, count, GFP_KERNEL);
> > > +	if (!driver_override)
> > > +		return -ENOMEM;
> > > +
> > > +	cp = strchr(driver_override, '\n');
> > > +	if (cp)
> > > +		*cp = '\0';
> > > +
> > > +	device_lock(dev);
> > > +	old = ddev->driver_override;
> > > +	if (strlen(driver_override)) {
> > > +		ddev->driver_override = driver_override;
> > > +	} else {
> > > +		kfree(driver_override);
> > > +		ddev->driver_override = NULL;
> > > +	}
> > > +	device_unlock(dev);
> > > +
> > > +	kfree(old);
> > > +
> > > +	return count;
> > > +}
> > > +static DEVICE_ATTR_RW(driver_override);
> > > +
> > >  static ssize_t
> > >  type_show(struct device *dev, struct device_attribute *attr, char *buf)
> > >  {
> > > @@ -324,6 +375,7 @@ static DEVICE_ATTR_RO(feature_id);
> > >  static struct attribute *dfl_dev_attrs[] = {
> > >  	&dev_attr_type.attr,
> > >  	&dev_attr_feature_id.attr,
> > > +	&dev_attr_driver_override.attr,
> > >  	NULL,
> > >  };
> > >  ATTRIBUTE_GROUPS(dfl_dev);
> > > @@ -469,7 +521,7 @@ static int dfl_devs_add(struct
> dfl_feature_platform_data *pdata)
> > >
> > >  int __dfl_driver_register(struct dfl_driver *dfl_drv, struct module *owner)
> > >  {
> > > -	if (!dfl_drv || !dfl_drv->probe || !dfl_drv->id_table)
> > > +	if (!dfl_drv || !dfl_drv->probe)
> >
> > id_table is still needed for the normal case.
> >
> > Instead of removing this check, could you add something like
> >
> > || (!dfl_drv->is_override && !dfl_drv->id_table)
> 
> I don't think it is needed. Seems is_override and !id_table are duplicated
> conditions for this implementation. And it may make confusing, e.g. could
> a driver been force matched when is_override is not set?
> 
> I think we could make it simple, if the dfl driver didn't provide the
> id_table, normally it could not match any device. I think it could be
> easily understood by dfl driver developers.
> 
> 
> Thanks,
> Yilun
> 
> >
> > Tom
> >
> > >  		return -EINVAL;
> > >
> > >  	dfl_drv->drv.owner = owner;
> > > diff --git a/include/linux/dfl.h b/include/linux/dfl.h
> > > index 7affba2f..e1b2471 100644
> > > --- a/include/linux/dfl.h
> > > +++ b/include/linux/dfl.h
> > > @@ -32,6 +32,7 @@ enum dfl_id_type {
> > >   * @num_irqs: number of IRQs supported by this dfl device.
> > >   * @cdev: pointer to DFL FPGA container device this dfl device belongs to.
> > >   * @id_entry: matched id entry in dfl driver's id table.
> > > + * @driver_override: driver name to force a match
> > >   */
> > >  struct dfl_device {
> > >  	struct device dev;
> > > @@ -43,6 +44,7 @@ struct dfl_device {
> > >  	unsigned int num_irqs;
> > >  	struct dfl_fpga_cdev *cdev;
> > >  	const struct dfl_device_id *id_entry;
> > > +	char *driver_override;
> > >  };
> > >
> > >  /**

Re: [PATCH 1/2] fpga: dfl: add driver_override support

[Xu Yilun]

On Mon, Oct 19, 2020 at 03:46:23PM +0800, Wu, Hao wrote:
> > On Fri, Oct 16, 2020 at 09:21:50AM -0700, Tom Rix wrote:
> > >
> > > On 10/15/20 11:02 PM, Xu Yilun wrote:
> > > > Add support for overriding the default matching of a dfl device to a dfl
> > > > driver. It follows the same way that can be used for PCI and platform
> > > > devices. This patch adds the 'driver_override' sysfs file.
> > > >
> > > > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > > > ---
> > > >  Documentation/ABI/testing/sysfs-bus-dfl | 28 ++++++++++++++---
> > > >  drivers/fpga/dfl.c                      | 54
> > ++++++++++++++++++++++++++++++++-
> > > >  include/linux/dfl.h                     |  2 ++
> > > >  3 files changed, 79 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/Documentation/ABI/testing/sysfs-bus-dfl
> > b/Documentation/ABI/testing/sysfs-bus-dfl
> > > > index 23543be..db7e8d3 100644
> > > > --- a/Documentation/ABI/testing/sysfs-bus-dfl
> > > > +++ b/Documentation/ABI/testing/sysfs-bus-dfl
> > > > @@ -1,15 +1,35 @@
> > > >  What:/sys/bus/dfl/devices/dfl_dev.X/type
> > > > -Date:Aug 2020
> > > > -KernelVersion:5.10
> > > > +Date:Oct 2020
> > > > +KernelVersion:5.11
> > > >  Contact:Xu Yilun <yilun.xu@intel.com>
> > > >  Description:Read-only. It returns type of DFL FIU of the device.
> > Now DFL
> > > >  supports 2 FIU types, 0 for FME, 1 for PORT.
> > > >  Format: 0x%x
> > > >
> > > >  What:/sys/bus/dfl/devices/dfl_dev.X/feature_id
> > > > -Date:Aug 2020
> > > > -KernelVersion:5.10
> > > > +Date:Oct 2020
> > > > +KernelVersion:5.11
> > > >  Contact:Xu Yilun <yilun.xu@intel.com>
> > > >  Description:Read-only. It returns feature identifier local to its DFL
> > FIU
> > > >  type.
> > > >  Format: 0x%x
> > >
> > > These updates, do not match the comment.
> > >
> > > Consider splitting this out.
> >
> > I'm sorry it's a typo. The above code should not be changed.
> >
> > >
> > > > +
> > > > +What:           /sys/bus/dfl/devices/.../driver_override
> > > > +Date:           Oct 2020
> > > > +KernelVersion:  5.11
> > > > +Contact:        Xu Yilun <yilun.xu@intel.com>
> > > I am looking at description and trying to make it consistent with sysfs-bus-
> > pci
> > > > +Description:    This file allows the driver for a device to be specified.
> > >
> > > 'to be specified which will override the standard dfl bus feature id to driver
> > mapping.'
> >
> > Yes, it could be improved.
> >
> > Actually now it is the "type" and "feature id" matching, the 2 fields
> > are defined for dfl_driver.id_table. In future for dfl v1, it may be
> > GUID matching, which will be added to id_table. So how about we make it
> > more generic:
> >
> > 'to be specified which will override the standard ID table matching.'
> >
> > >
> > >
> > > >  When
> > > > +                specified, only a driver with a name matching the value written
> > > > +                to driver_override will have an opportunity to bind to the
> > > > +                device. The override is specified by writing a string to the
> > > > +                driver_override file (echo dfl-uio-pdev > driver_override) and
> > > > +                may be cleared with an empty string (echo > driver_override).
> > > > +                This returns the device to standard matching rules binding.
> > > > +                Writing to driver_override does not automatically unbind the
> > > > +                device from its current driver or make any attempt to
> > > > +                automatically load the specified driver.  If no driver with a
> > > > +                matching name is currently loaded in the kernel, the device
> > > > +                will not bind to any driver.  This also allows devices to
> > > > +                opt-out of driver binding using a driver_override name such as
> > > > +                "none".  Only a single driver may be specified in the override,
> > > > +                there is no support for parsing delimiters.
> > > > diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> > > > index 511b20f..bc35750 100644
> > > > --- a/drivers/fpga/dfl.c
> > > > +++ b/drivers/fpga/dfl.c
> > > > @@ -262,6 +262,10 @@ static int dfl_bus_match(struct device *dev,
> > struct device_driver *drv)
> > > >  struct dfl_driver *ddrv = to_dfl_drv(drv);
> > > >  const struct dfl_device_id *id_entry;
> > > >
> > > > +/* When driver_override is set, only bind to the matching driver */
> > > > +if (ddev->driver_override)
> > > > +return !strcmp(ddev->driver_override, drv->name);
> > > > +
> > > >  id_entry = ddrv->id_table;
> > > >  if (id_entry) {
> > > >  while (id_entry->feature_id) {
> > > > @@ -303,6 +307,53 @@ static int dfl_bus_uevent(struct device *dev,
> > struct kobj_uevent_env *env)
> > > >        ddev->type, ddev->feature_id);
> > > >  }
> > > >
> > >
> > > I am looking at other implementations of driver_override* and looking for
> > consistency.
> > >
> > > > +static ssize_t driver_override_show(struct device *dev,
> > > > +    struct device_attribute *attr, char *buf)
> > > > +{
> > > > +struct dfl_device *ddev = to_dfl_dev(dev);
> > > > +ssize_t len;
> > > > +
> > > > +device_lock(dev);
> > > > +len = sprintf(buf, "%s\n", ddev->driver_override);
> > > len = snprintf(buf, PAGE_SIZE ...
> >
> > It is good to me.
> >
> > Some bus drivers use snprintf, some use sprintf.
> >
> > I think it is reasonable snprintf is used here, unlike %d, %u ... it is
> > uncertain for the output size of %s.
> 
> you limited the size < a page in store function for driver_override?

Yes. So normally the sprintf should be OK. But I think it may be safer
if the driver_override pointer is corrupted in some unexpected cases.

Thanks,
Yilun

> 
> Hao
> 
> >
> > > > +device_unlock(dev);
> > > > +return len;
> > > > +}
> > > > +
> > > > +static ssize_t driver_override_store(struct device *dev,
> > > > +     struct device_attribute *attr,
> > > > +     const char *buf, size_t count)
> > > > +{
> > > > +struct dfl_device *ddev = to_dfl_dev(dev);
> > > > +char *driver_override, *old, *cp;
> > > > +
> > > > +/* We need to keep extra room for a newline */
> > > > +if (count >= (PAGE_SIZE - 1))
> > > > +return -EINVAL;
> > > > +
> > > > +driver_override = kstrndup(buf, count, GFP_KERNEL);
> > > > +if (!driver_override)
> > > > +return -ENOMEM;
> > > > +
> > > > +cp = strchr(driver_override, '\n');
> > > > +if (cp)
> > > > +*cp = '\0';
> > > > +
> > > > +device_lock(dev);
> > > > +old = ddev->driver_override;
> > > > +if (strlen(driver_override)) {
> > > > +ddev->driver_override = driver_override;
> > > > +} else {
> > > > +kfree(driver_override);
> > > > +ddev->driver_override = NULL;
> > > > +}
> > > > +device_unlock(dev);
> > > > +
> > > > +kfree(old);
> > > > +
> > > > +return count;
> > > > +}
> > > > +static DEVICE_ATTR_RW(driver_override);
> > > > +
> > > >  static ssize_t
> > > >  type_show(struct device *dev, struct device_attribute *attr, char *buf)
> > > >  {
> > > > @@ -324,6 +375,7 @@ static DEVICE_ATTR_RO(feature_id);
> > > >  static struct attribute *dfl_dev_attrs[] = {
> > > >  &dev_attr_type.attr,
> > > >  &dev_attr_feature_id.attr,
> > > > +&dev_attr_driver_override.attr,
> > > >  NULL,
> > > >  };
> > > >  ATTRIBUTE_GROUPS(dfl_dev);
> > > > @@ -469,7 +521,7 @@ static int dfl_devs_add(struct
> > dfl_feature_platform_data *pdata)
> > > >
> > > >  int __dfl_driver_register(struct dfl_driver *dfl_drv, struct module *owner)
> > > >  {
> > > > -if (!dfl_drv || !dfl_drv->probe || !dfl_drv->id_table)
> > > > +if (!dfl_drv || !dfl_drv->probe)
> > >
> > > id_table is still needed for the normal case.
> > >
> > > Instead of removing this check, could you add something like
> > >
> > > || (!dfl_drv->is_override && !dfl_drv->id_table)
> >
> > I don't think it is needed. Seems is_override and !id_table are duplicated
> > conditions for this implementation. And it may make confusing, e.g. could
> > a driver been force matched when is_override is not set?
> >
> > I think we could make it simple, if the dfl driver didn't provide the
> > id_table, normally it could not match any device. I think it could be
> > easily understood by dfl driver developers.
> >
> >
> > Thanks,
> > Yilun
> >
> > >
> > > Tom
> > >
> > > >  return -EINVAL;
> > > >
> > > >  dfl_drv->drv.owner = owner;
> > > > diff --git a/include/linux/dfl.h b/include/linux/dfl.h
> > > > index 7affba2f..e1b2471 100644
> > > > --- a/include/linux/dfl.h
> > > > +++ b/include/linux/dfl.h
> > > > @@ -32,6 +32,7 @@ enum dfl_id_type {
> > > >   * @num_irqs: number of IRQs supported by this dfl device.
> > > >   * @cdev: pointer to DFL FPGA container device this dfl device belongs to.
> > > >   * @id_entry: matched id entry in dfl driver's id table.
> > > > + * @driver_override: driver name to force a match
> > > >   */
> > > >  struct dfl_device {
> > > >  struct device dev;
> > > > @@ -43,6 +44,7 @@ struct dfl_device {
> > > >  unsigned int num_irqs;
> > > >  struct dfl_fpga_cdev *cdev;
> > > >  const struct dfl_device_id *id_entry;
> > > > +char *driver_override;
> > > >  };
> > > >
> > > >  /**

Re: [PATCH 1/2] fpga: dfl: add driver_override support

[Xu Yilun]

On Mon, Oct 19, 2020 at 12:06:13PM +0800, Xu Yilun wrote:
> On Fri, Oct 16, 2020 at 09:21:50AM -0700, Tom Rix wrote:
> > 
> > On 10/15/20 11:02 PM, Xu Yilun wrote:
> > > Add support for overriding the default matching of a dfl device to a dfl
> > > driver. It follows the same way that can be used for PCI and platform
> > > devices. This patch adds the 'driver_override' sysfs file.
> > >
> > > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > > ---
> > >  Documentation/ABI/testing/sysfs-bus-dfl | 28 ++++++++++++++---
> > >  drivers/fpga/dfl.c                      | 54 ++++++++++++++++++++++++++++++++-
> > >  include/linux/dfl.h                     |  2 ++
> > >  3 files changed, 79 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/Documentation/ABI/testing/sysfs-bus-dfl b/Documentation/ABI/testing/sysfs-bus-dfl
> > > index 23543be..db7e8d3 100644
> > > --- a/Documentation/ABI/testing/sysfs-bus-dfl
> > > +++ b/Documentation/ABI/testing/sysfs-bus-dfl
> > > @@ -1,15 +1,35 @@
> > >  What:		/sys/bus/dfl/devices/dfl_dev.X/type
> > > -Date:		Aug 2020
> > > -KernelVersion:	5.10
> > > +Date:		Oct 2020
> > > +KernelVersion:	5.11
> > >  Contact:	Xu Yilun <yilun.xu@intel.com>
> > >  Description:	Read-only. It returns type of DFL FIU of the device. Now DFL
> > >  		supports 2 FIU types, 0 for FME, 1 for PORT.
> > >  		Format: 0x%x
> > >  
> > >  What:		/sys/bus/dfl/devices/dfl_dev.X/feature_id
> > > -Date:		Aug 2020
> > > -KernelVersion:	5.10
> > > +Date:		Oct 2020
> > > +KernelVersion:	5.11
> > >  Contact:	Xu Yilun <yilun.xu@intel.com>
> > >  Description:	Read-only. It returns feature identifier local to its DFL FIU
> > >  		type.
> > >  		Format: 0x%x
> > 
> > These updates, do not match the comment.
> > 
> > Consider splitting this out.
> 
> I'm sorry it's a typo. The above code should not be changed.
> 
> > 
> > > +
> > > +What:           /sys/bus/dfl/devices/.../driver_override
> > > +Date:           Oct 2020
> > > +KernelVersion:  5.11
> > > +Contact:        Xu Yilun <yilun.xu@intel.com>
> > I am looking at description and trying to make it consistent with sysfs-bus-pci
> > > +Description:    This file allows the driver for a device to be specified.
> > 
> > 'to be specified which will override the standard dfl bus feature id to driver mapping.'
> 
> Yes, it could be improved.
> 
> Actually now it is the "type" and "feature id" matching, the 2 fields
> are defined for dfl_driver.id_table. In future for dfl v1, it may be
> GUID matching, which will be added to id_table. So how about we make it
> more generic:
> 
> 'to be specified which will override the standard ID table matching.'
> 
> > 
> > 
> > >  When
> > > +                specified, only a driver with a name matching the value written
> > > +                to driver_override will have an opportunity to bind to the
> > > +                device. The override is specified by writing a string to the
> > > +                driver_override file (echo dfl-uio-pdev > driver_override) and
> > > +                may be cleared with an empty string (echo > driver_override).
> > > +                This returns the device to standard matching rules binding.
> > > +                Writing to driver_override does not automatically unbind the
> > > +                device from its current driver or make any attempt to
> > > +                automatically load the specified driver.  If no driver with a
> > > +                matching name is currently loaded in the kernel, the device
> > > +                will not bind to any driver.  This also allows devices to
> > > +                opt-out of driver binding using a driver_override name such as
> > > +                "none".  Only a single driver may be specified in the override,
> > > +                there is no support for parsing delimiters.
> > > diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> > > index 511b20f..bc35750 100644
> > > --- a/drivers/fpga/dfl.c
> > > +++ b/drivers/fpga/dfl.c
> > > @@ -262,6 +262,10 @@ static int dfl_bus_match(struct device *dev, struct device_driver *drv)
> > >  	struct dfl_driver *ddrv = to_dfl_drv(drv);
> > >  	const struct dfl_device_id *id_entry;
> > >  
> > > +	/* When driver_override is set, only bind to the matching driver */
> > > +	if (ddev->driver_override)
> > > +		return !strcmp(ddev->driver_override, drv->name);
> > > +
> > >  	id_entry = ddrv->id_table;
> > >  	if (id_entry) {
> > >  		while (id_entry->feature_id) {
> > > @@ -303,6 +307,53 @@ static int dfl_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
> > >  			      ddev->type, ddev->feature_id);
> > >  }
> > >  
> > 
> > I am looking at other implementations of driver_override* and looking for consistency.
> > 
> > > +static ssize_t driver_override_show(struct device *dev,
> > > +				    struct device_attribute *attr, char *buf)
> > > +{
> > > +	struct dfl_device *ddev = to_dfl_dev(dev);
> > > +	ssize_t len;
> > > +
> > > +	device_lock(dev);
> > > +	len = sprintf(buf, "%s\n", ddev->driver_override);
> > len = snprintf(buf, PAGE_SIZE ...
> 
> It is good to me.
> 
> Some bus drivers use snprintf, some use sprintf.
> 
> I think it is reasonable snprintf is used here, unlike %d, %u ... it is
> uncertain for the output size of %s.

Sorry, I checked the Documentation/filesystems/sysfs.rst again and found
I didn't remember it correctly.

The snprintf is must not be used, sprintf() or scnprintf() should be
used. So I prefer sprintf() here, following other implementations.

sprintf() should be safe here, as we already limited the input of
driver_override string to less then PAGE_SIZE on store().

Thanks,
Yilun

> 
> > > +	device_unlock(dev);
> > > +	return len;
> > > +}
> > > +
> > > +static ssize_t driver_override_store(struct device *dev,
> > > +				     struct device_attribute *attr,
> > > +				     const char *buf, size_t count)
> > > +{
> > > +	struct dfl_device *ddev = to_dfl_dev(dev);
> > > +	char *driver_override, *old, *cp;
> > > +
> > > +	/* We need to keep extra room for a newline */
> > > +	if (count >= (PAGE_SIZE - 1))
> > > +		return -EINVAL;
> > > +
> > > +	driver_override = kstrndup(buf, count, GFP_KERNEL);
> > > +	if (!driver_override)
> > > +		return -ENOMEM;
> > > +
> > > +	cp = strchr(driver_override, '\n');
> > > +	if (cp)
> > > +		*cp = '\0';
> > > +
> > > +	device_lock(dev);
> > > +	old = ddev->driver_override;
> > > +	if (strlen(driver_override)) {
> > > +		ddev->driver_override = driver_override;
> > > +	} else {
> > > +		kfree(driver_override);
> > > +		ddev->driver_override = NULL;
> > > +	}
> > > +	device_unlock(dev);
> > > +
> > > +	kfree(old);
> > > +
> > > +	return count;
> > > +}
> > > +static DEVICE_ATTR_RW(driver_override);
> > > +
> > >  static ssize_t
> > >  type_show(struct device *dev, struct device_attribute *attr, char *buf)
> > >  {
> > > @@ -324,6 +375,7 @@ static DEVICE_ATTR_RO(feature_id);
> > >  static struct attribute *dfl_dev_attrs[] = {
> > >  	&dev_attr_type.attr,
> > >  	&dev_attr_feature_id.attr,
> > > +	&dev_attr_driver_override.attr,
> > >  	NULL,
> > >  };
> > >  ATTRIBUTE_GROUPS(dfl_dev);
> > > @@ -469,7 +521,7 @@ static int dfl_devs_add(struct dfl_feature_platform_data *pdata)
> > >  
> > >  int __dfl_driver_register(struct dfl_driver *dfl_drv, struct module *owner)
> > >  {
> > > -	if (!dfl_drv || !dfl_drv->probe || !dfl_drv->id_table)
> > > +	if (!dfl_drv || !dfl_drv->probe)
> > 
> > id_table is still needed for the normal case.
> > 
> > Instead of removing this check, could you add something like
> > 
> > || (!dfl_drv->is_override && !dfl_drv->id_table)
> 
> I don't think it is needed. Seems is_override and !id_table are duplicated
> conditions for this implementation. And it may make confusing, e.g. could
> a driver been force matched when is_override is not set?
> 
> I think we could make it simple, if the dfl driver didn't provide the
> id_table, normally it could not match any device. I think it could be
> easily understood by dfl driver developers.
> 
> 
> Thanks,
> Yilun
> 
> > 
> > Tom
> > 
> > >  		return -EINVAL;
> > >  
> > >  	dfl_drv->drv.owner = owner;
> > > diff --git a/include/linux/dfl.h b/include/linux/dfl.h
> > > index 7affba2f..e1b2471 100644
> > > --- a/include/linux/dfl.h
> > > +++ b/include/linux/dfl.h
> > > @@ -32,6 +32,7 @@ enum dfl_id_type {
> > >   * @num_irqs: number of IRQs supported by this dfl device.
> > >   * @cdev: pointer to DFL FPGA container device this dfl device belongs to.
> > >   * @id_entry: matched id entry in dfl driver's id table.
> > > + * @driver_override: driver name to force a match
> > >   */
> > >  struct dfl_device {
> > >  	struct device dev;
> > > @@ -43,6 +44,7 @@ struct dfl_device {
> > >  	unsigned int num_irqs;
> > >  	struct dfl_fpga_cdev *cdev;
> > >  	const struct dfl_device_id *id_entry;
> > > +	char *driver_override;
> > >  };
> > >  
> > >  /**

Re: [PATCH 1/2] fpga: dfl: add driver_override support

[gregkh]

On Mon, Oct 19, 2020 at 03:50:32PM +0800, Xu Yilun wrote:
> On Mon, Oct 19, 2020 at 03:46:23PM +0800, Wu, Hao wrote:
> > > On Fri, Oct 16, 2020 at 09:21:50AM -0700, Tom Rix wrote:
> > > >
> > > > On 10/15/20 11:02 PM, Xu Yilun wrote:
> > > > > Add support for overriding the default matching of a dfl device to a dfl
> > > > > driver. It follows the same way that can be used for PCI and platform
> > > > > devices. This patch adds the 'driver_override' sysfs file.
> > > > >
> > > > > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > > > > ---
> > > > >  Documentation/ABI/testing/sysfs-bus-dfl | 28 ++++++++++++++---
> > > > >  drivers/fpga/dfl.c                      | 54
> > > ++++++++++++++++++++++++++++++++-
> > > > >  include/linux/dfl.h                     |  2 ++
> > > > >  3 files changed, 79 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/Documentation/ABI/testing/sysfs-bus-dfl
> > > b/Documentation/ABI/testing/sysfs-bus-dfl
> > > > > index 23543be..db7e8d3 100644
> > > > > --- a/Documentation/ABI/testing/sysfs-bus-dfl
> > > > > +++ b/Documentation/ABI/testing/sysfs-bus-dfl
> > > > > @@ -1,15 +1,35 @@
> > > > >  What:/sys/bus/dfl/devices/dfl_dev.X/type
> > > > > -Date:Aug 2020
> > > > > -KernelVersion:5.10
> > > > > +Date:Oct 2020
> > > > > +KernelVersion:5.11
> > > > >  Contact:Xu Yilun <yilun.xu@intel.com>
> > > > >  Description:Read-only. It returns type of DFL FIU of the device.
> > > Now DFL
> > > > >  supports 2 FIU types, 0 for FME, 1 for PORT.
> > > > >  Format: 0x%x
> > > > >
> > > > >  What:/sys/bus/dfl/devices/dfl_dev.X/feature_id
> > > > > -Date:Aug 2020
> > > > > -KernelVersion:5.10
> > > > > +Date:Oct 2020
> > > > > +KernelVersion:5.11
> > > > >  Contact:Xu Yilun <yilun.xu@intel.com>
> > > > >  Description:Read-only. It returns feature identifier local to its DFL
> > > FIU
> > > > >  type.
> > > > >  Format: 0x%x
> > > >
> > > > These updates, do not match the comment.
> > > >
> > > > Consider splitting this out.
> > >
> > > I'm sorry it's a typo. The above code should not be changed.
> > >
> > > >
> > > > > +
> > > > > +What:           /sys/bus/dfl/devices/.../driver_override
> > > > > +Date:           Oct 2020
> > > > > +KernelVersion:  5.11
> > > > > +Contact:        Xu Yilun <yilun.xu@intel.com>
> > > > I am looking at description and trying to make it consistent with sysfs-bus-
> > > pci
> > > > > +Description:    This file allows the driver for a device to be specified.
> > > >
> > > > 'to be specified which will override the standard dfl bus feature id to driver
> > > mapping.'
> > >
> > > Yes, it could be improved.
> > >
> > > Actually now it is the "type" and "feature id" matching, the 2 fields
> > > are defined for dfl_driver.id_table. In future for dfl v1, it may be
> > > GUID matching, which will be added to id_table. So how about we make it
> > > more generic:
> > >
> > > 'to be specified which will override the standard ID table matching.'
> > >
> > > >
> > > >
> > > > >  When
> > > > > +                specified, only a driver with a name matching the value written
> > > > > +                to driver_override will have an opportunity to bind to the
> > > > > +                device. The override is specified by writing a string to the
> > > > > +                driver_override file (echo dfl-uio-pdev > driver_override) and
> > > > > +                may be cleared with an empty string (echo > driver_override).
> > > > > +                This returns the device to standard matching rules binding.
> > > > > +                Writing to driver_override does not automatically unbind the
> > > > > +                device from its current driver or make any attempt to
> > > > > +                automatically load the specified driver.  If no driver with a
> > > > > +                matching name is currently loaded in the kernel, the device
> > > > > +                will not bind to any driver.  This also allows devices to
> > > > > +                opt-out of driver binding using a driver_override name such as
> > > > > +                "none".  Only a single driver may be specified in the override,
> > > > > +                there is no support for parsing delimiters.
> > > > > diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> > > > > index 511b20f..bc35750 100644
> > > > > --- a/drivers/fpga/dfl.c
> > > > > +++ b/drivers/fpga/dfl.c
> > > > > @@ -262,6 +262,10 @@ static int dfl_bus_match(struct device *dev,
> > > struct device_driver *drv)
> > > > >  struct dfl_driver *ddrv = to_dfl_drv(drv);
> > > > >  const struct dfl_device_id *id_entry;
> > > > >
> > > > > +/* When driver_override is set, only bind to the matching driver */
> > > > > +if (ddev->driver_override)
> > > > > +return !strcmp(ddev->driver_override, drv->name);
> > > > > +
> > > > >  id_entry = ddrv->id_table;
> > > > >  if (id_entry) {
> > > > >  while (id_entry->feature_id) {
> > > > > @@ -303,6 +307,53 @@ static int dfl_bus_uevent(struct device *dev,
> > > struct kobj_uevent_env *env)
> > > > >        ddev->type, ddev->feature_id);
> > > > >  }
> > > > >
> > > >
> > > > I am looking at other implementations of driver_override* and looking for
> > > consistency.
> > > >
> > > > > +static ssize_t driver_override_show(struct device *dev,
> > > > > +    struct device_attribute *attr, char *buf)
> > > > > +{
> > > > > +struct dfl_device *ddev = to_dfl_dev(dev);
> > > > > +ssize_t len;
> > > > > +
> > > > > +device_lock(dev);
> > > > > +len = sprintf(buf, "%s\n", ddev->driver_override);
> > > > len = snprintf(buf, PAGE_SIZE ...
> > >
> > > It is good to me.
> > >
> > > Some bus drivers use snprintf, some use sprintf.
> > >
> > > I think it is reasonable snprintf is used here, unlike %d, %u ... it is
> > > uncertain for the output size of %s.

No, no one should care at all about this.

If you are caring, then you are using sysfs wrong.

Just use sprintf(), you never care about the size of the buffer for a
show/store function.

Or better yet, use the new sysfs_emit() call, that is what everything
will be converted over to anyway.

> > you limited the size < a page in store function for driver_override?
> 
> Yes. So normally the sprintf should be OK. But I think it may be safer
> if the driver_override pointer is corrupted in some unexpected cases.

How can that happen?

greg k-h

Re: [PATCH 1/2] fpga: dfl: add driver_override support

[Greg KH]

On Mon, Oct 19, 2020 at 04:52:33PM +0800, Xu Yilun wrote:
> On Mon, Oct 19, 2020 at 12:06:13PM +0800, Xu Yilun wrote:
> > On Fri, Oct 16, 2020 at 09:21:50AM -0700, Tom Rix wrote:
> > > 
> > > On 10/15/20 11:02 PM, Xu Yilun wrote:
> > > > Add support for overriding the default matching of a dfl device to a dfl
> > > > driver. It follows the same way that can be used for PCI and platform
> > > > devices. This patch adds the 'driver_override' sysfs file.
> > > >
> > > > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > > > ---
> > > >  Documentation/ABI/testing/sysfs-bus-dfl | 28 ++++++++++++++---
> > > >  drivers/fpga/dfl.c                      | 54 ++++++++++++++++++++++++++++++++-
> > > >  include/linux/dfl.h                     |  2 ++
> > > >  3 files changed, 79 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/Documentation/ABI/testing/sysfs-bus-dfl b/Documentation/ABI/testing/sysfs-bus-dfl
> > > > index 23543be..db7e8d3 100644
> > > > --- a/Documentation/ABI/testing/sysfs-bus-dfl
> > > > +++ b/Documentation/ABI/testing/sysfs-bus-dfl
> > > > @@ -1,15 +1,35 @@
> > > >  What:		/sys/bus/dfl/devices/dfl_dev.X/type
> > > > -Date:		Aug 2020
> > > > -KernelVersion:	5.10
> > > > +Date:		Oct 2020
> > > > +KernelVersion:	5.11
> > > >  Contact:	Xu Yilun <yilun.xu@intel.com>
> > > >  Description:	Read-only. It returns type of DFL FIU of the device. Now DFL
> > > >  		supports 2 FIU types, 0 for FME, 1 for PORT.
> > > >  		Format: 0x%x
> > > >  
> > > >  What:		/sys/bus/dfl/devices/dfl_dev.X/feature_id
> > > > -Date:		Aug 2020
> > > > -KernelVersion:	5.10
> > > > +Date:		Oct 2020
> > > > +KernelVersion:	5.11
> > > >  Contact:	Xu Yilun <yilun.xu@intel.com>
> > > >  Description:	Read-only. It returns feature identifier local to its DFL FIU
> > > >  		type.
> > > >  		Format: 0x%x
> > > 
> > > These updates, do not match the comment.
> > > 
> > > Consider splitting this out.
> > 
> > I'm sorry it's a typo. The above code should not be changed.
> > 
> > > 
> > > > +
> > > > +What:           /sys/bus/dfl/devices/.../driver_override
> > > > +Date:           Oct 2020
> > > > +KernelVersion:  5.11
> > > > +Contact:        Xu Yilun <yilun.xu@intel.com>
> > > I am looking at description and trying to make it consistent with sysfs-bus-pci
> > > > +Description:    This file allows the driver for a device to be specified.
> > > 
> > > 'to be specified which will override the standard dfl bus feature id to driver mapping.'
> > 
> > Yes, it could be improved.
> > 
> > Actually now it is the "type" and "feature id" matching, the 2 fields
> > are defined for dfl_driver.id_table. In future for dfl v1, it may be
> > GUID matching, which will be added to id_table. So how about we make it
> > more generic:
> > 
> > 'to be specified which will override the standard ID table matching.'
> > 
> > > 
> > > 
> > > >  When
> > > > +                specified, only a driver with a name matching the value written
> > > > +                to driver_override will have an opportunity to bind to the
> > > > +                device. The override is specified by writing a string to the
> > > > +                driver_override file (echo dfl-uio-pdev > driver_override) and
> > > > +                may be cleared with an empty string (echo > driver_override).
> > > > +                This returns the device to standard matching rules binding.
> > > > +                Writing to driver_override does not automatically unbind the
> > > > +                device from its current driver or make any attempt to
> > > > +                automatically load the specified driver.  If no driver with a
> > > > +                matching name is currently loaded in the kernel, the device
> > > > +                will not bind to any driver.  This also allows devices to
> > > > +                opt-out of driver binding using a driver_override name such as
> > > > +                "none".  Only a single driver may be specified in the override,
> > > > +                there is no support for parsing delimiters.
> > > > diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> > > > index 511b20f..bc35750 100644
> > > > --- a/drivers/fpga/dfl.c
> > > > +++ b/drivers/fpga/dfl.c
> > > > @@ -262,6 +262,10 @@ static int dfl_bus_match(struct device *dev, struct device_driver *drv)
> > > >  	struct dfl_driver *ddrv = to_dfl_drv(drv);
> > > >  	const struct dfl_device_id *id_entry;
> > > >  
> > > > +	/* When driver_override is set, only bind to the matching driver */
> > > > +	if (ddev->driver_override)
> > > > +		return !strcmp(ddev->driver_override, drv->name);
> > > > +
> > > >  	id_entry = ddrv->id_table;
> > > >  	if (id_entry) {
> > > >  		while (id_entry->feature_id) {
> > > > @@ -303,6 +307,53 @@ static int dfl_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
> > > >  			      ddev->type, ddev->feature_id);
> > > >  }
> > > >  
> > > 
> > > I am looking at other implementations of driver_override* and looking for consistency.
> > > 
> > > > +static ssize_t driver_override_show(struct device *dev,
> > > > +				    struct device_attribute *attr, char *buf)
> > > > +{
> > > > +	struct dfl_device *ddev = to_dfl_dev(dev);
> > > > +	ssize_t len;
> > > > +
> > > > +	device_lock(dev);
> > > > +	len = sprintf(buf, "%s\n", ddev->driver_override);
> > > len = snprintf(buf, PAGE_SIZE ...
> > 
> > It is good to me.
> > 
> > Some bus drivers use snprintf, some use sprintf.
> > 
> > I think it is reasonable snprintf is used here, unlike %d, %u ... it is
> > uncertain for the output size of %s.
> 
> Sorry, I checked the Documentation/filesystems/sysfs.rst again and found
> I didn't remember it correctly.
> 
> The snprintf is must not be used, sprintf() or scnprintf() should be
> used. So I prefer sprintf() here, following other implementations.
> 
> sprintf() should be safe here, as we already limited the input of
> driver_override string to less then PAGE_SIZE on store().

As already mentioned, please use sysfs_emit() instead.

thanks,

greg k-h

Re: [PATCH 1/2] fpga: dfl: add driver_override support

[Tom Rix]

On 10/18/20 9:06 PM, Xu Yilun wrote:
> On Fri, Oct 16, 2020 at 09:21:50AM -0700, Tom Rix wrote:
>> On 10/15/20 11:02 PM, Xu Yilun wrote:
>>> Add support for overriding the default matching of a dfl device to a dfl
>>> driver. It follows the same way that can be used for PCI and platform
>>> devices. This patch adds the 'driver_override' sysfs file.
>>>
>>> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
>>> ---
>>>  Documentation/ABI/testing/sysfs-bus-dfl | 28 ++++++++++++++---
>>>  drivers/fpga/dfl.c                      | 54 ++++++++++++++++++++++++++++++++-
>>>  include/linux/dfl.h                     |  2 ++
>>>  3 files changed, 79 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-bus-dfl b/Documentation/ABI/testing/sysfs-bus-dfl
>>> index 23543be..db7e8d3 100644
>>> --- a/Documentation/ABI/testing/sysfs-bus-dfl
>>> +++ b/Documentation/ABI/testing/sysfs-bus-dfl
>>> @@ -1,15 +1,35 @@
>>>  What:		/sys/bus/dfl/devices/dfl_dev.X/type
>>> -Date:		Aug 2020
>>> -KernelVersion:	5.10
>>> +Date:		Oct 2020
>>> +KernelVersion:	5.11
>>>  Contact:	Xu Yilun <yilun.xu@intel.com>
>>>  Description:	Read-only. It returns type of DFL FIU of the device. Now DFL
>>>  		supports 2 FIU types, 0 for FME, 1 for PORT.
>>>  		Format: 0x%x
>>>  
>>>  What:		/sys/bus/dfl/devices/dfl_dev.X/feature_id
>>> -Date:		Aug 2020
>>> -KernelVersion:	5.10
>>> +Date:		Oct 2020
>>> +KernelVersion:	5.11
>>>  Contact:	Xu Yilun <yilun.xu@intel.com>
>>>  Description:	Read-only. It returns feature identifier local to its DFL FIU
>>>  		type.
>>>  		Format: 0x%x
>> These updates, do not match the comment.
>>
>> Consider splitting this out.
> I'm sorry it's a typo. The above code should not be changed.
>
>>> +
>>> +What:           /sys/bus/dfl/devices/.../driver_override
>>> +Date:           Oct 2020
>>> +KernelVersion:  5.11
>>> +Contact:        Xu Yilun <yilun.xu@intel.com>
>> I am looking at description and trying to make it consistent with sysfs-bus-pci
>>> +Description:    This file allows the driver for a device to be specified.
>> 'to be specified which will override the standard dfl bus feature id to driver mapping.'
> Yes, it could be improved.
>
> Actually now it is the "type" and "feature id" matching, the 2 fields
> are defined for dfl_driver.id_table. In future for dfl v1, it may be
> GUID matching, which will be added to id_table. So how about we make it
> more generic:
>
> 'to be specified which will override the standard ID table matching.'
yes, this is good.
>
>>
>>>  When
>>> +                specified, only a driver with a name matching the value written
>>> +                to driver_override will have an opportunity to bind to the
>>> +                device. The override is specified by writing a string to the
>>> +                driver_override file (echo dfl-uio-pdev > driver_override) and
>>> +                may be cleared with an empty string (echo > driver_override).
>>> +                This returns the device to standard matching rules binding.
>>> +                Writing to driver_override does not automatically unbind the
>>> +                device from its current driver or make any attempt to
>>> +                automatically load the specified driver.  If no driver with a
>>> +                matching name is currently loaded in the kernel, the device
>>> +                will not bind to any driver.  This also allows devices to
>>> +                opt-out of driver binding using a driver_override name such as
>>> +                "none".  Only a single driver may be specified in the override,
>>> +                there is no support for parsing delimiters.
>>> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
>>> index 511b20f..bc35750 100644
>>> --- a/drivers/fpga/dfl.c
>>> +++ b/drivers/fpga/dfl.c
>>> @@ -262,6 +262,10 @@ static int dfl_bus_match(struct device *dev, struct device_driver *drv)
>>>  	struct dfl_driver *ddrv = to_dfl_drv(drv);
>>>  	const struct dfl_device_id *id_entry;
>>>  
>>> +	/* When driver_override is set, only bind to the matching driver */
>>> +	if (ddev->driver_override)
>>> +		return !strcmp(ddev->driver_override, drv->name);
>>> +
>>>  	id_entry = ddrv->id_table;
>>>  	if (id_entry) {
>>>  		while (id_entry->feature_id) {
>>> @@ -303,6 +307,53 @@ static int dfl_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
>>>  			      ddev->type, ddev->feature_id);
>>>  }
>>>  
>> I am looking at other implementations of driver_override* and looking for consistency.
>>
>>> +static ssize_t driver_override_show(struct device *dev,
>>> +				    struct device_attribute *attr, char *buf)
>>> +{
>>> +	struct dfl_device *ddev = to_dfl_dev(dev);
>>> +	ssize_t len;
>>> +
>>> +	device_lock(dev);
>>> +	len = sprintf(buf, "%s\n", ddev->driver_override);
>> len = snprintf(buf, PAGE_SIZE ...
> It is good to me.
>
> Some bus drivers use snprintf, some use sprintf.
>
> I think it is reasonable snprintf is used here, unlike %d, %u ... it is
> uncertain for the output size of %s.

I am not sure if you are agreeing to do this change.

I was looking at the other driver_override_show() functions.

Generally the 'n' variants of string functions should be used.

>
>>> +	device_unlock(dev);
>>> +	return len;
>>> +}
>>> +
>>> +static ssize_t driver_override_store(struct device *dev,
>>> +				     struct device_attribute *attr,
>>> +				     const char *buf, size_t count)
>>> +{
>>> +	struct dfl_device *ddev = to_dfl_dev(dev);
>>> +	char *driver_override, *old, *cp;
>>> +
>>> +	/* We need to keep extra room for a newline */
>>> +	if (count >= (PAGE_SIZE - 1))
>>> +		return -EINVAL;
>>> +
>>> +	driver_override = kstrndup(buf, count, GFP_KERNEL);
>>> +	if (!driver_override)
>>> +		return -ENOMEM;
>>> +
>>> +	cp = strchr(driver_override, '\n');
>>> +	if (cp)
>>> +		*cp = '\0';
>>> +
>>> +	device_lock(dev);
>>> +	old = ddev->driver_override;
>>> +	if (strlen(driver_override)) {
>>> +		ddev->driver_override = driver_override;
>>> +	} else {
>>> +		kfree(driver_override);
>>> +		ddev->driver_override = NULL;
>>> +	}
>>> +	device_unlock(dev);
>>> +
>>> +	kfree(old);
>>> +
>>> +	return count;
>>> +}
>>> +static DEVICE_ATTR_RW(driver_override);
>>> +
>>>  static ssize_t
>>>  type_show(struct device *dev, struct device_attribute *attr, char *buf)
>>>  {
>>> @@ -324,6 +375,7 @@ static DEVICE_ATTR_RO(feature_id);
>>>  static struct attribute *dfl_dev_attrs[] = {
>>>  	&dev_attr_type.attr,
>>>  	&dev_attr_feature_id.attr,
>>> +	&dev_attr_driver_override.attr,
>>>  	NULL,
>>>  };
>>>  ATTRIBUTE_GROUPS(dfl_dev);
>>> @@ -469,7 +521,7 @@ static int dfl_devs_add(struct dfl_feature_platform_data *pdata)
>>>  
>>>  int __dfl_driver_register(struct dfl_driver *dfl_drv, struct module *owner)
>>>  {
>>> -	if (!dfl_drv || !dfl_drv->probe || !dfl_drv->id_table)
>>> +	if (!dfl_drv || !dfl_drv->probe)
>> id_table is still needed for the normal case.
>>
>> Instead of removing this check, could you add something like
>>
>> || (!dfl_drv->is_override && !dfl_drv->id_table)
> I don't think it is needed. Seems is_override and !id_table are duplicated
> conditions for this implementation. And it may make confusing, e.g. could
> a driver been force matched when is_override is not set?
>
> I think we could make it simple, if the dfl driver didn't provide the
> id_table, normally it could not match any device. I think it could be
> easily understood by dfl driver developers.
>
Then an ASSERT should be added in dfl_bus_match() for id_entry.

Tom

> Thanks,
> Yilun
>
>> Tom
>>
>>>  		return -EINVAL;
>>>  
>>>  	dfl_drv->drv.owner = owner;
>>> diff --git a/include/linux/dfl.h b/include/linux/dfl.h
>>> index 7affba2f..e1b2471 100644
>>> --- a/include/linux/dfl.h
>>> +++ b/include/linux/dfl.h
>>> @@ -32,6 +32,7 @@ enum dfl_id_type {
>>>   * @num_irqs: number of IRQs supported by this dfl device.
>>>   * @cdev: pointer to DFL FPGA container device this dfl device belongs to.
>>>   * @id_entry: matched id entry in dfl driver's id table.
>>> + * @driver_override: driver name to force a match
>>>   */
>>>  struct dfl_device {
>>>  	struct device dev;
>>> @@ -43,6 +44,7 @@ struct dfl_device {
>>>  	unsigned int num_irqs;
>>>  	struct dfl_fpga_cdev *cdev;
>>>  	const struct dfl_device_id *id_entry;
>>> +	char *driver_override;
>>>  };
>>>  
>>>  /**

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

[Tom Rix]

On 10/18/20 9:16 PM, Xu Yilun wrote:
> On Fri, Oct 16, 2020 at 09:36:00AM -0700, Tom Rix wrote:
>> On 10/15/20 11:02 PM, 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.
>>>
>>> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
>>> ---
>>>  drivers/fpga/Kconfig        | 10 ++++++
>>>  drivers/fpga/Makefile       |  1 +
>>>  drivers/fpga/dfl-uio-pdev.c | 83 +++++++++++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 94 insertions(+)
>>>  create mode 100644 drivers/fpga/dfl-uio-pdev.c
>>>
>>> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
>>> index 5d7f0ae..e054722 100644
>>> --- a/drivers/fpga/Kconfig
>>> +++ b/drivers/fpga/Kconfig
>>> @@ -202,6 +202,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 device's
>>> +	  resources, and let the generic UIO platform device driver provide
>> 'and lets the'
> Yes.
>
>>> +	  support to 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..e07b3d5 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..d35b846
>>> --- /dev/null
>>> +++ b/drivers/fpga/dfl-uio-pdev.c
>>> @@ -0,0 +1,83 @@
>>> +// 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 device *dev = &ddev->dev;
>>> +	struct platform_device_info pdevinfo = { 0 };
>>> +	struct uio_info uio_pdata = { 0 };
>>> +	struct platform_device *uio_pdev;
>>> +	struct resource *res;
>>> +	int i, idx = 0;
>> idx is not needed.
> I could remove the idx. But I think I could ++res during the resource
> filling.

ok.

You may want to check the code generation, my feeling is ++res will be worse.

Tom

>
> Thanks,
> Yilun
>
>>> +
>>> +	pdevinfo.name = "uio_pdrv_genirq";
>>> +
>>> +	res = kcalloc(ddev->num_irqs + 1, sizeof(*res), GFP_KERNEL);
>>> +	if (!res)
>>> +		return -ENOMEM;
>>> +
>>> +	res[idx].parent = &ddev->mmio_res;
>> res[0].parent =
>>> +	res[idx].flags = IORESOURCE_MEM;
>>> +	res[idx].start = ddev->mmio_res.start;
>>> +	res[idx].end = ddev->mmio_res.end;
>>> +	++idx;
>>> +
>>> +	/* then add irq resource */
>>> +	for (i = 0; i < ddev->num_irqs; i++) {
>>> +		res[idx].flags = IORESOURCE_IRQ;
>> res[i+1].flags =
>>
>> Tom
>>
>>> +		res[idx].start = ddev->irqs[i];
>>> +		res[idx].end = ddev->irqs[i];
>>> +		++idx;
>>> +	}
>>> +
>>> +	uio_pdata.name = DRIVER_NAME;
>>> +	uio_pdata.version = "0";
>>> +
>>> +	pdevinfo.res = res;
>>> +	pdevinfo.num_res = idx;
>>> +	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))
>>> +		dev_set_drvdata(dev, uio_pdev);
>>> +
>>> +	kfree(res);
>>> +
>>> +	return PTR_ERR_OR_ZERO(uio_pdev);
>>> +}
>>> +
>>> +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);
>>> +}
>>> +
>>> +static struct dfl_driver dfl_uio_pdev_driver = {
>>> +	.drv	= {
>>> +		.name       = DRIVER_NAME,
>>> +	},
>>> +	.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");

Re: [PATCH 1/2] fpga: dfl: add driver_override support

[Xu Yilun]

On Mon, Oct 19, 2020 at 11:03:16AM +0200, Greg KH wrote:
> On Mon, Oct 19, 2020 at 04:52:33PM +0800, Xu Yilun wrote:
> > On Mon, Oct 19, 2020 at 12:06:13PM +0800, Xu Yilun wrote:
> > > On Fri, Oct 16, 2020 at 09:21:50AM -0700, Tom Rix wrote:
> > > > 
> > > > On 10/15/20 11:02 PM, Xu Yilun wrote:
> > > > > Add support for overriding the default matching of a dfl device to a dfl
> > > > > driver. It follows the same way that can be used for PCI and platform
> > > > > devices. This patch adds the 'driver_override' sysfs file.
> > > > >
> > > > > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > > > > ---
> > > > >  Documentation/ABI/testing/sysfs-bus-dfl | 28 ++++++++++++++---
> > > > >  drivers/fpga/dfl.c                      | 54 ++++++++++++++++++++++++++++++++-
> > > > >  include/linux/dfl.h                     |  2 ++
> > > > >  3 files changed, 79 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/Documentation/ABI/testing/sysfs-bus-dfl b/Documentation/ABI/testing/sysfs-bus-dfl
> > > > > index 23543be..db7e8d3 100644
> > > > > --- a/Documentation/ABI/testing/sysfs-bus-dfl
> > > > > +++ b/Documentation/ABI/testing/sysfs-bus-dfl
> > > > > @@ -1,15 +1,35 @@
> > > > >  What:		/sys/bus/dfl/devices/dfl_dev.X/type
> > > > > -Date:		Aug 2020
> > > > > -KernelVersion:	5.10
> > > > > +Date:		Oct 2020
> > > > > +KernelVersion:	5.11
> > > > >  Contact:	Xu Yilun <yilun.xu@intel.com>
> > > > >  Description:	Read-only. It returns type of DFL FIU of the device. Now DFL
> > > > >  		supports 2 FIU types, 0 for FME, 1 for PORT.
> > > > >  		Format: 0x%x
> > > > >  
> > > > >  What:		/sys/bus/dfl/devices/dfl_dev.X/feature_id
> > > > > -Date:		Aug 2020
> > > > > -KernelVersion:	5.10
> > > > > +Date:		Oct 2020
> > > > > +KernelVersion:	5.11
> > > > >  Contact:	Xu Yilun <yilun.xu@intel.com>
> > > > >  Description:	Read-only. It returns feature identifier local to its DFL FIU
> > > > >  		type.
> > > > >  		Format: 0x%x
> > > > 
> > > > These updates, do not match the comment.
> > > > 
> > > > Consider splitting this out.
> > > 
> > > I'm sorry it's a typo. The above code should not be changed.
> > > 
> > > > 
> > > > > +
> > > > > +What:           /sys/bus/dfl/devices/.../driver_override
> > > > > +Date:           Oct 2020
> > > > > +KernelVersion:  5.11
> > > > > +Contact:        Xu Yilun <yilun.xu@intel.com>
> > > > I am looking at description and trying to make it consistent with sysfs-bus-pci
> > > > > +Description:    This file allows the driver for a device to be specified.
> > > > 
> > > > 'to be specified which will override the standard dfl bus feature id to driver mapping.'
> > > 
> > > Yes, it could be improved.
> > > 
> > > Actually now it is the "type" and "feature id" matching, the 2 fields
> > > are defined for dfl_driver.id_table. In future for dfl v1, it may be
> > > GUID matching, which will be added to id_table. So how about we make it
> > > more generic:
> > > 
> > > 'to be specified which will override the standard ID table matching.'
> > > 
> > > > 
> > > > 
> > > > >  When
> > > > > +                specified, only a driver with a name matching the value written
> > > > > +                to driver_override will have an opportunity to bind to the
> > > > > +                device. The override is specified by writing a string to the
> > > > > +                driver_override file (echo dfl-uio-pdev > driver_override) and
> > > > > +                may be cleared with an empty string (echo > driver_override).
> > > > > +                This returns the device to standard matching rules binding.
> > > > > +                Writing to driver_override does not automatically unbind the
> > > > > +                device from its current driver or make any attempt to
> > > > > +                automatically load the specified driver.  If no driver with a
> > > > > +                matching name is currently loaded in the kernel, the device
> > > > > +                will not bind to any driver.  This also allows devices to
> > > > > +                opt-out of driver binding using a driver_override name such as
> > > > > +                "none".  Only a single driver may be specified in the override,
> > > > > +                there is no support for parsing delimiters.
> > > > > diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> > > > > index 511b20f..bc35750 100644
> > > > > --- a/drivers/fpga/dfl.c
> > > > > +++ b/drivers/fpga/dfl.c
> > > > > @@ -262,6 +262,10 @@ static int dfl_bus_match(struct device *dev, struct device_driver *drv)
> > > > >  	struct dfl_driver *ddrv = to_dfl_drv(drv);
> > > > >  	const struct dfl_device_id *id_entry;
> > > > >  
> > > > > +	/* When driver_override is set, only bind to the matching driver */
> > > > > +	if (ddev->driver_override)
> > > > > +		return !strcmp(ddev->driver_override, drv->name);
> > > > > +
> > > > >  	id_entry = ddrv->id_table;
> > > > >  	if (id_entry) {
> > > > >  		while (id_entry->feature_id) {
> > > > > @@ -303,6 +307,53 @@ static int dfl_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
> > > > >  			      ddev->type, ddev->feature_id);
> > > > >  }
> > > > >  
> > > > 
> > > > I am looking at other implementations of driver_override* and looking for consistency.
> > > > 
> > > > > +static ssize_t driver_override_show(struct device *dev,
> > > > > +				    struct device_attribute *attr, char *buf)
> > > > > +{
> > > > > +	struct dfl_device *ddev = to_dfl_dev(dev);
> > > > > +	ssize_t len;
> > > > > +
> > > > > +	device_lock(dev);
> > > > > +	len = sprintf(buf, "%s\n", ddev->driver_override);
> > > > len = snprintf(buf, PAGE_SIZE ...
> > > 
> > > It is good to me.
> > > 
> > > Some bus drivers use snprintf, some use sprintf.
> > > 
> > > I think it is reasonable snprintf is used here, unlike %d, %u ... it is
> > > uncertain for the output size of %s.
> > 
> > Sorry, I checked the Documentation/filesystems/sysfs.rst again and found
> > I didn't remember it correctly.
> > 
> > The snprintf is must not be used, sprintf() or scnprintf() should be
> > used. So I prefer sprintf() here, following other implementations.
> > 
> > sprintf() should be safe here, as we already limited the input of
> > driver_override string to less then PAGE_SIZE on store().
> 
> As already mentioned, please use sysfs_emit() instead.

Yes, I'll do it.

Thanks,
Yilun

> 
> thanks,
> 
> greg k-h

Re: [PATCH 1/2] fpga: dfl: add driver_override support

[Xu Yilun]

> >>>  int __dfl_driver_register(struct dfl_driver *dfl_drv, struct module *owner)
> >>>  {
> >>> -	if (!dfl_drv || !dfl_drv->probe || !dfl_drv->id_table)
> >>> +	if (!dfl_drv || !dfl_drv->probe)
> >> id_table is still needed for the normal case.
> >>
> >> Instead of removing this check, could you add something like
> >>
> >> || (!dfl_drv->is_override && !dfl_drv->id_table)
> > I don't think it is needed. Seems is_override and !id_table are duplicated
> > conditions for this implementation. And it may make confusing, e.g. could
> > a driver been force matched when is_override is not set?
> >
> > I think we could make it simple, if the dfl driver didn't provide the
> > id_table, normally it could not match any device. I think it could be
> > easily understood by dfl driver developers.
> >
> Then an ASSERT should be added in dfl_bus_match() for id_entry.

I didn't get your idea. What Assertion should be added for id_entry?
BUG_ON(id_entry == NULL)? Then dfl-uio-pdev can't be inserted before
driver_override is set.

I think it is normal case that a driver is successfully registered but
doesn't match any device because it provides no id_table.

Thanks,
Yilun.

Re: [PATCH 1/2] fpga: dfl: add driver_override support

[Greg KH]

On Tue, Oct 20, 2020 at 03:11:58PM +0800, Xu Yilun wrote:
> I think it is normal case that a driver is successfully registered but
> doesn't match any device because it provides no id_table.

How is that "normal"?  What would ever cause that driver to be bound to
a device then?

And you better not say userspace is responsible for it...

thanks,

greg k-h

Re: [PATCH 1/2] fpga: dfl: add driver_override support

[Xu Yilun]

On Tue, Oct 20, 2020 at 09:32:59AM +0200, Greg KH wrote:
> On Tue, Oct 20, 2020 at 03:11:58PM +0800, Xu Yilun wrote:
> > I think it is normal case that a driver is successfully registered but
> > doesn't match any device because it provides no id_table.
> 
> How is that "normal"?  What would ever cause that driver to be bound to
> a device then?

This patchset adds the driver_override sysfs node, to let userspace
specify which driver (by name matching) to be bound to a device. The
driver "dfl-uio-pdev" doesn't provide an id_table, it could only be
bound to a device whose driver_override is set to "dfl-uio-pdev".

Sorry, this is actually not "normal". Usually dfl drivers with valid id_table
should be used to drive the dfl devices. But we also want to give an option
for userspace to take full control of the device, some IP blocks in FPGA are
written for specific purposes by FPGA user so a userspace driver may
serve them better.

> 
> And you better not say userspace is responsible for it...

Actually it is the userspace's decision which device they want to
access directly ...

I'm not sure if this idea is OK. I see similar implementation for
pci/platform devices.

Thanks,
Yilun

> 
> thanks,
> 
> greg k-h

Re: [PATCH 1/2] fpga: dfl: add driver_override support

[Greg KH]

On Tue, Oct 20, 2020 at 04:57:23PM +0800, Xu Yilun wrote:
> On Tue, Oct 20, 2020 at 09:32:59AM +0200, Greg KH wrote:
> > On Tue, Oct 20, 2020 at 03:11:58PM +0800, Xu Yilun wrote:
> > > I think it is normal case that a driver is successfully registered but
> > > doesn't match any device because it provides no id_table.
> > 
> > How is that "normal"?  What would ever cause that driver to be bound to
> > a device then?
> 
> This patchset adds the driver_override sysfs node, to let userspace
> specify which driver (by name matching) to be bound to a device. The
> driver "dfl-uio-pdev" doesn't provide an id_table, it could only be
> bound to a device whose driver_override is set to "dfl-uio-pdev".
> 
> Sorry, this is actually not "normal". Usually dfl drivers with valid id_table
> should be used to drive the dfl devices. But we also want to give an option
> for userspace to take full control of the device, some IP blocks in FPGA are
> written for specific purposes by FPGA user so a userspace driver may
> serve them better.
> 
> > 
> > And you better not say userspace is responsible for it...
> 
> Actually it is the userspace's decision which device they want to
> access directly ...
> 
> I'm not sure if this idea is OK. I see similar implementation for
> pci/platform devices.

That is there for debugging and for being able to support things when
the kernel is not updated with a new device id yet.

The virtio people took this to a new extreme and use it to bind real
devices through to virtual machines, but really, that's horrid.  And
they have problems with it as is seen in random patches at times.

Don't make this the only way to support this, that's not an ok api.

greg k-h

Re: [PATCH 1/2] fpga: dfl: add driver_override support

[Tom Rix]

On 10/20/20 12:11 AM, Xu Yilun wrote:
>>>>>  int __dfl_driver_register(struct dfl_driver *dfl_drv, struct module *owner)
>>>>>  {
>>>>> -	if (!dfl_drv || !dfl_drv->probe || !dfl_drv->id_table)
>>>>> +	if (!dfl_drv || !dfl_drv->probe)
>>>> id_table is still needed for the normal case.
>>>>
>>>> Instead of removing this check, could you add something like
>>>>
>>>> || (!dfl_drv->is_override && !dfl_drv->id_table)
>>> I don't think it is needed. Seems is_override and !id_table are duplicated
>>> conditions for this implementation. And it may make confusing, e.g. could
>>> a driver been force matched when is_override is not set?
>>>
>>> I think we could make it simple, if the dfl driver didn't provide the
>>> id_table, normally it could not match any device. I think it could be
>>> easily understood by dfl driver developers.
>>>
>> Then an ASSERT should be added in dfl_bus_match() for id_entry.
> I didn't get your idea. What Assertion should be added for id_entry?
> BUG_ON(id_entry == NULL)? Then dfl-uio-pdev can't be inserted before
> driver_override is set.
>
> I think it is normal case that a driver is successfully registered but
> doesn't match any device because it provides no id_table.

Ok, i see your point.

Thanks for the explanation.

Tom


>
> Thanks,
> Yilun.
>

Re: [PATCH 1/2] fpga: dfl: add driver_override support

[Xu Yilun]

Hi Greg,

On Tue, Oct 20, 2020 at 11:21:55AM +0200, Greg KH wrote:
> On Tue, Oct 20, 2020 at 04:57:23PM +0800, Xu Yilun wrote:
> > On Tue, Oct 20, 2020 at 09:32:59AM +0200, Greg KH wrote:
> > > On Tue, Oct 20, 2020 at 03:11:58PM +0800, Xu Yilun wrote:
> > > > I think it is normal case that a driver is successfully registered but
> > > > doesn't match any device because it provides no id_table.
> > > 
> > > How is that "normal"?  What would ever cause that driver to be bound to
> > > a device then?
> > 
> > This patchset adds the driver_override sysfs node, to let userspace
> > specify which driver (by name matching) to be bound to a device. The
> > driver "dfl-uio-pdev" doesn't provide an id_table, it could only be
> > bound to a device whose driver_override is set to "dfl-uio-pdev".
> > 
> > Sorry, this is actually not "normal". Usually dfl drivers with valid id_table
> > should be used to drive the dfl devices. But we also want to give an option
> > for userspace to take full control of the device, some IP blocks in FPGA are
> > written for specific purposes by FPGA user so a userspace driver may
> > serve them better.
> > 
> > > 
> > > And you better not say userspace is responsible for it...
> > 
> > Actually it is the userspace's decision which device they want to
> > access directly ...
> > 
> > I'm not sure if this idea is OK. I see similar implementation for
> > pci/platform devices.
> 
> That is there for debugging and for being able to support things when
> the kernel is not updated with a new device id yet.

We are trying to use the driver_override for similar purposes on FPGA.
The FPGA card could be reprogramed with a newly developed Hardware
component by users. Sometimes only the FPGA users know which dfl device
they want to operate on. This is why we give the option to users for
direct I/O access.

Also FPGA users have the requirement for diagnostic of the newly
developed components.

It is possible we define an id_table for dfl_uio_pdev driver, but then
users have to modify the driver code everytime they create & reprogram
a new HW component.

> 
> The virtio people took this to a new extreme and use it to bind real
> devices through to virtual machines, but really, that's horrid.  And
> they have problems with it as is seen in random patches at times.

Sorry I didn't have much knowledge on this.
Does the sysfs 'driver_override' or 'new_id' have problems, or the
userspace direct I/O access mechanism (UIO/VFIO) has problems?

If it is the former, I could consider some other way to generate the UIO
devices for DFL. But could you help specify more about the issues?

Thanks very much,
Yilun

> 
> Don't make this the only way to support this, that's not an ok api.
> 
> greg k-h

Re: [PATCH 1/2] fpga: dfl: add driver_override support

[Xu Yilun]

Hi Greg,

We created a 'driver_override' api for dfl bus, and a driver for
userspace I/O access for dfl devices.

Some new comments inline.

On Wed, Oct 21, 2020 at 03:25:32PM +0800, Xu Yilun wrote:
> Hi Greg,
> 
> On Tue, Oct 20, 2020 at 11:21:55AM +0200, Greg KH wrote:
> > On Tue, Oct 20, 2020 at 04:57:23PM +0800, Xu Yilun wrote:
> > > On Tue, Oct 20, 2020 at 09:32:59AM +0200, Greg KH wrote:
> > > > On Tue, Oct 20, 2020 at 03:11:58PM +0800, Xu Yilun wrote:
> > > > > I think it is normal case that a driver is successfully registered but
> > > > > doesn't match any device because it provides no id_table.
> > > > 
> > > > How is that "normal"?  What would ever cause that driver to be bound to
> > > > a device then?
> > > 
> > > This patchset adds the driver_override sysfs node, to let userspace
> > > specify which driver (by name matching) to be bound to a device. The
> > > driver "dfl-uio-pdev" doesn't provide an id_table, it could only be
> > > bound to a device whose driver_override is set to "dfl-uio-pdev".
> > > 
> > > Sorry, this is actually not "normal". Usually dfl drivers with valid id_table
> > > should be used to drive the dfl devices. But we also want to give an option
> > > for userspace to take full control of the device, some IP blocks in FPGA are
> > > written for specific purposes by FPGA user so a userspace driver may
> > > serve them better.
> > > 
> > > > 
> > > > And you better not say userspace is responsible for it...
> > > 
> > > Actually it is the userspace's decision which device they want to
> > > access directly ...
> > > 
> > > I'm not sure if this idea is OK. I see similar implementation for
> > > pci/platform devices.
> > 
> > That is there for debugging and for being able to support things when
> > the kernel is not updated with a new device id yet.
> 
> We are trying to use the driver_override for similar purposes on FPGA.
> The FPGA card could be reprogramed with a newly developed Hardware
> component by users. Sometimes only the FPGA users know which dfl device
> they want to operate on. This is why we give the option to users for
> direct I/O access.
> 
> Also FPGA users have the requirement for diagnostic of the newly
> developed components.
> 
> It is possible we define an id_table for dfl_uio_pdev driver, but then
> users have to modify the driver code everytime they create & reprogram
> a new HW component.
> 
> > 
> > The virtio people took this to a new extreme and use it to bind real
> > devices through to virtual machines, but really, that's horrid.  And
> > they have problems with it as is seen in random patches at times.

I searched the previous patches, it seems the main issue is the user
could unbind the vfio-pci for a device and bind it to another kernel
driver, while its iommu group is still being used by vfio for other
devices in the same group, then the integrity of the group is
compromised. The conclusion is the user should be responsible for the
correct operation of bind/unbind/driver_override/new_id ...

In this patchset, we use the driver_override for UIO case, I think the
issue doesn't impact it, is it?

> > 
> > Don't make this the only way to support this, that's not an ok api.

We are not going to match driver for every dfl device by user assignment.
We are developing generic kernel drivers for them, e.g. dfl-n3000-nios,
dfl-emif, as you can see in recent patches. They follows the generic
id_table matching.

We use the driver_override for UIO case. Similar to what you've mentioned,
FPGA users develop their own hardware components on FPGA, so they need to
run diagnostic tools for white-box test.
They also need to support some hardware designed for special purposes
and does not fit into one of the standard kernel subsystems.

The FPGA users are the developer of some hardware component, they have the
knowledge of the new dfl devices. So the driver_override give the option
to the users for driver assignment.

For these purposes, is it OK we use the driver_override?


Thanks,
Yilun


> > 
> > greg k-h