Hi Enric,
On 2018년 06월 19일 17:07, Enric Balletbo i Serra wrote:
> Hi Chanwoo,
>
> On 19/06/18 06:18, Chanwoo Choi wrote:
>> Hi Enric,
>>
>> On 2018년 06월 18일 18:10, Enric Balletbo Serra wrote:
>>> Hi Chanwoo,
>>>
>>> Missatge de Chanwoo Choi <cwcho...@gmail.com> del dia dg., 17 de juny
>>> 2018 a les 5:23:
>>>>
>>>> Hi Enric,
>>>>
>>>> 2018-06-16 0:12 GMT+09:00 Enric Balletbo i Serra
>>>> <enric.balle...@collabora.com>:
>>>>> The opp table is not removed when the driver is unloaded neither when
>>>>> there is an error within probe, so if the driver is reloaded the opp
>>>>> core shows the following warning:
>>>>>
>>>>> rk3399-dmc-freq dmc: _opp_add: duplicate OPPs detected. Existing: freq:
>>>>> 200000000, volt: 900000, enabled: 1. New: freq: 200000000,
>>>>> volt: 900000, enabled: 1
>>>>> rk3399-dmc-freq dmc: _opp_add: duplicate OPPs detected. Existing: freq:
>>>>> 400000000, volt: 900000, enabled: 1. New: freq: 400000000,
>>>>> volt: 900000, enabled: 1
>>>>> rk3399-dmc-freq dmc: _opp_add: duplicate OPPs detected. Existing: freq:
>>>>> 666000000, volt: 900000, enabled: 1. New: freq: 666000000,
>>>>> volt: 900000, enabled: 1
>>>>> rk3399-dmc-freq dmc: _opp_add: duplicate OPPs detected. Existing: freq:
>>>>> 800000000, volt: 900000, enabled: 1. New: freq: 800000000,
>>>>> volt: 900000, enabled: 1
>>>>> rk3399-dmc-freq dmc: _opp_add: duplicate OPPs detected. Existing: freq:
>>>>> 928000000, volt: 900000, enabled: 1. New: freq: 928000000,
>>>>> volt: 900000, enabled: 1
>>>>>
>>>>> This patch fixes the error path in the probe function and adds a .remove
>>>>> function to properly cleanup the opp table on unloading.
>>>>>
>>>>> Fixes: 5a893e31a636c (PM / devfreq: rockchip: add devfreq driver for
>>>>> rk3399 dmc)
>>>>> Signed-off-by: Enric Balletbo i Serra <enric.balle...@collabora.com>
>>>>> ---
>>>>>
>>>>> drivers/devfreq/rk3399_dmc.c | 31 +++++++++++++++++++++++++++----
>>>>> 1 file changed, 27 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c
>>>>> index d5c03e5abe13..e795ad2b3f6b 100644
>>>>> --- a/drivers/devfreq/rk3399_dmc.c
>>>>> +++ b/drivers/devfreq/rk3399_dmc.c
>>>>> @@ -375,8 +375,10 @@ static int rk3399_dmcfreq_probe(struct
>>>>> platform_device *pdev)
>>>>> data->rate = clk_get_rate(data->dmc_clk);
>>>>>
>>>>> opp = devfreq_recommended_opp(dev, &data->rate, 0);
>>>>> - if (IS_ERR(opp))
>>>>> - return PTR_ERR(opp);
>>>>> + if (IS_ERR(opp)) {
>>>>> + ret = PTR_ERR(opp);
>>>>> + goto err_free_opp;
>>>>> + }
>>>>>
>>>>> data->rate = dev_pm_opp_get_freq(opp);
>>>>> data->volt = dev_pm_opp_get_voltage(opp);
>>>>> @@ -388,13 +390,33 @@ static int rk3399_dmcfreq_probe(struct
>>>>> platform_device *pdev)
>>>>> &rk3399_devfreq_dmc_profile,
>>>>> DEVFREQ_GOV_SIMPLE_ONDEMAND,
>>>>> &data->ondemand_data);
>>>>> - if (IS_ERR(data->devfreq))
>>>>> - return PTR_ERR(data->devfreq);
>>>>> + if (IS_ERR(data->devfreq)) {
>>>>> + ret = PTR_ERR(data->devfreq);
>>>>> + goto err_free_opp;
>>>>> + }
>>>>> +
>>>>> devm_devfreq_register_opp_notifier(dev, data->devfreq);
>>>>>
>>>>> data->dev = dev;
>>>>> platform_set_drvdata(pdev, data);
>>>>>
>>>>> + return 0;
>>>>
>>>> It looks strange. Because rk3399_dmcfreq_probe() already include
>>>> 'return 0' when success.
>>>> What is the base commit of this patch?
>>>>
>>>
>>> Sorry, I am not sure I understand your question, If I am not answering
>>> below could you rephrase?
>>
>> When I check the rk3399_dmcfreq_probe()[1], as I commented,
>> rk3399_dmcfreq_probe() already 'return 0' after platform_set_drvdata().
>> You can check it on link[1].
>> [1]
>> https://elixir.bootlin.com/linux/v4.18-rc1/source/drivers/devfreq/rk3399_dmc.c#L443
>>
>> But, this patch add new '+ return 0;' line again in
>> rk3399_dmcfreq_probe().
>> So, just I asked what is base commit of this patch.
>>
>
> I think that this is just how git did the diff and if you only look at the
> diff
> is a bit confusing, if you apply the patch on top of mainline you will see
> that
> there is only one return 0 in the probe function.
Anyway, when I applied it to git, there is no problem.
Just I have never seen such a case and asked a question.
Don't care about this anymore. Thanks.
>
> + return 0; (this new return ...)
> +
> +err_free_opp:
> + dev_pm_opp_of_remove_table(&pdev->dev);
> + return ret;
> +}
> +
> +static int rk3399_dmcfreq_remove(struct platform_device *pdev)
> +{
> + struct rk3399_dmcfreq *dmcfreq = dev_get_drvdata(&pdev->dev);
> +
> + /*
> + * Before remove the opp table we need to unregister the opp notifier.
> + */
> + devm_devfreq_unregister_opp_notifier(dmcfreq->dev, dmcfreq->devfreq);
> + dev_pm_opp_of_remove_table(dmcfreq->dev);
> +
> return 0; (was this before the patch, but now is in another function)
>
> Cheers,
> Enric
>
>>>
>>> So, once the opp table is added we need an error path to free it if an
>>> error occurs later. When the probe returns 0, we need to free the opp
>>> table when we remove the module.
>>>
>>>> [snip]
>>>>
>>>> Anyway, if probe fail, device driver have to remove registered OPP table.
>>>> Looks good to me.
>>>>
>>>> Reviewed-by: Chanwoo Choi <cw00.c...@samsung.com>
>>>>
>>>
>>> Thanks,
>>> Enric
>>>
>>>> --
>>>> Best Regards,
>>>> Chanwoo Choi
>>>> Samsung Electronics
>>>
>>>
>>>
>>
>>
>
>
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
