| Message ID | 20240606070127.3506240-1-primoz.fiser@norik.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | viresh kumar |
| Headers | show |
| Series | OPP: ti: Fix ti_opp_supply_probe wrong return values | expand |
On 06-06-24, 09:01, Primoz Fiser wrote: > Function ti_opp_supply_probe() since commit 6baee034cb55 ("OPP: ti: > Migrate to dev_pm_opp_set_config_regulators()") returns wrong values > when all goes well and hence driver probing eventually fails. > > Fixes: 6baee034cb55 ("OPP: ti: Migrate to dev_pm_opp_set_config_regulators()") > Signed-off-by: Primoz Fiser <primoz.fiser@norik.com> > --- > drivers/opp/ti-opp-supply.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/opp/ti-opp-supply.c b/drivers/opp/ti-opp-supply.c > index e3b97cd1fbbf..ec0056a4bb13 100644 > --- a/drivers/opp/ti-opp-supply.c > +++ b/drivers/opp/ti-opp-supply.c > @@ -393,10 +393,12 @@ static int ti_opp_supply_probe(struct platform_device *pdev) > } > > ret = dev_pm_opp_set_config_regulators(cpu_dev, ti_opp_config_regulators); > - if (ret < 0) > + if (ret < 0) { > _free_optimized_voltages(dev, &opp_data); > + return ret; > + } > > - return ret; > + return 0; > } > > static struct platform_driver ti_opp_supply_driver = { Not sure I understand the problem here. Can you please explain with an example ?
Hi Viresh, On 6. 06. 24 10:59, Viresh Kumar wrote: > On 06-06-24, 09:01, Primoz Fiser wrote: >> Function ti_opp_supply_probe() since commit 6baee034cb55 ("OPP: ti: >> Migrate to dev_pm_opp_set_config_regulators()") returns wrong values >> when all goes well and hence driver probing eventually fails. >> >> Fixes: 6baee034cb55 ("OPP: ti: Migrate to dev_pm_opp_set_config_regulators()") >> Signed-off-by: Primoz Fiser <primoz.fiser@norik.com> >> --- >> drivers/opp/ti-opp-supply.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/opp/ti-opp-supply.c b/drivers/opp/ti-opp-supply.c >> index e3b97cd1fbbf..ec0056a4bb13 100644 >> --- a/drivers/opp/ti-opp-supply.c >> +++ b/drivers/opp/ti-opp-supply.c >> @@ -393,10 +393,12 @@ static int ti_opp_supply_probe(struct platform_device *pdev) >> } >> >> ret = dev_pm_opp_set_config_regulators(cpu_dev, ti_opp_config_regulators); >> - if (ret < 0) >> + if (ret < 0) { >> _free_optimized_voltages(dev, &opp_data); >> + return ret; >> + } >> >> - return ret; >> + return 0; >> } >> >> static struct platform_driver ti_opp_supply_driver = { > > Not sure I understand the problem here. Can you please explain with an > example ? ti_opp_supply_probe -> dev_pm_opp_set_config_regulators -> dev_pm_opp_set_config (returns negative if error, otherwise >= 1) Lets assume dev_pm_opp_set_config returns 1 (SUCCESS): so now, without my patch: ti_opp_supply_probe returns 1 -> FAILURE and hence error: ti_opp_supply 4a003b20.opp-supply: probe with driver ti_opp_supply failed with error 1 with my patch: ti_opp_supply_probe returns 0 -> SUCCESS BR, Primoz
On 06-06-24, 11:43, Primoz Fiser wrote: > ti_opp_supply_probe > -> dev_pm_opp_set_config_regulators > -> dev_pm_opp_set_config (returns negative if error, otherwise >= 1) Ah, I forgot about the token that is returned here. I think the driver should be fixed properly once and for all here. The token must be used to clean the module removal part. Else if you try to insert this module, remove it, insert again, you will get some errors as the resources aren't cleaned well. So either provide a remove() callback to the driver, or use devm_pm_opp_set_config() here.
Hi Viresh, On 6. 06. 24 11:48, Viresh Kumar wrote: > On 06-06-24, 11:43, Primoz Fiser wrote: >> ti_opp_supply_probe >> -> dev_pm_opp_set_config_regulators >> -> dev_pm_opp_set_config (returns negative if error, otherwise >= 1) > > Ah, I forgot about the token that is returned here. I think the driver > should be fixed properly once and for all here. > > The token must be used to clean the module removal part. Else if you > try to insert this module, remove it, insert again, you will get some > errors as the resources aren't cleaned well. > > So either provide a remove() callback to the driver, or use > devm_pm_opp_set_config() here. > So basically, you want v2 with: > diff --git a/drivers/opp/ti-opp-supply.c b/drivers/opp/ti-opp-supply.c > index e3b97cd1fbbf..8a4bcc5fb9dc 100644 > --- a/drivers/opp/ti-opp-supply.c > +++ b/drivers/opp/ti-opp-supply.c > @@ -392,7 +392,7 @@ static int ti_opp_supply_probe(struct platform_device *pdev) > return ret; > } > > - ret = dev_pm_opp_set_config_regulators(cpu_dev, ti_opp_config_regulators); > + ret = devm_pm_opp_set_config_regulators(cpu_dev, ti_opp_config_regulators); > if (ret < 0) > _free_optimized_voltages(dev, &opp_data); > > diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h > index dd7c8441af42..a2401878d1d9 100644 > --- a/include/linux/pm_opp.h > +++ b/include/linux/pm_opp.h > @@ -654,14 +654,14 @@ static inline int devm_pm_opp_set_clkname(struct device *dev, const char *name) > } > > /* config-regulators helpers */ > -static inline int dev_pm_opp_set_config_regulators(struct device *dev, > +static inline int devm_pm_opp_set_config_regulators(struct device *dev, > config_regulators_t helper) > { > struct dev_pm_opp_config config = { > .config_regulators = helper, > }; > > - return dev_pm_opp_set_config(dev, &config); > + return devm_pm_opp_set_config(dev, &config); > } > > static inline void dev_pm_opp_put_config_regulators(int token) Right? BR, Primoz
On 06-06-24, 12:34, Primoz Fiser wrote: > > Ah, I forgot about the token that is returned here. I think the driver > > should be fixed properly once and for all here. > > > > The token must be used to clean the module removal part. Else if you > > try to insert this module, remove it, insert again, you will get some > > errors as the resources aren't cleaned well. > > > > So either provide a remove() callback to the driver, or use > > devm_pm_opp_set_config() here. > > > > So basically, you want v2 with: > > > diff --git a/drivers/opp/ti-opp-supply.c b/drivers/opp/ti-opp-supply.c > > index e3b97cd1fbbf..8a4bcc5fb9dc 100644 > > --- a/drivers/opp/ti-opp-supply.c > > +++ b/drivers/opp/ti-opp-supply.c > > @@ -392,7 +392,7 @@ static int ti_opp_supply_probe(struct platform_device *pdev) > > return ret; > > } > > > > - ret = dev_pm_opp_set_config_regulators(cpu_dev, ti_opp_config_regulators); > > + ret = devm_pm_opp_set_config_regulators(cpu_dev, ti_opp_config_regulators); > > if (ret < 0) > > _free_optimized_voltages(dev, &opp_data); > > > > diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h > > index dd7c8441af42..a2401878d1d9 100644 > > --- a/include/linux/pm_opp.h > > +++ b/include/linux/pm_opp.h > > @@ -654,14 +654,14 @@ static inline int devm_pm_opp_set_clkname(struct device *dev, const char *name) > > } > > > > /* config-regulators helpers */ > > -static inline int dev_pm_opp_set_config_regulators(struct device *dev, Don't remove this, add a new devm_ counterpart. > > +static inline int devm_pm_opp_set_config_regulators(struct device *dev, > > config_regulators_t helper) > > { > > struct dev_pm_opp_config config = { > > .config_regulators = helper, > > }; > > > > - return dev_pm_opp_set_config(dev, &config); > > + return devm_pm_opp_set_config(dev, &config); > > }
Hi, sent new series instead of v2. Link: https://lore.kernel.org/linux-pm/20240606113334.396693-2-primoz.fiser@norik.com/ BR, Primoz On 6. 06. 24 12:58, Viresh Kumar wrote: > On 06-06-24, 12:34, Primoz Fiser wrote: >>> Ah, I forgot about the token that is returned here. I think the driver >>> should be fixed properly once and for all here. >>> >>> The token must be used to clean the module removal part. Else if you >>> try to insert this module, remove it, insert again, you will get some >>> errors as the resources aren't cleaned well. >>> >>> So either provide a remove() callback to the driver, or use >>> devm_pm_opp_set_config() here. >>> >> >> So basically, you want v2 with: >> >>> diff --git a/drivers/opp/ti-opp-supply.c b/drivers/opp/ti-opp-supply.c >>> index e3b97cd1fbbf..8a4bcc5fb9dc 100644 >>> --- a/drivers/opp/ti-opp-supply.c >>> +++ b/drivers/opp/ti-opp-supply.c >>> @@ -392,7 +392,7 @@ static int ti_opp_supply_probe(struct platform_device *pdev) >>> return ret; >>> } >>> >>> - ret = dev_pm_opp_set_config_regulators(cpu_dev, ti_opp_config_regulators); >>> + ret = devm_pm_opp_set_config_regulators(cpu_dev, ti_opp_config_regulators); >>> if (ret < 0) >>> _free_optimized_voltages(dev, &opp_data); >>> >>> diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h >>> index dd7c8441af42..a2401878d1d9 100644 >>> --- a/include/linux/pm_opp.h >>> +++ b/include/linux/pm_opp.h >>> @@ -654,14 +654,14 @@ static inline int devm_pm_opp_set_clkname(struct device *dev, const char *name) >>> } >>> >>> /* config-regulators helpers */ >>> -static inline int dev_pm_opp_set_config_regulators(struct device *dev, > > Don't remove this, add a new devm_ counterpart. > >>> +static inline int devm_pm_opp_set_config_regulators(struct device *dev, >>> config_regulators_t helper) >>> { >>> struct dev_pm_opp_config config = { >>> .config_regulators = helper, >>> }; >>> >>> - return dev_pm_opp_set_config(dev, &config); >>> + return devm_pm_opp_set_config(dev, &config); >>> } >
On 06-06-24, 09:01, Primoz Fiser wrote: > Function ti_opp_supply_probe() since commit 6baee034cb55 ("OPP: ti: > Migrate to dev_pm_opp_set_config_regulators()") returns wrong values > when all goes well and hence driver probing eventually fails. > > Fixes: 6baee034cb55 ("OPP: ti: Migrate to dev_pm_opp_set_config_regulators()") > Signed-off-by: Primoz Fiser <primoz.fiser@norik.com> > --- > drivers/opp/ti-opp-supply.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/opp/ti-opp-supply.c b/drivers/opp/ti-opp-supply.c > index e3b97cd1fbbf..ec0056a4bb13 100644 > --- a/drivers/opp/ti-opp-supply.c > +++ b/drivers/opp/ti-opp-supply.c > @@ -393,10 +393,12 @@ static int ti_opp_supply_probe(struct platform_device *pdev) > } > > ret = dev_pm_opp_set_config_regulators(cpu_dev, ti_opp_config_regulators); > - if (ret < 0) > + if (ret < 0) { > _free_optimized_voltages(dev, &opp_data); > + return ret; > + } > > - return ret; > + return 0; > } Applied. Thanks.
diff --git a/drivers/opp/ti-opp-supply.c b/drivers/opp/ti-opp-supply.c index e3b97cd1fbbf..ec0056a4bb13 100644 --- a/drivers/opp/ti-opp-supply.c +++ b/drivers/opp/ti-opp-supply.c @@ -393,10 +393,12 @@ static int ti_opp_supply_probe(struct platform_device *pdev) } ret = dev_pm_opp_set_config_regulators(cpu_dev, ti_opp_config_regulators); - if (ret < 0) + if (ret < 0) { _free_optimized_voltages(dev, &opp_data); + return ret; + } - return ret; + return 0; } static struct platform_driver ti_opp_supply_driver = {
Function ti_opp_supply_probe() since commit 6baee034cb55 ("OPP: ti: Migrate to dev_pm_opp_set_config_regulators()") returns wrong values when all goes well and hence driver probing eventually fails. Fixes: 6baee034cb55 ("OPP: ti: Migrate to dev_pm_opp_set_config_regulators()") Signed-off-by: Primoz Fiser <primoz.fiser@norik.com> --- drivers/opp/ti-opp-supply.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)