| 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: <20240823223950.7f952489@5400>
Date: Fri, 23 Aug 2024 22:39:50 -0400
From: Andres Salomon <dilinger@...ued.net>
To: Hans de Goede <hdegoede@...hat.com>
Cc: linux-kernel@...r.kernel.org, Pali Rohár
<pali@...nel.org>, platform-driver-x86@...r.kernel.org, Matthew Garrett
<mjg59@...f.ucam.org>, Sebastian Reichel <sre@...nel.org>, Ilpo
Järvinen <ilpo.jarvinen@...ux.intel.com>,
linux-pm@...r.kernel.org, Dell.Client.Kernel@...l.com
Subject: Re: [PATCH v3 1/2] platform/x86:dell-laptop: Add knobs to change
battery charge settings
On Mon, 19 Aug 2024 15:59:45 +0200
Hans de Goede <hdegoede@...hat.com> wrote:
> Hi,
>
> On 8/16/24 1:28 AM, Andres Salomon wrote:
[...]
>
> > ---
> > Changes in v3:
> > - switch tokenid and class types
> > - be stricter with results from both userspace and BIOS
> > - no longer allow failed BIOS reads
> > - rename/move dell_send_request_by_token_loc, and add helper function
> > - only allow registration for BAT0
> > - rename charge_type modes to match power_supply names
> > Changes in v2, based on extensive feedback from Pali Rohár <pali@...nel.org>:
> > - code style changes
> > - change battery write API to use token->value instead of passed value
> > - stop caching current mode, instead querying SMBIOS as needed
> > - drop the separate list of charging modes enum
> > - rework the list of charging mode strings
> > - query SMBIOS for supported charging modes
> > - split dell_battery_custom_set() up
> > ---
> > .../ABI/testing/sysfs-class-power-dell | 33 ++
> > drivers/platform/x86/dell/Kconfig | 1 +
> > drivers/platform/x86/dell/dell-laptop.c | 316 ++++++++++++++++++
> > drivers/platform/x86/dell/dell-smbios.h | 7 +
> > 4 files changed, 357 insertions(+)
> > create mode 100644 Documentation/ABI/testing/sysfs-class-power-dell
> >
> > diff --git a/Documentation/ABI/testing/sysfs-class-power-dell b/Documentation/ABI/testing/sysfs-class-power-dell
> > new file mode 100644
> > index 000000000000..d8c542177558
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-class-power-dell
> > @@ -0,0 +1,33 @@
> > +What: /sys/class/power_supply/<supply_name>/charge_type
> > +Date: August 2024
> > +KernelVersion: 6.12
> > +Contact: linux-pm@...r.kernel.org
> > +Description:
> > + Select the charging algorithm to use for the (primary)
> > + battery.
> > +
> > + Standard:
> > + Fully charge the battery at a moderate rate.
> > + Fast:
> > + Quickly charge the battery using fast-charge
> > + technology. This is harder on the battery than
> > + standard charging and may lower its lifespan.
> > + The Dell BIOS calls this ExpressCharge™.
> > + Trickle:
> > + Users who primarily operate the system while
> > + plugged into an external power source can extend
> > + battery life with this mode. The Dell BIOS calls
> > + this "Primarily AC Use".
> > + Adaptive:
> > + Automatically optimize battery charge rate based
> > + on typical usage pattern.
> > + Custom:
> > + Use the charge_control_* properties to determine
> > + when to start and stop charging. Advanced users
> > + can use this to drastically extend battery life.
> > +
> > + Access: Read, Write
> > + Valid values:
> > + "Standard", "Fast", "Trickle",
> > + "Adaptive", "Custom"
> > +
>
> As the kernel test robot reports:
>
> Warning: /sys/class/power_supply/<supply_name>/charge_type is defined 2 times: ./Documentation/ABI/testing/sysfs-class-power-dell:0 ./Documentation/ABI/testing/sysfs-class-power:375
>
> So please drop this. What you could do is instead submit (as a separate) patch
> an update to Documentation/ABI/testing/sysfs-class-power:375 to use your IMHO
> more readable version.
>
> And when doing so I think it would best to replace "The Dell BIOS calls this ..."
> with "In vendor tooling this may also be called ...".
>
>
Is this what you had in mind? I don't see many users of charge_type, and
I'm not sure whether the assumptions I'm making there are correct.
https://lore.kernel.org/lkml/20240820041942.30ed42f3@5400/
> > diff --git a/drivers/platform/x86/dell/Kconfig b/drivers/platform/x86/dell/Kconfig
> > index 85a78ef91182..02405793163c 100644
> > --- a/drivers/platform/x86/dell/Kconfig
> > +++ b/drivers/platform/x86/dell/Kconfig
> > @@ -49,6 +49,7 @@ config DELL_LAPTOP
> > default m
> > depends on DMI
> > depends on BACKLIGHT_CLASS_DEVICE
> > + depends on ACPI_BATTERY
> > depends on ACPI_VIDEO || ACPI_VIDEO = n
> > depends on RFKILL || RFKILL = n
> > depends on DELL_WMI || DELL_WMI = n
> > diff --git a/drivers/platform/x86/dell/dell-laptop.c b/drivers/platform/x86/dell/dell-laptop.c
> > index 6552dfe491c6..8cc05f0fab91 100644
> > --- a/drivers/platform/x86/dell/dell-laptop.c
> > +++ b/drivers/platform/x86/dell/dell-laptop.c
> > @@ -22,11 +22,13 @@
> > #include <linux/io.h>
> > #include <linux/rfkill.h>
> > #include <linux/power_supply.h>
> > +#include <linux/sysfs.h>
> > #include <linux/acpi.h>
> > #include <linux/mm.h>
> > #include <linux/i8042.h>
> > #include <linux/debugfs.h>
> > #include <linux/seq_file.h>
> > +#include <acpi/battery.h>
> > #include <acpi/video.h>
> > #include "dell-rbtn.h"
> > #include "dell-smbios.h"
> > @@ -99,6 +101,18 @@ static bool force_rfkill;
> > static bool micmute_led_registered;
> > static bool mute_led_registered;
> >
> > +static const struct {
> > + int token;
> > + const char *label;
> > +} battery_modes[] = {
> > + { BAT_STANDARD_MODE_TOKEN, "Standard" },
> > + { BAT_EXPRESS_MODE_TOKEN, "Fast" },
> > + { BAT_PRI_AC_MODE_TOKEN, "Trickle" },
> > + { BAT_ADAPTIVE_MODE_TOKEN, "Adaptive" },
> > + { BAT_CUSTOM_MODE_TOKEN, "Custom" },
> > +};
>
> So Ilpo suggested to split this (first declare the struct type and then
> declare an array of that type) and Pali suggested to keep this as is.
>
> > +static u32 battery_supported_modes;
>
> I see there also is some disagreement on keeping battery_modes[]
> const vs adding a flag for this in the struct.
>
> IMHO in both cases either option is fine, so you (Andres) get
> to choose which solution you prefer in either case.
>
> I do see there is some confusion about Ilpo's split suggestion,
> to clarify that, what I believe is Ilpo meant doing something
> like this:
Thanks for clarify that, my fault for misreading what they'd written!
--
I'm available for contract & employment work, please contact me if
interested.
Powered by blists - more mailing lists