Re: question about drivers/power/88pm860x_charger.c

From: anish kumar
Date: Thu Dec 13 2012 - 13:50:52 EST


On Sat, 2012-12-08 at 17:37 +0100, Julia Lawall wrote:
> The function pm860x_charger_probe in the file
> drivers/power/88pm860x_charger.c contains the following code:
>
> count = pdev->num_resources;
> for (i = 0, j = 0; i < count; i++) {
> info->irq[j] = platform_get_irq(pdev, i);
> if (info->irq[j] < 0)
> continue;
> j++;
> }
> info->irq_nums = j;
>
> and then later the following code:
>
> for (i = 0; i < ARRAY_SIZE(info->irq); i++) {
This seems to be wrong.We already know this size which is 7 so why
use ARRAY_SIZE?I think the intention is as below
> ret = request_threaded_irq(info->irq[i], NULL,
> pm860x_irq_descs[i].handler,
> IRQF_ONESHOT, pm860x_irq_descs[i].name, info);
> ...
> }
>
> and finally, in the function pm860x_charger_remove, the code:
>
> free_irq(info->irq[0], info);
> for (i = 0; i < info->irq_nums; i++)
> free_irq(info->irq[i], info);
>
> It looks like the irq_nums field is being used to record how many
> platform_get_irq calls were successful, but this information is not used
> in the second block of code, where request_threaded_irq is called. So it
> would seem that all of the requested irqs should be freed, and not just
> the first irq_nums of them.
>
> The remove code also looks like a double free of info->irq[0].
>
> Could I just get rid of the irq_nums field completely? It doesn't seem to
diff --git a/drivers/power/88pm860x_charger.c
b/drivers/power/88pm860x_charger.c
index 2dbeb14..821629f 100644
--- a/drivers/power/88pm860x_charger.c
+++ b/drivers/power/88pm860x_charger.c
@@ -698,7 +698,7 @@ static __devinit int pm860x_charger_probe(struct
platform_device *pdev)

pm860x_init_charger(info);

- for (i = 0; i < ARRAY_SIZE(info->irq); i++) {
+ for (i = 0; i < info->irq_nums; i++) {
ret = request_threaded_irq(info->irq[i], NULL,
pm860x_irq_descs[i].handler,
IRQF_ONESHOT, pm860x_irq_descs[i].name, info);

> be used elsewhere. Also, I was planning to use devm_request_threaded_irq,
> so then there won't be a need for the explicit frees at all.

>
> This file also contains a kfree of devm_kzalloc'd data, which is why I
> looked at it in the first place.
>
> thanks,
> julia
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/