Re: [PATCH] devfreq: rk3399_dmc: Fix duplicated opp table on reload.

Enric Balletbo i Serra
2018-06-19T08:07:43Z

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 <cwchoi00@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.balletbo@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.balletbo@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.

+ 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.choi@samsung.com>
>>>
>>
>> Thanks,
>> Enric
>>
>>> --
>>> Best Regards,
>>> Chanwoo Choi
>>> Samsung Electronics
>>
>>
>>
>
>