| Message ID | 20180615151217.29872-1-enric.balletbo@collabora.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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? [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>
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? 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
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. > > 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 > > >
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 >> >> >> > >
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@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. 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@samsung.com> >>>> >>> >>> Thanks, >>> Enric >>> >>>> -- >>>> Best Regards, >>>> Chanwoo Choi >>>> Samsung Electronics >>> >>> >>> >> >> > > >
Missatge de Chanwoo Choi <cw00.choi@samsung.com> del dia dc., 20 de juny 2018 a les 2:50: > > 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@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. > > 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@samsung.com> > >>>> A gentle ping, although is a fix I think is late and not enough critical to be merged in this release cycle, there is a chance this can be queued for 4.19? Thanks, Enric > >>> > >>> Thanks, > >>> Enric > >>> > >>>> -- > >>>> Best Regards, > >>>> Chanwoo Choi > >>>> Samsung Electronics > >>> > >>> > >>> > >> > >> > > > > > > > > > -- > Best Regards, > Chanwoo Choi > Samsung Electronics
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; + +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; } @@ -406,6 +428,7 @@ MODULE_DEVICE_TABLE(of, rk3399dmc_devfreq_of_match); static struct platform_driver rk3399_dmcfreq_driver = { .probe = rk3399_dmcfreq_probe, + .remove = rk3399_dmcfreq_remove, .driver = { .name = "rk3399-dmc-freq", .pm = &rk3399_dmcfreq_pm,
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(-)