lp8727_charger: free_irq when lp8727_register_psy fail

PATCH In review Devendra Naga 3 participants 6 messages Jul 29, 2012 – Aug 30, 2012

Share & tools

Devendra Naga

author

2012-07-29 17:46 UTC #0 lore raw

PATCH 1/2 lp8727_charger: free_irq when lp8727_register_psy fail

actually the driver does a request_threaded_irq and after this it calls lp8727_register_psy, and if it fails it doesn't free the irqs that it registered to

Signed-off-by Devendra Naga <[email protected]>

---
 drivers/power/lp8727_charger.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

drivers/power/lp8727_charger.c +3 -1

index d8b7578..699f0ef 100644
@@ -454,11 +454,13 @@ static int lp8727_probe(struct i2c_client *cl, const struct i2c_device_id *id)
 	ret = lp8727_register_psy(pchg);
 	if (ret) {
 		dev_err(pchg->dev, "power supplies register err: %d", ret);
-		goto error;
+		goto error_irq;
 	}
 
 	return 0;
 
+error_irq:
+	free_irq(pchg->client->irq, pchg);
 error:
 	kfree(pchg);
 	return ret;

Signature · 1 line

1.7.9.5

Devendra

author

2012-08-19 16:23 UTC #2 lore raw

In reply to Devendra Naga

Sorry Guys on disturbing you,

Will you guys please ACK, NACK or comment on this?

Thanks,

Quoted 28 lines · Devendra Naga

actually the driver does a request_threaded_irq and after this it calls lp8727_register_psy, and if it fails it doesn't free the irqs that it registered to

Signed-off-by Devendra Naga <[email protected]>

---
 drivers/power/lp8727_charger.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

drivers/power/lp8727_charger.c +3 -1

index d8b7578..699f0ef 100644
@@ -454,11 +454,13 @@ static int lp8727_probe(struct i2c_client *cl, const struct i2c_device_id *id)
 	ret = lp8727_register_psy(pchg);
 	if (ret) {
 		dev_err(pchg->dev, "power supplies register err: %d", ret);
-		goto error;
+		goto error_irq;
 	}
 
 	return 0;
 
+error_irq:
+	free_irq(pchg->client->irq, pchg);
 error:
 	kfree(pchg);
 	return ret;

Anton Vorontsov

2012-08-19 20:52 UTC #3 lore raw

Quoted 3 lines · Devendra

Sorry Guys on disturbing you,
Will you guys please ACK, NACK or comment on this?

Technically, it looks OK. But the irq registration logic is sill broken, and the hidden issue is much more serious.

1. lp8727_init_device() -> the function enables interrupts in hw;
2. lp8727_intr_config() -> the function requests interrupts, so CPU is now able to process them;
3. lp8727_register_psy() -> the function allocates power supply objects.

Now, the problem will arise if interrupt comes between "2." and "3.", the interrupt service routine will cause NULL dereference.

So, what you really want to fix the driver is to reorder "2." and "3.", and on error condition call lp8727_unregister_psy().

Thanks,

Quoted 29 lines · Devendra Naga

Quoted 28 lines
actually the driver does a request_threaded_irq and after this it calls lp8727_register_psy, and if it fails it doesn't free the irqs that it registered to
Signed-off-by Devendra Naga <[email protected]>
---
 drivers/power/lp8727_charger.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/power/lp8727_charger.c +3 -1
index d8b7578..699f0ef 100644
@@ -454,11 +454,13 @@ static int lp8727_probe(struct i2c_client *cl, const struct i2c_device_id *id)
 	ret = lp8727_register_psy(pchg);
 	if (ret) {
 		dev_err(pchg->dev, "power supplies register err: %d", ret);
-		goto error;
+		goto error_irq;
 	}
 
 	return 0;
 
+error_irq:
+	free_irq(pchg->client->irq, pchg);
 error:
 	kfree(pchg);
 	return ret;

Devendra Naga

author

2012-08-23 16:52 UTC #4 lore raw

Hi Anton,

Quoted 14 lines · Anton Vorontsov

Quoted 3 lines · Devendra
Sorry Guys on disturbing you,
Will you guys please ACK, NACK or comment on this?
Technically, it looks OK. But the irq registration logic is sill broken, and the hidden issue is much more serious.
1. lp8727_init_device() -> the function enables interrupts in hw;
2. lp8727_intr_config() -> the function requests interrupts, so CPU is now able to process them;
3. lp8727_register_psy() -> the function allocates power supply objects.

Quoted 6 lines

Now, the problem will arise if interrupt comes between "2." and "3.", the interrupt service routine will cause NULL dereference.
So, what you really want to fix the driver is to reorder "2." and "3.", and on error condition call lp8727_unregister_psy().

I think i need to learn more in order to touch this path. will give a try.

Thanks,

Quoted 31 lines

Thanks,
Quoted 29 lines · Devendra Naga
Quoted 28 lines
actually the driver does a request_threaded_irq and after this it calls lp8727_register_psy, and if it fails it doesn't free the irqs that it registered to
Signed-off-by Devendra Naga <[email protected]>
---
 drivers/power/lp8727_charger.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/power/lp8727_charger.c +3 -1
index d8b7578..699f0ef 100644
@@ -454,11 +454,13 @@ static int lp8727_probe(struct i2c_client *cl, const struct i2c_device_id *id)
    ret = lp8727_register_psy(pchg);
    if (ret) {
            dev_err(pchg->dev, "power supplies register err: %d", ret);
-           goto error;
+           goto error_irq;
    }
 
    return 0;
 
+error_irq:
+   free_irq(pchg->client->irq, pchg);
 error:
    kfree(pchg);
    return ret;

Kim, Milo

2012-08-30 11:41 UTC #5 lore raw

Best Regards,
Milo

Quoted 27 lines

-----Original Message-----
From: Devendra Naga [mailto:[email protected]]
Sent: Friday, August 24, 2012 1:52 AM
To: Anton Vorontsov
Cc: David Woodhouse; [email protected]; Kim, Milo
Subject: Re: [PATCH 1/2] lp8727_charger: free_irq when lp8727_register_psy fail
Hi Anton,
Quoted 6 lines · Anton Vorontsov
Quoted 3 lines · Devendra
Sorry Guys on disturbing you,
Will you guys please ACK, NACK or comment on this?
Technically, it looks OK. But the irq registration logic is sill
broken,
Quoted 8 lines
and the hidden issue is much more serious.
1. lp8727_init_device() -> the function enables interrupts in hw;
2. lp8727_intr_config() -> the function requests interrupts, so CPU is now able to process them;
3. lp8727_register_psy() -> the function allocates power supply objects.

Quoted 6 lines

Quoted 4 lines
Now, the problem will arise if interrupt comes between "2." and "3.", the interrupt service routine will cause NULL dereference.
So, what you really want to fix the driver is to reorder "2." and
"3.",
Quoted 1 line
and on error condition call lp8727_unregister_psy().

Please refer to the patch below.
[PATCH 2/8] lp8727_charger: cleanup _probe() and _remove()

Best Regards,
Milo