linux-kernel - Re: [PATCH v3 0/6] platform/x86: firmware_attributes

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <DA17A4BC-938D-45BC-848E-BD8FB43597BB@gmail.com>
Date: Sat, 21 Jun 2025 20:01:33 -0700
From: "Derek J. Clark" <derekjohn.clark@...il.com>
To: Kurt Borja <kuurtb@...il.com>, Hans de Goede <hdegoede@...hat.com>,
 Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
 Thomas Weißschuh <linux@...ssschuh.net>,
 Joshua Grisham <josh@...huagrisham.com>,
 Mark Pearson <mpearson-lenovo@...ebb.ca>, Armin Wolf <W_Armin@....de>,
 Mario Limonciello <mario.limonciello@....com>
CC: Antheas Kapenekakis <lkml@...heas.dev>, Prasanth Ksr <prasanth.ksr@...l.com>,
 Jorge Lopez <jorge.lopez2@...com>, platform-driver-x86@...r.kernel.org,
 linux-kernel@...r.kernel.org, Dell.Client.Kernel@...l.com
Subject: Re: [PATCH v3 0/6] platform/x86: firmware_attributes_class: Add a high level API



On June 21, 2025 5:04:03 PM PDT, Kurt Borja <kuurtb@...il.com> wrote:
>Hi all,
>
>I apologize for taking so long. I've been a bit busy these last few
>weeks.
>
>After my discussion with Joshua on v2, I realized the API I made was not
>ergonomic at all and it didn't exactly respond to driver needs. In this
>version I tried a completely different approach and IMO it's much much
>better now.
>
>First of all I adopted standard sysfs terminology for everything. A
>"firmware attribute" is just an attribute_group under the attributes/
>directory so everything related to this concept is just called "group"
>now. Everything refered as properties in the previous patch are now just
>plain "attributes".
>
>This new API revolves around the `fwat_{bool,enum,int,str}_data`
>structs. These hold all the metadata a "firmware_attribute" of that
>given type needs.
>
>These structs also hold `read` and `write` callbacks for the
>current_value attribute, because obviously that value is always dynamic.
>However the rest of attributes (default_value, display_name, min, max,
>etc) are constant.

Hi Kurt,

In the lenovo-wmi drivers the min/max for multiple attributes are actually dynamic based on if power is AC connected or on battery. Looking at patch 2 I might be able to do some pointer manipulation with the attribute's "data" member for those events to make this work, but it would be a lot easier if there was a simple way for me to call my own functions here instead. Perhaps a function pointer could be used to override the default method here? 

Cheers, 
Derek

>In the simple case this metadata structs can be defined statically with
>DEFINE_FWAT_{BOOL,ENUM,INT,STR}_GROUP() macros. However most users of
>this class obtain this values dynamically so you can also define this
>structs dynamically.
>
>In the end all groups (static and dynamic) will be created using
>fwat_create_group() after registering the class device.
>
>Let me know what you think, your feedback is very appreciated :)
>
>I do have one question for anyone interested. Should constraints over
>the current_value (such as min, max, increment, etc.) be enforced at the
>show/store level? i.e. before values reach read/write callbacks.
>
>Signed-off-by: Kurt Borja <kuurtb@...il.com>
>---
>Changes in v3:
>
>[Patch 1]
>- Fixed UAF in fwat_device_unregister(). Device was unregistered after
>  freeing fadev.
>
>[Patch 2]
>- Patch 2 was completely replaced. A new approach for the API is taken,
>  based on Joshua's suggestions.
>
>- Link to v2: https://lore.kernel.org/r/20250517-fw-attrs-api-v2-0-fa1ab045a01c@gmail.com
>
>Changes in v2:
>
>[Patch 1]
> - Include kdev_t.h header
>
>[Patch 2]
> - Use one line comments in fwat_create_attrs()
> - Check propagate errors in fwat_create_attrs()
> - Add `mode` to fwat_attr_config and related macros to let users
>   configure the `current_value` attribute mode
> - Use defined structs in fwat_attr_ops instead of anonymous ones
> - Move fwat_attr_type from config to ops
>
>[Patch 5]
> - Just transition to new API without chaing ABI
>
>- Link to v1: https://lore.kernel.org/r/20250509-fw-attrs-api-v1-0-258afed65bfa@gmail.com
>
>---
>Kurt Borja (5):
>      platform/x86: firmware_attributes_class: Add high level API for the attributes interface
>      platform/x86: firmware_attributes_class: Move header to include directory
>      platform/x86: samsung-galaxybook: Transition new firmware_attributes API
>      Documentation: ABI: Update sysfs-class-firmware-attributes documentation
>      MAINTAINERS: Add FIRMWARE ATTRIBUTES CLASS entry
>
>Thomas Weißschuh (1):
>      platform/x86: firmware_attributes_class: Add device initialization methods
>
> .../ABI/testing/sysfs-class-firmware-attributes    |   1 +
> MAINTAINERS                                        |   8 +
> drivers/platform/x86/dell/dell-wmi-sysman/sysman.c |   2 +-
> drivers/platform/x86/firmware_attributes_class.c   | 615 ++++++++++++++++++++-
> drivers/platform/x86/firmware_attributes_class.h   |  12 -
> drivers/platform/x86/hp/hp-bioscfg/bioscfg.c       |   2 +-
> drivers/platform/x86/lenovo/think-lmi.c            |   2 +-
> drivers/platform/x86/samsung-galaxybook.c          | 246 +++------
> include/linux/firmware_attributes_class.h          | 389 +++++++++++++
> 9 files changed, 1077 insertions(+), 200 deletions(-)
>---
>base-commit: 73f0f2b52c5ea67b3140b23f58d8079d158839c8
>change-id: 20250326-fw-attrs-api-0eea7c0225b6

- Derek