| 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
| ||
|
Message-ID: <20240722143432.35c356b1@5400>
Date: Mon, 22 Jul 2024 14:34:32 -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 Mon, 22 Jul 2024 09:18:45 +0200
Pali Rohár <pali@...nel.org> wrote:
> On Sunday 21 July 2024 19:58:51 Andres Salomon wrote:
> > On Mon, 22 Jul 2024 01:40:37 +0200
> > Pali Rohár <pali@...nel.org> wrote:
> >
> > > On Sunday 21 July 2024 19:37:16 Andres Salomon wrote:
> > > > On Sun, 21 Jul 2024 11:02:38 +0200
> > > > Pali Rohár <pali@...nel.org> wrote:
> > > >
> > > > > On Saturday 20 July 2024 22:06:06 Andres Salomon wrote:
> > > > > > 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:
> > > > > > > >
> > > >
> > > > [...]
> > > >
> > > > > > > > > > +
> > > > > > > > > > +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 *) ¤t_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.
> > > > >
> > > > > Interesting... Anyway, I still think that the API could be similar to
> > > > > what is used in keyboard backlight.
> > > > >
> > > > > Could you please dump all information about each token? They are in
> > > > > struct calling_interface_token returned by dell_smbios_find_token.
> > > > >
> > > > > I'm interesting in tokenID, location and value.
> > > > >
> > > > > Ideally to compare what is in token->value and then in buffer.output[1]
> > > > > (in case dell_send_request does not fail).
> > > >
> > > >
> > > > Alright, here's what I see:
> > > >
> > > > [ 5.904775] dell_laptop: dell_battery_read_req: token requested: 0x343, tokenID=0x343, location=0x343, value=5
> > > > [ 5.908675] dell_laptop: dell_battery_read_req: buffer.output[1]=3
> > > > [ 5.908680] dell_laptop: dell_battery_init: BAT_CUSTOM_MODE_TOKEN value: 3
> > > > [ 5.908682] dell_laptop: dell_battery_read_req: token requested: 0x341, tokenID=0x341, location=0x341, value=3
> > > > [ 5.910922] dell_laptop: dell_battery_read_req: buffer.output[1]=3
> > > > [ 5.910926] dell_laptop: dell_battery_init: BAT_PRI_AC_MODE_TOKEN value: 3
> > > > [ 5.910928] dell_laptop: dell_battery_read_req: token requested: 0x342, tokenID=0x342, location=0x342, value=4
> > > > [ 5.913042] dell_laptop: dell_battery_read_req: buffer.output[1]=3
> > > > [ 5.913046] dell_laptop: dell_battery_init: BAT_ADAPTIVE_MODE_TOKEN value: 3
> > > > [ 5.913048] dell_laptop: dell_battery_read_req: token requested: 0x346, tokenID=0x346, location=0x346, value=1
> > > > [ 5.914996] dell_laptop: dell_battery_read_req: buffer.output[1]=3
> > > > [ 5.914999] dell_laptop: dell_battery_init: BAT_STANDARD_MODE_TOKEN value: 3
> > > > [ 5.915000] dell_laptop: dell_battery_read_req: token requested: 0x347, tokenID=0x347, location=0x347, value=2
> > > > [ 5.916723] dell_laptop: dell_battery_read_req: buffer.output[1]=3
> > > > [ 5.916724] dell_laptop: dell_battery_init: BAT_EXPRESS_MODE_TOKEN value: 3
> > > > [ 5.916725] dell_laptop: dell_battery_read_req: token requested: 0x349, tokenID=0x349, location=0x349, value=65535
> > > > [ 5.918727] dell_laptop: dell_battery_read_req: buffer.output[1]=65
> > > > [ 5.918731] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_START value: 65
> > > > [ 5.918734] dell_laptop: dell_battery_read_req: token requested: 0x34a, tokenID=0x34a, location=0x34a, value=65535
> > > > [ 5.920864] dell_laptop: dell_battery_read_req: buffer.output[1]=85
> > > > [ 5.920867] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_END value: 85
> > >
> > > Perfect. And can you check dumps when the mode is set to some other than BAT_PRI_AC_MODE_TOKEN?
> >
> > Here's Express:
> >
> > [ 5.880090] dell_laptop: dell_battery_read_req: token requested: 0x343, tokenID=0x343, location=0x343, value=5
> > [ 5.882011] dell_laptop: dell_battery_read_req: buffer.output[1]=2
> > [ 5.882014] dell_laptop: dell_battery_init: BAT_CUSTOM_MODE_TOKEN value: 2
> > [ 5.882016] dell_laptop: dell_battery_read_req: token requested: 0x341, tokenID=0x341, location=0x341, value=3
> > [ 5.894513] dell_laptop: dell_battery_read_req: buffer.output[1]=2
> > [ 5.894518] dell_laptop: dell_battery_init: BAT_PRI_AC_MODE_TOKEN value: 2
> > [ 5.894520] dell_laptop: dell_battery_read_req: token requested: 0x342, tokenID=0x342, location=0x342, value=4
> > [ 5.913870] dell_laptop: dell_battery_read_req: buffer.output[1]=2
> > [ 5.913874] dell_laptop: dell_battery_init: BAT_ADAPTIVE_MODE_TOKEN value: 2
> > [ 5.913875] dell_laptop: dell_battery_read_req: token requested: 0x346, tokenID=0x346, location=0x346, value=1
> > [ 5.915622] dell_laptop: dell_battery_read_req: buffer.output[1]=2
> > [ 5.915625] dell_laptop: dell_battery_init: BAT_STANDARD_MODE_TOKEN value: 2
> > [ 5.915626] dell_laptop: dell_battery_read_req: token requested: 0x347, tokenID=0x347, location=0x347, value=2
> > [ 5.917349] dell_laptop: dell_battery_read_req: buffer.output[1]=2
> > [ 5.917351] dell_laptop: dell_battery_init: BAT_EXPRESS_MODE_TOKEN value: 2
> > [ 5.917352] dell_laptop: dell_battery_read_req: token requested: 0x349, tokenID=0x349, location=0x349, value=65535
> > [ 5.919068] dell_laptop: dell_battery_read_req: buffer.output[1]=65
> > [ 5.919070] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_START value: 65
> > [ 5.919071] dell_laptop: dell_battery_read_req: token requested: 0x34a, tokenID=0x34a, location=0x34a, value=65535
> > [ 5.920780] dell_laptop: dell_battery_read_req: buffer.output[1]=85
> > [ 5.920782] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_END value: 85
> >
> > And here's Adaptive:
> >
> > [ 5.945319] dell_laptop: dell_battery_read_req: token requested: 0x343, tokenID=0x343, location=0x343, value=5
> > [ 5.973685] dell_laptop: dell_battery_read_req: buffer.output[1]=4
> > [ 5.973690] dell_laptop: dell_battery_init: BAT_CUSTOM_MODE_TOKEN value: 4
> > [ 5.973692] dell_laptop: dell_battery_read_req: token requested: 0x341, tokenID=0x341, location=0x341, value=3
> > [ 5.976533] dell_laptop: dell_battery_read_req: buffer.output[1]=4
> > [ 5.976538] dell_laptop: dell_battery_init: BAT_PRI_AC_MODE_TOKEN value: 4
> > [ 5.976540] dell_laptop: dell_battery_read_req: token requested: 0x342, tokenID=0x342, location=0x342, value=4
> > [ 5.981013] dell_laptop: dell_battery_read_req: buffer.output[1]=4
> > [ 5.981018] dell_laptop: dell_battery_init: BAT_ADAPTIVE_MODE_TOKEN value: 4
> > [ 5.981020] dell_laptop: dell_battery_read_req: token requested: 0x346, tokenID=0x346, location=0x346, value=1
> > [ 5.983474] dell_laptop: dell_battery_read_req: buffer.output[1]=4
> > [ 5.983479] dell_laptop: dell_battery_init: BAT_STANDARD_MODE_TOKEN value: 4
> > [ 5.983481] dell_laptop: dell_battery_read_req: token requested: 0x347, tokenID=0x347, location=0x347, value=2
> > [ 5.985881] dell_laptop: dell_battery_read_req: buffer.output[1]=4
> > [ 5.985885] dell_laptop: dell_battery_init: BAT_EXPRESS_MODE_TOKEN value: 4
> > [ 5.985887] dell_laptop: dell_battery_read_req: token requested: 0x349, tokenID=0x349, location=0x349, value=65535
> > [ 5.988332] dell_laptop: dell_battery_read_req: buffer.output[1]=65
> > [ 5.988337] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_START value: 65
> > [ 5.988339] dell_laptop: dell_battery_read_req: token requested: 0x34a, tokenID=0x34a, location=0x34a, value=65535
> > [ 5.990769] dell_laptop: dell_battery_read_req: buffer.output[1]=85
> > [ 5.990774] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_END value: 85
> >
> >
> >
> > --
> > I'm available for contract & employment work, see:
> > https://spindle.queued.net/~dilinger/resume-tech.pdf
>
> Nice! So it is exactly same as API of keyboard backlight tokens. Thanks.
>
> In dell_battery_write_req function you can drop second "val" argument
> and replace it by token->value. So the dell_fill_request call in that
> function would look like:
>
> dell_fill_request(&buffer, token->location, token->value, 0, 0);
Well, except that we use dell_battery_write_req for writing the charge
start/end values as well (in dell_battery_custom_set). Those can't be
obtained from token->value.
We could have two separate functions for that, or set 'val' to a
sentinel value (0) that, if detected, we set val=token->value. I'm
still not really understanding the point, though.
>
> And then you can mimic the usage as it is done in keyboard backlight
> functions (kbd_get_first_active_token_bit).
>
> If you do not know what I mean then later (today or tomorrow) I can
> write code example of the functionality.
Sorry, I still don't understand what the goal is here. Is the goal to
not pull from a random location to determine the current charging mode?
Is the goal to determine what charging modes are currently supported
(and if so, I don't see how)? Is the goal to avoid having the kernel
hardcode a list of enums that the BIOS might have different values
for? Is the goal to merge the keyboard backlight and battery setting
functions?
--
I'm available for contract & employment work, see:
https://spindle.queued.net/~dilinger/resume-tech.pdf
Powered by blists - more mailing lists