[PATCH] OPP: ti: Fix ti_opp_supply_probe wrong return values

[PATCH] OPP: ti: Fix ti_opp_supply_probe wrong return values

[Primoz Fiser]

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 = {
-- 
2.25.1

Re: [PATCH] OPP: ti: Fix ti_opp_supply_probe wrong return values

[Viresh Kumar]

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 ?

-- 
viresh

Re: [PATCH] OPP: ti: Fix ti_opp_supply_probe wrong return values

[Primoz Fiser]

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

Re: [PATCH] OPP: ti: Fix ti_opp_supply_probe wrong return values

[Viresh Kumar]

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.

-- 
viresh

Re: [PATCH] OPP: ti: Fix ti_opp_supply_probe wrong return values

[Primoz Fiser]

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

Re: [PATCH] OPP: ti: Fix ti_opp_supply_probe wrong return values

[Viresh Kumar]

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);
> >  }

-- 
viresh

Re: [PATCH] OPP: ti: Fix ti_opp_supply_probe wrong return values

[Primoz Fiser]

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);
>>>  }
> 

-- 
Primoz Fiser                    | phone: +386-41-390-545
<tel:+386-41-390-545> |
---------------------------------------------------------|
Norik systems d.o.o.            | https://www.norik.com
<https://www.norik.com>  |
Your embedded software partner  | email: info@norik.com
<mailto:info@norik.com> |
Slovenia, EU                    | phone: +386-41-540-545
<tel:+386-41-540-545> |

Re: [PATCH] OPP: ti: Fix ti_opp_supply_probe wrong return values

[Viresh Kumar]

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.

-- 
viresh