mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: Kurt Borja <kuurtb@gmail.com>
To: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>,
	"Thomas Weißschuh" <linux@weissschuh.net>,
	"Joshua Grisham" <josh@joshuagrisham.com>,
	"Mark Pearson" <mpearson-lenovo@squebb.ca>,
	"Armin Wolf" <W_Armin@gmx.de>,
	"Mario Limonciello" <mario.limonciello@amd.com>,
	"Hans de Goede" <hansg@kernel.org>
Cc: Alok Tiwari <alok.a.tiwari@oracle.com>,
	 Antheas Kapenekakis <lkml@antheas.dev>,
	 "Derek J. Clark" <derekjohn.clark@gmail.com>,
	 Prasanth Ksr <prasanth.ksr@dell.com>,
	Jorge Lopez <jorge.lopez2@hp.com>,
	 platform-driver-x86@vger.kernel.org,
	linux-kernel@vger.kernel.org,  Dell.Client.Kernel@dell.com,
	Kurt Borja <kuurtb@gmail.com>
Subject: [PATCH v6 0/6] platform/x86: firmware_attributes_class: Add a high level API
Date: Thu, 10 Jul 2025 00:03:15 -0300	[thread overview]
Message-ID: <20250710-fw-attrs-api-v6-0-9959ef759771@gmail.com> (raw)

Hi all,

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.

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@gmail.com>
---
Changes in v6:
  [Patch 1]
    - Add put_device() if device_register() fails
    - Drop sysfs_remove_groups() in fwat_device_unregister()
    - Didn't drop kset_unregister() because I think it's required
  [Patch 2]
    - Introduce FWAT_GROUP_ATTR() macro to avoid errors when creating
      default ktype attributes.
    - Fix typos in firmware_attributes_class.h
    - Constify struct fwat_attribute in callbacks
    - Drop DEFINE_SYSFS_GROUP_VISIBLE() and pass the visibility callback
      directly
    - Drop <linux/list.h> in firmware_attributes_class.h
    - Rename enum fwat_group_type members
    - Move fwat_*_current_value assertions to firmware_attributes_class.c
    - Add a '__' prefix to fwat_create_*_group() functions
    - Some style improvements
    - Didn't drop fwat_remove_auto_groups() because I think it's
      required
  [Patch 4]
    - Don't drop mutex initialization
    - Lock fw_attrs_lock on *_write() callbacks

  - Link to v5: https://lore.kernel.org/r/20250705-fw-attrs-api-v5-0-60b6d51d93eb@gmail.com

Changes in v5:
  - Fix kernel test robot warning
  - Link to v4: https://lore.kernel.org/r/20250630-fw-attrs-api-v4-0-1a04952b255f@gmail.com

Changes in v4:
  [Patch 1]
    - Embbed a device in fwat_device instead of a kobject.
    - Instead of an attrs_kobj root kobj, create a kset with the same
      name.
  [Patch 2]
    - Add a (*show_override) callback in fwat_group_data.
    - Instead of allocating and filling sysfs groups and attributes
      manually, I defined custom ktypes for each fwat type. All groups are
      now statically defined and added through default_groups.
  
      I think this is a BIG optimization in terms of memory at least. Also
      fwat_group memory is now managed by a kobject which is allocated one
      time. This is also a less impactful performance optimization (less
      individual allocations).
    - No changes to API :) (I take suggestions though)

  I might have lost some of the changelog. Sorry for that!
  
  - Link to v3: https://lore.kernel.org/r/20250621-fw-attrs-api-v3-0-3dd55e463396@gmail.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   | 667 ++++++++++++++++++++-
 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          | 240 ++------
 include/linux/firmware_attributes_class.h          | 369 ++++++++++++
 9 files changed, 1111 insertions(+), 192 deletions(-)
---
base-commit: 428f6f3a56ac85f37a07a3fe5149b593185d5c4c
change-id: 20250326-fw-attrs-api-0eea7c0225b6
-- 
 ~ Kurt


             reply	other threads:[~2025-07-10  3:03 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-07-10  3:03 Kurt Borja [this message]
2025-07-10  3:03 ` [PATCH v6 1/6] platform/x86: firmware_attributes_class: Add device initialization methods Kurt Borja
2025-07-10  3:03 ` [PATCH v6 2/6] platform/x86: firmware_attributes_class: Add high level API for the attributes interface Kurt Borja
2025-07-10  3:03 ` [PATCH v6 3/6] platform/x86: firmware_attributes_class: Move header to include directory Kurt Borja
2025-07-14 16:04   ` kernel test robot
2025-07-10  3:03 ` [PATCH v6 4/6] platform/x86: samsung-galaxybook: Transition new firmware_attributes API Kurt Borja
2025-07-10  3:03 ` [PATCH v6 5/6] Documentation: ABI: Update sysfs-class-firmware-attributes documentation Kurt Borja
2025-07-10  3:03 ` [PATCH v6 6/6] MAINTAINERS: Add FIRMWARE ATTRIBUTES CLASS entry Kurt Borja

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20250710-fw-attrs-api-v6-0-9959ef759771@gmail.com \
    --to=kuurtb@gmail.com \
    --cc=Dell.Client.Kernel@dell.com \
    --cc=W_Armin@gmx.de \
    --cc=alok.a.tiwari@oracle.com \
    --cc=derekjohn.clark@gmail.com \
    --cc=hansg@kernel.org \
    --cc=ilpo.jarvinen@linux.intel.com \
    --cc=jorge.lopez2@hp.com \
    --cc=josh@joshuagrisham.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@weissschuh.net \
    --cc=lkml@antheas.dev \
    --cc=mario.limonciello@amd.com \
    --cc=mpearson-lenovo@squebb.ca \
    --cc=platform-driver-x86@vger.kernel.org \
    --cc=prasanth.ksr@dell.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®