[PATCH 0/2] platform/x86: firmware_attributes_class: Provide a highlevel interface

[PATCH 0/2] platform/x86: firmware_attributes_class: Provide a highlevel interface

[Thomas Weißschuh]

Currently each user of firmware_attributes_class has to manually set up
kobjects, devices, etc.
Provide a higher level API which takes care of the low-level details.

Also provide a test driver for the new API.

Based upon pdx86/for-next and my firmware_attributes simplification
series[0].

[0] https://lore.kernel.org/lkml/20250104-firmware-attributes-simplify-v1-0-949f9709e405@weissschuh.net/

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
Thomas Weißschuh (2):
      platform/x86: firmware_attributes_class: Provide a highlevel interface
      platform/x86: firmware_attributes_class: Add test driver

 drivers/platform/x86/Kconfig                     |  12 ++
 drivers/platform/x86/Makefile                    |   1 +
 drivers/platform/x86/firmware_attributes_class.c | 146 +++++++++++++++++++++++
 drivers/platform/x86/firmware_attributes_class.h |  37 ++++++
 drivers/platform/x86/firmware_attributes_test.c  |  78 ++++++++++++
 5 files changed, 274 insertions(+)
---
base-commit: 25f05112995242b5a7ec1a91fbcb4dab66719af2
change-id: 20250107-pdx86-firmware-attributes-a5175a425fca

Best regards,
-- 
Thomas Weißschuh <linux@weissschuh.net>

[PATCH 1/2] platform/x86: firmware_attributes_class: Provide a highlevel interface

[Thomas Weißschuh]

Currently each user of firmware_attributes_class has to manually set up
kobjects, devices, etc.
Provide a higher level API which takes care of the low-level details.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/platform/x86/firmware_attributes_class.c | 146 +++++++++++++++++++++++
 drivers/platform/x86/firmware_attributes_class.h |  37 ++++++
 2 files changed, 183 insertions(+)

diff --git a/drivers/platform/x86/firmware_attributes_class.c b/drivers/platform/x86/firmware_attributes_class.c
index 736e96c186d9dc6d945517f090e9af903e93bbf4..70ceae5215820098b017bfda991a3c2a7824c98e 100644
--- a/drivers/platform/x86/firmware_attributes_class.c
+++ b/drivers/platform/x86/firmware_attributes_class.c
@@ -2,6 +2,9 @@
 
 /* Firmware attributes class helper module */
 
+#include <linux/device/class.h>
+#include <linux/device.h>
+#include <linux/kobject.h>
 #include <linux/module.h>
 #include "firmware_attributes_class.h"
 
@@ -22,6 +25,149 @@ static __exit void fw_attributes_class_exit(void)
 }
 module_exit(fw_attributes_class_exit);
 
+static ssize_t fw_attributes_sysfs_show(struct kobject *kobj, struct attribute *attr, char *buf)
+{
+	struct firmware_attributes_device *fwadev = to_firmware_attribute_device(kobj);
+	const struct firmware_attribute *fw_attr = to_firmware_attribute(attr);
+
+	if (!fw_attr->show)
+		return -EIO;
+
+	return fw_attr->show(fwadev, fw_attr, buf);
+}
+
+static ssize_t fw_attributes_sysfs_store(struct kobject *kobj, struct attribute *attr,
+					 const char *buf, size_t count)
+{
+	struct firmware_attributes_device *fwadev = to_firmware_attribute_device(kobj);
+	const struct firmware_attribute *fw_attr = to_firmware_attribute(attr);
+
+	if (!fw_attr->store)
+		return -EIO;
+
+	return fw_attr->store(fwadev, fw_attr, buf, count);
+}
+
+static const struct sysfs_ops fw_attributes_sysfs_ops = {
+	.show	= fw_attributes_sysfs_show,
+	.store	= fw_attributes_sysfs_store,
+};
+
+static void fw_attributes_attr_release(struct kobject *kobj)
+{
+	struct firmware_attributes_device *fwadev = to_firmware_attribute_device(kobj);
+	struct device *cdev;
+
+	cdev = fwadev->dev;
+
+	kfree(fwadev);
+	device_unregister(cdev);
+}
+
+static const struct kobj_type fw_attributes_attr_type = {
+	.sysfs_ops	= &fw_attributes_sysfs_ops,
+	.release	= fw_attributes_attr_release,
+};
+
+DEFINE_FREE(firmware_attributes_device_unregister, struct firmware_attributes_device *,
+	    if (_T) firmware_attributes_device_unregister(_T))
+
+struct firmware_attributes_device *
+firmware_attributes_device_register(struct device *parent, const char *name,
+				    const struct attribute_group **groups, void *data)
+{
+	struct firmware_attributes_device *fwadev = NULL;
+	struct device *cdev = NULL;
+	int ret;
+
+	fwadev = kzalloc(sizeof(*fwadev), GFP_KERNEL);
+	if (!fwadev)
+		return ERR_PTR(-ENOMEM);
+
+	cdev = device_create(&firmware_attributes_class, parent, MKDEV(0, 0), "%s", name);
+	if (IS_ERR(cdev))
+		return ERR_CAST(cdev);
+
+	fwadev->data = data;
+	fwadev->dev = cdev;
+
+	ret = kobject_init_and_add(&fwadev->attributes, &fw_attributes_attr_type, &cdev->kobj,
+				   "attributes");
+	if (ret) {
+		device_del(cdev);
+		return ERR_PTR(ret);
+	}
+
+	if (groups) {
+		ret = sysfs_create_groups(&fwadev->attributes, groups);
+		if (ret) {
+			firmware_attributes_device_unregister(fwadev);
+			return ERR_PTR(ret);
+		}
+
+		kobject_uevent(&fwadev->dev->kobj, KOBJ_CHANGE);
+	}
+
+	return fwadev;
+}
+EXPORT_SYMBOL_GPL(firmware_attributes_device_register);
+
+void firmware_attributes_device_unregister(struct firmware_attributes_device *fwadev)
+{
+	kobject_del(&fwadev->attributes);
+	kobject_put(&fwadev->attributes);
+}
+EXPORT_SYMBOL_GPL(firmware_attributes_device_unregister);
+
+static void devm_firmware_attributes_device_release(void *data)
+{
+	struct firmware_attributes_device *fwadev = data;
+
+	firmware_attributes_device_unregister(fwadev);
+}
+
+struct firmware_attributes_device *
+devm_firmware_attributes_device_register(struct device *parent, const char *name,
+					 const struct attribute_group **groups, void *data)
+{
+	struct firmware_attributes_device *fwadev;
+	int ret;
+
+	fwadev = firmware_attributes_device_register(parent, name, groups, data);
+	if (IS_ERR(fwadev))
+		return fwadev;
+
+	ret = devm_add_action_or_reset(parent, devm_firmware_attributes_device_release, fwadev);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return fwadev;
+}
+EXPORT_SYMBOL_GPL(devm_firmware_attributes_device_register);
+
+static ssize_t firmware_attributes_type_show(struct firmware_attributes_device *fwadev,
+					     const struct firmware_attribute *attr, char *buf)
+{
+	if (attr == &_firmware_attribute_type_string)
+		return sysfs_emit(buf, "string\n");
+	else if (attr == &_firmware_attribute_type_enumeration)
+		return sysfs_emit(buf, "enumeration\n");
+	else if (attr == &_firmware_attribute_type_integer)
+		return sysfs_emit(buf, "integer\n");
+	else
+		return -EIO;
+}
+
+#define __FW_TYPE_ATTR	__ATTR(type, 0444, firmware_attributes_type_show, NULL)
+
+const struct firmware_attribute _firmware_attribute_type_string = __FW_TYPE_ATTR;
+EXPORT_SYMBOL_GPL(_firmware_attribute_type_string);
+const struct firmware_attribute _firmware_attribute_type_enumeration = __FW_TYPE_ATTR;
+EXPORT_SYMBOL_GPL(_firmware_attribute_type_enumeration);
+const struct firmware_attribute _firmware_attribute_type_integer = __FW_TYPE_ATTR;
+EXPORT_SYMBOL_GPL(_firmware_attribute_type_integer);
+
 MODULE_AUTHOR("Mark Pearson <markpearson@lenovo.com>");
+MODULE_AUTHOR("Thomas Weißschuh <linux@weissschuh.net>");
 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..66837ad9f65b8ca501dee73f48c01f2710d86bf5 100644
--- a/drivers/platform/x86/firmware_attributes_class.h
+++ b/drivers/platform/x86/firmware_attributes_class.h
@@ -5,8 +5,45 @@
 #ifndef FW_ATTR_CLASS_H
 #define FW_ATTR_CLASS_H
 
+#include <linux/device.h>
 #include <linux/device/class.h>
+#include <linux/sysfs.h>
 
 extern const struct class firmware_attributes_class;
 
+struct firmware_attributes_device {
+	struct device *dev;
+	struct kobject attributes;
+	void *data;
+};
+
+struct firmware_attribute {
+	struct attribute attr;
+	ssize_t (*show)(struct firmware_attributes_device *fwadev,
+			const struct firmware_attribute *attr, char *buf);
+	ssize_t (*store)(struct firmware_attributes_device *fwadev,
+			 const struct firmware_attribute *attr, const char *buf, size_t count);
+};
+
+#define to_firmware_attribute(_a) container_of_const(_a, struct firmware_attribute, attr)
+#define to_firmware_attribute_device(_s) \
+	container_of_const(_s, struct firmware_attributes_device, attributes)
+
+extern const struct firmware_attribute _firmware_attribute_type_string;
+#define firmware_attribute_type_string ((struct attribute *)&_firmware_attribute_type_string.attr)
+extern const struct firmware_attribute _firmware_attribute_type_enumeration;
+#define firmware_attribute_type_enumeration ((struct attribute *)&_firmware_attribute_type_enumeration.attr)
+extern const struct firmware_attribute _firmware_attribute_type_integer;
+#define firmware_attribute_type_integer ((struct attribute *)&_firmware_attribute_type_integer.attr)
+
+struct firmware_attributes_device * __must_check
+firmware_attributes_device_register(struct device *parent, const char *name,
+				    const struct attribute_group **groups, void *data);
+
+void firmware_attributes_device_unregister(struct firmware_attributes_device *fwadev);
+
+struct firmware_attributes_device * __must_check
+devm_firmware_attributes_device_register(struct device *parent, const char *name,
+					 const struct attribute_group **groups, void *data);
+
 #endif /* FW_ATTR_CLASS_H */

-- 
2.47.1

Re: [PATCH 1/2] platform/x86: firmware_attributes_class: Provide a highlevel interface

[Kurt Borja]

On Tue Jan 7, 2025 at 2:05 PM -03, Thomas Weißschuh wrote:
> Currently each user of firmware_attributes_class has to manually set up
> kobjects, devices, etc.
> Provide a higher level API which takes care of the low-level details.
>
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>  drivers/platform/x86/firmware_attributes_class.c | 146 +++++++++++++++++++++++
>  drivers/platform/x86/firmware_attributes_class.h |  37 ++++++
>  2 files changed, 183 insertions(+)
>
> diff --git a/drivers/platform/x86/firmware_attributes_class.c b/drivers/platform/x86/firmware_attributes_class.c
> index 736e96c186d9dc6d945517f090e9af903e93bbf4..70ceae5215820098b017bfda991a3c2a7824c98e 100644
> --- a/drivers/platform/x86/firmware_attributes_class.c
> +++ b/drivers/platform/x86/firmware_attributes_class.c
> @@ -2,6 +2,9 @@
>  
>  /* Firmware attributes class helper module */
>  
> +#include <linux/device/class.h>
> +#include <linux/device.h>
> +#include <linux/kobject.h>
>  #include <linux/module.h>
>  #include "firmware_attributes_class.h"
>  
> @@ -22,6 +25,149 @@ static __exit void fw_attributes_class_exit(void)
>  }
>  module_exit(fw_attributes_class_exit);
>  
> +static ssize_t fw_attributes_sysfs_show(struct kobject *kobj, struct attribute *attr, char *buf)
> +{
> +	struct firmware_attributes_device *fwadev = to_firmware_attribute_device(kobj);
> +	const struct firmware_attribute *fw_attr = to_firmware_attribute(attr);
> +
> +	if (!fw_attr->show)
> +		return -EIO;
> +
> +	return fw_attr->show(fwadev, fw_attr, buf);
> +}
> +
> +static ssize_t fw_attributes_sysfs_store(struct kobject *kobj, struct attribute *attr,
> +					 const char *buf, size_t count)
> +{
> +	struct firmware_attributes_device *fwadev = to_firmware_attribute_device(kobj);
> +	const struct firmware_attribute *fw_attr = to_firmware_attribute(attr);
> +
> +	if (!fw_attr->store)
> +		return -EIO;
> +
> +	return fw_attr->store(fwadev, fw_attr, buf, count);
> +}
> +
> +static const struct sysfs_ops fw_attributes_sysfs_ops = {
> +	.show	= fw_attributes_sysfs_show,
> +	.store	= fw_attributes_sysfs_store,
> +};
> +
> +static void fw_attributes_attr_release(struct kobject *kobj)
> +{
> +	struct firmware_attributes_device *fwadev = to_firmware_attribute_device(kobj);
> +	struct device *cdev;
> +
> +	cdev = fwadev->dev;
> +
> +	kfree(fwadev);
> +	device_unregister(cdev);
> +}
> +
> +static const struct kobj_type fw_attributes_attr_type = {
> +	.sysfs_ops	= &fw_attributes_sysfs_ops,
> +	.release	= fw_attributes_attr_release,
> +};
> +
> +DEFINE_FREE(firmware_attributes_device_unregister, struct firmware_attributes_device *,
> +	    if (_T) firmware_attributes_device_unregister(_T))
> +
> +struct firmware_attributes_device *
> +firmware_attributes_device_register(struct device *parent, const char *name,
> +				    const struct attribute_group **groups, void *data)
> +{
> +	struct firmware_attributes_device *fwadev = NULL;
> +	struct device *cdev = NULL;
> +	int ret;
> +
> +	fwadev = kzalloc(sizeof(*fwadev), GFP_KERNEL);
> +	if (!fwadev)
> +		return ERR_PTR(-ENOMEM);
> +
> +	cdev = device_create(&firmware_attributes_class, parent, MKDEV(0, 0), "%s", name);
> +	if (IS_ERR(cdev))
> +		return ERR_CAST(cdev);
> +
> +	fwadev->data = data;
> +	fwadev->dev = cdev;
> +
> +	ret = kobject_init_and_add(&fwadev->attributes, &fw_attributes_attr_type, &cdev->kobj,
> +				   "attributes");
> +	if (ret) {
> +		device_del(cdev);
> +		return ERR_PTR(ret);
> +	}
> +
> +	if (groups) {
> +		ret = sysfs_create_groups(&fwadev->attributes, groups);
> +		if (ret) {
> +			firmware_attributes_device_unregister(fwadev);
> +			return ERR_PTR(ret);
> +		}
> +
> +		kobject_uevent(&fwadev->dev->kobj, KOBJ_CHANGE);
> +	}
> +
> +	return fwadev;
> +}
> +EXPORT_SYMBOL_GPL(firmware_attributes_device_register);
> +
> +void firmware_attributes_device_unregister(struct firmware_attributes_device *fwadev)
> +{
> +	kobject_del(&fwadev->attributes);
> +	kobject_put(&fwadev->attributes);
> +}
> +EXPORT_SYMBOL_GPL(firmware_attributes_device_unregister);
> +
> +static void devm_firmware_attributes_device_release(void *data)
> +{
> +	struct firmware_attributes_device *fwadev = data;
> +
> +	firmware_attributes_device_unregister(fwadev);
> +}
> +
> +struct firmware_attributes_device *
> +devm_firmware_attributes_device_register(struct device *parent, const char *name,
> +					 const struct attribute_group **groups, void *data)
> +{
> +	struct firmware_attributes_device *fwadev;
> +	int ret;
> +
> +	fwadev = firmware_attributes_device_register(parent, name, groups, data);
> +	if (IS_ERR(fwadev))
> +		return fwadev;
> +
> +	ret = devm_add_action_or_reset(parent, devm_firmware_attributes_device_release, fwadev);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	return fwadev;
> +}
> +EXPORT_SYMBOL_GPL(devm_firmware_attributes_device_register);
> +
> +static ssize_t firmware_attributes_type_show(struct firmware_attributes_device *fwadev,
> +					     const struct firmware_attribute *attr, char *buf)
> +{
> +	if (attr == &_firmware_attribute_type_string)
> +		return sysfs_emit(buf, "string\n");
> +	else if (attr == &_firmware_attribute_type_enumeration)
> +		return sysfs_emit(buf, "enumeration\n");
> +	else if (attr == &_firmware_attribute_type_integer)
> +		return sysfs_emit(buf, "integer\n");
> +	else
> +		return -EIO;
> +}
> +
> +#define __FW_TYPE_ATTR	__ATTR(type, 0444, firmware_attributes_type_show, NULL)
> +
> +const struct firmware_attribute _firmware_attribute_type_string = __FW_TYPE_ATTR;
> +EXPORT_SYMBOL_GPL(_firmware_attribute_type_string);
> +const struct firmware_attribute _firmware_attribute_type_enumeration = __FW_TYPE_ATTR;
> +EXPORT_SYMBOL_GPL(_firmware_attribute_type_enumeration);
> +const struct firmware_attribute _firmware_attribute_type_integer = __FW_TYPE_ATTR;
> +EXPORT_SYMBOL_GPL(_firmware_attribute_type_integer);
> +
>  MODULE_AUTHOR("Mark Pearson <markpearson@lenovo.com>");
> +MODULE_AUTHOR("Thomas Weißschuh <linux@weissschuh.net>");
>  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..66837ad9f65b8ca501dee73f48c01f2710d86bf5 100644
> --- a/drivers/platform/x86/firmware_attributes_class.h
> +++ b/drivers/platform/x86/firmware_attributes_class.h
> @@ -5,8 +5,45 @@
>  #ifndef FW_ATTR_CLASS_H
>  #define FW_ATTR_CLASS_H
>  
> +#include <linux/device.h>
>  #include <linux/device/class.h>
> +#include <linux/sysfs.h>
>  
>  extern const struct class firmware_attributes_class;
>  
> +struct firmware_attributes_device {
> +	struct device *dev;
> +	struct kobject attributes;
> +	void *data;
> +};
> +
> +struct firmware_attribute {
> +	struct attribute attr;
> +	ssize_t (*show)(struct firmware_attributes_device *fwadev,
> +			const struct firmware_attribute *attr, char *buf);
> +	ssize_t (*store)(struct firmware_attributes_device *fwadev,
> +			 const struct firmware_attribute *attr, const char *buf, size_t count);
> +};
> +
> +#define to_firmware_attribute(_a) container_of_const(_a, struct firmware_attribute, attr)
> +#define to_firmware_attribute_device(_s) \
> +	container_of_const(_s, struct firmware_attributes_device, attributes)
> +
> +extern const struct firmware_attribute _firmware_attribute_type_string;
> +#define firmware_attribute_type_string ((struct attribute *)&_firmware_attribute_type_string.attr)
> +extern const struct firmware_attribute _firmware_attribute_type_enumeration;
> +#define firmware_attribute_type_enumeration ((struct attribute *)&_firmware_attribute_type_enumeration.attr)
> +extern const struct firmware_attribute _firmware_attribute_type_integer;
> +#define firmware_attribute_type_integer ((struct attribute *)&_firmware_attribute_type_integer.attr)
> +
> +struct firmware_attributes_device * __must_check
> +firmware_attributes_device_register(struct device *parent, const char *name,
> +				    const struct attribute_group **groups, void *data);
> +
> +void firmware_attributes_device_unregister(struct firmware_attributes_device *fwadev);
> +
> +struct firmware_attributes_device * __must_check
> +devm_firmware_attributes_device_register(struct device *parent, const char *name,
> +					 const struct attribute_group **groups, void *data);
> +
>  #endif /* FW_ATTR_CLASS_H */

Hi Thomas,

Are you still working on this patchset?

If you don't mind I can take it from here. I'm planning on fixing a
couple memory leaks of this patch and extend it a bit more with some
helper macros for ABI compliant drivers. I might drop the test driver
though.

I ask this because I want to use this class in another series I'm
to do, and I think this patch is a great starting point.

Let me know what you think!

-- 
 ~ Kurt

Re: [PATCH 1/2] platform/x86: firmware_attributes_class: Provide a highlevel interface

[Thomas Weißschuh]

On 2025-04-23 05:05:35-0300, Kurt Borja wrote:
> On Tue Jan 7, 2025 at 2:05 PM -03, Thomas Weißschuh wrote:
> > Currently each user of firmware_attributes_class has to manually set up
> > kobjects, devices, etc.
> > Provide a higher level API which takes care of the low-level details.
> >
> > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > ---
> >  drivers/platform/x86/firmware_attributes_class.c | 146 +++++++++++++++++++++++
> >  drivers/platform/x86/firmware_attributes_class.h |  37 ++++++
> >  2 files changed, 183 insertions(+)
> >
> > diff --git a/drivers/platform/x86/firmware_attributes_class.c b/drivers/platform/x86/firmware_attributes_class.c
> > index 736e96c186d9dc6d945517f090e9af903e93bbf4..70ceae5215820098b017bfda991a3c2a7824c98e 100644
> > --- a/drivers/platform/x86/firmware_attributes_class.c
> > +++ b/drivers/platform/x86/firmware_attributes_class.c
> > @@ -2,6 +2,9 @@
> >  
> >  /* Firmware attributes class helper module */
> >  
> > +#include <linux/device/class.h>
> > +#include <linux/device.h>
> > +#include <linux/kobject.h>
> >  #include <linux/module.h>
> >  #include "firmware_attributes_class.h"
> >  
> > @@ -22,6 +25,149 @@ static __exit void fw_attributes_class_exit(void)
> >  }
> >  module_exit(fw_attributes_class_exit);
> >  
> > +static ssize_t fw_attributes_sysfs_show(struct kobject *kobj, struct attribute *attr, char *buf)
> > +{
> > +	struct firmware_attributes_device *fwadev = to_firmware_attribute_device(kobj);
> > +	const struct firmware_attribute *fw_attr = to_firmware_attribute(attr);
> > +
> > +	if (!fw_attr->show)
> > +		return -EIO;
> > +
> > +	return fw_attr->show(fwadev, fw_attr, buf);
> > +}
> > +
> > +static ssize_t fw_attributes_sysfs_store(struct kobject *kobj, struct attribute *attr,
> > +					 const char *buf, size_t count)
> > +{
> > +	struct firmware_attributes_device *fwadev = to_firmware_attribute_device(kobj);
> > +	const struct firmware_attribute *fw_attr = to_firmware_attribute(attr);
> > +
> > +	if (!fw_attr->store)
> > +		return -EIO;
> > +
> > +	return fw_attr->store(fwadev, fw_attr, buf, count);
> > +}
> > +
> > +static const struct sysfs_ops fw_attributes_sysfs_ops = {
> > +	.show	= fw_attributes_sysfs_show,
> > +	.store	= fw_attributes_sysfs_store,
> > +};
> > +
> > +static void fw_attributes_attr_release(struct kobject *kobj)
> > +{
> > +	struct firmware_attributes_device *fwadev = to_firmware_attribute_device(kobj);
> > +	struct device *cdev;
> > +
> > +	cdev = fwadev->dev;
> > +
> > +	kfree(fwadev);
> > +	device_unregister(cdev);
> > +}
> > +
> > +static const struct kobj_type fw_attributes_attr_type = {
> > +	.sysfs_ops	= &fw_attributes_sysfs_ops,
> > +	.release	= fw_attributes_attr_release,
> > +};
> > +
> > +DEFINE_FREE(firmware_attributes_device_unregister, struct firmware_attributes_device *,
> > +	    if (_T) firmware_attributes_device_unregister(_T))
> > +
> > +struct firmware_attributes_device *
> > +firmware_attributes_device_register(struct device *parent, const char *name,
> > +				    const struct attribute_group **groups, void *data)
> > +{
> > +	struct firmware_attributes_device *fwadev = NULL;
> > +	struct device *cdev = NULL;
> > +	int ret;
> > +
> > +	fwadev = kzalloc(sizeof(*fwadev), GFP_KERNEL);
> > +	if (!fwadev)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	cdev = device_create(&firmware_attributes_class, parent, MKDEV(0, 0), "%s", name);
> > +	if (IS_ERR(cdev))
> > +		return ERR_CAST(cdev);
> > +
> > +	fwadev->data = data;
> > +	fwadev->dev = cdev;
> > +
> > +	ret = kobject_init_and_add(&fwadev->attributes, &fw_attributes_attr_type, &cdev->kobj,
> > +				   "attributes");
> > +	if (ret) {
> > +		device_del(cdev);
> > +		return ERR_PTR(ret);
> > +	}
> > +
> > +	if (groups) {
> > +		ret = sysfs_create_groups(&fwadev->attributes, groups);
> > +		if (ret) {
> > +			firmware_attributes_device_unregister(fwadev);
> > +			return ERR_PTR(ret);
> > +		}
> > +
> > +		kobject_uevent(&fwadev->dev->kobj, KOBJ_CHANGE);
> > +	}
> > +
> > +	return fwadev;
> > +}
> > +EXPORT_SYMBOL_GPL(firmware_attributes_device_register);
> > +
> > +void firmware_attributes_device_unregister(struct firmware_attributes_device *fwadev)
> > +{
> > +	kobject_del(&fwadev->attributes);
> > +	kobject_put(&fwadev->attributes);
> > +}
> > +EXPORT_SYMBOL_GPL(firmware_attributes_device_unregister);
> > +
> > +static void devm_firmware_attributes_device_release(void *data)
> > +{
> > +	struct firmware_attributes_device *fwadev = data;
> > +
> > +	firmware_attributes_device_unregister(fwadev);
> > +}
> > +
> > +struct firmware_attributes_device *
> > +devm_firmware_attributes_device_register(struct device *parent, const char *name,
> > +					 const struct attribute_group **groups, void *data)
> > +{
> > +	struct firmware_attributes_device *fwadev;
> > +	int ret;
> > +
> > +	fwadev = firmware_attributes_device_register(parent, name, groups, data);
> > +	if (IS_ERR(fwadev))
> > +		return fwadev;
> > +
> > +	ret = devm_add_action_or_reset(parent, devm_firmware_attributes_device_release, fwadev);
> > +	if (ret)
> > +		return ERR_PTR(ret);
> > +
> > +	return fwadev;
> > +}
> > +EXPORT_SYMBOL_GPL(devm_firmware_attributes_device_register);
> > +
> > +static ssize_t firmware_attributes_type_show(struct firmware_attributes_device *fwadev,
> > +					     const struct firmware_attribute *attr, char *buf)
> > +{
> > +	if (attr == &_firmware_attribute_type_string)
> > +		return sysfs_emit(buf, "string\n");
> > +	else if (attr == &_firmware_attribute_type_enumeration)
> > +		return sysfs_emit(buf, "enumeration\n");
> > +	else if (attr == &_firmware_attribute_type_integer)
> > +		return sysfs_emit(buf, "integer\n");
> > +	else
> > +		return -EIO;
> > +}
> > +
> > +#define __FW_TYPE_ATTR	__ATTR(type, 0444, firmware_attributes_type_show, NULL)
> > +
> > +const struct firmware_attribute _firmware_attribute_type_string = __FW_TYPE_ATTR;
> > +EXPORT_SYMBOL_GPL(_firmware_attribute_type_string);
> > +const struct firmware_attribute _firmware_attribute_type_enumeration = __FW_TYPE_ATTR;
> > +EXPORT_SYMBOL_GPL(_firmware_attribute_type_enumeration);
> > +const struct firmware_attribute _firmware_attribute_type_integer = __FW_TYPE_ATTR;
> > +EXPORT_SYMBOL_GPL(_firmware_attribute_type_integer);
> > +
> >  MODULE_AUTHOR("Mark Pearson <markpearson@lenovo.com>");
> > +MODULE_AUTHOR("Thomas Weißschuh <linux@weissschuh.net>");
> >  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..66837ad9f65b8ca501dee73f48c01f2710d86bf5 100644
> > --- a/drivers/platform/x86/firmware_attributes_class.h
> > +++ b/drivers/platform/x86/firmware_attributes_class.h
> > @@ -5,8 +5,45 @@
> >  #ifndef FW_ATTR_CLASS_H
> >  #define FW_ATTR_CLASS_H
> >  
> > +#include <linux/device.h>
> >  #include <linux/device/class.h>
> > +#include <linux/sysfs.h>
> >  
> >  extern const struct class firmware_attributes_class;
> >  
> > +struct firmware_attributes_device {
> > +	struct device *dev;
> > +	struct kobject attributes;
> > +	void *data;
> > +};
> > +
> > +struct firmware_attribute {
> > +	struct attribute attr;
> > +	ssize_t (*show)(struct firmware_attributes_device *fwadev,
> > +			const struct firmware_attribute *attr, char *buf);
> > +	ssize_t (*store)(struct firmware_attributes_device *fwadev,
> > +			 const struct firmware_attribute *attr, const char *buf, size_t count);
> > +};
> > +
> > +#define to_firmware_attribute(_a) container_of_const(_a, struct firmware_attribute, attr)
> > +#define to_firmware_attribute_device(_s) \
> > +	container_of_const(_s, struct firmware_attributes_device, attributes)
> > +
> > +extern const struct firmware_attribute _firmware_attribute_type_string;
> > +#define firmware_attribute_type_string ((struct attribute *)&_firmware_attribute_type_string.attr)
> > +extern const struct firmware_attribute _firmware_attribute_type_enumeration;
> > +#define firmware_attribute_type_enumeration ((struct attribute *)&_firmware_attribute_type_enumeration.attr)
> > +extern const struct firmware_attribute _firmware_attribute_type_integer;
> > +#define firmware_attribute_type_integer ((struct attribute *)&_firmware_attribute_type_integer.attr)
> > +
> > +struct firmware_attributes_device * __must_check
> > +firmware_attributes_device_register(struct device *parent, const char *name,
> > +				    const struct attribute_group **groups, void *data);
> > +
> > +void firmware_attributes_device_unregister(struct firmware_attributes_device *fwadev);
> > +
> > +struct firmware_attributes_device * __must_check
> > +devm_firmware_attributes_device_register(struct device *parent, const char *name,
> > +					 const struct attribute_group **groups, void *data);
> > +
> >  #endif /* FW_ATTR_CLASS_H */
> 
> Hi Thomas,

Hi Kurt,

> Are you still working on this patchset?

I did some more work on this patchset, adding higher-level
functionality. But so far I was not satisfied with the result.

> If you don't mind I can take it from here. I'm planning on fixing a
> couple memory leaks of this patch and extend it a bit more with some
> helper macros for ABI compliant drivers. I might drop the test driver
> though.

You're welcome to take over.
 
> I ask this because I want to use this class in another series I'm
> to do, and I think this patch is a great starting point.

Nice to hear :-)

> Let me know what you think!

Please go ahead!


Thomas

[PATCH 2/2] platform/x86: firmware_attributes_class: Add test driver

[Thomas Weißschuh]

The driver showcases the use of the new subsystem API.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/platform/x86/Kconfig                    | 12 ++++
 drivers/platform/x86/Makefile                   |  1 +
 drivers/platform/x86/firmware_attributes_test.c | 78 +++++++++++++++++++++++++
 3 files changed, 91 insertions(+)

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 0258dd879d64be389f4dd9bc309fe089f23cc798..2a0e828657d2f07074944d6c42dc204fc8825a42 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -1065,6 +1065,18 @@ source "drivers/platform/x86/x86-android-tablets/Kconfig"
 config FW_ATTR_CLASS
 	tristate
 
+config FW_ATTR_TEST
+	tristate "Firmware attribute test driver"
+	select FW_ATTR_CLASS
+	help
+	  This driver provides a test user of the firmware attribute subsystem.
+
+	  An instance is created at /sys/class/firmware-attributes/test/
+	  container various example attributes.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called firmware_attributes_test.
+
 config INTEL_IMR
 	bool "Intel Isolated Memory Region support"
 	depends on X86_INTEL_QUARK && IOSF_MBI
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index e1b142947067475ee5472400a5a1cd20d79e12bd..610a1ca850a4353fd490e43b214a9e5872d2d28d 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -121,6 +121,7 @@ obj-$(CONFIG_TOPSTAR_LAPTOP)	+= topstar-laptop.o
 
 # Platform drivers
 obj-$(CONFIG_FW_ATTR_CLASS)		+= firmware_attributes_class.o
+obj-$(CONFIG_FW_ATTR_TEST)		+= firmware_attributes_test.o
 obj-$(CONFIG_SERIAL_MULTI_INSTANTIATE)	+= serial-multi-instantiate.o
 obj-$(CONFIG_MLX_PLATFORM)		+= mlx-platform.o
 obj-$(CONFIG_TOUCHSCREEN_DMI)		+= touchscreen_dmi.o
diff --git a/drivers/platform/x86/firmware_attributes_test.c b/drivers/platform/x86/firmware_attributes_test.c
new file mode 100644
index 0000000000000000000000000000000000000000..84f6a92e5163378c655f30ac022d513d7df5a18c
--- /dev/null
+++ b/drivers/platform/x86/firmware_attributes_test.c
@@ -0,0 +1,78 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/module.h>
+#include <linux/sysfs.h>
+#include "firmware_attributes_class.h"
+
+struct fw_attr_test_data {
+	char attr1_value[PAGE_SIZE];
+};
+
+static ssize_t test_attr1_default_value_show(struct firmware_attributes_device *fwadev,
+					     const struct firmware_attribute *attr, char *buf)
+{
+	return sysfs_emit(buf, "default 1\n");
+}
+
+static struct firmware_attribute test_attr1_default_value = __ATTR(default_value, 0444,
+								   test_attr1_default_value_show,
+								   NULL);
+
+static ssize_t test_attr1_current_value_show(struct firmware_attributes_device *fwadev,
+					     const struct firmware_attribute *attr, char *buf)
+{
+	struct fw_attr_test_data *data = fwadev->data;
+
+	return sysfs_emit(buf, "%s\n", data->attr1_value);
+}
+
+static ssize_t test_attr1_current_value_store(struct firmware_attributes_device *fwadev,
+					      const struct firmware_attribute *attr,
+					      const char *buf, size_t count)
+{
+	struct fw_attr_test_data *data = fwadev->data;
+
+	return strscpy(data->attr1_value, buf);
+}
+
+static struct firmware_attribute test_attr1_current_value = __ATTR(current_value, 0644,
+								   test_attr1_current_value_show,
+								   test_attr1_current_value_store);
+
+static struct attribute *test_attr1_attrs[] = {
+	firmware_attribute_type_string,
+	&test_attr1_default_value.attr,
+	&test_attr1_current_value.attr,
+	NULL
+};
+
+static const struct attribute_group test_attr1_group = {
+	.name	= "attr1",
+	.attrs	= test_attr1_attrs,
+};
+
+static const struct attribute_group *test_attr_groups[] = {
+	&test_attr1_group,
+	NULL
+};
+
+static struct firmware_attributes_device *fwadev;
+
+static int __init fw_test_init(void)
+{
+	static struct fw_attr_test_data data = {
+		.attr1_value = "attr1",
+	};
+
+	fwadev = firmware_attributes_device_register(NULL, "test", test_attr_groups, &data);
+	return PTR_ERR_OR_ZERO(fwadev);
+}
+module_init(fw_test_init);
+
+static void __exit fw_test_exit(void)
+{
+	firmware_attributes_device_unregister(fwadev);
+}
+module_exit(fw_test_exit);
+
+MODULE_LICENSE("GPL");

-- 
2.47.1

Re: [PATCH 2/2] platform/x86: firmware_attributes_class: Add test driver

[Mario Limonciello]

On 1/7/2025 11:05, Thomas Weißschuh wrote:
> The driver showcases the use of the new subsystem API.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>   drivers/platform/x86/Kconfig                    | 12 ++++
>   drivers/platform/x86/Makefile                   |  1 +
>   drivers/platform/x86/firmware_attributes_test.c | 78 +++++++++++++++++++++++++
>   3 files changed, 91 insertions(+)
> 
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 0258dd879d64be389f4dd9bc309fe089f23cc798..2a0e828657d2f07074944d6c42dc204fc8825a42 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -1065,6 +1065,18 @@ source "drivers/platform/x86/x86-android-tablets/Kconfig"
>   config FW_ATTR_CLASS
>   	tristate
>   
> +config FW_ATTR_TEST
> +	tristate "Firmware attribute test driver"
> +	select FW_ATTR_CLASS
> +	help
> +	  This driver provides a test user of the firmware attribute subsystem.
> +
> +	  An instance is created at /sys/class/firmware-attributes/test/
> +	  container various example attributes.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called firmware_attributes_test.
> +

I think if you're going to be introducing a test driver it should be 
compliant to what's in sysfs-class-firmware-attributes so that when it's 
inevitably copy/pasted we end up with higher quality drivers.

That is you need at a minimum 'type', 'current_value', 'default_value', 
'display_name' and 'display_name_language_code'.  Then individual types 
employ additional requirements.

I see 'type', 'current_value', and 'default_value, but I don't see 
'display_name' or 'display_name_language_code' here.

Furthermore as this is a "string" attribute you're supposed to also have 
a "max_length" and "min_length".

>   config INTEL_IMR
>   	bool "Intel Isolated Memory Region support"
>   	depends on X86_INTEL_QUARK && IOSF_MBI
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index e1b142947067475ee5472400a5a1cd20d79e12bd..610a1ca850a4353fd490e43b214a9e5872d2d28d 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -121,6 +121,7 @@ obj-$(CONFIG_TOPSTAR_LAPTOP)	+= topstar-laptop.o
>   
>   # Platform drivers
>   obj-$(CONFIG_FW_ATTR_CLASS)		+= firmware_attributes_class.o
> +obj-$(CONFIG_FW_ATTR_TEST)		+= firmware_attributes_test.o
>   obj-$(CONFIG_SERIAL_MULTI_INSTANTIATE)	+= serial-multi-instantiate.o
>   obj-$(CONFIG_MLX_PLATFORM)		+= mlx-platform.o
>   obj-$(CONFIG_TOUCHSCREEN_DMI)		+= touchscreen_dmi.o
> diff --git a/drivers/platform/x86/firmware_attributes_test.c b/drivers/platform/x86/firmware_attributes_test.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..84f6a92e5163378c655f30ac022d513d7df5a18c
> --- /dev/null
> +++ b/drivers/platform/x86/firmware_attributes_test.c
> @@ -0,0 +1,78 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <linux/module.h>
> +#include <linux/sysfs.h>
> +#include "firmware_attributes_class.h"
> +
> +struct fw_attr_test_data {
> +	char attr1_value[PAGE_SIZE];
> +};
> +
> +static ssize_t test_attr1_default_value_show(struct firmware_attributes_device *fwadev,
> +					     const struct firmware_attribute *attr, char *buf)
> +{
> +	return sysfs_emit(buf, "default 1\n");
> +}
> +
> +static struct firmware_attribute test_attr1_default_value = __ATTR(default_value, 0444,
> +								   test_attr1_default_value_show,
> +								   NULL);
> +
> +static ssize_t test_attr1_current_value_show(struct firmware_attributes_device *fwadev,
> +					     const struct firmware_attribute *attr, char *buf)
> +{
> +	struct fw_attr_test_data *data = fwadev->data;
> +
> +	return sysfs_emit(buf, "%s\n", data->attr1_value);
> +}
> +
> +static ssize_t test_attr1_current_value_store(struct firmware_attributes_device *fwadev,
> +					      const struct firmware_attribute *attr,
> +					      const char *buf, size_t count)
> +{
> +	struct fw_attr_test_data *data = fwadev->data;
> +
> +	return strscpy(data->attr1_value, buf);
> +}
> +
> +static struct firmware_attribute test_attr1_current_value = __ATTR(current_value, 0644,
> +								   test_attr1_current_value_show,
> +								   test_attr1_current_value_store);
> +
> +static struct attribute *test_attr1_attrs[] = {
> +	firmware_attribute_type_string,
> +	&test_attr1_default_value.attr,
> +	&test_attr1_current_value.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group test_attr1_group = {
> +	.name	= "attr1",
> +	.attrs	= test_attr1_attrs,
> +};
> +
> +static const struct attribute_group *test_attr_groups[] = {
> +	&test_attr1_group,
> +	NULL
> +};
> +
> +static struct firmware_attributes_device *fwadev;
> +
> +static int __init fw_test_init(void)
> +{
> +	static struct fw_attr_test_data data = {
> +		.attr1_value = "attr1",
> +	};
> +
> +	fwadev = firmware_attributes_device_register(NULL, "test", test_attr_groups, &data);
> +	return PTR_ERR_OR_ZERO(fwadev);
> +}
> +module_init(fw_test_init);
> +
> +static void __exit fw_test_exit(void)
> +{
> +	firmware_attributes_device_unregister(fwadev);
> +}
> +module_exit(fw_test_exit);
> +
> +MODULE_LICENSE("GPL");
> 

Re: [PATCH 2/2] platform/x86: firmware_attributes_class: Add test driver

[Thomas Weißschuh]

On 2025-01-07 13:29:08-0600, Mario Limonciello wrote:
> On 1/7/2025 11:05, Thomas Weißschuh wrote:
> > The driver showcases the use of the new subsystem API.
> > 
> > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > ---
> >   drivers/platform/x86/Kconfig                    | 12 ++++
> >   drivers/platform/x86/Makefile                   |  1 +
> >   drivers/platform/x86/firmware_attributes_test.c | 78 +++++++++++++++++++++++++
> >   3 files changed, 91 insertions(+)
> > 
> > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> > index 0258dd879d64be389f4dd9bc309fe089f23cc798..2a0e828657d2f07074944d6c42dc204fc8825a42 100644
> > --- a/drivers/platform/x86/Kconfig
> > +++ b/drivers/platform/x86/Kconfig
> > @@ -1065,6 +1065,18 @@ source "drivers/platform/x86/x86-android-tablets/Kconfig"
> >   config FW_ATTR_CLASS
> >   	tristate
> > +config FW_ATTR_TEST
> > +	tristate "Firmware attribute test driver"
> > +	select FW_ATTR_CLASS
> > +	help
> > +	  This driver provides a test user of the firmware attribute subsystem.
> > +
> > +	  An instance is created at /sys/class/firmware-attributes/test/
> > +	  container various example attributes.
> > +
> > +	  To compile this driver as a module, choose M here: the module
> > +	  will be called firmware_attributes_test.
> > +
> 
> I think if you're going to be introducing a test driver it should be
> compliant to what's in sysfs-class-firmware-attributes so that when it's
> inevitably copy/pasted we end up with higher quality drivers.
> 
> That is you need at a minimum 'type', 'current_value', 'default_value',
> 'display_name' and 'display_name_language_code'.  Then individual types
> employ additional requirements.
> 
> I see 'type', 'current_value', and 'default_value, but I don't see
> 'display_name' or 'display_name_language_code' here.
> 
> Furthermore as this is a "string" attribute you're supposed to also have a
> "max_length" and "min_length".

Agreed that more examples are better.

But are these attributes really mandatory?
"This attribute is mandatory" is only specified for "type" and
"current_value".
While "possible_values" certainly looks necessary for "enumeration",
"min_length" for "strings" does so much less.


Thomas

Re: [PATCH 2/2] platform/x86: firmware_attributes_class: Add test driver

[Mario Limonciello]

On 1/7/2025 14:50, Thomas Weißschuh wrote:
> On 2025-01-07 13:29:08-0600, Mario Limonciello wrote:
>> On 1/7/2025 11:05, Thomas Weißschuh wrote:
>>> The driver showcases the use of the new subsystem API.
>>>
>>> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
>>> ---
>>>    drivers/platform/x86/Kconfig                    | 12 ++++
>>>    drivers/platform/x86/Makefile                   |  1 +
>>>    drivers/platform/x86/firmware_attributes_test.c | 78 +++++++++++++++++++++++++
>>>    3 files changed, 91 insertions(+)
>>>
>>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
>>> index 0258dd879d64be389f4dd9bc309fe089f23cc798..2a0e828657d2f07074944d6c42dc204fc8825a42 100644
>>> --- a/drivers/platform/x86/Kconfig
>>> +++ b/drivers/platform/x86/Kconfig
>>> @@ -1065,6 +1065,18 @@ source "drivers/platform/x86/x86-android-tablets/Kconfig"
>>>    config FW_ATTR_CLASS
>>>    	tristate
>>> +config FW_ATTR_TEST
>>> +	tristate "Firmware attribute test driver"
>>> +	select FW_ATTR_CLASS
>>> +	help
>>> +	  This driver provides a test user of the firmware attribute subsystem.
>>> +
>>> +	  An instance is created at /sys/class/firmware-attributes/test/
>>> +	  container various example attributes.
>>> +
>>> +	  To compile this driver as a module, choose M here: the module
>>> +	  will be called firmware_attributes_test.
>>> +
>>
>> I think if you're going to be introducing a test driver it should be
>> compliant to what's in sysfs-class-firmware-attributes so that when it's
>> inevitably copy/pasted we end up with higher quality drivers.
>>
>> That is you need at a minimum 'type', 'current_value', 'default_value',
>> 'display_name' and 'display_name_language_code'.  Then individual types
>> employ additional requirements.
>>
>> I see 'type', 'current_value', and 'default_value, but I don't see
>> 'display_name' or 'display_name_language_code' here.
>>
>> Furthermore as this is a "string" attribute you're supposed to also have a
>> "max_length" and "min_length".
> 
> Agreed that more examples are better.
> 
> But are these attributes really mandatory?
 > "This attribute is mandatory" is only specified for "type" and> 
"current_value".

Ah wow, I had thought they were, but you're right they're not!

> While "possible_values" certainly looks necessary for "enumeration",
> "min_length" for "strings" does so much less.

Even if they're not mandatory, I think it's better to include them for 
the same copy/paste reason I mentioned though.

Re: [PATCH 2/2] platform/x86: firmware_attributes_class: Add test driver

[Thomas Weißschuh]

On 2025-01-07 15:18:21-0600, Mario Limonciello wrote:
> On 1/7/2025 14:50, Thomas Weißschuh wrote:
> > On 2025-01-07 13:29:08-0600, Mario Limonciello wrote:
> > > On 1/7/2025 11:05, Thomas Weißschuh wrote:
> > > > The driver showcases the use of the new subsystem API.
> > > > 
> > > > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > > > ---
> > > >    drivers/platform/x86/Kconfig                    | 12 ++++
> > > >    drivers/platform/x86/Makefile                   |  1 +
> > > >    drivers/platform/x86/firmware_attributes_test.c | 78 +++++++++++++++++++++++++
> > > >    3 files changed, 91 insertions(+)
> > > > 
> > > > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> > > > index 0258dd879d64be389f4dd9bc309fe089f23cc798..2a0e828657d2f07074944d6c42dc204fc8825a42 100644
> > > > --- a/drivers/platform/x86/Kconfig
> > > > +++ b/drivers/platform/x86/Kconfig
> > > > @@ -1065,6 +1065,18 @@ source "drivers/platform/x86/x86-android-tablets/Kconfig"
> > > >    config FW_ATTR_CLASS
> > > >    	tristate
> > > > +config FW_ATTR_TEST
> > > > +	tristate "Firmware attribute test driver"
> > > > +	select FW_ATTR_CLASS
> > > > +	help
> > > > +	  This driver provides a test user of the firmware attribute subsystem.
> > > > +
> > > > +	  An instance is created at /sys/class/firmware-attributes/test/
> > > > +	  container various example attributes.
> > > > +
> > > > +	  To compile this driver as a module, choose M here: the module
> > > > +	  will be called firmware_attributes_test.
> > > > +
> > > 
> > > I think if you're going to be introducing a test driver it should be
> > > compliant to what's in sysfs-class-firmware-attributes so that when it's
> > > inevitably copy/pasted we end up with higher quality drivers.
> > > 
> > > That is you need at a minimum 'type', 'current_value', 'default_value',
> > > 'display_name' and 'display_name_language_code'.  Then individual types
> > > employ additional requirements.
> > > 
> > > I see 'type', 'current_value', and 'default_value, but I don't see
> > > 'display_name' or 'display_name_language_code' here.
> > > 
> > > Furthermore as this is a "string" attribute you're supposed to also have a
> > > "max_length" and "min_length".
> > 
> > Agreed that more examples are better.
> > 
> > But are these attributes really mandatory?
> > "This attribute is mandatory" is only specified for "type" and>
> "current_value".
> 
> Ah wow, I had thought they were, but you're right they're not!
> 
> > While "possible_values" certainly looks necessary for "enumeration",
> > "min_length" for "strings" does so much less.
> 
> Even if they're not mandatory, I think it's better to include them for the
> same copy/paste reason I mentioned though.

Thinking about it some more, which attributes should all be included?
Having all of them in there could motivate driver authors to implement
them all even it would mean filling in random values.
The provided examples can already be copied-and-pasted and slightly
adapted to add more attributes.

Also as for providing an even higher level interface. There exists a
fairly big feature matrix. For example the display_name_language_code:
* Is it used at all?
* Is it the same for all attributes implemented by the driver and the
  sysfs attribute can be reused for them all.
* Should the same handler logic be reused between different settings which
  only differ in their language code?

The answers seem to differ between each driver and having a uniform
high-level interface that can handle all cases would look horrible.
So I'd like to stick with the API provided currently for now and we can
revisit it if there are more drivers.
As mentioned before, the current API should be a decent baseline to
build on top of.


Thomas

Re: [PATCH 2/2] platform/x86: firmware_attributes_class: Add test driver

[Mario Limonciello]

On 1/7/2025 16:13, Thomas Weißschuh wrote:
> On 2025-01-07 15:18:21-0600, Mario Limonciello wrote:
>> On 1/7/2025 14:50, Thomas Weißschuh wrote:
>>> On 2025-01-07 13:29:08-0600, Mario Limonciello wrote:
>>>> On 1/7/2025 11:05, Thomas Weißschuh wrote:
>>>>> The driver showcases the use of the new subsystem API.
>>>>>
>>>>> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
>>>>> ---
>>>>>     drivers/platform/x86/Kconfig                    | 12 ++++
>>>>>     drivers/platform/x86/Makefile                   |  1 +
>>>>>     drivers/platform/x86/firmware_attributes_test.c | 78 +++++++++++++++++++++++++
>>>>>     3 files changed, 91 insertions(+)
>>>>>
>>>>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
>>>>> index 0258dd879d64be389f4dd9bc309fe089f23cc798..2a0e828657d2f07074944d6c42dc204fc8825a42 100644
>>>>> --- a/drivers/platform/x86/Kconfig
>>>>> +++ b/drivers/platform/x86/Kconfig
>>>>> @@ -1065,6 +1065,18 @@ source "drivers/platform/x86/x86-android-tablets/Kconfig"
>>>>>     config FW_ATTR_CLASS
>>>>>     	tristate
>>>>> +config FW_ATTR_TEST
>>>>> +	tristate "Firmware attribute test driver"
>>>>> +	select FW_ATTR_CLASS
>>>>> +	help
>>>>> +	  This driver provides a test user of the firmware attribute subsystem.
>>>>> +
>>>>> +	  An instance is created at /sys/class/firmware-attributes/test/
>>>>> +	  container various example attributes.
>>>>> +
>>>>> +	  To compile this driver as a module, choose M here: the module
>>>>> +	  will be called firmware_attributes_test.
>>>>> +
>>>>
>>>> I think if you're going to be introducing a test driver it should be
>>>> compliant to what's in sysfs-class-firmware-attributes so that when it's
>>>> inevitably copy/pasted we end up with higher quality drivers.
>>>>
>>>> That is you need at a minimum 'type', 'current_value', 'default_value',
>>>> 'display_name' and 'display_name_language_code'.  Then individual types
>>>> employ additional requirements.
>>>>
>>>> I see 'type', 'current_value', and 'default_value, but I don't see
>>>> 'display_name' or 'display_name_language_code' here.
>>>>
>>>> Furthermore as this is a "string" attribute you're supposed to also have a
>>>> "max_length" and "min_length".
>>>
>>> Agreed that more examples are better.
>>>
>>> But are these attributes really mandatory?
>>> "This attribute is mandatory" is only specified for "type" and>
>> "current_value".
>>
>> Ah wow, I had thought they were, but you're right they're not!
>>
>>> While "possible_values" certainly looks necessary for "enumeration",
>>> "min_length" for "strings" does so much less.
>>
>> Even if they're not mandatory, I think it's better to include them for the
>> same copy/paste reason I mentioned though.
> 
> Thinking about it some more, which attributes should all be included?
> Having all of them in there could motivate driver authors to implement
> them all even it would mean filling in random values.
> The provided examples can already be copied-and-pasted and slightly
> adapted to add more attributes.
> 
> Also as for providing an even higher level interface. There exists a
> fairly big feature matrix. For example the display_name_language_code:
> * Is it used at all?
> * Is it the same for all attributes implemented by the driver and the
>    sysfs attribute can be reused for them all.
> * Should the same handler logic be reused between different settings which
>    only differ in their language code?
> 
> The answers seem to differ between each driver and having a uniform
> high-level interface that can handle all cases would look horrible.
> So I'd like to stick with the API provided currently for now and we can
> revisit it if there are more drivers.
> As mentioned before, the current API should be a decent baseline to
> build on top of.
> 
> 
> Thomas

How about just adding min_length and max_length?  Since you're adding a 
string attribute then it makes a good example of a string attribute.
It should be pretty trivial; AFAICT it's 0 to PAGE_SIZE.

Re: [PATCH 2/2] platform/x86: firmware_attributes_class: Add test driver

[Thomas Weißschuh]

On 2025-01-07 16:20:14-0600, Mario Limonciello wrote:
> On 1/7/2025 16:13, Thomas Weißschuh wrote:
> > On 2025-01-07 15:18:21-0600, Mario Limonciello wrote:
> > > On 1/7/2025 14:50, Thomas Weißschuh wrote:
> > > > On 2025-01-07 13:29:08-0600, Mario Limonciello wrote:
> > > > > On 1/7/2025 11:05, Thomas Weißschuh wrote:
> > > > > > The driver showcases the use of the new subsystem API.
> > > > > > 
> > > > > > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > > > > > ---
> > > > > >     drivers/platform/x86/Kconfig                    | 12 ++++
> > > > > >     drivers/platform/x86/Makefile                   |  1 +
> > > > > >     drivers/platform/x86/firmware_attributes_test.c | 78 +++++++++++++++++++++++++
> > > > > >     3 files changed, 91 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> > > > > > index 0258dd879d64be389f4dd9bc309fe089f23cc798..2a0e828657d2f07074944d6c42dc204fc8825a42 100644
> > > > > > --- a/drivers/platform/x86/Kconfig
> > > > > > +++ b/drivers/platform/x86/Kconfig
> > > > > > @@ -1065,6 +1065,18 @@ source "drivers/platform/x86/x86-android-tablets/Kconfig"
> > > > > >     config FW_ATTR_CLASS
> > > > > >     	tristate
> > > > > > +config FW_ATTR_TEST
> > > > > > +	tristate "Firmware attribute test driver"
> > > > > > +	select FW_ATTR_CLASS
> > > > > > +	help
> > > > > > +	  This driver provides a test user of the firmware attribute subsystem.
> > > > > > +
> > > > > > +	  An instance is created at /sys/class/firmware-attributes/test/
> > > > > > +	  container various example attributes.
> > > > > > +
> > > > > > +	  To compile this driver as a module, choose M here: the module
> > > > > > +	  will be called firmware_attributes_test.
> > > > > > +
> > > > > 
> > > > > I think if you're going to be introducing a test driver it should be
> > > > > compliant to what's in sysfs-class-firmware-attributes so that when it's
> > > > > inevitably copy/pasted we end up with higher quality drivers.
> > > > > 
> > > > > That is you need at a minimum 'type', 'current_value', 'default_value',
> > > > > 'display_name' and 'display_name_language_code'.  Then individual types
> > > > > employ additional requirements.
> > > > > 
> > > > > I see 'type', 'current_value', and 'default_value, but I don't see
> > > > > 'display_name' or 'display_name_language_code' here.
> > > > > 
> > > > > Furthermore as this is a "string" attribute you're supposed to also have a
> > > > > "max_length" and "min_length".
> > > > 
> > > > Agreed that more examples are better.
> > > > 
> > > > But are these attributes really mandatory?
> > > > "This attribute is mandatory" is only specified for "type" and>
> > > "current_value".
> > > 
> > > Ah wow, I had thought they were, but you're right they're not!
> > > 
> > > > While "possible_values" certainly looks necessary for "enumeration",
> > > > "min_length" for "strings" does so much less.
> > > 
> > > Even if they're not mandatory, I think it's better to include them for the
> > > same copy/paste reason I mentioned though.
> > 
> > Thinking about it some more, which attributes should all be included?
> > Having all of them in there could motivate driver authors to implement
> > them all even it would mean filling in random values.
> > The provided examples can already be copied-and-pasted and slightly
> > adapted to add more attributes.
> > 
> > Also as for providing an even higher level interface. There exists a
> > fairly big feature matrix. For example the display_name_language_code:
> > * Is it used at all?
> > * Is it the same for all attributes implemented by the driver and the
> >    sysfs attribute can be reused for them all.
> > * Should the same handler logic be reused between different settings which
> >    only differ in their language code?
> > 
> > The answers seem to differ between each driver and having a uniform
> > high-level interface that can handle all cases would look horrible.
> > So I'd like to stick with the API provided currently for now and we can
> > revisit it if there are more drivers.
> > As mentioned before, the current API should be a decent baseline to
> > build on top of.
> > 
> > 
> > Thomas
> 
> How about just adding min_length and max_length?  Since you're adding a
> string attribute then it makes a good example of a string attribute.
> It should be pretty trivial; AFAICT it's 0 to PAGE_SIZE.

IMO attributes should only exist if they provide useful, driver-specific
information. The proposed values are exactly what I'd like to avoid being
copied around.
min_length=0 does not add any information.
max_length=PAGE_SIZE exposes an implementation detail of sysfs. If that
limit should become an issue for a driver they can switch to a bin_attribute.

I would prefer using get_random_u32() for both attributes, so it's clear
that they are only placeholders.

But I'm also open to getting outvoted.

Re: [PATCH 2/2] platform/x86: firmware_attributes_class: Add test driver

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 4404 bytes --]

On Tue, 7 Jan 2025, Thomas Weißschuh wrote:

> On 2025-01-07 15:18:21-0600, Mario Limonciello wrote:
> > On 1/7/2025 14:50, Thomas Weißschuh wrote:
> > > On 2025-01-07 13:29:08-0600, Mario Limonciello wrote:
> > > > On 1/7/2025 11:05, Thomas Weißschuh wrote:
> > > > > The driver showcases the use of the new subsystem API.
> > > > > 
> > > > > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > > > > ---
> > > > >    drivers/platform/x86/Kconfig                    | 12 ++++
> > > > >    drivers/platform/x86/Makefile                   |  1 +
> > > > >    drivers/platform/x86/firmware_attributes_test.c | 78 +++++++++++++++++++++++++
> > > > >    3 files changed, 91 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> > > > > index 0258dd879d64be389f4dd9bc309fe089f23cc798..2a0e828657d2f07074944d6c42dc204fc8825a42 100644
> > > > > --- a/drivers/platform/x86/Kconfig
> > > > > +++ b/drivers/platform/x86/Kconfig
> > > > > @@ -1065,6 +1065,18 @@ source "drivers/platform/x86/x86-android-tablets/Kconfig"
> > > > >    config FW_ATTR_CLASS
> > > > >    	tristate
> > > > > +config FW_ATTR_TEST
> > > > > +	tristate "Firmware attribute test driver"
> > > > > +	select FW_ATTR_CLASS
> > > > > +	help
> > > > > +	  This driver provides a test user of the firmware attribute subsystem.
> > > > > +
> > > > > +	  An instance is created at /sys/class/firmware-attributes/test/
> > > > > +	  container various example attributes.
> > > > > +
> > > > > +	  To compile this driver as a module, choose M here: the module
> > > > > +	  will be called firmware_attributes_test.
> > > > > +
> > > > 
> > > > I think if you're going to be introducing a test driver it should be
> > > > compliant to what's in sysfs-class-firmware-attributes so that when it's
> > > > inevitably copy/pasted we end up with higher quality drivers.
> > > > 
> > > > That is you need at a minimum 'type', 'current_value', 'default_value',
> > > > 'display_name' and 'display_name_language_code'.  Then individual types
> > > > employ additional requirements.
> > > > 
> > > > I see 'type', 'current_value', and 'default_value, but I don't see
> > > > 'display_name' or 'display_name_language_code' here.
> > > > 
> > > > Furthermore as this is a "string" attribute you're supposed to also have a
> > > > "max_length" and "min_length".
> > > 
> > > Agreed that more examples are better.
> > > 
> > > But are these attributes really mandatory?
> > > "This attribute is mandatory" is only specified for "type" and>
> > "current_value".
> > 
> > Ah wow, I had thought they were, but you're right they're not!
> > 
> > > While "possible_values" certainly looks necessary for "enumeration",
> > > "min_length" for "strings" does so much less.
> > 
> > Even if they're not mandatory, I think it's better to include them for the
> > same copy/paste reason I mentioned though.
> 
> Thinking about it some more, which attributes should all be included?
> Having all of them in there could motivate driver authors to implement
> them all even it would mean filling in random values.
> The provided examples can already be copied-and-pasted and slightly
> adapted to add more attributes.

Hi,

Can't you like add comments to the optional ones to reduce the incentive 
to fill them with random junk as it's a lot easier to just delete them than 
generating some random junk. So if a developer is unsure what to do a 
comment telling something is optional would help to lean towards 'I can 
safely delete this'?

-- 
 i.

> Also as for providing an even higher level interface. There exists a
> fairly big feature matrix. For example the display_name_language_code:
> * Is it used at all?
> * Is it the same for all attributes implemented by the driver and the
>   sysfs attribute can be reused for them all.
> * Should the same handler logic be reused between different settings which
>   only differ in their language code?
> 
> The answers seem to differ between each driver and having a uniform
> high-level interface that can handle all cases would look horrible.
> So I'd like to stick with the API provided currently for now and we can
> revisit it if there are more drivers.
> As mentioned before, the current API should be a decent baseline to
> build on top of.
> 
> 
> Thomas
> 

Re: [PATCH 2/2] platform/x86: firmware_attributes_class: Add test driver

[Thomas Weißschuh]

On 2025-01-08 11:30:12+0200, Ilpo Järvinen wrote:
> On Tue, 7 Jan 2025, Thomas Weißschuh wrote:
> 
> > On 2025-01-07 15:18:21-0600, Mario Limonciello wrote:
> > > On 1/7/2025 14:50, Thomas Weißschuh wrote:
> > > > On 2025-01-07 13:29:08-0600, Mario Limonciello wrote:
> > > > > On 1/7/2025 11:05, Thomas Weißschuh wrote:
> > > > > > The driver showcases the use of the new subsystem API.
> > > > > > 
> > > > > > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > > > > > ---
> > > > > >    drivers/platform/x86/Kconfig                    | 12 ++++
> > > > > >    drivers/platform/x86/Makefile                   |  1 +
> > > > > >    drivers/platform/x86/firmware_attributes_test.c | 78 +++++++++++++++++++++++++
> > > > > >    3 files changed, 91 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> > > > > > index 0258dd879d64be389f4dd9bc309fe089f23cc798..2a0e828657d2f07074944d6c42dc204fc8825a42 100644
> > > > > > --- a/drivers/platform/x86/Kconfig
> > > > > > +++ b/drivers/platform/x86/Kconfig
> > > > > > @@ -1065,6 +1065,18 @@ source "drivers/platform/x86/x86-android-tablets/Kconfig"
> > > > > >    config FW_ATTR_CLASS
> > > > > >    	tristate
> > > > > > +config FW_ATTR_TEST
> > > > > > +	tristate "Firmware attribute test driver"
> > > > > > +	select FW_ATTR_CLASS
> > > > > > +	help
> > > > > > +	  This driver provides a test user of the firmware attribute subsystem.
> > > > > > +
> > > > > > +	  An instance is created at /sys/class/firmware-attributes/test/
> > > > > > +	  container various example attributes.
> > > > > > +
> > > > > > +	  To compile this driver as a module, choose M here: the module
> > > > > > +	  will be called firmware_attributes_test.
> > > > > > +
> > > > > 
> > > > > I think if you're going to be introducing a test driver it should be
> > > > > compliant to what's in sysfs-class-firmware-attributes so that when it's
> > > > > inevitably copy/pasted we end up with higher quality drivers.
> > > > > 
> > > > > That is you need at a minimum 'type', 'current_value', 'default_value',
> > > > > 'display_name' and 'display_name_language_code'.  Then individual types
> > > > > employ additional requirements.
> > > > > 
> > > > > I see 'type', 'current_value', and 'default_value, but I don't see
> > > > > 'display_name' or 'display_name_language_code' here.
> > > > > 
> > > > > Furthermore as this is a "string" attribute you're supposed to also have a
> > > > > "max_length" and "min_length".
> > > > 
> > > > Agreed that more examples are better.
> > > > 
> > > > But are these attributes really mandatory?
> > > > "This attribute is mandatory" is only specified for "type" and>
> > > "current_value".
> > > 
> > > Ah wow, I had thought they were, but you're right they're not!
> > > 
> > > > While "possible_values" certainly looks necessary for "enumeration",
> > > > "min_length" for "strings" does so much less.
> > > 
> > > Even if they're not mandatory, I think it's better to include them for the
> > > same copy/paste reason I mentioned though.
> > 
> > Thinking about it some more, which attributes should all be included?
> > Having all of them in there could motivate driver authors to implement
> > them all even it would mean filling in random values.
> > The provided examples can already be copied-and-pasted and slightly
> > adapted to add more attributes.
> 
> Can't you like add comments to the optional ones to reduce the incentive 
> to fill them with random junk as it's a lot easier to just delete them than 
> generating some random junk. So if a developer is unsure what to do a 
> comment telling something is optional would help to lean towards 'I can 
> safely delete this'?

That would be possible. But I'm still not convinced.
If driver authors can't be expected to know how to implement their own
sysfs attribute groups from the similar provided examples as needed, we
would have to provide example code for sysfs attributes of all firmware
attributes. And that would be a lot of them.

Also the attributes themselves would be highly repetitive. The
interesting logic would be how to wire it up the the rest of the driver,
and the example code can't provide copy-paste code for that.

Re: [PATCH 2/2] platform/x86: firmware_attributes_class: Add test driver

[Mario Limonciello]

On 1/9/2025 09:17, Thomas Weißschuh wrote:
> On 2025-01-08 11:30:12+0200, Ilpo Järvinen wrote:
>> On Tue, 7 Jan 2025, Thomas Weißschuh wrote:
>>
>>> On 2025-01-07 15:18:21-0600, Mario Limonciello wrote:
>>>> On 1/7/2025 14:50, Thomas Weißschuh wrote:
>>>>> On 2025-01-07 13:29:08-0600, Mario Limonciello wrote:
>>>>>> On 1/7/2025 11:05, Thomas Weißschuh wrote:
>>>>>>> The driver showcases the use of the new subsystem API.
>>>>>>>
>>>>>>> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
>>>>>>> ---
>>>>>>>     drivers/platform/x86/Kconfig                    | 12 ++++
>>>>>>>     drivers/platform/x86/Makefile                   |  1 +
>>>>>>>     drivers/platform/x86/firmware_attributes_test.c | 78 +++++++++++++++++++++++++
>>>>>>>     3 files changed, 91 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
>>>>>>> index 0258dd879d64be389f4dd9bc309fe089f23cc798..2a0e828657d2f07074944d6c42dc204fc8825a42 100644
>>>>>>> --- a/drivers/platform/x86/Kconfig
>>>>>>> +++ b/drivers/platform/x86/Kconfig
>>>>>>> @@ -1065,6 +1065,18 @@ source "drivers/platform/x86/x86-android-tablets/Kconfig"
>>>>>>>     config FW_ATTR_CLASS
>>>>>>>     	tristate
>>>>>>> +config FW_ATTR_TEST
>>>>>>> +	tristate "Firmware attribute test driver"
>>>>>>> +	select FW_ATTR_CLASS
>>>>>>> +	help
>>>>>>> +	  This driver provides a test user of the firmware attribute subsystem.
>>>>>>> +
>>>>>>> +	  An instance is created at /sys/class/firmware-attributes/test/
>>>>>>> +	  container various example attributes.
>>>>>>> +
>>>>>>> +	  To compile this driver as a module, choose M here: the module
>>>>>>> +	  will be called firmware_attributes_test.
>>>>>>> +
>>>>>>
>>>>>> I think if you're going to be introducing a test driver it should be
>>>>>> compliant to what's in sysfs-class-firmware-attributes so that when it's
>>>>>> inevitably copy/pasted we end up with higher quality drivers.
>>>>>>
>>>>>> That is you need at a minimum 'type', 'current_value', 'default_value',
>>>>>> 'display_name' and 'display_name_language_code'.  Then individual types
>>>>>> employ additional requirements.
>>>>>>
>>>>>> I see 'type', 'current_value', and 'default_value, but I don't see
>>>>>> 'display_name' or 'display_name_language_code' here.
>>>>>>
>>>>>> Furthermore as this is a "string" attribute you're supposed to also have a
>>>>>> "max_length" and "min_length".
>>>>>
>>>>> Agreed that more examples are better.
>>>>>
>>>>> But are these attributes really mandatory?
>>>>> "This attribute is mandatory" is only specified for "type" and>
>>>> "current_value".
>>>>
>>>> Ah wow, I had thought they were, but you're right they're not!
>>>>
>>>>> While "possible_values" certainly looks necessary for "enumeration",
>>>>> "min_length" for "strings" does so much less.
>>>>
>>>> Even if they're not mandatory, I think it's better to include them for the
>>>> same copy/paste reason I mentioned though.
>>>
>>> Thinking about it some more, which attributes should all be included?
>>> Having all of them in there could motivate driver authors to implement
>>> them all even it would mean filling in random values.
>>> The provided examples can already be copied-and-pasted and slightly
>>> adapted to add more attributes.
>>
>> Can't you like add comments to the optional ones to reduce the incentive
>> to fill them with random junk as it's a lot easier to just delete them than
>> generating some random junk. So if a developer is unsure what to do a
>> comment telling something is optional would help to lean towards 'I can
>> safely delete this'?
> 
> That would be possible. But I'm still not convinced.
> If driver authors can't be expected to know how to implement their own
> sysfs attribute groups from the similar provided examples as needed, we
> would have to provide example code for sysfs attributes of all firmware
> attributes. And that would be a lot of them.
> 
> Also the attributes themselves would be highly repetitive. The
> interesting logic would be how to wire it up the the rest of the driver,
> and the example code can't provide copy-paste code for that.

Thinking about it a bit more what do you think about providing a macro 
helper for drivers to use?  Think about how we have macros for pm ops 
for example and drivers can optionally populate all fields with callbacks.

A macro for "enumeration" attributes, another for "string" attributes, 
and another for "integer" attributes.

For string it could have optional values .min_length and .max_length,

For enumeration it can have a callback that gets you a pointer to a 
string of possible options.

For integer attributes it can have a field for scalar value etc.

Re: [PATCH 2/2] platform/x86: firmware_attributes_class: Add test driver

[Thomas Weißschuh]

On 2025-01-09 09:37:15-0600, Mario Limonciello wrote:
> On 1/9/2025 09:17, Thomas Weißschuh wrote:
> > On 2025-01-08 11:30:12+0200, Ilpo Järvinen wrote:
> > > On Tue, 7 Jan 2025, Thomas Weißschuh wrote:
> > > 
> > > > On 2025-01-07 15:18:21-0600, Mario Limonciello wrote:
> > > > > On 1/7/2025 14:50, Thomas Weißschuh wrote:
> > > > > > On 2025-01-07 13:29:08-0600, Mario Limonciello wrote:
> > > > > > > On 1/7/2025 11:05, Thomas Weißschuh wrote:
> > > > > > > > The driver showcases the use of the new subsystem API.
> > > > > > > > 
> > > > > > > > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > > > > > > > ---
> > > > > > > >     drivers/platform/x86/Kconfig                    | 12 ++++
> > > > > > > >     drivers/platform/x86/Makefile                   |  1 +
> > > > > > > >     drivers/platform/x86/firmware_attributes_test.c | 78 +++++++++++++++++++++++++
> > > > > > > >     3 files changed, 91 insertions(+)
> > > > > > > > 
> > > > > > > > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> > > > > > > > index 0258dd879d64be389f4dd9bc309fe089f23cc798..2a0e828657d2f07074944d6c42dc204fc8825a42 100644
> > > > > > > > --- a/drivers/platform/x86/Kconfig
> > > > > > > > +++ b/drivers/platform/x86/Kconfig
> > > > > > > > @@ -1065,6 +1065,18 @@ source "drivers/platform/x86/x86-android-tablets/Kconfig"
> > > > > > > >     config FW_ATTR_CLASS
> > > > > > > >     	tristate
> > > > > > > > +config FW_ATTR_TEST
> > > > > > > > +	tristate "Firmware attribute test driver"
> > > > > > > > +	select FW_ATTR_CLASS
> > > > > > > > +	help
> > > > > > > > +	  This driver provides a test user of the firmware attribute subsystem.
> > > > > > > > +
> > > > > > > > +	  An instance is created at /sys/class/firmware-attributes/test/
> > > > > > > > +	  container various example attributes.
> > > > > > > > +
> > > > > > > > +	  To compile this driver as a module, choose M here: the module
> > > > > > > > +	  will be called firmware_attributes_test.
> > > > > > > > +
> > > > > > > 
> > > > > > > I think if you're going to be introducing a test driver it should be
> > > > > > > compliant to what's in sysfs-class-firmware-attributes so that when it's
> > > > > > > inevitably copy/pasted we end up with higher quality drivers.
> > > > > > > 
> > > > > > > That is you need at a minimum 'type', 'current_value', 'default_value',
> > > > > > > 'display_name' and 'display_name_language_code'.  Then individual types
> > > > > > > employ additional requirements.
> > > > > > > 
> > > > > > > I see 'type', 'current_value', and 'default_value, but I don't see
> > > > > > > 'display_name' or 'display_name_language_code' here.
> > > > > > > 
> > > > > > > Furthermore as this is a "string" attribute you're supposed to also have a
> > > > > > > "max_length" and "min_length".
> > > > > > 
> > > > > > Agreed that more examples are better.
> > > > > > 
> > > > > > But are these attributes really mandatory?
> > > > > > "This attribute is mandatory" is only specified for "type" and>
> > > > > "current_value".
> > > > > 
> > > > > Ah wow, I had thought they were, but you're right they're not!
> > > > > 
> > > > > > While "possible_values" certainly looks necessary for "enumeration",
> > > > > > "min_length" for "strings" does so much less.
> > > > > 
> > > > > Even if they're not mandatory, I think it's better to include them for the
> > > > > same copy/paste reason I mentioned though.
> > > > 
> > > > Thinking about it some more, which attributes should all be included?
> > > > Having all of them in there could motivate driver authors to implement
> > > > them all even it would mean filling in random values.
> > > > The provided examples can already be copied-and-pasted and slightly
> > > > adapted to add more attributes.
> > > 
> > > Can't you like add comments to the optional ones to reduce the incentive
> > > to fill them with random junk as it's a lot easier to just delete them than
> > > generating some random junk. So if a developer is unsure what to do a
> > > comment telling something is optional would help to lean towards 'I can
> > > safely delete this'?
> > 
> > That would be possible. But I'm still not convinced.
> > If driver authors can't be expected to know how to implement their own
> > sysfs attribute groups from the similar provided examples as needed, we
> > would have to provide example code for sysfs attributes of all firmware
> > attributes. And that would be a lot of them.
> > 
> > Also the attributes themselves would be highly repetitive. The
> > interesting logic would be how to wire it up the the rest of the driver,
> > and the example code can't provide copy-paste code for that.
> 
> Thinking about it a bit more what do you think about providing a macro
> helper for drivers to use?  Think about how we have macros for pm ops for
> example and drivers can optionally populate all fields with callbacks.

This is what I tried, but came to the conclusion that it would be very
complex.

> A macro for "enumeration" attributes, another for "string" attributes, and
> another for "integer" attributes.
> 
> For string it could have optional values .min_length and .max_length,

Then we need a flags field to track if the an attribute has either of
these fields, because 0 is a valid value.
And macros to initialize a static struct of it, with any combination of
optional attributes. And more macros to do the same for dynamically
allocated structs.

> For enumeration it can have a callback that gets you a pointer to a string
> of possible options.

Callbacks would be better as there is a clear value when a attribute is
missing.

> For integer attributes it can have a field for scalar value etc.

Same as for .min_length and .max_length.


Let me try again with callbacks.