linux-kernel - Re: [PATCH v2 8/9] power: supply: rt5033_battery: Adopt status property from charger

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <15a1ad2b-c07b-7470-6c4b-2c8feab667c5@rocketmail.com>
Date:   Sun, 23 Apr 2023 11:46:37 +0200
From:   Jakob Hauser <jahau@...ketmail.com>
To:     Sebastian Reichel <sre@...nel.org>
Cc:     Lee Jones <lee@...nel.org>, Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Beomho Seo <beomho.seo@...sung.com>,
        Chanwoo Choi <cw00.choi@...sung.com>,
        Stephan Gerhold <stephan@...hold.net>,
        Raymond Hackley <raymondhackley@...tonmail.com>,
        Pavel Machek <pavel@....cz>, Axel Lin <axel.lin@...ics.com>,
        ChiYuan Huang <cy_huang@...htek.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        linux-pm@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, phone-devel@...r.kernel.org,
        ~postmarketos/upstreaming@...ts.sr.ht
Subject: Re: [PATCH v2 8/9] power: supply: rt5033_battery: Adopt status
 property from charger

Hi Sebastian,

I noticed a small mistake in patch 8.

On 16.04.23 14:44, Jakob Hauser wrote:
> The rt5033-battery fuelgauge can't get a status by itself. The rt5033-charger
> can, let's get this value.
> 
> Tested-by: Raymond Hackley <raymondhackley@...tonmail.com>
> Signed-off-by: Jakob Hauser <jahau@...ketmail.com>
> ---
>   drivers/power/supply/rt5033_battery.c | 24 ++++++++++++++++++++++++
>   1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/power/supply/rt5033_battery.c b/drivers/power/supply/rt5033_battery.c
> index 5c04cf305219..48d4cccce4f6 100644
> --- a/drivers/power/supply/rt5033_battery.c
> +++ b/drivers/power/supply/rt5033_battery.c
> @@ -12,6 +12,26 @@
>   #include <linux/mfd/rt5033-private.h>
>   #include <linux/mfd/rt5033.h>
>   
> +static int rt5033_battery_get_status(struct i2c_client *client)
> +{
> +	struct power_supply *charger;
> +	union power_supply_propval val;
> +	int ret;
> +
> +	charger = power_supply_get_by_name("rt5033-charger");
> +	if (!charger)
> +		return -ENODEV;
> +
> +	ret = power_supply_get_property(charger, POWER_SUPPLY_PROP_STATUS, &val);
> +	if (ret) {
> +		power_supply_put(charger);
> +		return POWER_SUPPLY_STATUS_UNKNOWN;
> +	}
> +
> +	power_supply_put(charger);
> +	return val.intval;
> +}
> +

If the rt5033-charger driver is not available, this function returns 
"-ENODEV". Instead of an error, in fact the status node in sysfs just 
reports "-19" then. Userspace layer UPower makes status "unknown" out of 
this.

An error message would spam dmesg anyway, as it would be issued every 
time the battery gets polled by UPower, which is quite regularly. The 
scenario of a missing rt5033-charger driver is not unlikely for devices 
where it's not yet implemented in the devicetree or in the configs of 
the compiled kernel. For the displayed battery icon, UPower assumes 
"discharging" for a single battery in "unknown" state.

It makes more sense to return "POWER_SUPPLY_STATUS_UNKNOWN" right away. 
I'll change that line in v3.

>   static int rt5033_battery_get_capacity(struct i2c_client *client)
>   {
>   	struct rt5033_battery *battery = i2c_get_clientdata(client);
> @@ -84,6 +104,9 @@ static int rt5033_battery_get_property(struct power_supply *psy,
>   	case POWER_SUPPLY_PROP_CAPACITY:
>   		val->intval = rt5033_battery_get_capacity(battery->client);
>   		break;
> +	case POWER_SUPPLY_PROP_STATUS:
> +		val->intval = rt5033_battery_get_status(battery->client);
> +		break;
>   	default:
>   		return -EINVAL;
>   	}
> @@ -96,6 +119,7 @@ static enum power_supply_property rt5033_battery_props[] = {
>   	POWER_SUPPLY_PROP_VOLTAGE_OCV,
>   	POWER_SUPPLY_PROP_PRESENT,
>   	POWER_SUPPLY_PROP_CAPACITY,
> +	POWER_SUPPLY_PROP_STATUS,
>   };
>   
>   static const struct regmap_config rt5033_battery_regmap_config = {

Kind regards,
Jakob