lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Wed, 20 Jun 2018 09:50:22 +0900 From: Chanwoo Choi <cw00.choi@...sung.com> To: Enric Balletbo i Serra <enric.balletbo@...labora.com>, Enric Balletbo Serra <eballetbo@...il.com>, cwchoi00@...il.com Cc: linux-kernel <linux-kernel@...r.kernel.org>, kernel@...labora.com, Kyungmin Park <kyungmin.park@...sung.com>, MyungJoo Ham <myungjoo.ham@...sung.com>, Linux PM list <linux-pm@...r.kernel.org> Subject: Re: [PATCH] devfreq: rk3399_dmc: Fix duplicated opp table on reload. 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 <cwchoi00@...il.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@...labora.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@...labora.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.choi@...sung.com> >>>> >>> >>> Thanks, >>> Enric >>> >>>> -- >>>> Best Regards, >>>> Chanwoo Choi >>>> Samsung Electronics >>> >>> >>> >> >> > > > -- Best Regards, Chanwoo Choi Samsung Electronics
Powered by blists - more mailing lists