Message-ID: <DM6PR11MB3819C105F324C0E6CF2747D385C00@DM6PR11MB3819.namprd11.prod.outlook.com>
Date:   Mon, 21 Dec 2020 06:23:38 +0000
From:   "Wu, Hao" <hao.wu@...el.com>
To:     "Xu, Yilun" <yilun.xu@...el.com>, Tom Rix <trix@...hat.com>
CC:     "mdf@...nel.org" <mdf@...nel.org>,
        "linux-fpga@...r.kernel.org" <linux-fpga@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "lgoncalv@...hat.com" <lgoncalv@...hat.com>
Subject: RE: [PATCH v3 2/3] fpga: dfl: add the userspace I/O device support
  for DFL devices

> Subject: Re: [PATCH v3 2/3] fpga: dfl: add the userspace I/O device support
> for DFL devices
> 
> On Fri, Dec 18, 2020 at 05:59:17AM -0800, Tom Rix wrote:
> >
> > On 12/18/20 12:05 AM, Wu, Hao wrote:
> > >> Subject: [PATCH v3 2/3] fpga: dfl: add the userspace I/O device support
> for
> > >> DFL devices
> > >>
> > >> This patch supports the DFL drivers be written in userspace. This is
> > >> realized by exposing the userspace I/O device interfaces.
> > >>
> > >> The driver leverages the uio_pdrv_genirq, it adds the uio_pdrv_genirq
> > >> platform device with the DFL device's resources, and let the generic UIO
> > >> platform device driver provide support to userspace access to kernel
> > >> interrupts and memory locations.
> > >>
> > >> The driver matches DFL devices in a different way. It has no device id
> > >> table, instead it matches any DFL device which could not be handled by
> > >> other DFL drivers.
> > > Looks like we want to build UIO driver as the default/generic driver for
> DFL,
> 
> I'm not going to make UIO as the default driver for DFL devs, the driver
> module will not be autoloaded. I want to provide a choice to operate on
> the unhandled devs in userspace. Insmod it if it helps otherwise leave
> it.

But once it's loaded, then it will bind with all dfl devices which doesn't have
a matched driver yet. 

> 
> > > it seems fine but my concern is that UIO has its own limitation, if some
> day,
> > > dfl device is extended, but UIO has limitation, then we may need to select
> > > another one as the default driver.. or we can just match them using
> 
> I think we may not have to select a "default" driver, if we have a
> better way for userspace accessing, we could load that module, leave
> UIO.

It would not be an easy decision, it may impact users who have already
built upper layer software on top of UIO, and ask new users to use new
one too. Users may not be happy with this.

> 
> > > id_table as we know UIO meets the requirement for those DFL devices?
> 
> As we discussed, the drawback is that we should always change the code to
> support a new dfl device for user accessing. But it is OK to me if the
> generic UIO match rule is not considered proper.
> 
> >
> > When we have multiple defaults, could this be handled in the configury ?
> 
> I think we don't have to select a "default".

Actually I am fine with the idea of using a default / generic driver for DFL
device, and my only concern is UIO may not be good enough for us per my
understanding. Maybe we can have a more extendable one as the generic
driver, but if we don't have that yet, then using id table may be able to
avoid some problems I think.

Hao

> 
> >
> > Tom
> >
> > >
> > >> Signed-off-by: Xu Yilun <yilun.xu@...el.com>
> > >> ---
> > >> v2: switch to the new matching algorithem. It matches DFL devices which
> > >>      could not be handled by other DFL drivers.
> > >>     refacor the code about device resources filling.
> > >>     fix some comments.
> > >> v3: split the dfl.c changes out of this patch.
> > >>     some minor fixes
> > >> ---
> > >>  drivers/fpga/Kconfig        |  10 ++++
> > >>  drivers/fpga/Makefile       |   1 +
> > >>  drivers/fpga/dfl-uio-pdev.c | 110
> > >> ++++++++++++++++++++++++++++++++++++++++++++
> > >>  3 files changed, 121 insertions(+)
> > >>  create mode 100644 drivers/fpga/dfl-uio-pdev.c
> > >>
> > >> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> > >> index 5d7f0ae..7a88af9 100644
> > >> --- a/drivers/fpga/Kconfig
> > >> +++ b/drivers/fpga/Kconfig
> > >> @@ -202,6 +202,16 @@ config FPGA_DFL_NIOS_INTEL_PAC_N3000
> > >>  	  the card. It also instantiates the SPI master (spi-altera) for
> > >>  	  the card's BMC (Board Management Controller).
> > >>
> > >> +config FPGA_DFL_UIO_PDEV
> > >> +	tristate "FPGA DFL Driver for Userspace I/O platform devices"
> > >> +	depends on FPGA_DFL && UIO_PDRV_GENIRQ
> > >> +	help
> > >> +	  Enable this to allow some DFL drivers be written in userspace. It
> > >> +	  adds the uio_pdrv_genirq platform device with the DFL feature's
> > >> +	  resources, and lets the generic UIO platform device driver provide
> > >> +	  support for userspace access to kernel interrupts and memory
> > >> +	  locations.
> > > If we consider this as a default driver for everybody in DFL, then we could
> > > consider build it into the core, otherwise it always requires to be loaded
> > > manually, right?
> 
> It should be loaded manually. I don't want to make this as default.
> 
> > >
> > >> +
> > >>  config FPGA_DFL_PCI
> > >>  	tristate "FPGA DFL PCIe Device Driver"
> > >>  	depends on PCI && FPGA_DFL
> > >> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> > >> index 18dc9885..8847fe0 100644
> > >> --- a/drivers/fpga/Makefile
> > >> +++ b/drivers/fpga/Makefile
> > >> @@ -45,6 +45,7 @@ dfl-afu-objs := dfl-afu-main.o dfl-afu-region.o dfl-
> afu-
> > >> dma-region.o
> > >>  dfl-afu-objs += dfl-afu-error.o
> > >>
> > >>  obj-$(CONFIG_FPGA_DFL_NIOS_INTEL_PAC_N3000)	+= dfl-n3000-
> nios.o
> > >> +obj-$(CONFIG_FPGA_DFL_UIO_PDEV)		+= dfl-uio-pdev.o
> > >>
> > >>  # Drivers for FPGAs which implement DFL
> > >>  obj-$(CONFIG_FPGA_DFL_PCI)		+= dfl-pci.o
> > >> diff --git a/drivers/fpga/dfl-uio-pdev.c b/drivers/fpga/dfl-uio-pdev.c
> > >> new file mode 100644
> > >> index 0000000..8c57233
> > >> --- /dev/null
> > >> +++ b/drivers/fpga/dfl-uio-pdev.c
> > >> @@ -0,0 +1,110 @@
> > >> +// SPDX-License-Identifier: GPL-2.0
> > >> +/*
> > >> + * DFL driver for Userspace I/O platform devices
> > >> + *
> > >> + * Copyright (C) 2020 Intel Corporation, Inc.
> > >> + */
> > >> +#include <linux/dfl.h>
> > >> +#include <linux/errno.h>
> > >> +#include <linux/kernel.h>
> > >> +#include <linux/module.h>
> > >> +#include <linux/platform_device.h>
> > >> +#include <linux/slab.h>
> > >> +#include <linux/uio_driver.h>
> > >> +
> > >> +#include "dfl.h"
> > >> +
> > >> +#define DRIVER_NAME "dfl-uio-pdev"
> > >> +
> > >> +static struct dfl_driver dfl_uio_pdev_driver;
> > >> +
> > >> +static int check_for_other_drv_match(struct device_driver *drv, void
> *data)
> > >> +{
> > >> +	struct dfl_driver *ddrv = to_dfl_drv(drv);
> > >> +	struct dfl_device *ddev = data;
> > >> +
> > >> +	/* skip myself */
> > >> +	if (ddrv == &dfl_uio_pdev_driver)
> > >> +		return 0;
> > >> +
> > >> +	return dfl_match_device(ddev, ddrv);
> > >> +}
> > >> +
> > >> +static int dfl_uio_pdev_match(struct dfl_device *ddev)
> > >> +{
> > >> +	/*
> > >> +	 * If any other driver wants the device, leave the device to this other
> > >> +	 * driver.
> > >> +	 */
> > >> +	if (bus_for_each_drv(&dfl_bus_type, NULL, ddev,
> > >> check_for_other_drv_match))
> > >> +		return 0;
> > >> +
> > >> +	return 1;
> > >> +}
> > >> +
> > >> +static int dfl_uio_pdev_probe(struct dfl_device *ddev)
> > >> +{
> > >> +	struct device *dev = &ddev->dev;
> > >> +	struct platform_device_info pdevinfo = { 0 };
> > >> +	struct uio_info uio_pdata = { 0 };
> > >> +	struct platform_device *uio_pdev;
> > >> +	struct resource *res;
> > >> +	int i;
> > >> +
> > >> +	pdevinfo.name = "uio_pdrv_genirq";
> > >> +
> > >> +	res = kcalloc(ddev->num_irqs + 1, sizeof(*res), GFP_KERNEL);
> > >> +	if (!res)
> > >> +		return -ENOMEM;
> > >> +
> > >> +	res[0].parent = &ddev->mmio_res;
> > >> +	res[0].flags = IORESOURCE_MEM;
> > >> +	res[0].start = ddev->mmio_res.start;
> > >> +	res[0].end = ddev->mmio_res.end;
> > >> +
> > >> +	/* then add irq resource */
> > >> +	for (i = 0; i < ddev->num_irqs; i++) {
> > >> +		res[i + 1].flags = IORESOURCE_IRQ;
> > >> +		res[i + 1].start = ddev->irqs[i];
> > >> +		res[i + 1].end = ddev->irqs[i];
> > >> +	}
> > > How many interrupts UIO could support? or we need some
> > > warning or just even return error here?
> 
> Yes, I can add some warning here.
> 
> Thanks,
> Yilun

Powered by blists - more mailing lists