Message-ID: <0f5735f3-6979-4cc3-8d9b-a953be210683@gmx.de>
Date: Sun, 11 Aug 2024 07:28:52 +0200
From: Armin Wolf <W_Armin@....de>
To: Pali Rohár <pali@...nel.org>
Cc: Andres Salomon <dilinger@...ued.net>, 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
Subject: Re: [PATCH v2 1/2] platform/x86:dell-laptop: Add knobs to change
 battery charge settings

Am 26.07.24 um 02:04 schrieb Pali Rohár:

> On Friday 26 July 2024 01:48:50 Armin Wolf wrote:
>> 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@...nel.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@...nel.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.
> Ok. So it means that the filtering based on the primary battery in
> add_battery callback is needed.
>
>> 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.
> Dell has released documentation of some other API, see the end of this file
> https://github.com/dell/libsmbios/blob/master/src/bin/smbios-battery-ctl
>
>> 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.
> That is questionable. Some kernel drivers were written from reverse
> engineered data in past.
>
> Note that in some countries is reverse engineering legal if it is done
> for interoperability purposes (which this one could match).
>
>> Otherwise maybe someone at Dell can provide some clarifications if a different interface for controlling
>> battery charging exists and how to use it?
> Try to send an off-list/private email to Dell.Client.Kernel@...l.com
> with details for what you are asking. Maybe they would have access to
> some new documentation.
>
I received no response to the email i send to Dell.Client.Kernel@...l.com, so i started reverse-engineering
the Dell software which is used to control the battery charge settings.

My findings (as for now) are:

- there exists an extended battery control interface where the charge mode can be set for each individual battery
- this extended interface uses the Dell SMBIOS calling interface together with some extensions
- the interface has a design flaw which makes it impossible to reliably support hot-swap batteries (thanks Dell!)
- the extended interface might only be supported on WMI-capable machines, but i need to verify this

I will need some more time for reverse-engineering the whole interface, but i believe adding support for it should
be possible.

For now, i think this patch series is the way forward.

Thanks,
Armin Wolf

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

Powered by blists - more mailing lists