Hi Tomasz,
On 03/15/2014 02:49 AM, Tomasz Figa wrote:
> Hi Chanwoo,
>
> On 13.03.2014 09:17, Chanwoo Choi wrote:
>> This patch fix bug about resource leak when happening probe fail and code clean
>> to add debug message.
>>
>> Signed-off-by: Chanwoo Choi <cw00.choi@xxxxxxxxxxx>
>> ---
>> drivers/devfreq/exynos/exynos4_bus.c | 32 ++++++++++++++++++++++++++------
>> 1 file changed, 26 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/devfreq/exynos/exynos4_bus.c b/drivers/devfreq/exynos/exynos4_bus.c
>> index a2a3a47..152a3e9 100644
>> --- a/drivers/devfreq/exynos/exynos4_bus.c
>> +++ b/drivers/devfreq/exynos/exynos4_bus.c
>> @@ -1152,8 +1152,11 @@ static int exynos4_busfreq_probe(struct platform_device *pdev)
>> dev_err(dev, "Cannot determine the device id %d\n", data->type);
>> err = -EINVAL;
>> }
>> - if (err)
>> + if (err) {
>> + dev_err(dev, "Cannot initialize busfreq table %d\n",
>> + data->type);
>> return err;
>> + }
>>
>> rcu_read_lock();
>> opp = dev_pm_opp_find_freq_floor(dev,
>> @@ -1176,7 +1179,7 @@ static int exynos4_busfreq_probe(struct platform_device *pdev)
>> if (IS_ERR(data->devfreq)) {
>> dev_err(dev, "Failed to add devfreq device\n");
>> err = PTR_ERR(data->devfreq);
>> - goto err_opp;
>> + goto err_devfreq;
>> }
>>
>> /*
>> @@ -1185,18 +1188,35 @@ static int exynos4_busfreq_probe(struct platform_device *pdev)
>> */
>> busfreq_mon_reset(data);
>>
>> - devfreq_register_opp_notifier(dev, data->devfreq);
>> + /* Register opp_notifier for Exynos4 busfreq */
>> + err = devfreq_register_opp_notifier(dev, data->devfreq);
>> + if (err < 0) {
>> + dev_err(dev, "Failed to register opp notifier\n");
>> + goto err_notifier_opp;
>> + }
>>
>> + /* Register pm_notifier for Exynos4 busfreq */
>> err = register_pm_notifier(&data->pm_notifier);
>> if (err) {
>> dev_err(dev, "Failed to setup pm notifier\n");
>> - devfreq_remove_device(data->devfreq);
>> - return err;
>> + goto err_notifier_pm;
>> }
>>
>> return 0;
>>
>> -err_opp:
>> +err_notifier_pm:
>> + devfreq_unregister_opp_notifier(dev, data->devfreq);
>> +err_notifier_opp:
>> + /*
>> + * The devfreq_remove_device() would execute finally devfreq->profile
>> + * ->exit(). To avoid duplicate resource free operation, return directly
>> + * before executing resource free below 'err_devfreq' goto statement.
>> + */
>
> I'm not quite sure about this. I believe that in this case devfreq->profile->exit() would be exynos4_bus_exit() and all it does is devfreq_unregister_opp_notifier(dev, data->devfreq), so all remaining resources (regulators, clocks, etc.) would get leaked.
This patch execute following sequence to probe exynos4_busfreq.c:
1. Parse dt node to get resource(regulator/clock/memory address).
2. Enable regulator/clock and map memory.
3. Add devfreq device using devfreq_add_device().
The devfreq_add_device() return devfreq instance(data->devfreq).
4. Register opp_notifier using devfreq instance(data->devfreq) which is created in sequence #3.
Case 1,
If an error happens in sequence #3 for registering devfreq_add_device(),
this case can't execute devfreq->profile->exit() to free resource
because this case has failed to register devfreq->profile to devfreq_list.
So, must goto 'err_devfreq' statement to free resource(regulator/clock/memory).
Case 2,
If an error happens in sequence #4 for registering opp_notifier,
In contrast
this case can execute devfreq->profile->exit() to free resource.
But, After executed devfreq->profile->exit(),
should not execute 'err_devfreq' statement to free resource.
In case, will operate twice of resource.
If my explanation is wrong, please reply your comment.
>
> I believe the correct thing to do would be to remove the .exit() callback from exynos4_devfreq_profile struct and handle all the clean-up here in error path.
>
Best Regards,
Chanwoo Choi
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
