Message-ID: <20240720052419.73b1415a@5400>
Date: Sat, 20 Jul 2024 05:24:19 -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

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.
>
[...]

> 
> dell_send_request() returns negative value on error. As the read value
> seems to be always non-negative number, you can change API of the
> dell_battery_read_req() function to have read value in the return value
> (instead of in *val pointer). E.g.
> 
> static int dell_battery_read_req(const int type)
> {
> 	...
> 	err = dell_send_request(&buffer, CLASS_TOKEN_READ, SELECT_TOKEN_STD);
> 	if (err)
> 		return err;
> 
> 	return buffer.output[1];
> }
> 

Good call, I'll change that.


> > +
> > +static int dell_battery_write_req(const int type, int val)
> > +{
> > +	struct calling_interface_buffer buffer;
> > +	struct calling_interface_token *token;
> > +
> > +	token = dell_smbios_find_token(type);
> > +	if (!token)
> > +		return -ENODEV;
> > +
> > +	dell_fill_request(&buffer, token->location, val, 0, 0);
> > +	return dell_send_request(&buffer,
> > +			CLASS_TOKEN_WRITE, SELECT_TOKEN_STD);
> > +}
> > +
> > +/* The rules: the minimum start charging value is 50%. The maximum
> > + * start charging value is 95%. The minimum end charging value is
> > + * 55%. The maximum end charging value is 100%. And finally, there
> > + * has to be at least a 5% difference between start & end values.
> > + */
> > +#define CHARGE_START_MIN	50
> > +#define CHARGE_START_MAX	95
> > +#define CHARGE_END_MIN		55
> > +#define CHARGE_END_MAX		100
> > +#define CHARGE_MIN_DIFF		5
> > +
> > +static int dell_battery_custom_set(const int type, int val)
> > +{
> > +	if (type == BAT_CUSTOM_CHARGE_START) {
> > +		int end = CHARGE_END_MAX;
> > +
> > +		if (val < CHARGE_START_MIN)
> > +			val = CHARGE_START_MIN;
> > +		else if (val > CHARGE_START_MAX)
> > +			val = CHARGE_START_MAX;
> > +
> > +		dell_battery_read_req(BAT_CUSTOM_CHARGE_END, &end);  
> 
> Missing check for failure of dell_battery_read_req.

This is intentional; it's just a sanity check, we don't need to bail
if we hit a failure. I'll change the code to make that explicit
though, as it's not currently clear.



> 
> > +		if ((end - val) < CHARGE_MIN_DIFF)
> > +			val = end - CHARGE_MIN_DIFF;
> > +	} else if (type == BAT_CUSTOM_CHARGE_END) {
> > +		int start = CHARGE_START_MIN;
> > +
> > +		if (val < CHARGE_END_MIN)
> > +			val = CHARGE_END_MIN;
> > +		else if (val > CHARGE_END_MAX)
> > +			val = CHARGE_END_MAX;
> > +
> > +		dell_battery_read_req(BAT_CUSTOM_CHARGE_START, &start);  
> 
> Missing check for failure of dell_battery_read_req.
> 

Ditto.


> > +		if ((val - start) < CHARGE_MIN_DIFF)
> > +			val = start + CHARGE_MIN_DIFF;
> > +	}
> > +
> > +	return dell_battery_write_req(type, val);
> > +}
> > +
> > +static int battery_charging_mode_set(enum battery_charging_mode mode)
> > +{
> > +	int err;
> > +
> > +	switch (mode) {
> > +	case DELL_BAT_MODE_STANDARD:
> > +		err = dell_battery_write_req(BAT_STANDARD_MODE_TOKEN, mode);
> > +		break;
> > +	case DELL_BAT_MODE_EXPRESS:
> > +		err = dell_battery_write_req(BAT_EXPRESS_MODE_TOKEN, mode);
> > +		break;
> > +	case DELL_BAT_MODE_PRIMARILY_AC:
> > +		err = dell_battery_write_req(BAT_PRI_AC_MODE_TOKEN, mode);
> > +		break;
> > +	case DELL_BAT_MODE_ADAPTIVE:
> > +		err = dell_battery_write_req(BAT_ADAPTIVE_MODE_TOKEN, mode);
> > +		break;
> > +	case DELL_BAT_MODE_CUSTOM:
> > +		err = dell_battery_write_req(BAT_CUSTOM_MODE_TOKEN, mode);
> > +		break;
> > +	default:
> > +		err = -EINVAL;
> > +	}
> > +
> > +	return err;
> > +}  
> 
> You can make whole function smaller by avoiding err variable:
> 
> static int battery_charging_mode_set(enum battery_charging_mode mode)
> {
> 	switch (mode) {
> 	case DELL_BAT_MODE_STANDARD:
> 		return dell_battery_write_req(BAT_STANDARD_MODE_TOKEN, mode);
> 	case DELL_BAT_MODE_EXPRESS:
> 		return dell_battery_write_req(BAT_EXPRESS_MODE_TOKEN, mode);
> 	case DELL_BAT_MODE_PRIMARILY_AC:
> 		return dell_battery_write_req(BAT_PRI_AC_MODE_TOKEN, mode);
> 	case DELL_BAT_MODE_ADAPTIVE:
> 		return dell_battery_write_req(BAT_ADAPTIVE_MODE_TOKEN, mode);
> 	case DELL_BAT_MODE_CUSTOM:
> 		return dell_battery_write_req(BAT_CUSTOM_MODE_TOKEN, mode);
> 	default:
> 		return -EINVAL;
> 	}
> }
>

Okay, I'll change it.

 
> > +
> > +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),
but perhaps a static_assert or BUILD_BUG_ON to verify that the number of
modes > 0?

  
> > +
> > +	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;
> > +}
> > +
> > +static ssize_t charge_control_start_threshold_show(struct device *dev,
> > +		struct device_attribute *attr,
> > +		char *buf)
> > +{
> > +	int ret, start;
> > +
> > +	ret = dell_battery_read_req(BAT_CUSTOM_CHARGE_START, &start);
> > +	if (!ret)
> > +		ret = sysfs_emit(buf, "%d\n", start);
> > +
> > +	return ret;
> > +}  
> 
> This function and also following 3 functions have unusual error
> handling. Normally error handling is done by early return, as:
> 
>     ret = func1();
>     if (ret)
>         return ret;
> 
>     ret = func2();
>     if (ret)
>         return ret;
> 
>     return 0;
> 
> You can change it something like:
> 
> {
> 	int ret, start;
> 
> 	ret = dell_battery_read_req(BAT_CUSTOM_CHARGE_START, &start);
> 	if (ret)
> 		return ret;
> 
> 	return sysfs_emit(buf, "%d\n", start);
> }
> 

Okay.


> > +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.


> 
> My feeling is that dell_battery_read_req(BAT_CUSTOM_MODE_TOKEN) checks
> if the token BAT_CUSTOM_MODE_TOKEN is set or not.
> 
> Could you please check what is stored in every BAT_*_MODE_TOKEN token at
> this init stage?
> 
> I think it should work similarly, like keyboard backlight tokens as
> implemented in functions: kbd_set_token_bit, kbd_get_token_bit,
> kbd_get_first_active_token_bit.
> 
> > +		bat_chg_current = current_mode;
> > +		battery_hook_register(&dell_battery_hook);
> > +	}
> > +}
> > +
[...]

> >  #define GLOBAL_MUTE_ENABLE	0x058C
> >  #define GLOBAL_MUTE_DISABLE	0x058D
> >  
> > +enum battery_charging_mode {
> > +	DELL_BAT_MODE_NONE = 0,
> > +	DELL_BAT_MODE_STANDARD,
> > +	DELL_BAT_MODE_EXPRESS,
> > +	DELL_BAT_MODE_PRIMARILY_AC,
> > +	DELL_BAT_MODE_ADAPTIVE,
> > +	DELL_BAT_MODE_CUSTOM,
> > +	DELL_BAT_MODE_MAX,
> > +};
> > +  
> 
> I think that this is just an internal driver enum, not Dell API. So this
> enum should be in the dell-laptop.c file.
> 

Agreed, I'll change it.




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

Powered by blists - more mailing lists