Messages in this thread | | | Subject | Re: [PATCH] devfreq: rk3399_dmc: Fix duplicated opp table on reload. | From | Enric Balletbo i Serra <> | Date | Tue, 19 Jun 2018 10:07:33 +0200 |
| |
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 >> >> >> > >
| |