* [PATCH] es1371 pci fix/cleanup
@ 2001-04-23 15:51 Marcus Meissner
2001-04-23 15:58 ` Jeff Garzik
0 siblings, 1 reply; 4+ messages in thread
From: Marcus Meissner @ 2001-04-23 15:51 UTC (permalink / raw)
To: Alan Cox, linux-kernel
Hi,
This moves pci_enable_device in the es1371 driver before any resource
access and also replaces the RSRCISIOREGION by just pci_resource_flags
as suggested by Jeff.
Tested and verified.
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 15:49:15
@@ -2771,9 +2771,6 @@
{ 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;
@@ -2783,8 +2780,11 @@
signed long tmo2;
unsigned int cssr;
- if (!RSRCISIOREGION(pcidev, 0))
+ if (pci_enable_device(pcidev))
return -1;
+
+ if (!(pci_resource_flags(pcidev, 0) & IORESOURCE_IO))
+ return -1;
if (pcidev->irq == 0)
return -1;
i = pci_set_dma_mask(pcidev, 0xffffffff);
@@ -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;
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] es1371 pci fix/cleanup
2001-04-23 15:51 [PATCH] es1371 pci fix/cleanup Marcus Meissner
@ 2001-04-23 15:58 ` Jeff Garzik
2001-04-23 16:06 ` Marcus Meissner
0 siblings, 1 reply; 4+ messages in thread
From: Jeff Garzik @ 2001-04-23 15:58 UTC (permalink / raw)
To: Marcus Meissner; +Cc: Alan Cox, linux-kernel
Marcus Meissner wrote:
>
> Hi,
>
> This moves pci_enable_device in the es1371 driver before any resource
> access and also replaces the RSRCISIOREGION by just pci_resource_flags
> as suggested by Jeff.
>
> Tested and verified.
>
> 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 15:49:15
> @@ -2771,9 +2771,6 @@
> { 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;
> @@ -2783,8 +2780,11 @@
> signed long tmo2;
> unsigned int cssr;
>
> - if (!RSRCISIOREGION(pcidev, 0))
> + if (pci_enable_device(pcidev))
> return -1;
> +
> + if (!(pci_resource_flags(pcidev, 0) & IORESOURCE_IO))
> + return -1;
> if (pcidev->irq == 0)
> return -1;
Looks ok except error returns.
pci_enable_device - obtain its return value, and return that.
no IORESOURCE_IO or pcidev->irq==0 - I guess -ENODEV would be
appropriate. (basically look at errno.h and make a judgement call which
error best fits the situation)
--
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)
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] es1371 pci fix/cleanup
2001-04-23 15:58 ` Jeff Garzik
@ 2001-04-23 16:06 ` Marcus Meissner
2001-04-23 16:11 ` Jeff Garzik
0 siblings, 1 reply; 4+ messages in thread
From: Marcus Meissner @ 2001-04-23 16:06 UTC (permalink / raw)
To: Jeff Garzik; +Cc: Marcus Meissner, Alan Cox, linux-kernel
On Mon, Apr 23, 2001 at 11:58:25AM -0400, Jeff Garzik wrote:
> Marcus Meissner wrote:
> >
> > Hi,
> >
> > This moves pci_enable_device in the es1371 driver before any resource
> > access and also replaces the RSRCISIOREGION by just pci_resource_flags
> > as suggested by Jeff.
> >
> > Tested and verified.
> Looks ok except error returns.
>
> pci_enable_device - obtain its return value, and return that.
>
> no IORESOURCE_IO or pcidev->irq==0 - I guess -ENODEV would be
> appropriate. (basically look at errno.h and make a judgement call which
> error best fits the situation)
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?
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);
@@ -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)
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] es1371 pci fix/cleanup
2001-04-23 16:06 ` Marcus Meissner
@ 2001-04-23 16:11 ` Jeff Garzik
0 siblings, 0 replies; 4+ messages in thread
From: Jeff Garzik @ 2001-04-23 16:11 UTC (permalink / raw)
To: Marcus Meissner; +Cc: Alan Cox, linux-kernel
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)
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2001-04-23 16:12 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2001-04-23 15:51 [PATCH] es1371 pci fix/cleanup Marcus Meissner
2001-04-23 15:58 ` Jeff Garzik
2001-04-23 16:06 ` Marcus Meissner
2001-04-23 16:11 ` Jeff Garzik
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox