Re: UIO: missing resource mapping

["Hans J. Koch" <hjk@hansjkoch.de>]

On Wed, Jul 18, 2012 at 12:40:47PM +0200, Dominic Eschweiler wrote:
> Am Montag, den 16.07.2012, 23:58 +0200 schrieb Hans J. Koch:
> > Try to hack up a patch to add generic BAR mapping to uio_pci_generic.c
> > and post it for review.
> > 
> 
> Here we go ...

Thank you very much for your work. I'm really sorry for the long delay,
but I was busy finishing a project because I go to vacation tomorrow.
Sorry, that might cause further delay since I don't know yet how often
I can read my mail...
Greg, can you review the next one?

Here's a first review.

Thanks,
Hans

> > 
> Signed-off-by: Dominic Eschweiler <eschweiler@fias.uni-frankfurt.de>
> diff --git a/drivers/uio/uio_pci_generic.c
> b/drivers/uio/uio_pci_generic.c
> index 0bd08ef..e25991e 100644
> --- a/drivers/uio/uio_pci_generic.c
> +++ b/drivers/uio/uio_pci_generic.c
> @@ -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 @@ static int __devinit probe(struct pci_dev *pdev,
>  {
>  	struct uio_pci_generic_dev *gdev;
>  	int err;
> +	int i;
>  
>  	err = pci_enable_device(pdev);
>  	if (err) {
> @@ -67,8 +70,7 @@ static int __devinit probe(struct pci_dev *pdev,
>  	}
>  
>  	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");

Please configure your mail client not to break lines when sending a patch.
It can't be applied like this.

Why did you make that change anyway? If it's just coding style, please send
another patch, don't mix functional changes with coding style fixes.

>  		pci_disable_device(pdev);
>  		return -ENODEV;
>  	}
> @@ -91,10 +93,31 @@ static int __devinit probe(struct pci_dev *pdev,
>  	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_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;
> +		}
> +	}
> +
>  	if (uio_register_device(&pdev->dev, &gdev->info))
>  		goto err_register;
>  	pci_set_drvdata(pdev, gdev);
>  
> +	pr_info("UIO_PCI_GENERIC : initialized new device (%x %x)\n",

Please use dev_info()

> +	pdev->vendor, pdev->device);
> +
>  	return 0;
>  err_register:
>  	kfree(gdev);
> @@ -107,17 +130,21 @@ err_verify:
>  static void remove(struct pci_dev *pdev)
>  {
>  	struct uio_pci_generic_dev *gdev = pci_get_drvdata(pdev);
> -
>  	uio_unregister_device(&gdev->info);
> +
> +	pci_release_regions(pdev);
>  	pci_disable_device(pdev);
>  	kfree(gdev);
> +
> +	pr_info("UIO_PCI_GENERIC : removed device (%x %x)\n",

ditto

> +	pdev->vendor, pdev->device);
>  }
>  
>  static struct pci_driver driver = {
> -	.name = "uio_pci_generic",
> +	.name     = DRV_NAME,
>  	.id_table = NULL, /* only dynamic id's */
> -	.probe = probe,
> -	.remove = remove,
> +	.probe    = probe,
> +	.remove   = remove,

As above: Please put coding style fixes in an extra patch (if you really
insist on tabs instead of spaces...)

>  };
>  
>  static int __init init(void)
> 
> -- 
> Gruß
>   Dominic
> 
> Frankfurt Institute for Advanced Studies (FIAS)
> Ruth-Moufang-Straße 1
> D-60438 Frankfurt am Main
> Germany
> 
> Phone:  +49 69 79844114
> 
>