[RFC PATCH] power: supply: fix circular dependency with cooling device

[RFC PATCH] power: supply: fix circular dependency with cooling device

[Caleb Connolly]

A recent change in thermal/core means it now calls the cooling device
->get_max_state() callback during __thermal_cooling_device_register().
This creates a circular dependency as it attempts to fetch a power
supply property before the psy is initialised. Move this call later to
break the dependency.

Fixes: c408b3d1d9bb ("thermal: Validate new state in cur_state_store()")
Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
---
 drivers/power/supply/power_supply_core.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
index 7c790c41e2fe..c921111ff26a 100644
--- a/drivers/power/supply/power_supply_core.c
+++ b/drivers/power/supply/power_supply_core.c
@@ -1354,10 +1354,6 @@ __power_supply_register(struct device *parent,
 	if (rc)
 		goto register_thermal_failed;
 
-	rc = psy_register_cooler(psy);
-	if (rc)
-		goto register_cooler_failed;
-
 	rc = power_supply_create_triggers(psy);
 	if (rc)
 		goto create_triggers_failed;
@@ -1378,17 +1374,27 @@ __power_supply_register(struct device *parent,
 	atomic_inc(&psy->use_cnt);
 	psy->initialized = true;
 
+	/* This has to be done after updating use_cnt and initialized
+	 * otherwise when __thermal_cooling_device_register calls back
+	 * to ->get_max_state() the psy core will return -EAGAIN..
+	 */
+	rc = psy_register_cooler(psy);
+	if (rc)
+		goto register_cooler_failed;
+
 	queue_delayed_work(system_power_efficient_wq,
 			   &psy->deferred_register_work,
 			   POWER_SUPPLY_DEFERRED_REGISTER_TIME);
 
 	return psy;
 
+register_cooler_failed:
+	power_supply_remove_hwmon_sysfs(psy);
+	psy->initialized = false;
+	atomic_dec(&psy->use_cnt);
 add_hwmon_sysfs_failed:
 	power_supply_remove_triggers(psy);
 create_triggers_failed:
-	psy_unregister_cooler(psy);
-register_cooler_failed:
 	psy_unregister_thermal(psy);
 register_thermal_failed:
 wakeup_init_failed:
-- 
2.39.0

Re: [RFC PATCH] power: supply: fix circular dependency with cooling device

[Luca Weiss]

On Sonntag, 1. Jänner 2023 18:43:40 CET Caleb Connolly wrote:
> A recent change in thermal/core means it now calls the cooling device
> ->get_max_state() callback during __thermal_cooling_device_register().
> This creates a circular dependency as it attempts to fetch a power
> supply property before the psy is initialised. Move this call later to
> break the dependency.
> 
> Fixes: c408b3d1d9bb ("thermal: Validate new state in cur_state_store()")
> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>

Fixes USB on qcom-apq8026-lg-lenok (with smbb driver)

Tested-by: Luca Weiss <luca@z3ntu.xyz>

Regards
Luca

> ---
>  drivers/power/supply/power_supply_core.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/power/supply/power_supply_core.c
> b/drivers/power/supply/power_supply_core.c index 7c790c41e2fe..c921111ff26a
> 100644
> --- a/drivers/power/supply/power_supply_core.c
> +++ b/drivers/power/supply/power_supply_core.c
> @@ -1354,10 +1354,6 @@ __power_supply_register(struct device *parent,
>  	if (rc)
>  		goto register_thermal_failed;
> 
> -	rc = psy_register_cooler(psy);
> -	if (rc)
> -		goto register_cooler_failed;
> -
>  	rc = power_supply_create_triggers(psy);
>  	if (rc)
>  		goto create_triggers_failed;
> @@ -1378,17 +1374,27 @@ __power_supply_register(struct device *parent,
>  	atomic_inc(&psy->use_cnt);
>  	psy->initialized = true;
> 
> +	/* This has to be done after updating use_cnt and initialized
> +	 * otherwise when __thermal_cooling_device_register calls back
> +	 * to ->get_max_state() the psy core will return -EAGAIN..
> +	 */
> +	rc = psy_register_cooler(psy);
> +	if (rc)
> +		goto register_cooler_failed;
> +
>  	queue_delayed_work(system_power_efficient_wq,
>  			   &psy->deferred_register_work,
>  			   POWER_SUPPLY_DEFERRED_REGISTER_TIME);
> 
>  	return psy;
> 
> +register_cooler_failed:
> +	power_supply_remove_hwmon_sysfs(psy);
> +	psy->initialized = false;
> +	atomic_dec(&psy->use_cnt);
>  add_hwmon_sysfs_failed:
>  	power_supply_remove_triggers(psy);
>  create_triggers_failed:
> -	psy_unregister_cooler(psy);
> -register_cooler_failed:
>  	psy_unregister_thermal(psy);
>  register_thermal_failed:
>  wakeup_init_failed:

Re: [RFC PATCH] power: supply: fix circular dependency with cooling device

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 2687 bytes --]

Hi,

On Thu, Jan 12, 2023 at 10:47:37PM +0100, Luca Weiss wrote:
> On Sonntag, 1. Jänner 2023 18:43:40 CET Caleb Connolly wrote:
> > A recent change in thermal/core means it now calls the cooling device
> > ->get_max_state() callback during __thermal_cooling_device_register().
> > This creates a circular dependency as it attempts to fetch a power
> > supply property before the psy is initialised. Move this call later to
> > break the dependency.
> > 
> > Fixes: c408b3d1d9bb ("thermal: Validate new state in cur_state_store()")
> > Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
> 
> Fixes USB on qcom-apq8026-lg-lenok (with smbb driver)
> 
> Tested-by: Luca Weiss <luca@z3ntu.xyz>
> 
> Regards
> Luca

I queued the following patch instead, which rips out the broken
cooling support:

https://lore.kernel.org/all/20230121111621.2821558-1-andreas@kemnade.info/

-- Sebastian

> 
> > ---
> >  drivers/power/supply/power_supply_core.c | 18 ++++++++++++------
> >  1 file changed, 12 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/power/supply/power_supply_core.c
> > b/drivers/power/supply/power_supply_core.c index 7c790c41e2fe..c921111ff26a
> > 100644
> > --- a/drivers/power/supply/power_supply_core.c
> > +++ b/drivers/power/supply/power_supply_core.c
> > @@ -1354,10 +1354,6 @@ __power_supply_register(struct device *parent,
> >  	if (rc)
> >  		goto register_thermal_failed;
> > 
> > -	rc = psy_register_cooler(psy);
> > -	if (rc)
> > -		goto register_cooler_failed;
> > -
> >  	rc = power_supply_create_triggers(psy);
> >  	if (rc)
> >  		goto create_triggers_failed;
> > @@ -1378,17 +1374,27 @@ __power_supply_register(struct device *parent,
> >  	atomic_inc(&psy->use_cnt);
> >  	psy->initialized = true;
> > 
> > +	/* This has to be done after updating use_cnt and initialized
> > +	 * otherwise when __thermal_cooling_device_register calls back
> > +	 * to ->get_max_state() the psy core will return -EAGAIN..
> > +	 */
> > +	rc = psy_register_cooler(psy);
> > +	if (rc)
> > +		goto register_cooler_failed;
> > +
> >  	queue_delayed_work(system_power_efficient_wq,
> >  			   &psy->deferred_register_work,
> >  			   POWER_SUPPLY_DEFERRED_REGISTER_TIME);
> > 
> >  	return psy;
> > 
> > +register_cooler_failed:
> > +	power_supply_remove_hwmon_sysfs(psy);
> > +	psy->initialized = false;
> > +	atomic_dec(&psy->use_cnt);
> >  add_hwmon_sysfs_failed:
> >  	power_supply_remove_triggers(psy);
> >  create_triggers_failed:
> > -	psy_unregister_cooler(psy);
> > -register_cooler_failed:
> >  	psy_unregister_thermal(psy);
> >  register_thermal_failed:
> >  wakeup_init_failed:

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]