* [PATCH] devfreq: rk3399_dmc: Fix duplicated opp table on reload.
@ 2018-06-15 15:12 Enric Balletbo i Serra
2018-06-17 3:23 ` Chanwoo Choi
0 siblings, 1 reply; 7+ messages in thread
From: Enric Balletbo i Serra @ 2018-06-15 15:12 UTC (permalink / raw)
To: linux-kernel; +Cc: kernel, Chanwoo Choi, Kyungmin Park, MyungJoo Ham, linux-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(-)
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,
--
2.17.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH] devfreq: rk3399_dmc: Fix duplicated opp table on reload. 2018-06-15 15:12 [PATCH] devfreq: rk3399_dmc: Fix duplicated opp table on reload Enric Balletbo i Serra @ 2018-06-17 3:23 ` Chanwoo Choi 2018-06-18 9:10 ` Enric Balletbo Serra 0 siblings, 1 reply; 7+ messages in thread From: Chanwoo Choi @ 2018-06-17 3:23 UTC (permalink / raw) To: Enric Balletbo i Serra Cc: linux-kernel, kernel, Chanwoo Choi, Kyungmin Park, MyungJoo Ham, Linux PM list 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> -- Best Regards, Chanwoo Choi Samsung Electronics ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] devfreq: rk3399_dmc: Fix duplicated opp table on reload. 2018-06-17 3:23 ` Chanwoo Choi @ 2018-06-18 9:10 ` Enric Balletbo Serra 2018-06-19 4:18 ` Chanwoo Choi 0 siblings, 1 reply; 7+ messages in thread From: Enric Balletbo Serra @ 2018-06-18 9:10 UTC (permalink / raw) To: cwchoi00 Cc: Enric Balletbo i Serra, linux-kernel, kernel, Chanwoo Choi, Kyungmin Park, MyungJoo Ham, Linux PM list 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] devfreq: rk3399_dmc: Fix duplicated opp table on reload. 2018-06-18 9:10 ` Enric Balletbo Serra @ 2018-06-19 4:18 ` Chanwoo Choi 2018-06-19 8:07 ` Enric Balletbo i Serra 0 siblings, 1 reply; 7+ messages in thread From: Chanwoo Choi @ 2018-06-19 4:18 UTC (permalink / raw) To: Enric Balletbo Serra, cwchoi00 Cc: Enric Balletbo i Serra, linux-kernel, kernel, Kyungmin Park, MyungJoo Ham, Linux PM list 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 > > > -- Best Regards, Chanwoo Choi Samsung Electronics ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] devfreq: rk3399_dmc: Fix duplicated opp table on reload. 2018-06-19 4:18 ` Chanwoo Choi @ 2018-06-19 8:07 ` Enric Balletbo i Serra 2018-06-20 0:50 ` Chanwoo Choi 0 siblings, 1 reply; 7+ messages in thread From: Enric Balletbo i Serra @ 2018-06-19 8:07 UTC (permalink / raw) To: Chanwoo Choi, Enric Balletbo Serra, cwchoi00 Cc: linux-kernel, kernel, Kyungmin Park, MyungJoo Ham, Linux PM list 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 >> >> >> > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] devfreq: rk3399_dmc: Fix duplicated opp table on reload. 2018-06-19 8:07 ` Enric Balletbo i Serra @ 2018-06-20 0:50 ` Chanwoo Choi 2018-07-17 13:17 ` Enric Balletbo Serra 0 siblings, 1 reply; 7+ messages in thread From: Chanwoo Choi @ 2018-06-20 0:50 UTC (permalink / raw) To: Enric Balletbo i Serra, Enric Balletbo Serra, cwchoi00 Cc: linux-kernel, kernel, Kyungmin Park, MyungJoo Ham, Linux PM list 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 >>> >>> >>> >> >> > > > -- Best Regards, Chanwoo Choi Samsung Electronics ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] devfreq: rk3399_dmc: Fix duplicated opp table on reload. 2018-06-20 0:50 ` Chanwoo Choi @ 2018-07-17 13:17 ` Enric Balletbo Serra 0 siblings, 0 replies; 7+ messages in thread From: Enric Balletbo Serra @ 2018-07-17 13:17 UTC (permalink / raw) To: Chanwoo Choi Cc: Enric Balletbo i Serra, cwchoi00, linux-kernel, kernel, Kyungmin Park, MyungJoo Ham, Linux PM list 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-07-17 13:17 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-06-15 15:12 [PATCH] devfreq: rk3399_dmc: Fix duplicated opp table on reload Enric Balletbo i Serra 2018-06-17 3:23 ` Chanwoo Choi 2018-06-18 9:10 ` Enric Balletbo Serra 2018-06-19 4:18 ` Chanwoo Choi 2018-06-19 8:07 ` Enric Balletbo i Serra 2018-06-20 0:50 ` Chanwoo Choi 2018-07-17 13:17 ` Enric Balletbo Serra
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).