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

[Armin Wolf <W_Armin@gmx.de>]

Am 26.07.24 um 00:15 schrieb Pali Rohár:

> On Thursday 25 July 2024 16:24:57 Andres Salomon wrote:
>> On Thu, 25 Jul 2024 01:01:58 +0200
>> Pali Rohár <pali@kernel.org> wrote:
>>
>>> On Wednesday 24 July 2024 18:23:18 Andres Salomon wrote:
>>>> On Wed, 24 Jul 2024 22:45:23 +0200
>>>> Pali Rohár <pali@kernel.org> wrote:
>>>>
>>>>> On Wednesday 24 July 2024 22:34:03 Pali Rohár wrote:
>>>>>> Hello, the driver change looks good. I have just few minor comments for
>>>>>> this change below.
>>>>>>
>>>>>> Anyway, if there is somebody on the list with Dell laptop with 2 or 3
>>>>>> batteries, see below, it would be nice to check how such laptop would
>>>>>> behave with this patch. It does not seem that patch should cause
>>>>>> regression but always it is better to do testing if it is possible.
>>>>>>
>>>>>> On Tuesday 23 July 2024 22:05:02 Andres Salomon wrote:
>>>> [...]
>>>>> And because CLASS_TOKEN_WRITE is being repeated, what about defining
>>>>> something like this?
>>>>>
>>>>>      static inline int dell_set_token_value(struct calling_interface_buffer *buffer, u16 class, u16 tokenid, u32 value)
>>>>>      {
>>>>>          dell_send_request_for_tokenid(buffer, class, CLASS_TOKEN_WRITE, tokenid, value);
>>>>>      }
>>>>>
>>>>> Just an idea. Do you think that it could be useful in second patch?
>>>>>
>>>> Let me implement the other changes first and then take a look.
>>> Ok.
>>>
>> For the helper function, I noticed that all of the CLASS_TOKEN_WRITEs
>> also have SELECT_TOKEN_STD except for one (dell_send_intensity). So I
>> think it makes sense to have the helper function also do that as well.
>> Eg,
>>
>> static inline int dell_set_std_token_value(struct calling_interface_buffer *buffer, u16 tokenid, u32 value)
>> {
>> 	dell_send_request_for_tokenid(buffer, SELECT_TOKEN_STD, CLASS_TOKEN_WRITE, tokenid, value);
>> }
>>
>> I agree with your renaming to dell_send_request_for_tokenid, btw.
>>
>>
>>>>>>> +static int dell_battery_read(const int type)
>>>>>>> +{
>>>>>>> +	struct calling_interface_buffer buffer;
>>>>>>> +	int err;
>>>>>>> +
>>>>>>> +	err = dell_send_request_by_token_loc(&buffer, CLASS_TOKEN_READ,
>>>>>>> +			SELECT_TOKEN_STD, type, 0);
>>>>>>> +	if (err)
>>>>>>> +		return err;
>>>>>>> +
>>>>>>> +	return buffer.output[1];
>>>>>> buffer.output[1] is of type u32. So theoretically it can contain value
>>>>>> above 2^31. For safety it would be better to check if the output[1]
>>>>>> value fits into INT_MAX and if not then return something like -ERANGE
>>>>>> (or some other better errno code).
>>
>> I ended up returning -EIO here, with the logic that if we're getting
>> some nonsense value from SMBIOS, it could either be junk in the stored
>> values or communication corruption.
>>
>> Likewise, I used -EIO for the checks in charge_control_start_threshold_show
>> and charge_control_end_threshold_show when SMBIOS returns values > 100%.
>>
>>
>>
>>>>
>>>>>>
>>>>>>> +	if (end < 0)
>>>>>>> +		end = CHARGE_END_MAX;
>>>>>>> +	if ((end - start) < CHARGE_MIN_DIFF)
>>>>>> nit: I'm not sure what is the correct coding style for kernel drivers
>>>>>> but I thought that parenthesis around (end - start) are not being
>>>>>> written.
>>>>>>
>> I think it makes the comparison much easier to read. I'd prefer to
>> keep it, unless the coding style specifically forbids it.
> As I said I'm really not sure. So if nobody would complain then you can
> let it as is.
>
> You can use ./scripts/checkpatch.pl application which is in git tree,
> which does basic checks for code style. It cannot prove if something is
> really correct but it can prove if something is incorrect.
>
>>
>>
>>>>>>> +
>>>>>>> +static u32 __init battery_get_supported_modes(void)
>>>>>>> +{
>>>>>>> +	u32 modes = 0;
>>>>>>> +	int i;
>>>>>>> +
>>>>>>> +	for (i = 0; i < ARRAY_SIZE(battery_modes); i++) {
>>>>>>> +		if (dell_smbios_find_token(battery_modes[i].token))
>>>>>>> +			modes |= BIT(i);
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	return modes;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static void __init dell_battery_init(struct device *dev)
>>>>>>> +{
>>>>>>> +	battery_supported_modes = battery_get_supported_modes();
>>>>>>> +
>>>>>>> +	if (battery_supported_modes != 0)
>>>>>>> +		battery_hook_register(&dell_battery_hook);
>>>>>> Anyway, how is this battery_hook_register() suppose to work on systems
>>>>>> with multiple batteries? The provided API is only for the primary
>>>>>> battery, but on older Dell laptop it was possible to connect up to
>>>>>> 3 batteries. Would not this case some issues?
>>>> This interface is _only_ for controlling charging of the primary battery.
>>>> In theory, it should hopefully ignore any other batteries, which would
>>>> need to have their settings changed in the BIOS or with a special tool or
>>>> whatever.
>>> That is OK. But where it is specified that the hook is being registered
>>> only for the primary battery? Because from the usage it looks like that
>>> the hook is applied either for all batteries present in the system or
>>> for some one arbitrary chosen battery.
>>>
>>>> However, I'm basing that assumption on the SMBIOS interface. These tokens
>>>> are marked "Primary Battery". There are separate tokens marked "Battery
>>>> Slice", which from my understanding was an older type of battery that
>>>  From SMBIOS perspective it is clear, each battery seems to have its own
>>> tokens.
>>>
>>> The issue here is: how to tell kernel that the particular
>>> dell_battery_hook has to be bound with the primary battery?
>>>
>> So from userspace, we've got the expectation that multiple batteries
>> would show up as /sys/class/power_supply/BAT0, /sys/class/power_supply/BAT1,
>> and so on.
> Yes, I hope so.
>
>> The current BAT0 entry shows things like 'capacity' even without this
>> patch, and we're just piggybacking off of that to add charge_type and
>> other entries. So there shouldn't be any confusion there, agreed?
> I have not looked at the battery_hook_register() code yet (seems that I
> would have to properly read it and understand it). But does it mean that
> battery_hook_register() is adding hook just for "BAT0"?
>
> What I mean: cannot that hook be registered to "BAT1" too? Because if
> yes then we should prevent it. Otherwise this hook which is for "Dell
> Primary Battery" could be registered also for secondary battery "BAT1".
> (I hope that now it is more clear what I mean).

Hi,

the battery hook is being registered to all ACPI batteries present on a given system,
so you need to do some manual filtering when .add_battery() is called.

As a side note: i suspect that "newer" Dell machines use a different interface for controlling
battery charging, since the Dell Power Manager software on my machine seems to provide some
additional features not found in this token-based interface.

Unfortunately i am not sure if reverse-engineering the Dell software is legal, does the kernel
community have some helping guides in this area? If it is legal, then i would happily volunteer
to do the reverse-engineering.

Otherwise maybe someone at Dell can provide some clarifications if a different interface for controlling
battery charging exists and how to use it?

Thanks,
Armin Wolf

>> In the kernel, we're registering the acpi_battery_hook as "Dell Primary
>> Battery Extension". The functions set up by that acpi_battery_hook are
>> the only ones using battery_support_modes. We could make it more explicit
>> by:
>> 1) renaming it to primary_battery_modes, along with
>> dell_primary_battery_{init,exit} and/or
>> 2) allocating the mode mask and strings dynamically, and storing that
>> inside of the dev kobject.
>>
>> However, #2 seems overly complicated for what we're doing. In the
>> circumstances that we want to add support for secondary batteries,
>> we're going to need to query separate tokens, add another
>> acpi_battery_hook, and also add a second mask. Whether that's a global
>> variable or kept inside pdev seems like more of a style issue than
>> anything else.
>>
>> #1 is easy enough to change, if you think that's necessary.
> I think that "Dell Primary Battery Extension" is OK. All SMBIOS code is
> currently primary-battery only.
>
> The only my point is to prevent this &dell_battery_hook to be registered
> for non-primary battery.
>