* [PATCH 1/2] lp8727_charger: free_irq when lp8727_register_psy fail
@ 2012-07-29 17:46 Devendra Naga
2012-08-10 4:35 ` Devendra
2012-08-10 5:07 ` Devendra Naga
0 siblings, 2 replies; 6+ messages in thread
From: Devendra Naga @ 2012-07-29 17:46 UTC (permalink / raw)
To: Anton Vorontsov, David Woodhouse, linux-kernel; +Cc: 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 <develkernel412222@gmail.com>
---
drivers/power/lp8727_charger.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/power/lp8727_charger.c b/drivers/power/lp8727_charger.c
index d8b7578..699f0ef 100644
--- a/drivers/power/lp8727_charger.c
+++ b/drivers/power/lp8727_charger.c
@@ -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;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] lp8727_charger: free_irq when lp8727_register_psy fail
2012-07-29 17:46 [PATCH 1/2] lp8727_charger: free_irq when lp8727_register_psy fail Devendra Naga
@ 2012-08-10 4:35 ` Devendra
2012-08-19 20:50 ` Anton Vorontsov
2012-08-10 5:07 ` Devendra Naga
1 sibling, 1 reply; 6+ messages in thread
From: Devendra @ 2012-08-10 4:35 UTC (permalink / raw)
To: Anton Vorontsov; +Cc: David Woodhouse, linux-kernel
Sorry Guys on disturbing you,
Will you guys please ACK, NACK or comment on this?
Thanks,
On Sun, 2012-07-29 at 23:31 +0545, Devendra Naga wrote:
> 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 <develkernel412222@gmail.com>
> ---
> drivers/power/lp8727_charger.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/power/lp8727_charger.c b/drivers/power/lp8727_charger.c
> index d8b7578..699f0ef 100644
> --- a/drivers/power/lp8727_charger.c
> +++ b/drivers/power/lp8727_charger.c
> @@ -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;
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] lp8727_charger: free_irq when lp8727_register_psy fail
2012-07-29 17:46 [PATCH 1/2] lp8727_charger: free_irq when lp8727_register_psy fail Devendra Naga
2012-08-10 4:35 ` Devendra
@ 2012-08-10 5:07 ` Devendra Naga
1 sibling, 0 replies; 6+ messages in thread
From: Devendra Naga @ 2012-08-10 5:07 UTC (permalink / raw)
To: Anton Vorontsov, David Woodhouse, linux-kernel; +Cc: Devendra Naga
Sorry to ask this again,
But will anybody please ACK , NACK or comment on this patch?
Thanks,
Devendra.
On Sun, Jul 29, 2012 at 11:16 PM, Devendra Naga
<develkernel412222@gmail.com> wrote:
> 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 <develkernel412222@gmail.com>
> ---
> drivers/power/lp8727_charger.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/power/lp8727_charger.c b/drivers/power/lp8727_charger.c
> index d8b7578..699f0ef 100644
> --- a/drivers/power/lp8727_charger.c
> +++ b/drivers/power/lp8727_charger.c
> @@ -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;
> --
> 1.7.9.5
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] lp8727_charger: free_irq when lp8727_register_psy fail
2012-08-10 4:35 ` Devendra
@ 2012-08-19 20:50 ` Anton Vorontsov
2012-08-23 16:52 ` Devendra Naga
0 siblings, 1 reply; 6+ messages in thread
From: Anton Vorontsov @ 2012-08-19 20:50 UTC (permalink / raw)
To: Devendra; +Cc: David Woodhouse, linux-kernel, Milo.Kim
On Fri, Aug 10, 2012 at 10:05:55AM +0530, Devendra wrote:
> 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,
> On Sun, 2012-07-29 at 23:31 +0545, Devendra Naga wrote:
> > 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 <develkernel412222@gmail.com>
> > ---
> > drivers/power/lp8727_charger.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/power/lp8727_charger.c b/drivers/power/lp8727_charger.c
> > index d8b7578..699f0ef 100644
> > --- a/drivers/power/lp8727_charger.c
> > +++ b/drivers/power/lp8727_charger.c
> > @@ -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;
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] lp8727_charger: free_irq when lp8727_register_psy fail
2012-08-19 20:50 ` Anton Vorontsov
@ 2012-08-23 16:52 ` Devendra Naga
2012-08-30 11:41 ` Kim, Milo
0 siblings, 1 reply; 6+ messages in thread
From: Devendra Naga @ 2012-08-23 16:52 UTC (permalink / raw)
To: Anton Vorontsov; +Cc: David Woodhouse, linux-kernel, Milo.Kim
Hi Anton,
On Mon, Aug 20, 2012 at 2:20 AM, Anton Vorontsov <cbouatmailru@gmail.com> wrote:
> On Fri, Aug 10, 2012 at 10:05:55AM +0530, Devendra wrote:
>> 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().
>
I think i need to learn more in order to touch this path. will give a try.
Thanks,
> Thanks,
>
>> On Sun, 2012-07-29 at 23:31 +0545, Devendra Naga wrote:
>> > 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 <develkernel412222@gmail.com>
>> > ---
>> > drivers/power/lp8727_charger.c | 4 +++-
>> > 1 file changed, 3 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/power/lp8727_charger.c b/drivers/power/lp8727_charger.c
>> > index d8b7578..699f0ef 100644
>> > --- a/drivers/power/lp8727_charger.c
>> > +++ b/drivers/power/lp8727_charger.c
>> > @@ -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;
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH 1/2] lp8727_charger: free_irq when lp8727_register_psy fail
2012-08-23 16:52 ` Devendra Naga
@ 2012-08-30 11:41 ` Kim, Milo
0 siblings, 0 replies; 6+ messages in thread
From: Kim, Milo @ 2012-08-30 11:41 UTC (permalink / raw)
To: Devendra Naga, Anton Vorontsov; +Cc: David Woodhouse, linux-kernel
Best Regards,
Milo
> -----Original Message-----
> From: Devendra Naga [mailto:develkernel412222@gmail.com]
> Sent: Friday, August 24, 2012 1:52 AM
> To: Anton Vorontsov
> Cc: David Woodhouse; linux-kernel@vger.kernel.org; Kim, Milo
> Subject: Re: [PATCH 1/2] lp8727_charger: free_irq when
> lp8727_register_psy fail
>
> Hi Anton,
>
> On Mon, Aug 20, 2012 at 2:20 AM, Anton Vorontsov
> <cbouatmailru@gmail.com> wrote:
> > On Fri, Aug 10, 2012 at 10:05:55AM +0530, Devendra wrote:
> >> 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().
Please refer to the patch below.
[PATCH 2/8] lp8727_charger: cleanup _probe() and _remove()
Best Regards,
Milo
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-08-30 11:41 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-07-29 17:46 [PATCH 1/2] lp8727_charger: free_irq when lp8727_register_psy fail Devendra Naga
2012-08-10 4:35 ` Devendra
2012-08-19 20:50 ` Anton Vorontsov
2012-08-23 16:52 ` Devendra Naga
2012-08-30 11:41 ` Kim, Milo
2012-08-10 5:07 ` Devendra Naga
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).