Prev: perf: Export trace event utils
Next: gpio: sx150x: Add Semtech I2C sx150x gpio expander driver.
From: Borislav Petkov on 7 Jul 2010 04:20
From: Axel Lin <axel.lin(a)gmail.com>
Date: Wed, Jul 07, 2010 at 10:22:19AM +0800

Good, I like it. Go down for comments :)

> This patch fix resource reclaim in below cases:
> 1. acerhdf_register_platform() does not properly handle
> platform_device_alloc() failure and platform_device_add() failure.
> This patch adds error handling for acerhdf_register_platform().
> 2. acerhdf_register_platform() return err with acerhdf_dev == NULL.
> as a result, acerhdf_unregister_platform() does not do resource
> reclaim in acerhdf_init() error path.
> This patch adds error handing for acerhdf_register_platform(),
> thus correct the error handing path in acerhdf_init().
> goto out_err instead of err_unreg if acerhdf_register_platform() fail.
> 3. platform_device_del() should be only used in error handling.
> Current implementation missed a platform_device_put() in acerhdf_exit.
> This patch fixes it by using platform_device_unregister() instead of
> platform_device_del() in acerhdf_unregister_platform().
>
> Signed-off-by: Axel Lin <axel.lin(a)gmail.com>
> ---
> drivers/platform/x86/acerhdf.c | 20 ++++++++++++++++----
> 1 files changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c
> index 7b2384d..e938a86 100644
> --- a/drivers/platform/x86/acerhdf.c
> +++ b/drivers/platform/x86/acerhdf.c
> @@ -579,9 +579,21 @@ static int acerhdf_register_platform(void)
> return err;
>
> acerhdf_dev = platform_device_alloc("acerhdf", -1);
> - platform_device_add(acerhdf_dev);
> + if (!acerhdf_dev) {
> + err = -ENOMEM;
> + goto err_device_alloc;
> + }
> + err = platform_device_add(acerhdf_dev);
> + if (err)
> + goto err_device_add;
>
> return 0;
> +
> +err_device_add:
> + platform_device_put(acerhdf_dev);
> +err_device_alloc:
> + platform_driver_unregister(&acerhdf_driver);
> + return err;
> }
>
> static void acerhdf_unregister_platform(void)
> @@ -589,7 +601,7 @@ static void acerhdf_unregister_platform(void)
> if (!acerhdf_dev)
> return;

while you're at it, you can remove the above check since
platform_device_del() does that anyway and with your error checking,
acerhdf_dev won't be unitialized in this path.

>
> - platform_device_del(acerhdf_dev);
> + platform_device_unregister(acerhdf_dev);
> platform_driver_unregister(&acerhdf_driver);
> }
>
> @@ -633,7 +645,7 @@ static int __init acerhdf_init(void)
>
> err = acerhdf_register_platform();
> if (err)
> - goto err_unreg;
> + goto out_err;
>
> err = acerhdf_register_thermal();
> if (err)
> @@ -646,7 +658,7 @@ err_unreg:
> acerhdf_unregister_platform();
>
> out_err:
> - return -ENODEV;
> + return err;
> }
>
> static void __exit acerhdf_exit(void)
> --
> 1.5.4.3
>
>
>

--
Regards/Gruss,
Boris.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Borislav Petkov on 7 Jul 2010 08:30
From: Axel Lin <axel.lin(a)gmail.com>
Date: Wed, Jul 07, 2010 at 04:39:31PM +0800

> This patch fix resource reclaim in below cases:
> 1. acerhdf_register_platform() does not properly handle
> platform_device_alloc() failure and platform_device_add() failure
> This patch adds error handing for acerhdf_register_platform().
> 2. acerhdf_register_platform() return err with acerhdf_dev == NULL.
> as a result, acerhdf_unregister_platform() does not do resource
> reclaim in acerhdf_init() error path.
> This patch adds error handing for acerhdf_register_platform(),
> thus correct the error handing path in acerhdf_init().
> goto out_err instead of err_unreg if acerhdf_register_platform() fail.
> 3. platform_device_del() should only used in error handling.
> Current implementation missed a platform_device_put() in acerhdf_exit.
> This patch fixes it by using platform_device_unregister() instead of
> platform_device_del() in acerhdf_unregister_platform.
>
> Signed-off-by: Axel Lin <axel.lin(a)gmail.com>

Ok, I just gave it a run and it looks good. Just a minor nitpick: when
writing your commit messages you don't have to explain what the code
does since we can see it in the patch itself. Rather, your commit
message should explain why you do this.

See http://userweb.kernel.org/~akpm/stuff/tpp.txt for further info on
how to create your patches.

Other than that:

Acked-by: Borislav Petkov <bp(a)alien8.de>

> ---
> drivers/platform/x86/acerhdf.c | 23 ++++++++++++++++-------
> 1 files changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c
> index 7b2384d..d9f5ebb 100644
> --- a/drivers/platform/x86/acerhdf.c
> +++ b/drivers/platform/x86/acerhdf.c
> @@ -579,17 +579,26 @@ static int acerhdf_register_platform(void)
> return err;
>
> acerhdf_dev = platform_device_alloc("acerhdf", -1);
> - platform_device_add(acerhdf_dev);
> + if (!acerhdf_dev) {
> + err = -ENOMEM;
> + goto err_device_alloc;
> + }
> + err = platform_device_add(acerhdf_dev);
> + if (err)
> + goto err_device_add;
>
> return 0;
> +
> +err_device_add:
> + platform_device_put(acerhdf_dev);
> +err_device_alloc:
> + platform_driver_unregister(&acerhdf_driver);
> + return err;
> }
>
> static void acerhdf_unregister_platform(void)
> {
> - if (!acerhdf_dev)
> - return;
> -
> - platform_device_del(acerhdf_dev);
> + platform_device_unregister(acerhdf_dev);
> platform_driver_unregister(&acerhdf_driver);
> }
>
> @@ -633,7 +642,7 @@ static int __init acerhdf_init(void)
>
> err = acerhdf_register_platform();
> if (err)
> - goto err_unreg;
> + goto out_err;
>
> err = acerhdf_register_thermal();
> if (err)
> @@ -646,7 +655,7 @@ err_unreg:
> acerhdf_unregister_platform();
>
> out_err:
> - return -ENODEV;
> + return err;
> }
>
> static void __exit acerhdf_exit(void)
> --
> 1.5.4.3
>
>
>

--
Regards/Gruss,
Boris.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Peter Feuerer on 10 Jul 2010 13:50
Borislav Petkov writes:

> From: Axel Lin <axel.lin(a)gmail.com>
> Date: Wed, Jul 07, 2010 at 04:39:31PM +0800
>
>> This patch fix resource reclaim in below cases:
>> 1. acerhdf_register_platform() does not properly handle
>> platform_device_alloc() failure and platform_device_add() failure
>> This patch adds error handing for acerhdf_register_platform().
>> 2. acerhdf_register_platform() return err with acerhdf_dev == NULL.
>> as a result, acerhdf_unregister_platform() does not do resource
>> reclaim in acerhdf_init() error path.
>> This patch adds error handing for acerhdf_register_platform(),
>> thus correct the error handing path in acerhdf_init().
>> goto out_err instead of err_unreg if acerhdf_register_platform() fail.
>> 3. platform_device_del() should only used in error handling.
>> Current implementation missed a platform_device_put() in acerhdf_exit.
>> This patch fixes it by using platform_device_unregister() instead of
>> platform_device_del() in acerhdf_unregister_platform.
>>
>> Signed-off-by: Axel Lin <axel.lin(a)gmail.com>
>
> Ok, I just gave it a run and it looks good. Just a minor nitpick: when
> writing your commit messages you don't have to explain what the code
> does since we can see it in the patch itself. Rather, your commit
> message should explain why you do this.
>
> See http://userweb.kernel.org/~akpm/stuff/tpp.txt for further info on
> how to create your patches.
>
> Other than that:
>
> Acked-by: Borislav Petkov <bp(a)alien8.de>

Acked-by: Peter Feuerer <peter(a)piie.net>

>
>> ---
>> drivers/platform/x86/acerhdf.c | 23 ++++++++++++++++-------
>> 1 files changed, 16 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c
>> index 7b2384d..d9f5ebb 100644
>> --- a/drivers/platform/x86/acerhdf.c
>> +++ b/drivers/platform/x86/acerhdf.c
>> @@ -579,17 +579,26 @@ static int acerhdf_register_platform(void)
>> return err;
>>
>> acerhdf_dev = platform_device_alloc("acerhdf", -1);
>> - platform_device_add(acerhdf_dev);
>> + if (!acerhdf_dev) {
>> + err = -ENOMEM;
>> + goto err_device_alloc;
>> + }
>> + err = platform_device_add(acerhdf_dev);
>> + if (err)
>> + goto err_device_add;
>>
>> return 0;
>> +
>> +err_device_add:
>> + platform_device_put(acerhdf_dev);
>> +err_device_alloc:
>> + platform_driver_unregister(&acerhdf_driver);
>> + return err;
>> }
>>
>> static void acerhdf_unregister_platform(void)
>> {
>> - if (!acerhdf_dev)
>> - return;
>> -
>> - platform_device_del(acerhdf_dev);
>> + platform_device_unregister(acerhdf_dev);
>> platform_driver_unregister(&acerhdf_driver);
>> }
>>
>> @@ -633,7 +642,7 @@ static int __init acerhdf_init(void)
>>
>> err = acerhdf_register_platform();
>> if (err)
>> - goto err_unreg;
>> + goto out_err;
>>
>> err = acerhdf_register_thermal();
>> if (err)
>> @@ -646,7 +655,7 @@ err_unreg:
>> acerhdf_unregister_platform();
>>
>> out_err:
>> - return -ENODEV;
>> + return err;
>> }
>>
>> static void __exit acerhdf_exit(void)
>> --
>> 1.5.4.3

thank you,
--peter;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 | 
Pages: 1
Prev: perf: Export trace event utils
Next: gpio: sx150x: Add Semtech I2C sx150x gpio expander driver.