Re: [PATCH] es1371 pci fix/cleanup

[Jeff Garzik]

Marcus Meissner wrote:
> Hmm, I think I spotted all places in the probe function. I also return
> -ENODEV in case we can't request_region() or request_irq().
> 
> Some drivers use EBUSY, some ENOMEM, some ENODEV there, is there
> any standard return value?

request_region/request_mem_region - EBUSY
request_irq - return its return value

> Ciao, Marcus
> 
> Index: drivers/sound/es1371.c
> ===================================================================
> RCS file: /build/mm/work/repository/linux-mm/drivers/sound/es1371.c,v
> retrieving revision 1.7
> diff -u -r1.7 es1371.c
> --- drivers/sound/es1371.c      2001/04/17 17:26:05     1.7
> +++ drivers/sound/es1371.c      2001/04/23 16:03:34
> @@ -2771,22 +2771,22 @@
>         { SOUND_MIXER_WRITE_IGAIN, 0x4040 }
>  };
> 
> -#define RSRCISIOREGION(dev,num) (pci_resource_start((dev), (num)) != 0 && \
> -                                (pci_resource_flags((dev), (num)) & IORESOURCE_IO))
> -
>  static int __devinit es1371_probe(struct pci_dev *pcidev, const struct pci_device_id *pciid)
>  {
>         struct es1371_state *s;
>         mm_segment_t fs;
> -       int i, val;
> +       int i, val, res = -1;
>         unsigned long tmo;
>         signed long tmo2;
>         unsigned int cssr;
> +
> +       if ((res=pci_enable_device(pcidev)))
> +               return res;
> 
> -       if (!RSRCISIOREGION(pcidev, 0))
> -               return -1;
> +       if (!(pci_resource_flags(pcidev, 0) & IORESOURCE_IO))
> +               return -ENODEV;
>         if (pcidev->irq == 0)
> -               return -1;
> +               return -ENODEV;
>         i = pci_set_dma_mask(pcidev, 0xffffffff);
>         if (i) {
>                 printk(KERN_WARNING "es1371: architecture does not support 32bit PCI busmaster DMA\n");
> @@ -2794,7 +2794,7 @@
>         }
>         if (!(s = kmalloc(sizeof(struct es1371_state), GFP_KERNEL))) {
>                 printk(KERN_WARNING PFX "out of memory\n");
> -               return -1;
> +               return -ENOMEM;
>         }
>         memset(s, 0, sizeof(struct es1371_state));
>         init_waitqueue_head(&s->dma_adc.wait);

this part looks ok

> @@ -2822,8 +2822,6 @@
>                 printk(KERN_ERR PFX "io ports %#lx-%#lx in use\n", s->io, s->io+ES1371_EXTENT-1);
>                 goto err_region;
>         }
> -       if (pci_enable_device(pcidev))
> -               goto err_irq;
>         if (request_irq(s->irq, es1371_interrupt, SA_SHIRQ, "es1371", s)) {
>                 printk(KERN_ERR PFX "irq %u in use\n", s->irq);
>                 goto err_irq;
> @@ -2964,7 +2962,7 @@
>         release_region(s->io, ES1371_EXTENT);
>   err_region:
>         kfree(s);
> -       return -1;
> +       return -ENODEV;
>  }
> 
>  static void __devinit es1371_remove(struct pci_dev *dev)

Since you need to propagate return values from different situations,
have an 'err' variable.  Right before each goto err_foo statement, set
err=Exxxxx.  Then, at the very end of the function, return the value of
'err'.  For a request_region failure, set err to EBUSY before calling
goto.  For a request_irq failure, have 'err' take the return value of
request_irq.

	Jeff

-- 
Jeff Garzik      | The difference between America and England is that
Building 1024    | the English think 100 miles is a long distance and
MandrakeSoft     | the Americans think 100 years is a long time.
                 |      (random fortune)