linux-kernel - Re: [PATCH v4 1/2] platform/x86:dell-laptop: Add knobs to change battery charge settings

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: <20240827142408.0748911f@5400>
Date: Tue, 27 Aug 2024 14:24:08 -0400
From: Andres Salomon <dilinger@...ued.net>
To: Hans de Goede <hdegoede@...hat.com>
Cc: linux-kernel@...r.kernel.org, Thomas Weißschuh
 <linux@...ssschuh.net>, Pali Rohár <pali@...nel.org>,
 platform-driver-x86@...r.kernel.org, Matthew Garrett <mjg59@...f.ucam.org>,
 Sebastian Reichel <sre@...nel.org>, Ilpo Järvinen
 <ilpo.jarvinen@...ux.intel.com>, linux-pm@...r.kernel.org,
 Dell.Client.Kernel@...l.com
Subject: Re: [PATCH v4 1/2] platform/x86:dell-laptop: Add knobs to change
 battery charge settings

On Mon, 26 Aug 2024 16:44:35 +0200
Hans de Goede <hdegoede@...hat.com> wrote:

> Hi Andres,
> 
> On 8/20/24 9:30 AM, Andres Salomon wrote:
[...]
> > +
> > +static ssize_t charge_type_show(struct device *dev,
> > +		struct device_attribute *attr,
> > +		char *buf)
> > +{
> > +	ssize_t count = 0;
> > +	int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(battery_modes); i++) {
> > +		bool active;
> > +
> > +		if (!(battery_supported_modes & BIT(i)))
> > +			continue;
> > +
> > +		active = dell_battery_mode_is_active(battery_modes[i].token);
> > +		count += sysfs_emit_at(buf, count, active ? "[%s] " : "%s ",
> > +				battery_modes[i].label);
> > +	}  
> 
> If you look at the way how charge_type is shown by the power_supply_sysfs.c
> file which is used for power-supply drivers which directly register
> a power-supply themselves rather then extending an existing driver, this
> is not the correct format.
> 
> drivers/power/supply/power_supply_sysfs.c
> 
> lists charge_type as:
> 
>         POWER_SUPPLY_ENUM_ATTR(CHARGE_TYPE),
> 
> and ENUM type properties use the following for show() :
> 
> 	default:
> 		if (ps_attr->text_values_len > 0 &&
> 				value.intval < ps_attr->text_values_len && value.intval >= 0) {
> 			ret = sysfs_emit(buf, "%s\n", ps_attr->text_values[value.intval]);
> 		} else {
> 			ret = sysfs_emit(buf, "%d\n", value.intval);
> 		}
> 	}
> 
> with in this case text_values pointing to:
> 
> static const char * const POWER_SUPPLY_CHARGE_TYPE_TEXT[] = {
> 	[POWER_SUPPLY_CHARGE_TYPE_UNKNOWN]	= "Unknown",
> 	[POWER_SUPPLY_CHARGE_TYPE_NONE]		= "N/A",
> 	[POWER_SUPPLY_CHARGE_TYPE_TRICKLE]	= "Trickle",
> 	[POWER_SUPPLY_CHARGE_TYPE_FAST]		= "Fast",
> 	[POWER_SUPPLY_CHARGE_TYPE_STANDARD]	= "Standard",
> 	[POWER_SUPPLY_CHARGE_TYPE_ADAPTIVE]	= "Adaptive",
> 	[POWER_SUPPLY_CHARGE_TYPE_CUSTOM]	= "Custom",
> 	[POWER_SUPPLY_CHARGE_TYPE_LONGLIFE]	= "Long Life",
> 	[POWER_SUPPLY_CHARGE_TYPE_BYPASS]	= "Bypass",
> };
> 
> So value.intval will be within the expected range hitting:
> 
> 			ret = sysfs_emit(buf, "%s\n", ps_attr->text_values[value.intval]);
> 
> IOW instead of outputting something like this:
> 
> Fast [Standard] Long Life
> 
> which is what your show() function does it outputs only
> the active value as a string, e.g.:
> 
> Standard
> 
> Yes not being able to see the supported values is annoying I actually
> wrote an email about that earlier today:
> 
> https://lore.kernel.org/linux-pm/49993a42-aa91-46bf-acef-4a089db4c2db@redhat.com/
> 
> but we need to make sure that the output is consistent between drivers otherwise
> userspace can never know how to use the API, so for charge_type the dell
> driver should only output the active type, not all the options.

So should I just wait to make any changes until you hear back in that
thread? I'm not overly excited about changing it to use the current
charge_type API, given that the only way to get a list of modes that the
hardware supports is to try setting them all and seeing what fails.

I suppose another option is to rename it to charge_types in the dell
driver under the assumption that your proposed charge_types API (or
something like it) will be added..


> 
> This reminds me that there was a patch-series to allow battery extension drivers
> like this one to actually use the power-supply core code for show()/store()
> Thomas IIRC that series was done by you ?  What is the status of that ?
> 
> Also looking at the userspace API parts of this again I wonder
> if mapping  BAT_PRI_AC_MODE_TOKEN -> "Trickle" is the right thing do
> maybe "Long Life" would be a better match ?  That depends on what the option
> actually does under the hood I guess. Is this known ?
> 

I originally thought to use Long Life rather than Trickle. We discussed
it here:

https://lore.kernel.org/linux-pm/5cfe4c42-a003-4668-8c3a-f18fb6b7fba6@gmx.de/

Based on the existing documentation and the fact that the wilco driver
already mapped it, it was decided to stick with the existing precedent
of using Trickle.

That said, Armin at first suggested creating a new "Primarily AC" entry.
That's personally my favorite option, though I understand if we don't
have to have 50 CHARGE_TYPE entries that just slightly different
variations. :)



-- 
I'm available for contract & employment work, please contact me if
interested.