* [PATCH v4] i2c: busses: i2c-pxa.c: Fix for possible null pointer dereference
@ 2014-07-16 21:14 ` Rickard Strandqvist
0 siblings, 0 replies; 7+ messages in thread
From: Rickard Strandqvist @ 2014-07-16 21:14 UTC (permalink / raw)
To: Wolfram Sang, Grant Likely
Cc: Rickard Strandqvist, Rob Herring, Jingoo Han, Leilei Shang,
Peter Korsgaard, linux-i2c, linux-kernel, devicetree
This start as a fix for possible null pointer dereference.
But after discussion with especially Wolfram, Jingoo and Emil it was
decided to convert the code to uses Managed Device Resource instead.
Rickard Strandqvist (1):
i2c: busses: i2c-pxa.c: Fix for possible null pointer dereference
drivers/i2c/busses/i2c-pxa.c | 65 ++++++++++++++----------------------------
1 file changed, 22 insertions(+), 43 deletions(-)
--
1.7.10.4
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v4] i2c: busses: i2c-pxa.c: Fix for possible null pointer dereference
@ 2014-07-16 21:14 ` Rickard Strandqvist
0 siblings, 0 replies; 7+ messages in thread
From: Rickard Strandqvist @ 2014-07-16 21:14 UTC (permalink / raw)
To: Wolfram Sang, Grant Likely
Cc: Rickard Strandqvist, Rob Herring, Jingoo Han, Leilei Shang,
Peter Korsgaard, linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
This start as a fix for possible null pointer dereference.
But after discussion with especially Wolfram, Jingoo and Emil it was
decided to convert the code to uses Managed Device Resource instead.
Rickard Strandqvist (1):
i2c: busses: i2c-pxa.c: Fix for possible null pointer dereference
drivers/i2c/busses/i2c-pxa.c | 65 ++++++++++++++----------------------------
1 file changed, 22 insertions(+), 43 deletions(-)
--
1.7.10.4
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v4] i2c: busses: i2c-pxa.c: Fix for possible null pointer dereference
@ 2014-07-16 21:14 ` Rickard Strandqvist
0 siblings, 0 replies; 7+ messages in thread
From: Rickard Strandqvist @ 2014-07-16 21:14 UTC (permalink / raw)
To: Wolfram Sang, Grant Likely
Cc: Rickard Strandqvist, Rob Herring, Jingoo Han, Leilei Shang,
Peter Korsgaard, linux-i2c, linux-kernel, devicetree
Fix for possible null pointer dereference, and there was a risk for
memory leak if something unexpected happens and the function returns.
It now uses the Managed Device Resource instead.
Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
---
drivers/i2c/busses/i2c-pxa.c | 65 ++++++++++++++----------------------------
1 file changed, 22 insertions(+), 43 deletions(-)
diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index be671f7..0d0605c 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -1141,11 +1141,9 @@ static int i2c_pxa_probe(struct platform_device *dev)
struct resource *res = NULL;
int ret, irq;
- i2c = kzalloc(sizeof(struct pxa_i2c), GFP_KERNEL);
- if (!i2c) {
- ret = -ENOMEM;
- goto emalloc;
- }
+ i2c = devm_kzalloc(&dev->dev, sizeof(struct pxa_i2c), GFP_KERNEL);
+ if (!i2c)
+ return -ENOMEM;
/* Default adapter num to device id; i2c_pxa_probe_dt can override. */
i2c->adap.nr = dev->id;
@@ -1154,19 +1152,16 @@ static int i2c_pxa_probe(struct platform_device *dev)
if (ret > 0)
ret = i2c_pxa_probe_pdata(dev, i2c, &i2c_type);
if (ret < 0)
- goto eclk;
+ return ret;
res = platform_get_resource(dev, IORESOURCE_MEM, 0);
irq = platform_get_irq(dev, 0);
- if (res == NULL || irq < 0) {
- ret = -ENODEV;
- goto eclk;
- }
+ if (res == NULL || irq < 0)
+ return -ENODEV;
- if (!request_mem_region(res->start, resource_size(res), res->name)) {
- ret = -ENOMEM;
- goto eclk;
- }
+ if (!devm_request_mem_region(&dev->dev, res->start,
+ resource_size(res), res->name))
+ return -ENOMEM;
i2c->adap.owner = THIS_MODULE;
i2c->adap.retries = 5;
@@ -1176,17 +1171,13 @@ static int i2c_pxa_probe(struct platform_device *dev)
strlcpy(i2c->adap.name, "pxa_i2c-i2c", sizeof(i2c->adap.name));
- i2c->clk = clk_get(&dev->dev, NULL);
- if (IS_ERR(i2c->clk)) {
- ret = PTR_ERR(i2c->clk);
- goto eclk;
- }
+ i2c->clk = devm_clk_get(&dev->dev, NULL);
+ if (IS_ERR(i2c->clk))
+ return PTR_ERR(i2c->clk);
- i2c->reg_base = ioremap(res->start, resource_size(res));
- if (!i2c->reg_base) {
- ret = -EIO;
- goto eremap;
- }
+ i2c->reg_base = devm_ioremap_resource(&adev->dev, res));
+ if (IS_ERR(i2c->reg_base))
+ return -EIO;
i2c->reg_ibmr = i2c->reg_base + pxa_reg_layout[i2c_type].ibmr;
i2c->reg_idbr = i2c->reg_base + pxa_reg_layout[i2c_type].idbr;
@@ -1227,10 +1218,12 @@ static int i2c_pxa_probe(struct platform_device *dev)
i2c->adap.algo = &i2c_pxa_pio_algorithm;
} else {
i2c->adap.algo = &i2c_pxa_algorithm;
- ret = request_irq(irq, i2c_pxa_handler, IRQF_SHARED,
- dev_name(&dev->dev), i2c);
- if (ret)
- goto ereqirq;
+ ret = devm_request_irq(&dev->dev, irq, i2c_pxa_handler,
+ IRQF_SHARED, dev_name(&dev->dev), i2c);
+ if (ret) {
+ clk_disable_unprepare(i2c->clk);
+ return ret;
+ }
}
i2c_pxa_reset(i2c);
@@ -1244,7 +1237,7 @@ static int i2c_pxa_probe(struct platform_device *dev)
ret = i2c_add_numbered_adapter(&i2c->adap);
if (ret < 0) {
printk(KERN_INFO "I2C: Failed to add bus\n");
- goto eadapt;
+ return ret;
}
platform_set_drvdata(dev, i2c);
@@ -1257,20 +1250,6 @@ static int i2c_pxa_probe(struct platform_device *dev)
dev_name(&i2c->adap.dev));
#endif
return 0;
-
-eadapt:
- if (!i2c->use_pio)
- free_irq(irq, i2c);
-ereqirq:
- clk_disable_unprepare(i2c->clk);
- iounmap(i2c->reg_base);
-eremap:
- clk_put(i2c->clk);
-eclk:
- kfree(i2c);
-emalloc:
- release_mem_region(res->start, resource_size(res));
- return ret;
}
static int i2c_pxa_remove(struct platform_device *dev)
--
1.7.10.4
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v4] i2c: busses: i2c-pxa.c: Fix for possible null pointer dereference
@ 2014-07-16 21:14 ` Rickard Strandqvist
0 siblings, 0 replies; 7+ messages in thread
From: Rickard Strandqvist @ 2014-07-16 21:14 UTC (permalink / raw)
To: Wolfram Sang, Grant Likely
Cc: Rickard Strandqvist, Rob Herring, Jingoo Han, Leilei Shang,
Peter Korsgaard, linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
Fix for possible null pointer dereference, and there was a risk for
memory leak if something unexpected happens and the function returns.
It now uses the Managed Device Resource instead.
Signed-off-by: Rickard Strandqvist <rickard_strandqvist-IW2WV5XWFqGZkjO+N0TKoMugMpMbD5Xr@public.gmane.org>
---
drivers/i2c/busses/i2c-pxa.c | 65 ++++++++++++++----------------------------
1 file changed, 22 insertions(+), 43 deletions(-)
diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index be671f7..0d0605c 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -1141,11 +1141,9 @@ static int i2c_pxa_probe(struct platform_device *dev)
struct resource *res = NULL;
int ret, irq;
- i2c = kzalloc(sizeof(struct pxa_i2c), GFP_KERNEL);
- if (!i2c) {
- ret = -ENOMEM;
- goto emalloc;
- }
+ i2c = devm_kzalloc(&dev->dev, sizeof(struct pxa_i2c), GFP_KERNEL);
+ if (!i2c)
+ return -ENOMEM;
/* Default adapter num to device id; i2c_pxa_probe_dt can override. */
i2c->adap.nr = dev->id;
@@ -1154,19 +1152,16 @@ static int i2c_pxa_probe(struct platform_device *dev)
if (ret > 0)
ret = i2c_pxa_probe_pdata(dev, i2c, &i2c_type);
if (ret < 0)
- goto eclk;
+ return ret;
res = platform_get_resource(dev, IORESOURCE_MEM, 0);
irq = platform_get_irq(dev, 0);
- if (res == NULL || irq < 0) {
- ret = -ENODEV;
- goto eclk;
- }
+ if (res == NULL || irq < 0)
+ return -ENODEV;
- if (!request_mem_region(res->start, resource_size(res), res->name)) {
- ret = -ENOMEM;
- goto eclk;
- }
+ if (!devm_request_mem_region(&dev->dev, res->start,
+ resource_size(res), res->name))
+ return -ENOMEM;
i2c->adap.owner = THIS_MODULE;
i2c->adap.retries = 5;
@@ -1176,17 +1171,13 @@ static int i2c_pxa_probe(struct platform_device *dev)
strlcpy(i2c->adap.name, "pxa_i2c-i2c", sizeof(i2c->adap.name));
- i2c->clk = clk_get(&dev->dev, NULL);
- if (IS_ERR(i2c->clk)) {
- ret = PTR_ERR(i2c->clk);
- goto eclk;
- }
+ i2c->clk = devm_clk_get(&dev->dev, NULL);
+ if (IS_ERR(i2c->clk))
+ return PTR_ERR(i2c->clk);
- i2c->reg_base = ioremap(res->start, resource_size(res));
- if (!i2c->reg_base) {
- ret = -EIO;
- goto eremap;
- }
+ i2c->reg_base = devm_ioremap_resource(&adev->dev, res));
+ if (IS_ERR(i2c->reg_base))
+ return -EIO;
i2c->reg_ibmr = i2c->reg_base + pxa_reg_layout[i2c_type].ibmr;
i2c->reg_idbr = i2c->reg_base + pxa_reg_layout[i2c_type].idbr;
@@ -1227,10 +1218,12 @@ static int i2c_pxa_probe(struct platform_device *dev)
i2c->adap.algo = &i2c_pxa_pio_algorithm;
} else {
i2c->adap.algo = &i2c_pxa_algorithm;
- ret = request_irq(irq, i2c_pxa_handler, IRQF_SHARED,
- dev_name(&dev->dev), i2c);
- if (ret)
- goto ereqirq;
+ ret = devm_request_irq(&dev->dev, irq, i2c_pxa_handler,
+ IRQF_SHARED, dev_name(&dev->dev), i2c);
+ if (ret) {
+ clk_disable_unprepare(i2c->clk);
+ return ret;
+ }
}
i2c_pxa_reset(i2c);
@@ -1244,7 +1237,7 @@ static int i2c_pxa_probe(struct platform_device *dev)
ret = i2c_add_numbered_adapter(&i2c->adap);
if (ret < 0) {
printk(KERN_INFO "I2C: Failed to add bus\n");
- goto eadapt;
+ return ret;
}
platform_set_drvdata(dev, i2c);
@@ -1257,20 +1250,6 @@ static int i2c_pxa_probe(struct platform_device *dev)
dev_name(&i2c->adap.dev));
#endif
return 0;
-
-eadapt:
- if (!i2c->use_pio)
- free_irq(irq, i2c);
-ereqirq:
- clk_disable_unprepare(i2c->clk);
- iounmap(i2c->reg_base);
-eremap:
- clk_put(i2c->clk);
-eclk:
- kfree(i2c);
-emalloc:
- release_mem_region(res->start, resource_size(res));
- return ret;
}
static int i2c_pxa_remove(struct platform_device *dev)
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v4] i2c: busses: i2c-pxa.c: Fix for possible null pointer dereference
2014-07-16 21:14 ` Rickard Strandqvist
(?)
@ 2014-08-02 11:30 ` Wolfram Sang
2014-08-16 21:32 ` Rickard Strandqvist
-1 siblings, 1 reply; 7+ messages in thread
From: Wolfram Sang @ 2014-08-02 11:30 UTC (permalink / raw)
To: Rickard Strandqvist
Cc: Grant Likely, Rob Herring, Jingoo Han, Leilei Shang,
Peter Korsgaard, linux-i2c, linux-kernel, devicetree
[-- Attachment #1: Type: text/plain, Size: 2745 bytes --]
Hi,
> res = platform_get_resource(dev, IORESOURCE_MEM, 0);
> irq = platform_get_irq(dev, 0);
> - if (res == NULL || irq < 0) {
> - ret = -ENODEV;
> - goto eclk;
> - }
> + if (res == NULL || irq < 0)
> + return -ENODEV;
No need to check for valid 'res' here...
>
> - if (!request_mem_region(res->start, resource_size(res), res->name)) {
> - ret = -ENOMEM;
> - goto eclk;
> - }
> + if (!devm_request_mem_region(&dev->dev, res->start,
> + resource_size(res), res->name))
> + return -ENOMEM;
... and no need to use 'devm_request_mem_region' ...
> - i2c->reg_base = ioremap(res->start, resource_size(res));
> - if (!i2c->reg_base) {
> - ret = -EIO;
> - goto eremap;
> - }
> + i2c->reg_base = devm_ioremap_resource(&adev->dev, res));
> + if (IS_ERR(i2c->reg_base))
> + return -EIO;
... because devm_ioremap_resource does all that for you. Also, don't
return -EIO but the PTR_ERR given from the function. Just check other
drivers how they do it.
>
> i2c->reg_ibmr = i2c->reg_base + pxa_reg_layout[i2c_type].ibmr;
> i2c->reg_idbr = i2c->reg_base + pxa_reg_layout[i2c_type].idbr;
> @@ -1227,10 +1218,12 @@ static int i2c_pxa_probe(struct platform_device *dev)
> i2c->adap.algo = &i2c_pxa_pio_algorithm;
> } else {
> i2c->adap.algo = &i2c_pxa_algorithm;
> - ret = request_irq(irq, i2c_pxa_handler, IRQF_SHARED,
> - dev_name(&dev->dev), i2c);
> - if (ret)
> - goto ereqirq;
> + ret = devm_request_irq(&dev->dev, irq, i2c_pxa_handler,
> + IRQF_SHARED, dev_name(&dev->dev), i2c);
> + if (ret) {
> + clk_disable_unprepare(i2c->clk);
> + return ret;
Maybe you should make a error path with 'goto' out of it...
> + }
> }
>
> i2c_pxa_reset(i2c);
> @@ -1244,7 +1237,7 @@ static int i2c_pxa_probe(struct platform_device *dev)
> ret = i2c_add_numbered_adapter(&i2c->adap);
> if (ret < 0) {
> printk(KERN_INFO "I2C: Failed to add bus\n");
> - goto eadapt;
> + return ret;
... since you forgot disable_unprepare here and could simply reuse it.
> }
>
> platform_set_drvdata(dev, i2c);
> @@ -1257,20 +1250,6 @@ static int i2c_pxa_probe(struct platform_device *dev)
> dev_name(&i2c->adap.dev));
> #endif
> return 0;
> -
> -eadapt:
> - if (!i2c->use_pio)
> - free_irq(irq, i2c);
> -ereqirq:
> - clk_disable_unprepare(i2c->clk);
> - iounmap(i2c->reg_base);
> -eremap:
> - clk_put(i2c->clk);
> -eclk:
> - kfree(i2c);
> -emalloc:
> - release_mem_region(res->start, resource_size(res));
> - return ret;
> }
You forgot to cleanup the remove function.
BTW I forgot, can you test this patch on real hardware? If not, is there
somebody who does?
All the best,
Wolfram
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4] i2c: busses: i2c-pxa.c: Fix for possible null pointer dereference
2014-08-02 11:30 ` Wolfram Sang
@ 2014-08-16 21:32 ` Rickard Strandqvist
0 siblings, 0 replies; 7+ messages in thread
From: Rickard Strandqvist @ 2014-08-16 21:32 UTC (permalink / raw)
To: Wolfram Sang
Cc: Grant Likely, Rob Herring, Jingoo Han, Leilei Shang,
Peter Korsgaard, linux-i2c, linux-kernel, devicetree
2014-08-02 13:30 GMT+02:00 Wolfram Sang <wsa@the-dreams.de>:
> Hi,
>
>> res = platform_get_resource(dev, IORESOURCE_MEM, 0);
>> irq = platform_get_irq(dev, 0);
>> - if (res == NULL || irq < 0) {
>> - ret = -ENODEV;
>> - goto eclk;
>> - }
>> + if (res == NULL || irq < 0)
>> + return -ENODEV;
>
> No need to check for valid 'res' here...
>
>>
>> - if (!request_mem_region(res->start, resource_size(res), res->name)) {
>> - ret = -ENOMEM;
>> - goto eclk;
>> - }
>> + if (!devm_request_mem_region(&dev->dev, res->start,
>> + resource_size(res), res->name))
>> + return -ENOMEM;
>
> ... and no need to use 'devm_request_mem_region' ...
>
>> - i2c->reg_base = ioremap(res->start, resource_size(res));
>> - if (!i2c->reg_base) {
>> - ret = -EIO;
>> - goto eremap;
>> - }
>> + i2c->reg_base = devm_ioremap_resource(&adev->dev, res));
>> + if (IS_ERR(i2c->reg_base))
>> + return -EIO;
>
> ... because devm_ioremap_resource does all that for you. Also, don't
> return -EIO but the PTR_ERR given from the function. Just check other
> drivers how they do it.
>
>>
>> i2c->reg_ibmr = i2c->reg_base + pxa_reg_layout[i2c_type].ibmr;
>> i2c->reg_idbr = i2c->reg_base + pxa_reg_layout[i2c_type].idbr;
>> @@ -1227,10 +1218,12 @@ static int i2c_pxa_probe(struct platform_device *dev)
>> i2c->adap.algo = &i2c_pxa_pio_algorithm;
>> } else {
>> i2c->adap.algo = &i2c_pxa_algorithm;
>> - ret = request_irq(irq, i2c_pxa_handler, IRQF_SHARED,
>> - dev_name(&dev->dev), i2c);
>> - if (ret)
>> - goto ereqirq;
>> + ret = devm_request_irq(&dev->dev, irq, i2c_pxa_handler,
>> + IRQF_SHARED, dev_name(&dev->dev), i2c);
>> + if (ret) {
>> + clk_disable_unprepare(i2c->clk);
>> + return ret;
>
> Maybe you should make a error path with 'goto' out of it...
>
>
>> + }
>> }
>>
>> i2c_pxa_reset(i2c);
>> @@ -1244,7 +1237,7 @@ static int i2c_pxa_probe(struct platform_device *dev)
>> ret = i2c_add_numbered_adapter(&i2c->adap);
>> if (ret < 0) {
>> printk(KERN_INFO "I2C: Failed to add bus\n");
>> - goto eadapt;
>> + return ret;
>
> ... since you forgot disable_unprepare here and could simply reuse it.
>
>
>> }
>>
>> platform_set_drvdata(dev, i2c);
>> @@ -1257,20 +1250,6 @@ static int i2c_pxa_probe(struct platform_device *dev)
>> dev_name(&i2c->adap.dev));
>> #endif
>> return 0;
>> -
>> -eadapt:
>> - if (!i2c->use_pio)
>> - free_irq(irq, i2c);
>> -ereqirq:
>> - clk_disable_unprepare(i2c->clk);
>> - iounmap(i2c->reg_base);
>> -eremap:
>> - clk_put(i2c->clk);
>> -eclk:
>> - kfree(i2c);
>> -emalloc:
>> - release_mem_region(res->start, resource_size(res));
>> - return ret;
>> }
>
> You forgot to cleanup the remove function.
>
> BTW I forgot, can you test this patch on real hardware? If not, is there
> somebody who does?
>
> All the best,
>
> Wolfram
Hi
I have made a lot of changes on changes to changes, and have really
tried to ensure that all would be right.
But I feel like that's enough, Wolfram or anyone else may end this.
And no as I replied earlier, I lack this kind of hardware...
Kind regards
Rickard Strandqvist
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4] i2c: busses: i2c-pxa.c: Fix for possible null pointer dereference
@ 2014-08-16 21:32 ` Rickard Strandqvist
0 siblings, 0 replies; 7+ messages in thread
From: Rickard Strandqvist @ 2014-08-16 21:32 UTC (permalink / raw)
To: Wolfram Sang
Cc: Grant Likely, Rob Herring, Jingoo Han, Leilei Shang,
Peter Korsgaard, linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
2014-08-02 13:30 GMT+02:00 Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>:
> Hi,
>
>> res = platform_get_resource(dev, IORESOURCE_MEM, 0);
>> irq = platform_get_irq(dev, 0);
>> - if (res == NULL || irq < 0) {
>> - ret = -ENODEV;
>> - goto eclk;
>> - }
>> + if (res == NULL || irq < 0)
>> + return -ENODEV;
>
> No need to check for valid 'res' here...
>
>>
>> - if (!request_mem_region(res->start, resource_size(res), res->name)) {
>> - ret = -ENOMEM;
>> - goto eclk;
>> - }
>> + if (!devm_request_mem_region(&dev->dev, res->start,
>> + resource_size(res), res->name))
>> + return -ENOMEM;
>
> ... and no need to use 'devm_request_mem_region' ...
>
>> - i2c->reg_base = ioremap(res->start, resource_size(res));
>> - if (!i2c->reg_base) {
>> - ret = -EIO;
>> - goto eremap;
>> - }
>> + i2c->reg_base = devm_ioremap_resource(&adev->dev, res));
>> + if (IS_ERR(i2c->reg_base))
>> + return -EIO;
>
> ... because devm_ioremap_resource does all that for you. Also, don't
> return -EIO but the PTR_ERR given from the function. Just check other
> drivers how they do it.
>
>>
>> i2c->reg_ibmr = i2c->reg_base + pxa_reg_layout[i2c_type].ibmr;
>> i2c->reg_idbr = i2c->reg_base + pxa_reg_layout[i2c_type].idbr;
>> @@ -1227,10 +1218,12 @@ static int i2c_pxa_probe(struct platform_device *dev)
>> i2c->adap.algo = &i2c_pxa_pio_algorithm;
>> } else {
>> i2c->adap.algo = &i2c_pxa_algorithm;
>> - ret = request_irq(irq, i2c_pxa_handler, IRQF_SHARED,
>> - dev_name(&dev->dev), i2c);
>> - if (ret)
>> - goto ereqirq;
>> + ret = devm_request_irq(&dev->dev, irq, i2c_pxa_handler,
>> + IRQF_SHARED, dev_name(&dev->dev), i2c);
>> + if (ret) {
>> + clk_disable_unprepare(i2c->clk);
>> + return ret;
>
> Maybe you should make a error path with 'goto' out of it...
>
>
>> + }
>> }
>>
>> i2c_pxa_reset(i2c);
>> @@ -1244,7 +1237,7 @@ static int i2c_pxa_probe(struct platform_device *dev)
>> ret = i2c_add_numbered_adapter(&i2c->adap);
>> if (ret < 0) {
>> printk(KERN_INFO "I2C: Failed to add bus\n");
>> - goto eadapt;
>> + return ret;
>
> ... since you forgot disable_unprepare here and could simply reuse it.
>
>
>> }
>>
>> platform_set_drvdata(dev, i2c);
>> @@ -1257,20 +1250,6 @@ static int i2c_pxa_probe(struct platform_device *dev)
>> dev_name(&i2c->adap.dev));
>> #endif
>> return 0;
>> -
>> -eadapt:
>> - if (!i2c->use_pio)
>> - free_irq(irq, i2c);
>> -ereqirq:
>> - clk_disable_unprepare(i2c->clk);
>> - iounmap(i2c->reg_base);
>> -eremap:
>> - clk_put(i2c->clk);
>> -eclk:
>> - kfree(i2c);
>> -emalloc:
>> - release_mem_region(res->start, resource_size(res));
>> - return ret;
>> }
>
> You forgot to cleanup the remove function.
>
> BTW I forgot, can you test this patch on real hardware? If not, is there
> somebody who does?
>
> All the best,
>
> Wolfram
Hi
I have made a lot of changes on changes to changes, and have really
tried to ensure that all would be right.
But I feel like that's enough, Wolfram or anyone else may end this.
And no as I replied earlier, I lack this kind of hardware...
Kind regards
Rickard Strandqvist
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-08-16 21:32 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-07-16 21:14 [PATCH v4] i2c: busses: i2c-pxa.c: Fix for possible null pointer dereference Rickard Strandqvist
2014-07-16 21:14 ` Rickard Strandqvist
2014-07-16 21:14 ` Rickard Strandqvist
2014-07-16 21:14 ` Rickard Strandqvist
2014-08-02 11:30 ` Wolfram Sang
2014-08-16 21:32 ` Rickard Strandqvist
2014-08-16 21:32 ` Rickard Strandqvist
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.