Request_region should be used with release_region, not release_resource.
The changes are somewhat complicated, so I have only done a few cases to
start with.
The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@@
expression x,E;
@@
(
*x = request_region(...)
|
*x = request_mem_region(...)
)
... when != release_region(x)
when != x = E
* release_resource(x);
// </smpl>
The complete set of files in which this semantic match finds problesm is as
follows:
arch/unicore32/kernel/rtc.c
arch/x86/pci/direct.c
drivers/char/hw_random/omap-rng.c
drivers/char/pcmcia/ipwireless/main.c
drivers/i2c/busses/i2c-au1550.c
drivers/i2c/busses/i2c-nuc900.c
drivers/i2c/busses/i2c-s3c2410.c
drivers/i2c/busses/i2c-sh7760.c
drivers/i2c/busses/i2c-simtec.c
drivers/media/video/s5p-fimc/fimc-core.c
drivers/mfd/sm501.c
drivers/misc/atmel_tclib.c
drivers/mmc/host/au1xmmc.c
drivers/mmc/host/davinci_mmc.c
drivers/mmc/host/mvsdio.c
drivers/mmc/host/pxamci.c
drivers/mmc/host/sdhci-s3c.c
drivers/mtd/maps/ceiva.c
drivers/net/a2065.c
drivers/net/ariadne.c
drivers/net/arm/ixp4xx_eth.c
drivers/net/ax88796.c
drivers/parport/parport_ax88796.c
drivers/parport/parport_pc.c
drivers/rtc/rtc-s3c.c
drivers/spi/au1550_spi.c
drivers/spi/spi_s3c24xx.c
drivers/tty/serial/sh-sci.c
drivers/tty/serial/vr41xx_siu.c
drivers/usb/gadget/s3c-hsotg.c
drivers/video/s3c-fb.c
drivers/video/s3c2410fb.c
drivers/video/sh7760fb.c
drivers/video/sm501fb.c
drivers/watchdog/davinci_wdt.c
drivers/watchdog/max63xx_wdt.c
drivers/watchdog/pnx4008_wdt.c
drivers/watchdog/s3c2410_wdt.c
Request_region should be used with release_region, not release_resource.
The result of request_mem_region is no longer stored. Instead the field
ioarea is used to store a pointer to the resource structure that contains
the start address. This is the information that is needed later in
i2c_au1550_remove to release the region. The field ioarea is not used for
anything else.
The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@@
expression x,E;
@@
(
*x = request_region(...)
|
*x = request_mem_region(...)
)
... when != release_region(x)
when != x = E
* release_resource(x);
// </smpl>
Signed-off-by: Julia Lawall <[email protected]>
---
Not compiled due to incompatible architecture.
drivers/i2c/busses/i2c-au1550.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/drivers/i2c/busses/i2c-au1550.c b/drivers/i2c/busses/i2c-au1550.c
index 532828b..2ec78d7 100644
--- a/drivers/i2c/busses/i2c-au1550.c
+++ b/drivers/i2c/busses/i2c-au1550.c
@@ -389,13 +389,12 @@ i2c_au1550_probe(struct platform_device *pdev)
goto out;
}
- priv->ioarea = request_mem_region(r->start, resource_size(r),
- pdev->name);
- if (!priv->ioarea) {
+ if (!request_mem_region(r->start, resource_size(r), pdev->name)) {
ret = -EBUSY;
goto out_mem;
}
+ priv->ioarea = r;
priv->psc_base = CKSEG1ADDR(r->start);
priv->xfer_timeout = 200;
priv->ack_timeout = 200;
@@ -418,8 +417,7 @@ i2c_au1550_probe(struct platform_device *pdev)
i2c_au1550_disable(priv);
- release_resource(priv->ioarea);
- kfree(priv->ioarea);
+ release_mem_region(r->start, resource_size(r));
out_mem:
kfree(priv);
out:
@@ -430,12 +428,12 @@ static int __devexit
i2c_au1550_remove(struct platform_device *pdev)
{
struct i2c_au1550_data *priv = platform_get_drvdata(pdev);
+ struct resource *r = priv->ioarea;
platform_set_drvdata(pdev, NULL);
i2c_del_adapter(&priv->adap);
i2c_au1550_disable(priv);
- release_resource(priv->ioarea);
- kfree(priv->ioarea);
+ release_mem_region(r->start, resource_size(r));
kfree(priv);
return 0;
}
Request_region should be used with release_region, not release_resource.
This patch contains a number of changes, related to calls to request_region,
request_mem_region, and the associated error handling code.
1. For the call to request_region, the variable io_resource storing the
result is dropped. The call to release_resource at the end of the function
is changed to a call to release_region with the first two arguments of
request_region as its arguments. The same call to release_region is also
added to release_ipwireless.
2. The first call to request_mem_region is now tested and ret is set to
-EBUSY if the the call has failed. This call was associated with the
initialization of ipw->attr_memory. But the error handling code was
testing ipw->common_memory. The definition of release_ipwireless also
suggests that this call should be associated with ipw->common_memory, not
ipw->attr_memory.
3. The second call to request_mem_region is now tested and ret is
set to -EBUSY if the the call has failed.
4. The various gotos to the error handling code is adjusted so that there
is no need for ifs.
5. Return the value stored in the ret variable rather than -1.
The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@@
expression x,E;
@@
(
*x = request_region(...)
|
*x = request_mem_region(...)
)
... when != release_region(x)
when != x = E
* release_resource(x);
// </smpl>
Signed-off-by: Julia Lawall <[email protected]>
---
Compiled, not tested.
In release_ipwireless, should the added call to release_region be protected
by some if?
drivers/char/pcmcia/ipwireless/main.c | 52 ++++++++++++++++++++--------------
1 file changed, 32 insertions(+), 20 deletions(-)
diff --git a/drivers/char/pcmcia/ipwireless/main.c b/drivers/char/pcmcia/ipwireless/main.c
index 94b8eb4..444155a 100644
--- a/drivers/char/pcmcia/ipwireless/main.c
+++ b/drivers/char/pcmcia/ipwireless/main.c
@@ -78,7 +78,6 @@ static void signalled_reboot_callback(void *callback_data)
static int ipwireless_probe(struct pcmcia_device *p_dev, void *priv_data)
{
struct ipw_dev *ipw = priv_data;
- struct resource *io_resource;
int ret;
p_dev->resource[0]->flags &= ~IO_DATA_PATH_WIDTH;
@@ -92,9 +91,12 @@ static int ipwireless_probe(struct pcmcia_device *p_dev, void *priv_data)
if (ret)
return ret;
- io_resource = request_region(p_dev->resource[0]->start,
- resource_size(p_dev->resource[0]),
- IPWIRELESS_PCCARD_NAME);
+ if (!request_region(p_dev->resource[0]->start,
+ resource_size(p_dev->resource[0]),
+ IPWIRELESS_PCCARD_NAME)) {
+ ret = -EBUSY;
+ goto exit;
+ }
p_dev->resource[2]->flags |=
WIN_DATA_WIDTH_16 | WIN_MEMORY_TYPE_CM | WIN_ENABLE;
@@ -105,22 +107,25 @@ static int ipwireless_probe(struct pcmcia_device *p_dev, void *priv_data)
ret = pcmcia_map_mem_page(p_dev, p_dev->resource[2], p_dev->card_addr);
if (ret != 0)
- goto exit2;
+ goto exit1;
ipw->is_v2_card = resource_size(p_dev->resource[2]) == 0x100;
- ipw->attr_memory = ioremap(p_dev->resource[2]->start,
+ ipw->common_memory = ioremap(p_dev->resource[2]->start,
resource_size(p_dev->resource[2]));
- request_mem_region(p_dev->resource[2]->start,
- resource_size(p_dev->resource[2]),
- IPWIRELESS_PCCARD_NAME);
+ if (!request_mem_region(p_dev->resource[2]->start,
+ resource_size(p_dev->resource[2]),
+ IPWIRELESS_PCCARD_NAME)) {
+ ret = -EBUSY;
+ goto exit2;
+ }
p_dev->resource[3]->flags |= WIN_DATA_WIDTH_16 | WIN_MEMORY_TYPE_AM |
WIN_ENABLE;
p_dev->resource[3]->end = 0; /* this used to be 0x1000 */
ret = pcmcia_request_window(p_dev, p_dev->resource[3], 0);
if (ret != 0)
- goto exit2;
+ goto exit3;
ret = pcmcia_map_mem_page(p_dev, p_dev->resource[3], 0);
if (ret != 0)
@@ -128,23 +133,28 @@ static int ipwireless_probe(struct pcmcia_device *p_dev, void *priv_data)
ipw->attr_memory = ioremap(p_dev->resource[3]->start,
resource_size(p_dev->resource[3]));
- request_mem_region(p_dev->resource[3]->start,
- resource_size(p_dev->resource[3]),
- IPWIRELESS_PCCARD_NAME);
+ if (!request_mem_region(p_dev->resource[3]->start,
+ resource_size(p_dev->resource[3]),
+ IPWIRELESS_PCCARD_NAME)) {
+ ret = -EBUSY;
+ goto exit4;
+ }
return 0;
+exit4:
+ iounmap(ipw->attr_memory);
exit3:
+ release_mem_region(p_dev->resource[2]->start,
+ resource_size(p_dev->resource[2]));
exit2:
- if (ipw->common_memory) {
- release_mem_region(p_dev->resource[2]->start,
- resource_size(p_dev->resource[2]));
- iounmap(ipw->common_memory);
- }
+ iounmap(ipw->common_memory);
exit1:
- release_resource(io_resource);
+ release_region(p_dev->resource[0]->start,
+ resource_size(p_dev->resource[0]));
+exit:
pcmcia_disable_device(p_dev);
- return -1;
+ return ret;
}
static int config_ipwireless(struct ipw_dev *ipw)
@@ -219,6 +229,8 @@ exit:
static void release_ipwireless(struct ipw_dev *ipw)
{
+ release_region(ipw->link->resource[0]->start,
+ resource_size(ipw->link->resource[0]));
if (ipw->common_memory) {
release_mem_region(ipw->link->resource[2]->start,
resource_size(ipw->link->resource[2]));
Request_region should be used with release_region, not release_resource.
The result of request_mem_region is no longer stored. Instead the field
ioarea is used to store a pointer to the resource structure that contains
the start address. This is the information that is needed later in
nuc900_i2c_remove to release the region. The field ioarea is also printed
in some debugging code.
The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@@
expression x,E;
@@
(
*x = request_region(...)
|
*x = request_mem_region(...)
)
... when != release_region(x)
when != x = E
* release_resource(x);
// </smpl>
Signed-off-by: Julia Lawall <[email protected]>
---
Not compiled due to incompatible architecture.
drivers/i2c/busses/i2c-nuc900.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/drivers/i2c/busses/i2c-nuc900.c b/drivers/i2c/busses/i2c-nuc900.c
index 7243426..97b16b7 100644
--- a/drivers/i2c/busses/i2c-nuc900.c
+++ b/drivers/i2c/busses/i2c-nuc900.c
@@ -568,15 +568,13 @@ static int __devinit nuc900_i2c_probe(struct platform_device *pdev)
goto err_clk;
}
- i2c->ioarea = request_mem_region(res->start, resource_size(res),
- pdev->name);
-
- if (i2c->ioarea == NULL) {
+ if (!request_mem_region(res->start, resource_size(res), pdev->name)) {
dev_err(&pdev->dev, "cannot request IO\n");
ret = -ENXIO;
goto err_clk;
}
+ i2c->ioarea = res;
i2c->regs = ioremap(res->start, resource_size(res));
if (i2c->regs == NULL) {
@@ -645,8 +643,7 @@ static int __devinit nuc900_i2c_probe(struct platform_device *pdev)
iounmap(i2c->regs);
err_ioarea:
- release_resource(i2c->ioarea);
- kfree(i2c->ioarea);
+ release_mem_region(res->start, resource_size(res));
err_clk:
clk_disable(i2c->clk);
@@ -665,6 +662,7 @@ static int __devinit nuc900_i2c_probe(struct platform_device *pdev)
static int __devexit nuc900_i2c_remove(struct platform_device *pdev)
{
struct nuc900_i2c *i2c = platform_get_drvdata(pdev);
+ struct resource *res = i2c->ioarea;
i2c_del_adapter(&i2c->adap);
free_irq(i2c->irq, i2c);
@@ -674,8 +672,7 @@ static int __devexit nuc900_i2c_remove(struct platform_device *pdev)
iounmap(i2c->regs);
- release_resource(i2c->ioarea);
- kfree(i2c->ioarea);
+ release_mem_region(res->start, resource_size(res));
kfree(i2c);
return 0;
Request_region should be used with release_region, not release_resource.
The local variables region and region2 are dropped and the calls to
release_resource are replaced with calls to release_region, using the first
two arguments of the corresponding calls to request_region.
The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@@
expression x,E;
@@
(
*x = request_region(...)
|
*x = request_mem_region(...)
)
... when != release_region(x)
when != x = E
* release_resource(x);
// </smpl>
Signed-off-by: Julia Lawall <[email protected]>
---
Compiled, not tested.
arch/x86/pci/direct.c | 17 ++++++-----------
1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/arch/x86/pci/direct.c b/arch/x86/pci/direct.c
index bd33620..e6fd847 100644
--- a/arch/x86/pci/direct.c
+++ b/arch/x86/pci/direct.c
@@ -280,12 +280,9 @@ void __init pci_direct_init(int type)
int __init pci_direct_probe(void)
{
- struct resource *region, *region2;
-
if ((pci_probe & PCI_PROBE_CONF1) == 0)
goto type2;
- region = request_region(0xCF8, 8, "PCI conf1");
- if (!region)
+ if (!request_region(0xCF8, 8, "PCI conf1"))
goto type2;
if (pci_check_type1()) {
@@ -293,16 +290,14 @@ int __init pci_direct_probe(void)
port_cf9_safe = true;
return 1;
}
- release_resource(region);
+ release_region(0xCF8, 8);
type2:
if ((pci_probe & PCI_PROBE_CONF2) == 0)
return 0;
- region = request_region(0xCF8, 4, "PCI conf2");
- if (!region)
+ if (!request_region(0xCF8, 4, "PCI conf2"))
return 0;
- region2 = request_region(0xC000, 0x1000, "PCI conf2");
- if (!region2)
+ if (!request_region(0xC000, 0x1000, "PCI conf2"))
goto fail2;
if (pci_check_type2()) {
@@ -311,8 +306,8 @@ int __init pci_direct_probe(void)
return 2;
}
- release_resource(region2);
+ release_region(0xC000, 0x1000);
fail2:
- release_resource(region);
+ release_region(0xCF8, 4);
return 0;
}
Request_region should be used with release_region, not release_resource.
The local variable mem, storing the result of request_mem_region, is
dropped and instead the pointer res is stored in the drvdata field of the
platform device. This information is retrieved in omap_rng_remove to
release the region. The drvdata field is not used elsewhere.
The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@@
expression x,E;
@@
(
*x = request_region(...)
|
*x = request_mem_region(...)
)
... when != release_region(x)
when != x = E
* release_resource(x);
// </smpl>
Signed-off-by: Julia Lawall <[email protected]>
---
Not compiled.
drivers/char/hw_random/omap-rng.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/drivers/char/hw_random/omap-rng.c b/drivers/char/hw_random/omap-rng.c
index 06aad08..2cc755a 100644
--- a/drivers/char/hw_random/omap-rng.c
+++ b/drivers/char/hw_random/omap-rng.c
@@ -91,7 +91,7 @@ static struct hwrng omap_rng_ops = {
static int __devinit omap_rng_probe(struct platform_device *pdev)
{
- struct resource *res, *mem;
+ struct resource *res;
int ret;
/*
@@ -116,14 +116,12 @@ static int __devinit omap_rng_probe(struct platform_device *pdev)
if (!res)
return -ENOENT;
- mem = request_mem_region(res->start, resource_size(res),
- pdev->name);
- if (mem == NULL) {
+ if (!request_mem_region(res->start, resource_size(res), pdev->name)) {
ret = -EBUSY;
goto err_region;
}
- dev_set_drvdata(&pdev->dev, mem);
+ dev_set_drvdata(&pdev->dev, res);
rng_base = ioremap(res->start, resource_size(res));
if (!rng_base) {
ret = -ENOMEM;
@@ -146,7 +144,7 @@ err_register:
iounmap(rng_base);
rng_base = NULL;
err_ioremap:
- release_resource(mem);
+ release_mem_region(res->start, resource_size(res));
err_region:
if (cpu_is_omap24xx()) {
clk_disable(rng_ick);
@@ -157,7 +155,7 @@ err_region:
static int __exit omap_rng_remove(struct platform_device *pdev)
{
- struct resource *mem = dev_get_drvdata(&pdev->dev);
+ struct resource *res = dev_get_drvdata(&pdev->dev);
hwrng_unregister(&omap_rng_ops);
@@ -170,7 +168,7 @@ static int __exit omap_rng_remove(struct platform_device *pdev)
clk_put(rng_ick);
}
- release_resource(mem);
+ release_mem_region(res->start, resource_size(res));
rng_base = NULL;
return 0;
On Sunday 13 February 2011 13:12:12 Julia Lawall wrote:
> Request_region should be used with release_region, not release_resource.
>
> The result of request_mem_region is no longer stored. Instead the field
> ioarea is used to store a pointer to the resource structure that contains
> the start address. This is the information that is needed later in
> nuc900_i2c_remove to release the region. The field ioarea is also printed
> in some debugging code.
>
> The semantic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)
>
> // <smpl>
> @@
> expression x,E;
> @@
> (
> *x = request_region(...)
>
> *x = request_mem_region(...)
> )
> ... when != release_region(x)
> when != x = E
> * release_resource(x);
> // </smpl>
>
> Signed-off-by: Julia Lawall <[email protected]>
>
> ---
> Not compiled due to incompatible architecture.
>
> drivers/i2c/busses/i2c-nuc900.c | 13 +++++--------
> 1 file changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-nuc900.c
> b/drivers/i2c/busses/i2c-nuc900.c index 7243426..97b16b7 100644
> --- a/drivers/i2c/busses/i2c-nuc900.c
> +++ b/drivers/i2c/busses/i2c-nuc900.c
> @@ -568,15 +568,13 @@ static int __devinit nuc900_i2c_probe(struct
> platform_device *pdev) goto err_clk;
> }
>
> - i2c->ioarea = request_mem_region(res->start, resource_size(res),
> - pdev->name);
> -
> - if (i2c->ioarea == NULL) {
> + if (!request_mem_region(res->start, resource_size(res), pdev->name)) {
I prefer the original version here -- it's clearer and also, doesn't the new
version violate line-over-80 principle ?
> dev_err(&pdev->dev, "cannot request IO\n");
> ret = -ENXIO;
> goto err_clk;
> }
>
> + i2c->ioarea = res;
> i2c->regs = ioremap(res->start, resource_size(res));
>
> if (i2c->regs == NULL) {
> @@ -645,8 +643,7 @@ static int __devinit nuc900_i2c_probe(struct
> platform_device *pdev) iounmap(i2c->regs);
>
> err_ioarea:
> - release_resource(i2c->ioarea);
> - kfree(i2c->ioarea);
> + release_mem_region(res->start, resource_size(res));
>
> err_clk:
> clk_disable(i2c->clk);
> @@ -665,6 +662,7 @@ static int __devinit nuc900_i2c_probe(struct
> platform_device *pdev) static int __devexit nuc900_i2c_remove(struct
> platform_device *pdev) {
> struct nuc900_i2c *i2c = platform_get_drvdata(pdev);
> + struct resource *res = i2c->ioarea;
No need for this ...
>
> i2c_del_adapter(&i2c->adap);
> free_irq(i2c->irq, i2c);
> @@ -674,8 +672,7 @@ static int __devexit nuc900_i2c_remove(struct
> platform_device *pdev)
>
> iounmap(i2c->regs);
>
> - release_resource(i2c->ioarea);
> - kfree(i2c->ioarea);
... the i2c pointer is still available here, why introducing struct resource
*res. release_mem_region() looks like a sane change to me though.
> + release_mem_region(res->start, resource_size(res));
> kfree(i2c);
>
> return 0;
On Mon, 14 Feb 2011, Marek Vasut wrote:
> On Sunday 13 February 2011 13:12:12 Julia Lawall wrote:
> > Request_region should be used with release_region, not release_resource.
> >
> > The result of request_mem_region is no longer stored. Instead the field
> > ioarea is used to store a pointer to the resource structure that contains
> > the start address. This is the information that is needed later in
> > nuc900_i2c_remove to release the region. The field ioarea is also printed
> > in some debugging code.
> >
> > The semantic match that finds this problem is as follows:
> > (http://coccinelle.lip6.fr/)
> >
> > // <smpl>
> > @@
> > expression x,E;
> > @@
> > (
> > *x = request_region(...)
> >
> > *x = request_mem_region(...)
> > )
> > ... when != release_region(x)
> > when != x = E
> > * release_resource(x);
> > // </smpl>
> >
> > Signed-off-by: Julia Lawall <[email protected]>
> >
> > ---
> > Not compiled due to incompatible architecture.
> >
> > drivers/i2c/busses/i2c-nuc900.c | 13 +++++--------
> > 1 file changed, 5 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-nuc900.c
> > b/drivers/i2c/busses/i2c-nuc900.c index 7243426..97b16b7 100644
> > --- a/drivers/i2c/busses/i2c-nuc900.c
> > +++ b/drivers/i2c/busses/i2c-nuc900.c
> > @@ -568,15 +568,13 @@ static int __devinit nuc900_i2c_probe(struct
> > platform_device *pdev) goto err_clk;
> > }
> >
> > - i2c->ioarea = request_mem_region(res->start, resource_size(res),
> > - pdev->name);
> > -
> > - if (i2c->ioarea == NULL) {
> > + if (!request_mem_region(res->start, resource_size(res), pdev->name)) {
>
> I prefer the original version here -- it's clearer and also, doesn't the new
> version violate line-over-80 principle ?
The line ends before column 80.
I'm not sure that the result of request_mem_region is the proper argment
to give to release_region. In any case, all of the other calls to
request_mem_region that do use release_region do not use the return value.
They just test it directly as I have done here.
julia
> > dev_err(&pdev->dev, "cannot request IO\n");
> > ret = -ENXIO;
> > goto err_clk;
> > }
> >
> > + i2c->ioarea = res;
> > i2c->regs = ioremap(res->start, resource_size(res));
> >
> > if (i2c->regs == NULL) {
> > @@ -645,8 +643,7 @@ static int __devinit nuc900_i2c_probe(struct
> > platform_device *pdev) iounmap(i2c->regs);
> >
> > err_ioarea:
> > - release_resource(i2c->ioarea);
> > - kfree(i2c->ioarea);
> > + release_mem_region(res->start, resource_size(res));
> >
> > err_clk:
> > clk_disable(i2c->clk);
> > @@ -665,6 +662,7 @@ static int __devinit nuc900_i2c_probe(struct
> > platform_device *pdev) static int __devexit nuc900_i2c_remove(struct
> > platform_device *pdev) {
> > struct nuc900_i2c *i2c = platform_get_drvdata(pdev);
> > + struct resource *res = i2c->ioarea;
>
> No need for this ...
> >
> > i2c_del_adapter(&i2c->adap);
> > free_irq(i2c->irq, i2c);
> > @@ -674,8 +672,7 @@ static int __devexit nuc900_i2c_remove(struct
> > platform_device *pdev)
> >
> > iounmap(i2c->regs);
> >
> > - release_resource(i2c->ioarea);
> > - kfree(i2c->ioarea);
>
> ... the i2c pointer is still available here, why introducing struct resource
> *res. release_mem_region() looks like a sane change to me though.
>
> > + release_mem_region(res->start, resource_size(res));
> > kfree(i2c);
> >
> > return 0;
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
On Sun, 2011-02-13 at 13:12 +0100, Julia Lawall wrote:
> Request_region should be used with release_region, not release_resource.
>
> The local variable mem, storing the result of request_mem_region, is
> dropped and instead the pointer res is stored in the drvdata field of the
> platform device. This information is retrieved in omap_rng_remove to
> release the region. The drvdata field is not used elsewhere.
>
> The semantic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)
Looks ok to me, Herbert?
--
Mathematics is the supreme nostalgia of our time.
On Mon, Feb 14, 2011 at 04:42:24PM -0600, Matt Mackall wrote:
> On Sun, 2011-02-13 at 13:12 +0100, Julia Lawall wrote:
> > Request_region should be used with release_region, not release_resource.
> >
> > The local variable mem, storing the result of request_mem_region, is
> > dropped and instead the pointer res is stored in the drvdata field of the
> > platform device. This information is retrieved in omap_rng_remove to
> > release the region. The drvdata field is not used elsewhere.
> >
> > The semantic match that finds this problem is as follows:
> > (http://coccinelle.lip6.fr/)
>
> Looks ok to me, Herbert?
Yes I will apply it today.
Thanks,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Applied to the pcmcia treee, thanks.
On Sun, Feb 13, 2011 at 01:12:10PM +0100, Julia Lawall wrote:
> Request_region should be used with release_region, not release_resource.
>
> This patch contains a number of changes, related to calls to request_region,
> request_mem_region, and the associated error handling code.
>
> 1. For the call to request_region, the variable io_resource storing the
> result is dropped. The call to release_resource at the end of the function
> is changed to a call to release_region with the first two arguments of
> request_region as its arguments. The same call to release_region is also
> added to release_ipwireless.
>
> 2. The first call to request_mem_region is now tested and ret is set to
> -EBUSY if the the call has failed. This call was associated with the
> initialization of ipw->attr_memory. But the error handling code was
> testing ipw->common_memory. The definition of release_ipwireless also
> suggests that this call should be associated with ipw->common_memory, not
> ipw->attr_memory.
>
> 3. The second call to request_mem_region is now tested and ret is
> set to -EBUSY if the the call has failed.
>
> 4. The various gotos to the error handling code is adjusted so that there
> is no need for ifs.
>
> 5. Return the value stored in the ret variable rather than -1.
>
> The semantic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)
>
> // <smpl>
> @@
> expression x,E;
> @@
> (
> *x = request_region(...)
> |
> *x = request_mem_region(...)
> )
> ... when != release_region(x)
> when != x = E
> * release_resource(x);
> // </smpl>
>
> Signed-off-by: Julia Lawall <[email protected]>
>
> ---
> Compiled, not tested.
> In release_ipwireless, should the added call to release_region be protected
> by some if?
>
> drivers/char/pcmcia/ipwireless/main.c | 52 ++++++++++++++++++++--------------
> 1 file changed, 32 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/char/pcmcia/ipwireless/main.c b/drivers/char/pcmcia/ipwireless/main.c
> index 94b8eb4..444155a 100644
> --- a/drivers/char/pcmcia/ipwireless/main.c
> +++ b/drivers/char/pcmcia/ipwireless/main.c
> @@ -78,7 +78,6 @@ static void signalled_reboot_callback(void *callback_data)
> static int ipwireless_probe(struct pcmcia_device *p_dev, void *priv_data)
> {
> struct ipw_dev *ipw = priv_data;
> - struct resource *io_resource;
> int ret;
>
> p_dev->resource[0]->flags &= ~IO_DATA_PATH_WIDTH;
> @@ -92,9 +91,12 @@ static int ipwireless_probe(struct pcmcia_device *p_dev, void *priv_data)
> if (ret)
> return ret;
>
> - io_resource = request_region(p_dev->resource[0]->start,
> - resource_size(p_dev->resource[0]),
> - IPWIRELESS_PCCARD_NAME);
> + if (!request_region(p_dev->resource[0]->start,
> + resource_size(p_dev->resource[0]),
> + IPWIRELESS_PCCARD_NAME)) {
> + ret = -EBUSY;
> + goto exit;
> + }
>
> p_dev->resource[2]->flags |=
> WIN_DATA_WIDTH_16 | WIN_MEMORY_TYPE_CM | WIN_ENABLE;
> @@ -105,22 +107,25 @@ static int ipwireless_probe(struct pcmcia_device *p_dev, void *priv_data)
>
> ret = pcmcia_map_mem_page(p_dev, p_dev->resource[2], p_dev->card_addr);
> if (ret != 0)
> - goto exit2;
> + goto exit1;
>
> ipw->is_v2_card = resource_size(p_dev->resource[2]) == 0x100;
>
> - ipw->attr_memory = ioremap(p_dev->resource[2]->start,
> + ipw->common_memory = ioremap(p_dev->resource[2]->start,
> resource_size(p_dev->resource[2]));
> - request_mem_region(p_dev->resource[2]->start,
> - resource_size(p_dev->resource[2]),
> - IPWIRELESS_PCCARD_NAME);
> + if (!request_mem_region(p_dev->resource[2]->start,
> + resource_size(p_dev->resource[2]),
> + IPWIRELESS_PCCARD_NAME)) {
> + ret = -EBUSY;
> + goto exit2;
> + }
>
> p_dev->resource[3]->flags |= WIN_DATA_WIDTH_16 | WIN_MEMORY_TYPE_AM |
> WIN_ENABLE;
> p_dev->resource[3]->end = 0; /* this used to be 0x1000 */
> ret = pcmcia_request_window(p_dev, p_dev->resource[3], 0);
> if (ret != 0)
> - goto exit2;
> + goto exit3;
>
> ret = pcmcia_map_mem_page(p_dev, p_dev->resource[3], 0);
> if (ret != 0)
> @@ -128,23 +133,28 @@ static int ipwireless_probe(struct pcmcia_device *p_dev, void *priv_data)
>
> ipw->attr_memory = ioremap(p_dev->resource[3]->start,
> resource_size(p_dev->resource[3]));
> - request_mem_region(p_dev->resource[3]->start,
> - resource_size(p_dev->resource[3]),
> - IPWIRELESS_PCCARD_NAME);
> + if (!request_mem_region(p_dev->resource[3]->start,
> + resource_size(p_dev->resource[3]),
> + IPWIRELESS_PCCARD_NAME)) {
> + ret = -EBUSY;
> + goto exit4;
> + }
>
> return 0;
>
> +exit4:
> + iounmap(ipw->attr_memory);
> exit3:
> + release_mem_region(p_dev->resource[2]->start,
> + resource_size(p_dev->resource[2]));
> exit2:
> - if (ipw->common_memory) {
> - release_mem_region(p_dev->resource[2]->start,
> - resource_size(p_dev->resource[2]));
> - iounmap(ipw->common_memory);
> - }
> + iounmap(ipw->common_memory);
> exit1:
> - release_resource(io_resource);
> + release_region(p_dev->resource[0]->start,
> + resource_size(p_dev->resource[0]));
> +exit:
> pcmcia_disable_device(p_dev);
> - return -1;
> + return ret;
> }
>
> static int config_ipwireless(struct ipw_dev *ipw)
> @@ -219,6 +229,8 @@ exit:
>
> static void release_ipwireless(struct ipw_dev *ipw)
> {
> + release_region(ipw->link->resource[0]->start,
> + resource_size(ipw->link->resource[0]));
> if (ipw->common_memory) {
> release_mem_region(ipw->link->resource[2]->start,
> resource_size(ipw->link->resource[2]));
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
On Wed, 16 Feb 2011, Dominik Brodowski wrote:
> Applied to the pcmcia treee, thanks.
FWIW
Signed-off-by: Jiri Kosina <[email protected]>
> On Sun, Feb 13, 2011 at 01:12:10PM +0100, Julia Lawall wrote:
> > Request_region should be used with release_region, not release_resource.
> >
> > This patch contains a number of changes, related to calls to request_region,
> > request_mem_region, and the associated error handling code.
> >
> > 1. For the call to request_region, the variable io_resource storing the
> > result is dropped. The call to release_resource at the end of the function
> > is changed to a call to release_region with the first two arguments of
> > request_region as its arguments. The same call to release_region is also
> > added to release_ipwireless.
> >
> > 2. The first call to request_mem_region is now tested and ret is set to
> > -EBUSY if the the call has failed. This call was associated with the
> > initialization of ipw->attr_memory. But the error handling code was
> > testing ipw->common_memory. The definition of release_ipwireless also
> > suggests that this call should be associated with ipw->common_memory, not
> > ipw->attr_memory.
> >
> > 3. The second call to request_mem_region is now tested and ret is
> > set to -EBUSY if the the call has failed.
> >
> > 4. The various gotos to the error handling code is adjusted so that there
> > is no need for ifs.
> >
> > 5. Return the value stored in the ret variable rather than -1.
> >
> > The semantic match that finds this problem is as follows:
> > (http://coccinelle.lip6.fr/)
> >
> > // <smpl>
> > @@
> > expression x,E;
> > @@
> > (
> > *x = request_region(...)
> > |
> > *x = request_mem_region(...)
> > )
> > ... when != release_region(x)
> > when != x = E
> > * release_resource(x);
> > // </smpl>
> >
> > Signed-off-by: Julia Lawall <[email protected]>
> >
> > ---
> > Compiled, not tested.
> > In release_ipwireless, should the added call to release_region be protected
> > by some if?
> >
> > drivers/char/pcmcia/ipwireless/main.c | 52 ++++++++++++++++++++--------------
> > 1 file changed, 32 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/char/pcmcia/ipwireless/main.c b/drivers/char/pcmcia/ipwireless/main.c
> > index 94b8eb4..444155a 100644
> > --- a/drivers/char/pcmcia/ipwireless/main.c
> > +++ b/drivers/char/pcmcia/ipwireless/main.c
> > @@ -78,7 +78,6 @@ static void signalled_reboot_callback(void *callback_data)
> > static int ipwireless_probe(struct pcmcia_device *p_dev, void *priv_data)
> > {
> > struct ipw_dev *ipw = priv_data;
> > - struct resource *io_resource;
> > int ret;
> >
> > p_dev->resource[0]->flags &= ~IO_DATA_PATH_WIDTH;
> > @@ -92,9 +91,12 @@ static int ipwireless_probe(struct pcmcia_device *p_dev, void *priv_data)
> > if (ret)
> > return ret;
> >
> > - io_resource = request_region(p_dev->resource[0]->start,
> > - resource_size(p_dev->resource[0]),
> > - IPWIRELESS_PCCARD_NAME);
> > + if (!request_region(p_dev->resource[0]->start,
> > + resource_size(p_dev->resource[0]),
> > + IPWIRELESS_PCCARD_NAME)) {
> > + ret = -EBUSY;
> > + goto exit;
> > + }
> >
> > p_dev->resource[2]->flags |=
> > WIN_DATA_WIDTH_16 | WIN_MEMORY_TYPE_CM | WIN_ENABLE;
> > @@ -105,22 +107,25 @@ static int ipwireless_probe(struct pcmcia_device *p_dev, void *priv_data)
> >
> > ret = pcmcia_map_mem_page(p_dev, p_dev->resource[2], p_dev->card_addr);
> > if (ret != 0)
> > - goto exit2;
> > + goto exit1;
> >
> > ipw->is_v2_card = resource_size(p_dev->resource[2]) == 0x100;
> >
> > - ipw->attr_memory = ioremap(p_dev->resource[2]->start,
> > + ipw->common_memory = ioremap(p_dev->resource[2]->start,
> > resource_size(p_dev->resource[2]));
> > - request_mem_region(p_dev->resource[2]->start,
> > - resource_size(p_dev->resource[2]),
> > - IPWIRELESS_PCCARD_NAME);
> > + if (!request_mem_region(p_dev->resource[2]->start,
> > + resource_size(p_dev->resource[2]),
> > + IPWIRELESS_PCCARD_NAME)) {
> > + ret = -EBUSY;
> > + goto exit2;
> > + }
> >
> > p_dev->resource[3]->flags |= WIN_DATA_WIDTH_16 | WIN_MEMORY_TYPE_AM |
> > WIN_ENABLE;
> > p_dev->resource[3]->end = 0; /* this used to be 0x1000 */
> > ret = pcmcia_request_window(p_dev, p_dev->resource[3], 0);
> > if (ret != 0)
> > - goto exit2;
> > + goto exit3;
> >
> > ret = pcmcia_map_mem_page(p_dev, p_dev->resource[3], 0);
> > if (ret != 0)
> > @@ -128,23 +133,28 @@ static int ipwireless_probe(struct pcmcia_device *p_dev, void *priv_data)
> >
> > ipw->attr_memory = ioremap(p_dev->resource[3]->start,
> > resource_size(p_dev->resource[3]));
> > - request_mem_region(p_dev->resource[3]->start,
> > - resource_size(p_dev->resource[3]),
> > - IPWIRELESS_PCCARD_NAME);
> > + if (!request_mem_region(p_dev->resource[3]->start,
> > + resource_size(p_dev->resource[3]),
> > + IPWIRELESS_PCCARD_NAME)) {
> > + ret = -EBUSY;
> > + goto exit4;
> > + }
> >
> > return 0;
> >
> > +exit4:
> > + iounmap(ipw->attr_memory);
> > exit3:
> > + release_mem_region(p_dev->resource[2]->start,
> > + resource_size(p_dev->resource[2]));
> > exit2:
> > - if (ipw->common_memory) {
> > - release_mem_region(p_dev->resource[2]->start,
> > - resource_size(p_dev->resource[2]));
> > - iounmap(ipw->common_memory);
> > - }
> > + iounmap(ipw->common_memory);
> > exit1:
> > - release_resource(io_resource);
> > + release_region(p_dev->resource[0]->start,
> > + resource_size(p_dev->resource[0]));
> > +exit:
> > pcmcia_disable_device(p_dev);
> > - return -1;
> > + return ret;
> > }
> >
> > static int config_ipwireless(struct ipw_dev *ipw)
> > @@ -219,6 +229,8 @@ exit:
> >
> > static void release_ipwireless(struct ipw_dev *ipw)
> > {
> > + release_region(ipw->link->resource[0]->start,
> > + resource_size(ipw->link->resource[0]));
> > if (ipw->common_memory) {
> > release_mem_region(ipw->link->resource[2]->start,
> > resource_size(ipw->link->resource[2]));
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at http://www.tux.org/lkml/
>
--
Jiri Kosina
SUSE Labs, Novell Inc.
On Sun, 13 Feb 2011 13:12:11 +0100
Julia Lawall <[email protected]> wrote:
> Request_region should be used with release_region, not release_resource.
>
> The local variables region and region2 are dropped and the calls to
> release_resource are replaced with calls to release_region, using the first
> two arguments of the corresponding calls to request_region.
>
> The semantic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)
Applied to linux-next, thanks.
--
Jesse Barnes, Intel Open Source Technology Center