[RFC] fpga: dfl: a prototype uio driver

[RFC] fpga: dfl: a prototype uio driver

[trix]

From: Tom Rix <trix@redhat.com>

From [PATCH 0/2] UIO support for dfl devices
https://lore.kernel.org/linux-fpga/1602828151-24784-1-git-send-email-yilun.xu@intel.com/

Here is an idea to have uio support with no driver override.

This makes UIO the primary driver interface because every feature
will have one and makes the existing platform driver interface
secondary.  There will be a new burden for locking write access when
they compete.

Example shows finding the fpga's temperture.

Signed-off-by: Tom Rix <trix@redhat.com>
---
 drivers/fpga/dfl-fme-main.c |  9 +++-
 drivers/fpga/dfl-uio.c      | 96 +++++++++++++++++++++++++++++++++++++
 drivers/fpga/dfl.c          | 44 ++++++++++++++++-
 drivers/fpga/dfl.h          |  9 ++++
 uio.c                       | 56 ++++++++++++++++++++++
 5 files changed, 212 insertions(+), 2 deletions(-)
 create mode 100644 drivers/fpga/dfl-uio.c
 create mode 100644 uio.c

diff --git a/drivers/fpga/dfl-fme-main.c b/drivers/fpga/dfl-fme-main.c
index 037dc4f946f0..3323e90a18c4 100644
--- a/drivers/fpga/dfl-fme-main.c
+++ b/drivers/fpga/dfl-fme-main.c
@@ -709,12 +709,18 @@ static int fme_probe(struct platform_device *pdev)
 	if (ret)
 		goto dev_destroy;
 
-	ret = dfl_fpga_dev_ops_register(pdev, &fme_fops, THIS_MODULE);
+	ret = dfl_fpga_dev_feature_init_uio(pdev, DFH_TYPE_FIU);
 	if (ret)
 		goto feature_uinit;
 
+	ret = dfl_fpga_dev_ops_register(pdev, &fme_fops, THIS_MODULE);
+	if (ret)
+		goto feature_uinit_uio;
+
 	return 0;
 
+feature_uinit_uio:
+	dfl_fpga_dev_feature_uinit_uio(pdev, DFH_TYPE_FIU);
 feature_uinit:
 	dfl_fpga_dev_feature_uinit(pdev);
 dev_destroy:
@@ -726,6 +732,7 @@ exit:
 static int fme_remove(struct platform_device *pdev)
 {
 	dfl_fpga_dev_ops_unregister(pdev);
+	dfl_fpga_dev_feature_uinit_uio(pdev, DFH_TYPE_FIU);
 	dfl_fpga_dev_feature_uinit(pdev);
 	fme_dev_destroy(pdev);
 
diff --git a/drivers/fpga/dfl-uio.c b/drivers/fpga/dfl-uio.c
new file mode 100644
index 000000000000..7610ee0b19dc
--- /dev/null
+++ b/drivers/fpga/dfl-uio.c
@@ -0,0 +1,96 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * prototype dfl uio driver
+ *
+ * Copyright Tom Rix 2020
+ */
+#include <linux/module.h>
+#include "dfl.h"
+
+static irqreturn_t dfl_uio_handler(int irq, struct uio_info *info)
+{
+	return IRQ_HANDLED;
+}
+
+static int dfl_uio_mmap(struct uio_info *info, struct vm_area_struct *vma)
+{
+	int ret = -ENODEV;
+	return ret;
+}
+
+static int dfl_uio_open(struct uio_info *info, struct inode *inode)
+{
+	int ret = -ENODEV;
+	struct dfl_feature *feature = container_of(info, struct dfl_feature, uio);
+	if (feature->dev)
+		mutex_lock(&feature->lock);
+
+	ret = 0;
+	return ret;
+}
+
+static int dfl_uio_release(struct uio_info *info, struct inode *inode)
+{
+	int ret = -ENODEV;
+	struct dfl_feature *feature = container_of(info, struct dfl_feature, uio);
+	if (feature->dev)
+		mutex_unlock(&feature->lock);
+
+	ret = 0;
+	return ret;
+}
+
+static int dfl_uio_irqcontrol(struct uio_info *info, s32 irq_on)
+{
+	int ret = -ENODEV;
+	return ret;
+}
+
+int dfl_uio_add(struct dfl_feature *feature)
+{
+	struct uio_info *uio = &feature->uio;
+	struct resource *res =
+		&feature->dev->resource[feature->resource_index];
+	int ret = 0;
+
+	uio->name = kasprintf(GFP_KERNEL, "dfl-uio-%llx", feature->id);
+	if (!uio->name) {
+		ret = -ENOMEM;
+		goto exit;
+	}
+
+	uio->version = "0.1";
+	uio->mem[0].memtype = UIO_MEM_PHYS;
+	uio->mem[0].addr = res->start & PAGE_MASK;
+	uio->mem[0].offs = res->start & ~PAGE_MASK;
+	uio->mem[0].size = (uio->mem[0].offs + resource_size(res)
+			    + PAGE_SIZE - 1) & PAGE_MASK;
+	/* How are nr_irqs > 1 handled ??? */
+	if (feature->nr_irqs == 1)
+		uio->irq = feature->irq_ctx[0].irq;
+	uio->handler = dfl_uio_handler;
+	//uio->mmap = dfl_uio_mmap;
+	uio->open = dfl_uio_open;
+	uio->release = dfl_uio_release;
+	uio->irqcontrol = dfl_uio_irqcontrol;
+
+	ret = uio_register_device(&feature->dev->dev, uio);
+	if (ret)
+		goto err_register;
+
+exit:
+	return ret;
+err_register:
+	kfree(uio->name);
+	goto exit;
+}
+EXPORT_SYMBOL_GPL(dfl_uio_add);
+
+int dfl_uio_remove(struct dfl_feature *feature)
+{
+	uio_unregister_device(&feature->uio);
+	kfree(feature->uio.name);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(dfl_uio_remove);
+
diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
index 1305be48037d..af2cd3d1b5f6 100644
--- a/drivers/fpga/dfl.c
+++ b/drivers/fpga/dfl.c
@@ -603,6 +603,7 @@ static int dfl_feature_instance_init(struct platform_device *pdev,
 	}
 
 	feature->ops = drv->ops;
+	mutex_init(&feature->lock);
 
 	return ret;
 }
@@ -663,10 +664,51 @@ exit:
 }
 EXPORT_SYMBOL_GPL(dfl_fpga_dev_feature_init);
 
+int dfl_fpga_dev_feature_init_uio(struct platform_device *pdev, int dfh_type) {
+	struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
+	struct dfl_feature *feature;
+	int ret;
+
+	dfl_fpga_dev_for_each_feature(pdata, feature) {
+		if (dfh_type == DFH_TYPE_FIU) {
+			if (feature->id == FEATURE_ID_FIU_HEADER ||
+			    feature->id == FEATURE_ID_AFU)
+			    continue;
+
+			ret = dfl_uio_add(feature);
+			if (ret)
+				goto exit;
+		}
+	}
+
+	return 0;
+exit:
+	dfl_fpga_dev_feature_uinit_uio(pdev, dfh_type);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(dfl_fpga_dev_feature_init_uio);
+
+int dfl_fpga_dev_feature_uinit_uio(struct platform_device *pdev, int dfh_type) {
+	struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
+	struct dfl_feature *feature;
+	int ret = 0;
+
+	dfl_fpga_dev_for_each_feature(pdata, feature) {
+		if (dfh_type == DFH_TYPE_FIU) {
+			if (feature->id == FEATURE_ID_FIU_HEADER ||
+			    feature->id == FEATURE_ID_AFU)
+				continue;
+
+			ret |= dfl_uio_remove(feature);
+		}
+	}
+	return ret;
+}
+EXPORT_SYMBOL_GPL(dfl_fpga_dev_feature_uinit_uio);
+
 static void dfl_chardev_uinit(void)
 {
 	int i;
-
 	for (i = 0; i < DFL_FPGA_DEVT_MAX; i++)
 		if (MAJOR(dfl_chrdevs[i].devt)) {
 			unregister_chrdev_region(dfl_chrdevs[i].devt,
diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
index a85d1cd7a130..fde0fc902d4d 100644
--- a/drivers/fpga/dfl.h
+++ b/drivers/fpga/dfl.h
@@ -26,6 +26,7 @@
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/uuid.h>
+#include <linux/uio_driver.h>
 #include <linux/fpga/fpga-region.h>
 
 /* maximum supported number of ports */
@@ -232,6 +233,7 @@ struct dfl_feature_irq_ctx {
  * struct dfl_feature - sub feature of the feature devices
  *
  * @dev: ptr to pdev of the feature device which has the sub feature.
+ * @uio: uio interface for feature.
  * @id: sub feature id.
  * @index: unique identifier for an sub feature within the feature device.
  *	   It is possible that multiply sub features with same feature id are
@@ -248,6 +250,8 @@ struct dfl_feature_irq_ctx {
  */
 struct dfl_feature {
 	struct platform_device *dev;
+	struct uio_info uio;
+	struct mutex lock; /* serialize dev and uio */
 	u64 id;
 	int index;
 	int resource_index;
@@ -360,6 +364,11 @@ void dfl_fpga_dev_feature_uinit(struct platform_device *pdev);
 int dfl_fpga_dev_feature_init(struct platform_device *pdev,
 			      struct dfl_feature_driver *feature_drvs);
 
+int dfl_fpga_dev_feature_init_uio(struct platform_device *pdev, int dfh_type);
+int dfl_fpga_dev_feature_uinit_uio(struct platform_device *pdev, int dfh_type);
+int dfl_uio_add(struct dfl_feature *feature);
+int dfl_uio_remove(struct dfl_feature *feature);
+
 int dfl_fpga_dev_ops_register(struct platform_device *pdev,
 			      const struct file_operations *fops,
 			      struct module *owner);
diff --git a/uio.c b/uio.c
new file mode 100644
index 000000000000..50210aab4822
--- /dev/null
+++ b/uio.c
@@ -0,0 +1,56 @@
+#include <stdlib.h>
+#include <stdio.h>
+#include <unistd.h>
+#include <sys/mman.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <errno.h>
+#include <stdint.h>
+
+int main()
+{
+	int fd;
+	uint64_t *ptr;
+	unsigned page_size=sysconf(_SC_PAGESIZE);
+	struct stat sb;
+
+	/*
+	 * this is fid 1, thermal mgt
+	 * ex/ 
+	 * # cat /sys/class/hwmon/hwmon3/temp1_input
+	 * 39000
+	 */
+	fd = open("/dev/uio0", O_RDONLY|O_SYNC);
+	if (fd < 0) {
+		perror("uio open:");
+		return errno;
+	}
+
+	ptr = (uint64_t *) mmap(NULL, page_size, PROT_READ, MAP_SHARED, fd, 0);
+	if (!ptr) {
+		perror("uio mmap:");
+	} else {
+
+		/* from dfl-fme-main.c :
+		 * 
+		 * #define FME_THERM_RDSENSOR_FMT1	0x10
+		 * #define FPGA_TEMPERATURE	GENMASK_ULL(6, 0)
+		 *
+		 * case hwmon_temp_input:
+		 * v = readq(feature->ioaddr + FME_THERM_RDSENSOR_FMT1);
+		 * *val = (long)(FIELD_GET(FPGA_TEMPERATURE, v) * 1000);
+		 * break;
+		 */
+		uint64_t v = ptr[2];
+		v &= (1 << 6) -1;
+		v *= 1000;
+		printf("Temperature %d\n", v);
+	    
+		munmap(ptr, page_size);
+	}
+	if (close(fd))
+		perror("uio close:");
+
+	return errno;
+}
-- 
2.18.4

Re: [RFC] fpga: dfl: a prototype uio driver

[Xu Yilun]

On Sun, Dec 06, 2020 at 01:55:54PM -0800, trix@redhat.com wrote:
> From: Tom Rix <trix@redhat.com>
> 
> >From [PATCH 0/2] UIO support for dfl devices
> https://lore.kernel.org/linux-fpga/1602828151-24784-1-git-send-email-yilun.xu@intel.com/
> 
> Here is an idea to have uio support with no driver override.
> 
> This makes UIO the primary driver interface because every feature
> will have one and makes the existing platform driver interface
> secondary.  There will be a new burden for locking write access when
> they compete.
> 
> Example shows finding the fpga's temperture.

Hi Tom:

Thanks for the idea and detailed illustrate with the patch. I see it
abandons the driver_override and expose all the DFL devices to userspace
by UIO. I'd like to see if we could get some more comments on it.

Thanks,
Yilun

> 
> Signed-off-by: Tom Rix <trix@redhat.com>
> ---
>  drivers/fpga/dfl-fme-main.c |  9 +++-
>  drivers/fpga/dfl-uio.c      | 96 +++++++++++++++++++++++++++++++++++++
>  drivers/fpga/dfl.c          | 44 ++++++++++++++++-
>  drivers/fpga/dfl.h          |  9 ++++
>  uio.c                       | 56 ++++++++++++++++++++++
>  5 files changed, 212 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/fpga/dfl-uio.c
>  create mode 100644 uio.c
> 
> diff --git a/drivers/fpga/dfl-fme-main.c b/drivers/fpga/dfl-fme-main.c
> index 037dc4f946f0..3323e90a18c4 100644
> --- a/drivers/fpga/dfl-fme-main.c
> +++ b/drivers/fpga/dfl-fme-main.c
> @@ -709,12 +709,18 @@ static int fme_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto dev_destroy;
>  
> -	ret = dfl_fpga_dev_ops_register(pdev, &fme_fops, THIS_MODULE);
> +	ret = dfl_fpga_dev_feature_init_uio(pdev, DFH_TYPE_FIU);
>  	if (ret)
>  		goto feature_uinit;
>  
> +	ret = dfl_fpga_dev_ops_register(pdev, &fme_fops, THIS_MODULE);
> +	if (ret)
> +		goto feature_uinit_uio;
> +
>  	return 0;
>  
> +feature_uinit_uio:
> +	dfl_fpga_dev_feature_uinit_uio(pdev, DFH_TYPE_FIU);
>  feature_uinit:
>  	dfl_fpga_dev_feature_uinit(pdev);
>  dev_destroy:
> @@ -726,6 +732,7 @@ exit:
>  static int fme_remove(struct platform_device *pdev)
>  {
>  	dfl_fpga_dev_ops_unregister(pdev);
> +	dfl_fpga_dev_feature_uinit_uio(pdev, DFH_TYPE_FIU);
>  	dfl_fpga_dev_feature_uinit(pdev);
>  	fme_dev_destroy(pdev);
>  
> diff --git a/drivers/fpga/dfl-uio.c b/drivers/fpga/dfl-uio.c
> new file mode 100644
> index 000000000000..7610ee0b19dc
> --- /dev/null
> +++ b/drivers/fpga/dfl-uio.c
> @@ -0,0 +1,96 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * prototype dfl uio driver
> + *
> + * Copyright Tom Rix 2020
> + */
> +#include <linux/module.h>
> +#include "dfl.h"
> +
> +static irqreturn_t dfl_uio_handler(int irq, struct uio_info *info)
> +{
> +	return IRQ_HANDLED;
> +}
> +
> +static int dfl_uio_mmap(struct uio_info *info, struct vm_area_struct *vma)
> +{
> +	int ret = -ENODEV;
> +	return ret;
> +}
> +
> +static int dfl_uio_open(struct uio_info *info, struct inode *inode)
> +{
> +	int ret = -ENODEV;
> +	struct dfl_feature *feature = container_of(info, struct dfl_feature, uio);
> +	if (feature->dev)
> +		mutex_lock(&feature->lock);
> +
> +	ret = 0;
> +	return ret;
> +}
> +
> +static int dfl_uio_release(struct uio_info *info, struct inode *inode)
> +{
> +	int ret = -ENODEV;
> +	struct dfl_feature *feature = container_of(info, struct dfl_feature, uio);
> +	if (feature->dev)
> +		mutex_unlock(&feature->lock);
> +
> +	ret = 0;
> +	return ret;
> +}
> +
> +static int dfl_uio_irqcontrol(struct uio_info *info, s32 irq_on)
> +{
> +	int ret = -ENODEV;
> +	return ret;
> +}
> +
> +int dfl_uio_add(struct dfl_feature *feature)
> +{
> +	struct uio_info *uio = &feature->uio;
> +	struct resource *res =
> +		&feature->dev->resource[feature->resource_index];
> +	int ret = 0;
> +
> +	uio->name = kasprintf(GFP_KERNEL, "dfl-uio-%llx", feature->id);
> +	if (!uio->name) {
> +		ret = -ENOMEM;
> +		goto exit;
> +	}
> +
> +	uio->version = "0.1";
> +	uio->mem[0].memtype = UIO_MEM_PHYS;
> +	uio->mem[0].addr = res->start & PAGE_MASK;
> +	uio->mem[0].offs = res->start & ~PAGE_MASK;
> +	uio->mem[0].size = (uio->mem[0].offs + resource_size(res)
> +			    + PAGE_SIZE - 1) & PAGE_MASK;
> +	/* How are nr_irqs > 1 handled ??? */
> +	if (feature->nr_irqs == 1)
> +		uio->irq = feature->irq_ctx[0].irq;
> +	uio->handler = dfl_uio_handler;
> +	//uio->mmap = dfl_uio_mmap;
> +	uio->open = dfl_uio_open;
> +	uio->release = dfl_uio_release;
> +	uio->irqcontrol = dfl_uio_irqcontrol;
> +
> +	ret = uio_register_device(&feature->dev->dev, uio);
> +	if (ret)
> +		goto err_register;
> +
> +exit:
> +	return ret;
> +err_register:
> +	kfree(uio->name);
> +	goto exit;
> +}
> +EXPORT_SYMBOL_GPL(dfl_uio_add);
> +
> +int dfl_uio_remove(struct dfl_feature *feature)
> +{
> +	uio_unregister_device(&feature->uio);
> +	kfree(feature->uio.name);
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(dfl_uio_remove);
> +
> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> index 1305be48037d..af2cd3d1b5f6 100644
> --- a/drivers/fpga/dfl.c
> +++ b/drivers/fpga/dfl.c
> @@ -603,6 +603,7 @@ static int dfl_feature_instance_init(struct platform_device *pdev,
>  	}
>  
>  	feature->ops = drv->ops;
> +	mutex_init(&feature->lock);
>  
>  	return ret;
>  }
> @@ -663,10 +664,51 @@ exit:
>  }
>  EXPORT_SYMBOL_GPL(dfl_fpga_dev_feature_init);
>  
> +int dfl_fpga_dev_feature_init_uio(struct platform_device *pdev, int dfh_type) {
> +	struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> +	struct dfl_feature *feature;
> +	int ret;
> +
> +	dfl_fpga_dev_for_each_feature(pdata, feature) {
> +		if (dfh_type == DFH_TYPE_FIU) {
> +			if (feature->id == FEATURE_ID_FIU_HEADER ||
> +			    feature->id == FEATURE_ID_AFU)
> +			    continue;
> +
> +			ret = dfl_uio_add(feature);
> +			if (ret)
> +				goto exit;
> +		}
> +	}
> +
> +	return 0;
> +exit:
> +	dfl_fpga_dev_feature_uinit_uio(pdev, dfh_type);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(dfl_fpga_dev_feature_init_uio);
> +
> +int dfl_fpga_dev_feature_uinit_uio(struct platform_device *pdev, int dfh_type) {
> +	struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> +	struct dfl_feature *feature;
> +	int ret = 0;
> +
> +	dfl_fpga_dev_for_each_feature(pdata, feature) {
> +		if (dfh_type == DFH_TYPE_FIU) {
> +			if (feature->id == FEATURE_ID_FIU_HEADER ||
> +			    feature->id == FEATURE_ID_AFU)
> +				continue;
> +
> +			ret |= dfl_uio_remove(feature);
> +		}
> +	}
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(dfl_fpga_dev_feature_uinit_uio);
> +
>  static void dfl_chardev_uinit(void)
>  {
>  	int i;
> -
>  	for (i = 0; i < DFL_FPGA_DEVT_MAX; i++)
>  		if (MAJOR(dfl_chrdevs[i].devt)) {
>  			unregister_chrdev_region(dfl_chrdevs[i].devt,
> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> index a85d1cd7a130..fde0fc902d4d 100644
> --- a/drivers/fpga/dfl.h
> +++ b/drivers/fpga/dfl.h
> @@ -26,6 +26,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
>  #include <linux/uuid.h>
> +#include <linux/uio_driver.h>
>  #include <linux/fpga/fpga-region.h>
>  
>  /* maximum supported number of ports */
> @@ -232,6 +233,7 @@ struct dfl_feature_irq_ctx {
>   * struct dfl_feature - sub feature of the feature devices
>   *
>   * @dev: ptr to pdev of the feature device which has the sub feature.
> + * @uio: uio interface for feature.
>   * @id: sub feature id.
>   * @index: unique identifier for an sub feature within the feature device.
>   *	   It is possible that multiply sub features with same feature id are
> @@ -248,6 +250,8 @@ struct dfl_feature_irq_ctx {
>   */
>  struct dfl_feature {
>  	struct platform_device *dev;
> +	struct uio_info uio;
> +	struct mutex lock; /* serialize dev and uio */
>  	u64 id;
>  	int index;
>  	int resource_index;
> @@ -360,6 +364,11 @@ void dfl_fpga_dev_feature_uinit(struct platform_device *pdev);
>  int dfl_fpga_dev_feature_init(struct platform_device *pdev,
>  			      struct dfl_feature_driver *feature_drvs);
>  
> +int dfl_fpga_dev_feature_init_uio(struct platform_device *pdev, int dfh_type);
> +int dfl_fpga_dev_feature_uinit_uio(struct platform_device *pdev, int dfh_type);
> +int dfl_uio_add(struct dfl_feature *feature);
> +int dfl_uio_remove(struct dfl_feature *feature);
> +
>  int dfl_fpga_dev_ops_register(struct platform_device *pdev,
>  			      const struct file_operations *fops,
>  			      struct module *owner);
> diff --git a/uio.c b/uio.c
> new file mode 100644
> index 000000000000..50210aab4822
> --- /dev/null
> +++ b/uio.c
> @@ -0,0 +1,56 @@
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <unistd.h>
> +#include <sys/mman.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <fcntl.h>
> +#include <errno.h>
> +#include <stdint.h>
> +
> +int main()
> +{
> +	int fd;
> +	uint64_t *ptr;
> +	unsigned page_size=sysconf(_SC_PAGESIZE);
> +	struct stat sb;
> +
> +	/*
> +	 * this is fid 1, thermal mgt
> +	 * ex/ 
> +	 * # cat /sys/class/hwmon/hwmon3/temp1_input
> +	 * 39000
> +	 */
> +	fd = open("/dev/uio0", O_RDONLY|O_SYNC);
> +	if (fd < 0) {
> +		perror("uio open:");
> +		return errno;
> +	}
> +
> +	ptr = (uint64_t *) mmap(NULL, page_size, PROT_READ, MAP_SHARED, fd, 0);
> +	if (!ptr) {
> +		perror("uio mmap:");
> +	} else {
> +
> +		/* from dfl-fme-main.c :
> +		 * 
> +		 * #define FME_THERM_RDSENSOR_FMT1	0x10
> +		 * #define FPGA_TEMPERATURE	GENMASK_ULL(6, 0)
> +		 *
> +		 * case hwmon_temp_input:
> +		 * v = readq(feature->ioaddr + FME_THERM_RDSENSOR_FMT1);
> +		 * *val = (long)(FIELD_GET(FPGA_TEMPERATURE, v) * 1000);
> +		 * break;
> +		 */
> +		uint64_t v = ptr[2];
> +		v &= (1 << 6) -1;
> +		v *= 1000;
> +		printf("Temperature %d\n", v);
> +	    
> +		munmap(ptr, page_size);
> +	}
> +	if (close(fd))
> +		perror("uio close:");
> +
> +	return errno;
> +}
> -- 
> 2.18.4

RE: [RFC] fpga: dfl: a prototype uio driver

[Wu, Hao]

> Subject: Re: [RFC] fpga: dfl: a prototype uio driver
> 
> On Sun, Dec 06, 2020 at 01:55:54PM -0800, trix@redhat.com wrote:
> > From: Tom Rix <trix@redhat.com>
> >
> > >From [PATCH 0/2] UIO support for dfl devices
> > https://lore.kernel.org/linux-fpga/1602828151-24784-1-git-send-email-
> yilun.xu@intel.com/
> >
> > Here is an idea to have uio support with no driver override.
> >
> > This makes UIO the primary driver interface because every feature
> > will have one and makes the existing platform driver interface
> > secondary.  There will be a new burden for locking write access when
> > they compete.
> >
> > Example shows finding the fpga's temperture.
> 
> Hi Tom:
> 
> Thanks for the idea and detailed illustrate with the patch. I see it
> abandons the driver_override and expose all the DFL devices to userspace
> by UIO. I'd like to see if we could get some more comments on it.

This allows concurrent access from both userspace and kernel space driver
to the same feature device? conflicts?

Hao

> 
> Thanks,
> Yilun
> 
> >
> > Signed-off-by: Tom Rix <trix@redhat.com>
> > ---
> >  drivers/fpga/dfl-fme-main.c |  9 +++-
> >  drivers/fpga/dfl-uio.c      | 96 +++++++++++++++++++++++++++++++++++++
> >  drivers/fpga/dfl.c          | 44 ++++++++++++++++-
> >  drivers/fpga/dfl.h          |  9 ++++
> >  uio.c                       | 56 ++++++++++++++++++++++
> >  5 files changed, 212 insertions(+), 2 deletions(-)
> >  create mode 100644 drivers/fpga/dfl-uio.c
> >  create mode 100644 uio.c
> >
> > diff --git a/drivers/fpga/dfl-fme-main.c b/drivers/fpga/dfl-fme-main.c
> > index 037dc4f946f0..3323e90a18c4 100644
> > --- a/drivers/fpga/dfl-fme-main.c
> > +++ b/drivers/fpga/dfl-fme-main.c
> > @@ -709,12 +709,18 @@ static int fme_probe(struct platform_device
> *pdev)
> >  	if (ret)
> >  		goto dev_destroy;
> >
> > -	ret = dfl_fpga_dev_ops_register(pdev, &fme_fops, THIS_MODULE);
> > +	ret = dfl_fpga_dev_feature_init_uio(pdev, DFH_TYPE_FIU);
> >  	if (ret)
> >  		goto feature_uinit;
> >
> > +	ret = dfl_fpga_dev_ops_register(pdev, &fme_fops, THIS_MODULE);
> > +	if (ret)
> > +		goto feature_uinit_uio;
> > +
> >  	return 0;
> >
> > +feature_uinit_uio:
> > +	dfl_fpga_dev_feature_uinit_uio(pdev, DFH_TYPE_FIU);
> >  feature_uinit:
> >  	dfl_fpga_dev_feature_uinit(pdev);
> >  dev_destroy:
> > @@ -726,6 +732,7 @@ exit:
> >  static int fme_remove(struct platform_device *pdev)
> >  {
> >  	dfl_fpga_dev_ops_unregister(pdev);
> > +	dfl_fpga_dev_feature_uinit_uio(pdev, DFH_TYPE_FIU);
> >  	dfl_fpga_dev_feature_uinit(pdev);
> >  	fme_dev_destroy(pdev);
> >
> > diff --git a/drivers/fpga/dfl-uio.c b/drivers/fpga/dfl-uio.c
> > new file mode 100644
> > index 000000000000..7610ee0b19dc
> > --- /dev/null
> > +++ b/drivers/fpga/dfl-uio.c
> > @@ -0,0 +1,96 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * prototype dfl uio driver
> > + *
> > + * Copyright Tom Rix 2020
> > + */
> > +#include <linux/module.h>
> > +#include "dfl.h"
> > +
> > +static irqreturn_t dfl_uio_handler(int irq, struct uio_info *info)
> > +{
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int dfl_uio_mmap(struct uio_info *info, struct vm_area_struct *vma)
> > +{
> > +	int ret = -ENODEV;
> > +	return ret;
> > +}
> > +
> > +static int dfl_uio_open(struct uio_info *info, struct inode *inode)
> > +{
> > +	int ret = -ENODEV;
> > +	struct dfl_feature *feature = container_of(info, struct dfl_feature,
> uio);
> > +	if (feature->dev)
> > +		mutex_lock(&feature->lock);
> > +
> > +	ret = 0;
> > +	return ret;
> > +}
> > +
> > +static int dfl_uio_release(struct uio_info *info, struct inode *inode)
> > +{
> > +	int ret = -ENODEV;
> > +	struct dfl_feature *feature = container_of(info, struct dfl_feature,
> uio);
> > +	if (feature->dev)
> > +		mutex_unlock(&feature->lock);
> > +
> > +	ret = 0;
> > +	return ret;
> > +}
> > +
> > +static int dfl_uio_irqcontrol(struct uio_info *info, s32 irq_on)
> > +{
> > +	int ret = -ENODEV;
> > +	return ret;
> > +}
> > +
> > +int dfl_uio_add(struct dfl_feature *feature)
> > +{
> > +	struct uio_info *uio = &feature->uio;
> > +	struct resource *res =
> > +		&feature->dev->resource[feature->resource_index];
> > +	int ret = 0;
> > +
> > +	uio->name = kasprintf(GFP_KERNEL, "dfl-uio-%llx", feature->id);
> > +	if (!uio->name) {
> > +		ret = -ENOMEM;
> > +		goto exit;
> > +	}
> > +
> > +	uio->version = "0.1";
> > +	uio->mem[0].memtype = UIO_MEM_PHYS;
> > +	uio->mem[0].addr = res->start & PAGE_MASK;
> > +	uio->mem[0].offs = res->start & ~PAGE_MASK;
> > +	uio->mem[0].size = (uio->mem[0].offs + resource_size(res)
> > +			    + PAGE_SIZE - 1) & PAGE_MASK;
> > +	/* How are nr_irqs > 1 handled ??? */
> > +	if (feature->nr_irqs == 1)
> > +		uio->irq = feature->irq_ctx[0].irq;
> > +	uio->handler = dfl_uio_handler;
> > +	//uio->mmap = dfl_uio_mmap;
> > +	uio->open = dfl_uio_open;
> > +	uio->release = dfl_uio_release;
> > +	uio->irqcontrol = dfl_uio_irqcontrol;
> > +
> > +	ret = uio_register_device(&feature->dev->dev, uio);
> > +	if (ret)
> > +		goto err_register;
> > +
> > +exit:
> > +	return ret;
> > +err_register:
> > +	kfree(uio->name);
> > +	goto exit;
> > +}
> > +EXPORT_SYMBOL_GPL(dfl_uio_add);
> > +
> > +int dfl_uio_remove(struct dfl_feature *feature)
> > +{
> > +	uio_unregister_device(&feature->uio);
> > +	kfree(feature->uio.name);
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(dfl_uio_remove);
> > +
> > diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> > index 1305be48037d..af2cd3d1b5f6 100644
> > --- a/drivers/fpga/dfl.c
> > +++ b/drivers/fpga/dfl.c
> > @@ -603,6 +603,7 @@ static int dfl_feature_instance_init(struct
> platform_device *pdev,
> >  	}
> >
> >  	feature->ops = drv->ops;
> > +	mutex_init(&feature->lock);
> >
> >  	return ret;
> >  }
> > @@ -663,10 +664,51 @@ exit:
> >  }
> >  EXPORT_SYMBOL_GPL(dfl_fpga_dev_feature_init);
> >
> > +int dfl_fpga_dev_feature_init_uio(struct platform_device *pdev, int
> dfh_type) {
> > +	struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev-
> >dev);
> > +	struct dfl_feature *feature;
> > +	int ret;
> > +
> > +	dfl_fpga_dev_for_each_feature(pdata, feature) {
> > +		if (dfh_type == DFH_TYPE_FIU) {
> > +			if (feature->id == FEATURE_ID_FIU_HEADER ||
> > +			    feature->id == FEATURE_ID_AFU)
> > +			    continue;
> > +
> > +			ret = dfl_uio_add(feature);
> > +			if (ret)
> > +				goto exit;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +exit:
> > +	dfl_fpga_dev_feature_uinit_uio(pdev, dfh_type);
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(dfl_fpga_dev_feature_init_uio);
> > +
> > +int dfl_fpga_dev_feature_uinit_uio(struct platform_device *pdev, int
> dfh_type) {
> > +	struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev-
> >dev);
> > +	struct dfl_feature *feature;
> > +	int ret = 0;
> > +
> > +	dfl_fpga_dev_for_each_feature(pdata, feature) {
> > +		if (dfh_type == DFH_TYPE_FIU) {
> > +			if (feature->id == FEATURE_ID_FIU_HEADER ||
> > +			    feature->id == FEATURE_ID_AFU)
> > +				continue;
> > +
> > +			ret |= dfl_uio_remove(feature);
> > +		}
> > +	}
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(dfl_fpga_dev_feature_uinit_uio);
> > +
> >  static void dfl_chardev_uinit(void)
> >  {
> >  	int i;
> > -
> >  	for (i = 0; i < DFL_FPGA_DEVT_MAX; i++)
> >  		if (MAJOR(dfl_chrdevs[i].devt)) {
> >  			unregister_chrdev_region(dfl_chrdevs[i].devt,
> > diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> > index a85d1cd7a130..fde0fc902d4d 100644
> > --- a/drivers/fpga/dfl.h
> > +++ b/drivers/fpga/dfl.h
> > @@ -26,6 +26,7 @@
> >  #include <linux/platform_device.h>
> >  #include <linux/slab.h>
> >  #include <linux/uuid.h>
> > +#include <linux/uio_driver.h>
> >  #include <linux/fpga/fpga-region.h>
> >
> >  /* maximum supported number of ports */
> > @@ -232,6 +233,7 @@ struct dfl_feature_irq_ctx {
> >   * struct dfl_feature - sub feature of the feature devices
> >   *
> >   * @dev: ptr to pdev of the feature device which has the sub feature.
> > + * @uio: uio interface for feature.
> >   * @id: sub feature id.
> >   * @index: unique identifier for an sub feature within the feature device.
> >   *	   It is possible that multiply sub features with same feature id are
> > @@ -248,6 +250,8 @@ struct dfl_feature_irq_ctx {
> >   */
> >  struct dfl_feature {
> >  	struct platform_device *dev;
> > +	struct uio_info uio;
> > +	struct mutex lock; /* serialize dev and uio */
> >  	u64 id;
> >  	int index;
> >  	int resource_index;
> > @@ -360,6 +364,11 @@ void dfl_fpga_dev_feature_uinit(struct
> platform_device *pdev);
> >  int dfl_fpga_dev_feature_init(struct platform_device *pdev,
> >  			      struct dfl_feature_driver *feature_drvs);
> >
> > +int dfl_fpga_dev_feature_init_uio(struct platform_device *pdev, int
> dfh_type);
> > +int dfl_fpga_dev_feature_uinit_uio(struct platform_device *pdev, int
> dfh_type);
> > +int dfl_uio_add(struct dfl_feature *feature);
> > +int dfl_uio_remove(struct dfl_feature *feature);
> > +
> >  int dfl_fpga_dev_ops_register(struct platform_device *pdev,
> >  			      const struct file_operations *fops,
> >  			      struct module *owner);
> > diff --git a/uio.c b/uio.c
> > new file mode 100644
> > index 000000000000..50210aab4822
> > --- /dev/null
> > +++ b/uio.c
> > @@ -0,0 +1,56 @@
> > +#include <stdlib.h>
> > +#include <stdio.h>
> > +#include <unistd.h>
> > +#include <sys/mman.h>
> > +#include <sys/types.h>
> > +#include <sys/stat.h>
> > +#include <fcntl.h>
> > +#include <errno.h>
> > +#include <stdint.h>
> > +
> > +int main()
> > +{
> > +	int fd;
> > +	uint64_t *ptr;
> > +	unsigned page_size=sysconf(_SC_PAGESIZE);
> > +	struct stat sb;
> > +
> > +	/*
> > +	 * this is fid 1, thermal mgt
> > +	 * ex/
> > +	 * # cat /sys/class/hwmon/hwmon3/temp1_input
> > +	 * 39000
> > +	 */
> > +	fd = open("/dev/uio0", O_RDONLY|O_SYNC);
> > +	if (fd < 0) {
> > +		perror("uio open:");
> > +		return errno;
> > +	}
> > +
> > +	ptr = (uint64_t *) mmap(NULL, page_size, PROT_READ, MAP_SHARED,
> fd, 0);
> > +	if (!ptr) {
> > +		perror("uio mmap:");
> > +	} else {
> > +
> > +		/* from dfl-fme-main.c :
> > +		 *
> > +		 * #define FME_THERM_RDSENSOR_FMT1	0x10
> > +		 * #define FPGA_TEMPERATURE	GENMASK_ULL(6, 0)
> > +		 *
> > +		 * case hwmon_temp_input:
> > +		 * v = readq(feature->ioaddr +
> FME_THERM_RDSENSOR_FMT1);
> > +		 * *val = (long)(FIELD_GET(FPGA_TEMPERATURE, v) * 1000);
> > +		 * break;
> > +		 */
> > +		uint64_t v = ptr[2];
> > +		v &= (1 << 6) -1;
> > +		v *= 1000;
> > +		printf("Temperature %d\n", v);
> > +
> > +		munmap(ptr, page_size);
> > +	}
> > +	if (close(fd))
> > +		perror("uio close:");
> > +
> > +	return errno;
> > +}
> > --
> > 2.18.4

Re: [RFC] fpga: dfl: a prototype uio driver

[Greg KH]

On Sun, Dec 06, 2020 at 01:55:54PM -0800, trix@redhat.com wrote:
> From: Tom Rix <trix@redhat.com>
> 
> >From [PATCH 0/2] UIO support for dfl devices
> https://lore.kernel.org/linux-fpga/1602828151-24784-1-git-send-email-yilun.xu@intel.com/

Why is this here?
> 
> Here is an idea to have uio support with no driver override.
> 
> This makes UIO the primary driver interface because every feature
> will have one and makes the existing platform driver interface
> secondary.  There will be a new burden for locking write access when
> they compete.
> 
> Example shows finding the fpga's temperture.
> 
> Signed-off-by: Tom Rix <trix@redhat.com>
> ---
>  drivers/fpga/dfl-fme-main.c |  9 +++-
>  drivers/fpga/dfl-uio.c      | 96 +++++++++++++++++++++++++++++++++++++
>  drivers/fpga/dfl.c          | 44 ++++++++++++++++-
>  drivers/fpga/dfl.h          |  9 ++++
>  uio.c                       | 56 ++++++++++++++++++++++
>  5 files changed, 212 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/fpga/dfl-uio.c
>  create mode 100644 uio.c
> 
> diff --git a/drivers/fpga/dfl-fme-main.c b/drivers/fpga/dfl-fme-main.c
> index 037dc4f946f0..3323e90a18c4 100644
> --- a/drivers/fpga/dfl-fme-main.c
> +++ b/drivers/fpga/dfl-fme-main.c
> @@ -709,12 +709,18 @@ static int fme_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto dev_destroy;
>  
> -	ret = dfl_fpga_dev_ops_register(pdev, &fme_fops, THIS_MODULE);
> +	ret = dfl_fpga_dev_feature_init_uio(pdev, DFH_TYPE_FIU);
>  	if (ret)
>  		goto feature_uinit;
>  
> +	ret = dfl_fpga_dev_ops_register(pdev, &fme_fops, THIS_MODULE);
> +	if (ret)
> +		goto feature_uinit_uio;
> +
>  	return 0;
>  
> +feature_uinit_uio:
> +	dfl_fpga_dev_feature_uinit_uio(pdev, DFH_TYPE_FIU);
>  feature_uinit:
>  	dfl_fpga_dev_feature_uinit(pdev);
>  dev_destroy:
> @@ -726,6 +732,7 @@ exit:
>  static int fme_remove(struct platform_device *pdev)
>  {
>  	dfl_fpga_dev_ops_unregister(pdev);
> +	dfl_fpga_dev_feature_uinit_uio(pdev, DFH_TYPE_FIU);
>  	dfl_fpga_dev_feature_uinit(pdev);
>  	fme_dev_destroy(pdev);
>  
> diff --git a/drivers/fpga/dfl-uio.c b/drivers/fpga/dfl-uio.c
> new file mode 100644
> index 000000000000..7610ee0b19dc
> --- /dev/null
> +++ b/drivers/fpga/dfl-uio.c
> @@ -0,0 +1,96 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * prototype dfl uio driver
> + *
> + * Copyright Tom Rix 2020
> + */
> +#include <linux/module.h>
> +#include "dfl.h"
> +
> +static irqreturn_t dfl_uio_handler(int irq, struct uio_info *info)
> +{
> +	return IRQ_HANDLED;
> +}
> +
> +static int dfl_uio_mmap(struct uio_info *info, struct vm_area_struct *vma)
> +{
> +	int ret = -ENODEV;
> +	return ret;

Did you run this through checkpatch?

Does the code make sense?

> +}
> +
> +static int dfl_uio_open(struct uio_info *info, struct inode *inode)
> +{
> +	int ret = -ENODEV;
> +	struct dfl_feature *feature = container_of(info, struct dfl_feature, uio);
> +	if (feature->dev)
> +		mutex_lock(&feature->lock);
> +
> +	ret = 0;
> +	return ret;

Same here, does this make sense?

And wait, you are having userspace grab a kernel lock?  What could go
wrong?  :(


> +}
> +
> +static int dfl_uio_release(struct uio_info *info, struct inode *inode)
> +{
> +	int ret = -ENODEV;
> +	struct dfl_feature *feature = container_of(info, struct dfl_feature, uio);
> +	if (feature->dev)
> +		mutex_unlock(&feature->lock);
> +
> +	ret = 0;
> +	return ret;
> +}
> +
> +static int dfl_uio_irqcontrol(struct uio_info *info, s32 irq_on)
> +{
> +	int ret = -ENODEV;
> +	return ret;
> +}
> +
> +int dfl_uio_add(struct dfl_feature *feature)
> +{
> +	struct uio_info *uio = &feature->uio;
> +	struct resource *res =
> +		&feature->dev->resource[feature->resource_index];
> +	int ret = 0;
> +
> +	uio->name = kasprintf(GFP_KERNEL, "dfl-uio-%llx", feature->id);
> +	if (!uio->name) {
> +		ret = -ENOMEM;
> +		goto exit;
> +	}
> +
> +	uio->version = "0.1";
> +	uio->mem[0].memtype = UIO_MEM_PHYS;
> +	uio->mem[0].addr = res->start & PAGE_MASK;
> +	uio->mem[0].offs = res->start & ~PAGE_MASK;
> +	uio->mem[0].size = (uio->mem[0].offs + resource_size(res)
> +			    + PAGE_SIZE - 1) & PAGE_MASK;
> +	/* How are nr_irqs > 1 handled ??? */
> +	if (feature->nr_irqs == 1)
> +		uio->irq = feature->irq_ctx[0].irq;
> +	uio->handler = dfl_uio_handler;
> +	//uio->mmap = dfl_uio_mmap;

???

I don't understand what this patch is trying to show...

thanks,

greg k-h

Re: [RFC] fpga: dfl: a prototype uio driver

[Tom Rix]

On 12/6/20 10:24 PM, Wu, Hao wrote:
>> Subject: Re: [RFC] fpga: dfl: a prototype uio driver
>>
>> On Sun, Dec 06, 2020 at 01:55:54PM -0800, trix@redhat.com wrote:
>>> From: Tom Rix <trix@redhat.com>
>>>
>>> >From [PATCH 0/2] UIO support for dfl devices
>>> https://lore.kernel.org/linux-fpga/1602828151-24784-1-git-send-email-
>> yilun.xu@intel.com/
>>> Here is an idea to have uio support with no driver override.
>>>
>>> This makes UIO the primary driver interface because every feature
>>> will have one and makes the existing platform driver interface
>>> secondary.  There will be a new burden for locking write access when
>>> they compete.
>>>
>>> Example shows finding the fpga's temperture.
>> Hi Tom:
>>
>> Thanks for the idea and detailed illustrate with the patch. I see it
>> abandons the driver_override and expose all the DFL devices to userspace
>> by UIO. I'd like to see if we could get some more comments on it.
> This allows concurrent access from both userspace and kernel space driver
> to the same feature device? conflicts?

Yes conflicts, that is why a lock is needed and the unpleasant part of this idea.

Another way is split them and have uio for the unclaimed feature id's

Tom

>
> Hao
>
>> Thanks,
>> Yilun
>>
>>> Signed-off-by: Tom Rix <trix@redhat.com>
>>> ---
>>>  drivers/fpga/dfl-fme-main.c |  9 +++-
>>>  drivers/fpga/dfl-uio.c      | 96 +++++++++++++++++++++++++++++++++++++
>>>  drivers/fpga/dfl.c          | 44 ++++++++++++++++-
>>>  drivers/fpga/dfl.h          |  9 ++++
>>>  uio.c                       | 56 ++++++++++++++++++++++
>>>  5 files changed, 212 insertions(+), 2 deletions(-)
>>>  create mode 100644 drivers/fpga/dfl-uio.c
>>>  create mode 100644 uio.c
>>>
>>> diff --git a/drivers/fpga/dfl-fme-main.c b/drivers/fpga/dfl-fme-main.c
>>> index 037dc4f946f0..3323e90a18c4 100644
>>> --- a/drivers/fpga/dfl-fme-main.c
>>> +++ b/drivers/fpga/dfl-fme-main.c
>>> @@ -709,12 +709,18 @@ static int fme_probe(struct platform_device
>> *pdev)
>>>  	if (ret)
>>>  		goto dev_destroy;
>>>
>>> -	ret = dfl_fpga_dev_ops_register(pdev, &fme_fops, THIS_MODULE);
>>> +	ret = dfl_fpga_dev_feature_init_uio(pdev, DFH_TYPE_FIU);
>>>  	if (ret)
>>>  		goto feature_uinit;
>>>
>>> +	ret = dfl_fpga_dev_ops_register(pdev, &fme_fops, THIS_MODULE);
>>> +	if (ret)
>>> +		goto feature_uinit_uio;
>>> +
>>>  	return 0;
>>>
>>> +feature_uinit_uio:
>>> +	dfl_fpga_dev_feature_uinit_uio(pdev, DFH_TYPE_FIU);
>>>  feature_uinit:
>>>  	dfl_fpga_dev_feature_uinit(pdev);
>>>  dev_destroy:
>>> @@ -726,6 +732,7 @@ exit:
>>>  static int fme_remove(struct platform_device *pdev)
>>>  {
>>>  	dfl_fpga_dev_ops_unregister(pdev);
>>> +	dfl_fpga_dev_feature_uinit_uio(pdev, DFH_TYPE_FIU);
>>>  	dfl_fpga_dev_feature_uinit(pdev);
>>>  	fme_dev_destroy(pdev);
>>>
>>> diff --git a/drivers/fpga/dfl-uio.c b/drivers/fpga/dfl-uio.c
>>> new file mode 100644
>>> index 000000000000..7610ee0b19dc
>>> --- /dev/null
>>> +++ b/drivers/fpga/dfl-uio.c
>>> @@ -0,0 +1,96 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +/*
>>> + * prototype dfl uio driver
>>> + *
>>> + * Copyright Tom Rix 2020
>>> + */
>>> +#include <linux/module.h>
>>> +#include "dfl.h"
>>> +
>>> +static irqreturn_t dfl_uio_handler(int irq, struct uio_info *info)
>>> +{
>>> +	return IRQ_HANDLED;
>>> +}
>>> +
>>> +static int dfl_uio_mmap(struct uio_info *info, struct vm_area_struct *vma)
>>> +{
>>> +	int ret = -ENODEV;
>>> +	return ret;
>>> +}
>>> +
>>> +static int dfl_uio_open(struct uio_info *info, struct inode *inode)
>>> +{
>>> +	int ret = -ENODEV;
>>> +	struct dfl_feature *feature = container_of(info, struct dfl_feature,
>> uio);
>>> +	if (feature->dev)
>>> +		mutex_lock(&feature->lock);
>>> +
>>> +	ret = 0;
>>> +	return ret;
>>> +}
>>> +
>>> +static int dfl_uio_release(struct uio_info *info, struct inode *inode)
>>> +{
>>> +	int ret = -ENODEV;
>>> +	struct dfl_feature *feature = container_of(info, struct dfl_feature,
>> uio);
>>> +	if (feature->dev)
>>> +		mutex_unlock(&feature->lock);
>>> +
>>> +	ret = 0;
>>> +	return ret;
>>> +}
>>> +
>>> +static int dfl_uio_irqcontrol(struct uio_info *info, s32 irq_on)
>>> +{
>>> +	int ret = -ENODEV;
>>> +	return ret;
>>> +}
>>> +
>>> +int dfl_uio_add(struct dfl_feature *feature)
>>> +{
>>> +	struct uio_info *uio = &feature->uio;
>>> +	struct resource *res =
>>> +		&feature->dev->resource[feature->resource_index];
>>> +	int ret = 0;
>>> +
>>> +	uio->name = kasprintf(GFP_KERNEL, "dfl-uio-%llx", feature->id);
>>> +	if (!uio->name) {
>>> +		ret = -ENOMEM;
>>> +		goto exit;
>>> +	}
>>> +
>>> +	uio->version = "0.1";
>>> +	uio->mem[0].memtype = UIO_MEM_PHYS;
>>> +	uio->mem[0].addr = res->start & PAGE_MASK;
>>> +	uio->mem[0].offs = res->start & ~PAGE_MASK;
>>> +	uio->mem[0].size = (uio->mem[0].offs + resource_size(res)
>>> +			    + PAGE_SIZE - 1) & PAGE_MASK;
>>> +	/* How are nr_irqs > 1 handled ??? */
>>> +	if (feature->nr_irqs == 1)
>>> +		uio->irq = feature->irq_ctx[0].irq;
>>> +	uio->handler = dfl_uio_handler;
>>> +	//uio->mmap = dfl_uio_mmap;
>>> +	uio->open = dfl_uio_open;
>>> +	uio->release = dfl_uio_release;
>>> +	uio->irqcontrol = dfl_uio_irqcontrol;
>>> +
>>> +	ret = uio_register_device(&feature->dev->dev, uio);
>>> +	if (ret)
>>> +		goto err_register;
>>> +
>>> +exit:
>>> +	return ret;
>>> +err_register:
>>> +	kfree(uio->name);
>>> +	goto exit;
>>> +}
>>> +EXPORT_SYMBOL_GPL(dfl_uio_add);
>>> +
>>> +int dfl_uio_remove(struct dfl_feature *feature)
>>> +{
>>> +	uio_unregister_device(&feature->uio);
>>> +	kfree(feature->uio.name);
>>> +	return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(dfl_uio_remove);
>>> +
>>> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
>>> index 1305be48037d..af2cd3d1b5f6 100644
>>> --- a/drivers/fpga/dfl.c
>>> +++ b/drivers/fpga/dfl.c
>>> @@ -603,6 +603,7 @@ static int dfl_feature_instance_init(struct
>> platform_device *pdev,
>>>  	}
>>>
>>>  	feature->ops = drv->ops;
>>> +	mutex_init(&feature->lock);
>>>
>>>  	return ret;
>>>  }
>>> @@ -663,10 +664,51 @@ exit:
>>>  }
>>>  EXPORT_SYMBOL_GPL(dfl_fpga_dev_feature_init);
>>>
>>> +int dfl_fpga_dev_feature_init_uio(struct platform_device *pdev, int
>> dfh_type) {
>>> +	struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev-
>>> dev);
>>> +	struct dfl_feature *feature;
>>> +	int ret;
>>> +
>>> +	dfl_fpga_dev_for_each_feature(pdata, feature) {
>>> +		if (dfh_type == DFH_TYPE_FIU) {
>>> +			if (feature->id == FEATURE_ID_FIU_HEADER ||
>>> +			    feature->id == FEATURE_ID_AFU)
>>> +			    continue;
>>> +
>>> +			ret = dfl_uio_add(feature);
>>> +			if (ret)
>>> +				goto exit;
>>> +		}
>>> +	}
>>> +
>>> +	return 0;
>>> +exit:
>>> +	dfl_fpga_dev_feature_uinit_uio(pdev, dfh_type);
>>> +	return ret;
>>> +}
>>> +EXPORT_SYMBOL_GPL(dfl_fpga_dev_feature_init_uio);
>>> +
>>> +int dfl_fpga_dev_feature_uinit_uio(struct platform_device *pdev, int
>> dfh_type) {
>>> +	struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev-
>>> dev);
>>> +	struct dfl_feature *feature;
>>> +	int ret = 0;
>>> +
>>> +	dfl_fpga_dev_for_each_feature(pdata, feature) {
>>> +		if (dfh_type == DFH_TYPE_FIU) {
>>> +			if (feature->id == FEATURE_ID_FIU_HEADER ||
>>> +			    feature->id == FEATURE_ID_AFU)
>>> +				continue;
>>> +
>>> +			ret |= dfl_uio_remove(feature);
>>> +		}
>>> +	}
>>> +	return ret;
>>> +}
>>> +EXPORT_SYMBOL_GPL(dfl_fpga_dev_feature_uinit_uio);
>>> +
>>>  static void dfl_chardev_uinit(void)
>>>  {
>>>  	int i;
>>> -
>>>  	for (i = 0; i < DFL_FPGA_DEVT_MAX; i++)
>>>  		if (MAJOR(dfl_chrdevs[i].devt)) {
>>>  			unregister_chrdev_region(dfl_chrdevs[i].devt,
>>> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
>>> index a85d1cd7a130..fde0fc902d4d 100644
>>> --- a/drivers/fpga/dfl.h
>>> +++ b/drivers/fpga/dfl.h
>>> @@ -26,6 +26,7 @@
>>>  #include <linux/platform_device.h>
>>>  #include <linux/slab.h>
>>>  #include <linux/uuid.h>
>>> +#include <linux/uio_driver.h>
>>>  #include <linux/fpga/fpga-region.h>
>>>
>>>  /* maximum supported number of ports */
>>> @@ -232,6 +233,7 @@ struct dfl_feature_irq_ctx {
>>>   * struct dfl_feature - sub feature of the feature devices
>>>   *
>>>   * @dev: ptr to pdev of the feature device which has the sub feature.
>>> + * @uio: uio interface for feature.
>>>   * @id: sub feature id.
>>>   * @index: unique identifier for an sub feature within the feature device.
>>>   *	   It is possible that multiply sub features with same feature id are
>>> @@ -248,6 +250,8 @@ struct dfl_feature_irq_ctx {
>>>   */
>>>  struct dfl_feature {
>>>  	struct platform_device *dev;
>>> +	struct uio_info uio;
>>> +	struct mutex lock; /* serialize dev and uio */
>>>  	u64 id;
>>>  	int index;
>>>  	int resource_index;
>>> @@ -360,6 +364,11 @@ void dfl_fpga_dev_feature_uinit(struct
>> platform_device *pdev);
>>>  int dfl_fpga_dev_feature_init(struct platform_device *pdev,
>>>  			      struct dfl_feature_driver *feature_drvs);
>>>
>>> +int dfl_fpga_dev_feature_init_uio(struct platform_device *pdev, int
>> dfh_type);
>>> +int dfl_fpga_dev_feature_uinit_uio(struct platform_device *pdev, int
>> dfh_type);
>>> +int dfl_uio_add(struct dfl_feature *feature);
>>> +int dfl_uio_remove(struct dfl_feature *feature);
>>> +
>>>  int dfl_fpga_dev_ops_register(struct platform_device *pdev,
>>>  			      const struct file_operations *fops,
>>>  			      struct module *owner);
>>> diff --git a/uio.c b/uio.c
>>> new file mode 100644
>>> index 000000000000..50210aab4822
>>> --- /dev/null
>>> +++ b/uio.c
>>> @@ -0,0 +1,56 @@
>>> +#include <stdlib.h>
>>> +#include <stdio.h>
>>> +#include <unistd.h>
>>> +#include <sys/mman.h>
>>> +#include <sys/types.h>
>>> +#include <sys/stat.h>
>>> +#include <fcntl.h>
>>> +#include <errno.h>
>>> +#include <stdint.h>
>>> +
>>> +int main()
>>> +{
>>> +	int fd;
>>> +	uint64_t *ptr;
>>> +	unsigned page_size=sysconf(_SC_PAGESIZE);
>>> +	struct stat sb;
>>> +
>>> +	/*
>>> +	 * this is fid 1, thermal mgt
>>> +	 * ex/
>>> +	 * # cat /sys/class/hwmon/hwmon3/temp1_input
>>> +	 * 39000
>>> +	 */
>>> +	fd = open("/dev/uio0", O_RDONLY|O_SYNC);
>>> +	if (fd < 0) {
>>> +		perror("uio open:");
>>> +		return errno;
>>> +	}
>>> +
>>> +	ptr = (uint64_t *) mmap(NULL, page_size, PROT_READ, MAP_SHARED,
>> fd, 0);
>>> +	if (!ptr) {
>>> +		perror("uio mmap:");
>>> +	} else {
>>> +
>>> +		/* from dfl-fme-main.c :
>>> +		 *
>>> +		 * #define FME_THERM_RDSENSOR_FMT1	0x10
>>> +		 * #define FPGA_TEMPERATURE	GENMASK_ULL(6, 0)
>>> +		 *
>>> +		 * case hwmon_temp_input:
>>> +		 * v = readq(feature->ioaddr +
>> FME_THERM_RDSENSOR_FMT1);
>>> +		 * *val = (long)(FIELD_GET(FPGA_TEMPERATURE, v) * 1000);
>>> +		 * break;
>>> +		 */
>>> +		uint64_t v = ptr[2];
>>> +		v &= (1 << 6) -1;
>>> +		v *= 1000;
>>> +		printf("Temperature %d\n", v);
>>> +
>>> +		munmap(ptr, page_size);
>>> +	}
>>> +	if (close(fd))
>>> +		perror("uio close:");
>>> +
>>> +	return errno;
>>> +}
>>> --
>>> 2.18.4

Re: [RFC] fpga: dfl: a prototype uio driver

[Tom Rix]

On 12/7/20 12:02 AM, Greg KH wrote:
> On Sun, Dec 06, 2020 at 01:55:54PM -0800, trix@redhat.com wrote:
>> From: Tom Rix <trix@redhat.com>
>>
>> >From [PATCH 0/2] UIO support for dfl devices
>> https://lore.kernel.org/linux-fpga/1602828151-24784-1-git-send-email-yilun.xu@intel.com/
> Why is this here?

As reference, Yilun's work has precedence for a uio driver and this rfc is trying to address what i believe is a sticking point of the driver override.  This rfc is some code i hacked out to show the idea and move uio support along.  I would like to see uio support for at least the unclaimed feature id's because this would make it easier for them to be developed.

>> Here is an idea to have uio support with no driver override.
>>
>> This makes UIO the primary driver interface because every feature
>> will have one and makes the existing platform driver interface
>> secondary.  There will be a new burden for locking write access when
>> they compete.
>>
>> Example shows finding the fpga's temperture.
>>
>> Signed-off-by: Tom Rix <trix@redhat.com>
>> ---
>>  drivers/fpga/dfl-fme-main.c |  9 +++-
>>  drivers/fpga/dfl-uio.c      | 96 +++++++++++++++++++++++++++++++++++++
>>  drivers/fpga/dfl.c          | 44 ++++++++++++++++-
>>  drivers/fpga/dfl.h          |  9 ++++
>>  uio.c                       | 56 ++++++++++++++++++++++
>>  5 files changed, 212 insertions(+), 2 deletions(-)
>>  create mode 100644 drivers/fpga/dfl-uio.c
>>  create mode 100644 uio.c
>>
>> diff --git a/drivers/fpga/dfl-fme-main.c b/drivers/fpga/dfl-fme-main.c
>> index 037dc4f946f0..3323e90a18c4 100644
>> --- a/drivers/fpga/dfl-fme-main.c
>> +++ b/drivers/fpga/dfl-fme-main.c
>> @@ -709,12 +709,18 @@ static int fme_probe(struct platform_device *pdev)
>>  	if (ret)
>>  		goto dev_destroy;
>>  
>> -	ret = dfl_fpga_dev_ops_register(pdev, &fme_fops, THIS_MODULE);
>> +	ret = dfl_fpga_dev_feature_init_uio(pdev, DFH_TYPE_FIU);
>>  	if (ret)
>>  		goto feature_uinit;
>>  
>> +	ret = dfl_fpga_dev_ops_register(pdev, &fme_fops, THIS_MODULE);
>> +	if (ret)
>> +		goto feature_uinit_uio;
>> +
>>  	return 0;
>>  
>> +feature_uinit_uio:
>> +	dfl_fpga_dev_feature_uinit_uio(pdev, DFH_TYPE_FIU);
>>  feature_uinit:
>>  	dfl_fpga_dev_feature_uinit(pdev);
>>  dev_destroy:
>> @@ -726,6 +732,7 @@ exit:
>>  static int fme_remove(struct platform_device *pdev)
>>  {
>>  	dfl_fpga_dev_ops_unregister(pdev);
>> +	dfl_fpga_dev_feature_uinit_uio(pdev, DFH_TYPE_FIU);
>>  	dfl_fpga_dev_feature_uinit(pdev);
>>  	fme_dev_destroy(pdev);
>>  
>> diff --git a/drivers/fpga/dfl-uio.c b/drivers/fpga/dfl-uio.c
>> new file mode 100644
>> index 000000000000..7610ee0b19dc
>> --- /dev/null
>> +++ b/drivers/fpga/dfl-uio.c
>> @@ -0,0 +1,96 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * prototype dfl uio driver
>> + *
>> + * Copyright Tom Rix 2020
>> + */
>> +#include <linux/module.h>
>> +#include "dfl.h"
>> +
>> +static irqreturn_t dfl_uio_handler(int irq, struct uio_info *info)
>> +{
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int dfl_uio_mmap(struct uio_info *info, struct vm_area_struct *vma)
>> +{
>> +	int ret = -ENODEV;
>> +	return ret;
> Did you run this through checkpatch?
>
> Does the code make sense?
>
>> +}
>> +
>> +static int dfl_uio_open(struct uio_info *info, struct inode *inode)
>> +{
>> +	int ret = -ENODEV;
>> +	struct dfl_feature *feature = container_of(info, struct dfl_feature, uio);
>> +	if (feature->dev)
>> +		mutex_lock(&feature->lock);
>> +
>> +	ret = 0;
>> +	return ret;
> Same here, does this make sense?
>
> And wait, you are having userspace grab a kernel lock?  What could go
> wrong?  :(
>
Yes, this is the bad part of this idea.

Tom


>> +}
>> +
>> +static int dfl_uio_release(struct uio_info *info, struct inode *inode)
>> +{
>> +	int ret = -ENODEV;
>> +	struct dfl_feature *feature = container_of(info, struct dfl_feature, uio);
>> +	if (feature->dev)
>> +		mutex_unlock(&feature->lock);
>> +
>> +	ret = 0;
>> +	return ret;
>> +}
>> +
>> +static int dfl_uio_irqcontrol(struct uio_info *info, s32 irq_on)
>> +{
>> +	int ret = -ENODEV;
>> +	return ret;
>> +}
>> +
>> +int dfl_uio_add(struct dfl_feature *feature)
>> +{
>> +	struct uio_info *uio = &feature->uio;
>> +	struct resource *res =
>> +		&feature->dev->resource[feature->resource_index];
>> +	int ret = 0;
>> +
>> +	uio->name = kasprintf(GFP_KERNEL, "dfl-uio-%llx", feature->id);
>> +	if (!uio->name) {
>> +		ret = -ENOMEM;
>> +		goto exit;
>> +	}
>> +
>> +	uio->version = "0.1";
>> +	uio->mem[0].memtype = UIO_MEM_PHYS;
>> +	uio->mem[0].addr = res->start & PAGE_MASK;
>> +	uio->mem[0].offs = res->start & ~PAGE_MASK;
>> +	uio->mem[0].size = (uio->mem[0].offs + resource_size(res)
>> +			    + PAGE_SIZE - 1) & PAGE_MASK;
>> +	/* How are nr_irqs > 1 handled ??? */
>> +	if (feature->nr_irqs == 1)
>> +		uio->irq = feature->irq_ctx[0].irq;
>> +	uio->handler = dfl_uio_handler;
>> +	//uio->mmap = dfl_uio_mmap;
> ???
>
> I don't understand what this patch is trying to show...
> thanks,
>
> greg k-h
>

Re: [RFC] fpga: dfl: a prototype uio driver

[Xu Yilun]

Hi Tom:

On Mon, Dec 07, 2020 at 05:07:05AM -0800, Tom Rix wrote:
> 
> On 12/7/20 12:02 AM, Greg KH wrote:
> > On Sun, Dec 06, 2020 at 01:55:54PM -0800, trix@redhat.com wrote:
> >> From: Tom Rix <trix@redhat.com>
> >>
> >> >From [PATCH 0/2] UIO support for dfl devices
> >> https://lore.kernel.org/linux-fpga/1602828151-24784-1-git-send-email-yilun.xu@intel.com/
> > Why is this here?
> 
> As reference, Yilun's work has precedence for a uio driver and this rfc is trying to address what i believe is a sticking point of the driver override.  This rfc is some code i hacked out to show the idea and move uio support along.  I would like to see uio support for at least the unclaimed feature id's because this would make it easier for them to be developed.

I see there is concern about sharing DFL devices for both UIO and kernel
drivers. Even if a lock could be created to serialize the accesses of
both interfaces, they could potentially impact each other without notice
on hardware level.

Maybe it is better we split the uio driver for unclaimed features. But
how we could know it is an unclaimed feature, may be for simplicity we
list the feature ids in device id table for dfl uio driver? We should
change the code of dfl uio when we want to use uio for a new dfl device,
is that acceptable?

Thanks,
Yilun

> 
> >> Here is an idea to have uio support with no driver override.
> >>
> >> This makes UIO the primary driver interface because every feature
> >> will have one and makes the existing platform driver interface
> >> secondary.  There will be a new burden for locking write access when
> >> they compete.
> >>
> >> Example shows finding the fpga's temperture.
> >>
> >> Signed-off-by: Tom Rix <trix@redhat.com>
> >> ---
> >>  drivers/fpga/dfl-fme-main.c |  9 +++-
> >>  drivers/fpga/dfl-uio.c      | 96 +++++++++++++++++++++++++++++++++++++
> >>  drivers/fpga/dfl.c          | 44 ++++++++++++++++-
> >>  drivers/fpga/dfl.h          |  9 ++++
> >>  uio.c                       | 56 ++++++++++++++++++++++
> >>  5 files changed, 212 insertions(+), 2 deletions(-)
> >>  create mode 100644 drivers/fpga/dfl-uio.c
> >>  create mode 100644 uio.c
> >>
> >> diff --git a/drivers/fpga/dfl-fme-main.c b/drivers/fpga/dfl-fme-main.c
> >> index 037dc4f946f0..3323e90a18c4 100644
> >> --- a/drivers/fpga/dfl-fme-main.c
> >> +++ b/drivers/fpga/dfl-fme-main.c
> >> @@ -709,12 +709,18 @@ static int fme_probe(struct platform_device *pdev)
> >>  	if (ret)
> >>  		goto dev_destroy;
> >>  
> >> -	ret = dfl_fpga_dev_ops_register(pdev, &fme_fops, THIS_MODULE);
> >> +	ret = dfl_fpga_dev_feature_init_uio(pdev, DFH_TYPE_FIU);
> >>  	if (ret)
> >>  		goto feature_uinit;
> >>  
> >> +	ret = dfl_fpga_dev_ops_register(pdev, &fme_fops, THIS_MODULE);
> >> +	if (ret)
> >> +		goto feature_uinit_uio;
> >> +
> >>  	return 0;
> >>  
> >> +feature_uinit_uio:
> >> +	dfl_fpga_dev_feature_uinit_uio(pdev, DFH_TYPE_FIU);
> >>  feature_uinit:
> >>  	dfl_fpga_dev_feature_uinit(pdev);
> >>  dev_destroy:
> >> @@ -726,6 +732,7 @@ exit:
> >>  static int fme_remove(struct platform_device *pdev)
> >>  {
> >>  	dfl_fpga_dev_ops_unregister(pdev);
> >> +	dfl_fpga_dev_feature_uinit_uio(pdev, DFH_TYPE_FIU);
> >>  	dfl_fpga_dev_feature_uinit(pdev);
> >>  	fme_dev_destroy(pdev);
> >>  
> >> diff --git a/drivers/fpga/dfl-uio.c b/drivers/fpga/dfl-uio.c
> >> new file mode 100644
> >> index 000000000000..7610ee0b19dc
> >> --- /dev/null
> >> +++ b/drivers/fpga/dfl-uio.c
> >> @@ -0,0 +1,96 @@
> >> +/* SPDX-License-Identifier: GPL-2.0 */
> >> +/*
> >> + * prototype dfl uio driver
> >> + *
> >> + * Copyright Tom Rix 2020
> >> + */
> >> +#include <linux/module.h>
> >> +#include "dfl.h"
> >> +
> >> +static irqreturn_t dfl_uio_handler(int irq, struct uio_info *info)
> >> +{
> >> +	return IRQ_HANDLED;
> >> +}
> >> +
> >> +static int dfl_uio_mmap(struct uio_info *info, struct vm_area_struct *vma)
> >> +{
> >> +	int ret = -ENODEV;
> >> +	return ret;
> > Did you run this through checkpatch?
> >
> > Does the code make sense?
> >
> >> +}
> >> +
> >> +static int dfl_uio_open(struct uio_info *info, struct inode *inode)
> >> +{
> >> +	int ret = -ENODEV;
> >> +	struct dfl_feature *feature = container_of(info, struct dfl_feature, uio);
> >> +	if (feature->dev)
> >> +		mutex_lock(&feature->lock);
> >> +
> >> +	ret = 0;
> >> +	return ret;
> > Same here, does this make sense?
> >
> > And wait, you are having userspace grab a kernel lock?  What could go
> > wrong?  :(
> >
> Yes, this is the bad part of this idea.
> 
> Tom
> 
> 
> >> +}
> >> +
> >> +static int dfl_uio_release(struct uio_info *info, struct inode *inode)
> >> +{
> >> +	int ret = -ENODEV;
> >> +	struct dfl_feature *feature = container_of(info, struct dfl_feature, uio);
> >> +	if (feature->dev)
> >> +		mutex_unlock(&feature->lock);
> >> +
> >> +	ret = 0;
> >> +	return ret;
> >> +}
> >> +
> >> +static int dfl_uio_irqcontrol(struct uio_info *info, s32 irq_on)
> >> +{
> >> +	int ret = -ENODEV;
> >> +	return ret;
> >> +}
> >> +
> >> +int dfl_uio_add(struct dfl_feature *feature)
> >> +{
> >> +	struct uio_info *uio = &feature->uio;
> >> +	struct resource *res =
> >> +		&feature->dev->resource[feature->resource_index];
> >> +	int ret = 0;
> >> +
> >> +	uio->name = kasprintf(GFP_KERNEL, "dfl-uio-%llx", feature->id);
> >> +	if (!uio->name) {
> >> +		ret = -ENOMEM;
> >> +		goto exit;
> >> +	}
> >> +
> >> +	uio->version = "0.1";
> >> +	uio->mem[0].memtype = UIO_MEM_PHYS;
> >> +	uio->mem[0].addr = res->start & PAGE_MASK;
> >> +	uio->mem[0].offs = res->start & ~PAGE_MASK;
> >> +	uio->mem[0].size = (uio->mem[0].offs + resource_size(res)
> >> +			    + PAGE_SIZE - 1) & PAGE_MASK;
> >> +	/* How are nr_irqs > 1 handled ??? */
> >> +	if (feature->nr_irqs == 1)
> >> +		uio->irq = feature->irq_ctx[0].irq;
> >> +	uio->handler = dfl_uio_handler;
> >> +	//uio->mmap = dfl_uio_mmap;
> > ???
> >
> > I don't understand what this patch is trying to show...
> > thanks,
> >
> > greg k-h
> >

Re: [RFC] fpga: dfl: a prototype uio driver

[Tom Rix]

On 12/9/20 12:56 AM, Xu Yilun wrote:
> Hi Tom:
>
> On Mon, Dec 07, 2020 at 05:07:05AM -0800, Tom Rix wrote:
>> On 12/7/20 12:02 AM, Greg KH wrote:
>>> On Sun, Dec 06, 2020 at 01:55:54PM -0800, trix@redhat.com wrote:
>>>> From: Tom Rix <trix@redhat.com>
>>>>
>>>> >From [PATCH 0/2] UIO support for dfl devices
>>>> https://lore.kernel.org/linux-fpga/1602828151-24784-1-git-send-email-yilun.xu@intel.com/
>>> Why is this here?
>> As reference, Yilun's work has precedence for a uio driver and this rfc is trying to address what i believe is a sticking point of the driver override.  This rfc is some code i hacked out to show the idea and move uio support along.  I would like to see uio support for at least the unclaimed feature id's because this would make it easier for them to be developed.
> I see there is concern about sharing DFL devices for both UIO and kernel
> drivers. Even if a lock could be created to serialize the accesses of
> both interfaces, they could potentially impact each other without notice
> on hardware level.
>
> Maybe it is better we split the uio driver for unclaimed features. But
> how we could know it is an unclaimed feature, may be for simplicity we
> list the feature ids in device id table for dfl uio driver? We should
> change the code of dfl uio when we want to use uio for a new dfl device,
> is that acceptable?

No entry in the device id table would mean there would never be a conflict, so this is good.

This set could be expanded if the platform device driver was checked, and then uio could also used whose platform drivers were disabled in the configury.  There would be this problem: on the module case, disabling uio per feature so the platform driver kmod could be used.

I think we could do your the device id table suggestion now since it is simple and will help almost all developers.

Tom

>
> Thanks,
> Yilun
>
>>>> Here is an idea to have uio support with no driver override.
>>>>
>>>> This makes UIO the primary driver interface because every feature
>>>> will have one and makes the existing platform driver interface
>>>> secondary.  There will be a new burden for locking write access when
>>>> they compete.
>>>>
>>>> Example shows finding the fpga's temperture.
>>>>
>>>> Signed-off-by: Tom Rix <trix@redhat.com>
>>>> ---
>>>>  drivers/fpga/dfl-fme-main.c |  9 +++-
>>>>  drivers/fpga/dfl-uio.c      | 96 +++++++++++++++++++++++++++++++++++++
>>>>  drivers/fpga/dfl.c          | 44 ++++++++++++++++-
>>>>  drivers/fpga/dfl.h          |  9 ++++
>>>>  uio.c                       | 56 ++++++++++++++++++++++
>>>>  5 files changed, 212 insertions(+), 2 deletions(-)
>>>>  create mode 100644 drivers/fpga/dfl-uio.c
>>>>  create mode 100644 uio.c
>>>>
>>>> diff --git a/drivers/fpga/dfl-fme-main.c b/drivers/fpga/dfl-fme-main.c
>>>> index 037dc4f946f0..3323e90a18c4 100644
>>>> --- a/drivers/fpga/dfl-fme-main.c
>>>> +++ b/drivers/fpga/dfl-fme-main.c
>>>> @@ -709,12 +709,18 @@ static int fme_probe(struct platform_device *pdev)
>>>>  	if (ret)
>>>>  		goto dev_destroy;
>>>>  
>>>> -	ret = dfl_fpga_dev_ops_register(pdev, &fme_fops, THIS_MODULE);
>>>> +	ret = dfl_fpga_dev_feature_init_uio(pdev, DFH_TYPE_FIU);
>>>>  	if (ret)
>>>>  		goto feature_uinit;
>>>>  
>>>> +	ret = dfl_fpga_dev_ops_register(pdev, &fme_fops, THIS_MODULE);
>>>> +	if (ret)
>>>> +		goto feature_uinit_uio;
>>>> +
>>>>  	return 0;
>>>>  
>>>> +feature_uinit_uio:
>>>> +	dfl_fpga_dev_feature_uinit_uio(pdev, DFH_TYPE_FIU);
>>>>  feature_uinit:
>>>>  	dfl_fpga_dev_feature_uinit(pdev);
>>>>  dev_destroy:
>>>> @@ -726,6 +732,7 @@ exit:
>>>>  static int fme_remove(struct platform_device *pdev)
>>>>  {
>>>>  	dfl_fpga_dev_ops_unregister(pdev);
>>>> +	dfl_fpga_dev_feature_uinit_uio(pdev, DFH_TYPE_FIU);
>>>>  	dfl_fpga_dev_feature_uinit(pdev);
>>>>  	fme_dev_destroy(pdev);
>>>>  
>>>> diff --git a/drivers/fpga/dfl-uio.c b/drivers/fpga/dfl-uio.c
>>>> new file mode 100644
>>>> index 000000000000..7610ee0b19dc
>>>> --- /dev/null
>>>> +++ b/drivers/fpga/dfl-uio.c
>>>> @@ -0,0 +1,96 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>>> +/*
>>>> + * prototype dfl uio driver
>>>> + *
>>>> + * Copyright Tom Rix 2020
>>>> + */
>>>> +#include <linux/module.h>
>>>> +#include "dfl.h"
>>>> +
>>>> +static irqreturn_t dfl_uio_handler(int irq, struct uio_info *info)
>>>> +{
>>>> +	return IRQ_HANDLED;
>>>> +}
>>>> +
>>>> +static int dfl_uio_mmap(struct uio_info *info, struct vm_area_struct *vma)
>>>> +{
>>>> +	int ret = -ENODEV;
>>>> +	return ret;
>>> Did you run this through checkpatch?
>>>
>>> Does the code make sense?
>>>
>>>> +}
>>>> +
>>>> +static int dfl_uio_open(struct uio_info *info, struct inode *inode)
>>>> +{
>>>> +	int ret = -ENODEV;
>>>> +	struct dfl_feature *feature = container_of(info, struct dfl_feature, uio);
>>>> +	if (feature->dev)
>>>> +		mutex_lock(&feature->lock);
>>>> +
>>>> +	ret = 0;
>>>> +	return ret;
>>> Same here, does this make sense?
>>>
>>> And wait, you are having userspace grab a kernel lock?  What could go
>>> wrong?  :(
>>>
>> Yes, this is the bad part of this idea.
>>
>> Tom
>>
>>
>>>> +}
>>>> +
>>>> +static int dfl_uio_release(struct uio_info *info, struct inode *inode)
>>>> +{
>>>> +	int ret = -ENODEV;
>>>> +	struct dfl_feature *feature = container_of(info, struct dfl_feature, uio);
>>>> +	if (feature->dev)
>>>> +		mutex_unlock(&feature->lock);
>>>> +
>>>> +	ret = 0;
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static int dfl_uio_irqcontrol(struct uio_info *info, s32 irq_on)
>>>> +{
>>>> +	int ret = -ENODEV;
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +int dfl_uio_add(struct dfl_feature *feature)
>>>> +{
>>>> +	struct uio_info *uio = &feature->uio;
>>>> +	struct resource *res =
>>>> +		&feature->dev->resource[feature->resource_index];
>>>> +	int ret = 0;
>>>> +
>>>> +	uio->name = kasprintf(GFP_KERNEL, "dfl-uio-%llx", feature->id);
>>>> +	if (!uio->name) {
>>>> +		ret = -ENOMEM;
>>>> +		goto exit;
>>>> +	}
>>>> +
>>>> +	uio->version = "0.1";
>>>> +	uio->mem[0].memtype = UIO_MEM_PHYS;
>>>> +	uio->mem[0].addr = res->start & PAGE_MASK;
>>>> +	uio->mem[0].offs = res->start & ~PAGE_MASK;
>>>> +	uio->mem[0].size = (uio->mem[0].offs + resource_size(res)
>>>> +			    + PAGE_SIZE - 1) & PAGE_MASK;
>>>> +	/* How are nr_irqs > 1 handled ??? */
>>>> +	if (feature->nr_irqs == 1)
>>>> +		uio->irq = feature->irq_ctx[0].irq;
>>>> +	uio->handler = dfl_uio_handler;
>>>> +	//uio->mmap = dfl_uio_mmap;
>>> ???
>>>
>>> I don't understand what this patch is trying to show...
>>> thanks,
>>>
>>> greg k-h
>>>

Re: [RFC] fpga: dfl: a prototype uio driver

[Tom Rix]

On 9/21/20 8:18 PM, Xu Yilun wrote:
> On Mon, Sep 21, 2020 at 12:32:16PM -0700, Tom Rix wrote:
>> On 9/21/20 1:55 AM, Xu Yilun wrote:
>>> On Sat, Sep 19, 2020 at 09:51:13AM -0700, trix@redhat.com wrote:
>>>> From: Tom Rix <trix@redhat.com>
>>>>
>>>> I following up on Moritz asking for early RFC's by showing how this
>>>> could be done with the concrete example of around
>>>>
>>>> [PATCH 0/3] add VFIO mdev support for DFL devices
>>>>
>>>> I hacked this together, it barely works. Do not expect that this
>>>> patch to apply anywhere.  It has enough to show where I want
>>>> to go and people can comment without me having invested a lot of
>>>> effort.  I am not expecting to carry this forward, it only
>>>> addresses the issues I had with the origin patch.
>>>>
>>>> This change adds a uio driver for any unclaimed dfl feature
>>>>
>>>> During the normal matching of feature id's to drivers, some
>>>> of the features are not claimed because there neither a
>>>> builtin driver nor a module driver that matches the feature
>>>> id.  For these unclaimed features, provide a uio driver to a
>>>> possible user space driver.
>>> I think we don't have to setup UIO interfaces for all unclaimed 
>>> features. It could be the decision of the user whether the UIO
>>> device is needed for a feature. My opinion is that, the
>>> driver_override sysfs is still generic enough for user's to switch
>>> between kernel device drivers and dfl uio driver.
>> Instead of a string, could there just be a 'use_uio' flag ?
>>
>> How does the 'driver_override' work when there is no starting driver ?
> Now we have a dfl bus_type, we could add the 'driver_override' to each
> dfl device on dfl bus. It is the same as 'feature_id' & 'type'.
>
> Actually the 'driver_override' also exists in various bus type (platform,
> pci ...).
>
>>> There may be cases the dfl device driver modules are not inserted
>>> during dfl-fme/port initialization, but they are to be inserted
>>> manually. If the features are all registered to uio, then there will
>>> be problems when dfl device drivers module are inserted.
>> How does this manual loading work ? The driver list for the features
>>
>> seems to only be used during the card probe time.
>>
>> To change the order the dfl-pci needs to be unloaded and that will
>>
>> destroy all the uio devices as well as usual feature drivers attached to
>>
>> the pci driver.
> After we have introduced the dfl bus patches. The initialization flow
> is:
>
> 1. dfl-fme/port initializes its core private features (listed in
>    fme/port_feature_drvs array), these private features are part of
>    the dfl-fme/port module. They cannot be exposed as uio devices cause
>    they are managed by the dfl-fme/afu kernel driver.
>
>
> 2. The rest of the private features are added as dfl devices. They can
>    be matched by independent dfl driver modules. dfl-n3000-nios is the
>    first use case. The dfl-n3000-nios.ko may not be loaded when dfl-fme
>    probe, but later user loads the module manually by
>    "insmod drivers/fpga/dfl-n3000-nios.ko".
>
>    If we create uio interfaces for all unclaimed features on
>    dfl-fme/port probe, then I can see problem when user insmod
>    dfl-n3000-nios.ko
>
>
> For these dfl devices, we could give users an option to manage them
> by userspace I/O access. The flow I suggest is like:
> a) load the uio-dfl.ko, it will not match any dfl device now.
>    # modprobe uio-dfl   
>
> b) unbind the kernel driver for the specific dfl device
>    # echo dfl_dev.0 > /sys/bus/dfl/drivers/n3000-nios/unbind
>
> c) assign the uio driver for the dfl device. Please note this will
>    not trigger any driver matching.
>    # echo uio-dfl > /sys/bus/dfl/devices/dfl_dev.0/driver_override
>
> d) actually trigger the driver matching, then the uio-dfl module takes
>    function.
>    # echo dfl_dev.0 > /sys/bus/dfl/drivers_probe
>
>>
>>>
>>>> I have doubts that a uio for an afu feature is approptiate as these
>>>> can be any device.
>>> I think generally afu could also be as uio device if we don't concern
>>> about dma isolation.
>> I am thinking about afu with its own pci id.
>>
>> So there could be a conflict with some other driver that responds to the pci id.
> I think 'driver_override' mechanism solves the problem, it ensures no
> other driver could be matched to the device except your assigned one.
>
>>> But now seems not possible to match afu to uio driver, cause now in DFL
>>> framework AFU is managed by dfl-afu.ko
>>>
>>>> Another possible problem is if the number of interrupts for the
>>>> feature is greater than 1.  Uio seems to only support 1. My guess
>>>> is this would need some hackery in the open() to add to the
>>>> interrupt handler.
>>> I think it may not possible for UIO to support multiple interrupts if
>>> user cannot access the interrupt enable/pending registers. The uio
>>> device have just one /dev/uioX node for interrupt. So I tend to
>>> accept the limitation. And for now we don't have board specific
>>> features that needs multiple interrupts. For PAC N3000, no interrupt is
>>> needed.
>> Maybe uio needs to change to support multiple interrupts ?
> I could expect a big change for uio, especially the change of APIs to
> userspace, which would make much impact.
>
> For now I didn't see the demand of multiple interrupts for dfl. And for
> PAC N3000, no interrupt is needed. So this could be considered later.
>
> Actually, to meet our current need, the only changes for dfl framework could
> be the common 'driver_override'. We could try to improve the other part
> if there is a clear request.
>
>>>> It is for this type of reason I think a dfl-uio driver is needed
>>>> rather than reusing an existing generic uio driver.
>>> So if we don't try to change interrupt, the implementation of dfl-uio is
>>> just a subset of generic uio platform driver, so why not we just use it?
>> Its a toss up.
> I agree. I also hesitate at this for sometime.
>
>> Maybe there some dfl only constraints like write/read is a multiple 4 bytes
> When you mmap your IO to users, you cannot limit the way they access the
> registers. It is the user's responsibility to keep it right.
>
>> Or just why have another layer in the setup.
> It's mainly about reusing the code. DFL devices are mainly the same as
> platform devices (except the way they are enumerated). Actually people
> may integrates IP blocks in FPGA which are already widely used on other
> systems and implemented as platform devices. So I think we may get more
> benifit leveraging uio platform.
>
> Thanks,
> Yilun

Thanks for the detailed explanation!

I look forward to your next rev.

Tom


>
>> Tom
>>
>>> Thanks,
>>> Yilun
>>>
>>>> Signed-off-by: Tom Rix <trix@redhat.com>
>>>> ---
>>>>  drivers/fpga/dfl-fme-main.c |   9 ++-
>>>>  drivers/fpga/dfl-uio.c      | 107 ++++++++++++++++++++++++++++++++++++
>>>>  drivers/fpga/dfl.c          |  47 +++++++++++++++-
>>>>  drivers/fpga/dfl.h          |   8 +++
>>>>  4 files changed, 169 insertions(+), 2 deletions(-)
>>>>  create mode 100644 drivers/fpga/dfl-uio.c
>>>>
>>>> diff --git a/drivers/fpga/dfl-fme-main.c b/drivers/fpga/dfl-fme-main.c
>>>> index 037dc4f..3323e90 100644
>>>> --- a/drivers/fpga/dfl-fme-main.c
>>>> +++ b/drivers/fpga/dfl-fme-main.c
>>>> @@ -709,12 +709,18 @@ static int fme_probe(struct platform_device *pdev)
>>>>  	if (ret)
>>>>  		goto dev_destroy;
>>>>  
>>>> -	ret = dfl_fpga_dev_ops_register(pdev, &fme_fops, THIS_MODULE);
>>>> +	ret = dfl_fpga_dev_feature_init_uio(pdev, DFH_TYPE_FIU);
>>>>  	if (ret)
>>>>  		goto feature_uinit;
>>>>  
>>>> +	ret = dfl_fpga_dev_ops_register(pdev, &fme_fops, THIS_MODULE);
>>>> +	if (ret)
>>>> +		goto feature_uinit_uio;
>>>> +
>>>>  	return 0;
>>>>  
>>>> +feature_uinit_uio:
>>>> +	dfl_fpga_dev_feature_uinit_uio(pdev, DFH_TYPE_FIU);
>>>>  feature_uinit:
>>>>  	dfl_fpga_dev_feature_uinit(pdev);
>>>>  dev_destroy:
>>>> @@ -726,6 +732,7 @@ exit:
>>>>  static int fme_remove(struct platform_device *pdev)
>>>>  {
>>>>  	dfl_fpga_dev_ops_unregister(pdev);
>>>> +	dfl_fpga_dev_feature_uinit_uio(pdev, DFH_TYPE_FIU);
>>>>  	dfl_fpga_dev_feature_uinit(pdev);
>>>>  	fme_dev_destroy(pdev);
>>>>  
>>>> diff --git a/drivers/fpga/dfl-uio.c b/drivers/fpga/dfl-uio.c
>>>> new file mode 100644
>>>> index 0000000..185fbab
>>>> --- /dev/null
>>>> +++ b/drivers/fpga/dfl-uio.c
>>>> @@ -0,0 +1,107 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * prototype dfl uio driver
>>>> + *
>>>> + * Copyright Tom Rix 2020
>>>> + */
>>>> +#include <linux/module.h>
>>>> +#include "dfl.h"
>>>> +
>>>> +static irqreturn_t dfl_uio_handler(int irq, struct uio_info *dev_info)
>>>> +{
>>>> +	return IRQ_HANDLED;
>>>> +}
>>>> +
>>>> +static int dfl_uio_mmap(struct uio_info *info, struct vm_area_struct *vma)
>>>> +{
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int dfl_uio_open(struct uio_info *info, struct inode *inode)
>>>> +{
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int dfl_uio_release(struct uio_info *info, struct inode *inode)
>>>> +{
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int dfl_uio_irqcontrol(struct uio_info *info, s32 irq_on)
>>>> +{
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +int dfl_uio_add(struct dfl_feature *feature)
>>>> +{
>>>> +	struct uio_info *uio;
>>>> +	struct resource *res =
>>>> +		&feature->dev->resource[feature->resource_index];
>>>> +	int ret = 0;
>>>> +
>>>> +	uio = kzalloc(sizeof(struct uio_info), GFP_KERNEL);
>>>> +	if (!uio) {
>>>> +		ret = -ENOMEM;
>>>> +		goto exit;
>>>> +	}
>>>> +
>>>> +	uio->name = kasprintf(GFP_KERNEL, "dfl-uio-%llx", feature->id);
>>>> +	if (!uio->name) {
>>>> +		ret = -ENOMEM;
>>>> +		goto err_name;
>>>> +	}
>>>> +
>>>> +	uio->version = "0.1";
>>>> +	uio->mem[0].memtype = UIO_MEM_PHYS;
>>>> +	uio->mem[0].addr = res->start & PAGE_MASK;
>>>> +	uio->mem[0].offs = res->start & ~PAGE_MASK;
>>>> +	uio->mem[0].size = (uio->mem[0].offs + resource_size(res)
>>>> +			    + PAGE_SIZE - 1) & PAGE_MASK;
>>>> +	/* How are nr_irqs > 1 handled ??? */
>>>> +	if (feature->nr_irqs == 1)
>>>> +		uio->irq = feature->irq_ctx[0].irq;
>>>> +	uio->handler = dfl_uio_handler;
>>>> +	uio->mmap = dfl_uio_mmap;
>>>> +	uio->open = dfl_uio_open;
>>>> +	uio->release = dfl_uio_release;
>>>> +	uio->irqcontrol = dfl_uio_irqcontrol;
>>>> +
>>>> +	ret = uio_register_device(&feature->dev->dev, uio);
>>>> +	if (ret)
>>>> +		goto err_register;
>>>> +
>>>> +	feature->uio = uio;
>>>> +exit:
>>>> +	return ret;
>>>> +err_register:
>>>> +	kfree(uio->name);
>>>> +err_name:
>>>> +	kfree(uio);
>>>> +	goto exit;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(dfl_uio_add);
>>>> +
>>>> +int dfl_uio_remove(struct dfl_feature *feature)
>>>> +{
>>>> +	uio_unregister_device(feature->uio);
>>>> +	kfree(feature->uio->name);
>>>> +	kfree(feature->uio);
>>>> +	return 0;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(dfl_uio_remove);
>>>> +
>>>> +static int __init dfl_uio_init(void)
>>>> +{
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static void __exit dfl_uio_exit(void)
>>>> +{
>>>> +}
>>>> +
>>>> +module_init(dfl_uio_init);
>>>> +module_exit(dfl_uio_exit);
>>>> +
>>>> +MODULE_DESCRIPTION("DFL UIO prototype driver");
>>>> +MODULE_AUTHOR("Tom");
>>>> +MODULE_LICENSE("GPL v2");
>>>> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
>>>> index 1305be4..26de8e1 100644
>>>> --- a/drivers/fpga/dfl.c
>>>> +++ b/drivers/fpga/dfl.c
>>>> @@ -663,10 +664,57 @@ exit:
>>>>  }
>>>>  EXPORT_SYMBOL_GPL(dfl_fpga_dev_feature_init);
>>>>  
>>>> +int dfl_fpga_dev_feature_init_uio(struct platform_device *pdev, int dfh_type)
>>>> +{
>>>> +	struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
>>>> +	struct dfl_feature *feature;
>>>> +	int ret;
>>>> +
>>>> +	dfl_fpga_dev_for_each_feature(pdata, feature) {
>>>> +		if (dfh_type == DFH_TYPE_FIU) {
>>>> +			if (feature->id == FEATURE_ID_FIU_HEADER ||
>>>> +			    feature->id == FEATURE_ID_AFU)
>>>> +			continue;
>>>> +
>>>> +			/* give the unclamined feature to uio */
>>>> +			if (!feature->ioaddr) {
>>>> +				ret = dfl_uio_add(feature);
>>>> +				if (ret)
>>>> +					goto exit;
>>>> +			}
>>>> +		}
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +exit:
>>>> +	dfl_fpga_dev_feature_uinit_uio(pdev, dfh_type);
>>>> +	return ret;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(dfl_fpga_dev_feature_init_uio);
>>>> +
>>>> +int dfl_fpga_dev_feature_uinit_uio(struct platform_device *pdev, int dfh_type)
>>>> +{
>>>> +	struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
>>>> +	struct dfl_feature *feature;
>>>> +	int ret = 0;
>>>> +
>>>> +	dfl_fpga_dev_for_each_feature(pdata, feature) {
>>>> +		if (dfh_type == DFH_TYPE_FIU) {
>>>> +			if (feature->id == FEATURE_ID_FIU_HEADER ||
>>>> +			    feature->id == FEATURE_ID_AFU)
>>>> +				continue;
>>>> +
>>>> +			if (feature->uio)
>>>> +				ret |= dfl_uio_remove(feature);
>>>> +		}
>>>> +	}
>>>> +	return ret;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(dfl_fpga_dev_feature_uinit_uio);
>>>> +
>>>>  static void dfl_chardev_uinit(void)
>>>>  {
>>>>  	int i;
>>>> -
>>>>  	for (i = 0; i < DFL_FPGA_DEVT_MAX; i++)
>>>>  		if (MAJOR(dfl_chrdevs[i].devt)) {
>>>>  			unregister_chrdev_region(dfl_chrdevs[i].devt,
>>>> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
>>>> index a85d1cd..6e37aef 100644
>>>> --- a/drivers/fpga/dfl.h
>>>> +++ b/drivers/fpga/dfl.h
>>>> @@ -26,6 +26,7 @@
>>>>  #include <linux/platform_device.h>
>>>>  #include <linux/slab.h>
>>>>  #include <linux/uuid.h>
>>>> +#include <linux/uio_driver.h>
>>>>  #include <linux/fpga/fpga-region.h>
>>>>  
>>>>  /* maximum supported number of ports */
>>>> @@ -232,6 +233,7 @@ struct dfl_feature_irq_ctx {
>>>>   * struct dfl_feature - sub feature of the feature devices
>>>>   *
>>>>   * @dev: ptr to pdev of the feature device which has the sub feature.
>>>> + * @uio: for fallback uio driver.
>>>>   * @id: sub feature id.
>>>>   * @index: unique identifier for an sub feature within the feature device.
>>>>   *	   It is possible that multiply sub features with same feature id are
>>>> @@ -248,6 +250,7 @@ struct dfl_feature_irq_ctx {
>>>>   */
>>>>  struct dfl_feature {
>>>>  	struct platform_device *dev;
>>>> +	struct uio_info *uio;
>>>>  	u64 id;
>>>>  	int index;
>>>>  	int resource_index;
>>>> @@ -360,6 +363,11 @@ void dfl_fpga_dev_feature_uinit(struct platform_device *pdev);
>>>>  int dfl_fpga_dev_feature_init(struct platform_device *pdev,
>>>>  			      struct dfl_feature_driver *feature_drvs);
>>>>  
>>>> +int dfl_fpga_dev_feature_init_uio(struct platform_device *pdev, int dfh_type);
>>>> +int dfl_fpga_dev_feature_uinit_uio(struct platform_device *pdev, int dfh_type);
>>>> +int dfl_uio_add(struct dfl_feature *feature);
>>>> +int dfl_uio_remove(struct dfl_feature *feature);
>>>> +
>>>>  int dfl_fpga_dev_ops_register(struct platform_device *pdev,
>>>>  			      const struct file_operations *fops,
>>>>  			      struct module *owner);
>>>> -- 
>>>> 2.18.1

Re: [RFC] fpga: dfl: a prototype uio driver

[Xu Yilun]

On Mon, Sep 21, 2020 at 12:32:16PM -0700, Tom Rix wrote:
> 
> On 9/21/20 1:55 AM, Xu Yilun wrote:
> > On Sat, Sep 19, 2020 at 09:51:13AM -0700, trix@redhat.com wrote:
> >> From: Tom Rix <trix@redhat.com>
> >>
> >> I following up on Moritz asking for early RFC's by showing how this
> >> could be done with the concrete example of around
> >>
> >> [PATCH 0/3] add VFIO mdev support for DFL devices
> >>
> >> I hacked this together, it barely works. Do not expect that this
> >> patch to apply anywhere.  It has enough to show where I want
> >> to go and people can comment without me having invested a lot of
> >> effort.  I am not expecting to carry this forward, it only
> >> addresses the issues I had with the origin patch.
> >>
> >> This change adds a uio driver for any unclaimed dfl feature
> >>
> >> During the normal matching of feature id's to drivers, some
> >> of the features are not claimed because there neither a
> >> builtin driver nor a module driver that matches the feature
> >> id.  For these unclaimed features, provide a uio driver to a
> >> possible user space driver.
> > I think we don't have to setup UIO interfaces for all unclaimed 
> > features. It could be the decision of the user whether the UIO
> > device is needed for a feature. My opinion is that, the
> > driver_override sysfs is still generic enough for user's to switch
> > between kernel device drivers and dfl uio driver.
> 
> Instead of a string, could there just be a 'use_uio' flag ?
> 
> How does the 'driver_override' work when there is no starting driver ?

Now we have a dfl bus_type, we could add the 'driver_override' to each
dfl device on dfl bus. It is the same as 'feature_id' & 'type'.

Actually the 'driver_override' also exists in various bus type (platform,
pci ...).

> 
> >
> > There may be cases the dfl device driver modules are not inserted
> > during dfl-fme/port initialization, but they are to be inserted
> > manually. If the features are all registered to uio, then there will
> > be problems when dfl device drivers module are inserted.
> 
> How does this manual loading work ? The driver list for the features
> 
> seems to only be used during the card probe time.
> 
> To change the order the dfl-pci needs to be unloaded and that will
> 
> destroy all the uio devices as well as usual feature drivers attached to
> 
> the pci driver.

After we have introduced the dfl bus patches. The initialization flow
is:

1. dfl-fme/port initializes its core private features (listed in
   fme/port_feature_drvs array), these private features are part of
   the dfl-fme/port module. They cannot be exposed as uio devices cause
   they are managed by the dfl-fme/afu kernel driver.


2. The rest of the private features are added as dfl devices. They can
   be matched by independent dfl driver modules. dfl-n3000-nios is the
   first use case. The dfl-n3000-nios.ko may not be loaded when dfl-fme
   probe, but later user loads the module manually by
   "insmod drivers/fpga/dfl-n3000-nios.ko".

   If we create uio interfaces for all unclaimed features on
   dfl-fme/port probe, then I can see problem when user insmod
   dfl-n3000-nios.ko


For these dfl devices, we could give users an option to manage them
by userspace I/O access. The flow I suggest is like:
a) load the uio-dfl.ko, it will not match any dfl device now.
   # modprobe uio-dfl   

b) unbind the kernel driver for the specific dfl device
   # echo dfl_dev.0 > /sys/bus/dfl/drivers/n3000-nios/unbind

c) assign the uio driver for the dfl device. Please note this will
   not trigger any driver matching.
   # echo uio-dfl > /sys/bus/dfl/devices/dfl_dev.0/driver_override

d) actually trigger the driver matching, then the uio-dfl module takes
   function.
   # echo dfl_dev.0 > /sys/bus/dfl/drivers_probe

> 
> 
> >
> >
> >> I have doubts that a uio for an afu feature is approptiate as these
> >> can be any device.
> > I think generally afu could also be as uio device if we don't concern
> > about dma isolation.
> 
> I am thinking about afu with its own pci id.
> 
> So there could be a conflict with some other driver that responds to the pci id.

I think 'driver_override' mechanism solves the problem, it ensures no
other driver could be matched to the device except your assigned one.

> 
> >
> > But now seems not possible to match afu to uio driver, cause now in DFL
> > framework AFU is managed by dfl-afu.ko
> >
> >> Another possible problem is if the number of interrupts for the
> >> feature is greater than 1.  Uio seems to only support 1. My guess
> >> is this would need some hackery in the open() to add to the
> >> interrupt handler.
> > I think it may not possible for UIO to support multiple interrupts if
> > user cannot access the interrupt enable/pending registers. The uio
> > device have just one /dev/uioX node for interrupt. So I tend to
> > accept the limitation. And for now we don't have board specific
> > features that needs multiple interrupts. For PAC N3000, no interrupt is
> > needed.
> Maybe uio needs to change to support multiple interrupts ?

I could expect a big change for uio, especially the change of APIs to
userspace, which would make much impact.

For now I didn't see the demand of multiple interrupts for dfl. And for
PAC N3000, no interrupt is needed. So this could be considered later.

Actually, to meet our current need, the only changes for dfl framework could
be the common 'driver_override'. We could try to improve the other part
if there is a clear request.

> >
> >> It is for this type of reason I think a dfl-uio driver is needed
> >> rather than reusing an existing generic uio driver.
> > So if we don't try to change interrupt, the implementation of dfl-uio is
> > just a subset of generic uio platform driver, so why not we just use it?
> 
> Its a toss up.

I agree. I also hesitate at this for sometime.

> 
> Maybe there some dfl only constraints like write/read is a multiple 4 bytes

When you mmap your IO to users, you cannot limit the way they access the
registers. It is the user's responsibility to keep it right.

> 
> Or just why have another layer in the setup.

It's mainly about reusing the code. DFL devices are mainly the same as
platform devices (except the way they are enumerated). Actually people
may integrates IP blocks in FPGA which are already widely used on other
systems and implemented as platform devices. So I think we may get more
benifit leveraging uio platform.

Thanks,
Yilun

> 
> Tom
> 
> >
> > Thanks,
> > Yilun
> >
> >> Signed-off-by: Tom Rix <trix@redhat.com>
> >> ---
> >>  drivers/fpga/dfl-fme-main.c |   9 ++-
> >>  drivers/fpga/dfl-uio.c      | 107 ++++++++++++++++++++++++++++++++++++
> >>  drivers/fpga/dfl.c          |  47 +++++++++++++++-
> >>  drivers/fpga/dfl.h          |   8 +++
> >>  4 files changed, 169 insertions(+), 2 deletions(-)
> >>  create mode 100644 drivers/fpga/dfl-uio.c
> >>
> >> diff --git a/drivers/fpga/dfl-fme-main.c b/drivers/fpga/dfl-fme-main.c
> >> index 037dc4f..3323e90 100644
> >> --- a/drivers/fpga/dfl-fme-main.c
> >> +++ b/drivers/fpga/dfl-fme-main.c
> >> @@ -709,12 +709,18 @@ static int fme_probe(struct platform_device *pdev)
> >>  	if (ret)
> >>  		goto dev_destroy;
> >>  
> >> -	ret = dfl_fpga_dev_ops_register(pdev, &fme_fops, THIS_MODULE);
> >> +	ret = dfl_fpga_dev_feature_init_uio(pdev, DFH_TYPE_FIU);
> >>  	if (ret)
> >>  		goto feature_uinit;
> >>  
> >> +	ret = dfl_fpga_dev_ops_register(pdev, &fme_fops, THIS_MODULE);
> >> +	if (ret)
> >> +		goto feature_uinit_uio;
> >> +
> >>  	return 0;
> >>  
> >> +feature_uinit_uio:
> >> +	dfl_fpga_dev_feature_uinit_uio(pdev, DFH_TYPE_FIU);
> >>  feature_uinit:
> >>  	dfl_fpga_dev_feature_uinit(pdev);
> >>  dev_destroy:
> >> @@ -726,6 +732,7 @@ exit:
> >>  static int fme_remove(struct platform_device *pdev)
> >>  {
> >>  	dfl_fpga_dev_ops_unregister(pdev);
> >> +	dfl_fpga_dev_feature_uinit_uio(pdev, DFH_TYPE_FIU);
> >>  	dfl_fpga_dev_feature_uinit(pdev);
> >>  	fme_dev_destroy(pdev);
> >>  
> >> diff --git a/drivers/fpga/dfl-uio.c b/drivers/fpga/dfl-uio.c
> >> new file mode 100644
> >> index 0000000..185fbab
> >> --- /dev/null
> >> +++ b/drivers/fpga/dfl-uio.c
> >> @@ -0,0 +1,107 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * prototype dfl uio driver
> >> + *
> >> + * Copyright Tom Rix 2020
> >> + */
> >> +#include <linux/module.h>
> >> +#include "dfl.h"
> >> +
> >> +static irqreturn_t dfl_uio_handler(int irq, struct uio_info *dev_info)
> >> +{
> >> +	return IRQ_HANDLED;
> >> +}
> >> +
> >> +static int dfl_uio_mmap(struct uio_info *info, struct vm_area_struct *vma)
> >> +{
> >> +	return 0;
> >> +}
> >> +
> >> +static int dfl_uio_open(struct uio_info *info, struct inode *inode)
> >> +{
> >> +	return 0;
> >> +}
> >> +
> >> +static int dfl_uio_release(struct uio_info *info, struct inode *inode)
> >> +{
> >> +	return 0;
> >> +}
> >> +
> >> +static int dfl_uio_irqcontrol(struct uio_info *info, s32 irq_on)
> >> +{
> >> +	return 0;
> >> +}
> >> +
> >> +int dfl_uio_add(struct dfl_feature *feature)
> >> +{
> >> +	struct uio_info *uio;
> >> +	struct resource *res =
> >> +		&feature->dev->resource[feature->resource_index];
> >> +	int ret = 0;
> >> +
> >> +	uio = kzalloc(sizeof(struct uio_info), GFP_KERNEL);
> >> +	if (!uio) {
> >> +		ret = -ENOMEM;
> >> +		goto exit;
> >> +	}
> >> +
> >> +	uio->name = kasprintf(GFP_KERNEL, "dfl-uio-%llx", feature->id);
> >> +	if (!uio->name) {
> >> +		ret = -ENOMEM;
> >> +		goto err_name;
> >> +	}
> >> +
> >> +	uio->version = "0.1";
> >> +	uio->mem[0].memtype = UIO_MEM_PHYS;
> >> +	uio->mem[0].addr = res->start & PAGE_MASK;
> >> +	uio->mem[0].offs = res->start & ~PAGE_MASK;
> >> +	uio->mem[0].size = (uio->mem[0].offs + resource_size(res)
> >> +			    + PAGE_SIZE - 1) & PAGE_MASK;
> >> +	/* How are nr_irqs > 1 handled ??? */
> >> +	if (feature->nr_irqs == 1)
> >> +		uio->irq = feature->irq_ctx[0].irq;
> >> +	uio->handler = dfl_uio_handler;
> >> +	uio->mmap = dfl_uio_mmap;
> >> +	uio->open = dfl_uio_open;
> >> +	uio->release = dfl_uio_release;
> >> +	uio->irqcontrol = dfl_uio_irqcontrol;
> >> +
> >> +	ret = uio_register_device(&feature->dev->dev, uio);
> >> +	if (ret)
> >> +		goto err_register;
> >> +
> >> +	feature->uio = uio;
> >> +exit:
> >> +	return ret;
> >> +err_register:
> >> +	kfree(uio->name);
> >> +err_name:
> >> +	kfree(uio);
> >> +	goto exit;
> >> +}
> >> +EXPORT_SYMBOL_GPL(dfl_uio_add);
> >> +
> >> +int dfl_uio_remove(struct dfl_feature *feature)
> >> +{
> >> +	uio_unregister_device(feature->uio);
> >> +	kfree(feature->uio->name);
> >> +	kfree(feature->uio);
> >> +	return 0;
> >> +}
> >> +EXPORT_SYMBOL_GPL(dfl_uio_remove);
> >> +
> >> +static int __init dfl_uio_init(void)
> >> +{
> >> +	return 0;
> >> +}
> >> +
> >> +static void __exit dfl_uio_exit(void)
> >> +{
> >> +}
> >> +
> >> +module_init(dfl_uio_init);
> >> +module_exit(dfl_uio_exit);
> >> +
> >> +MODULE_DESCRIPTION("DFL UIO prototype driver");
> >> +MODULE_AUTHOR("Tom");
> >> +MODULE_LICENSE("GPL v2");
> >> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> >> index 1305be4..26de8e1 100644
> >> --- a/drivers/fpga/dfl.c
> >> +++ b/drivers/fpga/dfl.c
> >> @@ -663,10 +664,57 @@ exit:
> >>  }
> >>  EXPORT_SYMBOL_GPL(dfl_fpga_dev_feature_init);
> >>  
> >> +int dfl_fpga_dev_feature_init_uio(struct platform_device *pdev, int dfh_type)
> >> +{
> >> +	struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> >> +	struct dfl_feature *feature;
> >> +	int ret;
> >> +
> >> +	dfl_fpga_dev_for_each_feature(pdata, feature) {
> >> +		if (dfh_type == DFH_TYPE_FIU) {
> >> +			if (feature->id == FEATURE_ID_FIU_HEADER ||
> >> +			    feature->id == FEATURE_ID_AFU)
> >> +			continue;
> >> +
> >> +			/* give the unclamined feature to uio */
> >> +			if (!feature->ioaddr) {
> >> +				ret = dfl_uio_add(feature);
> >> +				if (ret)
> >> +					goto exit;
> >> +			}
> >> +		}
> >> +	}
> >> +
> >> +	return 0;
> >> +exit:
> >> +	dfl_fpga_dev_feature_uinit_uio(pdev, dfh_type);
> >> +	return ret;
> >> +}
> >> +EXPORT_SYMBOL_GPL(dfl_fpga_dev_feature_init_uio);
> >> +
> >> +int dfl_fpga_dev_feature_uinit_uio(struct platform_device *pdev, int dfh_type)
> >> +{
> >> +	struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> >> +	struct dfl_feature *feature;
> >> +	int ret = 0;
> >> +
> >> +	dfl_fpga_dev_for_each_feature(pdata, feature) {
> >> +		if (dfh_type == DFH_TYPE_FIU) {
> >> +			if (feature->id == FEATURE_ID_FIU_HEADER ||
> >> +			    feature->id == FEATURE_ID_AFU)
> >> +				continue;
> >> +
> >> +			if (feature->uio)
> >> +				ret |= dfl_uio_remove(feature);
> >> +		}
> >> +	}
> >> +	return ret;
> >> +}
> >> +EXPORT_SYMBOL_GPL(dfl_fpga_dev_feature_uinit_uio);
> >> +
> >>  static void dfl_chardev_uinit(void)
> >>  {
> >>  	int i;
> >> -
> >>  	for (i = 0; i < DFL_FPGA_DEVT_MAX; i++)
> >>  		if (MAJOR(dfl_chrdevs[i].devt)) {
> >>  			unregister_chrdev_region(dfl_chrdevs[i].devt,
> >> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> >> index a85d1cd..6e37aef 100644
> >> --- a/drivers/fpga/dfl.h
> >> +++ b/drivers/fpga/dfl.h
> >> @@ -26,6 +26,7 @@
> >>  #include <linux/platform_device.h>
> >>  #include <linux/slab.h>
> >>  #include <linux/uuid.h>
> >> +#include <linux/uio_driver.h>
> >>  #include <linux/fpga/fpga-region.h>
> >>  
> >>  /* maximum supported number of ports */
> >> @@ -232,6 +233,7 @@ struct dfl_feature_irq_ctx {
> >>   * struct dfl_feature - sub feature of the feature devices
> >>   *
> >>   * @dev: ptr to pdev of the feature device which has the sub feature.
> >> + * @uio: for fallback uio driver.
> >>   * @id: sub feature id.
> >>   * @index: unique identifier for an sub feature within the feature device.
> >>   *	   It is possible that multiply sub features with same feature id are
> >> @@ -248,6 +250,7 @@ struct dfl_feature_irq_ctx {
> >>   */
> >>  struct dfl_feature {
> >>  	struct platform_device *dev;
> >> +	struct uio_info *uio;
> >>  	u64 id;
> >>  	int index;
> >>  	int resource_index;
> >> @@ -360,6 +363,11 @@ void dfl_fpga_dev_feature_uinit(struct platform_device *pdev);
> >>  int dfl_fpga_dev_feature_init(struct platform_device *pdev,
> >>  			      struct dfl_feature_driver *feature_drvs);
> >>  
> >> +int dfl_fpga_dev_feature_init_uio(struct platform_device *pdev, int dfh_type);
> >> +int dfl_fpga_dev_feature_uinit_uio(struct platform_device *pdev, int dfh_type);
> >> +int dfl_uio_add(struct dfl_feature *feature);
> >> +int dfl_uio_remove(struct dfl_feature *feature);
> >> +
> >>  int dfl_fpga_dev_ops_register(struct platform_device *pdev,
> >>  			      const struct file_operations *fops,
> >>  			      struct module *owner);
> >> -- 
> >> 2.18.1

Re: [RFC] fpga: dfl: a prototype uio driver

[Tom Rix]

On 9/21/20 1:55 AM, Xu Yilun wrote:
> On Sat, Sep 19, 2020 at 09:51:13AM -0700, trix@redhat.com wrote:
>> From: Tom Rix <trix@redhat.com>
>>
>> I following up on Moritz asking for early RFC's by showing how this
>> could be done with the concrete example of around
>>
>> [PATCH 0/3] add VFIO mdev support for DFL devices
>>
>> I hacked this together, it barely works. Do not expect that this
>> patch to apply anywhere.  It has enough to show where I want
>> to go and people can comment without me having invested a lot of
>> effort.  I am not expecting to carry this forward, it only
>> addresses the issues I had with the origin patch.
>>
>> This change adds a uio driver for any unclaimed dfl feature
>>
>> During the normal matching of feature id's to drivers, some
>> of the features are not claimed because there neither a
>> builtin driver nor a module driver that matches the feature
>> id.  For these unclaimed features, provide a uio driver to a
>> possible user space driver.
> I think we don't have to setup UIO interfaces for all unclaimed 
> features. It could be the decision of the user whether the UIO
> device is needed for a feature. My opinion is that, the
> driver_override sysfs is still generic enough for user's to switch
> between kernel device drivers and dfl uio driver.

Instead of a string, could there just be a 'use_uio' flag ?

How does the 'driver_override' work when there is no starting driver ?

>
> There may be cases the dfl device driver modules are not inserted
> during dfl-fme/port initialization, but they are to be inserted
> manually. If the features are all registered to uio, then there will
> be problems when dfl device drivers module are inserted.

How does this manual loading work ? The driver list for the features

seems to only be used during the card probe time.

To change the order the dfl-pci needs to be unloaded and that will

destroy all the uio devices as well as usual feature drivers attached to

the pci driver.


>
>
>> I have doubts that a uio for an afu feature is approptiate as these
>> can be any device.
> I think generally afu could also be as uio device if we don't concern
> about dma isolation.

I am thinking about afu with its own pci id.

So there could be a conflict with some other driver that responds to the pci id.

>
> But now seems not possible to match afu to uio driver, cause now in DFL
> framework AFU is managed by dfl-afu.ko
>
>> Another possible problem is if the number of interrupts for the
>> feature is greater than 1.  Uio seems to only support 1. My guess
>> is this would need some hackery in the open() to add to the
>> interrupt handler.
> I think it may not possible for UIO to support multiple interrupts if
> user cannot access the interrupt enable/pending registers. The uio
> device have just one /dev/uioX node for interrupt. So I tend to
> accept the limitation. And for now we don't have board specific
> features that needs multiple interrupts. For PAC N3000, no interrupt is
> needed.
Maybe uio needs to change to support multiple interrupts ?
>
>> It is for this type of reason I think a dfl-uio driver is needed
>> rather than reusing an existing generic uio driver.
> So if we don't try to change interrupt, the implementation of dfl-uio is
> just a subset of generic uio platform driver, so why not we just use it?

Its a toss up.

Maybe there some dfl only constraints like write/read is a multiple 4 bytes

Or just why have another layer in the setup.

Tom

>
> Thanks,
> Yilun
>
>> Signed-off-by: Tom Rix <trix@redhat.com>
>> ---
>>  drivers/fpga/dfl-fme-main.c |   9 ++-
>>  drivers/fpga/dfl-uio.c      | 107 ++++++++++++++++++++++++++++++++++++
>>  drivers/fpga/dfl.c          |  47 +++++++++++++++-
>>  drivers/fpga/dfl.h          |   8 +++
>>  4 files changed, 169 insertions(+), 2 deletions(-)
>>  create mode 100644 drivers/fpga/dfl-uio.c
>>
>> diff --git a/drivers/fpga/dfl-fme-main.c b/drivers/fpga/dfl-fme-main.c
>> index 037dc4f..3323e90 100644
>> --- a/drivers/fpga/dfl-fme-main.c
>> +++ b/drivers/fpga/dfl-fme-main.c
>> @@ -709,12 +709,18 @@ static int fme_probe(struct platform_device *pdev)
>>  	if (ret)
>>  		goto dev_destroy;
>>  
>> -	ret = dfl_fpga_dev_ops_register(pdev, &fme_fops, THIS_MODULE);
>> +	ret = dfl_fpga_dev_feature_init_uio(pdev, DFH_TYPE_FIU);
>>  	if (ret)
>>  		goto feature_uinit;
>>  
>> +	ret = dfl_fpga_dev_ops_register(pdev, &fme_fops, THIS_MODULE);
>> +	if (ret)
>> +		goto feature_uinit_uio;
>> +
>>  	return 0;
>>  
>> +feature_uinit_uio:
>> +	dfl_fpga_dev_feature_uinit_uio(pdev, DFH_TYPE_FIU);
>>  feature_uinit:
>>  	dfl_fpga_dev_feature_uinit(pdev);
>>  dev_destroy:
>> @@ -726,6 +732,7 @@ exit:
>>  static int fme_remove(struct platform_device *pdev)
>>  {
>>  	dfl_fpga_dev_ops_unregister(pdev);
>> +	dfl_fpga_dev_feature_uinit_uio(pdev, DFH_TYPE_FIU);
>>  	dfl_fpga_dev_feature_uinit(pdev);
>>  	fme_dev_destroy(pdev);
>>  
>> diff --git a/drivers/fpga/dfl-uio.c b/drivers/fpga/dfl-uio.c
>> new file mode 100644
>> index 0000000..185fbab
>> --- /dev/null
>> +++ b/drivers/fpga/dfl-uio.c
>> @@ -0,0 +1,107 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * prototype dfl uio driver
>> + *
>> + * Copyright Tom Rix 2020
>> + */
>> +#include <linux/module.h>
>> +#include "dfl.h"
>> +
>> +static irqreturn_t dfl_uio_handler(int irq, struct uio_info *dev_info)
>> +{
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int dfl_uio_mmap(struct uio_info *info, struct vm_area_struct *vma)
>> +{
>> +	return 0;
>> +}
>> +
>> +static int dfl_uio_open(struct uio_info *info, struct inode *inode)
>> +{
>> +	return 0;
>> +}
>> +
>> +static int dfl_uio_release(struct uio_info *info, struct inode *inode)
>> +{
>> +	return 0;
>> +}
>> +
>> +static int dfl_uio_irqcontrol(struct uio_info *info, s32 irq_on)
>> +{
>> +	return 0;
>> +}
>> +
>> +int dfl_uio_add(struct dfl_feature *feature)
>> +{
>> +	struct uio_info *uio;
>> +	struct resource *res =
>> +		&feature->dev->resource[feature->resource_index];
>> +	int ret = 0;
>> +
>> +	uio = kzalloc(sizeof(struct uio_info), GFP_KERNEL);
>> +	if (!uio) {
>> +		ret = -ENOMEM;
>> +		goto exit;
>> +	}
>> +
>> +	uio->name = kasprintf(GFP_KERNEL, "dfl-uio-%llx", feature->id);
>> +	if (!uio->name) {
>> +		ret = -ENOMEM;
>> +		goto err_name;
>> +	}
>> +
>> +	uio->version = "0.1";
>> +	uio->mem[0].memtype = UIO_MEM_PHYS;
>> +	uio->mem[0].addr = res->start & PAGE_MASK;
>> +	uio->mem[0].offs = res->start & ~PAGE_MASK;
>> +	uio->mem[0].size = (uio->mem[0].offs + resource_size(res)
>> +			    + PAGE_SIZE - 1) & PAGE_MASK;
>> +	/* How are nr_irqs > 1 handled ??? */
>> +	if (feature->nr_irqs == 1)
>> +		uio->irq = feature->irq_ctx[0].irq;
>> +	uio->handler = dfl_uio_handler;
>> +	uio->mmap = dfl_uio_mmap;
>> +	uio->open = dfl_uio_open;
>> +	uio->release = dfl_uio_release;
>> +	uio->irqcontrol = dfl_uio_irqcontrol;
>> +
>> +	ret = uio_register_device(&feature->dev->dev, uio);
>> +	if (ret)
>> +		goto err_register;
>> +
>> +	feature->uio = uio;
>> +exit:
>> +	return ret;
>> +err_register:
>> +	kfree(uio->name);
>> +err_name:
>> +	kfree(uio);
>> +	goto exit;
>> +}
>> +EXPORT_SYMBOL_GPL(dfl_uio_add);
>> +
>> +int dfl_uio_remove(struct dfl_feature *feature)
>> +{
>> +	uio_unregister_device(feature->uio);
>> +	kfree(feature->uio->name);
>> +	kfree(feature->uio);
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(dfl_uio_remove);
>> +
>> +static int __init dfl_uio_init(void)
>> +{
>> +	return 0;
>> +}
>> +
>> +static void __exit dfl_uio_exit(void)
>> +{
>> +}
>> +
>> +module_init(dfl_uio_init);
>> +module_exit(dfl_uio_exit);
>> +
>> +MODULE_DESCRIPTION("DFL UIO prototype driver");
>> +MODULE_AUTHOR("Tom");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
>> index 1305be4..26de8e1 100644
>> --- a/drivers/fpga/dfl.c
>> +++ b/drivers/fpga/dfl.c
>> @@ -663,10 +664,57 @@ exit:
>>  }
>>  EXPORT_SYMBOL_GPL(dfl_fpga_dev_feature_init);
>>  
>> +int dfl_fpga_dev_feature_init_uio(struct platform_device *pdev, int dfh_type)
>> +{
>> +	struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
>> +	struct dfl_feature *feature;
>> +	int ret;
>> +
>> +	dfl_fpga_dev_for_each_feature(pdata, feature) {
>> +		if (dfh_type == DFH_TYPE_FIU) {
>> +			if (feature->id == FEATURE_ID_FIU_HEADER ||
>> +			    feature->id == FEATURE_ID_AFU)
>> +			continue;
>> +
>> +			/* give the unclamined feature to uio */
>> +			if (!feature->ioaddr) {
>> +				ret = dfl_uio_add(feature);
>> +				if (ret)
>> +					goto exit;
>> +			}
>> +		}
>> +	}
>> +
>> +	return 0;
>> +exit:
>> +	dfl_fpga_dev_feature_uinit_uio(pdev, dfh_type);
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(dfl_fpga_dev_feature_init_uio);
>> +
>> +int dfl_fpga_dev_feature_uinit_uio(struct platform_device *pdev, int dfh_type)
>> +{
>> +	struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
>> +	struct dfl_feature *feature;
>> +	int ret = 0;
>> +
>> +	dfl_fpga_dev_for_each_feature(pdata, feature) {
>> +		if (dfh_type == DFH_TYPE_FIU) {
>> +			if (feature->id == FEATURE_ID_FIU_HEADER ||
>> +			    feature->id == FEATURE_ID_AFU)
>> +				continue;
>> +
>> +			if (feature->uio)
>> +				ret |= dfl_uio_remove(feature);
>> +		}
>> +	}
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(dfl_fpga_dev_feature_uinit_uio);
>> +
>>  static void dfl_chardev_uinit(void)
>>  {
>>  	int i;
>> -
>>  	for (i = 0; i < DFL_FPGA_DEVT_MAX; i++)
>>  		if (MAJOR(dfl_chrdevs[i].devt)) {
>>  			unregister_chrdev_region(dfl_chrdevs[i].devt,
>> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
>> index a85d1cd..6e37aef 100644
>> --- a/drivers/fpga/dfl.h
>> +++ b/drivers/fpga/dfl.h
>> @@ -26,6 +26,7 @@
>>  #include <linux/platform_device.h>
>>  #include <linux/slab.h>
>>  #include <linux/uuid.h>
>> +#include <linux/uio_driver.h>
>>  #include <linux/fpga/fpga-region.h>
>>  
>>  /* maximum supported number of ports */
>> @@ -232,6 +233,7 @@ struct dfl_feature_irq_ctx {
>>   * struct dfl_feature - sub feature of the feature devices
>>   *
>>   * @dev: ptr to pdev of the feature device which has the sub feature.
>> + * @uio: for fallback uio driver.
>>   * @id: sub feature id.
>>   * @index: unique identifier for an sub feature within the feature device.
>>   *	   It is possible that multiply sub features with same feature id are
>> @@ -248,6 +250,7 @@ struct dfl_feature_irq_ctx {
>>   */
>>  struct dfl_feature {
>>  	struct platform_device *dev;
>> +	struct uio_info *uio;
>>  	u64 id;
>>  	int index;
>>  	int resource_index;
>> @@ -360,6 +363,11 @@ void dfl_fpga_dev_feature_uinit(struct platform_device *pdev);
>>  int dfl_fpga_dev_feature_init(struct platform_device *pdev,
>>  			      struct dfl_feature_driver *feature_drvs);
>>  
>> +int dfl_fpga_dev_feature_init_uio(struct platform_device *pdev, int dfh_type);
>> +int dfl_fpga_dev_feature_uinit_uio(struct platform_device *pdev, int dfh_type);
>> +int dfl_uio_add(struct dfl_feature *feature);
>> +int dfl_uio_remove(struct dfl_feature *feature);
>> +
>>  int dfl_fpga_dev_ops_register(struct platform_device *pdev,
>>  			      const struct file_operations *fops,
>>  			      struct module *owner);
>> -- 
>> 2.18.1

[RFC] fpga: dfl: a prototype uio driver

[trix]

From: Tom Rix <trix@redhat.com>

I following up on Moritz asking for early RFC's by showing how this
could be done with the concrete example of around

[PATCH 0/3] add VFIO mdev support for DFL devices

I hacked this together, it barely works. Do not expect that this
patch to apply anywhere.  It has enough to show where I want
to go and people can comment without me having invested a lot of
effort.  I am not expecting to carry this forward, it only
addresses the issues I had with the origin patch.

This change adds a uio driver for any unclaimed dfl feature

During the normal matching of feature id's to drivers, some
of the features are not claimed because there neither a
builtin driver nor a module driver that matches the feature
id.  For these unclaimed features, provide a uio driver to a
possible user space driver.

I have doubts that a uio for an afu feature is approptiate as these
can be any device.

Another possible problem is if the number of interrupts for the
feature is greater than 1.  Uio seems to only support 1. My guess
is this would need some hackery in the open() to add to the
interrupt handler.

It is for this type of reason I think a dfl-uio driver is needed
rather than reusing an existing generic uio driver.

Signed-off-by: Tom Rix <trix@redhat.com>
---
 drivers/fpga/dfl-fme-main.c |   9 ++-
 drivers/fpga/dfl-uio.c      | 107 ++++++++++++++++++++++++++++++++++++
 drivers/fpga/dfl.c          |  47 +++++++++++++++-
 drivers/fpga/dfl.h          |   8 +++
 4 files changed, 169 insertions(+), 2 deletions(-)
 create mode 100644 drivers/fpga/dfl-uio.c

diff --git a/drivers/fpga/dfl-fme-main.c b/drivers/fpga/dfl-fme-main.c
index 037dc4f..3323e90 100644
--- a/drivers/fpga/dfl-fme-main.c
+++ b/drivers/fpga/dfl-fme-main.c
@@ -709,12 +709,18 @@ static int fme_probe(struct platform_device *pdev)
 	if (ret)
 		goto dev_destroy;
 
-	ret = dfl_fpga_dev_ops_register(pdev, &fme_fops, THIS_MODULE);
+	ret = dfl_fpga_dev_feature_init_uio(pdev, DFH_TYPE_FIU);
 	if (ret)
 		goto feature_uinit;
 
+	ret = dfl_fpga_dev_ops_register(pdev, &fme_fops, THIS_MODULE);
+	if (ret)
+		goto feature_uinit_uio;
+
 	return 0;
 
+feature_uinit_uio:
+	dfl_fpga_dev_feature_uinit_uio(pdev, DFH_TYPE_FIU);
 feature_uinit:
 	dfl_fpga_dev_feature_uinit(pdev);
 dev_destroy:
@@ -726,6 +732,7 @@ exit:
 static int fme_remove(struct platform_device *pdev)
 {
 	dfl_fpga_dev_ops_unregister(pdev);
+	dfl_fpga_dev_feature_uinit_uio(pdev, DFH_TYPE_FIU);
 	dfl_fpga_dev_feature_uinit(pdev);
 	fme_dev_destroy(pdev);
 
diff --git a/drivers/fpga/dfl-uio.c b/drivers/fpga/dfl-uio.c
new file mode 100644
index 0000000..185fbab
--- /dev/null
+++ b/drivers/fpga/dfl-uio.c
@@ -0,0 +1,107 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * prototype dfl uio driver
+ *
+ * Copyright Tom Rix 2020
+ */
+#include <linux/module.h>
+#include "dfl.h"
+
+static irqreturn_t dfl_uio_handler(int irq, struct uio_info *dev_info)
+{
+	return IRQ_HANDLED;
+}
+
+static int dfl_uio_mmap(struct uio_info *info, struct vm_area_struct *vma)
+{
+	return 0;
+}
+
+static int dfl_uio_open(struct uio_info *info, struct inode *inode)
+{
+	return 0;
+}
+
+static int dfl_uio_release(struct uio_info *info, struct inode *inode)
+{
+	return 0;
+}
+
+static int dfl_uio_irqcontrol(struct uio_info *info, s32 irq_on)
+{
+	return 0;
+}
+
+int dfl_uio_add(struct dfl_feature *feature)
+{
+	struct uio_info *uio;
+	struct resource *res =
+		&feature->dev->resource[feature->resource_index];
+	int ret = 0;
+
+	uio = kzalloc(sizeof(struct uio_info), GFP_KERNEL);
+	if (!uio) {
+		ret = -ENOMEM;
+		goto exit;
+	}
+
+	uio->name = kasprintf(GFP_KERNEL, "dfl-uio-%llx", feature->id);
+	if (!uio->name) {
+		ret = -ENOMEM;
+		goto err_name;
+	}
+
+	uio->version = "0.1";
+	uio->mem[0].memtype = UIO_MEM_PHYS;
+	uio->mem[0].addr = res->start & PAGE_MASK;
+	uio->mem[0].offs = res->start & ~PAGE_MASK;
+	uio->mem[0].size = (uio->mem[0].offs + resource_size(res)
+			    + PAGE_SIZE - 1) & PAGE_MASK;
+	/* How are nr_irqs > 1 handled ??? */
+	if (feature->nr_irqs == 1)
+		uio->irq = feature->irq_ctx[0].irq;
+	uio->handler = dfl_uio_handler;
+	uio->mmap = dfl_uio_mmap;
+	uio->open = dfl_uio_open;
+	uio->release = dfl_uio_release;
+	uio->irqcontrol = dfl_uio_irqcontrol;
+
+	ret = uio_register_device(&feature->dev->dev, uio);
+	if (ret)
+		goto err_register;
+
+	feature->uio = uio;
+exit:
+	return ret;
+err_register:
+	kfree(uio->name);
+err_name:
+	kfree(uio);
+	goto exit;
+}
+EXPORT_SYMBOL_GPL(dfl_uio_add);
+
+int dfl_uio_remove(struct dfl_feature *feature)
+{
+	uio_unregister_device(feature->uio);
+	kfree(feature->uio->name);
+	kfree(feature->uio);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(dfl_uio_remove);
+
+static int __init dfl_uio_init(void)
+{
+	return 0;
+}
+
+static void __exit dfl_uio_exit(void)
+{
+}
+
+module_init(dfl_uio_init);
+module_exit(dfl_uio_exit);
+
+MODULE_DESCRIPTION("DFL UIO prototype driver");
+MODULE_AUTHOR("Tom");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
index 1305be4..26de8e1 100644
--- a/drivers/fpga/dfl.c
+++ b/drivers/fpga/dfl.c
@@ -663,10 +664,57 @@ exit:
 }
 EXPORT_SYMBOL_GPL(dfl_fpga_dev_feature_init);
 
+int dfl_fpga_dev_feature_init_uio(struct platform_device *pdev, int dfh_type)
+{
+	struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
+	struct dfl_feature *feature;
+	int ret;
+
+	dfl_fpga_dev_for_each_feature(pdata, feature) {
+		if (dfh_type == DFH_TYPE_FIU) {
+			if (feature->id == FEATURE_ID_FIU_HEADER ||
+			    feature->id == FEATURE_ID_AFU)
+			continue;
+
+			/* give the unclamined feature to uio */
+			if (!feature->ioaddr) {
+				ret = dfl_uio_add(feature);
+				if (ret)
+					goto exit;
+			}
+		}
+	}
+
+	return 0;
+exit:
+	dfl_fpga_dev_feature_uinit_uio(pdev, dfh_type);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(dfl_fpga_dev_feature_init_uio);
+
+int dfl_fpga_dev_feature_uinit_uio(struct platform_device *pdev, int dfh_type)
+{
+	struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
+	struct dfl_feature *feature;
+	int ret = 0;
+
+	dfl_fpga_dev_for_each_feature(pdata, feature) {
+		if (dfh_type == DFH_TYPE_FIU) {
+			if (feature->id == FEATURE_ID_FIU_HEADER ||
+			    feature->id == FEATURE_ID_AFU)
+				continue;
+
+			if (feature->uio)
+				ret |= dfl_uio_remove(feature);
+		}
+	}
+	return ret;
+}
+EXPORT_SYMBOL_GPL(dfl_fpga_dev_feature_uinit_uio);
+
 static void dfl_chardev_uinit(void)
 {
 	int i;
-
 	for (i = 0; i < DFL_FPGA_DEVT_MAX; i++)
 		if (MAJOR(dfl_chrdevs[i].devt)) {
 			unregister_chrdev_region(dfl_chrdevs[i].devt,
diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
index a85d1cd..6e37aef 100644
--- a/drivers/fpga/dfl.h
+++ b/drivers/fpga/dfl.h
@@ -26,6 +26,7 @@
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/uuid.h>
+#include <linux/uio_driver.h>
 #include <linux/fpga/fpga-region.h>
 
 /* maximum supported number of ports */
@@ -232,6 +233,7 @@ struct dfl_feature_irq_ctx {
  * struct dfl_feature - sub feature of the feature devices
  *
  * @dev: ptr to pdev of the feature device which has the sub feature.
+ * @uio: for fallback uio driver.
  * @id: sub feature id.
  * @index: unique identifier for an sub feature within the feature device.
  *	   It is possible that multiply sub features with same feature id are
@@ -248,6 +250,7 @@ struct dfl_feature_irq_ctx {
  */
 struct dfl_feature {
 	struct platform_device *dev;
+	struct uio_info *uio;
 	u64 id;
 	int index;
 	int resource_index;
@@ -360,6 +363,11 @@ void dfl_fpga_dev_feature_uinit(struct platform_device *pdev);
 int dfl_fpga_dev_feature_init(struct platform_device *pdev,
 			      struct dfl_feature_driver *feature_drvs);
 
+int dfl_fpga_dev_feature_init_uio(struct platform_device *pdev, int dfh_type);
+int dfl_fpga_dev_feature_uinit_uio(struct platform_device *pdev, int dfh_type);
+int dfl_uio_add(struct dfl_feature *feature);
+int dfl_uio_remove(struct dfl_feature *feature);
+
 int dfl_fpga_dev_ops_register(struct platform_device *pdev,
 			      const struct file_operations *fops,
 			      struct module *owner);
-- 
2.18.1