* [PATCH 1/9] drivers/scsi/atp870u.c: add missing kfree
@ 2011-08-08 11:17 Julia Lawall
2011-08-08 18:12 ` Dan Carpenter
0 siblings, 1 reply; 5+ messages in thread
From: Julia Lawall @ 2011-08-08 11:17 UTC (permalink / raw)
To: James E.J. Bottomley; +Cc: kernel-janitors, linux-scsi, linux-kernel
From: Julia Lawall <julia@diku.dk>
Atpdev is only used as a local buffer, so it should be freed in both the
normal exist case and in all error handling code.
The initial comment is also incorrect - non-zero is returned on failure.
This patch also required a lot of cleanups to satisfy checkpatch.
A simplified version of the semantic match that finds this problem is as
follows: (http://coccinelle.lip6.fr/)
// <smpl>
@exists@
local idexpression x;
statement S,S1;
expression E;
identifier fl;
expression *ptr != NULL;
@@
x = \(kmalloc\|kzalloc\|kcalloc\)(...);
...
if (x == NULL) S
<... when != x
when != if (...) { <+...kfree(x)...+> }
when any
when != true x == NULL
x->fl
...>
(
if (x == NULL) S1
|
if (...) { ... when != x
when forall
(
return \(0\|<+...x...+>\|ptr\);
|
* return ...;
)
}
)
// </smpl>
Signed-off-by: Julia Lawall <julia@diku.dk>
---
-1 is probably not the best return value either.
drivers/scsi/atp870u.c | 113 +++++++++++++++++++++++++++++--------------------
1 file changed, 69 insertions(+), 44 deletions(-)
diff --git a/drivers/scsi/atp870u.c b/drivers/scsi/atp870u.c
index 7e6eca4..271211c 100644
--- a/drivers/scsi/atp870u.c
+++ b/drivers/scsi/atp870u.c
@@ -2552,7 +2552,7 @@ static int atp870u_init_tables(struct Scsi_Host *host)
return 0;
}
-/* return non-zero on detection */
+/* return zero on detection */
static int atp870u_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
{
unsigned char k, m, c;
@@ -2563,19 +2563,23 @@ static int atp870u_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
struct atp_unit *atpdev, *p;
unsigned char setupdata[2][16];
int count = 0;
+ int ret = 0;
atpdev = kzalloc(sizeof(*atpdev), GFP_KERNEL);
if (!atpdev)
return -ENOMEM;
- if (pci_enable_device(pdev))
- goto err_eio;
+ if (pci_enable_device(pdev)) {
+ ret = -EIO;
+ goto out;
+ }
if (!pci_set_dma_mask(pdev, DMA_BIT_MASK(32))) {
printk(KERN_INFO "atp870u: use 32bit DMA mask.\n");
} else {
printk(KERN_ERR "atp870u: DMA mask required but not available.\n");
- goto err_eio;
+ ret = -EIO;
+ goto out;
}
/*
@@ -2584,8 +2588,10 @@ static int atp870u_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
*/
if (ent->device == PCI_DEVICE_ID_ARTOP_AEC7610) {
error = pci_read_config_byte(pdev, PCI_CLASS_REVISION, &atpdev->chip_ver);
- if (atpdev->chip_ver < 2)
- goto err_eio;
+ if (atpdev->chip_ver < 2) {
+ ret = -EIO;
+ goto out;
+ }
}
switch (ent->device) {
@@ -2675,8 +2681,10 @@ flash_ok_880:
outb(atpdev->global_map[0], base_io + 0x35);
shpnt = scsi_host_alloc(&atp870u_template, sizeof(struct atp_unit));
- if (!shpnt)
- goto err_nomem;
+ if (!shpnt) {
+ ret = -ENOMEM;
+ goto out;
+ }
p = (struct atp_unit *)&shpnt->hostdata;
@@ -2686,11 +2694,13 @@ flash_ok_880:
memcpy(p, atpdev, sizeof(*atpdev));
if (atp870u_init_tables(shpnt) < 0) {
printk(KERN_ERR "Unable to allocate tables for Acard controller\n");
+ ret = -1;
goto unregister;
}
if (request_irq(pdev->irq, atp870u_intr_handle, IRQF_SHARED, "atp880i", shpnt)) {
printk(KERN_ERR "Unable to allocate IRQ%d for Acard controller.\n", pdev->irq);
+ ret = -1;
goto free_tables;
}
@@ -2745,8 +2755,10 @@ flash_ok_880:
atpdev->pciport[1] = base_io + 0x50;
shpnt = scsi_host_alloc(&atp870u_template, sizeof(struct atp_unit));
- if (!shpnt)
- goto err_nomem;
+ if (!shpnt) {
+ ret = -ENOMEM;
+ goto out;
+ }
p = (struct atp_unit *)&shpnt->hostdata;
@@ -2754,14 +2766,17 @@ flash_ok_880:
atpdev->pdev = pdev;
pci_set_drvdata(pdev, p);
memcpy(p, atpdev, sizeof(struct atp_unit));
- if (atp870u_init_tables(shpnt) < 0)
+ if (atp870u_init_tables(shpnt) < 0) {
+ ret = -1;
goto unregister;
+ }
#ifdef ED_DBGP
printk("request_irq() shpnt %p hostdata %p\n", shpnt, p);
#endif
if (request_irq(pdev->irq, atp870u_intr_handle, IRQF_SHARED, "atp870u", shpnt)) {
- printk(KERN_ERR "Unable to allocate IRQ for Acard controller.\n");
+ printk(KERN_ERR "Unable to allocate IRQ for Acard controller.\n");
+ ret = -1;
goto free_tables;
}
@@ -2772,13 +2787,11 @@ flash_ok_880:
n=0x1f80;
next_fblk_885:
- if (n >= 0x2000) {
- goto flash_ok_885;
- }
+ if (n >= 0x2000)
+ goto flash_ok_885;
outw(n,base_io + 0x3c);
- if (inl(base_io + 0x38) == 0xffffffff) {
- goto flash_ok_885;
- }
+ if (inl(base_io + 0x38) == 0xffffffff)
+ goto flash_ok_885;
for (m=0; m < 2; m++) {
p->global_map[m]= 0;
for (k=0; k < 4; k++) {
@@ -2930,8 +2943,10 @@ flash_ok_885:
}
shpnt = scsi_host_alloc(&atp870u_template, sizeof(struct atp_unit));
- if (!shpnt)
- goto err_nomem;
+ if (!shpnt) {
+ ret = -ENOMEM;
+ goto out;
+ }
p = (struct atp_unit *)&shpnt->hostdata;
@@ -2940,10 +2955,12 @@ flash_ok_885:
pci_set_drvdata(pdev, p);
memcpy(p, atpdev, sizeof(*atpdev));
if (atp870u_init_tables(shpnt) < 0)
+ ret = -1;
goto unregister;
if (request_irq(pdev->irq, atp870u_intr_handle, IRQF_SHARED, "atp870i", shpnt)) {
printk(KERN_ERR "Unable to allocate IRQ%d for Acard controller.\n", pdev->irq);
+ ret = -1;
goto free_tables;
}
@@ -2991,26 +3008,38 @@ flash_ok_885:
shpnt->io_port = base_io;
shpnt->n_io_port = 0x40; /* Number of bytes of I/O space used */
shpnt->irq = pdev->irq;
- }
- spin_unlock_irqrestore(shpnt->host_lock, flags);
- if(ent->device==ATP885_DEVID) {
- if(!request_region(base_io, 0xff, "atp870u")) /* Register the IO ports that we use */
- goto request_io_fail;
- } else if((ent->device==ATP880_DEVID1)||(ent->device==ATP880_DEVID2)) {
- if(!request_region(base_io, 0x60, "atp870u")) /* Register the IO ports that we use */
- goto request_io_fail;
- } else {
- if(!request_region(base_io, 0x40, "atp870u")) /* Register the IO ports that we use */
- goto request_io_fail;
- }
- count++;
- if (scsi_add_host(shpnt, &pdev->dev))
- goto scsi_add_fail;
- scsi_scan_host(shpnt);
+ }
+ spin_unlock_irqrestore(shpnt->host_lock, flags);
+ if (ent->device == ATP885_DEVID) {
+ /* Register the IO ports that we use */
+ if (!request_region(base_io, 0xff, "atp870u")) {
+ ret = -1;
+ goto request_io_fail;
+ }
+ } else if ((ent->device == ATP880_DEVID1) ||
+ (ent->device == ATP880_DEVID2)) {
+ /* Register the IO ports that we use */
+ if (!request_region(base_io, 0x60, "atp870u")) {
+ ret = -1;
+ goto request_io_fail;
+ }
+ } else {
+ /* Register the IO ports that we use */
+ if (!request_region(base_io, 0x40, "atp870u")) {
+ ret = -1;
+ goto request_io_fail;
+ }
+ }
+ count++;
+ if (scsi_add_host(shpnt, &pdev->dev)) {
+ ret = -1;
+ goto scsi_add_fail;
+ }
+ scsi_scan_host(shpnt);
#ifdef ED_DBGP
- printk("atp870u_prob : exit\n");
+ printk(KERN_DEBUG "atp870u_prob : exit\n");
#endif
- return 0;
+ goto out;
scsi_add_fail:
printk("atp870u_prob:scsi_add_fail\n");
@@ -3030,13 +3059,9 @@ free_tables:
unregister:
printk("atp870u_prob:unregister\n");
scsi_host_put(shpnt);
- return -1;
-err_eio:
- kfree(atpdev);
- return -EIO;
-err_nomem:
+out:
kfree(atpdev);
- return -ENOMEM;
+ return ret;
}
/* The abort command does not leave the device in a clean state where
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/9] drivers/scsi/atp870u.c: add missing kfree
2011-08-08 11:17 [PATCH 1/9] drivers/scsi/atp870u.c: add missing kfree Julia Lawall
@ 2011-08-08 18:12 ` Dan Carpenter
2011-08-08 18:37 ` Julia Lawall
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2011-08-08 18:12 UTC (permalink / raw)
To: Julia Lawall
Cc: James E.J. Bottomley, kernel-janitors, linux-scsi, linux-kernel
This one is going to need to be redone. There are some parenthesis
missing so the new code will always fail.
> -1 is probably not the best return value either.
Nope. It's not. You could avoid it most of the time by passing the
error code from the lower levels, as I show below.
>
> drivers/scsi/atp870u.c | 113 +++++++++++++++++++++++++++++--------------------
> 1 file changed, 69 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/scsi/atp870u.c b/drivers/scsi/atp870u.c
> index 7e6eca4..271211c 100644
> --- a/drivers/scsi/atp870u.c
> +++ b/drivers/scsi/atp870u.c
> @@ -2552,7 +2552,7 @@ static int atp870u_init_tables(struct Scsi_Host *host)
> return 0;
> }
>
> -/* return non-zero on detection */
> +/* return zero on detection */
> static int atp870u_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> {
> unsigned char k, m, c;
> @@ -2563,19 +2563,23 @@ static int atp870u_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> struct atp_unit *atpdev, *p;
> unsigned char setupdata[2][16];
> int count = 0;
> + int ret = 0;
>
> atpdev = kzalloc(sizeof(*atpdev), GFP_KERNEL);
> if (!atpdev)
> return -ENOMEM;
>
> - if (pci_enable_device(pdev))
> - goto err_eio;
> + if (pci_enable_device(pdev)) {
ret = pci_enable_device();
> + ret = -EIO;
> + goto out;
> + }
>
> if (!pci_set_dma_mask(pdev, DMA_BIT_MASK(32))) {
ret = pci_set_dma_mask();
> printk(KERN_INFO "atp870u: use 32bit DMA mask.\n");
> } else {
> printk(KERN_ERR "atp870u: DMA mask required but not available.\n");
> - goto err_eio;
> + ret = -EIO;
> + goto out;
> }
>
> /*
> @@ -2584,8 +2588,10 @@ static int atp870u_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> */
> if (ent->device == PCI_DEVICE_ID_ARTOP_AEC7610) {
> error = pci_read_config_byte(pdev, PCI_CLASS_REVISION, &atpdev->chip_ver);
> - if (atpdev->chip_ver < 2)
> - goto err_eio;
> + if (atpdev->chip_ver < 2) {
> + ret = -EIO;
> + goto out;
> + }
> }
>
> switch (ent->device) {
> @@ -2675,8 +2681,10 @@ flash_ok_880:
> outb(atpdev->global_map[0], base_io + 0x35);
>
> shpnt = scsi_host_alloc(&atp870u_template, sizeof(struct atp_unit));
> - if (!shpnt)
> - goto err_nomem;
> + if (!shpnt) {
> + ret = -ENOMEM;
> + goto out;
> + }
>
> p = (struct atp_unit *)&shpnt->hostdata;
>
> @@ -2686,11 +2694,13 @@ flash_ok_880:
> memcpy(p, atpdev, sizeof(*atpdev));
> if (atp870u_init_tables(shpnt) < 0) {
ret = atp870u_init_tables();
> printk(KERN_ERR "Unable to allocate tables for Acard controller\n");
> + ret = -1;
> goto unregister;
> }
>
> if (request_irq(pdev->irq, atp870u_intr_handle, IRQF_SHARED, "atp880i", shpnt)) {
ret = request_irq();
> printk(KERN_ERR "Unable to allocate IRQ%d for Acard controller.\n", pdev->irq);
> + ret = -1;
> goto free_tables;
> }
>
> @@ -2745,8 +2755,10 @@ flash_ok_880:
> atpdev->pciport[1] = base_io + 0x50;
>
> shpnt = scsi_host_alloc(&atp870u_template, sizeof(struct atp_unit));
> - if (!shpnt)
> - goto err_nomem;
> + if (!shpnt) {
> + ret = -ENOMEM;
> + goto out;
> + }
>
> p = (struct atp_unit *)&shpnt->hostdata;
>
> @@ -2754,14 +2766,17 @@ flash_ok_880:
> atpdev->pdev = pdev;
> pci_set_drvdata(pdev, p);
> memcpy(p, atpdev, sizeof(struct atp_unit));
> - if (atp870u_init_tables(shpnt) < 0)
> + if (atp870u_init_tables(shpnt) < 0) {
ret = atp870u_init_tables();
> + ret = -1;
> goto unregister;
> + }
>
> #ifdef ED_DBGP
> printk("request_irq() shpnt %p hostdata %p\n", shpnt, p);
> #endif
> if (request_irq(pdev->irq, atp870u_intr_handle, IRQF_SHARED, "atp870u", shpnt)) {
ret = request_irq();
> - printk(KERN_ERR "Unable to allocate IRQ for Acard controller.\n");
> + printk(KERN_ERR "Unable to allocate IRQ for Acard controller.\n");
> + ret = -1;
> goto free_tables;
> }
>
> @@ -2772,13 +2787,11 @@ flash_ok_880:
>
> n=0x1f80;
> next_fblk_885:
> - if (n >= 0x2000) {
> - goto flash_ok_885;
> - }
> + if (n >= 0x2000)
> + goto flash_ok_885;
> outw(n,base_io + 0x3c);
> - if (inl(base_io + 0x38) == 0xffffffff) {
> - goto flash_ok_885;
> - }
> + if (inl(base_io + 0x38) == 0xffffffff)
> + goto flash_ok_885;
> for (m=0; m < 2; m++) {
> p->global_map[m]= 0;
> for (k=0; k < 4; k++) {
> @@ -2930,8 +2943,10 @@ flash_ok_885:
> }
>
> shpnt = scsi_host_alloc(&atp870u_template, sizeof(struct atp_unit));
> - if (!shpnt)
> - goto err_nomem;
> + if (!shpnt) {
> + ret = -ENOMEM;
> + goto out;
> + }
>
> p = (struct atp_unit *)&shpnt->hostdata;
>
> @@ -2940,10 +2955,12 @@ flash_ok_885:
> pci_set_drvdata(pdev, p);
> memcpy(p, atpdev, sizeof(*atpdev));
> if (atp870u_init_tables(shpnt) < 0)
ret = atp870u_init_tables()
> + ret = -1;
> goto unregister;
parenthesis needed here.
>
> if (request_irq(pdev->irq, atp870u_intr_handle, IRQF_SHARED, "atp870i", shpnt)) {
ret = request_irq();
> printk(KERN_ERR "Unable to allocate IRQ%d for Acard controller.\n", pdev->irq);
> + ret = -1;
> goto free_tables;
> }
>
> @@ -2991,26 +3008,38 @@ flash_ok_885:
> shpnt->io_port = base_io;
> shpnt->n_io_port = 0x40; /* Number of bytes of I/O space used */
> shpnt->irq = pdev->irq;
> - }
> - spin_unlock_irqrestore(shpnt->host_lock, flags);
> - if(ent->device==ATP885_DEVID) {
> - if(!request_region(base_io, 0xff, "atp870u")) /* Register the IO ports that we use */
> - goto request_io_fail;
> - } else if((ent->device==ATP880_DEVID1)||(ent->device==ATP880_DEVID2)) {
> - if(!request_region(base_io, 0x60, "atp870u")) /* Register the IO ports that we use */
> - goto request_io_fail;
> - } else {
> - if(!request_region(base_io, 0x40, "atp870u")) /* Register the IO ports that we use */
> - goto request_io_fail;
> - }
> - count++;
> - if (scsi_add_host(shpnt, &pdev->dev))
> - goto scsi_add_fail;
> - scsi_scan_host(shpnt);
> + }
> + spin_unlock_irqrestore(shpnt->host_lock, flags);
> + if (ent->device == ATP885_DEVID) {
> + /* Register the IO ports that we use */
> + if (!request_region(base_io, 0xff, "atp870u")) {
> + ret = -1;
If request_region() fails then the correct return is actually
-EBUSY.
> + goto request_io_fail;
> + }
> + } else if ((ent->device == ATP880_DEVID1) ||
> + (ent->device == ATP880_DEVID2)) {
> + /* Register the IO ports that we use */
> + if (!request_region(base_io, 0x60, "atp870u")) {
> + ret = -1;
ret = -EBUSY;
> + goto request_io_fail;
> + }
> + } else {
> + /* Register the IO ports that we use */
> + if (!request_region(base_io, 0x40, "atp870u")) {
> + ret = -1;
ret = -EBUSY;
> + goto request_io_fail;
> + }
> + }
> + count++;
> + if (scsi_add_host(shpnt, &pdev->dev)) {
ret = scsi_add_host();
> + ret = -1;
> + goto scsi_add_fail;
> + }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/9] drivers/scsi/atp870u.c: add missing kfree
2011-08-08 18:12 ` Dan Carpenter
@ 2011-08-08 18:37 ` Julia Lawall
2011-08-11 11:36 ` Julia Lawall
0 siblings, 1 reply; 5+ messages in thread
From: Julia Lawall @ 2011-08-08 18:37 UTC (permalink / raw)
To: Dan Carpenter
Cc: James E.J. Bottomley, kernel-janitors, linux-scsi, linux-kernel
On Mon, 8 Aug 2011, Dan Carpenter wrote:
> This one is going to need to be redone. There are some parenthesis
> missing so the new code will always fail.
>
> > -1 is probably not the best return value either.
>
> Nope. It's not. You could avoid it most of the time by passing the
> error code from the lower levels, as I show below.
OK, I will look into it. Thanks.
julia
> > drivers/scsi/atp870u.c | 113 +++++++++++++++++++++++++++++--------------------
> > 1 file changed, 69 insertions(+), 44 deletions(-)
> >
> > diff --git a/drivers/scsi/atp870u.c b/drivers/scsi/atp870u.c
> > index 7e6eca4..271211c 100644
> > --- a/drivers/scsi/atp870u.c
> > +++ b/drivers/scsi/atp870u.c
> > @@ -2552,7 +2552,7 @@ static int atp870u_init_tables(struct Scsi_Host *host)
> > return 0;
> > }
> >
> > -/* return non-zero on detection */
> > +/* return zero on detection */
> > static int atp870u_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> > {
> > unsigned char k, m, c;
> > @@ -2563,19 +2563,23 @@ static int atp870u_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> > struct atp_unit *atpdev, *p;
> > unsigned char setupdata[2][16];
> > int count = 0;
> > + int ret = 0;
> >
> > atpdev = kzalloc(sizeof(*atpdev), GFP_KERNEL);
> > if (!atpdev)
> > return -ENOMEM;
> >
> > - if (pci_enable_device(pdev))
> > - goto err_eio;
> > + if (pci_enable_device(pdev)) {
>
> ret = pci_enable_device();
>
> > + ret = -EIO;
> > + goto out;
> > + }
> >
> > if (!pci_set_dma_mask(pdev, DMA_BIT_MASK(32))) {
> ret = pci_set_dma_mask();
>
>
> > printk(KERN_INFO "atp870u: use 32bit DMA mask.\n");
> > } else {
> > printk(KERN_ERR "atp870u: DMA mask required but not available.\n");
> > - goto err_eio;
> > + ret = -EIO;
> > + goto out;
> > }
> >
> > /*
> > @@ -2584,8 +2588,10 @@ static int atp870u_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> > */
> > if (ent->device == PCI_DEVICE_ID_ARTOP_AEC7610) {
> > error = pci_read_config_byte(pdev, PCI_CLASS_REVISION, &atpdev->chip_ver);
> > - if (atpdev->chip_ver < 2)
> > - goto err_eio;
> > + if (atpdev->chip_ver < 2) {
> > + ret = -EIO;
> > + goto out;
> > + }
> > }
> >
> > switch (ent->device) {
> > @@ -2675,8 +2681,10 @@ flash_ok_880:
> > outb(atpdev->global_map[0], base_io + 0x35);
> >
> > shpnt = scsi_host_alloc(&atp870u_template, sizeof(struct atp_unit));
> > - if (!shpnt)
> > - goto err_nomem;
> > + if (!shpnt) {
> > + ret = -ENOMEM;
> > + goto out;
> > + }
> >
> > p = (struct atp_unit *)&shpnt->hostdata;
> >
> > @@ -2686,11 +2694,13 @@ flash_ok_880:
> > memcpy(p, atpdev, sizeof(*atpdev));
> > if (atp870u_init_tables(shpnt) < 0) {
>
> ret = atp870u_init_tables();
>
>
> > printk(KERN_ERR "Unable to allocate tables for Acard controller\n");
> > + ret = -1;
> > goto unregister;
> > }
> >
> > if (request_irq(pdev->irq, atp870u_intr_handle, IRQF_SHARED, "atp880i", shpnt)) {
>
> ret = request_irq();
>
>
> > printk(KERN_ERR "Unable to allocate IRQ%d for Acard controller.\n", pdev->irq);
> > + ret = -1;
> > goto free_tables;
> > }
> >
> > @@ -2745,8 +2755,10 @@ flash_ok_880:
> > atpdev->pciport[1] = base_io + 0x50;
> >
> > shpnt = scsi_host_alloc(&atp870u_template, sizeof(struct atp_unit));
> > - if (!shpnt)
> > - goto err_nomem;
> > + if (!shpnt) {
> > + ret = -ENOMEM;
> > + goto out;
> > + }
> >
> > p = (struct atp_unit *)&shpnt->hostdata;
> >
> > @@ -2754,14 +2766,17 @@ flash_ok_880:
> > atpdev->pdev = pdev;
> > pci_set_drvdata(pdev, p);
> > memcpy(p, atpdev, sizeof(struct atp_unit));
> > - if (atp870u_init_tables(shpnt) < 0)
> > + if (atp870u_init_tables(shpnt) < 0) {
>
> ret = atp870u_init_tables();
>
> > + ret = -1;
> > goto unregister;
> > + }
> >
> > #ifdef ED_DBGP
> > printk("request_irq() shpnt %p hostdata %p\n", shpnt, p);
> > #endif
> > if (request_irq(pdev->irq, atp870u_intr_handle, IRQF_SHARED, "atp870u", shpnt)) {
>
> ret = request_irq();
>
> > - printk(KERN_ERR "Unable to allocate IRQ for Acard controller.\n");
> > + printk(KERN_ERR "Unable to allocate IRQ for Acard controller.\n");
> > + ret = -1;
> > goto free_tables;
> > }
> >
> > @@ -2772,13 +2787,11 @@ flash_ok_880:
> >
> > n=0x1f80;
> > next_fblk_885:
> > - if (n >= 0x2000) {
> > - goto flash_ok_885;
> > - }
> > + if (n >= 0x2000)
> > + goto flash_ok_885;
> > outw(n,base_io + 0x3c);
> > - if (inl(base_io + 0x38) == 0xffffffff) {
> > - goto flash_ok_885;
> > - }
> > + if (inl(base_io + 0x38) == 0xffffffff)
> > + goto flash_ok_885;
> > for (m=0; m < 2; m++) {
> > p->global_map[m]= 0;
> > for (k=0; k < 4; k++) {
> > @@ -2930,8 +2943,10 @@ flash_ok_885:
> > }
> >
> > shpnt = scsi_host_alloc(&atp870u_template, sizeof(struct atp_unit));
> > - if (!shpnt)
> > - goto err_nomem;
> > + if (!shpnt) {
> > + ret = -ENOMEM;
> > + goto out;
> > + }
> >
> > p = (struct atp_unit *)&shpnt->hostdata;
> >
> > @@ -2940,10 +2955,12 @@ flash_ok_885:
> > pci_set_drvdata(pdev, p);
> > memcpy(p, atpdev, sizeof(*atpdev));
> > if (atp870u_init_tables(shpnt) < 0)
>
> ret = atp870u_init_tables()
>
> > + ret = -1;
> > goto unregister;
>
> parenthesis needed here.
>
> >
> > if (request_irq(pdev->irq, atp870u_intr_handle, IRQF_SHARED, "atp870i", shpnt)) {
>
> ret = request_irq();
>
> > printk(KERN_ERR "Unable to allocate IRQ%d for Acard controller.\n", pdev->irq);
> > + ret = -1;
> > goto free_tables;
> > }
> >
> > @@ -2991,26 +3008,38 @@ flash_ok_885:
> > shpnt->io_port = base_io;
> > shpnt->n_io_port = 0x40; /* Number of bytes of I/O space used */
> > shpnt->irq = pdev->irq;
> > - }
> > - spin_unlock_irqrestore(shpnt->host_lock, flags);
> > - if(ent->device==ATP885_DEVID) {
> > - if(!request_region(base_io, 0xff, "atp870u")) /* Register the IO ports that we use */
> > - goto request_io_fail;
> > - } else if((ent->device==ATP880_DEVID1)||(ent->device==ATP880_DEVID2)) {
> > - if(!request_region(base_io, 0x60, "atp870u")) /* Register the IO ports that we use */
> > - goto request_io_fail;
> > - } else {
> > - if(!request_region(base_io, 0x40, "atp870u")) /* Register the IO ports that we use */
> > - goto request_io_fail;
> > - }
> > - count++;
> > - if (scsi_add_host(shpnt, &pdev->dev))
> > - goto scsi_add_fail;
> > - scsi_scan_host(shpnt);
> > + }
> > + spin_unlock_irqrestore(shpnt->host_lock, flags);
> > + if (ent->device == ATP885_DEVID) {
> > + /* Register the IO ports that we use */
> > + if (!request_region(base_io, 0xff, "atp870u")) {
> > + ret = -1;
>
> If request_region() fails then the correct return is actually
> -EBUSY.
>
> > + goto request_io_fail;
> > + }
> > + } else if ((ent->device == ATP880_DEVID1) ||
> > + (ent->device == ATP880_DEVID2)) {
> > + /* Register the IO ports that we use */
> > + if (!request_region(base_io, 0x60, "atp870u")) {
> > + ret = -1;
>
> ret = -EBUSY;
>
> > + goto request_io_fail;
> > + }
> > + } else {
> > + /* Register the IO ports that we use */
> > + if (!request_region(base_io, 0x40, "atp870u")) {
> > + ret = -1;
>
> ret = -EBUSY;
>
> > + goto request_io_fail;
> > + }
> > + }
> > + count++;
> > + if (scsi_add_host(shpnt, &pdev->dev)) {
> ret = scsi_add_host();
>
>
> > + ret = -1;
> > + goto scsi_add_fail;
> > + }
>
> regards,
> dan carpenter
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/9] drivers/scsi/atp870u.c: add missing kfree
2011-08-08 18:37 ` Julia Lawall
@ 2011-08-11 11:36 ` Julia Lawall
2011-08-11 11:46 ` Dan Carpenter
0 siblings, 1 reply; 5+ messages in thread
From: Julia Lawall @ 2011-08-11 11:36 UTC (permalink / raw)
To: Dan Carpenter
Cc: James E.J. Bottomley, kernel-janitors, linux-scsi, linux-kernel
From: Julia Lawall <julia@diku.dk>
Atpdev is only used as a local buffer, so it should be freed in both the
normal exist case and in all error handling code.
The initial comment is also incorrect - non-zero is returned on failure.
-1 is no longer used as an error return value, and is replaced by a value
that is somehow related to the cause of the error. -EBUSY is used in the
case of the failure of request_region.
This patch also includes a lot of cleanups to satisfy checkpatch.
A simplified version of the semantic match that finds this problem is as
follows: (http://coccinelle.lip6.fr/)
// <smpl>
@exists@
local idexpression x;
statement S,S1;
expression E;
identifier fl;
expression *ptr != NULL;
@@
x = \(kmalloc\|kzalloc\|kcalloc\)(...);
...
if (x == NULL) S
<... when != x
when != if (...) { <+...kfree(x)...+> }
when any
when != true x == NULL
x->fl
...>
(
if (x == NULL) S1
|
if (...) { ... when != x
when forall
(
return \(0\|<+...x...+>\|ptr\);
|
* return ...;
)
}
)
// </smpl>
Signed-off-by: Julia Lawall <julia@diku.dk>
---
As compared to the previous version, -1 is no longer used as a return value
and some missing braces have been added.
drivers/scsi/atp870u.c | 126 +++++++++++++++++++++++++++++--------------------
1 file changed, 76 insertions(+), 50 deletions(-)
diff --git a/drivers/scsi/atp870u.c b/drivers/scsi/atp870u.c
index 7e6eca4..99b1da1 100644
--- a/drivers/scsi/atp870u.c
+++ b/drivers/scsi/atp870u.c
@@ -2552,7 +2552,7 @@ static int atp870u_init_tables(struct Scsi_Host *host)
return 0;
}
-/* return non-zero on detection */
+/* return zero on detection */
static int atp870u_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
{
unsigned char k, m, c;
@@ -2563,19 +2563,23 @@ static int atp870u_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
struct atp_unit *atpdev, *p;
unsigned char setupdata[2][16];
int count = 0;
+ int ret = 0;
atpdev = kzalloc(sizeof(*atpdev), GFP_KERNEL);
if (!atpdev)
return -ENOMEM;
- if (pci_enable_device(pdev))
- goto err_eio;
+ if (pci_enable_device(pdev)) {
+ ret = -EIO;
+ goto out;
+ }
if (!pci_set_dma_mask(pdev, DMA_BIT_MASK(32))) {
printk(KERN_INFO "atp870u: use 32bit DMA mask.\n");
} else {
printk(KERN_ERR "atp870u: DMA mask required but not available.\n");
- goto err_eio;
+ ret = -EIO;
+ goto out;
}
/*
@@ -2584,8 +2588,10 @@ static int atp870u_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
*/
if (ent->device == PCI_DEVICE_ID_ARTOP_AEC7610) {
error = pci_read_config_byte(pdev, PCI_CLASS_REVISION, &atpdev->chip_ver);
- if (atpdev->chip_ver < 2)
- goto err_eio;
+ if (atpdev->chip_ver < 2) {
+ ret = -EIO;
+ goto out;
+ }
}
switch (ent->device) {
@@ -2675,8 +2681,10 @@ flash_ok_880:
outb(atpdev->global_map[0], base_io + 0x35);
shpnt = scsi_host_alloc(&atp870u_template, sizeof(struct atp_unit));
- if (!shpnt)
- goto err_nomem;
+ if (!shpnt) {
+ ret = -ENOMEM;
+ goto out;
+ }
p = (struct atp_unit *)&shpnt->hostdata;
@@ -2684,12 +2692,15 @@ flash_ok_880:
atpdev->pdev = pdev;
pci_set_drvdata(pdev, p);
memcpy(p, atpdev, sizeof(*atpdev));
- if (atp870u_init_tables(shpnt) < 0) {
+ ret = atp870u_init_tables(shpnt);
+ if (ret < 0) {
printk(KERN_ERR "Unable to allocate tables for Acard controller\n");
goto unregister;
}
- if (request_irq(pdev->irq, atp870u_intr_handle, IRQF_SHARED, "atp880i", shpnt)) {
+ ret = request_irq(pdev->irq, atp870u_intr_handle, IRQF_SHARED,
+ "atp880i", shpnt);
+ if (ret) {
printk(KERN_ERR "Unable to allocate IRQ%d for Acard controller.\n", pdev->irq);
goto free_tables;
}
@@ -2745,8 +2756,10 @@ flash_ok_880:
atpdev->pciport[1] = base_io + 0x50;
shpnt = scsi_host_alloc(&atp870u_template, sizeof(struct atp_unit));
- if (!shpnt)
- goto err_nomem;
+ if (!shpnt) {
+ ret = -ENOMEM;
+ goto out;
+ }
p = (struct atp_unit *)&shpnt->hostdata;
@@ -2754,14 +2767,17 @@ flash_ok_880:
atpdev->pdev = pdev;
pci_set_drvdata(pdev, p);
memcpy(p, atpdev, sizeof(struct atp_unit));
- if (atp870u_init_tables(shpnt) < 0)
+ ret = atp870u_init_tables(shpnt);
+ if (ret < 0)
goto unregister;
#ifdef ED_DBGP
- printk("request_irq() shpnt %p hostdata %p\n", shpnt, p);
+ printk("request_irq() shpnt %p hostdata %p\n", shpnt, p);
#endif
- if (request_irq(pdev->irq, atp870u_intr_handle, IRQF_SHARED, "atp870u", shpnt)) {
- printk(KERN_ERR "Unable to allocate IRQ for Acard controller.\n");
+ ret = request_irq(pdev->irq, atp870u_intr_handle, IRQF_SHARED,
+ "atp870u", shpnt);
+ if (ret) {
+ printk(KERN_ERR "Unable to allocate IRQ for Acard controller.\n");
goto free_tables;
}
@@ -2772,13 +2788,11 @@ flash_ok_880:
n=0x1f80;
next_fblk_885:
- if (n >= 0x2000) {
- goto flash_ok_885;
- }
+ if (n >= 0x2000)
+ goto flash_ok_885;
outw(n,base_io + 0x3c);
- if (inl(base_io + 0x38) == 0xffffffff) {
- goto flash_ok_885;
- }
+ if (inl(base_io + 0x38) == 0xffffffff)
+ goto flash_ok_885;
for (m=0; m < 2; m++) {
p->global_map[m]= 0;
for (k=0; k < 4; k++) {
@@ -2930,8 +2944,10 @@ flash_ok_885:
}
shpnt = scsi_host_alloc(&atp870u_template, sizeof(struct atp_unit));
- if (!shpnt)
- goto err_nomem;
+ if (!shpnt) {
+ ret = -ENOMEM;
+ goto out;
+ }
p = (struct atp_unit *)&shpnt->hostdata;
@@ -2939,10 +2955,13 @@ flash_ok_885:
atpdev->pdev = pdev;
pci_set_drvdata(pdev, p);
memcpy(p, atpdev, sizeof(*atpdev));
- if (atp870u_init_tables(shpnt) < 0)
+ ret = atp870u_init_tables(shpnt);
+ if (ret < 0)
goto unregister;
- if (request_irq(pdev->irq, atp870u_intr_handle, IRQF_SHARED, "atp870i", shpnt)) {
+ ret = request_irq(pdev->irq, atp870u_intr_handle, IRQF_SHARED,
+ "atp870i", shpnt);
+ if (ret) {
printk(KERN_ERR "Unable to allocate IRQ%d for Acard controller.\n", pdev->irq);
goto free_tables;
}
@@ -2991,26 +3010,37 @@ flash_ok_885:
shpnt->io_port = base_io;
shpnt->n_io_port = 0x40; /* Number of bytes of I/O space used */
shpnt->irq = pdev->irq;
- }
- spin_unlock_irqrestore(shpnt->host_lock, flags);
- if(ent->device==ATP885_DEVID) {
- if(!request_region(base_io, 0xff, "atp870u")) /* Register the IO ports that we use */
- goto request_io_fail;
- } else if((ent->device==ATP880_DEVID1)||(ent->device==ATP880_DEVID2)) {
- if(!request_region(base_io, 0x60, "atp870u")) /* Register the IO ports that we use */
- goto request_io_fail;
- } else {
- if(!request_region(base_io, 0x40, "atp870u")) /* Register the IO ports that we use */
- goto request_io_fail;
- }
- count++;
- if (scsi_add_host(shpnt, &pdev->dev))
- goto scsi_add_fail;
- scsi_scan_host(shpnt);
+ }
+ spin_unlock_irqrestore(shpnt->host_lock, flags);
+ if (ent->device == ATP885_DEVID) {
+ /* Register the IO ports that we use */
+ if (!request_region(base_io, 0xff, "atp870u")) {
+ ret = -EBUSY;
+ goto request_io_fail;
+ }
+ } else if ((ent->device == ATP880_DEVID1) ||
+ (ent->device == ATP880_DEVID2)) {
+ /* Register the IO ports that we use */
+ if (!request_region(base_io, 0x60, "atp870u")) {
+ ret = -EBUSY;
+ goto request_io_fail;
+ }
+ } else {
+ /* Register the IO ports that we use */
+ if (!request_region(base_io, 0x40, "atp870u")) {
+ ret = -EBUSY;
+ goto request_io_fail;
+ }
+ }
+ count++;
+ ret = scsi_add_host(shpnt, &pdev->dev);
+ if (ret)
+ goto scsi_add_fail;
+ scsi_scan_host(shpnt);
#ifdef ED_DBGP
- printk("atp870u_prob : exit\n");
+ printk(KERN_DEBUG "atp870u_prob : exit\n");
#endif
- return 0;
+ goto out;
scsi_add_fail:
printk("atp870u_prob:scsi_add_fail\n");
@@ -3030,13 +3060,9 @@ free_tables:
unregister:
printk("atp870u_prob:unregister\n");
scsi_host_put(shpnt);
- return -1;
-err_eio:
- kfree(atpdev);
- return -EIO;
-err_nomem:
+out:
kfree(atpdev);
- return -ENOMEM;
+ return ret;
}
/* The abort command does not leave the device in a clean state where
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/9] drivers/scsi/atp870u.c: add missing kfree
2011-08-11 11:36 ` Julia Lawall
@ 2011-08-11 11:46 ` Dan Carpenter
0 siblings, 0 replies; 5+ messages in thread
From: Dan Carpenter @ 2011-08-11 11:46 UTC (permalink / raw)
To: Julia Lawall
Cc: James E.J. Bottomley, kernel-janitors, linux-scsi, linux-kernel
Reviewed-by: Dan Carpenter <error27@gmail.com>
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2011-08-11 11:48 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-08-08 11:17 [PATCH 1/9] drivers/scsi/atp870u.c: add missing kfree Julia Lawall
2011-08-08 18:12 ` Dan Carpenter
2011-08-08 18:37 ` Julia Lawall
2011-08-11 11:36 ` Julia Lawall
2011-08-11 11:46 ` Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).