Message-ID: <20240720220606.1934df43@5400>
Date: Sat, 20 Jul 2024 22:06:06 -0400
From: Andres Salomon <dilinger@...ued.net>
To: Pali Rohár <pali@...nel.org>
Cc: linux-kernel@...r.kernel.org, platform-driver-x86@...r.kernel.org,
 Matthew Garrett <mjg59@...f.ucam.org>, Sebastian Reichel <sre@...nel.org>,
 Hans de Goede <hdegoede@...hat.com>, Ilpo Järvinen
 <ilpo.jarvinen@...ux.intel.com>, linux-pm@...r.kernel.org,
 Dell.Client.Kernel@...l.com, Mario Limonciello <mario.limonciello@....com>
Subject: Re: [PATCH] platform/x86:dell-laptop: Add knobs to change battery
 charge settings

On Sat, 20 Jul 2024 11:55:07 +0200
Pali Rohár <pali@...nel.org> wrote:

> On Saturday 20 July 2024 05:24:19 Andres Salomon wrote:
> > Thanks for the quick feedback! Responses below.
> > 
> > On Sat, 20 Jul 2024 10:40:19 +0200
> > Pali Rohár <pali@...nel.org> wrote:
> >   
> > > Hello,
> > > 
> > > I looked at your patch. I wrote some comments below. The main issue is
> > > how to correctly interpret read token values.
> > >  
[...]
> > > > +
> > > > +static ssize_t charge_type_show(struct device *dev,
> > > > +		struct device_attribute *attr,
> > > > +		char *buf)
> > > > +{
> > > > +	enum battery_charging_mode mode;
> > > > +	ssize_t count = 0;
> > > > +
> > > > +	for (mode = DELL_BAT_MODE_STANDARD; mode < DELL_BAT_MODE_MAX; mode++) {
> > > > +		if (battery_state[mode]) {
> > > > +			count += sysfs_emit_at(buf, count,
> > > > +				mode == bat_chg_current ? "[%s] " : "%s ",
> > > > +				battery_state[mode]);
> > > > +		}
> > > > +	}
> > > > +
> > > > +	/* convert the last space to a newline */
> > > > +	count--;
> > > > +	count += sysfs_emit_at(buf, count, "\n");    
> > > 
> > > Here is missing protection in the case when number of valid modes is
> > > zero, so count is 0 and buf was untouched.
> > >   
> > 
> > This will never be zero (based on the hardcoded value of DELL_BAT_MODE_MAX),  
> 
> Now I see. You are iterating over members of constant array battery_state[].
> I overlooked it and I thought that this iteration over mode values.
> 
> What about writing the for- conditions to be visible that you are
> iterating over the array? E.g.
> 
> 	size_t i;
> 	...
> 	for (i = 0; i < ARRAY_SIZE(battery_state); i++) {
> 		if (!battery_state[i])
> 			continue;
> 		count += sysfs_emit_at(buf, count, i == bat_chg_current ? "[%s] " : "%s ", battery_state[i]);
> 	}
> 	...
> 
> This has an advantage that you do not depend on DELL_BAT_MODE_MAX value,
> compiler will calculate upper bound automatically.
> 
> Or another way. You can define array of tokens, like it is done for
> keyboard backlight. See how the array kbd_tokens[] is used.
> 
> With this approach you can completely get rid of the DELL_BAT_MODE_MAX.
> 

See below for a question about charge_type_store() if we get rid of
DELL_BAT_MODE_MAX.

> > but perhaps a static_assert or BUILD_BUG_ON to verify that the number of
> > modes > 0?  
> 
> I think it is not needed.
> 
> >     
> > > > +
> > > > +	return count;
> > > > +}
> > > > +
> > > > +static ssize_t charge_type_store(struct device *dev,
> > > > +		struct device_attribute *attr,
> > > > +		const char *buf, size_t size)
> > > > +{
> > > > +	enum battery_charging_mode mode;
> > > > +	const char *label;
> > > > +	int ret = -EINVAL;
> > > > +
> > > > +	for (mode = DELL_BAT_MODE_STANDARD; mode < DELL_BAT_MODE_MAX; mode++) {
> > > > +		label = battery_state[mode];
> > > > +		if (label && sysfs_streq(label, buf))
> > > > +			break;
> > > > +	}
> > > > +
> > > > +	if (mode > DELL_BAT_MODE_NONE && mode < DELL_BAT_MODE_MAX) {
> > > > +		ret = battery_charging_mode_set(mode);
> > > > +		if (!ret) {
> > > > +			bat_chg_current = mode;
> > > > +			ret = size;
> > > > +		}
> > > > +	}
> > > > +
> > > > +	return ret;
> > > > +}

Here we're using DELL_BAT_MODE_MAX to determine if we failed to match
any mode strings sent from the user. If we get rid of that, we're either
going to have to use a separate variable (eg, 'bool matched = false;' and
set it to true in case of a match), or iterate backwards (eg,
for (mode = ARRAY_SIZE(battery_state); mode >= DELL_BAT_MODE_NONE; mode--) {
	...
}
if (mode != DELL_BAT_MODE_NONE) {
	ret = battery_charging_mode_set(mode);


Do you have a preference?


[...]
> > > > +static ssize_t charge_control_start_threshold_store(struct device *dev,
> > > > +		struct device_attribute *attr,
> > > > +		const char *buf, size_t size)
> > > > +{  
> > [...]
> >   
> > > > +
> > > > +static void __init dell_battery_init(struct device *dev)
> > > > +{
> > > > +	enum battery_charging_mode current_mode = DELL_BAT_MODE_NONE;
> > > > +
> > > > +	dell_battery_read_req(BAT_CUSTOM_MODE_TOKEN, (int *) &current_mode);
> > > > +	if (current_mode != DELL_BAT_MODE_NONE) {    
> > > 
> > > I quite do not understand how is this code suppose to work.
> > > 
> > > Why is there mix of custom kernel enum battery_charging_mode and return
> > > value from Dell's API?  
> > 
> > This is from the original patch from Dell; tbh, I'm not sure. It does
> > work, though. That is, current_mode ends up holding the correct value
> > based on what was previously set, even if the charging mode is set from
> > the BIOS.
> > 
> > I just scanned through the libsmbios code to see what it's doing, and
> > it appears to loop through every charging mode to check if its active.
> > I'm not really sure that makes much more sense, so I'll try some more
> > tests.  
> 
> Keyboard backlight code (kbd_get_first_active_token_bit) is doing also
> this type scan. If I remember correctly, for every keyboard backlight
> token we just know the boolean value - if the token is set or not.
> 
> It would really nice to see what (raw) value is returned by the
> dell_battery_read_req(token) function for every battery token and for
> every initial state.

I checked this. The BIOS sets the mode value in every related token
location. I'm still not really sure what libsmbios is doing, but the
kernel code seems to arbitrarily choose one of the token locations
to read from. This makes sense to me now.

In the BIOS when I set the mode to "ExpressCharge",
this what I pulled for each token location:

[    5.704651] dell-laptop dell-laptop: BAT_CUSTOM_MODE_TOKEN value: 2
[    5.707015] dell-laptop dell-laptop: BAT_PRI_AC_MODE_TOKEN value: 2
[    5.709114] dell-laptop dell-laptop: BAT_ADAPTIVE_MODE_TOKEN value: 2
[    5.711041] dell-laptop dell-laptop: BAT_STANDARD_MODE_TOKEN value: 2
[    5.713705] dell-laptop dell-laptop: BAT_EXPRESS_MODE_TOKEN value: 2

Similar story when I set it to Custom (all were '5'), or Standard ('1').
When I set it from linux as well, it changed all location values.

> 
> Because it is really suspicious if dell_battery_read_req() would return
> value of the enum battery_charging_mode (as this is kernel enum).
> 
> 
> Also another important thing. In past it was possible to buy from Dell
> special batteries with long warranty (3+ years). I'm not sure if these
> batteries are still available for end-user customers. With this type of
> battery, it was not possible to change charging mode to ExpressCharge
> (bios option was fade-out). I do not have such battery anymore, but I
> would expect that the firmware disabled BAT_EXPRESS_MODE_TOKEN as mark
> it as unavailable.
> 
> I think that we should scan list of available tokens, like it is done
> for keyboard backlight in kbd_init_tokens(). And export via sysfs only
> those battery modes for which there is available token.

I agree, but I'm not seeing a way to do that right now given how the
BIOS sets the mode. Maybe libsmbios has some clues...





-- 
I'm available for contract & employment work, see:
https://spindle.queued.net/~dilinger/resume-tech.pdf

Powered by blists - more mailing lists