Re: [PATCH] power: supply: disable faulty cooling logic

[Sebastian Reichel]

Hi,

On Sat, Jan 21, 2023 at 12:16:21PM +0100, Andreas Kemnade wrote:
> The rn5t618 power driver fails to register
> a cooling device because POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX
> is missing but availability is not checked before registering
> cooling device. After improved error checking in the thermal
> code, the registration of the power supply fails entirely.
> 
> Checking for availability of _MAX before registering cooling device
> fixes the rn5t618 problem. But the whole logic feels questionable.
> 
> First, the logic is inverted here:
> the code tells: max_current = max_cooling but
> 0 = max_cooling, so there needs to be some inversion
> in the code which cannot be found. Comparing with other
> cooling devices, it can be found that value for fan speed is not
> inverted, value for cpufreq cooling is inverted (similar situation
> as here lowest frequency = max cooling)
> 
> Second, analyzing usage of _MAX: it is seems that maximum capabilities
> of charging controller are specified and not of the battery. Probably
> there is not too much mismatch in the drivers actually implementing
> that. So nothing has exploded yet.  So there is no easy and safe way
> to specifify a max cooling value now.
> 
> Conclusion for now (as a regression fix) just remove the cooling device
> registration and do it properly later on.
> 
> Fixes: e49a1e1ee078 ("thermal/core: fix error code in __thermal_cooling_device_register()")
> Fixes: 952aeeb3ee28 ("power_supply: Register power supply for thermal cooling device")
> Signed-off-by: Andreas Kemnade <andreas@xxxxxxxxxxxx>
> ---

Thanks, queued to for-next.

-- Sebastian

>  drivers/power/supply/power_supply_core.c | 93 ------------------------
>  1 file changed, 93 deletions(-)
> 
> diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
> index 7c790c41e2fe..cc5b2e22b42a 100644
> --- a/drivers/power/supply/power_supply_core.c
> +++ b/drivers/power/supply/power_supply_core.c
> @@ -1186,83 +1186,6 @@ static void psy_unregister_thermal(struct power_supply *psy)
>  	thermal_zone_device_unregister(psy->tzd);
>  }
>  
> -/* thermal cooling device callbacks */
> -static int ps_get_max_charge_cntl_limit(struct thermal_cooling_device *tcd,
> -					unsigned long *state)
> -{
> -	struct power_supply *psy;
> -	union power_supply_propval val;
> -	int ret;
> -
> -	psy = tcd->devdata;
> -	ret = power_supply_get_property(psy,
> -			POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX, &val);
> -	if (ret)
> -		return ret;
> -
> -	*state = val.intval;
> -
> -	return ret;
> -}
> -
> -static int ps_get_cur_charge_cntl_limit(struct thermal_cooling_device *tcd,
> -					unsigned long *state)
> -{
> -	struct power_supply *psy;
> -	union power_supply_propval val;
> -	int ret;
> -
> -	psy = tcd->devdata;
> -	ret = power_supply_get_property(psy,
> -			POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT, &val);
> -	if (ret)
> -		return ret;
> -
> -	*state = val.intval;
> -
> -	return ret;
> -}
> -
> -static int ps_set_cur_charge_cntl_limit(struct thermal_cooling_device *tcd,
> -					unsigned long state)
> -{
> -	struct power_supply *psy;
> -	union power_supply_propval val;
> -	int ret;
> -
> -	psy = tcd->devdata;
> -	val.intval = state;
> -	ret = psy->desc->set_property(psy,
> -		POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT, &val);
> -
> -	return ret;
> -}
> -
> -static const struct thermal_cooling_device_ops psy_tcd_ops = {
> -	.get_max_state = ps_get_max_charge_cntl_limit,
> -	.get_cur_state = ps_get_cur_charge_cntl_limit,
> -	.set_cur_state = ps_set_cur_charge_cntl_limit,
> -};
> -
> -static int psy_register_cooler(struct power_supply *psy)
> -{
> -	/* Register for cooling device if psy can control charging */
> -	if (psy_has_property(psy->desc, POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT)) {
> -		psy->tcd = thermal_cooling_device_register(
> -			(char *)psy->desc->name,
> -			psy, &psy_tcd_ops);
> -		return PTR_ERR_OR_ZERO(psy->tcd);
> -	}
> -
> -	return 0;
> -}
> -
> -static void psy_unregister_cooler(struct power_supply *psy)
> -{
> -	if (IS_ERR_OR_NULL(psy->tcd))
> -		return;
> -	thermal_cooling_device_unregister(psy->tcd);
> -}
>  #else
>  static int psy_register_thermal(struct power_supply *psy)
>  {
> @@ -1272,15 +1195,6 @@ static int psy_register_thermal(struct power_supply *psy)
>  static void psy_unregister_thermal(struct power_supply *psy)
>  {
>  }
> -
> -static int psy_register_cooler(struct power_supply *psy)
> -{
> -	return 0;
> -}
> -
> -static void psy_unregister_cooler(struct power_supply *psy)
> -{
> -}
>  #endif
>  
>  static struct power_supply *__must_check
> @@ -1354,10 +1268,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;
> @@ -1387,8 +1297,6 @@ __power_supply_register(struct device *parent,
>  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:
> @@ -1540,7 +1448,6 @@ void power_supply_unregister(struct power_supply *psy)
>  	sysfs_remove_link(&psy->dev.kobj, "powers");
>  	power_supply_remove_hwmon_sysfs(psy);
>  	power_supply_remove_triggers(psy);
> -	psy_unregister_cooler(psy);
>  	psy_unregister_thermal(psy);
>  	device_init_wakeup(&psy->dev, false);
>  	device_unregister(&psy->dev);
> -- 
> 2.30.2
>

Attachment: signature.asc
Description: PGP signature