| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 3 Feb 2023 13:28:25 +0100
From: Sebastian Reichel <sebastian.reichel@...labora.com>
To: Andreas Kemnade <andreas@...nade.info>
Cc: error27@...il.com, rafael.j.wysocki@...el.com,
anton.vorontsov@...aro.org, ramakrishna.pallala@...el.com,
linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org,
hns@...delico.com
Subject: Re: [PATCH] power: supply: disable faulty cooling logic
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@...nade.info>
> ---
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
>
Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)
Powered by blists - more mailing lists