[PATCH v5 0/6] platform/x86: firmware_attributes_class: Add a high level API

[PATCH v5 0/6] platform/x86: firmware_attributes_class: Add a high level API

[Kurt Borja]

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 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   | 659 ++++++++++++++++++++-
 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          | 375 ++++++++++++
 9 files changed, 1107 insertions(+), 200 deletions(-)
---
base-commit: 73f0f2b52c5ea67b3140b23f58d8079d158839c8
change-id: 20250326-fw-attrs-api-0eea7c0225b6
-- 
 ~ Kurt

[PATCH v5 1/6] platform/x86: firmware_attributes_class: Add device initialization methods

[Kurt Borja]

From: Thomas Weißschuh <linux@weissschuh.net>

Currently each user of firmware_attributes_class has to manually set up
kobjects, devices, etc.

Provide this infrastructure out-of-the-box through the newly introduced
fwat_device_register().

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
Co-developed-by: Kurt Borja <kuurtb@gmail.com>
Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
 drivers/platform/x86/firmware_attributes_class.c | 125 +++++++++++++++++++++++
 drivers/platform/x86/firmware_attributes_class.h |  28 +++++
 2 files changed, 153 insertions(+)

diff --git a/drivers/platform/x86/firmware_attributes_class.c b/drivers/platform/x86/firmware_attributes_class.c
index 736e96c186d9dc6d945517f090e9af903e93bbf4..290364202cce64bb0e9046e0b2bbb8d85e2cbc6f 100644
--- a/drivers/platform/x86/firmware_attributes_class.c
+++ b/drivers/platform/x86/firmware_attributes_class.c
@@ -2,7 +2,14 @@
 
 /* Firmware attributes class helper module */
 
+#include <linux/cleanup.h>
+#include <linux/device.h>
+#include <linux/device/class.h>
+#include <linux/kdev_t.h>
+#include <linux/kobject.h>
 #include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/types.h>
 #include "firmware_attributes_class.h"
 
 const struct class firmware_attributes_class = {
@@ -10,6 +17,122 @@ const struct class firmware_attributes_class = {
 };
 EXPORT_SYMBOL_GPL(firmware_attributes_class);
 
+static void fwat_device_release(struct device *dev)
+{
+	struct fwat_device *fadev = to_fwat_device(dev);
+
+	kfree(fadev);
+}
+
+/**
+ * fwat_device_register - Create and register a firmware-attributes class
+ *			  device
+ * @parent: Parent device
+ * @name: Name of the class device
+ * @drvdata: Drvdata of the class device
+ * @groups: Extra groups for the "attributes" directory
+ *
+ * Return: pointer to the new fwat_device on success, ERR_PTR on failure
+ */
+struct fwat_device *
+fwat_device_register(struct device *parent, const char *name, void *drvdata,
+		     const struct attribute_group **groups)
+{
+	struct fwat_device *fadev;
+	int ret;
+
+	if (!parent || !name)
+		return ERR_PTR(-EINVAL);
+
+	fadev = kzalloc(sizeof(*fadev), GFP_KERNEL);
+	if (!fadev)
+		return ERR_PTR(-ENOMEM);
+
+	fadev->groups = groups;
+	fadev->dev.class = &firmware_attributes_class;
+	fadev->dev.parent = parent;
+	fadev->dev.release = fwat_device_release;
+	dev_set_drvdata(&fadev->dev, drvdata);
+	ret = dev_set_name(&fadev->dev, "%s", name);
+	if (ret) {
+		kfree(fadev);
+		return ERR_PTR(ret);
+	}
+	ret = device_register(&fadev->dev);
+	if (ret)
+		return ERR_PTR(ret);
+
+	fadev->attrs_kset = kset_create_and_add("attributes", NULL, &fadev->dev.kobj);
+	if (!fadev->attrs_kset) {
+		ret = -ENOMEM;
+		goto out_device_unregister;
+	}
+
+	ret = sysfs_create_groups(&fadev->attrs_kset->kobj, groups);
+	if (ret)
+		goto out_kset_unregister;
+
+	return fadev;
+
+out_kset_unregister:
+	kset_unregister(fadev->attrs_kset);
+
+out_device_unregister:
+	device_unregister(&fadev->dev);
+
+	return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(fwat_device_register);
+
+void fwat_device_unregister(struct fwat_device *fadev)
+{
+	if (!fadev)
+		return;
+
+	sysfs_remove_groups(&fadev->attrs_kset->kobj, fadev->groups);
+	kset_unregister(fadev->attrs_kset);
+	device_unregister(&fadev->dev);
+}
+EXPORT_SYMBOL_GPL(fwat_device_unregister);
+
+static void devm_fwat_device_release(void *data)
+{
+	struct fwat_device *fadev = data;
+
+	fwat_device_unregister(fadev);
+}
+
+/**
+ * devm_fwat_device_register - Create and register a firmware-attributes class
+ *			       device
+ * @parent: Parent device
+ * @name: Name of the class device
+ * @data: Drvdata of the class device
+ * @groups: Extra groups for the class device (Optional)
+ *
+ * Device managed version of fwat_device_register().
+ *
+ * Return: pointer to the new fwat_device on success, ERR_PTR on failure
+ */
+struct fwat_device *
+devm_fwat_device_register(struct device *parent, const char *name, void *data,
+			  const struct attribute_group **groups)
+{
+	struct fwat_device *fadev;
+	int ret;
+
+	fadev = fwat_device_register(parent, name, data, groups);
+	if (IS_ERR(fadev))
+		return fadev;
+
+	ret = devm_add_action_or_reset(parent, devm_fwat_device_release, fadev);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return fadev;
+}
+EXPORT_SYMBOL_GPL(devm_fwat_device_register);
+
 static __init int fw_attributes_class_init(void)
 {
 	return class_register(&firmware_attributes_class);
@@ -23,5 +146,7 @@ static __exit void fw_attributes_class_exit(void)
 module_exit(fw_attributes_class_exit);
 
 MODULE_AUTHOR("Mark Pearson <markpearson@lenovo.com>");
+MODULE_AUTHOR("Thomas Weißschuh <linux@weissschuh.net>");
+MODULE_AUTHOR("Kurt Borja <kuurtb@gmail.com>");
 MODULE_DESCRIPTION("Firmware attributes class helper module");
 MODULE_LICENSE("GPL");
diff --git a/drivers/platform/x86/firmware_attributes_class.h b/drivers/platform/x86/firmware_attributes_class.h
index d27abe54fcf9812a2f0868eec5426bbc8e7eb21c..048fd0904f767357ef856e687ec4cf3260016ec6 100644
--- a/drivers/platform/x86/firmware_attributes_class.h
+++ b/drivers/platform/x86/firmware_attributes_class.h
@@ -5,8 +5,36 @@
 #ifndef FW_ATTR_CLASS_H
 #define FW_ATTR_CLASS_H
 
+#include <linux/container_of.h>
+#include <linux/device.h>
 #include <linux/device/class.h>
+#include <linux/kobject.h>
+#include <linux/sysfs.h>
 
 extern const struct class firmware_attributes_class;
 
+/**
+ * struct fwat_device - The firmware-attributes device
+ * @dev: The class device.
+ * @attrs_kobj: The "attributes" root kobject.
+ * @groups: Sysfs groups attached to the @attrs_kobj.
+ */
+struct fwat_device {
+	struct device dev;
+	struct kset *attrs_kset;
+	const struct attribute_group **groups;
+};
+
+#define to_fwat_device(_d)	container_of_const(_d, struct fwat_device, dev)
+
+struct fwat_device * __must_check
+fwat_device_register(struct device *parent, const char *name, void *drvdata,
+		     const struct attribute_group **groups);
+
+void fwat_device_unregister(struct fwat_device *fwadev);
+
+struct fwat_device * __must_check
+devm_fwat_device_register(struct device *parent, const char *name, void *data,
+			  const struct attribute_group **groups);
+
 #endif /* FW_ATTR_CLASS_H */

-- 
2.50.0

Re: [PATCH v5 1/6] platform/x86: firmware_attributes_class: Add device initialization methods

[Thomas Weißschuh]

On 2025-07-05 00:33:56-0300, Kurt Borja wrote:
> From: Thomas Weißschuh <linux@weissschuh.net>
> 
> Currently each user of firmware_attributes_class has to manually set up
> kobjects, devices, etc.
> 
> Provide this infrastructure out-of-the-box through the newly introduced
> fwat_device_register().
> 
> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> Co-developed-by: Kurt Borja <kuurtb@gmail.com>
> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
> ---
>  drivers/platform/x86/firmware_attributes_class.c | 125 +++++++++++++++++++++++
>  drivers/platform/x86/firmware_attributes_class.h |  28 +++++
>  2 files changed, 153 insertions(+)
> 
> diff --git a/drivers/platform/x86/firmware_attributes_class.c b/drivers/platform/x86/firmware_attributes_class.c
> index 736e96c186d9dc6d945517f090e9af903e93bbf4..290364202cce64bb0e9046e0b2bbb8d85e2cbc6f 100644
> --- a/drivers/platform/x86/firmware_attributes_class.c
> +++ b/drivers/platform/x86/firmware_attributes_class.c
> @@ -2,7 +2,14 @@
>  
>  /* Firmware attributes class helper module */
>  
> +#include <linux/cleanup.h>
> +#include <linux/device.h>
> +#include <linux/device/class.h>
> +#include <linux/kdev_t.h>
> +#include <linux/kobject.h>
>  #include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
>  #include "firmware_attributes_class.h"
>  
>  const struct class firmware_attributes_class = {
> @@ -10,6 +17,122 @@ const struct class firmware_attributes_class = {
>  };
>  EXPORT_SYMBOL_GPL(firmware_attributes_class);
>  
> +static void fwat_device_release(struct device *dev)
> +{
> +	struct fwat_device *fadev = to_fwat_device(dev);
> +
> +	kfree(fadev);
> +}
> +
> +/**
> + * fwat_device_register - Create and register a firmware-attributes class
> + *			  device
> + * @parent: Parent device
> + * @name: Name of the class device
> + * @drvdata: Drvdata of the class device
> + * @groups: Extra groups for the "attributes" directory
> + *
> + * Return: pointer to the new fwat_device on success, ERR_PTR on failure
> + */
> +struct fwat_device *
> +fwat_device_register(struct device *parent, const char *name, void *drvdata,
> +		     const struct attribute_group **groups)
> +{
> +	struct fwat_device *fadev;
> +	int ret;
> +
> +	if (!parent || !name)
> +		return ERR_PTR(-EINVAL);
> +
> +	fadev = kzalloc(sizeof(*fadev), GFP_KERNEL);
> +	if (!fadev)
> +		return ERR_PTR(-ENOMEM);
> +
> +	fadev->groups = groups;
> +	fadev->dev.class = &firmware_attributes_class;
> +	fadev->dev.parent = parent;
> +	fadev->dev.release = fwat_device_release;
> +	dev_set_drvdata(&fadev->dev, drvdata);
> +	ret = dev_set_name(&fadev->dev, "%s", name);
> +	if (ret) {
> +		kfree(fadev);
> +		return ERR_PTR(ret);
> +	}
> +	ret = device_register(&fadev->dev);
> +	if (ret)
> +		return ERR_PTR(ret);

I think we need a put_device() here.

> +
> +	fadev->attrs_kset = kset_create_and_add("attributes", NULL, &fadev->dev.kobj);
> +	if (!fadev->attrs_kset) {
> +		ret = -ENOMEM;
> +		goto out_device_unregister;
> +	}
> +
> +	ret = sysfs_create_groups(&fadev->attrs_kset->kobj, groups);
> +	if (ret)
> +		goto out_kset_unregister;

It would be nicer for userspace to add the device to the hierarchy
only when it is set up fully.
Replacing device_register() with a device_initialize() above and
device_add() down here.

> +
> +	return fadev;
> +
> +out_kset_unregister:
> +	kset_unregister(fadev->attrs_kset);

I *think* the driver core should clean up any child objects
automatically, so this is unnecessary.

> +
> +out_device_unregister:
> +	device_unregister(&fadev->dev);
> +
> +	return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL_GPL(fwat_device_register);
> +
> +void fwat_device_unregister(struct fwat_device *fadev)
> +{
> +	if (!fadev)
> +		return;
> +
> +	sysfs_remove_groups(&fadev->attrs_kset->kobj, fadev->groups);
> +	kset_unregister(fadev->attrs_kset);

The also the two lines above would be unnecessary.

> +	device_unregister(&fadev->dev);
> +}
> +EXPORT_SYMBOL_GPL(fwat_device_unregister);
> +
> +static void devm_fwat_device_release(void *data)
> +{
> +	struct fwat_device *fadev = data;
> +
> +	fwat_device_unregister(fadev);
> +}
> +
> +/**
> + * devm_fwat_device_register - Create and register a firmware-attributes class
> + *			       device
> + * @parent: Parent device
> + * @name: Name of the class device
> + * @data: Drvdata of the class device
> + * @groups: Extra groups for the class device (Optional)
> + *
> + * Device managed version of fwat_device_register().
> + *
> + * Return: pointer to the new fwat_device on success, ERR_PTR on failure
> + */
> +struct fwat_device *
> +devm_fwat_device_register(struct device *parent, const char *name, void *data,
> +			  const struct attribute_group **groups)
> +{
> +	struct fwat_device *fadev;
> +	int ret;
> +
> +	fadev = fwat_device_register(parent, name, data, groups);
> +	if (IS_ERR(fadev))
> +		return fadev;
> +
> +	ret = devm_add_action_or_reset(parent, devm_fwat_device_release, fadev);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	return fadev;
> +}
> +EXPORT_SYMBOL_GPL(devm_fwat_device_register);

... and also all of the devm stuff.

> +
>  static __init int fw_attributes_class_init(void)
>  {
>  	return class_register(&firmware_attributes_class);
> @@ -23,5 +146,7 @@ static __exit void fw_attributes_class_exit(void)
>  module_exit(fw_attributes_class_exit);
>  
>  MODULE_AUTHOR("Mark Pearson <markpearson@lenovo.com>");
> +MODULE_AUTHOR("Thomas Weißschuh <linux@weissschuh.net>");
> +MODULE_AUTHOR("Kurt Borja <kuurtb@gmail.com>");
>  MODULE_DESCRIPTION("Firmware attributes class helper module");
>  MODULE_LICENSE("GPL");

<snip>

Re: [PATCH v5 1/6] platform/x86: firmware_attributes_class: Add device initialization methods

[Kurt Borja]

Hi Thomas,

On Sun Jul 6, 2025 at 3:42 AM -03, Thomas Weißschuh wrote:
> On 2025-07-05 00:33:56-0300, Kurt Borja wrote:
>> From: Thomas Weißschuh <linux@weissschuh.net>
>> 
>> Currently each user of firmware_attributes_class has to manually set up
>> kobjects, devices, etc.
>> 
>> Provide this infrastructure out-of-the-box through the newly introduced
>> fwat_device_register().
>> 
>> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
>> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
>> Co-developed-by: Kurt Borja <kuurtb@gmail.com>
>> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
>> ---
>>  drivers/platform/x86/firmware_attributes_class.c | 125 +++++++++++++++++++++++
>>  drivers/platform/x86/firmware_attributes_class.h |  28 +++++
>>  2 files changed, 153 insertions(+)
>> 
>> diff --git a/drivers/platform/x86/firmware_attributes_class.c b/drivers/platform/x86/firmware_attributes_class.c
>> index 736e96c186d9dc6d945517f090e9af903e93bbf4..290364202cce64bb0e9046e0b2bbb8d85e2cbc6f 100644
>> --- a/drivers/platform/x86/firmware_attributes_class.c
>> +++ b/drivers/platform/x86/firmware_attributes_class.c
>> @@ -2,7 +2,14 @@
>>  
>>  /* Firmware attributes class helper module */
>>  
>> +#include <linux/cleanup.h>
>> +#include <linux/device.h>
>> +#include <linux/device/class.h>
>> +#include <linux/kdev_t.h>
>> +#include <linux/kobject.h>
>>  #include <linux/module.h>
>> +#include <linux/slab.h>
>> +#include <linux/types.h>
>>  #include "firmware_attributes_class.h"
>>  
>>  const struct class firmware_attributes_class = {
>> @@ -10,6 +17,122 @@ const struct class firmware_attributes_class = {
>>  };
>>  EXPORT_SYMBOL_GPL(firmware_attributes_class);
>>  
>> +static void fwat_device_release(struct device *dev)
>> +{
>> +	struct fwat_device *fadev = to_fwat_device(dev);
>> +
>> +	kfree(fadev);
>> +}
>> +
>> +/**
>> + * fwat_device_register - Create and register a firmware-attributes class
>> + *			  device
>> + * @parent: Parent device
>> + * @name: Name of the class device
>> + * @drvdata: Drvdata of the class device
>> + * @groups: Extra groups for the "attributes" directory
>> + *
>> + * Return: pointer to the new fwat_device on success, ERR_PTR on failure
>> + */
>> +struct fwat_device *
>> +fwat_device_register(struct device *parent, const char *name, void *drvdata,
>> +		     const struct attribute_group **groups)
>> +{
>> +	struct fwat_device *fadev;
>> +	int ret;
>> +
>> +	if (!parent || !name)
>> +		return ERR_PTR(-EINVAL);
>> +
>> +	fadev = kzalloc(sizeof(*fadev), GFP_KERNEL);
>> +	if (!fadev)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	fadev->groups = groups;
>> +	fadev->dev.class = &firmware_attributes_class;
>> +	fadev->dev.parent = parent;
>> +	fadev->dev.release = fwat_device_release;
>> +	dev_set_drvdata(&fadev->dev, drvdata);
>> +	ret = dev_set_name(&fadev->dev, "%s", name);
>> +	if (ret) {
>> +		kfree(fadev);
>> +		return ERR_PTR(ret);
>> +	}
>> +	ret = device_register(&fadev->dev);
>> +	if (ret)
>> +		return ERR_PTR(ret);
>
> I think we need a put_device() here.

Indeed. I'll fix it.

>
>> +
>> +	fadev->attrs_kset = kset_create_and_add("attributes", NULL, &fadev->dev.kobj);
>> +	if (!fadev->attrs_kset) {
>> +		ret = -ENOMEM;
>> +		goto out_device_unregister;
>> +	}
>> +
>> +	ret = sysfs_create_groups(&fadev->attrs_kset->kobj, groups);
>> +	if (ret)
>> +		goto out_kset_unregister;
>
> It would be nicer for userspace to add the device to the hierarchy
> only when it is set up fully.
> Replacing device_register() with a device_initialize() above and
> device_add() down here.

Sure!

In the end however, children kobjects corresponding to "firmware
attributes" will still be added after device_add(), with
fwat_create_group().

Obviously, that only applies if we can all agree on the approach of
Patch 2. If you could take a look I would be very grateful!

>
>> +
>> +	return fadev;
>> +
>> +out_kset_unregister:
>> +	kset_unregister(fadev->attrs_kset);
>
> I *think* the driver core should clean up any child objects
> automatically, so this is unnecessary.

Hmm - I think filesystem is automatically cleaned up. But not putting
down the kset ref would leak it's allocated memory.

I'll verify this.

>
>> +
>> +out_device_unregister:
>> +	device_unregister(&fadev->dev);
>> +
>> +	return ERR_PTR(ret);
>> +}
>> +EXPORT_SYMBOL_GPL(fwat_device_register);
>> +
>> +void fwat_device_unregister(struct fwat_device *fadev)
>> +{
>> +	if (!fadev)
>> +		return;
>> +
>> +	sysfs_remove_groups(&fadev->attrs_kset->kobj, fadev->groups);
>> +	kset_unregister(fadev->attrs_kset);
>
> The also the two lines above would be unnecessary.

For the reason above I think kset_unregister() is still required.

Removing groups manually, on the other hand, is not required (I think).
I can remove that call.

>
>> +	device_unregister(&fadev->dev);
>> +}
>> +EXPORT_SYMBOL_GPL(fwat_device_unregister);
>> +
>> +static void devm_fwat_device_release(void *data)
>> +{
>> +	struct fwat_device *fadev = data;
>> +
>> +	fwat_device_unregister(fadev);
>> +}
>> +
>> +/**
>> + * devm_fwat_device_register - Create and register a firmware-attributes class
>> + *			       device
>> + * @parent: Parent device
>> + * @name: Name of the class device
>> + * @data: Drvdata of the class device
>> + * @groups: Extra groups for the class device (Optional)
>> + *
>> + * Device managed version of fwat_device_register().
>> + *
>> + * Return: pointer to the new fwat_device on success, ERR_PTR on failure
>> + */
>> +struct fwat_device *
>> +devm_fwat_device_register(struct device *parent, const char *name, void *data,
>> +			  const struct attribute_group **groups)
>> +{
>> +	struct fwat_device *fadev;
>> +	int ret;
>> +
>> +	fadev = fwat_device_register(parent, name, data, groups);
>> +	if (IS_ERR(fadev))
>> +		return fadev;
>> +
>> +	ret = devm_add_action_or_reset(parent, devm_fwat_device_release, fadev);
>> +	if (ret)
>> +		return ERR_PTR(ret);
>> +
>> +	return fadev;
>> +}
>> +EXPORT_SYMBOL_GPL(devm_fwat_device_register);
>
> ... and also all of the devm stuff.

Could you elaborate on this?

>
>> +
>>  static __init int fw_attributes_class_init(void)
>>  {
>>  	return class_register(&firmware_attributes_class);
>> @@ -23,5 +146,7 @@ static __exit void fw_attributes_class_exit(void)
>>  module_exit(fw_attributes_class_exit);
>>  
>>  MODULE_AUTHOR("Mark Pearson <markpearson@lenovo.com>");
>> +MODULE_AUTHOR("Thomas Weißschuh <linux@weissschuh.net>");
>> +MODULE_AUTHOR("Kurt Borja <kuurtb@gmail.com>");
>>  MODULE_DESCRIPTION("Firmware attributes class helper module");
>>  MODULE_LICENSE("GPL");
>
> <snip>

Thanks for reviewing this!


-- 
 ~ Kurt

[PATCH v5 2/6] platform/x86: firmware_attributes_class: Add high level API for the attributes interface

[Kurt Borja]

Add high level API to aid in the creation of attribute groups attached
to the `attrs_kobj` (per ABI specification).

This new API lets users configure each group, either statically or
dynamically through a (per type) data struct and then create this group
through the generic fwat_create_group() macro.

Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
 drivers/platform/x86/firmware_attributes_class.c | 532 +++++++++++++++++++++++
 drivers/platform/x86/firmware_attributes_class.h | 335 ++++++++++++++
 2 files changed, 867 insertions(+)

diff --git a/drivers/platform/x86/firmware_attributes_class.c b/drivers/platform/x86/firmware_attributes_class.c
index 290364202cce64bb0e9046e0b2bbb8d85e2cbc6f..96473b3b1a2a87cf21a6e2a9a14d72ae322c94ae 100644
--- a/drivers/platform/x86/firmware_attributes_class.c
+++ b/drivers/platform/x86/firmware_attributes_class.c
@@ -10,13 +10,67 @@
 #include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/types.h>
+#include <linux/string_choices.h>
 #include "firmware_attributes_class.h"
 
+#define FWAT_TYPE_NONE				-1
+
+#define to_fwat_bool_data(_c) \
+	container_of_const(_c, struct fwat_bool_data, group)
+#define to_fwat_enum_data(_c) \
+	container_of_const(_c, struct fwat_enum_data, group)
+#define to_fwat_int_data(_c) \
+	container_of_const(_c, struct fwat_int_data, group)
+#define to_fwat_str_data(_c) \
+	container_of_const(_c, struct fwat_str_data, group)
+
+struct fwat_attribute {
+	struct attribute attr;
+	ssize_t (*show)(struct kobject *kobj, struct fwat_attribute *attr,
+			char *buf);
+	ssize_t (*store)(struct kobject *kobj, struct fwat_attribute *attr,
+			 const char *buf, size_t count);
+	int type;
+};
+
+#define to_fwat_attribute(_a) \
+	container_of_const(_a, struct fwat_attribute, attr)
+
+#define __FWAT_ATTR(_name, _mode, _show, _store, _type) \
+	{								\
+		.attr = { .name = __stringify(_name), .mode = _mode },	\
+		.show = _show, .store = _store, .type = _type,		\
+	}
+
+#define FWAT_ATTR_RO(_prefix, _name, _show, _type) \
+	static struct fwat_attribute fwat_##_prefix##_##_name##_attr = \
+		__FWAT_ATTR(_name, 0444, _show, NULL, _type)
+
+#define FWAT_ATTR_RW(_prefix, _name, _show, _store, _type) \
+	static struct fwat_attribute fwat_##_prefix##_##_name##_attr = \
+		__FWAT_ATTR(_name, 0644, _show, _store, _type)
+
+struct fwat_group {
+	const struct fwat_group_data *data;
+	struct device *dev;
+	struct kobject kobj;
+};
+
+#define kobj_to_fwat_group(_k) \
+	container_of_const(_k, struct fwat_group, kobj)
+
 const struct class firmware_attributes_class = {
 	.name = "firmware-attributes",
 };
 EXPORT_SYMBOL_GPL(firmware_attributes_class);
 
+static const char * const fwat_type_labels[] = {
+	[fwat_group_boolean]			= "boolean",
+	[fwat_group_enumeration]		= "enumeration",
+	[fwat_group_integer]			= "integer",
+	[fwat_group_string]			= "string",
+};
+
 static void fwat_device_release(struct device *dev)
 {
 	struct fwat_device *fadev = to_fwat_device(dev);
@@ -24,6 +78,483 @@ static void fwat_device_release(struct device *dev)
 	kfree(fadev);
 }
 
+static ssize_t
+type_show(struct kobject *kobj, struct fwat_attribute *attr, char *buf)
+{
+	return sysfs_emit(buf, "%s\n", fwat_type_labels[attr->type]);
+}
+
+static ssize_t
+display_name_show(struct kobject *kobj, struct fwat_attribute *attr, char *buf)
+{
+	struct fwat_group *group = kobj_to_fwat_group(kobj);
+	const char *disp_name = group->data->display_name;
+
+	if (!disp_name)
+		return -EOPNOTSUPP;
+
+	return sysfs_emit(buf, "%s\n", disp_name);
+}
+
+static ssize_t
+display_name_language_code_show(struct kobject *kobj, struct fwat_attribute *attr,
+				char *buf)
+{
+	struct fwat_group *group = kobj_to_fwat_group(kobj);
+	const char *lang_code = group->data->language_code;
+
+	if (!lang_code)
+		return -EOPNOTSUPP;
+
+	return sysfs_emit(buf, "%s\n", lang_code);
+}
+
+static ssize_t
+bool_group_show(struct kobject *kobj, struct fwat_attribute *attr, char *buf)
+{
+	const struct fwat_group *group = kobj_to_fwat_group(kobj);
+	const struct fwat_bool_data *data = to_fwat_bool_data(group->data);
+	bool val;
+	int ret;
+
+	/* show_override does not affect current_value */
+	if (data->group.show_override && attr->type != fwat_bool_current_value)
+		return data->group.show_override(group->dev, attr->type, buf);
+
+	switch (attr->type) {
+	case fwat_bool_current_value:
+		ret = data->read(group->dev, data->group.id, &val);
+		if (ret < 0)
+			return ret;
+		break;
+	case fwat_bool_default_value:
+		val = data->default_val;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return sysfs_emit(buf, "%s\n", str_yes_no(val));
+}
+
+static ssize_t
+bool_group_store(struct kobject *kobj, struct fwat_attribute *attr, const char *buf,
+		 size_t count)
+{
+	const struct fwat_group *group = kobj_to_fwat_group(kobj);
+	const struct fwat_bool_data *data = to_fwat_bool_data(group->data);
+	bool val;
+	int ret;
+
+	ret = kstrtobool(buf, &val);
+	if (ret)
+		return ret;
+
+	ret = data->write(group->dev, data->group.id, val);
+	if (ret)
+		return ret;
+
+	return count;
+}
+
+static ssize_t
+enum_group_show(struct kobject *kobj, struct fwat_attribute *attr, char *buf)
+{
+	const struct fwat_group *group = kobj_to_fwat_group(kobj);
+	const struct fwat_enum_data *data = to_fwat_enum_data(group->data);
+	int val_idx, sz = 0;
+	int ret;
+
+	/* show_override does not affect current_value */
+	if (data->group.show_override && attr->type != fwat_enum_current_value)
+		return data->group.show_override(group->dev, attr->type, buf);
+
+	switch (attr->type) {
+	case fwat_enum_current_value:
+		ret = data->read(group->dev, data->group.id, &val_idx);
+		if (ret < 0)
+			return ret;
+		break;
+	case fwat_enum_default_value:
+		val_idx = data->default_idx;
+		break;
+	case fwat_enum_possible_values:
+		sz += sysfs_emit_at(buf, sz, "%s", data->possible_vals[0]);
+		for (unsigned int i = 1; data->possible_vals[i]; i++)
+			sz += sysfs_emit_at(buf, sz, ";%s", data->possible_vals[i]);
+		sz += sysfs_emit_at(buf, sz, "\n");
+		return sz;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return sysfs_emit(buf, "%s\n", data->possible_vals[val_idx]);
+}
+
+static ssize_t
+enum_group_store(struct kobject *kobj, struct fwat_attribute *attr, const char *buf,
+		 size_t count)
+{
+	const struct fwat_group *group = kobj_to_fwat_group(kobj);
+	const struct fwat_enum_data *data = to_fwat_enum_data(group->data);
+	int val_idx;
+	int ret;
+
+	val_idx = __sysfs_match_string(data->possible_vals, -1, buf);
+	if (val_idx < 0)
+		return val_idx;
+
+	ret = data->write(group->dev, data->group.id, val_idx);
+	if (ret)
+		return ret;
+
+	return count;
+}
+
+static ssize_t
+int_group_show(struct kobject *kobj, struct fwat_attribute *attr, char *buf)
+{
+	const struct fwat_group *group = kobj_to_fwat_group(kobj);
+	const struct fwat_int_data *data = to_fwat_int_data(group->data);
+	long val;
+	int ret;
+
+	/* show_override does not affect current_value */
+	if (data->group.show_override && attr->type != fwat_int_current_value)
+		return data->group.show_override(group->dev, attr->type, buf);
+
+	switch (attr->type) {
+	case fwat_int_current_value:
+		ret = data->read(group->dev, data->group.id, &val);
+		if (ret < 0)
+			return ret;
+		break;
+	case fwat_int_default_value:
+		val = data->default_val;
+		break;
+	case fwat_int_min_value:
+		val = data->min_val;
+		break;
+	case fwat_int_max_value:
+		val = data->max_val;
+		break;
+	case fwat_int_scalar_increment:
+		val = data->increment;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return sysfs_emit(buf, "%ld\n", val);
+}
+
+static ssize_t
+int_group_store(struct kobject *kobj, struct fwat_attribute *attr, const char *buf,
+		size_t count)
+{
+	const struct fwat_group *group = kobj_to_fwat_group(kobj);
+	const struct fwat_int_data *data = to_fwat_int_data(group->data);
+	long val;
+	int ret;
+
+	ret = kstrtol(buf, 0, &val);
+	if (ret)
+		return ret;
+
+	ret = data->write(group->dev, data->group.id, val);
+	if (ret)
+		return ret;
+
+	return count;
+}
+
+static ssize_t
+str_group_show(struct kobject *kobj, struct fwat_attribute *attr, char *buf)
+{
+	const struct fwat_group *group = kobj_to_fwat_group(kobj);
+	const struct fwat_str_data *data = to_fwat_str_data(group->data);
+	const char *val;
+	long len;
+	int ret;
+
+	/* show_override does not affect current_value */
+	if (data->group.show_override && attr->type != fwat_bool_current_value)
+		return data->group.show_override(group->dev, attr->type, buf);
+
+	switch (attr->type) {
+	case fwat_str_current_value:
+		ret = data->read(group->dev, data->group.id, &val);
+		if (ret < 0)
+			return ret;
+		break;
+	case fwat_str_default_value:
+		val = data->default_val;
+		break;
+	case fwat_str_min_length:
+		len = data->min_len;
+		return sysfs_emit(buf, "%ld\n", len);
+	case fwat_str_max_length:
+		len = data->max_len;
+		return sysfs_emit(buf, "%ld\n", len);
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return sysfs_emit(buf, "%s\n", val);
+}
+
+static ssize_t
+str_group_store(struct kobject *kobj, struct fwat_attribute *attr, const char *buf,
+		size_t count)
+{
+	const struct fwat_group *group = kobj_to_fwat_group(kobj);
+	const struct fwat_str_data *data = to_fwat_str_data(group->data);
+	int ret;
+
+	ret = data->write(group->dev, data->group.id, buf);
+	if (ret)
+		return ret;
+
+	return count;
+}
+
+FWAT_ATTR_RO(all, display_name, display_name_show, FWAT_TYPE_NONE);
+FWAT_ATTR_RO(all, display_name_language_code, display_name_language_code_show, FWAT_TYPE_NONE);
+
+FWAT_ATTR_RO(bool, type, type_show, fwat_group_boolean);
+FWAT_ATTR_RW(bool, current_value, bool_group_show, bool_group_store, fwat_bool_current_value);
+FWAT_ATTR_RO(bool, default_value, bool_group_show, fwat_bool_default_value);
+
+FWAT_ATTR_RO(enum, type, type_show, fwat_group_enumeration);
+FWAT_ATTR_RW(enum, current_value, enum_group_show, enum_group_store, fwat_enum_current_value);
+FWAT_ATTR_RO(enum, default_value, enum_group_show, fwat_enum_default_value);
+FWAT_ATTR_RO(enum, possible_values, enum_group_show, fwat_enum_possible_values);
+
+FWAT_ATTR_RO(int, type, type_show, fwat_group_integer);
+FWAT_ATTR_RW(int, current_value, int_group_show, int_group_store, fwat_int_current_value);
+FWAT_ATTR_RO(int, default_value, int_group_show, fwat_int_default_value);
+FWAT_ATTR_RO(int, min_value, int_group_show, fwat_int_min_value);
+FWAT_ATTR_RO(int, max_value, int_group_show, fwat_int_max_value);
+FWAT_ATTR_RO(int, scalar_increment, int_group_show, fwat_int_scalar_increment);
+
+FWAT_ATTR_RO(str, type, type_show, fwat_group_string);
+FWAT_ATTR_RW(str, current_value, str_group_show, str_group_store, fwat_int_current_value);
+FWAT_ATTR_RO(str, default_value, str_group_show, fwat_str_default_value);
+FWAT_ATTR_RO(str, min_length, str_group_show, fwat_str_min_length);
+FWAT_ATTR_RO(str, max_length, str_group_show, fwat_str_max_length);
+
+static struct attribute *fwat_bool_attrs[] = {
+	&fwat_bool_type_attr.attr,
+	&fwat_all_display_name_attr.attr,
+	&fwat_all_display_name_language_code_attr.attr,
+	&fwat_bool_current_value_attr.attr,
+	&fwat_bool_default_value_attr.attr,
+	NULL
+};
+
+static struct attribute *fwat_enum_attrs[] = {
+	&fwat_enum_type_attr.attr,
+	&fwat_all_display_name_attr.attr,
+	&fwat_all_display_name_language_code_attr.attr,
+	&fwat_enum_current_value_attr.attr,
+	&fwat_enum_default_value_attr.attr,
+	&fwat_enum_possible_values_attr.attr,
+	NULL
+};
+
+static struct attribute *fwat_int_attrs[] = {
+	&fwat_int_type_attr.attr,
+	&fwat_all_display_name_attr.attr,
+	&fwat_all_display_name_language_code_attr.attr,
+	&fwat_int_current_value_attr.attr,
+	&fwat_int_default_value_attr.attr,
+	&fwat_int_min_value_attr.attr,
+	&fwat_int_max_value_attr.attr,
+	&fwat_int_scalar_increment_attr.attr,
+	NULL
+};
+
+static struct attribute *fwat_str_attrs[] = {
+	&fwat_str_type_attr.attr,
+	&fwat_all_display_name_attr.attr,
+	&fwat_all_display_name_language_code_attr.attr,
+	&fwat_str_current_value_attr.attr,
+	&fwat_str_default_value_attr.attr,
+	&fwat_str_min_length_attr.attr,
+	&fwat_str_max_length_attr.attr,
+	NULL
+};
+
+static umode_t fwat_attr_visible(struct kobject *kobj, struct attribute *attr, int n)
+{
+	struct fwat_attribute *fwat_attr = to_fwat_attribute(attr);
+	struct fwat_group *group = kobj_to_fwat_group(kobj);
+	const struct fwat_group_data *data = group->data;
+
+	/* The `type` attribute is always first */
+	if (n == 0)
+		return attr->mode;
+
+	if (attr == &fwat_all_display_name_attr.attr)
+		return data->display_name ? attr->mode : 0;
+
+	if (attr == &fwat_all_display_name_language_code_attr.attr)
+		return data->language_code ? attr->mode : 0;
+
+	/* The `current_value` attribute always has type == 0 */
+	if (!fwat_attr->type)
+		return data->mode;
+
+	return test_bit(fwat_attr->type, &data->fattrs) ? attr->mode : 0;
+}
+
+static umode_t fwat_group_visible(struct kobject *kobj)
+{
+	return true;
+}
+
+DEFINE_SYSFS_GROUP_VISIBLE(fwat);
+
+static const struct attribute_group fwat_bool_group = {
+	.attrs = fwat_bool_attrs,
+	.is_visible = SYSFS_GROUP_VISIBLE(fwat),
+};
+__ATTRIBUTE_GROUPS(fwat_bool);
+
+static const struct attribute_group fwat_enum_group = {
+	.attrs = fwat_enum_attrs,
+	.is_visible = SYSFS_GROUP_VISIBLE(fwat),
+};
+__ATTRIBUTE_GROUPS(fwat_enum);
+
+static const struct attribute_group fwat_int_group = {
+	.attrs = fwat_int_attrs,
+	.is_visible = SYSFS_GROUP_VISIBLE(fwat),
+};
+__ATTRIBUTE_GROUPS(fwat_int);
+
+static const struct attribute_group fwat_str_group = {
+	.attrs = fwat_str_attrs,
+	.is_visible = SYSFS_GROUP_VISIBLE(fwat),
+};
+__ATTRIBUTE_GROUPS(fwat_str);
+
+static ssize_t
+fwat_attr_sysfs_show(struct kobject *kobj, struct attribute *attr, char *buf)
+{
+	struct fwat_attribute *fwat_attr = to_fwat_attribute(attr);
+
+	if (!fwat_attr->show)
+		return -EOPNOTSUPP;
+
+	return fwat_attr->show(kobj, fwat_attr, buf);
+}
+
+static ssize_t
+fwat_attr_sysfs_store(struct kobject *kobj, struct attribute *attr, const char *buf,
+		      size_t count)
+{
+	struct fwat_attribute *fwat_attr = to_fwat_attribute(attr);
+
+	if (!fwat_attr->show)
+		return -EOPNOTSUPP;
+
+	return fwat_attr->store(kobj, fwat_attr, buf, count);
+}
+
+static void fwat_group_release(struct kobject *kobj)
+{
+	struct fwat_group *group = kobj_to_fwat_group(kobj);
+
+	kfree(group);
+}
+
+static const struct sysfs_ops fwat_attr_sysfs_ops = {
+	.show = fwat_attr_sysfs_show,
+	.store = fwat_attr_sysfs_store,
+};
+
+static const struct kobj_type fwat_boolean_ktype = {
+	.sysfs_ops = &fwat_attr_sysfs_ops,
+	.release = fwat_group_release,
+	.default_groups = fwat_bool_groups,
+};
+
+static const struct kobj_type fwat_enumeration_ktype = {
+	.sysfs_ops = &fwat_attr_sysfs_ops,
+	.release = fwat_group_release,
+	.default_groups = fwat_enum_groups,
+};
+
+static const struct kobj_type fwat_integer_ktype = {
+	.sysfs_ops = &fwat_attr_sysfs_ops,
+	.release = fwat_group_release,
+	.default_groups = fwat_int_groups,
+};
+
+static const struct kobj_type fwat_string_ktype = {
+	.sysfs_ops = &fwat_attr_sysfs_ops,
+	.release = fwat_group_release,
+	.default_groups = fwat_str_groups,
+};
+
+static int __fwat_create_group(struct fwat_device *fadev, const struct kobj_type *ktype,
+			       const struct fwat_group_data *data)
+{
+	struct fwat_group *group;
+	int ret;
+
+	group = kzalloc(sizeof(*group), GFP_KERNEL);
+	if (!group)
+		return -ENOMEM;
+
+	group->dev = &fadev->dev;
+	group->data = data;
+
+	group->kobj.kset = fadev->attrs_kset;
+	ret = kobject_init_and_add(&group->kobj, ktype, NULL, "%s", data->name);
+	if (ret) {
+		kobject_put(&group->kobj);
+		return ret;
+	}
+
+	kobject_uevent(&group->kobj, KOBJ_ADD);
+
+	return 0;
+}
+
+static void fwat_remove_auto_groups(struct fwat_device *fadev)
+{
+	struct kobject *pos, *n;
+
+	list_for_each_entry_safe(pos, n, &fadev->attrs_kset->list, entry)
+		kobject_put(pos);
+}
+
+int fwat_create_bool_group(struct fwat_device *fadev, const struct fwat_bool_data *data)
+{
+	return __fwat_create_group(fadev, &fwat_boolean_ktype, &data->group);
+}
+EXPORT_SYMBOL_GPL(fwat_create_bool_group);
+
+int fwat_create_enum_group(struct fwat_device *fadev, const struct fwat_enum_data *data)
+{
+	return __fwat_create_group(fadev, &fwat_enumeration_ktype, &data->group);
+}
+EXPORT_SYMBOL_GPL(fwat_create_enum_group);
+
+int fwat_create_int_group(struct fwat_device *fadev, const struct fwat_int_data *data)
+{
+	return __fwat_create_group(fadev, &fwat_integer_ktype, &data->group);
+}
+EXPORT_SYMBOL_GPL(fwat_create_int_group);
+
+int fwat_create_str_group(struct fwat_device *fadev, const struct fwat_str_data *data)
+{
+	return __fwat_create_group(fadev, &fwat_string_ktype, &data->group);
+}
+EXPORT_SYMBOL_GPL(fwat_create_str_group);
+
 /**
  * fwat_device_register - Create and register a firmware-attributes class
  *			  device
@@ -89,6 +620,7 @@ void fwat_device_unregister(struct fwat_device *fadev)
 	if (!fadev)
 		return;
 
+	fwat_remove_auto_groups(fadev);
 	sysfs_remove_groups(&fadev->attrs_kset->kobj, fadev->groups);
 	kset_unregister(fadev->attrs_kset);
 	device_unregister(&fadev->dev);
diff --git a/drivers/platform/x86/firmware_attributes_class.h b/drivers/platform/x86/firmware_attributes_class.h
index 048fd0904f767357ef856e687ec4cf3260016ec6..e8868ce05b595eda94a98975428391b9f9341e3d 100644
--- a/drivers/platform/x86/firmware_attributes_class.h
+++ b/drivers/platform/x86/firmware_attributes_class.h
@@ -10,6 +10,7 @@
 #include <linux/device/class.h>
 #include <linux/kobject.h>
 #include <linux/sysfs.h>
+#include <linux/list.h>
 
 extern const struct class firmware_attributes_class;
 
@@ -27,6 +28,340 @@ struct fwat_device {
 
 #define to_fwat_device(_d)	container_of_const(_d, struct fwat_device, dev)
 
+enum fwat_group_type {
+	fwat_group_boolean,
+	fwat_group_enumeration,
+	fwat_group_integer,
+	fwat_group_string,
+};
+
+enum fwat_bool_attrs {
+	fwat_bool_current_value,
+	fwat_bool_default_value,
+	fwat_bool_attrs_last
+};
+
+#define FWAT_BOOL_CURRENT_VALUE			BIT(fwat_bool_current_value)
+#define FWAT_BOOL_DEFAULT_VALUE			BIT(fwat_bool_default_value)
+#define FWAT_BOOL_ALL_ATTRS			GENMASK(fwat_bool_attrs_last, 0)
+
+enum fwat_enum_attrs {
+	fwat_enum_current_value,
+	fwat_enum_default_value,
+	fwat_enum_possible_values,
+	fwat_enum_attrs_last
+};
+
+#define FWAT_ENUM_CURRENT_VALUE			BIT(fwat_enum_current_value)
+#define FWAT_ENUM_DEFAULT_VALUE			BIT(fwat_enum_default_value)
+#define FWAT_ENUM_POSSIBLE_VALUES		BIT(fwat_enum_possible_values)
+#define FWAT_ENUM_ALL_ATTRS			GENMASK(fwat_enum_attrs_last, 0)
+
+enum fwat_int_attrs {
+	fwat_int_current_value,
+	fwat_int_default_value,
+	fwat_int_min_value,
+	fwat_int_max_value,
+	fwat_int_scalar_increment,
+	fwat_int_attrs_last
+};
+
+#define FWAT_INT_CURRENT_VALUE			BIT(fwat_int_current_value)
+#define FWAT_INT_DEFAULT_VALUE			BIT(fwat_int_default_value)
+#define FWAT_INT_MIN_VALUE			BIT(fwat_int_min_value)
+#define FWAT_INT_MAX_VALUE			BIT(fwat_int_max_value)
+#define FWAT_INT_SCALAR_INCREMENT		BIT(fwat_int_scalar_increment)
+#define FWAT_INT_ALL_ATTRS			GENMASK(fwat_int_attrs_last, 0)
+
+enum fwat_str_attrs {
+	fwat_str_current_value,
+	fwat_str_default_value,
+	fwat_str_min_length,
+	fwat_str_max_length,
+	fwat_str_attrs_last
+};
+
+#define FWAT_STR_CURRENT_VALUE			BIT(fwat_str_current_value)
+#define FWAT_STR_DEFAULT_VALUE			BIT(fwat_str_default_value)
+#define FWAT_STR_MIN_LENGTH			BIT(fwat_str_min_length)
+#define FWAT_STR_MAX_LENGTH			BIT(fwat_str_max_length)
+#define FWAT_STR_ALL_ATTRS			GENMASK(fwat_str_attrs_last, 0)
+
+static_assert(fwat_bool_current_value == 0);
+static_assert(fwat_enum_current_value == 0);
+static_assert(fwat_int_current_value == 0);
+static_assert(fwat_str_current_value == 0);
+
+/**
+ * struct fwat_group_data - Data struct common between group types
+ * @id: Group ID defined by the user.
+ * @name: Name of the group.
+ * @display_name: Name showed in the display_name attribute. (Optional)
+ * @language_code: Language code showed in the display_name_language_code
+ *                 attribute. (Optional)
+ * @mode: Mode for the current_value attribute. All other attributes will have
+ *        0444 permissions.
+ * @fattrs: Bitmap of selected attributes for this group type.
+ * @show_override: Custom show method for attributes in this group, except for
+ *		   the current_value attribute, for which the a `read` callback
+ *		   will still be used. (Optional)
+ *
+ * NOTE: This struct is not meant to be defined directly. It is supposed to be
+ * embedded and defined as part of fwat_[type]_data structs.
+ */
+struct fwat_group_data {
+	long id;
+	umode_t mode;
+	const char *name;
+	const char *display_name;
+	const char *language_code;
+	unsigned long fattrs;
+	ssize_t (*show_override)(struct device *dev, int type, char *buf);
+};
+
+/**
+ * struct fwat_bool_data - Data struct for the boolean group type
+ * @read: Read callback for the current_value attribute.
+ * @write: Write callback for the current_value attribute.
+ * @default_val: Default value.
+ * @group: Group data.
+ */
+struct fwat_bool_data {
+	int (*read)(struct device *dev, long id, bool *val);
+	int (*write)(struct device *dev, long id, bool val);
+	bool default_val;
+	struct fwat_group_data group;
+};
+
+/**
+ * struct fwat_enum_data - Data struct for the enumeration group type
+ * @read: Read callback for the current_value attribute.
+ * @write: Write callback for the current_value attribute.
+ * @default_idx: Index of the default value in the @possible_vals array.
+ * @possible_vals: Array of possible value strings for this group type.
+ * @group: Group data.
+ *
+ * NOTE: The `val_idx` argument in the @write callback is guaranteed to be a
+ *       valid (within bounds) index. However, the user is in charge of writing
+ *       valid indexes to the `*val_idx` argument of the @read callback.
+ *       Failing to do so may result in an OOB access.
+ */
+struct fwat_enum_data {
+	int (*read)(struct device *dev, long id, int *val_idx);
+	int (*write)(struct device *dev, long id, int val_idx);
+	int default_idx;
+	const char * const *possible_vals;
+	struct fwat_group_data group;
+};
+
+/**
+ * struct fwat_int_data - Data struct for the integer group type
+ * @read: Read callback for the current_value attribute.
+ * @write: Write callback for the current_value attribute.
+ * @default_val: Default value.
+ * @min_val: Minimum value.
+ * @max_val: Maximum value.
+ * @increment: Scalar increment for this value.
+ * @group: Group data.
+ *
+ * NOTE: The @min_val, @max_val, @increment constraints are merely informative.
+ *       These values are not enforced in any of the callbacks.
+ */
+struct fwat_int_data {
+	int (*read)(struct device *dev, long id, long *val);
+	int (*write)(struct device *dev, long id, long val);
+	long default_val;
+	long min_val;
+	long max_val;
+	long increment;
+	struct fwat_group_data group;
+};
+
+/**
+ * struct fwat_str_data - Data struct for the string group type
+ * @read: Read callback for the current_value attribute.
+ * @write: Write callback for the current_value attribute.
+ * @default_val: Default value.
+ * @min_len: Minimum string length.
+ * @max_len: Maximum string length.
+ * @group: Group data.
+ *
+ * NOTE: The @min_len, @max_len constraints are merely informative. These
+ *       values are not enforced in any of the callbacks.
+ */
+struct fwat_str_data {
+	int (*read)(struct device *dev, long id, const char **buf);
+	int (*write)(struct device *dev, long id, const char *buf);
+	const char *default_val;
+	long min_len;
+	long max_len;
+	struct fwat_group_data group;
+};
+
+#define __FWAT_GROUP(_name, _disp_name, _mode, _fattrs) \
+	{ .name = __stringify(_name), .display_name = _disp_name, .mode = _mode, .fattrs = _fattrs }
+
+/**
+ * DEFINE_FWAT_BOOL_GROUP - Convenience macro to quickly define an static
+ *                          struct fwat_bool_data instance
+ * @_name: Name of the group.
+ * @_disp_name: Name showed in the display_name attribute. (Optional)
+ * @_def_val: Default value.
+ * @_mode: Mode for the current_value attribute. All other attributes will have
+ *         0444 permissions.
+ * @_fattrs: Bitmap of selected attributes for this group type.
+ *
+ * `read` and `write` callbacks are required to be already defined as
+ * `_name##_read` and `_name##_write` respectively.
+ */
+#define DEFINE_FWAT_BOOL_GROUP(_name, _disp_name, _def_val, _mode, _fattrs) \
+	static const struct fwat_bool_data _name##_group_data = {	\
+		.read = _name##_read,					\
+		.write = _name##_write,					\
+		.default_val = _def_val,				\
+		.group = __FWAT_GROUP(_name, _disp_name, _mode, _fattrs), \
+	}
+
+/**
+ * DEFINE_FWAT_ENUM_GROUP - Convenience macro to quickly define an static
+ *                          struct fwat_enum_data instance
+ * @_name: Name of the group.
+ * @_disp_name: Name showed in the display_name attribute. (Optional)
+ * @_def_idx: Index of the default value in the @_poss_vals array.
+ * @_poss_vals: Array of possible value strings for this group type.
+ * @_mode: Mode for the current_value attribute. All other attributes will have
+ *         0444 permissions.
+ * @_fattrs: Bitmap of selected attributes for this group type.
+ *
+ * `read` and `write` callbacks are required to be already defined as
+ * `_name##_read` and `_name##_write` respectively.
+ *
+ * NOTE: The `val_idx` argument in the `write` callback is guaranteed to be a
+ *       valid (within bounds) index. However, the user is in charge of writing
+ *       valid indexes to the `*val_idx` argument of the `read` callback.
+ *       Failing to do so may result in an OOB access.
+ */
+#define DEFINE_FWAT_ENUM_GROUP(_name, _disp_name, _poss_vals, _def_idx, _mode, _fattrs) \
+	static const struct fwat_enum_data _name##_group_data = {	\
+		.read = _name##_read,					\
+		.write = _name##_write,					\
+		.default_idx = _def_idx,				\
+		.possible_vals = _poss_vals,				\
+		.group = __FWAT_GROUP(_name, _disp_name, _mode, _fattrs), \
+	}
+
+/**
+ * DEFINE_FWAT_INT_GROUP - Convenience macro to quickly define an static
+ *                         struct fwat_int_data instance
+ * @_name: Name of the group.
+ * @_disp_name: Name showed in the display_name attribute. (Optional)
+ * @_def_val: Default value.
+ * @_min: Minimum value.
+ * @_max: Maximum value.
+ * @_inc: Scalar increment for this value.
+ * @_mode: Mode for the current_value attribute. All other attributes will have
+ *         0444 permissions.
+ * @_fattrs: Bitmap of selected attributes for this group type.
+ *
+ * `read` and `write` callbacks are required to be already defined as
+ * `_name##_read` and `_name##_write` respectively.
+ *
+ * NOTE: The @_min, @_max, @_inc constraints are merely informative. These
+ *       values are not enforced in any of the callbacks.
+ */
+#define DEFINE_FWAT_INT_GROUP(_name, _disp_name, _def_val, _min, _max, _inc, _mode, _fattrs) \
+	static const struct fwat_int_data _name##_group_data = {	\
+		.read = _name##_read,					\
+		.write = _name##_write,					\
+		.default_val = _def_val,				\
+		.min_val = _min,					\
+		.max_val = _max,					\
+		.increment = _inc,					\
+		.group = __FWAT_GROUP(_name, _disp_name, _mode, _fattrs), \
+	}
+
+/**
+ * DEFINE_FWAT_STR_GROUP - Convenience macro to quickly define an static
+ *                         struct fwat_str_data instance
+ * @_name: Name of the group.
+ * @_disp_name: Name showed in the display_name attribute. (Optional)
+ * @_def_val: Default value.
+ * @_min: Minimum string length.
+ * @_max: Maximum string length.
+ * @_mode: Mode for the current_value attribute. All other attributes will have
+ *         0444 permissions.
+ * @_fattrs: Bitmap of selected attributes for this group type.
+ *
+ * `read` and `write` callbacks are required to be already defined as
+ * `_name##_read` and `_name##_write` respectively.
+ *
+ * NOTE: The @_min, @_max constraints are merely informative. These values are
+ *       not enforced in any of the callbacks.
+ */
+#define DEFINE_FWAT_STR_GROUP(_name, _disp_name, _def_val, _min, _max, _mode, _fattrs) \
+	static const struct fwat_str_data _name##_group_data = {	\
+		.read = _name##_read,					\
+		.write = _name##_write,					\
+		.default_val = _def_val,				\
+		.min_len = _min,					\
+		.max_len = _max,					\
+		.group = __FWAT_GROUP(_name, _disp_name, _mode, _fattrs), \
+	}
+
+int fwat_create_bool_group(struct fwat_device *fadev,
+			   const struct fwat_bool_data *data);
+int fwat_create_enum_group(struct fwat_device *fadev,
+			   const struct fwat_enum_data *data);
+int fwat_create_int_group(struct fwat_device *fadev,
+			  const struct fwat_int_data *data);
+int fwat_create_str_group(struct fwat_device *fadev,
+			  const struct fwat_str_data *data);
+
+/**
+ * fwat_create_group - Convenience generic macro to create a group
+ * @_dev: fwat_device
+ * @_data: One of fwat_{bool,enum,int,str}_data instance
+ *
+ * This macro (and associated functions) creates a sysfs group under the
+ * 'attributes' directory, which is located in the class device root directory.
+ *
+ * See Documentation/ABI/testing/sysfs-class-firmware-attributes for details.
+ *
+ * The @_data associated with this group may be created either statically,
+ * through DEFINE_FWAT_*_GROUP macros or dynamically, in which case the user
+ * would have allocate and fill the struct manually. The dynamic approach should
+ * be preferred when group constraints and/or visibility is decided dynamically.
+ *
+ * Example:
+ *
+ * static int stat_read(...){...};
+ * static int stat_write(...){...};
+ *
+ * DEFINE_FWAT_(BOOL|ENUM|INT|STR)_GROUP(stat, ...);
+ *
+ * static int create_groups(struct fwat_device *fadev)
+ * {
+ *	struct fwat_enum_data *dyn_group_data;
+ *
+ *	dyn_group_data = kzalloc(...);
+ *	// Fill the data
+ *	...
+ *	fwat_create_group(fadev, &stat_group_data);
+ *	fwat_create_group(fadev, &dyn_group_data);
+ *	fwat_create_group(...);
+ *	...
+ * }
+ *
+ * Return: 0 on success, -errno on failure
+ */
+#define fwat_create_group(_dev, _data) \
+	_Generic((_data),							\
+		 const struct fwat_bool_data * : fwat_create_bool_group,	\
+		 const struct fwat_enum_data * : fwat_create_enum_group,	\
+		 const struct fwat_int_data * : fwat_create_int_group,		\
+		 const struct fwat_str_data * : fwat_create_str_group)		\
+		(_dev, _data)
+
 struct fwat_device * __must_check
 fwat_device_register(struct device *parent, const char *name, void *drvdata,
 		     const struct attribute_group **groups);

-- 
2.50.0

Re: [PATCH v5 2/6] platform/x86: firmware_attributes_class: Add high level API for the attributes interface

[Thomas Weißschuh]

On 2025-07-05 00:33:57-0300, Kurt Borja wrote:
> Add high level API to aid in the creation of attribute groups attached
> to the `attrs_kobj` (per ABI specification).
> 
> This new API lets users configure each group, either statically or
> dynamically through a (per type) data struct and then create this group
> through the generic fwat_create_group() macro.

How does this deal with non-standard types and non-standard attribute
"subattributes"?

Did you compare the code sizes of the framework and using drivers before
and after? It looks like it will grow quite a bit.

> 
> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
> ---
>  drivers/platform/x86/firmware_attributes_class.c | 532 +++++++++++++++++++++++
>  drivers/platform/x86/firmware_attributes_class.h | 335 ++++++++++++++
>  2 files changed, 867 insertions(+)
> 
> diff --git a/drivers/platform/x86/firmware_attributes_class.c b/drivers/platform/x86/firmware_attributes_class.c
> index 290364202cce64bb0e9046e0b2bbb8d85e2cbc6f..96473b3b1a2a87cf21a6e2a9a14d72ae322c94ae 100644
> --- a/drivers/platform/x86/firmware_attributes_class.c
> +++ b/drivers/platform/x86/firmware_attributes_class.c
> @@ -10,13 +10,67 @@
>  #include <linux/module.h>
>  #include <linux/slab.h>
>  #include <linux/types.h>
> +#include <linux/string_choices.h>
>  #include "firmware_attributes_class.h"
>  
> +#define FWAT_TYPE_NONE				-1

Braces around '-1'.

One the other hand this define is only used in two places and the value
does not matter and is only passed because the macros expect something.
Maybe move the definition to where it is used and do:

__FWAT_TYPE_NONE (-1) /* dummy value, never evaluated */

> +
> +#define to_fwat_bool_data(_c) \
> +	container_of_const(_c, struct fwat_bool_data, group)
> +#define to_fwat_enum_data(_c) \
> +	container_of_const(_c, struct fwat_enum_data, group)
> +#define to_fwat_int_data(_c) \
> +	container_of_const(_c, struct fwat_int_data, group)
> +#define to_fwat_str_data(_c) \
> +	container_of_const(_c, struct fwat_str_data, group)
> +
> +struct fwat_attribute {
> +	struct attribute attr;
> +	ssize_t (*show)(struct kobject *kobj, struct fwat_attribute *attr,

Please make the 'struct fwat_attribute *' argument const.
Somebody (me) is currently working on constifying 'struct attribute'.

> +			char *buf);
> +	ssize_t (*store)(struct kobject *kobj, struct fwat_attribute *attr,
> +			 const char *buf, size_t count);
> +	int type;
> +};
> +
> +#define to_fwat_attribute(_a) \
> +	container_of_const(_a, struct fwat_attribute, attr)
> +
> +#define __FWAT_ATTR(_name, _mode, _show, _store, _type) \
> +	{								\

Move the opening brace to the line above.

> +		.attr = { .name = __stringify(_name), .mode = _mode },	\
> +		.show = _show, .store = _store, .type = _type,		\
> +	}
> +

<snip>

> +static ssize_t
> +enum_group_show(struct kobject *kobj, struct fwat_attribute *attr, char *buf)
> +{
> +	const struct fwat_group *group = kobj_to_fwat_group(kobj);
> +	const struct fwat_enum_data *data = to_fwat_enum_data(group->data);
> +	int val_idx, sz = 0;
> +	int ret;
> +
> +	/* show_override does not affect current_value */
> +	if (data->group.show_override && attr->type != fwat_enum_current_value)
> +		return data->group.show_override(group->dev, attr->type, buf);
> +
> +	switch (attr->type) {
> +	case fwat_enum_current_value:
> +		ret = data->read(group->dev, data->group.id, &val_idx);
> +		if (ret < 0)
> +			return ret;
> +		break;
> +	case fwat_enum_default_value:
> +		val_idx = data->default_idx;
> +		break;
> +	case fwat_enum_possible_values:
> +		sz += sysfs_emit_at(buf, sz, "%s", data->possible_vals[0]);

Slightly easier to read if the line above is:

	sz = sysfs_emit_at(buf, 0, "%s", data->possible_vals[0]);

And drop the initialization of sz.


> +		for (unsigned int i = 1; data->possible_vals[i]; i++)
> +			sz += sysfs_emit_at(buf, sz, ";%s", data->possible_vals[i]);
> +		sz += sysfs_emit_at(buf, sz, "\n");
> +		return sz;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return sysfs_emit(buf, "%s\n", data->possible_vals[val_idx]);
> +}

<snip>

> +static umode_t fwat_attr_visible(struct kobject *kobj, struct attribute *attr, int n)
> +{
> +	struct fwat_attribute *fwat_attr = to_fwat_attribute(attr);

const

> +	struct fwat_group *group = kobj_to_fwat_group(kobj);
> +	const struct fwat_group_data *data = group->data;
> +
> +	/* The `type` attribute is always first */
> +	if (n == 0)
> +		return attr->mode;
> +
> +	if (attr == &fwat_all_display_name_attr.attr)
> +		return data->display_name ? attr->mode : 0;
> +
> +	if (attr == &fwat_all_display_name_language_code_attr.attr)
> +		return data->language_code ? attr->mode : 0;
> +
> +	/* The `current_value` attribute always has type == 0 */
> +	if (!fwat_attr->type)
> +		return data->mode;
> +
> +	return test_bit(fwat_attr->type, &data->fattrs) ? attr->mode : 0;
> +}
> +
> +static umode_t fwat_group_visible(struct kobject *kobj)
> +{
> +	return true;
> +}
> +
> +DEFINE_SYSFS_GROUP_VISIBLE(fwat);

Why DEFINE_SYSFS_GROUP_VISIBLE() if the per-group callback doesn't do
anything?

> +
> +static const struct attribute_group fwat_bool_group = {
> +	.attrs = fwat_bool_attrs,
> +	.is_visible = SYSFS_GROUP_VISIBLE(fwat),
> +};
> +__ATTRIBUTE_GROUPS(fwat_bool);

<snip>

>  /**
>   * fwat_device_register - Create and register a firmware-attributes class
>   *			  device
> @@ -89,6 +620,7 @@ void fwat_device_unregister(struct fwat_device *fadev)
>  	if (!fadev)
>  		return;
>  
> +	fwat_remove_auto_groups(fadev);

As before, the groups are children of the device so should be removed
automatically.

>  	sysfs_remove_groups(&fadev->attrs_kset->kobj, fadev->groups);
>  	kset_unregister(fadev->attrs_kset);
>  	device_unregister(&fadev->dev);
> diff --git a/drivers/platform/x86/firmware_attributes_class.h b/drivers/platform/x86/firmware_attributes_class.h
> index 048fd0904f767357ef856e687ec4cf3260016ec6..e8868ce05b595eda94a98975428391b9f9341e3d 100644
> --- a/drivers/platform/x86/firmware_attributes_class.h
> +++ b/drivers/platform/x86/firmware_attributes_class.h
> @@ -10,6 +10,7 @@
>  #include <linux/device/class.h>
>  #include <linux/kobject.h>
>  #include <linux/sysfs.h>
> +#include <linux/list.h>

Unnecessary for public header.

>  
>  extern const struct class firmware_attributes_class;
>  
> @@ -27,6 +28,340 @@ struct fwat_device {
>  
>  #define to_fwat_device(_d)	container_of_const(_d, struct fwat_device, dev)
>  
> +enum fwat_group_type {
> +	fwat_group_boolean,

I have a slight preference for "fwat_group_type_boolean".

> +	fwat_group_enumeration,
> +	fwat_group_integer,
> +	fwat_group_string,
> +};
> +
> +enum fwat_bool_attrs {
> +	fwat_bool_current_value,
> +	fwat_bool_default_value,
> +	fwat_bool_attrs_last
> +};
> +
> +#define FWAT_BOOL_CURRENT_VALUE			BIT(fwat_bool_current_value)
> +#define FWAT_BOOL_DEFAULT_VALUE			BIT(fwat_bool_default_value)
> +#define FWAT_BOOL_ALL_ATTRS			GENMASK(fwat_bool_attrs_last, 0)

Is the ALL_ATTRS really necessary? It could lead to issues when new
optional attributes are added to the core.

> +
> +enum fwat_enum_attrs {
> +	fwat_enum_current_value,
> +	fwat_enum_default_value,
> +	fwat_enum_possible_values,
> +	fwat_enum_attrs_last
> +};
> +
> +#define FWAT_ENUM_CURRENT_VALUE			BIT(fwat_enum_current_value)
> +#define FWAT_ENUM_DEFAULT_VALUE			BIT(fwat_enum_default_value)
> +#define FWAT_ENUM_POSSIBLE_VALUES		BIT(fwat_enum_possible_values)
> +#define FWAT_ENUM_ALL_ATTRS			GENMASK(fwat_enum_attrs_last, 0)
> +
> +enum fwat_int_attrs {
> +	fwat_int_current_value,
> +	fwat_int_default_value,
> +	fwat_int_min_value,
> +	fwat_int_max_value,
> +	fwat_int_scalar_increment,
> +	fwat_int_attrs_last
> +};
> +
> +#define FWAT_INT_CURRENT_VALUE			BIT(fwat_int_current_value)
> +#define FWAT_INT_DEFAULT_VALUE			BIT(fwat_int_default_value)
> +#define FWAT_INT_MIN_VALUE			BIT(fwat_int_min_value)
> +#define FWAT_INT_MAX_VALUE			BIT(fwat_int_max_value)
> +#define FWAT_INT_SCALAR_INCREMENT		BIT(fwat_int_scalar_increment)
> +#define FWAT_INT_ALL_ATTRS			GENMASK(fwat_int_attrs_last, 0)
> +
> +enum fwat_str_attrs {
> +	fwat_str_current_value,
> +	fwat_str_default_value,
> +	fwat_str_min_length,
> +	fwat_str_max_length,
> +	fwat_str_attrs_last
> +};
> +
> +#define FWAT_STR_CURRENT_VALUE			BIT(fwat_str_current_value)
> +#define FWAT_STR_DEFAULT_VALUE			BIT(fwat_str_default_value)
> +#define FWAT_STR_MIN_LENGTH			BIT(fwat_str_min_length)
> +#define FWAT_STR_MAX_LENGTH			BIT(fwat_str_max_length)
> +#define FWAT_STR_ALL_ATTRS			GENMASK(fwat_str_attrs_last, 0)
> +
> +static_assert(fwat_bool_current_value == 0);
> +static_assert(fwat_enum_current_value == 0);
> +static_assert(fwat_int_current_value == 0);
> +static_assert(fwat_str_current_value == 0);

Move this into the the .c file right next to the code relying on this
invariant.

<snip>

> +/**
> + * DEFINE_FWAT_ENUM_GROUP - Convenience macro to quickly define an static
> + *                          struct fwat_enum_data instance
> + * @_name: Name of the group.
> + * @_disp_name: Name showed in the display_name attribute. (Optional)
> + * @_def_idx: Index of the default value in the @_poss_vals array.
> + * @_poss_vals: Array of possible value strings for this group type.
> + * @_mode: Mode for the current_value attribute. All other attributes will have
> + *         0444 permissions.
> + * @_fattrs: Bitmap of selected attributes for this group type.
> + *
> + * `read` and `write` callbacks are required to be already defined as
> + * `_name##_read` and `_name##_write` respectively.
> + *
> + * NOTE: The `val_idx` argument in the `write` callback is guaranteed to be a
> + *       valid (within bounds) index. However, the user is in charge of writing
> + *       valid indexes to the `*val_idx` argument of the `read` callback.
> + *       Failing to do so may result in an OOB access.
> + */
> +#define DEFINE_FWAT_ENUM_GROUP(_name, _disp_name, _poss_vals, _def_idx, _mode, _fattrs) \
> +	static const struct fwat_enum_data _name##_group_data = {	\

Align all backslashes of the definition.

> +		.read = _name##_read,					\
> +		.write = _name##_write,					\
> +		.default_idx = _def_idx,				\
> +		.possible_vals = _poss_vals,				\
> +		.group = __FWAT_GROUP(_name, _disp_name, _mode, _fattrs), \
> +	}

<snip>

> +int fwat_create_bool_group(struct fwat_device *fadev,
> +			   const struct fwat_bool_data *data);
> +int fwat_create_enum_group(struct fwat_device *fadev,
> +			   const struct fwat_enum_data *data);
> +int fwat_create_int_group(struct fwat_device *fadev,
> +			  const struct fwat_int_data *data);
> +int fwat_create_str_group(struct fwat_device *fadev,
> +			  const struct fwat_str_data *data);

If these are not meant to be used by users, give them a leading
underscore?

> +
> +/**
> + * fwat_create_group - Convenience generic macro to create a group
> + * @_dev: fwat_device
> + * @_data: One of fwat_{bool,enum,int,str}_data instance
> + *
> + * This macro (and associated functions) creates a sysfs group under the
> + * 'attributes' directory, which is located in the class device root directory.
> + *
> + * See Documentation/ABI/testing/sysfs-class-firmware-attributes for details.
> + *
> + * The @_data associated with this group may be created either statically,
> + * through DEFINE_FWAT_*_GROUP macros or dynamically, in which case the user
> + * would have allocate and fill the struct manually. The dynamic approach should
> + * be preferred when group constraints and/or visibility is decided dynamically.

I'd prefer if users don't need dynamic data allocations if possible.
Let's see how it works out with the existing drivers.

> + *
> + * Example:
> + *
> + * static int stat_read(...){...};
> + * static int stat_write(...){...};
> + *
> + * DEFINE_FWAT_(BOOL|ENUM|INT|STR)_GROUP(stat, ...);
> + *
> + * static int create_groups(struct fwat_device *fadev)
> + * {
> + *	struct fwat_enum_data *dyn_group_data;
> + *
> + *	dyn_group_data = kzalloc(...);
> + *	// Fill the data
> + *	...
> + *	fwat_create_group(fadev, &stat_group_data);
> + *	fwat_create_group(fadev, &dyn_group_data);
> + *	fwat_create_group(...);
> + *	...
> + * }
> + *
> + * Return: 0 on success, -errno on failure
> + */
> +#define fwat_create_group(_dev, _data) \
> +	_Generic((_data),							\
> +		 const struct fwat_bool_data * : fwat_create_bool_group,	\
> +		 const struct fwat_enum_data * : fwat_create_enum_group,	\
> +		 const struct fwat_int_data * : fwat_create_int_group,		\
> +		 const struct fwat_str_data * : fwat_create_str_group)		\
> +		(_dev, _data)
> +
>  struct fwat_device * __must_check
>  fwat_device_register(struct device *parent, const char *name, void *drvdata,
>  		     const struct attribute_group **groups);
> 
> -- 
> 2.50.0
> 

Re: [PATCH v5 2/6] platform/x86: firmware_attributes_class: Add high level API for the attributes interface

[Kurt Borja]

On Sun Jul 6, 2025 at 4:40 AM -03, Thomas Weißschuh wrote:
> On 2025-07-05 00:33:57-0300, Kurt Borja wrote:
>> Add high level API to aid in the creation of attribute groups attached
>> to the `attrs_kobj` (per ABI specification).
>> 
>> This new API lets users configure each group, either statically or
>> dynamically through a (per type) data struct and then create this group
>> through the generic fwat_create_group() macro.
>
> How does this deal with non-standard types and non-standard attribute
> "subattributes"?

IMO non-standard types aren't priority right now. Even if we accomodate
something for those, it would be rarely used, if ever. Also the
attrs_kset is exposed to users, so they can add custom kobjects to it
(with some consideration about ownership ofc).

Non-standard "subattributes" are easy to accomodate for. We can take
extra groups in fwat_create_group().

>
> Did you compare the code sizes of the framework and using drivers before
> and after? It looks like it will grow quite a bit.

I'll work a comparison for the next version.

By looking at users of the class (think-lmi for example). Drivers
already use some kind of fwat_group_data structure, which stores values
for the static properties of each "firmware attribute". These are also
dynamically allocated in most cases because FW provides enumeration
methods.

This makes me think that total used memory (static and dynamic) should
be fairly similar, in most cases.

>
>> 
>> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
>> ---
>>  drivers/platform/x86/firmware_attributes_class.c | 532 +++++++++++++++++++++++
>>  drivers/platform/x86/firmware_attributes_class.h | 335 ++++++++++++++
>>  2 files changed, 867 insertions(+)
>> 
>> diff --git a/drivers/platform/x86/firmware_attributes_class.c b/drivers/platform/x86/firmware_attributes_class.c
>> index 290364202cce64bb0e9046e0b2bbb8d85e2cbc6f..96473b3b1a2a87cf21a6e2a9a14d72ae322c94ae 100644
>> --- a/drivers/platform/x86/firmware_attributes_class.c
>> +++ b/drivers/platform/x86/firmware_attributes_class.c
>> @@ -10,13 +10,67 @@
>>  #include <linux/module.h>
>>  #include <linux/slab.h>
>>  #include <linux/types.h>
>> +#include <linux/string_choices.h>
>>  #include "firmware_attributes_class.h"
>>  
>> +#define FWAT_TYPE_NONE				-1
>
> Braces around '-1'.
>
> One the other hand this define is only used in two places and the value
> does not matter and is only passed because the macros expect something.
> Maybe move the definition to where it is used and do:
>
> __FWAT_TYPE_NONE (-1) /* dummy value, never evaluated */
>
>> +
>> +#define to_fwat_bool_data(_c) \
>> +	container_of_const(_c, struct fwat_bool_data, group)
>> +#define to_fwat_enum_data(_c) \
>> +	container_of_const(_c, struct fwat_enum_data, group)
>> +#define to_fwat_int_data(_c) \
>> +	container_of_const(_c, struct fwat_int_data, group)
>> +#define to_fwat_str_data(_c) \
>> +	container_of_const(_c, struct fwat_str_data, group)
>> +
>> +struct fwat_attribute {
>> +	struct attribute attr;
>> +	ssize_t (*show)(struct kobject *kobj, struct fwat_attribute *attr,
>
> Please make the 'struct fwat_attribute *' argument const.
> Somebody (me) is currently working on constifying 'struct attribute'.
>
>> +			char *buf);
>> +	ssize_t (*store)(struct kobject *kobj, struct fwat_attribute *attr,
>> +			 const char *buf, size_t count);
>> +	int type;
>> +};
>> +
>> +#define to_fwat_attribute(_a) \
>> +	container_of_const(_a, struct fwat_attribute, attr)
>> +
>> +#define __FWAT_ATTR(_name, _mode, _show, _store, _type) \
>> +	{								\
>
> Move the opening brace to the line above.
>
>> +		.attr = { .name = __stringify(_name), .mode = _mode },	\
>> +		.show = _show, .store = _store, .type = _type,		\
>> +	}
>> +
>
> <snip>
>
>> +static ssize_t
>> +enum_group_show(struct kobject *kobj, struct fwat_attribute *attr, char *buf)
>> +{
>> +	const struct fwat_group *group = kobj_to_fwat_group(kobj);
>> +	const struct fwat_enum_data *data = to_fwat_enum_data(group->data);
>> +	int val_idx, sz = 0;
>> +	int ret;
>> +
>> +	/* show_override does not affect current_value */
>> +	if (data->group.show_override && attr->type != fwat_enum_current_value)
>> +		return data->group.show_override(group->dev, attr->type, buf);
>> +
>> +	switch (attr->type) {
>> +	case fwat_enum_current_value:
>> +		ret = data->read(group->dev, data->group.id, &val_idx);
>> +		if (ret < 0)
>> +			return ret;
>> +		break;
>> +	case fwat_enum_default_value:
>> +		val_idx = data->default_idx;
>> +		break;
>> +	case fwat_enum_possible_values:
>> +		sz += sysfs_emit_at(buf, sz, "%s", data->possible_vals[0]);
>
> Slightly easier to read if the line above is:
>
> 	sz = sysfs_emit_at(buf, 0, "%s", data->possible_vals[0]);
>
> And drop the initialization of sz.
>
>
>> +		for (unsigned int i = 1; data->possible_vals[i]; i++)
>> +			sz += sysfs_emit_at(buf, sz, ";%s", data->possible_vals[i]);
>> +		sz += sysfs_emit_at(buf, sz, "\n");
>> +		return sz;
>> +	default:
>> +		return -EOPNOTSUPP;
>> +	}
>> +
>> +	return sysfs_emit(buf, "%s\n", data->possible_vals[val_idx]);
>> +}
>
> <snip>
>
>> +static umode_t fwat_attr_visible(struct kobject *kobj, struct attribute *attr, int n)
>> +{
>> +	struct fwat_attribute *fwat_attr = to_fwat_attribute(attr);
>
> const
>
>> +	struct fwat_group *group = kobj_to_fwat_group(kobj);
>> +	const struct fwat_group_data *data = group->data;
>> +
>> +	/* The `type` attribute is always first */
>> +	if (n == 0)
>> +		return attr->mode;
>> +
>> +	if (attr == &fwat_all_display_name_attr.attr)
>> +		return data->display_name ? attr->mode : 0;
>> +
>> +	if (attr == &fwat_all_display_name_language_code_attr.attr)
>> +		return data->language_code ? attr->mode : 0;
>> +
>> +	/* The `current_value` attribute always has type == 0 */
>> +	if (!fwat_attr->type)
>> +		return data->mode;
>> +
>> +	return test_bit(fwat_attr->type, &data->fattrs) ? attr->mode : 0;
>> +}
>> +
>> +static umode_t fwat_group_visible(struct kobject *kobj)
>> +{
>> +	return true;
>> +}
>> +
>> +DEFINE_SYSFS_GROUP_VISIBLE(fwat);
>
> Why DEFINE_SYSFS_GROUP_VISIBLE() if the per-group callback doesn't do
> anything?
>
>> +
>> +static const struct attribute_group fwat_bool_group = {
>> +	.attrs = fwat_bool_attrs,
>> +	.is_visible = SYSFS_GROUP_VISIBLE(fwat),
>> +};
>> +__ATTRIBUTE_GROUPS(fwat_bool);
>
> <snip>
>
>>  /**
>>   * fwat_device_register - Create and register a firmware-attributes class
>>   *			  device
>> @@ -89,6 +620,7 @@ void fwat_device_unregister(struct fwat_device *fadev)
>>  	if (!fadev)
>>  		return;
>>  
>> +	fwat_remove_auto_groups(fadev);
>
> As before, the groups are children of the device so should be removed
> automatically.

I'm pretty sure that would leak the fwat_group memory allocated in
__fwat_create_group(). Filesystem should be fine, but we still need to
put down kobject references.

>
>>  	sysfs_remove_groups(&fadev->attrs_kset->kobj, fadev->groups);
>>  	kset_unregister(fadev->attrs_kset);
>>  	device_unregister(&fadev->dev);
>> diff --git a/drivers/platform/x86/firmware_attributes_class.h b/drivers/platform/x86/firmware_attributes_class.h
>> index 048fd0904f767357ef856e687ec4cf3260016ec6..e8868ce05b595eda94a98975428391b9f9341e3d 100644
>> --- a/drivers/platform/x86/firmware_attributes_class.h
>> +++ b/drivers/platform/x86/firmware_attributes_class.h
>> @@ -10,6 +10,7 @@
>>  #include <linux/device/class.h>
>>  #include <linux/kobject.h>
>>  #include <linux/sysfs.h>
>> +#include <linux/list.h>
>
> Unnecessary for public header.
>
>>  
>>  extern const struct class firmware_attributes_class;
>>  
>> @@ -27,6 +28,340 @@ struct fwat_device {
>>  
>>  #define to_fwat_device(_d)	container_of_const(_d, struct fwat_device, dev)
>>  
>> +enum fwat_group_type {
>> +	fwat_group_boolean,
>
> I have a slight preference for "fwat_group_type_boolean".

Ack.

>
>> +	fwat_group_enumeration,
>> +	fwat_group_integer,
>> +	fwat_group_string,
>> +};
>> +
>> +enum fwat_bool_attrs {
>> +	fwat_bool_current_value,
>> +	fwat_bool_default_value,
>> +	fwat_bool_attrs_last
>> +};
>> +
>> +#define FWAT_BOOL_CURRENT_VALUE			BIT(fwat_bool_current_value)
>> +#define FWAT_BOOL_DEFAULT_VALUE			BIT(fwat_bool_default_value)
>> +#define FWAT_BOOL_ALL_ATTRS			GENMASK(fwat_bool_attrs_last, 0)
>
> Is the ALL_ATTRS really necessary? It could lead to issues when new
> optional attributes are added to the core.

Most drivers end up using all sub-attrs so I included it to avoid
seeing this every time: 

FWAT_INT_DEFAULT_VALUE | FWAT_INT_MIN_VALUE | FWAT_INT_MIN_VALUE | ...

>
>> +
>> +enum fwat_enum_attrs {
>> +	fwat_enum_current_value,
>> +	fwat_enum_default_value,
>> +	fwat_enum_possible_values,
>> +	fwat_enum_attrs_last
>> +};
>> +
>> +#define FWAT_ENUM_CURRENT_VALUE			BIT(fwat_enum_current_value)
>> +#define FWAT_ENUM_DEFAULT_VALUE			BIT(fwat_enum_default_value)
>> +#define FWAT_ENUM_POSSIBLE_VALUES		BIT(fwat_enum_possible_values)
>> +#define FWAT_ENUM_ALL_ATTRS			GENMASK(fwat_enum_attrs_last, 0)
>> +
>> +enum fwat_int_attrs {
>> +	fwat_int_current_value,
>> +	fwat_int_default_value,
>> +	fwat_int_min_value,
>> +	fwat_int_max_value,
>> +	fwat_int_scalar_increment,
>> +	fwat_int_attrs_last
>> +};
>> +
>> +#define FWAT_INT_CURRENT_VALUE			BIT(fwat_int_current_value)
>> +#define FWAT_INT_DEFAULT_VALUE			BIT(fwat_int_default_value)
>> +#define FWAT_INT_MIN_VALUE			BIT(fwat_int_min_value)
>> +#define FWAT_INT_MAX_VALUE			BIT(fwat_int_max_value)
>> +#define FWAT_INT_SCALAR_INCREMENT		BIT(fwat_int_scalar_increment)
>> +#define FWAT_INT_ALL_ATTRS			GENMASK(fwat_int_attrs_last, 0)
>> +
>> +enum fwat_str_attrs {
>> +	fwat_str_current_value,
>> +	fwat_str_default_value,
>> +	fwat_str_min_length,
>> +	fwat_str_max_length,
>> +	fwat_str_attrs_last
>> +};
>> +
>> +#define FWAT_STR_CURRENT_VALUE			BIT(fwat_str_current_value)
>> +#define FWAT_STR_DEFAULT_VALUE			BIT(fwat_str_default_value)
>> +#define FWAT_STR_MIN_LENGTH			BIT(fwat_str_min_length)
>> +#define FWAT_STR_MAX_LENGTH			BIT(fwat_str_max_length)
>> +#define FWAT_STR_ALL_ATTRS			GENMASK(fwat_str_attrs_last, 0)
>> +
>> +static_assert(fwat_bool_current_value == 0);
>> +static_assert(fwat_enum_current_value == 0);
>> +static_assert(fwat_int_current_value == 0);
>> +static_assert(fwat_str_current_value == 0);
>
> Move this into the the .c file right next to the code relying on this
> invariant.
>
> <snip>
>
>> +/**
>> + * DEFINE_FWAT_ENUM_GROUP - Convenience macro to quickly define an static
>> + *                          struct fwat_enum_data instance
>> + * @_name: Name of the group.
>> + * @_disp_name: Name showed in the display_name attribute. (Optional)
>> + * @_def_idx: Index of the default value in the @_poss_vals array.
>> + * @_poss_vals: Array of possible value strings for this group type.
>> + * @_mode: Mode for the current_value attribute. All other attributes will have
>> + *         0444 permissions.
>> + * @_fattrs: Bitmap of selected attributes for this group type.
>> + *
>> + * `read` and `write` callbacks are required to be already defined as
>> + * `_name##_read` and `_name##_write` respectively.
>> + *
>> + * NOTE: The `val_idx` argument in the `write` callback is guaranteed to be a
>> + *       valid (within bounds) index. However, the user is in charge of writing
>> + *       valid indexes to the `*val_idx` argument of the `read` callback.
>> + *       Failing to do so may result in an OOB access.
>> + */
>> +#define DEFINE_FWAT_ENUM_GROUP(_name, _disp_name, _poss_vals, _def_idx, _mode, _fattrs) \
>> +	static const struct fwat_enum_data _name##_group_data = {	\
>
> Align all backslashes of the definition.
>
>> +		.read = _name##_read,					\
>> +		.write = _name##_write,					\
>> +		.default_idx = _def_idx,				\
>> +		.possible_vals = _poss_vals,				\
>> +		.group = __FWAT_GROUP(_name, _disp_name, _mode, _fattrs), \
>> +	}
>
> <snip>
>
>> +int fwat_create_bool_group(struct fwat_device *fadev,
>> +			   const struct fwat_bool_data *data);
>> +int fwat_create_enum_group(struct fwat_device *fadev,
>> +			   const struct fwat_enum_data *data);
>> +int fwat_create_int_group(struct fwat_device *fadev,
>> +			  const struct fwat_int_data *data);
>> +int fwat_create_str_group(struct fwat_device *fadev,
>> +			  const struct fwat_str_data *data);
>
> If these are not meant to be used by users, give them a leading
> underscore?

Ack.

>
>> +
>> +/**
>> + * fwat_create_group - Convenience generic macro to create a group
>> + * @_dev: fwat_device
>> + * @_data: One of fwat_{bool,enum,int,str}_data instance
>> + *
>> + * This macro (and associated functions) creates a sysfs group under the
>> + * 'attributes' directory, which is located in the class device root directory.
>> + *
>> + * See Documentation/ABI/testing/sysfs-class-firmware-attributes for details.
>> + *
>> + * The @_data associated with this group may be created either statically,
>> + * through DEFINE_FWAT_*_GROUP macros or dynamically, in which case the user
>> + * would have allocate and fill the struct manually. The dynamic approach should
>> + * be preferred when group constraints and/or visibility is decided dynamically.
>
> I'd prefer if users don't need dynamic data allocations if possible.
> Let's see how it works out with the existing drivers.

That would be ideal ofc. But, as I said above, most drivers dynamically
enumerate and allocate "firmware attributes" so I think we need to
accomodate for this.

Most new drivers on the other hand know all these values beforehand so
in the end the user can decide how to define this data structs.

>
>> + *
>> + * Example:
>> + *
>> + * static int stat_read(...){...};
>> + * static int stat_write(...){...};
>> + *
>> + * DEFINE_FWAT_(BOOL|ENUM|INT|STR)_GROUP(stat, ...);
>> + *
>> + * static int create_groups(struct fwat_device *fadev)
>> + * {
>> + *	struct fwat_enum_data *dyn_group_data;
>> + *
>> + *	dyn_group_data = kzalloc(...);
>> + *	// Fill the data
>> + *	...
>> + *	fwat_create_group(fadev, &stat_group_data);
>> + *	fwat_create_group(fadev, &dyn_group_data);
>> + *	fwat_create_group(...);
>> + *	...
>> + * }
>> + *
>> + * Return: 0 on success, -errno on failure
>> + */
>> +#define fwat_create_group(_dev, _data) \
>> +	_Generic((_data),							\
>> +		 const struct fwat_bool_data * : fwat_create_bool_group,	\
>> +		 const struct fwat_enum_data * : fwat_create_enum_group,	\
>> +		 const struct fwat_int_data * : fwat_create_int_group,		\
>> +		 const struct fwat_str_data * : fwat_create_str_group)		\
>> +		(_dev, _data)
>> +
>>  struct fwat_device * __must_check
>>  fwat_device_register(struct device *parent, const char *name, void *drvdata,
>>  		     const struct attribute_group **groups);
>> 
>> -- 
>> 2.50.0
>> 

Ack to the rest of comments. Thank you for your feedback!


-- 
 ~ Kurt

Re: [PATCH v5 2/6] platform/x86: firmware_attributes_class: Add high level API for the attributes interface

[ALOK TIWARI]

On 7/5/2025 9:03 AM, Kurt Borja wrote:
> Add high level API to aid in the creation of attribute groups attached
> to the `attrs_kobj` (per ABI specification).
> 
> This new API lets users configure each group, either statically or
> dynamically through a (per type) data struct and then create this group
> through the generic fwat_create_group() macro.
> 
> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
> ---
>   drivers/platform/x86/firmware_attributes_class.c | 532 +++++++++++++++++++++++
>   drivers/platform/x86/firmware_attributes_class.h | 335 ++++++++++++++
>   2 files changed, 867 insertions(+)
> 
> diff --git a/drivers/platform/x86/firmware_attributes_class.c b/drivers/platform/x86/firmware_attributes_class.c
> index 290364202cce64bb0e9046e0b2bbb8d85e2cbc6f..96473b3b1a2a87cf21a6e2a9a14d72ae322c94ae 100644
> --- a/drivers/platform/x86/firmware_attributes_class.c
> +++ b/drivers/platform/x86/firmware_attributes_class.c
> @@ -10,13 +10,67 @@
>   #include <linux/module.h>
>   #include <linux/slab.h>
>   #include <linux/types.h>
> +#include <linux/string_choices.h>
>   #include "firmware_attributes_class.h"
>   
> +#define FWAT_TYPE_NONE				-1
> +
> +#define to_fwat_bool_data(_c) \
> +	container_of_const(_c, struct fwat_bool_data, group)
> +#define to_fwat_enum_data(_c) \
> +	container_of_const(_c, struct fwat_enum_data, group)
> +#define to_fwat_int_data(_c) \
> +	container_of_const(_c, struct fwat_int_data, group)
> +#define to_fwat_str_data(_c) \
> +	container_of_const(_c, struct fwat_str_data, group)
> +
> +struct fwat_attribute {
> +	struct attribute attr;
> +	ssize_t (*show)(struct kobject *kobj, struct fwat_attribute *attr,
> +			char *buf);
> +	ssize_t (*store)(struct kobject *kobj, struct fwat_attribute *attr,
> +			 const char *buf, size_t count);
> +	int type;
> +};
> +
> +#define to_fwat_attribute(_a) \
> +	container_of_const(_a, struct fwat_attribute, attr)
> +
> +#define __FWAT_ATTR(_name, _mode, _show, _store, _type) \
> +	{								\
> +		.attr = { .name = __stringify(_name), .mode = _mode },	\
> +		.show = _show, .store = _store, .type = _type,		\
> +	}
> +
> +#define FWAT_ATTR_RO(_prefix, _name, _show, _type) \
> +	static struct fwat_attribute fwat_##_prefix##_##_name##_attr = \
> +		__FWAT_ATTR(_name, 0444, _show, NULL, _type)
> +
> +#define FWAT_ATTR_RW(_prefix, _name, _show, _store, _type) \
> +	static struct fwat_attribute fwat_##_prefix##_##_name##_attr = \
> +		__FWAT_ATTR(_name, 0644, _show, _store, _type)
> +
> +struct fwat_group {
> +	const struct fwat_group_data *data;
> +	struct device *dev;
> +	struct kobject kobj;
> +};
> +
> +#define kobj_to_fwat_group(_k) \
> +	container_of_const(_k, struct fwat_group, kobj)
> +
>   const struct class firmware_attributes_class = {
>   	.name = "firmware-attributes",
>   };
>   EXPORT_SYMBOL_GPL(firmware_attributes_class);
>   
> +static const char * const fwat_type_labels[] = {
> +	[fwat_group_boolean]			= "boolean",
> +	[fwat_group_enumeration]		= "enumeration",
> +	[fwat_group_integer]			= "integer",
> +	[fwat_group_string]			= "string",
> +};
> +
>   static void fwat_device_release(struct device *dev)
>   {
>   	struct fwat_device *fadev = to_fwat_device(dev);
> @@ -24,6 +78,483 @@ static void fwat_device_release(struct device *dev)
>   	kfree(fadev);
>   }
>   
> +static ssize_t
> +type_show(struct kobject *kobj, struct fwat_attribute *attr, char *buf)
> +{
> +	return sysfs_emit(buf, "%s\n", fwat_type_labels[attr->type]);
> +}
> +
> +static ssize_t
> +display_name_show(struct kobject *kobj, struct fwat_attribute *attr, char *buf)
> +{
> +	struct fwat_group *group = kobj_to_fwat_group(kobj);
> +	const char *disp_name = group->data->display_name;
> +
> +	if (!disp_name)
> +		return -EOPNOTSUPP;
> +
> +	return sysfs_emit(buf, "%s\n", disp_name);
> +}
> +
> +static ssize_t
> +display_name_language_code_show(struct kobject *kobj, struct fwat_attribute *attr,
> +				char *buf)
> +{
> +	struct fwat_group *group = kobj_to_fwat_group(kobj);
> +	const char *lang_code = group->data->language_code;
> +
> +	if (!lang_code)
> +		return -EOPNOTSUPP;
> +
> +	return sysfs_emit(buf, "%s\n", lang_code);
> +}
> +
> +static ssize_t
> +bool_group_show(struct kobject *kobj, struct fwat_attribute *attr, char *buf)
> +{
> +	const struct fwat_group *group = kobj_to_fwat_group(kobj);
> +	const struct fwat_bool_data *data = to_fwat_bool_data(group->data);
> +	bool val;
> +	int ret;
> +
> +	/* show_override does not affect current_value */
> +	if (data->group.show_override && attr->type != fwat_bool_current_value)
> +		return data->group.show_override(group->dev, attr->type, buf);
> +
> +	switch (attr->type) {
> +	case fwat_bool_current_value:
> +		ret = data->read(group->dev, data->group.id, &val);
> +		if (ret < 0)
> +			return ret;
> +		break;
> +	case fwat_bool_default_value:
> +		val = data->default_val;
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return sysfs_emit(buf, "%s\n", str_yes_no(val));
> +}
> +
> +static ssize_t
> +bool_group_store(struct kobject *kobj, struct fwat_attribute *attr, const char *buf,
> +		 size_t count)
> +{
> +	const struct fwat_group *group = kobj_to_fwat_group(kobj);
> +	const struct fwat_bool_data *data = to_fwat_bool_data(group->data);
> +	bool val;
> +	int ret;
> +
> +	ret = kstrtobool(buf, &val);
> +	if (ret)
> +		return ret;
> +
> +	ret = data->write(group->dev, data->group.id, val);
> +	if (ret)
> +		return ret;
> +
> +	return count;
> +}
> +
> +static ssize_t
> +enum_group_show(struct kobject *kobj, struct fwat_attribute *attr, char *buf)
> +{
> +	const struct fwat_group *group = kobj_to_fwat_group(kobj);
> +	const struct fwat_enum_data *data = to_fwat_enum_data(group->data);
> +	int val_idx, sz = 0;
> +	int ret;
> +
> +	/* show_override does not affect current_value */
> +	if (data->group.show_override && attr->type != fwat_enum_current_value)
> +		return data->group.show_override(group->dev, attr->type, buf);
> +
> +	switch (attr->type) {
> +	case fwat_enum_current_value:
> +		ret = data->read(group->dev, data->group.id, &val_idx);
> +		if (ret < 0)
> +			return ret;
> +		break;
> +	case fwat_enum_default_value:
> +		val_idx = data->default_idx;
> +		break;
> +	case fwat_enum_possible_values:
> +		sz += sysfs_emit_at(buf, sz, "%s", data->possible_vals[0]);
> +		for (unsigned int i = 1; data->possible_vals[i]; i++)
> +			sz += sysfs_emit_at(buf, sz, ";%s", data->possible_vals[i]);
> +		sz += sysfs_emit_at(buf, sz, "\n");
> +		return sz;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return sysfs_emit(buf, "%s\n", data->possible_vals[val_idx]);
> +}
> +
> +static ssize_t
> +enum_group_store(struct kobject *kobj, struct fwat_attribute *attr, const char *buf,
> +		 size_t count)
> +{
> +	const struct fwat_group *group = kobj_to_fwat_group(kobj);
> +	const struct fwat_enum_data *data = to_fwat_enum_data(group->data);
> +	int val_idx;
> +	int ret;
> +
> +	val_idx = __sysfs_match_string(data->possible_vals, -1, buf);
> +	if (val_idx < 0)
> +		return val_idx;
> +
> +	ret = data->write(group->dev, data->group.id, val_idx);
> +	if (ret)
> +		return ret;
> +
> +	return count;
> +}
> +
> +static ssize_t
> +int_group_show(struct kobject *kobj, struct fwat_attribute *attr, char *buf)
> +{
> +	const struct fwat_group *group = kobj_to_fwat_group(kobj);
> +	const struct fwat_int_data *data = to_fwat_int_data(group->data);
> +	long val;
> +	int ret;
> +
> +	/* show_override does not affect current_value */
> +	if (data->group.show_override && attr->type != fwat_int_current_value)
> +		return data->group.show_override(group->dev, attr->type, buf);
> +
> +	switch (attr->type) {
> +	case fwat_int_current_value:
> +		ret = data->read(group->dev, data->group.id, &val);
> +		if (ret < 0)
> +			return ret;
> +		break;
> +	case fwat_int_default_value:
> +		val = data->default_val;
> +		break;
> +	case fwat_int_min_value:
> +		val = data->min_val;
> +		break;
> +	case fwat_int_max_value:
> +		val = data->max_val;
> +		break;
> +	case fwat_int_scalar_increment:
> +		val = data->increment;
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return sysfs_emit(buf, "%ld\n", val);
> +}
> +
> +static ssize_t
> +int_group_store(struct kobject *kobj, struct fwat_attribute *attr, const char *buf,
> +		size_t count)
> +{
> +	const struct fwat_group *group = kobj_to_fwat_group(kobj);
> +	const struct fwat_int_data *data = to_fwat_int_data(group->data);
> +	long val;
> +	int ret;
> +
> +	ret = kstrtol(buf, 0, &val);
> +	if (ret)
> +		return ret;
> +
> +	ret = data->write(group->dev, data->group.id, val);
> +	if (ret)
> +		return ret;
> +
> +	return count;
> +}
> +
> +static ssize_t
> +str_group_show(struct kobject *kobj, struct fwat_attribute *attr, char *buf)
> +{
> +	const struct fwat_group *group = kobj_to_fwat_group(kobj);
> +	const struct fwat_str_data *data = to_fwat_str_data(group->data);
> +	const char *val;
> +	long len;
> +	int ret;
> +
> +	/* show_override does not affect current_value */
> +	if (data->group.show_override && attr->type != fwat_bool_current_value)

fwat_bool_current_value ? or str

> +		return data->group.show_override(group->dev, attr->type, buf);
> +
> +	switch (attr->type) {
> +	case fwat_str_current_value:
> +		ret = data->read(group->dev, data->group.id, &val);
> +		if (ret < 0)
> +			return ret;
> +		break;
> +	case fwat_str_default_value:
> +		val = data->default_val;
> +		break;
> +	case fwat_str_min_length:
> +		len = data->min_len;
> +		return sysfs_emit(buf, "%ld\n", len);
> +	case fwat_str_max_length:
> +		len = data->max_len;
> +		return sysfs_emit(buf, "%ld\n", len);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return sysfs_emit(buf, "%s\n", val);
> +}
> +
> +static ssize_t
> +str_group_store(struct kobject *kobj, struct fwat_attribute *attr, const char *buf,
> +		size_t count)
> +{
> +	const struct fwat_group *group = kobj_to_fwat_group(kobj);
> +	const struct fwat_str_data *data = to_fwat_str_data(group->data);
> +	int ret;
> +
> +	ret = data->write(group->dev, data->group.id, buf);
> +	if (ret)
> +		return ret;
> +
> +	return count;
> +}
> +
> +FWAT_ATTR_RO(all, display_name, display_name_show, FWAT_TYPE_NONE);
> +FWAT_ATTR_RO(all, display_name_language_code, display_name_language_code_show, FWAT_TYPE_NONE);
> +
> +FWAT_ATTR_RO(bool, type, type_show, fwat_group_boolean);
> +FWAT_ATTR_RW(bool, current_value, bool_group_show, bool_group_store, fwat_bool_current_value);
> +FWAT_ATTR_RO(bool, default_value, bool_group_show, fwat_bool_default_value);
> +
> +FWAT_ATTR_RO(enum, type, type_show, fwat_group_enumeration);
> +FWAT_ATTR_RW(enum, current_value, enum_group_show, enum_group_store, fwat_enum_current_value);
> +FWAT_ATTR_RO(enum, default_value, enum_group_show, fwat_enum_default_value);
> +FWAT_ATTR_RO(enum, possible_values, enum_group_show, fwat_enum_possible_values);
> +
> +FWAT_ATTR_RO(int, type, type_show, fwat_group_integer);
> +FWAT_ATTR_RW(int, current_value, int_group_show, int_group_store, fwat_int_current_value);
> +FWAT_ATTR_RO(int, default_value, int_group_show, fwat_int_default_value);
> +FWAT_ATTR_RO(int, min_value, int_group_show, fwat_int_min_value);
> +FWAT_ATTR_RO(int, max_value, int_group_show, fwat_int_max_value);
> +FWAT_ATTR_RO(int, scalar_increment, int_group_show, fwat_int_scalar_increment);
> +
> +FWAT_ATTR_RO(str, type, type_show, fwat_group_string);
> +FWAT_ATTR_RW(str, current_value, str_group_show, str_group_store, fwat_int_current_value);

wrong enum fwat_int_current_value -> fwat_str_current_value

> +FWAT_ATTR_RO(str, default_value, str_group_show, fwat_str_default_value);
> +FWAT_ATTR_RO(str, min_length, str_group_show, fwat_str_min_length);
> +FWAT_ATTR_RO(str, max_length, str_group_show, fwat_str_max_length);
> +
> +static struct attribute *fwat_bool_attrs[] = {
> +	&fwat_bool_type_attr.attr,
> +	&fwat_all_display_name_attr.attr,
> +	&fwat_all_display_name_language_code_attr.attr,
> +	&fwat_bool_current_value_attr.attr,
> +	&fwat_bool_default_value_attr.attr,
> +	NULL
> +};
> +
> +static struct attribute *fwat_enum_attrs[] = {
> +	&fwat_enum_type_attr.attr,
> +	&fwat_all_display_name_attr.attr,
> +	&fwat_all_display_name_language_code_attr.attr,
> +	&fwat_enum_current_value_attr.attr,
> +	&fwat_enum_default_value_attr.attr,
> +	&fwat_enum_possible_values_attr.attr,
> +	NULL
> +};
> +
> +static struct attribute *fwat_int_attrs[] = {
> +	&fwat_int_type_attr.attr,
> +	&fwat_all_display_name_attr.attr,
> +	&fwat_all_display_name_language_code_attr.attr,
> +	&fwat_int_current_value_attr.attr,
> +	&fwat_int_default_value_attr.attr,
> +	&fwat_int_min_value_attr.attr,
> +	&fwat_int_max_value_attr.attr,
> +	&fwat_int_scalar_increment_attr.attr,
> +	NULL
> +};
> +
> +static struct attribute *fwat_str_attrs[] = {
> +	&fwat_str_type_attr.attr,
> +	&fwat_all_display_name_attr.attr,
> +	&fwat_all_display_name_language_code_attr.attr,
> +	&fwat_str_current_value_attr.attr,
> +	&fwat_str_default_value_attr.attr,
> +	&fwat_str_min_length_attr.attr,
> +	&fwat_str_max_length_attr.attr,
> +	NULL
> +};
> +
> +static umode_t fwat_attr_visible(struct kobject *kobj, struct attribute *attr, int n)
> +{
> +	struct fwat_attribute *fwat_attr = to_fwat_attribute(attr);
> +	struct fwat_group *group = kobj_to_fwat_group(kobj);
> +	const struct fwat_group_data *data = group->data;
> +
> +	/* The `type` attribute is always first */
> +	if (n == 0)
> +		return attr->mode;
> +
> +	if (attr == &fwat_all_display_name_attr.attr)
> +		return data->display_name ? attr->mode : 0;
> +
> +	if (attr == &fwat_all_display_name_language_code_attr.attr)
> +		return data->language_code ? attr->mode : 0;
> +
> +	/* The `current_value` attribute always has type == 0 */
> +	if (!fwat_attr->type)
> +		return data->mode;
> +
> +	return test_bit(fwat_attr->type, &data->fattrs) ? attr->mode : 0;
> +}
> +
> +static umode_t fwat_group_visible(struct kobject *kobj)
> +{
> +	return true;
> +}
> +
> +DEFINE_SYSFS_GROUP_VISIBLE(fwat);
> +
> +static const struct attribute_group fwat_bool_group = {
> +	.attrs = fwat_bool_attrs,
> +	.is_visible = SYSFS_GROUP_VISIBLE(fwat),
> +};
> +__ATTRIBUTE_GROUPS(fwat_bool);
> +
> +static const struct attribute_group fwat_enum_group = {
> +	.attrs = fwat_enum_attrs,
> +	.is_visible = SYSFS_GROUP_VISIBLE(fwat),
> +};
> +__ATTRIBUTE_GROUPS(fwat_enum);
> +
> +static const struct attribute_group fwat_int_group = {
> +	.attrs = fwat_int_attrs,
> +	.is_visible = SYSFS_GROUP_VISIBLE(fwat),
> +};
> +__ATTRIBUTE_GROUPS(fwat_int);
> +
> +static const struct attribute_group fwat_str_group = {
> +	.attrs = fwat_str_attrs,
> +	.is_visible = SYSFS_GROUP_VISIBLE(fwat),
> +};
> +__ATTRIBUTE_GROUPS(fwat_str);
> +
> +static ssize_t
> +fwat_attr_sysfs_show(struct kobject *kobj, struct attribute *attr, char *buf)
> +{
> +	struct fwat_attribute *fwat_attr = to_fwat_attribute(attr);
> +
> +	if (!fwat_attr->show)
> +		return -EOPNOTSUPP;
> +
> +	return fwat_attr->show(kobj, fwat_attr, buf);
> +}
> +
> +static ssize_t
> +fwat_attr_sysfs_store(struct kobject *kobj, struct attribute *attr, const char *buf,
> +		      size_t count)
> +{
> +	struct fwat_attribute *fwat_attr = to_fwat_attribute(attr);
> +
> +	if (!fwat_attr->show)
> +		return -EOPNOTSUPP;

check for fwat_attr->store ?

> +
> +	return fwat_attr->store(kobj, fwat_attr, buf, count);
> +}
> +
> +static void fwat_group_release(struct kobject *kobj)
> +{
> +	struct fwat_group *group = kobj_to_fwat_group(kobj);
> +
> +	kfree(group);
> +}
> +
> +static const struct sysfs_ops fwat_attr_sysfs_ops = {
> +	.show = fwat_attr_sysfs_show,
> +	.store = fwat_attr_sysfs_store,
> +};
> +
> +static const struct kobj_type fwat_boolean_ktype = {
> +	.sysfs_ops = &fwat_attr_sysfs_ops,
> +	.release = fwat_group_release,
> +	.default_groups = fwat_bool_groups,
> +};
> +
> +static const struct kobj_type fwat_enumeration_ktype = {
> +	.sysfs_ops = &fwat_attr_sysfs_ops,
> +	.release = fwat_group_release,
> +	.default_groups = fwat_enum_groups,
> +};
> +
> +static const struct kobj_type fwat_integer_ktype = {
> +	.sysfs_ops = &fwat_attr_sysfs_ops,
> +	.release = fwat_group_release,
> +	.default_groups = fwat_int_groups,
> +};
> +
> +static const struct kobj_type fwat_string_ktype = {
> +	.sysfs_ops = &fwat_attr_sysfs_ops,
> +	.release = fwat_group_release,
> +	.default_groups = fwat_str_groups,
> +};
> +
> +static int __fwat_create_group(struct fwat_device *fadev, const struct kobj_type *ktype,
> +			       const struct fwat_group_data *data)
> +{
> +	struct fwat_group *group;
> +	int ret;
> +
> +	group = kzalloc(sizeof(*group), GFP_KERNEL);
> +	if (!group)
> +		return -ENOMEM;
> +
> +	group->dev = &fadev->dev;
> +	group->data = data;
> +
> +	group->kobj.kset = fadev->attrs_kset;
> +	ret = kobject_init_and_add(&group->kobj, ktype, NULL, "%s", data->name);
> +	if (ret) {
> +		kobject_put(&group->kobj);
> +		return ret;
> +	}
> +
> +	kobject_uevent(&group->kobj, KOBJ_ADD);
> +
> +	return 0;
> +}
> +
> +static void fwat_remove_auto_groups(struct fwat_device *fadev)
> +{
> +	struct kobject *pos, *n;
> +
> +	list_for_each_entry_safe(pos, n, &fadev->attrs_kset->list, entry)
> +		kobject_put(pos);
> +}
> +
> +int fwat_create_bool_group(struct fwat_device *fadev, const struct fwat_bool_data *data)
> +{
> +	return __fwat_create_group(fadev, &fwat_boolean_ktype, &data->group);
> +}
> +EXPORT_SYMBOL_GPL(fwat_create_bool_group);
> +
> +int fwat_create_enum_group(struct fwat_device *fadev, const struct fwat_enum_data *data)
> +{
> +	return __fwat_create_group(fadev, &fwat_enumeration_ktype, &data->group);
> +}
> +EXPORT_SYMBOL_GPL(fwat_create_enum_group);
> +
> +int fwat_create_int_group(struct fwat_device *fadev, const struct fwat_int_data *data)
> +{
> +	return __fwat_create_group(fadev, &fwat_integer_ktype, &data->group);
> +}
> +EXPORT_SYMBOL_GPL(fwat_create_int_group);
> +
> +int fwat_create_str_group(struct fwat_device *fadev, const struct fwat_str_data *data)
> +{
> +	return __fwat_create_group(fadev, &fwat_string_ktype, &data->group);
> +}
> +EXPORT_SYMBOL_GPL(fwat_create_str_group);
> +
>   /**
>    * fwat_device_register - Create and register a firmware-attributes class
>    *			  device
> @@ -89,6 +620,7 @@ void fwat_device_unregister(struct fwat_device *fadev)
>   	if (!fadev)
>   		return;
>   
> +	fwat_remove_auto_groups(fadev);
>   	sysfs_remove_groups(&fadev->attrs_kset->kobj, fadev->groups);
>   	kset_unregister(fadev->attrs_kset);
>   	device_unregister(&fadev->dev);
> diff --git a/drivers/platform/x86/firmware_attributes_class.h b/drivers/platform/x86/firmware_attributes_class.h
> index 048fd0904f767357ef856e687ec4cf3260016ec6..e8868ce05b595eda94a98975428391b9f9341e3d 100644
> --- a/drivers/platform/x86/firmware_attributes_class.h
> +++ b/drivers/platform/x86/firmware_attributes_class.h
> @@ -10,6 +10,7 @@
>   #include <linux/device/class.h>
>   #include <linux/kobject.h>
>   #include <linux/sysfs.h>
> +#include <linux/list.h>
>   
>   extern const struct class firmware_attributes_class;
>   
> @@ -27,6 +28,340 @@ struct fwat_device {
>   
>   #define to_fwat_device(_d)	container_of_const(_d, struct fwat_device, dev)
>   
> +enum fwat_group_type {
> +	fwat_group_boolean,
> +	fwat_group_enumeration,
> +	fwat_group_integer,
> +	fwat_group_string,
> +};
> +
> +enum fwat_bool_attrs {
> +	fwat_bool_current_value,
> +	fwat_bool_default_value,
> +	fwat_bool_attrs_last
> +};
> +
> +#define FWAT_BOOL_CURRENT_VALUE			BIT(fwat_bool_current_value)
> +#define FWAT_BOOL_DEFAULT_VALUE			BIT(fwat_bool_default_value)
> +#define FWAT_BOOL_ALL_ATTRS			GENMASK(fwat_bool_attrs_last, 0)
> +
> +enum fwat_enum_attrs {
> +	fwat_enum_current_value,
> +	fwat_enum_default_value,
> +	fwat_enum_possible_values,
> +	fwat_enum_attrs_last
> +};
> +
> +#define FWAT_ENUM_CURRENT_VALUE			BIT(fwat_enum_current_value)
> +#define FWAT_ENUM_DEFAULT_VALUE			BIT(fwat_enum_default_value)
> +#define FWAT_ENUM_POSSIBLE_VALUES		BIT(fwat_enum_possible_values)
> +#define FWAT_ENUM_ALL_ATTRS			GENMASK(fwat_enum_attrs_last, 0)
> +
> +enum fwat_int_attrs {
> +	fwat_int_current_value,
> +	fwat_int_default_value,
> +	fwat_int_min_value,
> +	fwat_int_max_value,
> +	fwat_int_scalar_increment,
> +	fwat_int_attrs_last
> +};
> +
> +#define FWAT_INT_CURRENT_VALUE			BIT(fwat_int_current_value)
> +#define FWAT_INT_DEFAULT_VALUE			BIT(fwat_int_default_value)
> +#define FWAT_INT_MIN_VALUE			BIT(fwat_int_min_value)
> +#define FWAT_INT_MAX_VALUE			BIT(fwat_int_max_value)
> +#define FWAT_INT_SCALAR_INCREMENT		BIT(fwat_int_scalar_increment)
> +#define FWAT_INT_ALL_ATTRS			GENMASK(fwat_int_attrs_last, 0)
> +
> +enum fwat_str_attrs {
> +	fwat_str_current_value,
> +	fwat_str_default_value,
> +	fwat_str_min_length,
> +	fwat_str_max_length,
> +	fwat_str_attrs_last
> +};
> +
> +#define FWAT_STR_CURRENT_VALUE			BIT(fwat_str_current_value)
> +#define FWAT_STR_DEFAULT_VALUE			BIT(fwat_str_default_value)
> +#define FWAT_STR_MIN_LENGTH			BIT(fwat_str_min_length)
> +#define FWAT_STR_MAX_LENGTH			BIT(fwat_str_max_length)
> +#define FWAT_STR_ALL_ATTRS			GENMASK(fwat_str_attrs_last, 0)
> +
> +static_assert(fwat_bool_current_value == 0);
> +static_assert(fwat_enum_current_value == 0);
> +static_assert(fwat_int_current_value == 0);
> +static_assert(fwat_str_current_value == 0);
> +
> +/**
> + * struct fwat_group_data - Data struct common between group types
> + * @id: Group ID defined by the user.
> + * @name: Name of the group.
> + * @display_name: Name showed in the display_name attribute. (Optional)
> + * @language_code: Language code showed in the display_name_language_code
> + *                 attribute. (Optional)
> + * @mode: Mode for the current_value attribute. All other attributes will have
> + *        0444 permissions.
> + * @fattrs: Bitmap of selected attributes for this group type.
> + * @show_override: Custom show method for attributes in this group, except for
> + *		   the current_value attribute, for which the a `read` callback
> + *		   will still be used. (Optional)
> + *
> + * NOTE: This struct is not meant to be defined directly. It is supposed to be
> + * embedded and defined as part of fwat_[type]_data structs.
> + */
> +struct fwat_group_data {
> +	long id;
> +	umode_t mode;
> +	const char *name;
> +	const char *display_name;
> +	const char *language_code;
> +	unsigned long fattrs;
> +	ssize_t (*show_override)(struct device *dev, int type, char *buf);
> +};
> +
> +/**
> + * struct fwat_bool_data - Data struct for the boolean group type
> + * @read: Read callback for the current_value attribute.
> + * @write: Write callback for the current_value attribute.
> + * @default_val: Default value.
> + * @group: Group data.
> + */
> +struct fwat_bool_data {
> +	int (*read)(struct device *dev, long id, bool *val);
> +	int (*write)(struct device *dev, long id, bool val);
> +	bool default_val;
> +	struct fwat_group_data group;
> +};
> +
> +/**
> + * struct fwat_enum_data - Data struct for the enumeration group type
> + * @read: Read callback for the current_value attribute.
> + * @write: Write callback for the current_value attribute.
> + * @default_idx: Index of the default value in the @possible_vals array.
> + * @possible_vals: Array of possible value strings for this group type.
> + * @group: Group data.
> + *
> + * NOTE: The `val_idx` argument in the @write callback is guaranteed to be a
> + *       valid (within bounds) index. However, the user is in charge of writing
> + *       valid indexes to the `*val_idx` argument of the @read callback.
> + *       Failing to do so may result in an OOB access.
> + */
> +struct fwat_enum_data {
> +	int (*read)(struct device *dev, long id, int *val_idx);
> +	int (*write)(struct device *dev, long id, int val_idx);
> +	int default_idx;
> +	const char * const *possible_vals;
> +	struct fwat_group_data group;
> +};
> +
> +/**
> + * struct fwat_int_data - Data struct for the integer group type
> + * @read: Read callback for the current_value attribute.
> + * @write: Write callback for the current_value attribute.
> + * @default_val: Default value.
> + * @min_val: Minimum value.
> + * @max_val: Maximum value.
> + * @increment: Scalar increment for this value.
> + * @group: Group data.
> + *
> + * NOTE: The @min_val, @max_val, @increment constraints are merely informative.
> + *       These values are not enforced in any of the callbacks.
> + */
> +struct fwat_int_data {
> +	int (*read)(struct device *dev, long id, long *val);
> +	int (*write)(struct device *dev, long id, long val);
> +	long default_val;
> +	long min_val;
> +	long max_val;
> +	long increment;
> +	struct fwat_group_data group;
> +};
> +
> +/**
> + * struct fwat_str_data - Data struct for the string group type
> + * @read: Read callback for the current_value attribute.
> + * @write: Write callback for the current_value attribute.
> + * @default_val: Default value.
> + * @min_len: Minimum string length.
> + * @max_len: Maximum string length.
> + * @group: Group data.
> + *
> + * NOTE: The @min_len, @max_len constraints are merely informative. These
> + *       values are not enforced in any of the callbacks.
> + */
> +struct fwat_str_data {
> +	int (*read)(struct device *dev, long id, const char **buf);
> +	int (*write)(struct device *dev, long id, const char *buf);
> +	const char *default_val;
> +	long min_len;
> +	long max_len;
> +	struct fwat_group_data group;
> +};
> +
> +#define __FWAT_GROUP(_name, _disp_name, _mode, _fattrs) \
> +	{ .name = __stringify(_name), .display_name = _disp_name, .mode = _mode, .fattrs = _fattrs }
> +
> +/**
> + * DEFINE_FWAT_BOOL_GROUP - Convenience macro to quickly define an static

an static -> a static (all place)

> + *                          struct fwat_bool_data instance
> + * @_name: Name of the group.
> + * @_disp_name: Name showed in the display_name attribute. (Optional)

showed -> shown (all place)

> + * @_def_val: Default value.
> + * @_mode: Mode for the current_value attribute. All other attributes will have
> + *         0444 permissions.
> + * @_fattrs: Bitmap of selected attributes for this group type.
> + *
> + * `read` and `write` callbacks are required to be already defined as
> + * `_name##_read` and `_name##_write` respectively.
> + */
> +#define DEFINE_FWAT_BOOL_GROUP(_name, _disp_name, _def_val, _mode, _fattrs) \
> +	static const struct fwat_bool_data _name##_group_data = {	\
> +		.read = _name##_read,					\
> +		.write = _name##_write,					\
> +		.default_val = _def_val,				\
> +		.group = __FWAT_GROUP(_name, _disp_name, _mode, _fattrs), \
> +	}
> +
> +/**
> + * DEFINE_FWAT_ENUM_GROUP - Convenience macro to quickly define an static
> + *                          struct fwat_enum_data instance
> + * @_name: Name of the group.
> + * @_disp_name: Name showed in the display_name attribute. (Optional)
> + * @_def_idx: Index of the default value in the @_poss_vals array.
> + * @_poss_vals: Array of possible value strings for this group type.
> + * @_mode: Mode for the current_value attribute. All other attributes will have
> + *         0444 permissions.
> + * @_fattrs: Bitmap of selected attributes for this group type.
> + *
> + * `read` and `write` callbacks are required to be already defined as
> + * `_name##_read` and `_name##_write` respectively.
> + *
> + * NOTE: The `val_idx` argument in the `write` callback is guaranteed to be a
> + *       valid (within bounds) index. However, the user is in charge of writing
> + *       valid indexes to the `*val_idx` argument of the `read` callback.
> + *       Failing to do so may result in an OOB access.
> + */
> +#define DEFINE_FWAT_ENUM_GROUP(_name, _disp_name, _poss_vals, _def_idx, _mode, _fattrs) \
> +	static const struct fwat_enum_data _name##_group_data = {	\
> +		.read = _name##_read,					\
> +		.write = _name##_write,					\
> +		.default_idx = _def_idx,				\
> +		.possible_vals = _poss_vals,				\
> +		.group = __FWAT_GROUP(_name, _disp_name, _mode, _fattrs), \
> +	}
> +
> +/**
> + * DEFINE_FWAT_INT_GROUP - Convenience macro to quickly define an static
> + *                         struct fwat_int_data instance
> + * @_name: Name of the group.
> + * @_disp_name: Name showed in the display_name attribute. (Optional)
> + * @_def_val: Default value.
> + * @_min: Minimum value.
> + * @_max: Maximum value.
> + * @_inc: Scalar increment for this value.
> + * @_mode: Mode for the current_value attribute. All other attributes will have
> + *         0444 permissions.
> + * @_fattrs: Bitmap of selected attributes for this group type.
> + *
> + * `read` and `write` callbacks are required to be already defined as
> + * `_name##_read` and `_name##_write` respectively.
> + *
> + * NOTE: The @_min, @_max, @_inc constraints are merely informative. These
> + *       values are not enforced in any of the callbacks.
> + */
> +#define DEFINE_FWAT_INT_GROUP(_name, _disp_name, _def_val, _min, _max, _inc, _mode, _fattrs) \
> +	static const struct fwat_int_data _name##_group_data = {	\
> +		.read = _name##_read,					\
> +		.write = _name##_write,					\
> +		.default_val = _def_val,				\
> +		.min_val = _min,					\
> +		.max_val = _max,					\
> +		.increment = _inc,					\
> +		.group = __FWAT_GROUP(_name, _disp_name, _mode, _fattrs), \
> +	}
> +
> +/**
> + * DEFINE_FWAT_STR_GROUP - Convenience macro to quickly define an static
> + *                         struct fwat_str_data instance
> + * @_name: Name of the group.
> + * @_disp_name: Name showed in the display_name attribute. (Optional)
> + * @_def_val: Default value.
> + * @_min: Minimum string length.
> + * @_max: Maximum string length.
> + * @_mode: Mode for the current_value attribute. All other attributes will have
> + *         0444 permissions.
> + * @_fattrs: Bitmap of selected attributes for this group type.
> + *
> + * `read` and `write` callbacks are required to be already defined as
> + * `_name##_read` and `_name##_write` respectively.
> + *
> + * NOTE: The @_min, @_max constraints are merely informative. These values are
> + *       not enforced in any of the callbacks.
> + */
> +#define DEFINE_FWAT_STR_GROUP(_name, _disp_name, _def_val, _min, _max, _mode, _fattrs) \
> +	static const struct fwat_str_data _name##_group_data = {	\
> +		.read = _name##_read,					\
> +		.write = _name##_write,					\
> +		.default_val = _def_val,				\
> +		.min_len = _min,					\
> +		.max_len = _max,					\
> +		.group = __FWAT_GROUP(_name, _disp_name, _mode, _fattrs), \
> +	}
> +
> +int fwat_create_bool_group(struct fwat_device *fadev,
> +			   const struct fwat_bool_data *data);
> +int fwat_create_enum_group(struct fwat_device *fadev,
> +			   const struct fwat_enum_data *data);
> +int fwat_create_int_group(struct fwat_device *fadev,
> +			  const struct fwat_int_data *data);
> +int fwat_create_str_group(struct fwat_device *fadev,
> +			  const struct fwat_str_data *data);
> +
> +/**
> + * fwat_create_group - Convenience generic macro to create a group
> + * @_dev: fwat_device
> + * @_data: One of fwat_{bool,enum,int,str}_data instance
> + *
> + * This macro (and associated functions) creates a sysfs group under the
> + * 'attributes' directory, which is located in the class device root directory.
> + *
> + * See Documentation/ABI/testing/sysfs-class-firmware-attributes for details.
> + *
> + * The @_data associated with this group may be created either statically,
> + * through DEFINE_FWAT_*_GROUP macros or dynamically, in which case the user
> + * would have allocate and fill the struct manually. The dynamic approach should
> + * be preferred when group constraints and/or visibility is decided dynamically.
> + *
> + * Example:
> + *
> + * static int stat_read(...){...};
> + * static int stat_write(...){...};
> + *
> + * DEFINE_FWAT_(BOOL|ENUM|INT|STR)_GROUP(stat, ...);
> + *
> + * static int create_groups(struct fwat_device *fadev)
> + * {
> + *	struct fwat_enum_data *dyn_group_data;
> + *
> + *	dyn_group_data = kzalloc(...);
> + *	// Fill the data
> + *	...
> + *	fwat_create_group(fadev, &stat_group_data);
> + *	fwat_create_group(fadev, &dyn_group_data);
> + *	fwat_create_group(...);
> + *	...
> + * }
> + *
> + * Return: 0 on success, -errno on failure
> + */
> +#define fwat_create_group(_dev, _data) \
> +	_Generic((_data),							\
> +		 const struct fwat_bool_data * : fwat_create_bool_group,	\
> +		 const struct fwat_enum_data * : fwat_create_enum_group,	\
> +		 const struct fwat_int_data * : fwat_create_int_group,		\
> +		 const struct fwat_str_data * : fwat_create_str_group)		\
> +		(_dev, _data)
> +
>   struct fwat_device * __must_check
>   fwat_device_register(struct device *parent, const char *name, void *drvdata,
>   		     const struct attribute_group **groups);
> 

Thanks
Alok

Re: [PATCH v5 2/6] platform/x86: firmware_attributes_class: Add high level API for the attributes interface

[Kurt Borja]

Hi Alok,

On Sun Jul 6, 2025 at 6:28 AM -03, ALOK TIWARI wrote:
>
>
> On 7/5/2025 9:03 AM, Kurt Borja wrote:
>> Add high level API to aid in the creation of attribute groups attached
>> to the `attrs_kobj` (per ABI specification).
>> 
>> This new API lets users configure each group, either statically or
>> dynamically through a (per type) data struct and then create this group
>> through the generic fwat_create_group() macro.
>> 
>> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
>> ---
>>   drivers/platform/x86/firmware_attributes_class.c | 532 +++++++++++++++++++++++
>>   drivers/platform/x86/firmware_attributes_class.h | 335 ++++++++++++++
>>   2 files changed, 867 insertions(+)
>> 
>> diff --git a/drivers/platform/x86/firmware_attributes_class.c b/drivers/platform/x86/firmware_attributes_class.c
>> index 290364202cce64bb0e9046e0b2bbb8d85e2cbc6f..96473b3b1a2a87cf21a6e2a9a14d72ae322c94ae 100644
>> --- a/drivers/platform/x86/firmware_attributes_class.c
>> +++ b/drivers/platform/x86/firmware_attributes_class.c
>> @@ -10,13 +10,67 @@
>>   #include <linux/module.h>
>>   #include <linux/slab.h>
>>   #include <linux/types.h>
>> +#include <linux/string_choices.h>
>>   #include "firmware_attributes_class.h"
>>   
>> +#define FWAT_TYPE_NONE				-1
>> +
>> +#define to_fwat_bool_data(_c) \
>> +	container_of_const(_c, struct fwat_bool_data, group)
>> +#define to_fwat_enum_data(_c) \
>> +	container_of_const(_c, struct fwat_enum_data, group)
>> +#define to_fwat_int_data(_c) \
>> +	container_of_const(_c, struct fwat_int_data, group)
>> +#define to_fwat_str_data(_c) \
>> +	container_of_const(_c, struct fwat_str_data, group)
>> +
>> +struct fwat_attribute {
>> +	struct attribute attr;
>> +	ssize_t (*show)(struct kobject *kobj, struct fwat_attribute *attr,
>> +			char *buf);
>> +	ssize_t (*store)(struct kobject *kobj, struct fwat_attribute *attr,
>> +			 const char *buf, size_t count);
>> +	int type;
>> +};
>> +
>> +#define to_fwat_attribute(_a) \
>> +	container_of_const(_a, struct fwat_attribute, attr)
>> +
>> +#define __FWAT_ATTR(_name, _mode, _show, _store, _type) \
>> +	{								\
>> +		.attr = { .name = __stringify(_name), .mode = _mode },	\
>> +		.show = _show, .store = _store, .type = _type,		\
>> +	}
>> +
>> +#define FWAT_ATTR_RO(_prefix, _name, _show, _type) \
>> +	static struct fwat_attribute fwat_##_prefix##_##_name##_attr = \
>> +		__FWAT_ATTR(_name, 0444, _show, NULL, _type)
>> +
>> +#define FWAT_ATTR_RW(_prefix, _name, _show, _store, _type) \
>> +	static struct fwat_attribute fwat_##_prefix##_##_name##_attr = \
>> +		__FWAT_ATTR(_name, 0644, _show, _store, _type)
>> +
>> +struct fwat_group {
>> +	const struct fwat_group_data *data;
>> +	struct device *dev;
>> +	struct kobject kobj;
>> +};
>> +
>> +#define kobj_to_fwat_group(_k) \
>> +	container_of_const(_k, struct fwat_group, kobj)
>> +
>>   const struct class firmware_attributes_class = {
>>   	.name = "firmware-attributes",
>>   };
>>   EXPORT_SYMBOL_GPL(firmware_attributes_class);
>>   
>> +static const char * const fwat_type_labels[] = {
>> +	[fwat_group_boolean]			= "boolean",
>> +	[fwat_group_enumeration]		= "enumeration",
>> +	[fwat_group_integer]			= "integer",
>> +	[fwat_group_string]			= "string",
>> +};
>> +
>>   static void fwat_device_release(struct device *dev)
>>   {
>>   	struct fwat_device *fadev = to_fwat_device(dev);
>> @@ -24,6 +78,483 @@ static void fwat_device_release(struct device *dev)
>>   	kfree(fadev);
>>   }
>>   
>> +static ssize_t
>> +type_show(struct kobject *kobj, struct fwat_attribute *attr, char *buf)
>> +{
>> +	return sysfs_emit(buf, "%s\n", fwat_type_labels[attr->type]);
>> +}
>> +
>> +static ssize_t
>> +display_name_show(struct kobject *kobj, struct fwat_attribute *attr, char *buf)
>> +{
>> +	struct fwat_group *group = kobj_to_fwat_group(kobj);
>> +	const char *disp_name = group->data->display_name;
>> +
>> +	if (!disp_name)
>> +		return -EOPNOTSUPP;
>> +
>> +	return sysfs_emit(buf, "%s\n", disp_name);
>> +}
>> +
>> +static ssize_t
>> +display_name_language_code_show(struct kobject *kobj, struct fwat_attribute *attr,
>> +				char *buf)
>> +{
>> +	struct fwat_group *group = kobj_to_fwat_group(kobj);
>> +	const char *lang_code = group->data->language_code;
>> +
>> +	if (!lang_code)
>> +		return -EOPNOTSUPP;
>> +
>> +	return sysfs_emit(buf, "%s\n", lang_code);
>> +}
>> +
>> +static ssize_t
>> +bool_group_show(struct kobject *kobj, struct fwat_attribute *attr, char *buf)
>> +{
>> +	const struct fwat_group *group = kobj_to_fwat_group(kobj);
>> +	const struct fwat_bool_data *data = to_fwat_bool_data(group->data);
>> +	bool val;
>> +	int ret;
>> +
>> +	/* show_override does not affect current_value */
>> +	if (data->group.show_override && attr->type != fwat_bool_current_value)
>> +		return data->group.show_override(group->dev, attr->type, buf);
>> +
>> +	switch (attr->type) {
>> +	case fwat_bool_current_value:
>> +		ret = data->read(group->dev, data->group.id, &val);
>> +		if (ret < 0)
>> +			return ret;
>> +		break;
>> +	case fwat_bool_default_value:
>> +		val = data->default_val;
>> +		break;
>> +	default:
>> +		return -EOPNOTSUPP;
>> +	}
>> +
>> +	return sysfs_emit(buf, "%s\n", str_yes_no(val));
>> +}
>> +
>> +static ssize_t
>> +bool_group_store(struct kobject *kobj, struct fwat_attribute *attr, const char *buf,
>> +		 size_t count)
>> +{
>> +	const struct fwat_group *group = kobj_to_fwat_group(kobj);
>> +	const struct fwat_bool_data *data = to_fwat_bool_data(group->data);
>> +	bool val;
>> +	int ret;
>> +
>> +	ret = kstrtobool(buf, &val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = data->write(group->dev, data->group.id, val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return count;
>> +}
>> +
>> +static ssize_t
>> +enum_group_show(struct kobject *kobj, struct fwat_attribute *attr, char *buf)
>> +{
>> +	const struct fwat_group *group = kobj_to_fwat_group(kobj);
>> +	const struct fwat_enum_data *data = to_fwat_enum_data(group->data);
>> +	int val_idx, sz = 0;
>> +	int ret;
>> +
>> +	/* show_override does not affect current_value */
>> +	if (data->group.show_override && attr->type != fwat_enum_current_value)
>> +		return data->group.show_override(group->dev, attr->type, buf);
>> +
>> +	switch (attr->type) {
>> +	case fwat_enum_current_value:
>> +		ret = data->read(group->dev, data->group.id, &val_idx);
>> +		if (ret < 0)
>> +			return ret;
>> +		break;
>> +	case fwat_enum_default_value:
>> +		val_idx = data->default_idx;
>> +		break;
>> +	case fwat_enum_possible_values:
>> +		sz += sysfs_emit_at(buf, sz, "%s", data->possible_vals[0]);
>> +		for (unsigned int i = 1; data->possible_vals[i]; i++)
>> +			sz += sysfs_emit_at(buf, sz, ";%s", data->possible_vals[i]);
>> +		sz += sysfs_emit_at(buf, sz, "\n");
>> +		return sz;
>> +	default:
>> +		return -EOPNOTSUPP;
>> +	}
>> +
>> +	return sysfs_emit(buf, "%s\n", data->possible_vals[val_idx]);
>> +}
>> +
>> +static ssize_t
>> +enum_group_store(struct kobject *kobj, struct fwat_attribute *attr, const char *buf,
>> +		 size_t count)
>> +{
>> +	const struct fwat_group *group = kobj_to_fwat_group(kobj);
>> +	const struct fwat_enum_data *data = to_fwat_enum_data(group->data);
>> +	int val_idx;
>> +	int ret;
>> +
>> +	val_idx = __sysfs_match_string(data->possible_vals, -1, buf);
>> +	if (val_idx < 0)
>> +		return val_idx;
>> +
>> +	ret = data->write(group->dev, data->group.id, val_idx);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return count;
>> +}
>> +
>> +static ssize_t
>> +int_group_show(struct kobject *kobj, struct fwat_attribute *attr, char *buf)
>> +{
>> +	const struct fwat_group *group = kobj_to_fwat_group(kobj);
>> +	const struct fwat_int_data *data = to_fwat_int_data(group->data);
>> +	long val;
>> +	int ret;
>> +
>> +	/* show_override does not affect current_value */
>> +	if (data->group.show_override && attr->type != fwat_int_current_value)
>> +		return data->group.show_override(group->dev, attr->type, buf);
>> +
>> +	switch (attr->type) {
>> +	case fwat_int_current_value:
>> +		ret = data->read(group->dev, data->group.id, &val);
>> +		if (ret < 0)
>> +			return ret;
>> +		break;
>> +	case fwat_int_default_value:
>> +		val = data->default_val;
>> +		break;
>> +	case fwat_int_min_value:
>> +		val = data->min_val;
>> +		break;
>> +	case fwat_int_max_value:
>> +		val = data->max_val;
>> +		break;
>> +	case fwat_int_scalar_increment:
>> +		val = data->increment;
>> +		break;
>> +	default:
>> +		return -EOPNOTSUPP;
>> +	}
>> +
>> +	return sysfs_emit(buf, "%ld\n", val);
>> +}
>> +
>> +static ssize_t
>> +int_group_store(struct kobject *kobj, struct fwat_attribute *attr, const char *buf,
>> +		size_t count)
>> +{
>> +	const struct fwat_group *group = kobj_to_fwat_group(kobj);
>> +	const struct fwat_int_data *data = to_fwat_int_data(group->data);
>> +	long val;
>> +	int ret;
>> +
>> +	ret = kstrtol(buf, 0, &val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = data->write(group->dev, data->group.id, val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return count;
>> +}
>> +
>> +static ssize_t
>> +str_group_show(struct kobject *kobj, struct fwat_attribute *attr, char *buf)
>> +{
>> +	const struct fwat_group *group = kobj_to_fwat_group(kobj);
>> +	const struct fwat_str_data *data = to_fwat_str_data(group->data);
>> +	const char *val;
>> +	long len;
>> +	int ret;
>> +
>> +	/* show_override does not affect current_value */
>> +	if (data->group.show_override && attr->type != fwat_bool_current_value)
>
> fwat_bool_current_value ? or str
>
>> +		return data->group.show_override(group->dev, attr->type, buf);
>> +
>> +	switch (attr->type) {
>> +	case fwat_str_current_value:
>> +		ret = data->read(group->dev, data->group.id, &val);
>> +		if (ret < 0)
>> +			return ret;
>> +		break;
>> +	case fwat_str_default_value:
>> +		val = data->default_val;
>> +		break;
>> +	case fwat_str_min_length:
>> +		len = data->min_len;
>> +		return sysfs_emit(buf, "%ld\n", len);
>> +	case fwat_str_max_length:
>> +		len = data->max_len;
>> +		return sysfs_emit(buf, "%ld\n", len);
>> +	default:
>> +		return -EOPNOTSUPP;
>> +	}
>> +
>> +	return sysfs_emit(buf, "%s\n", val);
>> +}
>> +
>> +static ssize_t
>> +str_group_store(struct kobject *kobj, struct fwat_attribute *attr, const char *buf,
>> +		size_t count)
>> +{
>> +	const struct fwat_group *group = kobj_to_fwat_group(kobj);
>> +	const struct fwat_str_data *data = to_fwat_str_data(group->data);
>> +	int ret;
>> +
>> +	ret = data->write(group->dev, data->group.id, buf);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return count;
>> +}
>> +
>> +FWAT_ATTR_RO(all, display_name, display_name_show, FWAT_TYPE_NONE);
>> +FWAT_ATTR_RO(all, display_name_language_code, display_name_language_code_show, FWAT_TYPE_NONE);
>> +
>> +FWAT_ATTR_RO(bool, type, type_show, fwat_group_boolean);
>> +FWAT_ATTR_RW(bool, current_value, bool_group_show, bool_group_store, fwat_bool_current_value);
>> +FWAT_ATTR_RO(bool, default_value, bool_group_show, fwat_bool_default_value);
>> +
>> +FWAT_ATTR_RO(enum, type, type_show, fwat_group_enumeration);
>> +FWAT_ATTR_RW(enum, current_value, enum_group_show, enum_group_store, fwat_enum_current_value);
>> +FWAT_ATTR_RO(enum, default_value, enum_group_show, fwat_enum_default_value);
>> +FWAT_ATTR_RO(enum, possible_values, enum_group_show, fwat_enum_possible_values);
>> +
>> +FWAT_ATTR_RO(int, type, type_show, fwat_group_integer);
>> +FWAT_ATTR_RW(int, current_value, int_group_show, int_group_store, fwat_int_current_value);
>> +FWAT_ATTR_RO(int, default_value, int_group_show, fwat_int_default_value);
>> +FWAT_ATTR_RO(int, min_value, int_group_show, fwat_int_min_value);
>> +FWAT_ATTR_RO(int, max_value, int_group_show, fwat_int_max_value);
>> +FWAT_ATTR_RO(int, scalar_increment, int_group_show, fwat_int_scalar_increment);
>> +
>> +FWAT_ATTR_RO(str, type, type_show, fwat_group_string);
>> +FWAT_ATTR_RW(str, current_value, str_group_show, str_group_store, fwat_int_current_value);
>
> wrong enum fwat_int_current_value -> fwat_str_current_value
>
>> +FWAT_ATTR_RO(str, default_value, str_group_show, fwat_str_default_value);
>> +FWAT_ATTR_RO(str, min_length, str_group_show, fwat_str_min_length);
>> +FWAT_ATTR_RO(str, max_length, str_group_show, fwat_str_max_length);
>> +
>> +static struct attribute *fwat_bool_attrs[] = {
>> +	&fwat_bool_type_attr.attr,
>> +	&fwat_all_display_name_attr.attr,
>> +	&fwat_all_display_name_language_code_attr.attr,
>> +	&fwat_bool_current_value_attr.attr,
>> +	&fwat_bool_default_value_attr.attr,
>> +	NULL
>> +};
>> +
>> +static struct attribute *fwat_enum_attrs[] = {
>> +	&fwat_enum_type_attr.attr,
>> +	&fwat_all_display_name_attr.attr,
>> +	&fwat_all_display_name_language_code_attr.attr,
>> +	&fwat_enum_current_value_attr.attr,
>> +	&fwat_enum_default_value_attr.attr,
>> +	&fwat_enum_possible_values_attr.attr,
>> +	NULL
>> +};
>> +
>> +static struct attribute *fwat_int_attrs[] = {
>> +	&fwat_int_type_attr.attr,
>> +	&fwat_all_display_name_attr.attr,
>> +	&fwat_all_display_name_language_code_attr.attr,
>> +	&fwat_int_current_value_attr.attr,
>> +	&fwat_int_default_value_attr.attr,
>> +	&fwat_int_min_value_attr.attr,
>> +	&fwat_int_max_value_attr.attr,
>> +	&fwat_int_scalar_increment_attr.attr,
>> +	NULL
>> +};
>> +
>> +static struct attribute *fwat_str_attrs[] = {
>> +	&fwat_str_type_attr.attr,
>> +	&fwat_all_display_name_attr.attr,
>> +	&fwat_all_display_name_language_code_attr.attr,
>> +	&fwat_str_current_value_attr.attr,
>> +	&fwat_str_default_value_attr.attr,
>> +	&fwat_str_min_length_attr.attr,
>> +	&fwat_str_max_length_attr.attr,
>> +	NULL
>> +};
>> +
>> +static umode_t fwat_attr_visible(struct kobject *kobj, struct attribute *attr, int n)
>> +{
>> +	struct fwat_attribute *fwat_attr = to_fwat_attribute(attr);
>> +	struct fwat_group *group = kobj_to_fwat_group(kobj);
>> +	const struct fwat_group_data *data = group->data;
>> +
>> +	/* The `type` attribute is always first */
>> +	if (n == 0)
>> +		return attr->mode;
>> +
>> +	if (attr == &fwat_all_display_name_attr.attr)
>> +		return data->display_name ? attr->mode : 0;
>> +
>> +	if (attr == &fwat_all_display_name_language_code_attr.attr)
>> +		return data->language_code ? attr->mode : 0;
>> +
>> +	/* The `current_value` attribute always has type == 0 */
>> +	if (!fwat_attr->type)
>> +		return data->mode;
>> +
>> +	return test_bit(fwat_attr->type, &data->fattrs) ? attr->mode : 0;
>> +}
>> +
>> +static umode_t fwat_group_visible(struct kobject *kobj)
>> +{
>> +	return true;
>> +}
>> +
>> +DEFINE_SYSFS_GROUP_VISIBLE(fwat);
>> +
>> +static const struct attribute_group fwat_bool_group = {
>> +	.attrs = fwat_bool_attrs,
>> +	.is_visible = SYSFS_GROUP_VISIBLE(fwat),
>> +};
>> +__ATTRIBUTE_GROUPS(fwat_bool);
>> +
>> +static const struct attribute_group fwat_enum_group = {
>> +	.attrs = fwat_enum_attrs,
>> +	.is_visible = SYSFS_GROUP_VISIBLE(fwat),
>> +};
>> +__ATTRIBUTE_GROUPS(fwat_enum);
>> +
>> +static const struct attribute_group fwat_int_group = {
>> +	.attrs = fwat_int_attrs,
>> +	.is_visible = SYSFS_GROUP_VISIBLE(fwat),
>> +};
>> +__ATTRIBUTE_GROUPS(fwat_int);
>> +
>> +static const struct attribute_group fwat_str_group = {
>> +	.attrs = fwat_str_attrs,
>> +	.is_visible = SYSFS_GROUP_VISIBLE(fwat),
>> +};
>> +__ATTRIBUTE_GROUPS(fwat_str);
>> +
>> +static ssize_t
>> +fwat_attr_sysfs_show(struct kobject *kobj, struct attribute *attr, char *buf)
>> +{
>> +	struct fwat_attribute *fwat_attr = to_fwat_attribute(attr);
>> +
>> +	if (!fwat_attr->show)
>> +		return -EOPNOTSUPP;
>> +
>> +	return fwat_attr->show(kobj, fwat_attr, buf);
>> +}
>> +
>> +static ssize_t
>> +fwat_attr_sysfs_store(struct kobject *kobj, struct attribute *attr, const char *buf,
>> +		      size_t count)
>> +{
>> +	struct fwat_attribute *fwat_attr = to_fwat_attribute(attr);
>> +
>> +	if (!fwat_attr->show)
>> +		return -EOPNOTSUPP;
>
> check for fwat_attr->store ?
>
>> +
>> +	return fwat_attr->store(kobj, fwat_attr, buf, count);
>> +}
>> +
>> +static void fwat_group_release(struct kobject *kobj)
>> +{
>> +	struct fwat_group *group = kobj_to_fwat_group(kobj);
>> +
>> +	kfree(group);
>> +}
>> +
>> +static const struct sysfs_ops fwat_attr_sysfs_ops = {
>> +	.show = fwat_attr_sysfs_show,
>> +	.store = fwat_attr_sysfs_store,
>> +};
>> +
>> +static const struct kobj_type fwat_boolean_ktype = {
>> +	.sysfs_ops = &fwat_attr_sysfs_ops,
>> +	.release = fwat_group_release,
>> +	.default_groups = fwat_bool_groups,
>> +};
>> +
>> +static const struct kobj_type fwat_enumeration_ktype = {
>> +	.sysfs_ops = &fwat_attr_sysfs_ops,
>> +	.release = fwat_group_release,
>> +	.default_groups = fwat_enum_groups,
>> +};
>> +
>> +static const struct kobj_type fwat_integer_ktype = {
>> +	.sysfs_ops = &fwat_attr_sysfs_ops,
>> +	.release = fwat_group_release,
>> +	.default_groups = fwat_int_groups,
>> +};
>> +
>> +static const struct kobj_type fwat_string_ktype = {
>> +	.sysfs_ops = &fwat_attr_sysfs_ops,
>> +	.release = fwat_group_release,
>> +	.default_groups = fwat_str_groups,
>> +};
>> +
>> +static int __fwat_create_group(struct fwat_device *fadev, const struct kobj_type *ktype,
>> +			       const struct fwat_group_data *data)
>> +{
>> +	struct fwat_group *group;
>> +	int ret;
>> +
>> +	group = kzalloc(sizeof(*group), GFP_KERNEL);
>> +	if (!group)
>> +		return -ENOMEM;
>> +
>> +	group->dev = &fadev->dev;
>> +	group->data = data;
>> +
>> +	group->kobj.kset = fadev->attrs_kset;
>> +	ret = kobject_init_and_add(&group->kobj, ktype, NULL, "%s", data->name);
>> +	if (ret) {
>> +		kobject_put(&group->kobj);
>> +		return ret;
>> +	}
>> +
>> +	kobject_uevent(&group->kobj, KOBJ_ADD);
>> +
>> +	return 0;
>> +}
>> +
>> +static void fwat_remove_auto_groups(struct fwat_device *fadev)
>> +{
>> +	struct kobject *pos, *n;
>> +
>> +	list_for_each_entry_safe(pos, n, &fadev->attrs_kset->list, entry)
>> +		kobject_put(pos);
>> +}
>> +
>> +int fwat_create_bool_group(struct fwat_device *fadev, const struct fwat_bool_data *data)
>> +{
>> +	return __fwat_create_group(fadev, &fwat_boolean_ktype, &data->group);
>> +}
>> +EXPORT_SYMBOL_GPL(fwat_create_bool_group);
>> +
>> +int fwat_create_enum_group(struct fwat_device *fadev, const struct fwat_enum_data *data)
>> +{
>> +	return __fwat_create_group(fadev, &fwat_enumeration_ktype, &data->group);
>> +}
>> +EXPORT_SYMBOL_GPL(fwat_create_enum_group);
>> +
>> +int fwat_create_int_group(struct fwat_device *fadev, const struct fwat_int_data *data)
>> +{
>> +	return __fwat_create_group(fadev, &fwat_integer_ktype, &data->group);
>> +}
>> +EXPORT_SYMBOL_GPL(fwat_create_int_group);
>> +
>> +int fwat_create_str_group(struct fwat_device *fadev, const struct fwat_str_data *data)
>> +{
>> +	return __fwat_create_group(fadev, &fwat_string_ktype, &data->group);
>> +}
>> +EXPORT_SYMBOL_GPL(fwat_create_str_group);
>> +
>>   /**
>>    * fwat_device_register - Create and register a firmware-attributes class
>>    *			  device
>> @@ -89,6 +620,7 @@ void fwat_device_unregister(struct fwat_device *fadev)
>>   	if (!fadev)
>>   		return;
>>   
>> +	fwat_remove_auto_groups(fadev);
>>   	sysfs_remove_groups(&fadev->attrs_kset->kobj, fadev->groups);
>>   	kset_unregister(fadev->attrs_kset);
>>   	device_unregister(&fadev->dev);
>> diff --git a/drivers/platform/x86/firmware_attributes_class.h b/drivers/platform/x86/firmware_attributes_class.h
>> index 048fd0904f767357ef856e687ec4cf3260016ec6..e8868ce05b595eda94a98975428391b9f9341e3d 100644
>> --- a/drivers/platform/x86/firmware_attributes_class.h
>> +++ b/drivers/platform/x86/firmware_attributes_class.h
>> @@ -10,6 +10,7 @@
>>   #include <linux/device/class.h>
>>   #include <linux/kobject.h>
>>   #include <linux/sysfs.h>
>> +#include <linux/list.h>
>>   
>>   extern const struct class firmware_attributes_class;
>>   
>> @@ -27,6 +28,340 @@ struct fwat_device {
>>   
>>   #define to_fwat_device(_d)	container_of_const(_d, struct fwat_device, dev)
>>   
>> +enum fwat_group_type {
>> +	fwat_group_boolean,
>> +	fwat_group_enumeration,
>> +	fwat_group_integer,
>> +	fwat_group_string,
>> +};
>> +
>> +enum fwat_bool_attrs {
>> +	fwat_bool_current_value,
>> +	fwat_bool_default_value,
>> +	fwat_bool_attrs_last
>> +};
>> +
>> +#define FWAT_BOOL_CURRENT_VALUE			BIT(fwat_bool_current_value)
>> +#define FWAT_BOOL_DEFAULT_VALUE			BIT(fwat_bool_default_value)
>> +#define FWAT_BOOL_ALL_ATTRS			GENMASK(fwat_bool_attrs_last, 0)
>> +
>> +enum fwat_enum_attrs {
>> +	fwat_enum_current_value,
>> +	fwat_enum_default_value,
>> +	fwat_enum_possible_values,
>> +	fwat_enum_attrs_last
>> +};
>> +
>> +#define FWAT_ENUM_CURRENT_VALUE			BIT(fwat_enum_current_value)
>> +#define FWAT_ENUM_DEFAULT_VALUE			BIT(fwat_enum_default_value)
>> +#define FWAT_ENUM_POSSIBLE_VALUES		BIT(fwat_enum_possible_values)
>> +#define FWAT_ENUM_ALL_ATTRS			GENMASK(fwat_enum_attrs_last, 0)
>> +
>> +enum fwat_int_attrs {
>> +	fwat_int_current_value,
>> +	fwat_int_default_value,
>> +	fwat_int_min_value,
>> +	fwat_int_max_value,
>> +	fwat_int_scalar_increment,
>> +	fwat_int_attrs_last
>> +};
>> +
>> +#define FWAT_INT_CURRENT_VALUE			BIT(fwat_int_current_value)
>> +#define FWAT_INT_DEFAULT_VALUE			BIT(fwat_int_default_value)
>> +#define FWAT_INT_MIN_VALUE			BIT(fwat_int_min_value)
>> +#define FWAT_INT_MAX_VALUE			BIT(fwat_int_max_value)
>> +#define FWAT_INT_SCALAR_INCREMENT		BIT(fwat_int_scalar_increment)
>> +#define FWAT_INT_ALL_ATTRS			GENMASK(fwat_int_attrs_last, 0)
>> +
>> +enum fwat_str_attrs {
>> +	fwat_str_current_value,
>> +	fwat_str_default_value,
>> +	fwat_str_min_length,
>> +	fwat_str_max_length,
>> +	fwat_str_attrs_last
>> +};
>> +
>> +#define FWAT_STR_CURRENT_VALUE			BIT(fwat_str_current_value)
>> +#define FWAT_STR_DEFAULT_VALUE			BIT(fwat_str_default_value)
>> +#define FWAT_STR_MIN_LENGTH			BIT(fwat_str_min_length)
>> +#define FWAT_STR_MAX_LENGTH			BIT(fwat_str_max_length)
>> +#define FWAT_STR_ALL_ATTRS			GENMASK(fwat_str_attrs_last, 0)
>> +
>> +static_assert(fwat_bool_current_value == 0);
>> +static_assert(fwat_enum_current_value == 0);
>> +static_assert(fwat_int_current_value == 0);
>> +static_assert(fwat_str_current_value == 0);
>> +
>> +/**
>> + * struct fwat_group_data - Data struct common between group types
>> + * @id: Group ID defined by the user.
>> + * @name: Name of the group.
>> + * @display_name: Name showed in the display_name attribute. (Optional)
>> + * @language_code: Language code showed in the display_name_language_code
>> + *                 attribute. (Optional)
>> + * @mode: Mode for the current_value attribute. All other attributes will have
>> + *        0444 permissions.
>> + * @fattrs: Bitmap of selected attributes for this group type.
>> + * @show_override: Custom show method for attributes in this group, except for
>> + *		   the current_value attribute, for which the a `read` callback
>> + *		   will still be used. (Optional)
>> + *
>> + * NOTE: This struct is not meant to be defined directly. It is supposed to be
>> + * embedded and defined as part of fwat_[type]_data structs.
>> + */
>> +struct fwat_group_data {
>> +	long id;
>> +	umode_t mode;
>> +	const char *name;
>> +	const char *display_name;
>> +	const char *language_code;
>> +	unsigned long fattrs;
>> +	ssize_t (*show_override)(struct device *dev, int type, char *buf);
>> +};
>> +
>> +/**
>> + * struct fwat_bool_data - Data struct for the boolean group type
>> + * @read: Read callback for the current_value attribute.
>> + * @write: Write callback for the current_value attribute.
>> + * @default_val: Default value.
>> + * @group: Group data.
>> + */
>> +struct fwat_bool_data {
>> +	int (*read)(struct device *dev, long id, bool *val);
>> +	int (*write)(struct device *dev, long id, bool val);
>> +	bool default_val;
>> +	struct fwat_group_data group;
>> +};
>> +
>> +/**
>> + * struct fwat_enum_data - Data struct for the enumeration group type
>> + * @read: Read callback for the current_value attribute.
>> + * @write: Write callback for the current_value attribute.
>> + * @default_idx: Index of the default value in the @possible_vals array.
>> + * @possible_vals: Array of possible value strings for this group type.
>> + * @group: Group data.
>> + *
>> + * NOTE: The `val_idx` argument in the @write callback is guaranteed to be a
>> + *       valid (within bounds) index. However, the user is in charge of writing
>> + *       valid indexes to the `*val_idx` argument of the @read callback.
>> + *       Failing to do so may result in an OOB access.
>> + */
>> +struct fwat_enum_data {
>> +	int (*read)(struct device *dev, long id, int *val_idx);
>> +	int (*write)(struct device *dev, long id, int val_idx);
>> +	int default_idx;
>> +	const char * const *possible_vals;
>> +	struct fwat_group_data group;
>> +};
>> +
>> +/**
>> + * struct fwat_int_data - Data struct for the integer group type
>> + * @read: Read callback for the current_value attribute.
>> + * @write: Write callback for the current_value attribute.
>> + * @default_val: Default value.
>> + * @min_val: Minimum value.
>> + * @max_val: Maximum value.
>> + * @increment: Scalar increment for this value.
>> + * @group: Group data.
>> + *
>> + * NOTE: The @min_val, @max_val, @increment constraints are merely informative.
>> + *       These values are not enforced in any of the callbacks.
>> + */
>> +struct fwat_int_data {
>> +	int (*read)(struct device *dev, long id, long *val);
>> +	int (*write)(struct device *dev, long id, long val);
>> +	long default_val;
>> +	long min_val;
>> +	long max_val;
>> +	long increment;
>> +	struct fwat_group_data group;
>> +};
>> +
>> +/**
>> + * struct fwat_str_data - Data struct for the string group type
>> + * @read: Read callback for the current_value attribute.
>> + * @write: Write callback for the current_value attribute.
>> + * @default_val: Default value.
>> + * @min_len: Minimum string length.
>> + * @max_len: Maximum string length.
>> + * @group: Group data.
>> + *
>> + * NOTE: The @min_len, @max_len constraints are merely informative. These
>> + *       values are not enforced in any of the callbacks.
>> + */
>> +struct fwat_str_data {
>> +	int (*read)(struct device *dev, long id, const char **buf);
>> +	int (*write)(struct device *dev, long id, const char *buf);
>> +	const char *default_val;
>> +	long min_len;
>> +	long max_len;
>> +	struct fwat_group_data group;
>> +};
>> +
>> +#define __FWAT_GROUP(_name, _disp_name, _mode, _fattrs) \
>> +	{ .name = __stringify(_name), .display_name = _disp_name, .mode = _mode, .fattrs = _fattrs }
>> +
>> +/**
>> + * DEFINE_FWAT_BOOL_GROUP - Convenience macro to quickly define an static
>
> an static -> a static (all place)
>
>> + *                          struct fwat_bool_data instance
>> + * @_name: Name of the group.
>> + * @_disp_name: Name showed in the display_name attribute. (Optional)
>
> showed -> shown (all place)
>
>> + * @_def_val: Default value.
>> + * @_mode: Mode for the current_value attribute. All other attributes will have
>> + *         0444 permissions.
>> + * @_fattrs: Bitmap of selected attributes for this group type.
>> + *
>> + * `read` and `write` callbacks are required to be already defined as
>> + * `_name##_read` and `_name##_write` respectively.
>> + */
>> +#define DEFINE_FWAT_BOOL_GROUP(_name, _disp_name, _def_val, _mode, _fattrs) \
>> +	static const struct fwat_bool_data _name##_group_data = {	\
>> +		.read = _name##_read,					\
>> +		.write = _name##_write,					\
>> +		.default_val = _def_val,				\
>> +		.group = __FWAT_GROUP(_name, _disp_name, _mode, _fattrs), \
>> +	}
>> +
>> +/**
>> + * DEFINE_FWAT_ENUM_GROUP - Convenience macro to quickly define an static
>> + *                          struct fwat_enum_data instance
>> + * @_name: Name of the group.
>> + * @_disp_name: Name showed in the display_name attribute. (Optional)
>> + * @_def_idx: Index of the default value in the @_poss_vals array.
>> + * @_poss_vals: Array of possible value strings for this group type.
>> + * @_mode: Mode for the current_value attribute. All other attributes will have
>> + *         0444 permissions.
>> + * @_fattrs: Bitmap of selected attributes for this group type.
>> + *
>> + * `read` and `write` callbacks are required to be already defined as
>> + * `_name##_read` and `_name##_write` respectively.
>> + *
>> + * NOTE: The `val_idx` argument in the `write` callback is guaranteed to be a
>> + *       valid (within bounds) index. However, the user is in charge of writing
>> + *       valid indexes to the `*val_idx` argument of the `read` callback.
>> + *       Failing to do so may result in an OOB access.
>> + */
>> +#define DEFINE_FWAT_ENUM_GROUP(_name, _disp_name, _poss_vals, _def_idx, _mode, _fattrs) \
>> +	static const struct fwat_enum_data _name##_group_data = {	\
>> +		.read = _name##_read,					\
>> +		.write = _name##_write,					\
>> +		.default_idx = _def_idx,				\
>> +		.possible_vals = _poss_vals,				\
>> +		.group = __FWAT_GROUP(_name, _disp_name, _mode, _fattrs), \
>> +	}
>> +
>> +/**
>> + * DEFINE_FWAT_INT_GROUP - Convenience macro to quickly define an static
>> + *                         struct fwat_int_data instance
>> + * @_name: Name of the group.
>> + * @_disp_name: Name showed in the display_name attribute. (Optional)
>> + * @_def_val: Default value.
>> + * @_min: Minimum value.
>> + * @_max: Maximum value.
>> + * @_inc: Scalar increment for this value.
>> + * @_mode: Mode for the current_value attribute. All other attributes will have
>> + *         0444 permissions.
>> + * @_fattrs: Bitmap of selected attributes for this group type.
>> + *
>> + * `read` and `write` callbacks are required to be already defined as
>> + * `_name##_read` and `_name##_write` respectively.
>> + *
>> + * NOTE: The @_min, @_max, @_inc constraints are merely informative. These
>> + *       values are not enforced in any of the callbacks.
>> + */
>> +#define DEFINE_FWAT_INT_GROUP(_name, _disp_name, _def_val, _min, _max, _inc, _mode, _fattrs) \
>> +	static const struct fwat_int_data _name##_group_data = {	\
>> +		.read = _name##_read,					\
>> +		.write = _name##_write,					\
>> +		.default_val = _def_val,				\
>> +		.min_val = _min,					\
>> +		.max_val = _max,					\
>> +		.increment = _inc,					\
>> +		.group = __FWAT_GROUP(_name, _disp_name, _mode, _fattrs), \
>> +	}
>> +
>> +/**
>> + * DEFINE_FWAT_STR_GROUP - Convenience macro to quickly define an static
>> + *                         struct fwat_str_data instance
>> + * @_name: Name of the group.
>> + * @_disp_name: Name showed in the display_name attribute. (Optional)
>> + * @_def_val: Default value.
>> + * @_min: Minimum string length.
>> + * @_max: Maximum string length.
>> + * @_mode: Mode for the current_value attribute. All other attributes will have
>> + *         0444 permissions.
>> + * @_fattrs: Bitmap of selected attributes for this group type.
>> + *
>> + * `read` and `write` callbacks are required to be already defined as
>> + * `_name##_read` and `_name##_write` respectively.
>> + *
>> + * NOTE: The @_min, @_max constraints are merely informative. These values are
>> + *       not enforced in any of the callbacks.
>> + */
>> +#define DEFINE_FWAT_STR_GROUP(_name, _disp_name, _def_val, _min, _max, _mode, _fattrs) \
>> +	static const struct fwat_str_data _name##_group_data = {	\
>> +		.read = _name##_read,					\
>> +		.write = _name##_write,					\
>> +		.default_val = _def_val,				\
>> +		.min_len = _min,					\
>> +		.max_len = _max,					\
>> +		.group = __FWAT_GROUP(_name, _disp_name, _mode, _fattrs), \
>> +	}
>> +
>> +int fwat_create_bool_group(struct fwat_device *fadev,
>> +			   const struct fwat_bool_data *data);
>> +int fwat_create_enum_group(struct fwat_device *fadev,
>> +			   const struct fwat_enum_data *data);
>> +int fwat_create_int_group(struct fwat_device *fadev,
>> +			  const struct fwat_int_data *data);
>> +int fwat_create_str_group(struct fwat_device *fadev,
>> +			  const struct fwat_str_data *data);
>> +
>> +/**
>> + * fwat_create_group - Convenience generic macro to create a group
>> + * @_dev: fwat_device
>> + * @_data: One of fwat_{bool,enum,int,str}_data instance
>> + *
>> + * This macro (and associated functions) creates a sysfs group under the
>> + * 'attributes' directory, which is located in the class device root directory.
>> + *
>> + * See Documentation/ABI/testing/sysfs-class-firmware-attributes for details.
>> + *
>> + * The @_data associated with this group may be created either statically,
>> + * through DEFINE_FWAT_*_GROUP macros or dynamically, in which case the user
>> + * would have allocate and fill the struct manually. The dynamic approach should
>> + * be preferred when group constraints and/or visibility is decided dynamically.
>> + *
>> + * Example:
>> + *
>> + * static int stat_read(...){...};
>> + * static int stat_write(...){...};
>> + *
>> + * DEFINE_FWAT_(BOOL|ENUM|INT|STR)_GROUP(stat, ...);
>> + *
>> + * static int create_groups(struct fwat_device *fadev)
>> + * {
>> + *	struct fwat_enum_data *dyn_group_data;
>> + *
>> + *	dyn_group_data = kzalloc(...);
>> + *	// Fill the data
>> + *	...
>> + *	fwat_create_group(fadev, &stat_group_data);
>> + *	fwat_create_group(fadev, &dyn_group_data);
>> + *	fwat_create_group(...);
>> + *	...
>> + * }
>> + *
>> + * Return: 0 on success, -errno on failure
>> + */
>> +#define fwat_create_group(_dev, _data) \
>> +	_Generic((_data),							\
>> +		 const struct fwat_bool_data * : fwat_create_bool_group,	\
>> +		 const struct fwat_enum_data * : fwat_create_enum_group,	\
>> +		 const struct fwat_int_data * : fwat_create_int_group,		\
>> +		 const struct fwat_str_data * : fwat_create_str_group)		\
>> +		(_dev, _data)
>> +
>>   struct fwat_device * __must_check
>>   fwat_device_register(struct device *parent, const char *name, void *drvdata,
>>   		     const struct attribute_group **groups);
>> 
>
> Thanks
> Alok

Ack for everything. Thank you!


-- 
 ~ Kurt

[PATCH v5 3/6] platform/x86: firmware_attributes_class: Move header to include directory

[Kurt Borja]

Move firmware_attributes_class.h to include/linux/ to avoid hardcoding
paths inside drivers/platform/x86/.

Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
 drivers/platform/x86/dell/dell-wmi-sysman/sysman.c                  | 2 +-
 drivers/platform/x86/firmware_attributes_class.c                    | 2 +-
 drivers/platform/x86/hp/hp-bioscfg/bioscfg.c                        | 2 +-
 drivers/platform/x86/lenovo/think-lmi.c                             | 2 +-
 drivers/platform/x86/samsung-galaxybook.c                           | 2 +-
 {drivers/platform/x86 => include/linux}/firmware_attributes_class.h | 0
 6 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/dell/dell-wmi-sysman/sysman.c b/drivers/platform/x86/dell/dell-wmi-sysman/sysman.c
index d00389b860e4ea0655c740c78bc3751f323b6370..3aec09987ab145508ed05b02e61a6d94edf79484 100644
--- a/drivers/platform/x86/dell/dell-wmi-sysman/sysman.c
+++ b/drivers/platform/x86/dell/dell-wmi-sysman/sysman.c
@@ -12,8 +12,8 @@
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/wmi.h>
+#include <linux/firmware_attributes_class.h>
 #include "dell-wmi-sysman.h"
-#include "../../firmware_attributes_class.h"
 
 #define MAX_TYPES  4
 #include <linux/nls.h>
diff --git a/drivers/platform/x86/firmware_attributes_class.c b/drivers/platform/x86/firmware_attributes_class.c
index 96473b3b1a2a87cf21a6e2a9a14d72ae322c94ae..25a0f424ea9f691570f399e1771da4fc9ff12397 100644
--- a/drivers/platform/x86/firmware_attributes_class.c
+++ b/drivers/platform/x86/firmware_attributes_class.c
@@ -11,7 +11,7 @@
 #include <linux/slab.h>
 #include <linux/types.h>
 #include <linux/string_choices.h>
-#include "firmware_attributes_class.h"
+#include <linux/firmware_attributes_class.h>
 
 #define FWAT_TYPE_NONE				-1
 
diff --git a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
index 13237890fc92002e7e730b1c235ddf068a6737cd..2df31af8a3b4ac88710af1fae2d5dabbb3185f1d 100644
--- a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
+++ b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
@@ -12,7 +12,7 @@
 #include <linux/kernel.h>
 #include <linux/wmi.h>
 #include "bioscfg.h"
-#include "../../firmware_attributes_class.h"
+#include <linux/firmware_attributes_class.h>
 #include <linux/nls.h>
 #include <linux/errno.h>
 
diff --git a/drivers/platform/x86/lenovo/think-lmi.c b/drivers/platform/x86/lenovo/think-lmi.c
index 34a47269e3d34d2eda6b71af73892656cd2bf67d..f61a6287eb0ebe9ac4c0c9445c3b54c12b276691 100644
--- a/drivers/platform/x86/lenovo/think-lmi.c
+++ b/drivers/platform/x86/lenovo/think-lmi.c
@@ -20,7 +20,7 @@
 #include <linux/types.h>
 #include <linux/dmi.h>
 #include <linux/wmi.h>
-#include "../firmware_attributes_class.h"
+#include <linux/firmware_attributes_class.h>
 #include "think-lmi.h"
 
 static bool debug_support;
diff --git a/drivers/platform/x86/samsung-galaxybook.c b/drivers/platform/x86/samsung-galaxybook.c
index 5878a351993eb05a4c5c2c75b4915d972ce9becc..9a5a7b956a9f6a2738470e83ce93f4cccf4bf3b4 100644
--- a/drivers/platform/x86/samsung-galaxybook.c
+++ b/drivers/platform/x86/samsung-galaxybook.c
@@ -28,7 +28,7 @@
 #include <linux/uuid.h>
 #include <linux/workqueue.h>
 #include <acpi/battery.h>
-#include "firmware_attributes_class.h"
+#include <linux/firmware_attributes_class.h>
 
 #define DRIVER_NAME "samsung-galaxybook"
 
diff --git a/drivers/platform/x86/firmware_attributes_class.h b/include/linux/firmware_attributes_class.h
similarity index 100%
rename from drivers/platform/x86/firmware_attributes_class.h
rename to include/linux/firmware_attributes_class.h

-- 
2.50.0

[PATCH v5 4/6] platform/x86: samsung-galaxybook: Transition new firmware_attributes API

[Kurt Borja]

Transition to new firmware_attributes API.

Defining firmware_attributes groups statically through
DEFINE_FWAT_ENUM_GROUP() incurs in a minor ABI change. In particular the
display_name_language_code attribute is no longer created. Fortunately,
this doesn't break user-space compatibility, because this attribute is
not required, neither by the ABI specification nor by user-space tools.

Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
 drivers/platform/x86/samsung-galaxybook.c | 244 ++++++++----------------------
 1 file changed, 61 insertions(+), 183 deletions(-)

diff --git a/drivers/platform/x86/samsung-galaxybook.c b/drivers/platform/x86/samsung-galaxybook.c
index 9a5a7b956a9f6a2738470e83ce93f4cccf4bf3b4..d1db2a3f716aafc5a314f2bdc57f7c958a9ae346 100644
--- a/drivers/platform/x86/samsung-galaxybook.c
+++ b/drivers/platform/x86/samsung-galaxybook.c
@@ -36,8 +36,6 @@ struct samsung_galaxybook {
 	struct platform_device *platform;
 	struct acpi_device *acpi;
 
-	struct device *fw_attrs_dev;
-	struct kset *fw_attrs_kset;
 	/* block in case firmware attributes are updated in multiple threads */
 	struct mutex fw_attr_lock;
 
@@ -60,36 +58,6 @@ struct samsung_galaxybook {
 	u8 profile_performance_modes[PLATFORM_PROFILE_LAST];
 };
 
-enum galaxybook_fw_attr_id {
-	GB_ATTR_POWER_ON_LID_OPEN,
-	GB_ATTR_USB_CHARGING,
-	GB_ATTR_BLOCK_RECORDING,
-};
-
-static const char * const galaxybook_fw_attr_name[] = {
-	[GB_ATTR_POWER_ON_LID_OPEN] = "power_on_lid_open",
-	[GB_ATTR_USB_CHARGING]      = "usb_charging",
-	[GB_ATTR_BLOCK_RECORDING]   = "block_recording",
-};
-
-static const char * const galaxybook_fw_attr_desc[] = {
-	[GB_ATTR_POWER_ON_LID_OPEN] = "Power On Lid Open",
-	[GB_ATTR_USB_CHARGING]      = "USB Charging",
-	[GB_ATTR_BLOCK_RECORDING]   = "Block Recording",
-};
-
-#define GB_ATTR_LANGUAGE_CODE "en_US.UTF-8"
-
-struct galaxybook_fw_attr {
-	struct samsung_galaxybook *galaxybook;
-	enum galaxybook_fw_attr_id fw_attr_id;
-	struct attribute_group attr_group;
-	struct kobj_attribute display_name;
-	struct kobj_attribute current_value;
-	int (*get_value)(struct samsung_galaxybook *galaxybook, bool *value);
-	int (*set_value)(struct samsung_galaxybook *galaxybook, const bool value);
-};
-
 struct sawb {
 	u16 safn;
 	u16 sasb;
@@ -908,193 +876,106 @@ static int galaxybook_block_recording_init(struct samsung_galaxybook *galaxybook
 
 /* Firmware Attributes setup */
 
-static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+static const char * const galaxybook_possible_vals[] = {
+	[false] = "0", [true] = "1", NULL
+};
+
+static int power_on_lid_open_read(struct device *dev, long id, int *val_idx)
 {
-	return sysfs_emit(buf, "enumeration\n");
-}
-
-static struct kobj_attribute fw_attr_type = __ATTR_RO(type);
-
-static ssize_t default_value_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
-{
-	return sysfs_emit(buf, "0\n");
-}
-
-static struct kobj_attribute fw_attr_default_value = __ATTR_RO(default_value);
-
-static ssize_t possible_values_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
-{
-	return sysfs_emit(buf, "0;1\n");
-}
-
-static struct kobj_attribute fw_attr_possible_values = __ATTR_RO(possible_values);
-
-static ssize_t display_name_language_code_show(struct kobject *kobj, struct kobj_attribute *attr,
-					       char *buf)
-{
-	return sysfs_emit(buf, "%s\n", GB_ATTR_LANGUAGE_CODE);
-}
-
-static struct kobj_attribute fw_attr_display_name_language_code =
-	__ATTR_RO(display_name_language_code);
-
-static ssize_t display_name_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
-{
-	struct galaxybook_fw_attr *fw_attr =
-		container_of(attr, struct galaxybook_fw_attr, display_name);
-
-	return sysfs_emit(buf, "%s\n", galaxybook_fw_attr_desc[fw_attr->fw_attr_id]);
-}
-
-static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
-{
-	struct galaxybook_fw_attr *fw_attr =
-		container_of(attr, struct galaxybook_fw_attr, current_value);
-	bool value;
+	struct samsung_galaxybook *galaxybook = dev_get_drvdata(dev);
+	bool val;
 	int err;
 
-	err = fw_attr->get_value(fw_attr->galaxybook, &value);
+	err = power_on_lid_open_acpi_get(galaxybook, &val);
 	if (err)
 		return err;
 
-	return sysfs_emit(buf, "%u\n", value);
+	*val_idx = val;
+
+	return 0;
 }
 
-static ssize_t current_value_store(struct kobject *kobj, struct kobj_attribute *attr,
-				   const char *buf, size_t count)
+static int power_on_lid_open_write(struct device *dev, long id, int val_idx)
 {
-	struct galaxybook_fw_attr *fw_attr =
-		container_of(attr, struct galaxybook_fw_attr, current_value);
-	struct samsung_galaxybook *galaxybook = fw_attr->galaxybook;
-	bool value;
+	struct samsung_galaxybook *galaxybook = dev_get_drvdata(dev);
+
+	return power_on_lid_open_acpi_set(galaxybook, val_idx ? true : false);
+}
+
+DEFINE_FWAT_ENUM_GROUP(power_on_lid_open, "Power On Lid Open", galaxybook_possible_vals,
+		       false, 0644, FWAT_ENUM_ALL_ATTRS);
+
+static int usb_charging_read(struct device *dev, long id, int *val_idx)
+{
+	struct samsung_galaxybook *galaxybook = dev_get_drvdata(dev);
+	bool val;
 	int err;
 
-	if (!count)
-		return -EINVAL;
-
-	err = kstrtobool(buf, &value);
+	err = usb_charging_acpi_get(galaxybook, &val);
 	if (err)
 		return err;
 
-	guard(mutex)(&galaxybook->fw_attr_lock);
+	*val_idx = val;
 
-	err = fw_attr->set_value(galaxybook, value);
+	return 0;
+}
+
+static int usb_charging_write(struct device *dev, long id, int val_idx)
+{
+	struct samsung_galaxybook *galaxybook = dev_get_drvdata(dev);
+
+	return usb_charging_acpi_set(galaxybook, val_idx ? true : false);
+}
+
+DEFINE_FWAT_ENUM_GROUP(usb_charging, "USB Charging", galaxybook_possible_vals,
+		       false, 0644, FWAT_ENUM_ALL_ATTRS);
+
+static int block_recording_read(struct device *dev, long id, int *val_idx)
+{
+	struct samsung_galaxybook *galaxybook = dev_get_drvdata(dev);
+	bool val;
+	int err;
+
+	err = block_recording_acpi_get(galaxybook, &val);
 	if (err)
 		return err;
 
-	return count;
+	*val_idx = val;
+
+	return 0;
 }
 
-#define NUM_FW_ATTR_ENUM_ATTRS  6
-
-static int galaxybook_fw_attr_init(struct samsung_galaxybook *galaxybook,
-				   const enum galaxybook_fw_attr_id fw_attr_id,
-				   int (*get_value)(struct samsung_galaxybook *galaxybook,
-						    bool *value),
-				   int (*set_value)(struct samsung_galaxybook *galaxybook,
-						    const bool value))
+static int block_recording_write(struct device *dev, long id, int val_idx)
 {
-	struct galaxybook_fw_attr *fw_attr;
-	struct attribute **attrs;
 
-	fw_attr = devm_kzalloc(&galaxybook->platform->dev, sizeof(*fw_attr), GFP_KERNEL);
-	if (!fw_attr)
-		return -ENOMEM;
+	struct samsung_galaxybook *galaxybook = dev_get_drvdata(dev);
 
-	attrs = devm_kcalloc(&galaxybook->platform->dev, NUM_FW_ATTR_ENUM_ATTRS + 1,
-			     sizeof(*attrs), GFP_KERNEL);
-	if (!attrs)
-		return -ENOMEM;
-
-	attrs[0] = &fw_attr_type.attr;
-	attrs[1] = &fw_attr_default_value.attr;
-	attrs[2] = &fw_attr_possible_values.attr;
-	attrs[3] = &fw_attr_display_name_language_code.attr;
-
-	sysfs_attr_init(&fw_attr->display_name.attr);
-	fw_attr->display_name.attr.name = "display_name";
-	fw_attr->display_name.attr.mode = 0444;
-	fw_attr->display_name.show = display_name_show;
-	attrs[4] = &fw_attr->display_name.attr;
-
-	sysfs_attr_init(&fw_attr->current_value.attr);
-	fw_attr->current_value.attr.name = "current_value";
-	fw_attr->current_value.attr.mode = 0644;
-	fw_attr->current_value.show = current_value_show;
-	fw_attr->current_value.store = current_value_store;
-	attrs[5] = &fw_attr->current_value.attr;
-
-	attrs[6] = NULL;
-
-	fw_attr->galaxybook = galaxybook;
-	fw_attr->fw_attr_id = fw_attr_id;
-	fw_attr->attr_group.name = galaxybook_fw_attr_name[fw_attr_id];
-	fw_attr->attr_group.attrs = attrs;
-	fw_attr->get_value = get_value;
-	fw_attr->set_value = set_value;
-
-	return sysfs_create_group(&galaxybook->fw_attrs_kset->kobj, &fw_attr->attr_group);
+	return block_recording_acpi_set(galaxybook, val_idx ? true : false);
 }
 
-static void galaxybook_kset_unregister(void *data)
-{
-	struct kset *kset = data;
-
-	kset_unregister(kset);
-}
-
-static void galaxybook_fw_attrs_dev_unregister(void *data)
-{
-	struct device *fw_attrs_dev = data;
-
-	device_unregister(fw_attrs_dev);
-}
+DEFINE_FWAT_ENUM_GROUP(block_recording, "Block Recording", galaxybook_possible_vals,
+		       false, 0644, FWAT_ENUM_ALL_ATTRS);
 
 static int galaxybook_fw_attrs_init(struct samsung_galaxybook *galaxybook)
 {
+	struct fwat_device *fdev;
 	bool value;
 	int err;
 
-	err = devm_mutex_init(&galaxybook->platform->dev, &galaxybook->fw_attr_lock);
-	if (err)
-		return err;
-
-	galaxybook->fw_attrs_dev = device_create(&firmware_attributes_class, NULL, MKDEV(0, 0),
-						 NULL, "%s", DRIVER_NAME);
-	if (IS_ERR(galaxybook->fw_attrs_dev))
-		return PTR_ERR(galaxybook->fw_attrs_dev);
-
-	err = devm_add_action_or_reset(&galaxybook->platform->dev,
-				       galaxybook_fw_attrs_dev_unregister,
-				       galaxybook->fw_attrs_dev);
-	if (err)
-		return err;
-
-	galaxybook->fw_attrs_kset = kset_create_and_add("attributes", NULL,
-							&galaxybook->fw_attrs_dev->kobj);
-	if (!galaxybook->fw_attrs_kset)
-		return -ENOMEM;
-	err = devm_add_action_or_reset(&galaxybook->platform->dev,
-				       galaxybook_kset_unregister, galaxybook->fw_attrs_kset);
-	if (err)
-		return err;
+	fdev = devm_fwat_device_register(&galaxybook->platform->dev, DRIVER_NAME, galaxybook, NULL);
+	if (IS_ERR(fdev))
+		return PTR_ERR(fdev);
 
 	err = power_on_lid_open_acpi_get(galaxybook, &value);
 	if (!err) {
-		err = galaxybook_fw_attr_init(galaxybook,
-					      GB_ATTR_POWER_ON_LID_OPEN,
-					      &power_on_lid_open_acpi_get,
-					      &power_on_lid_open_acpi_set);
+		err = fwat_create_group(fdev, &power_on_lid_open_group_data);
 		if (err)
 			return err;
 	}
 
 	err = usb_charging_acpi_get(galaxybook, &value);
 	if (!err) {
-		err = galaxybook_fw_attr_init(galaxybook,
-					      GB_ATTR_USB_CHARGING,
-					      &usb_charging_acpi_get,
-					      &usb_charging_acpi_set);
+		err = fwat_create_group(fdev, &usb_charging_group_data);
 		if (err)
 			return err;
 	}
@@ -1107,10 +988,7 @@ static int galaxybook_fw_attrs_init(struct samsung_galaxybook *galaxybook)
 
 	galaxybook->has_block_recording = true;
 
-	return galaxybook_fw_attr_init(galaxybook,
-				       GB_ATTR_BLOCK_RECORDING,
-				       &block_recording_acpi_get,
-				       &block_recording_acpi_set);
+	return fwat_create_group(fdev, &block_recording_group_data);
 }
 
 /*

-- 
2.50.0

Re: [PATCH v5 4/6] platform/x86: samsung-galaxybook: Transition new firmware_attributes API

[Thomas Weißschuh]

On 2025-07-05 00:33:59-0300, Kurt Borja wrote:
> Transition to new firmware_attributes API.
> 
> Defining firmware_attributes groups statically through
> DEFINE_FWAT_ENUM_GROUP() incurs in a minor ABI change. In particular the
> display_name_language_code attribute is no longer created. Fortunately,
> this doesn't break user-space compatibility, because this attribute is
> not required, neither by the ABI specification nor by user-space tools.
> 
> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
> ---
>  drivers/platform/x86/samsung-galaxybook.c | 244 ++++++++----------------------
>  1 file changed, 61 insertions(+), 183 deletions(-)

<snip>

>  
>  static int galaxybook_fw_attrs_init(struct samsung_galaxybook *galaxybook)
>  {
> +	struct fwat_device *fdev;
>  	bool value;
>  	int err;
>  
> -	err = devm_mutex_init(&galaxybook->platform->dev, &galaxybook->fw_attr_lock);

The mutex is still used, so this can't be removed.

> -	if (err)
> -		return err;
> -

<snip>

[PATCH v5 5/6] Documentation: ABI: Update sysfs-class-firmware-attributes documentation

[Kurt Borja]

Add a simple boolean type.

Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
 Documentation/ABI/testing/sysfs-class-firmware-attributes | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/ABI/testing/sysfs-class-firmware-attributes b/Documentation/ABI/testing/sysfs-class-firmware-attributes
index 2713efa509b465a39bf014180794bf487e5b42d6..64b8d5d49716e8387fee26e3e56910862f6a4f5c 100644
--- a/Documentation/ABI/testing/sysfs-class-firmware-attributes
+++ b/Documentation/ABI/testing/sysfs-class-firmware-attributes
@@ -18,6 +18,7 @@ Description:
 
 		The following are known types:
 
+			- boolean
 			- enumeration: a set of pre-defined valid values
 			- integer: a range of numerical values
 			- string

-- 
2.50.0

[PATCH v5 6/6] MAINTAINERS: Add FIRMWARE ATTRIBUTES CLASS entry

[Kurt Borja]

Add entry for the FIRMWARE ATTRIBUTES CLASS.

Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
 MAINTAINERS | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index c14614613377df7f40565c6df50661fe3f510034..c799f603e9210e4703eeb1f0ac9d6b9e8bd469c5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9352,6 +9352,14 @@ F:	include/linux/firewire.h
 F:	include/uapi/linux/firewire*.h
 F:	tools/firewire/
 
+FIRMWARE ATTRIBUTES CLASS
+M:	Kurt Borja <kuurtb@gmail.com>
+L:	platform-driver-x86@vger.kernel.org
+S:	Maintained
+F:	drivers/platform/x86/firmware_attributes_class.c
+F:	include/linux/firmware_attributes_class.h
+K:	(devm_)?fwat_device_(un)?register
+
 FIRMWARE FRAMEWORK FOR ARMV8-A
 M:	Sudeep Holla <sudeep.holla@arm.com>
 L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)

-- 
2.50.0