Hi Tomasz,
On 03/18/2014 09:18 PM, Tomasz Figa wrote:
> On 17.03.2014 06:05, Chanwoo Choi wrote:
>> 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.
>>
>
> OK, it will work indeed, however the code is barely readable with this.
>
> For consistency (and readability), resources acquired in platform driver's .probe() should be freed in .remove() and error path of .probe() should not rely on external code to do the clean-up.
>
> So, as I proposed in my previous reply:
OK, I'll modify it according to your previous 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
