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

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>

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