Re: [PATCHv2 4/8] devfreq: exynos4: Fix bug of resource leak and code clean on probe()

[Chanwoo Choi <cw00.choi@samsung.com>]

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@samsung.com>
>> ---
>>   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