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.

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 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
-- 
 ~ Kurt

On June 21, 2025 5:04:03 PM PDT, Kurt Borja <kuurtb@gmail.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@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   | 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

On Sun Jun 22, 2025 at 12:01 AM -03, Derek J. Clark wrote:
>
>
> On June 21, 2025 5:04:03 PM PDT, Kurt Borja <kuurtb@gmail.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? 

Hi Derek,

All attributes in a given group have the same show method. Maybe we can
let users override this with their own show method, i.e. Add a

	ssize_t (*attr_show)(struct device *dev, const struct fwat_attribute *attr, const char *buf)

to struct fwat_group_data. That should be fairly simple to implement.

Did you have another solution in mind?


-- 
 ~ Kurt

On June 21, 2025 8:27:06 PM PDT, Kurt Borja <kuurtb@gmail.com> wrote:
>On Sun Jun 22, 2025 at 12:01 AM -03, Derek J. Clark wrote:
>>
>>
>> On June 21, 2025 5:04:03 PM PDT, Kurt Borja <kuurtb@gmail.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? 
>
>Hi Derek,
>
>All attributes in a given group have the same show method. Maybe we can
>let users override this with their own show method, i.e. Add a
>
>	ssize_t (*attr_show)(struct device *dev, const struct fwat_attribute *attr, const char *buf)
>
>to struct fwat_group_data. That should be fairly simple to implement.
>
>Did you have another solution in mind?
>
>

That should work, yeah. 
- Derek

On Sun Jun 22, 2025 at 12:42 AM -03, Derek J. Clark wrote:
>
>
> On June 21, 2025 8:27:06 PM PDT, Kurt Borja <kuurtb@gmail.com> wrote:
>>On Sun Jun 22, 2025 at 12:01 AM -03, Derek J. Clark wrote:
>>>
>>>
>>> On June 21, 2025 5:04:03 PM PDT, Kurt Borja <kuurtb@gmail.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? 
>>
>>Hi Derek,
>>
>>All attributes in a given group have the same show method. Maybe we can
>>let users override this with their own show method, i.e. Add a
>>
>>	ssize_t (*attr_show)(struct device *dev, const struct fwat_attribute *attr, const char *buf)
>>
>>to struct fwat_group_data. That should be fairly simple to implement.
>>
>>Did you have another solution in mind?
>>
>>
>
> That should work, yeah. 
> - Derek

Just realized that's exactly what you said :)

It was indeed easy to implement. It will be there for next version.


-- 
 ~ Kurt