Re: [PATCH] uio_pci_generic does not export memory resources

Hans J. Koch
2012-06-08T16:08:09Z

On Fri, Jun 08, 2012 at 01:56:56PM +0200, Dominic Eschweiler wrote:
> Hello,
>
> the current version of the uio_pci_generic module does not export memory
> resources, such as BARs. As far as I can see, the related module does
> only support interrupts, which is not really useful. My suggestion in
> this case would be to either fix this issue, or to completely remove it.
> The latter would not be an option for us, since we need this
> functionality at some stuff at CERN.
>
> Therefore, here is a patch that fixes the issue. Please give me further
> advice, since I'm doing this the first time ...
>
>
>
> Signed-off-by: Dominic Eschweiler <eschweiler@fias.uni-frankfurt.de>
> ---
> diff -uNr linux-3.4_old/drivers/uio/uio_pci_generic.c
> linux-3.4_new/drivers/uio/uio_pci_generic.c
> --- linux-3.4_old/drivers/uio/uio_pci_generic.c 2012-05-21
> 00:29:13.000000000 +0200
> +++ linux-3.4_new/drivers/uio/uio_pci_generic.c 2012-06-08
> 13:01:12.000000000 +0200
> @@ -25,10 +25,12 @@
> #include <linux/slab.h>
> #include <linux/uio_driver.h>
>
> -#define DRIVER_VERSION "0.01.0"
> +#define DRIVER_VERSION "0.02.0"
> #define DRIVER_AUTHOR "Michael S. Tsirkin <mst@redhat.com>"
> #define DRIVER_DESC "Generic UIO driver for PCI 2.3 devices"
>
> +#define DRV_NAME "uio_pci_generic"
> +
> struct uio_pci_generic_dev {
> struct uio_info info;
> struct pci_dev *pdev;
> @@ -58,6 +60,7 @@
> {
> struct uio_pci_generic_dev *gdev;
> int err;
> + int i;
>
> err = pci_enable_device(pdev);
> if (err) {
> @@ -66,9 +69,33 @@
> return err;
> }
>
> + /* set master */
> + pci_set_master(pdev);
> +
> + /* set DMA mask */
> + err = pci_set_dma_mask(pdev, DMA_BIT_MASK(64));
> + if (err) {
> + dev_warn(&pdev->dev, "Warning: couldn't set 64-bit PCI DMA mask.\n");
> + err = pci_set_dma_mask(pdev, DMA_BIT_MASK(32));
> + if (err) {
> + dev_err(&pdev->dev, "Can't set PCI DMA mask, aborting\n");
> + return -ENODEV;
> + }
> + }
> +
> + /* set consistent DMA mask */
> + err = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(64));
> + if (err) {
> + dev_warn(&pdev->dev, "Warning: couldn't set 64-bit consistent PCI DMA
> mask.\n");
> + err = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(32));
> + if (err) {
> + dev_err(&pdev->dev, "Can't set consistent PCI DMA mask, aborting
> \n");
> + return -ENODEV;
> + }
> + }
> +

All this DMA stuff doesn't fit into a "uio_pci_generic" driver. There might
be users who need other kinds of DMA handling.

If you need this, please make it a new driver and name it after your device.

> if (!pdev->irq) {
> - dev_warn(&pdev->dev, "No IRQ assigned to device: "
> - "no support for interrupts?\n");
> + dev_warn(&pdev->dev, "No IRQ assigned to device: no support for
> interrupts?\n");
> pci_disable_device(pdev);
> return -ENODEV;
> }
> @@ -91,10 +118,40 @@
> gdev->info.handler = irqhandler;
> gdev->pdev = pdev;
>
> + /* request regions */
> + err = pci_request_regions(pdev, DRV_NAME);
> + if (err) {
> + dev_err(&pdev->dev, "Couldn't get PCI resources, aborting\n");
> + return err;
> + }
> +
> + /* create attributes for BAR mappings */
> + for (i = 0; i < PCI_NUM_RESOURCES; i++) {
> + if (pdev->resource[i].flags &&
> + (pdev->resource[i].flags & IORESOURCE_IO)) {
> + gdev->info.port[i].size = 0;
> + gdev->info.port[i].porttype = UIO_PORT_OTHER;
> + #ifdef CONFIG_X86
> + gdev->info.port[i].porttype = UIO_PORT_X86;
> + #endif
> + }

Do you really have x86 ports on your PCI card?

> +
> + if (pdev->resource[i].flags &&
> + (pdev->resource[i].flags & IORESOURCE_MEM)) {
> + gdev->info.mem[i].addr = pci_resource_start(pdev, i);
> + gdev->info.mem[i].size = pci_resource_len(pdev, i);
> + gdev->info.mem[i].internal_addr = NULL;
> + gdev->info.mem[i].memtype = UIO_MEM_PHYS;
> + }
> + }

As Michael said, why don't you just use the existing PCI sysfs files?

I don't really object to your approach, but I'd like to hear a reason
why you can't live with the existing possibilities.

Thanks,
Hans