Re: [PATCH v4] Add new uio device for PCI with dynamic memory allocation
On Mi, 2020-04-29 at 11:41 +0200, gre...@linuxfoundation.org wrote: > On Wed, Apr 29, 2020 at 07:51:01AM +, Stahl, Manuel wrote: > > On Di, 2020-04-28 at 15:54 +0200, gregkh @ linuxfoundation . org wrote: > > > On Thu, Apr 16, 2020 at 06:38:30PM +0200, Manuel Stahl wrote: > > > > > > > > + * > > > > + * Since the driver does not declare any device ids, you must allocate > > > > + * id and bind the device to the driver yourself. For example: > > > > + * > > > > + * # echo "8086 10f5" > /sys/bus/pci/drivers/uio_pci_dmem_genirq/new_id > > > > + * # echo -n :00:19.0 > /sys/bus/pci/drivers/e1000e/unbind > > > > + * # echo -n :00:19.0 > > > > > /sys/bus/pci/drivers/uio_pci_dmem_genirq/bind > > > > + * # ls -l /sys/bus/pci/devices/:00:19.0/driver > > > > + * .../:00:19.0/driver -> > > > > ../../../bus/pci/drivers/uio_pci_dmem_genirq > > > > + * > > > > + * Or use a modprobe alias: > > > > + * # alias pci:v10EEd1000sv*sd*sc*i* uio_pci_dmem_genirq > > > > + * > > > > + * Driver won't bind to devices which do not support the Interrupt > > > > Disable Bit > > > > + * in the command register. All devices compliant to PCI 2.3 (circa > > > > 2002) and > > > > + * all compliant PCI Express devices should support this bit. > > > > + * > > > > + * The DMA mask bits and sizes of dynamic regions are derived from > > > > module > > > > + * parameters. > > > > + * > > > > + * The format for specifying dynamic region sizes in module parameters > > > > + * is as follows: > > > > + * > > > > + * uio_pci_dmem_genirq.dmem_sizes := > > > > [;] > > > > + *:= :[,] > > > > + *:= : > > > > + * := standard linux memsize > > > > + * > > > > + * Examples: > > > > + * > > > > + * 1) UIO dmem device with 3 dynamic regions: > > > > + * uio_pci_dmem_genirq.dmem_sizes=8086:10f5:4K,16K,4M > > > > + * > > > > + * 2) Two UIO dmem devices with different number of dynamic regions: > > > > + * uio_pci_dmem_genirq.dmem_sizes=8086:10f5:4K,16K,4M;1234:0001:8K > > > > > > Module parameters are horrid, are you sure there is no other way? > > > > You're right, seemed to be the simplest solution back when we started > > developing this driver. I will try to change it to sysfs, so that one can > > add regions while the module is already loaded. > > /me hands you some \n characters... > > Anyway, configfs is for configuring stuff, don't make a sysfs file that > you have to somehow "parse" please. Looking back at this driver after some years I realized again the reason for using kernel parameters: The current UIO API needs the information about available memory maps when registering a new UIO device with __uio_register_device(), which obviously needs to be called during probe() in uio_pci_dmem_genirq. Otherwise there is no device file in /dev to open for user space applications. After that there is no function to update the uio_map info. So we can either keep the module parameters and allocate the DMA memory during probe() or allocate the DMA memory during mmap() and a) replicate parts of uio_dev_add_attributes() in this driver to update sysfs b) add a function in uio.c to allow updates to the uio_map Which way would you go? Best regards, Manuel Stahl
Re: [PATCH v4] Add new uio device for PCI with dynamic memory allocation
On Wed, Apr 29, 2020 at 07:51:01AM +, Stahl, Manuel wrote: > On Di, 2020-04-28 at 15:54 +0200, gregkh @ linuxfoundation . org wrote: > > On Thu, Apr 16, 2020 at 06:38:30PM +0200, Manuel Stahl wrote: > > > > > > + * > > > + * Since the driver does not declare any device ids, you must allocate > > > + * id and bind the device to the driver yourself. For example: > > > + * > > > + * # echo "8086 10f5" > /sys/bus/pci/drivers/uio_pci_dmem_genirq/new_id > > > + * # echo -n :00:19.0 > /sys/bus/pci/drivers/e1000e/unbind > > > + * # echo -n :00:19.0 > /sys/bus/pci/drivers/uio_pci_dmem_genirq/bind > > > + * # ls -l /sys/bus/pci/devices/:00:19.0/driver > > > + * .../:00:19.0/driver -> > > > ../../../bus/pci/drivers/uio_pci_dmem_genirq > > > + * > > > + * Or use a modprobe alias: > > > + * # alias pci:v10EEd1000sv*sd*sc*i* uio_pci_dmem_genirq > > > + * > > > + * Driver won't bind to devices which do not support the Interrupt > > > Disable Bit > > > + * in the command register. All devices compliant to PCI 2.3 (circa > > > 2002) and > > > + * all compliant PCI Express devices should support this bit. > > > + * > > > + * The DMA mask bits and sizes of dynamic regions are derived from module > > > + * parameters. > > > + * > > > + * The format for specifying dynamic region sizes in module parameters > > > + * is as follows: > > > + * > > > + * uio_pci_dmem_genirq.dmem_sizes := > > > [;] > > > + *:= :[,] > > > + *:= : > > > + * := standard linux memsize > > > + * > > > + * Examples: > > > + * > > > + * 1) UIO dmem device with 3 dynamic regions: > > > + * uio_pci_dmem_genirq.dmem_sizes=8086:10f5:4K,16K,4M > > > + * > > > + * 2) Two UIO dmem devices with different number of dynamic regions: > > > + * uio_pci_dmem_genirq.dmem_sizes=8086:10f5:4K,16K,4M;1234:0001:8K > > > > Module parameters are horrid, are you sure there is no other way? > > You're right, seemed to be the simplest solution back when we started > developing this driver. I will try to change it to sysfs, so that one can add > regions while the module is already loaded. /me hands you some \n characters... Anyway, configfs is for configuring stuff, don't make a sysfs file that you have to somehow "parse" please. thanks, greg k-h
Re: [PATCH v4] Add new uio device for PCI with dynamic memory allocation
On Di, 2020-04-28 at 15:54 +0200, gregkh @ linuxfoundation . org wrote: > On Thu, Apr 16, 2020 at 06:38:30PM +0200, Manuel Stahl wrote: > > > > + * > > + * Since the driver does not declare any device ids, you must allocate > > + * id and bind the device to the driver yourself. For example: > > + * > > + * # echo "8086 10f5" > /sys/bus/pci/drivers/uio_pci_dmem_genirq/new_id > > + * # echo -n :00:19.0 > /sys/bus/pci/drivers/e1000e/unbind > > + * # echo -n :00:19.0 > /sys/bus/pci/drivers/uio_pci_dmem_genirq/bind > > + * # ls -l /sys/bus/pci/devices/:00:19.0/driver > > + * .../:00:19.0/driver -> ../../../bus/pci/drivers/uio_pci_dmem_genirq > > + * > > + * Or use a modprobe alias: > > + * # alias pci:v10EEd1000sv*sd*sc*i* uio_pci_dmem_genirq > > + * > > + * Driver won't bind to devices which do not support the Interrupt Disable > > Bit > > + * in the command register. All devices compliant to PCI 2.3 (circa 2002) > > and > > + * all compliant PCI Express devices should support this bit. > > + * > > + * The DMA mask bits and sizes of dynamic regions are derived from module > > + * parameters. > > + * > > + * The format for specifying dynamic region sizes in module parameters > > + * is as follows: > > + * > > + * uio_pci_dmem_genirq.dmem_sizes := > > [;] > > + *:= :[,] > > + *:= : > > + * := standard linux memsize > > + * > > + * Examples: > > + * > > + * 1) UIO dmem device with 3 dynamic regions: > > + * uio_pci_dmem_genirq.dmem_sizes=8086:10f5:4K,16K,4M > > + * > > + * 2) Two UIO dmem devices with different number of dynamic regions: > > + * uio_pci_dmem_genirq.dmem_sizes=8086:10f5:4K,16K,4M;1234:0001:8K > > Module parameters are horrid, are you sure there is no other way? You're right, seemed to be the simplest solution back when we started developing this driver. I will try to change it to sysfs, so that one can add regions while the module is already loaded. Regards, Manuel
Re: [PATCH v4] Add new uio device for PCI with dynamic memory allocation
On Tue, Apr 28, 2020 at 03:47:42PM +, Stahl, Manuel wrote: > > > > > + return err; > > > + } > > > + pci_set_master(pdev); > > > + > > > + dev_info(>dev, "Legacy IRQ: %i", pdev->irq); > > > > Again, remove, be quiet :) > > Use dev_dbg() or remove completely? If it helps in debugging, dev_dbg() is fine to use. thanks, greg k-h
Re: [PATCH v4] Add new uio device for PCI with dynamic memory allocation
On Di, 2020-04-28 at 15:54 +0200, greg k-h wrote: > On Thu, Apr 16, 2020 at 06:38:30PM +0200, Manuel Stahl wrote: > > This device combines the uio_pci_generic driver and the uio_dmem_genirq > > driver since PCI uses a slightly different API for interrupts. > > A fixed number of DMA capable memory regions can be defined using the > > module parameter "dmem_sizes". The memory is not allocated until the uio > > device file is opened for the first time. When the device file is closed, > > the allocated memory block is freed. Physical (DMA) addresses for the > > dynamic regions are provided to the userspace via > > /sys/class/uio/uioX/maps/mapY/addr > > When no processes are holding the device file open, the address returned > > to userspace is DMA_ERROR_CODE. > > > > Signed-off-by: Manuel Stahl > > --- > > MAINTAINERS | 6 + > > drivers/uio/Kconfig | 9 + > > drivers/uio/Makefile | 1 + > > drivers/uio/uio_pci_dmem_genirq.c | 351 ++ > > 4 files changed, 367 insertions(+) > > create mode 100644 drivers/uio/uio_pci_dmem_genirq.c > > What changed from previous versions? Always put that below the --- > line. Only rebased to the latest kernel version and incorporated changes from uio_pci_generic and uio_dmem_genirq. No recommendations were made about other required changes, the discussion stopped about the fact if such a driver (UIO with DMA) should be allowed at all. For me such a driver allows me to quickly test different FPGA designs on computers that don't have specific security requirements. > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index e64e5db31497..446931530dbc 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -7149,6 +7149,12 @@ L: k...@vger.kernel.org > > S: Supported > > F: drivers/uio/uio_pci_generic.c > > > > +GENERIC UIO DRIVER FOR PCI DEVICES WITH DMA > > +M: "Manuel Stahl" > > +L: k...@vger.kernel.org > > +S: Supported > > +F: drivers/uio/uio_pci_dmem_genirq.c > > + > > GENERIC VDSO LIBRARY > > M: Andy Lutomirski > > M: Thomas Gleixner > > diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig > > index 202ee81cfc2b..0d3f8a01ec74 100644 > > --- a/drivers/uio/Kconfig > > +++ b/drivers/uio/Kconfig > > @@ -94,6 +94,15 @@ config UIO_PCI_GENERIC > > primarily, for virtualization scenarios. > > If you compile this as a module, it will be called uio_pci_generic. > > > > +config UIO_PCI_DMEM_GENIRQ > > + tristate "Generic driver for PCI 2.3 and PCI Express cards with DMA" > > + depends on PCI > > + help > > + Generic driver that you can bind, dynamically, to any > > + PCI 2.3 compliant and PCI Express card. It is useful > > + for FPGAs with DMA capability connected via PCI. > > + If you compile this as a module, it will be called > > uio_pci_dmem_genirq. > > + > > config UIO_NETX > > tristate "Hilscher NetX Card driver" > > depends on PCI > > diff --git a/drivers/uio/Makefile b/drivers/uio/Makefile > > index c285dd2a4539..202d6bfdd5aa 100644 > > --- a/drivers/uio/Makefile > > +++ b/drivers/uio/Makefile > > @@ -6,6 +6,7 @@ obj-$(CONFIG_UIO_DMEM_GENIRQ) += uio_dmem_genirq.o > > obj-$(CONFIG_UIO_AEC) += uio_aec.o > > obj-$(CONFIG_UIO_SERCOS3) += uio_sercos3.o > > obj-$(CONFIG_UIO_PCI_GENERIC) += uio_pci_generic.o > > +obj-$(CONFIG_UIO_PCI_DMEM_GENIRQ) += uio_pci_dmem_genirq.o > > obj-$(CONFIG_UIO_NETX) += uio_netx.o > > obj-$(CONFIG_UIO_PRUSS) += uio_pruss.o > > obj-$(CONFIG_UIO_MF624) += uio_mf624.o > > diff --git a/drivers/uio/uio_pci_dmem_genirq.c > > b/drivers/uio/uio_pci_dmem_genirq.c > > new file mode 100644 > > index ..be1bdcc552fe > > --- /dev/null > > +++ b/drivers/uio/uio_pci_dmem_genirq.c > > @@ -0,0 +1,351 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* uio_pci_generic - generic UIO driver for PCI 2.3 devices with DMA memory > > + * > > + * Copyright (C) 2016 Fraunhofer IIS > > + * Author: Manuel Stahl > > + * > > + * Based on uio_pci_generic.c by Michael S. Tsirkin > > + * and uio_dmem_genirq.c by Damian Hobson-Garcia. > > + * > > + * Since the driver does not declare any device ids, you must allocate > > + * id and bind the device to the driver yourself. For example: > > + * > > + * # echo "8086 10f5" > /sys/bus/pci/drivers/uio_pci_dmem_genirq/new_id > > + * # echo -n :00:19.0 > /sys/bus/pci/drivers/e1000e/unbind > > + * # echo -n :00:19.0 > /sys/bus/pci/drivers/uio_pci_dmem_genirq/bind > > + * # ls -l /sys/bus/pci/devices/:00:19.0/driver > > + * .../:00:19.0/driver -> ../../../bus/pci/drivers/uio_pci_dmem_genirq > > + * > > + * Or use a modprobe alias: > > + * # alias pci:v10EEd1000sv*sd*sc*i* uio_pci_dmem_genirq > > + * > > + * Driver won't bind to devices which do not support the Interrupt Disable > > Bit > > + * in the command register. All devices compliant to PCI 2.3 (circa 2002) > > and > > + * all compliant PCI Express devices
Re: [PATCH v4] Add new uio device for PCI with dynamic memory allocation
On Thu, Apr 16, 2020 at 06:38:30PM +0200, Manuel Stahl wrote: > This device combines the uio_pci_generic driver and the uio_dmem_genirq > driver since PCI uses a slightly different API for interrupts. > A fixed number of DMA capable memory regions can be defined using the > module parameter "dmem_sizes". The memory is not allocated until the uio > device file is opened for the first time. When the device file is closed, > the allocated memory block is freed. Physical (DMA) addresses for the > dynamic regions are provided to the userspace via > /sys/class/uio/uioX/maps/mapY/addr > When no processes are holding the device file open, the address returned > to userspace is DMA_ERROR_CODE. > > Signed-off-by: Manuel Stahl > --- > MAINTAINERS | 6 + > drivers/uio/Kconfig | 9 + > drivers/uio/Makefile | 1 + > drivers/uio/uio_pci_dmem_genirq.c | 351 ++ > 4 files changed, 367 insertions(+) > create mode 100644 drivers/uio/uio_pci_dmem_genirq.c What changed from previous versions? Always put that below the --- line. > > diff --git a/MAINTAINERS b/MAINTAINERS > index e64e5db31497..446931530dbc 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -7149,6 +7149,12 @@ L: k...@vger.kernel.org > S: Supported > F: drivers/uio/uio_pci_generic.c > > +GENERIC UIO DRIVER FOR PCI DEVICES WITH DMA > +M: "Manuel Stahl" > +L: k...@vger.kernel.org > +S: Supported > +F: drivers/uio/uio_pci_dmem_genirq.c > + > GENERIC VDSO LIBRARY > M: Andy Lutomirski > M: Thomas Gleixner > diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig > index 202ee81cfc2b..0d3f8a01ec74 100644 > --- a/drivers/uio/Kconfig > +++ b/drivers/uio/Kconfig > @@ -94,6 +94,15 @@ config UIO_PCI_GENERIC > primarily, for virtualization scenarios. > If you compile this as a module, it will be called uio_pci_generic. > > +config UIO_PCI_DMEM_GENIRQ > + tristate "Generic driver for PCI 2.3 and PCI Express cards with DMA" > + depends on PCI > + help > + Generic driver that you can bind, dynamically, to any > + PCI 2.3 compliant and PCI Express card. It is useful > + for FPGAs with DMA capability connected via PCI. > + If you compile this as a module, it will be called > uio_pci_dmem_genirq. > + > config UIO_NETX > tristate "Hilscher NetX Card driver" > depends on PCI > diff --git a/drivers/uio/Makefile b/drivers/uio/Makefile > index c285dd2a4539..202d6bfdd5aa 100644 > --- a/drivers/uio/Makefile > +++ b/drivers/uio/Makefile > @@ -6,6 +6,7 @@ obj-$(CONFIG_UIO_DMEM_GENIRQ) += uio_dmem_genirq.o > obj-$(CONFIG_UIO_AEC)+= uio_aec.o > obj-$(CONFIG_UIO_SERCOS3)+= uio_sercos3.o > obj-$(CONFIG_UIO_PCI_GENERIC)+= uio_pci_generic.o > +obj-$(CONFIG_UIO_PCI_DMEM_GENIRQ)+= uio_pci_dmem_genirq.o > obj-$(CONFIG_UIO_NETX) += uio_netx.o > obj-$(CONFIG_UIO_PRUSS) += uio_pruss.o > obj-$(CONFIG_UIO_MF624) += uio_mf624.o > diff --git a/drivers/uio/uio_pci_dmem_genirq.c > b/drivers/uio/uio_pci_dmem_genirq.c > new file mode 100644 > index ..be1bdcc552fe > --- /dev/null > +++ b/drivers/uio/uio_pci_dmem_genirq.c > @@ -0,0 +1,351 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* uio_pci_generic - generic UIO driver for PCI 2.3 devices with DMA memory > + * > + * Copyright (C) 2016 Fraunhofer IIS > + * Author: Manuel Stahl > + * > + * Based on uio_pci_generic.c by Michael S. Tsirkin > + * and uio_dmem_genirq.c by Damian Hobson-Garcia. > + * > + * Since the driver does not declare any device ids, you must allocate > + * id and bind the device to the driver yourself. For example: > + * > + * # echo "8086 10f5" > /sys/bus/pci/drivers/uio_pci_dmem_genirq/new_id > + * # echo -n :00:19.0 > /sys/bus/pci/drivers/e1000e/unbind > + * # echo -n :00:19.0 > /sys/bus/pci/drivers/uio_pci_dmem_genirq/bind > + * # ls -l /sys/bus/pci/devices/:00:19.0/driver > + * .../:00:19.0/driver -> ../../../bus/pci/drivers/uio_pci_dmem_genirq > + * > + * Or use a modprobe alias: > + * # alias pci:v10EEd1000sv*sd*sc*i* uio_pci_dmem_genirq > + * > + * Driver won't bind to devices which do not support the Interrupt Disable > Bit > + * in the command register. All devices compliant to PCI 2.3 (circa 2002) and > + * all compliant PCI Express devices should support this bit. > + * > + * The DMA mask bits and sizes of dynamic regions are derived from module > + * parameters. > + * > + * The format for specifying dynamic region sizes in module parameters > + * is as follows: > + * > + * uio_pci_dmem_genirq.dmem_sizes := > [;] > + *:= :[,] > + *:= : > + * := standard linux memsize > + * > + * Examples: > + * > + * 1) UIO dmem device with 3 dynamic regions: > + * uio_pci_dmem_genirq.dmem_sizes=8086:10f5:4K,16K,4M > + * > + * 2) Two UIO dmem devices with different number of dynamic regions: > + *
