* [PATCH RFC 1/5] platform/x86: firmware_attributes_class: Add device initialization methods
2025-05-09 7:48 [PATCH RFC 0/5] platform/x86: firmware_attributes_class: Add a high level API Kurt Borja
@ 2025-05-09 7:48 ` Kurt Borja
2025-05-09 15:59 ` Mario Limonciello
2025-05-09 7:48 ` [PATCH RFC 2/5] platform/x86: firmware_attributes_class: Add a high level API Kurt Borja
` (3 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Kurt Borja @ 2025-05-09 7:48 UTC (permalink / raw)
To: Hans de Goede, Ilpo Järvinen, Thomas Weißschuh,
Joshua Grisham, Mark Pearson, Armin Wolf, Mario Limonciello
Cc: Antheas Kapenekakis, Derek J. Clark, Prasanth Ksr, Jorge Lopez,
platform-driver-x86, linux-kernel, Dell.Client.Kernel, Kurt Borja
From: Thomas Weißschuh <linux@weissschuh.net>
Currently each user of firmware_attributes_class has to manually set up
kobjects, devices, etc.
Provide this infrastructure out-of-the-box through the newly introduced
fwat_device_register().
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 | 165 +++++++++++++++++++++++
drivers/platform/x86/firmware_attributes_class.h | 44 ++++++
2 files changed, 209 insertions(+)
diff --git a/drivers/platform/x86/firmware_attributes_class.c b/drivers/platform/x86/firmware_attributes_class.c
index 736e96c186d9dc6d945517f090e9af903e93bbf4..58ab1495ba3bd449cfe17de2827a57a0c5937788 100644
--- a/drivers/platform/x86/firmware_attributes_class.c
+++ b/drivers/platform/x86/firmware_attributes_class.c
@@ -2,7 +2,12 @@
/* Firmware attributes class helper module */
+#include <linux/device.h>
+#include <linux/device/class.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 +15,164 @@ const struct class firmware_attributes_class = {
};
EXPORT_SYMBOL_GPL(firmware_attributes_class);
+static ssize_t fwat_attrs_kobj_show(struct kobject *kobj, struct attribute *attr,
+ char *buf)
+{
+ const struct fwat_attribute *fattr = to_fwat_attribute(attr);
+ struct fwat_device *fadev = to_fwat_device(kobj);
+
+ if (!fattr->show)
+ return -ENOENT;
+
+ return fattr->show(fadev->dev, fattr, buf);
+}
+
+static ssize_t fwat_attrs_kobj_store(struct kobject *kobj, struct attribute *attr,
+ const char *buf, size_t count)
+{
+ const struct fwat_attribute *fattr = to_fwat_attribute(attr);
+ struct fwat_device *fadev = to_fwat_device(kobj);
+
+ if (!fattr->store)
+ return -ENOENT;
+
+ return fattr->store(fadev->dev, fattr, buf, count);
+}
+
+static const struct sysfs_ops fwat_attrs_kobj_ops = {
+ .show = fwat_attrs_kobj_show,
+ .store = fwat_attrs_kobj_store,
+};
+
+static void fwat_attrs_kobj_release(struct kobject *kobj)
+{
+ struct fwat_device *fadev = to_fwat_device(kobj);
+
+ kfree(fadev);
+}
+
+static const struct kobj_type fwat_attrs_ktype = {
+ .sysfs_ops = &fwat_attrs_kobj_ops,
+ .release = fwat_attrs_kobj_release,
+};
+
+/**
+ * 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: Sysfs groups for the custom `fwat_attrs_ktype` kobj_type
+ *
+ * NOTE: @groups are attached to the .attrs_kobj of the new fwat_device which
+ * has a custom ktype, which makes use of `struct fwat_attribute` to embed
+ * attributes.
+ *
+ * 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 *data,
+ const struct attribute_group **groups)
+{
+ struct fwat_device *fadev;
+ struct device *dev;
+ int ret;
+
+ if (!parent || !name)
+ return ERR_PTR(-EINVAL);
+
+ fadev = kzalloc(sizeof(*fadev), GFP_KERNEL);
+ if (!fadev)
+ return ERR_PTR(-ENOMEM);
+
+ dev = device_create(&firmware_attributes_class, parent, MKDEV(0, 0),
+ data, "%s", name);
+ if (IS_ERR(dev)) {
+ kfree(fadev);
+ return ERR_CAST(dev);
+ }
+
+ ret = kobject_init_and_add(&fadev->attrs_kobj, &fwat_attrs_ktype, &dev->kobj,
+ "attributes");
+ if (ret)
+ goto out_kobj_put;
+
+ if (groups) {
+ ret = sysfs_create_groups(&fadev->attrs_kobj, groups);
+ if (ret)
+ goto out_kobj_unregister;
+ }
+
+ fadev->dev = dev;
+ fadev->groups = groups;
+
+ kobject_uevent(&fadev->attrs_kobj, KOBJ_ADD);
+
+ return fadev;
+
+out_kobj_unregister:
+ kobject_del(&fadev->attrs_kobj);
+
+out_kobj_put:
+ kobject_put(&fadev->attrs_kobj);
+ device_unregister(dev);
+
+ return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(fwat_device_register);
+
+void fwat_device_unregister(struct fwat_device *fwadev)
+{
+ if (fwadev->groups)
+ sysfs_remove_groups(&fwadev->attrs_kobj, fwadev->groups);
+ kobject_del(&fwadev->attrs_kobj);
+ kobject_put(&fwadev->attrs_kobj);
+ device_unregister(fwadev->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: Sysfs groups for the custom `fwat_attrs_ktype` kobj_type
+ *
+ * Device managed version of fwat_device_register().
+ *
+ * NOTE: @groups are attached to the .attrs_kobj of the new fwat_device which
+ * has a custom ktype, which makes use of `struct fwat_attribute` to embed
+ * attributes.
+ *
+ * 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 +186,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..ad94bf91e5af30a2b8feb9abf224ee6f0d17600a 100644
--- a/drivers/platform/x86/firmware_attributes_class.h
+++ b/drivers/platform/x86/firmware_attributes_class.h
@@ -5,8 +5,52 @@
#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 kobject attrs_kobj;
+ const struct attribute_group **groups;
+};
+
+#define to_fwat_device(_k) container_of_const(_k, struct fwat_device, attrs_kobj)
+
+/**
+ * struct fwat_attribute - The firmware-attributes's custom attribute
+ * @attr: Embedded struct attribute.
+ * @show: Show method called by the "attributes" kobject's ktype.
+ * @store: Store method called by the "attributes" kobject's ktype.
+ */
+struct fwat_attribute {
+ struct attribute attr;
+ ssize_t (*show)(struct device *dev, const struct fwat_attribute *attr,
+ char *buf);
+ ssize_t (*store)(struct device *dev, const struct fwat_attribute *attr,
+ const char *buf, size_t count);
+};
+
+#define to_fwat_attribute(_a) container_of_const(_a, struct fwat_attribute, attr)
+
+struct fwat_device * __must_check
+fwat_device_register(struct device *parent, const char *name, void *data,
+ 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.49.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH RFC 1/5] platform/x86: firmware_attributes_class: Add device initialization methods
2025-05-09 7:48 ` [PATCH RFC 1/5] platform/x86: firmware_attributes_class: Add device initialization methods Kurt Borja
@ 2025-05-09 15:59 ` Mario Limonciello
0 siblings, 0 replies; 12+ messages in thread
From: Mario Limonciello @ 2025-05-09 15:59 UTC (permalink / raw)
To: Kurt Borja, Hans de Goede, Ilpo Järvinen,
Thomas Weißschuh, Joshua Grisham, Mark Pearson, Armin Wolf
Cc: Antheas Kapenekakis, Derek J. Clark, Prasanth Ksr, Jorge Lopez,
platform-driver-x86, linux-kernel, Dell.Client.Kernel
On 5/9/2025 2:48 AM, 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().
>
> 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>
Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> drivers/platform/x86/firmware_attributes_class.c | 165 +++++++++++++++++++++++
> drivers/platform/x86/firmware_attributes_class.h | 44 ++++++
> 2 files changed, 209 insertions(+)
>
> diff --git a/drivers/platform/x86/firmware_attributes_class.c b/drivers/platform/x86/firmware_attributes_class.c
> index 736e96c186d9dc6d945517f090e9af903e93bbf4..58ab1495ba3bd449cfe17de2827a57a0c5937788 100644
> --- a/drivers/platform/x86/firmware_attributes_class.c
> +++ b/drivers/platform/x86/firmware_attributes_class.c
> @@ -2,7 +2,12 @@
>
> /* Firmware attributes class helper module */
>
> +#include <linux/device.h>
> +#include <linux/device/class.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 +15,164 @@ const struct class firmware_attributes_class = {
> };
> EXPORT_SYMBOL_GPL(firmware_attributes_class);
>
> +static ssize_t fwat_attrs_kobj_show(struct kobject *kobj, struct attribute *attr,
> + char *buf)
> +{
> + const struct fwat_attribute *fattr = to_fwat_attribute(attr);
> + struct fwat_device *fadev = to_fwat_device(kobj);
> +
> + if (!fattr->show)
> + return -ENOENT;
> +
> + return fattr->show(fadev->dev, fattr, buf);
> +}
> +
> +static ssize_t fwat_attrs_kobj_store(struct kobject *kobj, struct attribute *attr,
> + const char *buf, size_t count)
> +{
> + const struct fwat_attribute *fattr = to_fwat_attribute(attr);
> + struct fwat_device *fadev = to_fwat_device(kobj);
> +
> + if (!fattr->store)
> + return -ENOENT;
> +
> + return fattr->store(fadev->dev, fattr, buf, count);
> +}
> +
> +static const struct sysfs_ops fwat_attrs_kobj_ops = {
> + .show = fwat_attrs_kobj_show,
> + .store = fwat_attrs_kobj_store,
> +};
> +
> +static void fwat_attrs_kobj_release(struct kobject *kobj)
> +{
> + struct fwat_device *fadev = to_fwat_device(kobj);
> +
> + kfree(fadev);
> +}
> +
> +static const struct kobj_type fwat_attrs_ktype = {
> + .sysfs_ops = &fwat_attrs_kobj_ops,
> + .release = fwat_attrs_kobj_release,
> +};
> +
> +/**
> + * 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: Sysfs groups for the custom `fwat_attrs_ktype` kobj_type
> + *
> + * NOTE: @groups are attached to the .attrs_kobj of the new fwat_device which
> + * has a custom ktype, which makes use of `struct fwat_attribute` to embed
> + * attributes.
> + *
> + * 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 *data,
> + const struct attribute_group **groups)
> +{
> + struct fwat_device *fadev;
> + struct device *dev;
> + int ret;
> +
> + if (!parent || !name)
> + return ERR_PTR(-EINVAL);
> +
> + fadev = kzalloc(sizeof(*fadev), GFP_KERNEL);
> + if (!fadev)
> + return ERR_PTR(-ENOMEM);
> +
> + dev = device_create(&firmware_attributes_class, parent, MKDEV(0, 0),
> + data, "%s", name);
> + if (IS_ERR(dev)) {
> + kfree(fadev);
> + return ERR_CAST(dev);
> + }
> +
> + ret = kobject_init_and_add(&fadev->attrs_kobj, &fwat_attrs_ktype, &dev->kobj,
> + "attributes");
> + if (ret)
> + goto out_kobj_put;
> +
> + if (groups) {
> + ret = sysfs_create_groups(&fadev->attrs_kobj, groups);
> + if (ret)
> + goto out_kobj_unregister;
> + }
> +
> + fadev->dev = dev;
> + fadev->groups = groups;
> +
> + kobject_uevent(&fadev->attrs_kobj, KOBJ_ADD);
> +
> + return fadev;
> +
> +out_kobj_unregister:
> + kobject_del(&fadev->attrs_kobj);
> +
> +out_kobj_put:
> + kobject_put(&fadev->attrs_kobj);
> + device_unregister(dev);
> +
> + return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL_GPL(fwat_device_register);
> +
> +void fwat_device_unregister(struct fwat_device *fwadev)
> +{
> + if (fwadev->groups)
> + sysfs_remove_groups(&fwadev->attrs_kobj, fwadev->groups);
> + kobject_del(&fwadev->attrs_kobj);
> + kobject_put(&fwadev->attrs_kobj);
> + device_unregister(fwadev->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: Sysfs groups for the custom `fwat_attrs_ktype` kobj_type
> + *
> + * Device managed version of fwat_device_register().
> + *
> + * NOTE: @groups are attached to the .attrs_kobj of the new fwat_device which
> + * has a custom ktype, which makes use of `struct fwat_attribute` to embed
> + * attributes.
> + *
> + * 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 +186,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..ad94bf91e5af30a2b8feb9abf224ee6f0d17600a 100644
> --- a/drivers/platform/x86/firmware_attributes_class.h
> +++ b/drivers/platform/x86/firmware_attributes_class.h
> @@ -5,8 +5,52 @@
> #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 kobject attrs_kobj;
> + const struct attribute_group **groups;
> +};
> +
> +#define to_fwat_device(_k) container_of_const(_k, struct fwat_device, attrs_kobj)
> +
> +/**
> + * struct fwat_attribute - The firmware-attributes's custom attribute
> + * @attr: Embedded struct attribute.
> + * @show: Show method called by the "attributes" kobject's ktype.
> + * @store: Store method called by the "attributes" kobject's ktype.
> + */
> +struct fwat_attribute {
> + struct attribute attr;
> + ssize_t (*show)(struct device *dev, const struct fwat_attribute *attr,
> + char *buf);
> + ssize_t (*store)(struct device *dev, const struct fwat_attribute *attr,
> + const char *buf, size_t count);
> +};
> +
> +#define to_fwat_attribute(_a) container_of_const(_a, struct fwat_attribute, attr)
> +
> +struct fwat_device * __must_check
> +fwat_device_register(struct device *parent, const char *name, void *data,
> + 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 */
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH RFC 2/5] platform/x86: firmware_attributes_class: Add a high level API
2025-05-09 7:48 [PATCH RFC 0/5] platform/x86: firmware_attributes_class: Add a high level API Kurt Borja
2025-05-09 7:48 ` [PATCH RFC 1/5] platform/x86: firmware_attributes_class: Add device initialization methods Kurt Borja
@ 2025-05-09 7:48 ` Kurt Borja
2025-05-09 15:58 ` Mario Limonciello
2025-05-09 7:48 ` [PATCH RFC 3/5] platform/x86: firmware_attributes_class: Add a boolean type Kurt Borja
` (2 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Kurt Borja @ 2025-05-09 7:48 UTC (permalink / raw)
To: Hans de Goede, Ilpo Järvinen, Thomas Weißschuh,
Joshua Grisham, Mark Pearson, Armin Wolf, Mario Limonciello
Cc: Antheas Kapenekakis, Derek J. Clark, Prasanth Ksr, Jorge Lopez,
platform-driver-x86, linux-kernel, Dell.Client.Kernel, Kurt Borja
Add an attribute configuration mechanism through the newly introduced
`struct fwat_dev_config`, which makes use of other documented structs
and callbacks.
This API aims to be simple, yet flexible. In order to accomplish this,
the following features were taken into account:
* Ability to statically define attributes
* Custom read/write callbacks for each attribute type
* Ability to map attributes to numbers in order to differentiate them in
callbacks (`aux` number)
* Ability to reuse read/write callbacks in different attributes
* Ability to reuse property selection in different attributes
* Optional visibility callback for dynamic attribute visibility
Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
drivers/platform/x86/firmware_attributes_class.c | 249 ++++++++++++++++++++++-
drivers/platform/x86/firmware_attributes_class.h | 228 +++++++++++++++++++++
2 files changed, 474 insertions(+), 3 deletions(-)
diff --git a/drivers/platform/x86/firmware_attributes_class.c b/drivers/platform/x86/firmware_attributes_class.c
index 58ab1495ba3bd449cfe17de2827a57a0c5937788..7cfb0f49f235728c7450a82a7e9d00b8963d3dea 100644
--- a/drivers/platform/x86/firmware_attributes_class.c
+++ b/drivers/platform/x86/firmware_attributes_class.c
@@ -7,14 +7,233 @@
#include <linux/kobject.h>
#include <linux/module.h>
#include <linux/slab.h>
+#include <linux/sysfs.h>
#include <linux/types.h>
#include "firmware_attributes_class.h"
+#define to_fwat_attribute_ext(_a) container_of_const(_a, struct fwat_attribute_ext, attr)
+
+struct fwat_attribute_ext {
+ struct fwat_attribute attr;
+ enum fwat_property prop;
+ const struct fwat_attr_config *config;
+};
+
const struct class firmware_attributes_class = {
.name = "firmware-attributes",
};
EXPORT_SYMBOL_GPL(firmware_attributes_class);
+static const char * const fwat_type_labels[] = {
+ [fwat_type_integer] = "integer",
+ [fwat_type_string] = "string",
+ [fwat_type_enumeration] = "enumeration",
+};
+
+static const char * const fwat_prop_labels[] = {
+ [FWAT_PROP_DISPLAY_NAME] = "display_name",
+ [FWAT_PROP_LANGUAGE_CODE] = "display_name_language_code",
+ [FWAT_PROP_DEFAULT] = "default",
+
+ [FWAT_INT_PROP_MIN] = "min_value",
+ [FWAT_INT_PROP_MAX] = "max_value",
+ [FWAT_INT_PROP_INCREMENT] = "scalar_increment",
+
+ [FWAT_STR_PROP_MIN] = "min_length",
+ [FWAT_STR_PROP_MAX] = "max_length",
+
+ [FWAT_ENUM_PROP_POSSIBLE_VALUES] = "possible_values",
+};
+
+static ssize_t
+fwat_type_show(struct device *dev, const struct fwat_attribute *attr, char *buf)
+{
+ const struct fwat_attribute_ext *ext = to_fwat_attribute_ext(attr);
+ const struct fwat_attr_config *config = ext->config;
+
+ return sysfs_emit(buf, "%s\n", fwat_type_labels[config->type]);
+}
+
+static ssize_t
+fwat_property_show(struct device *dev, const struct fwat_attribute *attr, char *buf)
+{
+ const struct fwat_attribute_ext *ext = to_fwat_attribute_ext(attr);
+ const struct fwat_attr_config *config = ext->config;
+
+ if (!config->ops->prop_read)
+ return -EOPNOTSUPP;
+
+ return config->ops->prop_read(dev, config->aux, ext->prop, buf);
+}
+
+static ssize_t
+fwat_current_value_show(struct device *dev, const struct fwat_attribute *attr, char *buf)
+{
+ const struct fwat_attribute_ext *ext = to_fwat_attribute_ext(attr);
+ const struct fwat_attr_config *config = ext->config;
+ const char *str;
+ long int_val;
+ int ret;
+
+ switch (config->type) {
+ case fwat_type_integer:
+ ret = config->ops->integer_read(dev, config->aux, &int_val);
+ if (ret)
+ return ret;
+
+ return sysfs_emit(buf, "%ld\n", int_val);
+ case fwat_type_string:
+ ret = config->ops->string_read(dev, config->aux, &str);
+ if (ret)
+ return ret;
+
+ return sysfs_emit(buf, "%s\n", str);
+ case fwat_type_enumeration:
+ ret = config->ops->enumeration_read(dev, config->aux, &str);
+ if (ret)
+ return ret;
+
+ return sysfs_emit(buf, "%s\n", str);
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+static ssize_t
+fwat_current_value_store(struct device *dev, const struct fwat_attribute *attr,
+ const char *buf, size_t count)
+{
+ const struct fwat_attribute_ext *ext = to_fwat_attribute_ext(attr);
+ const struct fwat_attr_config *config = ext->config;
+ long int_val;
+ int ret;
+
+ switch (config->type) {
+ case fwat_type_integer:
+ ret = kstrtol(buf, 0, &int_val);
+ if (ret)
+ return ret;
+
+ ret = config->ops->integer_write(dev, config->aux, int_val);
+ break;
+ case fwat_type_string:
+ ret = config->ops->string_write(dev, config->aux, buf);
+ break;
+ case fwat_type_enumeration:
+ ret = config->ops->enumeration_write(dev, config->aux, buf);
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
+
+ return ret ? ret : count;
+}
+
+static struct attribute *
+fwat_alloc_attr(struct device *dev, const struct fwat_attr_config *config,
+ const char *attr_name, umode_t mode, enum fwat_property prop,
+ ssize_t (*show)(struct device *dev, const struct fwat_attribute *attr,
+ char *buf),
+ ssize_t (*store)(struct device *dev, const struct fwat_attribute *attr,
+ const char *buf, size_t count))
+{
+ struct fwat_attribute_ext *fattr;
+
+ fattr = devm_kzalloc(dev, sizeof(*fattr), GFP_KERNEL);
+ if (!fattr)
+ return NULL;
+
+ fattr->attr.attr.name = attr_name;
+ fattr->attr.attr.mode = mode;
+ fattr->attr.show = show;
+ fattr->attr.store = store;
+ fattr->prop = prop;
+ fattr->config = config;
+ sysfs_attr_init(&fattr->attr.attr);
+
+ return &fattr->attr.attr;
+}
+
+static struct attribute **
+fwat_create_attrs(struct device *dev, const struct fwat_attr_config *config)
+{
+ struct attribute **attrs;
+ enum fwat_property prop;
+ unsigned int index = 0;
+
+ attrs = devm_kcalloc(dev, config->num_props + 3, sizeof(*attrs), GFP_KERNEL);
+ if (!attrs)
+ return NULL;
+
+ /*
+ * Create optional attributes
+ */
+ for (; index < config->num_props; index++) {
+ prop = config->props[index];
+ attrs[index] = fwat_alloc_attr(dev, config, fwat_prop_labels[prop],
+ 0444, prop, fwat_property_show, NULL);
+ }
+
+ /*
+ * Create mandatory attributes
+ */
+ attrs[index++] = fwat_alloc_attr(dev, config, "type", 0444, 0, fwat_type_show, NULL);
+ attrs[index++] = fwat_alloc_attr(dev, config, "current_value", 0644, 0,
+ fwat_current_value_show, fwat_current_value_store);
+
+ return attrs;
+}
+
+static const struct attribute_group *
+fwat_create_group(struct device *dev, const struct fwat_attr_config *config)
+{
+ struct attribute_group *group;
+ struct attribute **attrs;
+
+ group = devm_kzalloc(dev, sizeof(*group), GFP_KERNEL);
+ if (!group)
+ return NULL;
+
+ attrs = fwat_create_attrs(dev, config);
+ if (!attrs)
+ return NULL;
+
+ group->name = config->name;
+ group->attrs = attrs;
+
+ return group;
+}
+
+static const struct attribute_group **
+fwat_create_auto_groups(struct device *dev, const struct fwat_dev_config *config)
+{
+ const struct attribute_group **groups;
+ const struct attribute_group *grp;
+ unsigned int index = 0;
+ size_t ngroups = 0;
+
+ while (config->attrs_config[ngroups])
+ ngroups++;
+
+ groups = devm_kcalloc(dev, ngroups + 1, sizeof(*groups), GFP_KERNEL);
+ if (!groups)
+ return NULL;
+
+ for (unsigned int i = 0; i < ngroups; i++) {
+ if (config->is_visible &&
+ !config->is_visible(dev, config->attrs_config[i]))
+ continue;
+
+ grp = fwat_create_group(dev, config->attrs_config[i]);
+ if (!grp)
+ return NULL;
+
+ groups[index++] = grp;
+ }
+
+ return groups;
+}
+
static ssize_t fwat_attrs_kobj_show(struct kobject *kobj, struct attribute *attr,
char *buf)
{
@@ -61,6 +280,7 @@ static const struct kobj_type fwat_attrs_ktype = {
* device
* @parent: Parent device
* @name: Name of the class device
+ * @config: Device configuration
* @data: Drvdata of the class device
* @groups: Sysfs groups for the custom `fwat_attrs_ktype` kobj_type
*
@@ -72,8 +292,10 @@ static const struct kobj_type fwat_attrs_ktype = {
*/
struct fwat_device *
fwat_device_register(struct device *parent, const char *name, void *data,
+ const struct fwat_dev_config *config,
const struct attribute_group **groups)
{
+ const struct attribute_group **auto_groups;
struct fwat_device *fadev;
struct device *dev;
int ret;
@@ -97,19 +319,36 @@ fwat_device_register(struct device *parent, const char *name, void *data,
if (ret)
goto out_kobj_put;
- if (groups) {
- ret = sysfs_create_groups(&fadev->attrs_kobj, groups);
+ if (config) {
+ auto_groups = fwat_create_auto_groups(dev, config);
+ if (!auto_groups) {
+ ret = -ENOMEM;
+ goto out_kobj_unregister;
+ }
+
+ ret = sysfs_create_groups(&fadev->attrs_kobj, auto_groups);
if (ret)
goto out_kobj_unregister;
}
+ if (groups) {
+ ret = sysfs_create_groups(&fadev->attrs_kobj, groups);
+ if (ret)
+ goto out_remove_auto_groups;
+ }
+
fadev->dev = dev;
fadev->groups = groups;
+ fadev->auto_groups = groups;
kobject_uevent(&fadev->attrs_kobj, KOBJ_ADD);
return fadev;
+out_remove_auto_groups:
+ if (config)
+ sysfs_remove_groups(&fadev->attrs_kobj, auto_groups);
+
out_kobj_unregister:
kobject_del(&fadev->attrs_kobj);
@@ -125,6 +364,8 @@ void fwat_device_unregister(struct fwat_device *fwadev)
{
if (fwadev->groups)
sysfs_remove_groups(&fwadev->attrs_kobj, fwadev->groups);
+ if (fwadev->auto_groups)
+ sysfs_remove_groups(&fwadev->attrs_kobj, fwadev->auto_groups);
kobject_del(&fwadev->attrs_kobj);
kobject_put(&fwadev->attrs_kobj);
device_unregister(fwadev->dev);
@@ -143,6 +384,7 @@ static void devm_fwat_device_release(void *data)
* device
* @parent: Parent device
* @name: Name of the class device
+ * @config: Device configuration
* @data: Drvdata of the class device
* @groups: Sysfs groups for the custom `fwat_attrs_ktype` kobj_type
*
@@ -156,12 +398,13 @@ static void devm_fwat_device_release(void *data)
*/
struct fwat_device *
devm_fwat_device_register(struct device *parent, const char *name, void *data,
+ const struct fwat_dev_config *config,
const struct attribute_group **groups)
{
struct fwat_device *fadev;
int ret;
- fadev = fwat_device_register(parent, name, data, groups);
+ fadev = fwat_device_register(parent, name, data, config, groups);
if (IS_ERR(fadev))
return fadev;
diff --git a/drivers/platform/x86/firmware_attributes_class.h b/drivers/platform/x86/firmware_attributes_class.h
index ad94bf91e5af30a2b8feb9abf224ee6f0d17600a..f250875d785c3439682c43c693f98e1c20e9ff54 100644
--- a/drivers/platform/x86/firmware_attributes_class.h
+++ b/drivers/platform/x86/firmware_attributes_class.h
@@ -18,11 +18,14 @@ extern const struct class firmware_attributes_class;
* @dev: The class device.
* @attrs_kobj: The "attributes" root kobject.
* @groups: Sysfs groups attached to the @attrs_kobj.
+ * @auto_groups: Sysgs groups generated from &struct fwat_attr_config attached.
+ * to the @attrs_kobj
*/
struct fwat_device {
struct device *dev;
struct kobject attrs_kobj;
const struct attribute_group **groups;
+ const struct attribute_group **auto_groups;
};
#define to_fwat_device(_k) container_of_const(_k, struct fwat_device, attrs_kobj)
@@ -30,6 +33,7 @@ struct fwat_device {
/**
* struct fwat_attribute - The firmware-attributes's custom attribute
* @attr: Embedded struct attribute.
+ * @aux: Auxiliary number defined by the user.
* @show: Show method called by the "attributes" kobject's ktype.
* @store: Store method called by the "attributes" kobject's ktype.
*/
@@ -43,14 +47,238 @@ struct fwat_attribute {
#define to_fwat_attribute(_a) container_of_const(_a, struct fwat_attribute, attr)
+enum fwat_attr_type {
+ fwat_type_integer,
+ fwat_type_string,
+ fwat_type_enumeration,
+};
+
+enum fwat_property {
+ FWAT_PROP_DISPLAY_NAME,
+ FWAT_PROP_LANGUAGE_CODE,
+ FWAT_PROP_DEFAULT,
+
+ FWAT_INT_PROP_MIN,
+ FWAT_INT_PROP_MAX,
+ FWAT_INT_PROP_INCREMENT,
+
+ FWAT_STR_PROP_MIN,
+ FWAT_STR_PROP_MAX,
+
+ FWAT_ENUM_PROP_POSSIBLE_VALUES,
+};
+
+struct fwat_attr_config;
+
+/**
+ * struct fwat_attr_ops - Operations for a firmware *attribute*
+ * @prop_read: Callback for retrieving each configured property of an attribute.
+ * @integer_read: Callback for reading the current_value of an attribute of
+ * type *integer*.
+ * @integer_write: Callback for writing the current_value of an attribute of
+ * type *integer*.
+ * @string_read: Callback for reading the current_value of an attribute of type
+ * *string*.
+ * @string_write: Callback for writing the current_value of an attribute of type
+ * *string*.
+ * @enumeration_read: Callback for reading the current_value of an attribute of
+ * type *enumeration*.
+ * @enumeration_write: Callback for writing the current_value of an attribute
+ * of type *enumeration*.
+ */
+struct fwat_attr_ops {
+ ssize_t (*prop_read)(struct device *dev, long aux,
+ enum fwat_property prop, const char *buf);
+ union {
+ struct {
+ int (*integer_read)(struct device *dev, long aux,
+ long *val);
+ int (*integer_write)(struct device *dev, long aux,
+ long val);
+ };
+ struct {
+ int (*string_read)(struct device *dev, long aux,
+ const char **str);
+ int (*string_write)(struct device *dev, long aux,
+ const char *str);
+ };
+ struct {
+ int (*enumeration_read)(struct device *dev, long aux,
+ const char **str);
+ int (*enumeration_write)(struct device *dev, long aux,
+ const char *str);
+ };
+ };
+};
+
+/**
+ * struct fwat_attr_config - Configuration for a single firmware *attribute*
+ * @name: Name of the sysfs group associated with this *attribute*.
+ * @type: Type of this *attribute*.
+ * @aux: Auxiliary number defined by the user, which will be passed to
+ * read/write callbacks.
+ * @ops: Operations for this *attribute*.
+ * @props: Array of properties of this *attribute*.
+ * @num_props: Size of the props array.
+ */
+struct fwat_attr_config {
+ const char *name;
+ enum fwat_attr_type type;
+ long aux;
+ const struct fwat_attr_ops *ops;
+ const enum fwat_property *props;
+ size_t num_props;
+};
+
+/**
+ * DEFINE_SIMPLE_FWAT_OPS() - Define static &struct fwat_attr_ops for a simple
+ * *attribute* with no properties, i.e. No
+ * &fwat_attr_ops.read_prop callback
+ * @_name: Prefix of the `read` and `write` callbacks.
+ * @_type: Firmware *attribute* type.
+ *
+ * Example:
+ *
+ * static int example_read(...) {...}
+ * static int example_write(...) {...}
+ *
+ * DEFINE_SIMPLE_FWAT_OPS(example, ...);
+ */
+#define DEFINE_SIMPLE_FWAT_OPS(_name, _type) \
+ static const struct fwat_attr_ops _name##_ops = { \
+ ._type##_read = _name##_read, \
+ ._type##_write = _name##_write, \
+ }
+
+/**
+ * DEFINE_FWAT_OPS() - Define static &struct fwat_attr_ops with all callbacks.
+ * @_name: Prefix of the `read` and `write` callbacks.
+ * @_type: Firmware *attribute* type.
+ *
+ * Example:
+ *
+ * static int example_read(...) {...}
+ * static int example_write(...) {...}
+ * static int example_prop_read(...) {...}
+ *
+ * DEFINE_FWAT_OPS(example, ...);
+ */
+#define DEFINE_FWAT_OPS(_name, _type) \
+ static const struct fwat_attr_ops _name##_ops = { \
+ .prop_read = _name##_prop_read, \
+ ._type##_read = _name##_read, \
+ ._type##_write = _name##_write, \
+ }
+
+/**
+ * FWAT_CONFIG() - Configuration pointer for a single firmware *attribute*.
+ * @_name: String name of this *attribute*.
+ * @_type: Firmware *attribute* type.
+ * @_ops: Pointer to &struct fwat_attr_ops.
+ * @_props: Pointer to a enum fwat_property array.
+ * @_num_props: Size of the @_props array.
+ *
+ * This is a convenience macro to quickly construct a &struct fwat_attr_config
+ * array, which will be passed to &struct fwat_dev_config.
+ *
+ * Example:
+ *
+ * static int example_read(...) {...}
+ * static int example_write(...) {...}
+ * static int example_prop_read(...) {...}
+ *
+ * DEFINE_FWAT_OPS(example, ...);
+ *
+ * static const enum fwat_property props[] = {...};
+ *
+ * static const struct fwat_attr_config * const attrs_config[] = {
+ * FWAT_CONFIG(example, ..., &example_fwat_ops, props,
+ * ARRAY_SIZE(props)),
+ * ...
+ * NULL
+ * };
+ *
+ * static const struct fwat_dev_config fdev_config = {
+ * .attrs_config = attrs_config,
+ * ...
+ * }
+ */
+#define FWAT_CONFIG(_name, _type, _ops, _props, _num_props) \
+ (&(const struct fwat_attr_config) { \
+ .name = _name, \
+ .type = fwat_type_##_type, \
+ .ops = _ops, \
+ .num_props = _num_props, \
+ .props = _props, \
+ })
+
+/**
+ * FWAT_CONFIG_AUX() - Configuration pointer for a single firmware *attribute*
+ * with an auxiliary number defined by the user
+ * @_name: String name of this *attribute*.
+ * @_type: Firmware *attribute* type.
+ * @_aux: Auxiliary number defined by the user.
+ * @_ops: Pointer to &struct fwat_attr_ops.
+ * @_props: Pointer to a enum fwat_property array.
+ * @_num_props: Size of the @_props array.
+ *
+ * This is a convenience macro to quickly construct a &struct fwat_attr_config
+ * array, which will be passed to &struct fwat_dev_config.
+ *
+ * Example:
+ *
+ * static int example_read(...) {...}
+ * static int example_write(...) {...}
+ * static int example_prop_read(...) {...}
+ *
+ * DEFINE_FWAT_OPS(example, ...);
+ *
+ * static const enum fwat_property props[] = {...};
+ *
+ * static const struct fwat_attr_config * const config[] = {
+ * FWAT_CONFIG_AUX(example, ..., n, &example_fwat_ops, props,
+ * ARRAY_SIZE(props)),
+ * ...
+ * NULL
+ * };
+ *
+ * static const struct fwat_dev_config fdev_config = {
+ * .attrs_config = attrs_config,
+ * ...
+ * }
+ */
+#define FWAT_CONFIG_AUX(_name, _type, _aux, _ops, _props, _num_props) \
+ (&(const struct fwat_attr_config) { \
+ .name = _name, \
+ .type = fwat_type_##_type, \
+ .aux = _aux, \
+ .ops = _ops, \
+ .num_props = _num_props, \
+ .props = _props, \
+ })
+
+/**
+ * struct fwat_dev_config - Configuration for this devices's
+ * &fwat_device.auto_groups
+ * @attrs_config: NULL terminated &struct fwat_attr_config array.
+ * @is_visible: Optional visibility callback to determine the visibility
+ * of each auto_group.
+ */
+struct fwat_dev_config {
+ const struct fwat_attr_config *const *attrs_config;
+ bool (*is_visible)(struct device *dev, const struct fwat_attr_config *config);
+};
+
struct fwat_device * __must_check
fwat_device_register(struct device *parent, const char *name, void *data,
+ const struct fwat_dev_config *config,
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 fwat_dev_config *config,
const struct attribute_group **groups);
#endif /* FW_ATTR_CLASS_H */
--
2.49.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH RFC 2/5] platform/x86: firmware_attributes_class: Add a high level API
2025-05-09 7:48 ` [PATCH RFC 2/5] platform/x86: firmware_attributes_class: Add a high level API Kurt Borja
@ 2025-05-09 15:58 ` Mario Limonciello
2025-05-09 19:56 ` Kurt Borja
0 siblings, 1 reply; 12+ messages in thread
From: Mario Limonciello @ 2025-05-09 15:58 UTC (permalink / raw)
To: Kurt Borja, Hans de Goede, Ilpo Järvinen,
Thomas Weißschuh, Joshua Grisham, Mark Pearson, Armin Wolf
Cc: Antheas Kapenekakis, Derek J. Clark, Prasanth Ksr, Jorge Lopez,
platform-driver-x86, linux-kernel, Dell.Client.Kernel
On 5/9/2025 2:48 AM, Kurt Borja wrote:
> Add an attribute configuration mechanism through the newly introduced
> `struct fwat_dev_config`, which makes use of other documented structs
> and callbacks.
>
> This API aims to be simple, yet flexible. In order to accomplish this,
> the following features were taken into account:
>
> * Ability to statically define attributes
> * Custom read/write callbacks for each attribute type
> * Ability to map attributes to numbers in order to differentiate them in
> callbacks (`aux` number)
> * Ability to reuse read/write callbacks in different attributes
> * Ability to reuse property selection in different attributes
> * Optional visibility callback for dynamic attribute visibility
>
> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
> ---
> drivers/platform/x86/firmware_attributes_class.c | 249 ++++++++++++++++++++++-
> drivers/platform/x86/firmware_attributes_class.h | 228 +++++++++++++++++++++
> 2 files changed, 474 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/platform/x86/firmware_attributes_class.c b/drivers/platform/x86/firmware_attributes_class.c
> index 58ab1495ba3bd449cfe17de2827a57a0c5937788..7cfb0f49f235728c7450a82a7e9d00b8963d3dea 100644
> --- a/drivers/platform/x86/firmware_attributes_class.c
> +++ b/drivers/platform/x86/firmware_attributes_class.c
> @@ -7,14 +7,233 @@
> #include <linux/kobject.h>
> #include <linux/module.h>
> #include <linux/slab.h>
> +#include <linux/sysfs.h>
> #include <linux/types.h>
> #include "firmware_attributes_class.h"
>
> +#define to_fwat_attribute_ext(_a) container_of_const(_a, struct fwat_attribute_ext, attr)
> +
> +struct fwat_attribute_ext {
> + struct fwat_attribute attr;
> + enum fwat_property prop;
> + const struct fwat_attr_config *config;
> +};
> +
> const struct class firmware_attributes_class = {
> .name = "firmware-attributes",
> };
> EXPORT_SYMBOL_GPL(firmware_attributes_class);
>
> +static const char * const fwat_type_labels[] = {
> + [fwat_type_integer] = "integer",
> + [fwat_type_string] = "string",
> + [fwat_type_enumeration] = "enumeration",
> +};
> +
> +static const char * const fwat_prop_labels[] = {
> + [FWAT_PROP_DISPLAY_NAME] = "display_name",
> + [FWAT_PROP_LANGUAGE_CODE] = "display_name_language_code",
> + [FWAT_PROP_DEFAULT] = "default",
> +
> + [FWAT_INT_PROP_MIN] = "min_value",
> + [FWAT_INT_PROP_MAX] = "max_value",
> + [FWAT_INT_PROP_INCREMENT] = "scalar_increment",
> +
> + [FWAT_STR_PROP_MIN] = "min_length",
> + [FWAT_STR_PROP_MAX] = "max_length",
> +
> + [FWAT_ENUM_PROP_POSSIBLE_VALUES] = "possible_values",
> +};
> +
> +static ssize_t
> +fwat_type_show(struct device *dev, const struct fwat_attribute *attr, char *buf)
> +{
> + const struct fwat_attribute_ext *ext = to_fwat_attribute_ext(attr);
> + const struct fwat_attr_config *config = ext->config;
> +
> + return sysfs_emit(buf, "%s\n", fwat_type_labels[config->type]);
> +}
> +
> +static ssize_t
> +fwat_property_show(struct device *dev, const struct fwat_attribute *attr, char *buf)
> +{
> + const struct fwat_attribute_ext *ext = to_fwat_attribute_ext(attr);
> + const struct fwat_attr_config *config = ext->config;
> +
> + if (!config->ops->prop_read)
> + return -EOPNOTSUPP;
> +
> + return config->ops->prop_read(dev, config->aux, ext->prop, buf);
> +}
> +
> +static ssize_t
> +fwat_current_value_show(struct device *dev, const struct fwat_attribute *attr, char *buf)
> +{
> + const struct fwat_attribute_ext *ext = to_fwat_attribute_ext(attr);
> + const struct fwat_attr_config *config = ext->config;
> + const char *str;
> + long int_val;
> + int ret;
> +
> + switch (config->type) {
> + case fwat_type_integer:
> + ret = config->ops->integer_read(dev, config->aux, &int_val);
> + if (ret)
> + return ret;
> +
> + return sysfs_emit(buf, "%ld\n", int_val);
> + case fwat_type_string:
> + ret = config->ops->string_read(dev, config->aux, &str);
> + if (ret)
> + return ret;
> +
> + return sysfs_emit(buf, "%s\n", str);
> + case fwat_type_enumeration:
> + ret = config->ops->enumeration_read(dev, config->aux, &str);
> + if (ret)
> + return ret;
> +
> + return sysfs_emit(buf, "%s\n", str);
> + default:
> + return -EOPNOTSUPP;
> + }
> +}
> +
> +static ssize_t
> +fwat_current_value_store(struct device *dev, const struct fwat_attribute *attr,
> + const char *buf, size_t count)
> +{
> + const struct fwat_attribute_ext *ext = to_fwat_attribute_ext(attr);
> + const struct fwat_attr_config *config = ext->config;
> + long int_val;
> + int ret;
> +
> + switch (config->type) {
> + case fwat_type_integer:
> + ret = kstrtol(buf, 0, &int_val);
> + if (ret)
> + return ret;
> +
> + ret = config->ops->integer_write(dev, config->aux, int_val);
> + break;
> + case fwat_type_string:
> + ret = config->ops->string_write(dev, config->aux, buf);
> + break;
> + case fwat_type_enumeration:
> + ret = config->ops->enumeration_write(dev, config->aux, buf);
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> +
> + return ret ? ret : count;
> +}
> +
> +static struct attribute *
> +fwat_alloc_attr(struct device *dev, const struct fwat_attr_config *config,
> + const char *attr_name, umode_t mode, enum fwat_property prop,
> + ssize_t (*show)(struct device *dev, const struct fwat_attribute *attr,
> + char *buf),
> + ssize_t (*store)(struct device *dev, const struct fwat_attribute *attr,
> + const char *buf, size_t count))
> +{
> + struct fwat_attribute_ext *fattr;
> +
> + fattr = devm_kzalloc(dev, sizeof(*fattr), GFP_KERNEL);
> + if (!fattr)
> + return NULL;
> +
> + fattr->attr.attr.name = attr_name;
> + fattr->attr.attr.mode = mode;
> + fattr->attr.show = show;
> + fattr->attr.store = store;
> + fattr->prop = prop;
> + fattr->config = config;
> + sysfs_attr_init(&fattr->attr.attr);
> +
> + return &fattr->attr.attr;
> +}
> +
> +static struct attribute **
> +fwat_create_attrs(struct device *dev, const struct fwat_attr_config *config)
> +{
> + struct attribute **attrs;
> + enum fwat_property prop;
> + unsigned int index = 0;
> +
> + attrs = devm_kcalloc(dev, config->num_props + 3, sizeof(*attrs), GFP_KERNEL);
> + if (!attrs)
> + return NULL;
> +
> + /*
> + * Create optional attributes
> + */
Just a nit here; this probably doesn't need to be a multiline comment.
a single line like this is fine:
/* optional attributes */
> + for (; index < config->num_props; index++) {
> + prop = config->props[index];
> + attrs[index] = fwat_alloc_attr(dev, config, fwat_prop_labels[prop],
> + 0444, prop, fwat_property_show, NULL);
> + }
> +
> + /*
> + * Create mandatory attributes
> + */
Same as above
> + attrs[index++] = fwat_alloc_attr(dev, config, "type", 0444, 0, fwat_type_show, NULL);
> + attrs[index++] = fwat_alloc_attr(dev, config, "current_value", 0644, 0,
Is this permission right? Some attributes could be considered more
sensitive can't they?
> + fwat_current_value_show, fwat_current_value_store);
> +
> + return attrs;
> +}
> +
> +static const struct attribute_group *
> +fwat_create_group(struct device *dev, const struct fwat_attr_config *config)
> +{
> + struct attribute_group *group;
> + struct attribute **attrs;
> +
> + group = devm_kzalloc(dev, sizeof(*group), GFP_KERNEL);
> + if (!group)
> + return NULL;
> +
> + attrs = fwat_create_attrs(dev, config);
> + if (!attrs)
> + return NULL;
> +
> + group->name = config->name;
> + group->attrs = attrs;
> +
> + return group;
> +}
> +
> +static const struct attribute_group **
> +fwat_create_auto_groups(struct device *dev, const struct fwat_dev_config *config)
> +{
> + const struct attribute_group **groups;
> + const struct attribute_group *grp;
> + unsigned int index = 0;
> + size_t ngroups = 0;
> +
> + while (config->attrs_config[ngroups])
> + ngroups++;
> +
> + groups = devm_kcalloc(dev, ngroups + 1, sizeof(*groups), GFP_KERNEL);
> + if (!groups)
> + return NULL;
> +
> + for (unsigned int i = 0; i < ngroups; i++) {
> + if (config->is_visible &&
> + !config->is_visible(dev, config->attrs_config[i]))
> + continue;
> +
> + grp = fwat_create_group(dev, config->attrs_config[i]);
> + if (!grp)
> + return NULL;
> +
> + groups[index++] = grp;
> + }
> +
> + return groups;
> +}
> +
> static ssize_t fwat_attrs_kobj_show(struct kobject *kobj, struct attribute *attr,
> char *buf)
> {
> @@ -61,6 +280,7 @@ static const struct kobj_type fwat_attrs_ktype = {
> * device
> * @parent: Parent device
> * @name: Name of the class device
> + * @config: Device configuration
> * @data: Drvdata of the class device
> * @groups: Sysfs groups for the custom `fwat_attrs_ktype` kobj_type
> *
> @@ -72,8 +292,10 @@ static const struct kobj_type fwat_attrs_ktype = {
> */
> struct fwat_device *
> fwat_device_register(struct device *parent, const char *name, void *data,
> + const struct fwat_dev_config *config,
> const struct attribute_group **groups)
> {
> + const struct attribute_group **auto_groups;
> struct fwat_device *fadev;
> struct device *dev;
> int ret;
> @@ -97,19 +319,36 @@ fwat_device_register(struct device *parent, const char *name, void *data,
> if (ret)
> goto out_kobj_put;
>
> - if (groups) {
> - ret = sysfs_create_groups(&fadev->attrs_kobj, groups);
> + if (config) {
> + auto_groups = fwat_create_auto_groups(dev, config);
> + if (!auto_groups) {
> + ret = -ENOMEM;
> + goto out_kobj_unregister;
> + }
> +
> + ret = sysfs_create_groups(&fadev->attrs_kobj, auto_groups);
> if (ret)
> goto out_kobj_unregister;
> }
>
> + if (groups) {
> + ret = sysfs_create_groups(&fadev->attrs_kobj, groups);
> + if (ret)
> + goto out_remove_auto_groups;
> + }
> +
> fadev->dev = dev;
> fadev->groups = groups;
> + fadev->auto_groups = groups;
>
> kobject_uevent(&fadev->attrs_kobj, KOBJ_ADD);
>
> return fadev;
>
> +out_remove_auto_groups:
> + if (config)
> + sysfs_remove_groups(&fadev->attrs_kobj, auto_groups);
> +
> out_kobj_unregister:
> kobject_del(&fadev->attrs_kobj);
>
> @@ -125,6 +364,8 @@ void fwat_device_unregister(struct fwat_device *fwadev)
> {
> if (fwadev->groups)
> sysfs_remove_groups(&fwadev->attrs_kobj, fwadev->groups);
> + if (fwadev->auto_groups)
> + sysfs_remove_groups(&fwadev->attrs_kobj, fwadev->auto_groups);
> kobject_del(&fwadev->attrs_kobj);
> kobject_put(&fwadev->attrs_kobj);
> device_unregister(fwadev->dev);
> @@ -143,6 +384,7 @@ static void devm_fwat_device_release(void *data)
> * device
> * @parent: Parent device
> * @name: Name of the class device
> + * @config: Device configuration
> * @data: Drvdata of the class device
> * @groups: Sysfs groups for the custom `fwat_attrs_ktype` kobj_type
> *
> @@ -156,12 +398,13 @@ static void devm_fwat_device_release(void *data)
> */
> struct fwat_device *
> devm_fwat_device_register(struct device *parent, const char *name, void *data,
> + const struct fwat_dev_config *config,
> const struct attribute_group **groups)
> {
> struct fwat_device *fadev;
> int ret;
>
> - fadev = fwat_device_register(parent, name, data, groups);
> + fadev = fwat_device_register(parent, name, data, config, groups);
> if (IS_ERR(fadev))
> return fadev;
>
> diff --git a/drivers/platform/x86/firmware_attributes_class.h b/drivers/platform/x86/firmware_attributes_class.h
> index ad94bf91e5af30a2b8feb9abf224ee6f0d17600a..f250875d785c3439682c43c693f98e1c20e9ff54 100644
> --- a/drivers/platform/x86/firmware_attributes_class.h
> +++ b/drivers/platform/x86/firmware_attributes_class.h
> @@ -18,11 +18,14 @@ extern const struct class firmware_attributes_class;
> * @dev: The class device.
> * @attrs_kobj: The "attributes" root kobject.
> * @groups: Sysfs groups attached to the @attrs_kobj.
> + * @auto_groups: Sysgs groups generated from &struct fwat_attr_config attached.
> + * to the @attrs_kobj
> */
> struct fwat_device {
> struct device *dev;
> struct kobject attrs_kobj;
> const struct attribute_group **groups;
> + const struct attribute_group **auto_groups;
> };
>
> #define to_fwat_device(_k) container_of_const(_k, struct fwat_device, attrs_kobj)
> @@ -30,6 +33,7 @@ struct fwat_device {
> /**
> * struct fwat_attribute - The firmware-attributes's custom attribute
> * @attr: Embedded struct attribute.
> + * @aux: Auxiliary number defined by the user.
> * @show: Show method called by the "attributes" kobject's ktype.
> * @store: Store method called by the "attributes" kobject's ktype.
> */
> @@ -43,14 +47,238 @@ struct fwat_attribute {
>
> #define to_fwat_attribute(_a) container_of_const(_a, struct fwat_attribute, attr)
>
> +enum fwat_attr_type {
> + fwat_type_integer,
> + fwat_type_string,
> + fwat_type_enumeration,
> +};
> +
> +enum fwat_property {
> + FWAT_PROP_DISPLAY_NAME,
> + FWAT_PROP_LANGUAGE_CODE,
> + FWAT_PROP_DEFAULT,
> +
> + FWAT_INT_PROP_MIN,
> + FWAT_INT_PROP_MAX,
> + FWAT_INT_PROP_INCREMENT,
> +
> + FWAT_STR_PROP_MIN,
> + FWAT_STR_PROP_MAX,
> +
> + FWAT_ENUM_PROP_POSSIBLE_VALUES,
> +};
> +
> +struct fwat_attr_config;
> +
> +/**
> + * struct fwat_attr_ops - Operations for a firmware *attribute*
> + * @prop_read: Callback for retrieving each configured property of an attribute.
> + * @integer_read: Callback for reading the current_value of an attribute of
> + * type *integer*.
> + * @integer_write: Callback for writing the current_value of an attribute of
> + * type *integer*.
> + * @string_read: Callback for reading the current_value of an attribute of type
> + * *string*.
> + * @string_write: Callback for writing the current_value of an attribute of type
> + * *string*.
> + * @enumeration_read: Callback for reading the current_value of an attribute of
> + * type *enumeration*.
> + * @enumeration_write: Callback for writing the current_value of an attribute
> + * of type *enumeration*.
> + */
> +struct fwat_attr_ops {
> + ssize_t (*prop_read)(struct device *dev, long aux,
> + enum fwat_property prop, const char *buf);
> + union {
> + struct {
> + int (*integer_read)(struct device *dev, long aux,
> + long *val);
> + int (*integer_write)(struct device *dev, long aux,
> + long val);
> + };
> + struct {
> + int (*string_read)(struct device *dev, long aux,
> + const char **str);
> + int (*string_write)(struct device *dev, long aux,
> + const char *str);
> + };
> + struct {
> + int (*enumeration_read)(struct device *dev, long aux,
> + const char **str);
> + int (*enumeration_write)(struct device *dev, long aux,
> + const char *str);
> + };
> + };
> +};
> +
> +/**
> + * struct fwat_attr_config - Configuration for a single firmware *attribute*
> + * @name: Name of the sysfs group associated with this *attribute*.
> + * @type: Type of this *attribute*.
> + * @aux: Auxiliary number defined by the user, which will be passed to
> + * read/write callbacks.
> + * @ops: Operations for this *attribute*.
> + * @props: Array of properties of this *attribute*.
> + * @num_props: Size of the props array.
> + */
> +struct fwat_attr_config {
> + const char *name;
> + enum fwat_attr_type type;
> + long aux;
> + const struct fwat_attr_ops *ops;
> + const enum fwat_property *props;
> + size_t num_props;
> +};
> +
> +/**
> + * DEFINE_SIMPLE_FWAT_OPS() - Define static &struct fwat_attr_ops for a simple
> + * *attribute* with no properties, i.e. No
> + * &fwat_attr_ops.read_prop callback
> + * @_name: Prefix of the `read` and `write` callbacks.
> + * @_type: Firmware *attribute* type.
> + *
> + * Example:
> + *
> + * static int example_read(...) {...}
> + * static int example_write(...) {...}
> + *
> + * DEFINE_SIMPLE_FWAT_OPS(example, ...);
> + */
> +#define DEFINE_SIMPLE_FWAT_OPS(_name, _type) \
> + static const struct fwat_attr_ops _name##_ops = { \
> + ._type##_read = _name##_read, \
> + ._type##_write = _name##_write, \
> + }
> +
> +/**
> + * DEFINE_FWAT_OPS() - Define static &struct fwat_attr_ops with all callbacks.
> + * @_name: Prefix of the `read` and `write` callbacks.
> + * @_type: Firmware *attribute* type.
> + *
> + * Example:
> + *
> + * static int example_read(...) {...}
> + * static int example_write(...) {...}
> + * static int example_prop_read(...) {...}
> + *
> + * DEFINE_FWAT_OPS(example, ...);
> + */
> +#define DEFINE_FWAT_OPS(_name, _type) \
> + static const struct fwat_attr_ops _name##_ops = { \
> + .prop_read = _name##_prop_read, \
> + ._type##_read = _name##_read, \
> + ._type##_write = _name##_write, \
> + }
> +
> +/**
> + * FWAT_CONFIG() - Configuration pointer for a single firmware *attribute*.
> + * @_name: String name of this *attribute*.
> + * @_type: Firmware *attribute* type.
> + * @_ops: Pointer to &struct fwat_attr_ops.
> + * @_props: Pointer to a enum fwat_property array.
> + * @_num_props: Size of the @_props array.
> + *
> + * This is a convenience macro to quickly construct a &struct fwat_attr_config
> + * array, which will be passed to &struct fwat_dev_config.
> + *
> + * Example:
> + *
> + * static int example_read(...) {...}
> + * static int example_write(...) {...}
> + * static int example_prop_read(...) {...}
> + *
> + * DEFINE_FWAT_OPS(example, ...);
> + *
> + * static const enum fwat_property props[] = {...};
> + *
> + * static const struct fwat_attr_config * const attrs_config[] = {
> + * FWAT_CONFIG(example, ..., &example_fwat_ops, props,
> + * ARRAY_SIZE(props)),
> + * ...
> + * NULL
> + * };
> + *
> + * static const struct fwat_dev_config fdev_config = {
> + * .attrs_config = attrs_config,
> + * ...
> + * }
> + */
> +#define FWAT_CONFIG(_name, _type, _ops, _props, _num_props) \
> + (&(const struct fwat_attr_config) { \
> + .name = _name, \
> + .type = fwat_type_##_type, \
> + .ops = _ops, \
> + .num_props = _num_props, \
> + .props = _props, \
> + })
> +
> +/**
> + * FWAT_CONFIG_AUX() - Configuration pointer for a single firmware *attribute*
> + * with an auxiliary number defined by the user
> + * @_name: String name of this *attribute*.
> + * @_type: Firmware *attribute* type.
> + * @_aux: Auxiliary number defined by the user.
> + * @_ops: Pointer to &struct fwat_attr_ops.
> + * @_props: Pointer to a enum fwat_property array.
> + * @_num_props: Size of the @_props array.
> + *
> + * This is a convenience macro to quickly construct a &struct fwat_attr_config
> + * array, which will be passed to &struct fwat_dev_config.
> + *
> + * Example:
> + *
> + * static int example_read(...) {...}
> + * static int example_write(...) {...}
> + * static int example_prop_read(...) {...}
> + *
> + * DEFINE_FWAT_OPS(example, ...);
> + *
> + * static const enum fwat_property props[] = {...};
> + *
> + * static const struct fwat_attr_config * const config[] = {
> + * FWAT_CONFIG_AUX(example, ..., n, &example_fwat_ops, props,
> + * ARRAY_SIZE(props)),
> + * ...
> + * NULL
> + * };
> + *
> + * static const struct fwat_dev_config fdev_config = {
> + * .attrs_config = attrs_config,
> + * ...
> + * }
> + */
> +#define FWAT_CONFIG_AUX(_name, _type, _aux, _ops, _props, _num_props) \
> + (&(const struct fwat_attr_config) { \
> + .name = _name, \
> + .type = fwat_type_##_type, \
> + .aux = _aux, \
> + .ops = _ops, \
> + .num_props = _num_props, \
> + .props = _props, \
> + })
> +
> +/**
> + * struct fwat_dev_config - Configuration for this devices's
> + * &fwat_device.auto_groups
> + * @attrs_config: NULL terminated &struct fwat_attr_config array.
> + * @is_visible: Optional visibility callback to determine the visibility
> + * of each auto_group.
> + */
> +struct fwat_dev_config {
> + const struct fwat_attr_config *const *attrs_config;
> + bool (*is_visible)(struct device *dev, const struct fwat_attr_config *config);
> +};
> +
> struct fwat_device * __must_check
> fwat_device_register(struct device *parent, const char *name, void *data,
> + const struct fwat_dev_config *config,
> 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 fwat_dev_config *config,
> const struct attribute_group **groups);
>
> #endif /* FW_ATTR_CLASS_H */
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH RFC 2/5] platform/x86: firmware_attributes_class: Add a high level API
2025-05-09 15:58 ` Mario Limonciello
@ 2025-05-09 19:56 ` Kurt Borja
2025-05-09 20:00 ` Mario Limonciello
0 siblings, 1 reply; 12+ messages in thread
From: Kurt Borja @ 2025-05-09 19:56 UTC (permalink / raw)
To: Mario Limonciello, Hans de Goede, Ilpo Järvinen,
Thomas Weißschuh, Joshua Grisham, Mark Pearson, Armin Wolf
Cc: Antheas Kapenekakis, Derek J. Clark, Prasanth Ksr, Jorge Lopez,
platform-driver-x86, linux-kernel, Dell.Client.Kernel
[-- Attachment #1: Type: text/plain, Size: 22198 bytes --]
Hi Mario,
On Fri May 9, 2025 at 12:58 PM -03, Mario Limonciello wrote:
> On 5/9/2025 2:48 AM, Kurt Borja wrote:
>> Add an attribute configuration mechanism through the newly introduced
>> `struct fwat_dev_config`, which makes use of other documented structs
>> and callbacks.
>>
>> This API aims to be simple, yet flexible. In order to accomplish this,
>> the following features were taken into account:
>>
>> * Ability to statically define attributes
>> * Custom read/write callbacks for each attribute type
>> * Ability to map attributes to numbers in order to differentiate them in
>> callbacks (`aux` number)
>> * Ability to reuse read/write callbacks in different attributes
>> * Ability to reuse property selection in different attributes
>> * Optional visibility callback for dynamic attribute visibility
>>
>> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
>> ---
>> drivers/platform/x86/firmware_attributes_class.c | 249 ++++++++++++++++++++++-
>> drivers/platform/x86/firmware_attributes_class.h | 228 +++++++++++++++++++++
>> 2 files changed, 474 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/platform/x86/firmware_attributes_class.c b/drivers/platform/x86/firmware_attributes_class.c
>> index 58ab1495ba3bd449cfe17de2827a57a0c5937788..7cfb0f49f235728c7450a82a7e9d00b8963d3dea 100644
>> --- a/drivers/platform/x86/firmware_attributes_class.c
>> +++ b/drivers/platform/x86/firmware_attributes_class.c
>> @@ -7,14 +7,233 @@
>> #include <linux/kobject.h>
>> #include <linux/module.h>
>> #include <linux/slab.h>
>> +#include <linux/sysfs.h>
>> #include <linux/types.h>
>> #include "firmware_attributes_class.h"
>>
>> +#define to_fwat_attribute_ext(_a) container_of_const(_a, struct fwat_attribute_ext, attr)
>> +
>> +struct fwat_attribute_ext {
>> + struct fwat_attribute attr;
>> + enum fwat_property prop;
>> + const struct fwat_attr_config *config;
>> +};
>> +
>> const struct class firmware_attributes_class = {
>> .name = "firmware-attributes",
>> };
>> EXPORT_SYMBOL_GPL(firmware_attributes_class);
>>
>> +static const char * const fwat_type_labels[] = {
>> + [fwat_type_integer] = "integer",
>> + [fwat_type_string] = "string",
>> + [fwat_type_enumeration] = "enumeration",
>> +};
>> +
>> +static const char * const fwat_prop_labels[] = {
>> + [FWAT_PROP_DISPLAY_NAME] = "display_name",
>> + [FWAT_PROP_LANGUAGE_CODE] = "display_name_language_code",
>> + [FWAT_PROP_DEFAULT] = "default",
>> +
>> + [FWAT_INT_PROP_MIN] = "min_value",
>> + [FWAT_INT_PROP_MAX] = "max_value",
>> + [FWAT_INT_PROP_INCREMENT] = "scalar_increment",
>> +
>> + [FWAT_STR_PROP_MIN] = "min_length",
>> + [FWAT_STR_PROP_MAX] = "max_length",
>> +
>> + [FWAT_ENUM_PROP_POSSIBLE_VALUES] = "possible_values",
>> +};
>> +
>> +static ssize_t
>> +fwat_type_show(struct device *dev, const struct fwat_attribute *attr, char *buf)
>> +{
>> + const struct fwat_attribute_ext *ext = to_fwat_attribute_ext(attr);
>> + const struct fwat_attr_config *config = ext->config;
>> +
>> + return sysfs_emit(buf, "%s\n", fwat_type_labels[config->type]);
>> +}
>> +
>> +static ssize_t
>> +fwat_property_show(struct device *dev, const struct fwat_attribute *attr, char *buf)
>> +{
>> + const struct fwat_attribute_ext *ext = to_fwat_attribute_ext(attr);
>> + const struct fwat_attr_config *config = ext->config;
>> +
>> + if (!config->ops->prop_read)
>> + return -EOPNOTSUPP;
>> +
>> + return config->ops->prop_read(dev, config->aux, ext->prop, buf);
>> +}
>> +
>> +static ssize_t
>> +fwat_current_value_show(struct device *dev, const struct fwat_attribute *attr, char *buf)
>> +{
>> + const struct fwat_attribute_ext *ext = to_fwat_attribute_ext(attr);
>> + const struct fwat_attr_config *config = ext->config;
>> + const char *str;
>> + long int_val;
>> + int ret;
>> +
>> + switch (config->type) {
>> + case fwat_type_integer:
>> + ret = config->ops->integer_read(dev, config->aux, &int_val);
>> + if (ret)
>> + return ret;
>> +
>> + return sysfs_emit(buf, "%ld\n", int_val);
>> + case fwat_type_string:
>> + ret = config->ops->string_read(dev, config->aux, &str);
>> + if (ret)
>> + return ret;
>> +
>> + return sysfs_emit(buf, "%s\n", str);
>> + case fwat_type_enumeration:
>> + ret = config->ops->enumeration_read(dev, config->aux, &str);
>> + if (ret)
>> + return ret;
>> +
>> + return sysfs_emit(buf, "%s\n", str);
>> + default:
>> + return -EOPNOTSUPP;
>> + }
>> +}
>> +
>> +static ssize_t
>> +fwat_current_value_store(struct device *dev, const struct fwat_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + const struct fwat_attribute_ext *ext = to_fwat_attribute_ext(attr);
>> + const struct fwat_attr_config *config = ext->config;
>> + long int_val;
>> + int ret;
>> +
>> + switch (config->type) {
>> + case fwat_type_integer:
>> + ret = kstrtol(buf, 0, &int_val);
>> + if (ret)
>> + return ret;
>> +
>> + ret = config->ops->integer_write(dev, config->aux, int_val);
>> + break;
>> + case fwat_type_string:
>> + ret = config->ops->string_write(dev, config->aux, buf);
>> + break;
>> + case fwat_type_enumeration:
>> + ret = config->ops->enumeration_write(dev, config->aux, buf);
>> + break;
>> + default:
>> + return -EOPNOTSUPP;
>> + }
>> +
>> + return ret ? ret : count;
>> +}
>> +
>> +static struct attribute *
>> +fwat_alloc_attr(struct device *dev, const struct fwat_attr_config *config,
>> + const char *attr_name, umode_t mode, enum fwat_property prop,
>> + ssize_t (*show)(struct device *dev, const struct fwat_attribute *attr,
>> + char *buf),
>> + ssize_t (*store)(struct device *dev, const struct fwat_attribute *attr,
>> + const char *buf, size_t count))
>> +{
>> + struct fwat_attribute_ext *fattr;
>> +
>> + fattr = devm_kzalloc(dev, sizeof(*fattr), GFP_KERNEL);
>> + if (!fattr)
>> + return NULL;
>> +
>> + fattr->attr.attr.name = attr_name;
>> + fattr->attr.attr.mode = mode;
>> + fattr->attr.show = show;
>> + fattr->attr.store = store;
>> + fattr->prop = prop;
>> + fattr->config = config;
>> + sysfs_attr_init(&fattr->attr.attr);
>> +
>> + return &fattr->attr.attr;
>> +}
>> +
>> +static struct attribute **
>> +fwat_create_attrs(struct device *dev, const struct fwat_attr_config *config)
>> +{
>> + struct attribute **attrs;
>> + enum fwat_property prop;
>> + unsigned int index = 0;
>> +
>> + attrs = devm_kcalloc(dev, config->num_props + 3, sizeof(*attrs), GFP_KERNEL);
>> + if (!attrs)
>> + return NULL;
>> +
>> + /*
>> + * Create optional attributes
>> + */
> Just a nit here; this probably doesn't need to be a multiline comment.
> a single line like this is fine:
>
> /* optional attributes */
>
>> + for (; index < config->num_props; index++) {
>> + prop = config->props[index];
>> + attrs[index] = fwat_alloc_attr(dev, config, fwat_prop_labels[prop],
>> + 0444, prop, fwat_property_show, NULL);
>> + }
>> +
>> + /*
>> + * Create mandatory attributes
>> + */
>
> Same as above
Ack for these two.
>
>> + attrs[index++] = fwat_alloc_attr(dev, config, "type", 0444, 0, fwat_type_show, NULL);
>> + attrs[index++] = fwat_alloc_attr(dev, config, "current_value", 0644, 0,
>
> Is this permission right? Some attributes could be considered more
> sensitive can't they?
You are right. I assumed most drivers would want 0644 but think-lmi and
dell sysman use 0600.
I can add a mode_t mask to fwat_attr_config and use that for
current_value, while other properties would be masked by it, i.e.
`0444 & config->mode`
What do you think?
--
~ Kurt
>
>> + fwat_current_value_show, fwat_current_value_store);
>> +
>> + return attrs;
>> +}
>> +
>> +static const struct attribute_group *
>> +fwat_create_group(struct device *dev, const struct fwat_attr_config *config)
>> +{
>> + struct attribute_group *group;
>> + struct attribute **attrs;
>> +
>> + group = devm_kzalloc(dev, sizeof(*group), GFP_KERNEL);
>> + if (!group)
>> + return NULL;
>> +
>> + attrs = fwat_create_attrs(dev, config);
>> + if (!attrs)
>> + return NULL;
>> +
>> + group->name = config->name;
>> + group->attrs = attrs;
>> +
>> + return group;
>> +}
>> +
>> +static const struct attribute_group **
>> +fwat_create_auto_groups(struct device *dev, const struct fwat_dev_config *config)
>> +{
>> + const struct attribute_group **groups;
>> + const struct attribute_group *grp;
>> + unsigned int index = 0;
>> + size_t ngroups = 0;
>> +
>> + while (config->attrs_config[ngroups])
>> + ngroups++;
>> +
>> + groups = devm_kcalloc(dev, ngroups + 1, sizeof(*groups), GFP_KERNEL);
>> + if (!groups)
>> + return NULL;
>> +
>> + for (unsigned int i = 0; i < ngroups; i++) {
>> + if (config->is_visible &&
>> + !config->is_visible(dev, config->attrs_config[i]))
>> + continue;
>> +
>> + grp = fwat_create_group(dev, config->attrs_config[i]);
>> + if (!grp)
>> + return NULL;
>> +
>> + groups[index++] = grp;
>> + }
>> +
>> + return groups;
>> +}
>> +
>> static ssize_t fwat_attrs_kobj_show(struct kobject *kobj, struct attribute *attr,
>> char *buf)
>> {
>> @@ -61,6 +280,7 @@ static const struct kobj_type fwat_attrs_ktype = {
>> * device
>> * @parent: Parent device
>> * @name: Name of the class device
>> + * @config: Device configuration
>> * @data: Drvdata of the class device
>> * @groups: Sysfs groups for the custom `fwat_attrs_ktype` kobj_type
>> *
>> @@ -72,8 +292,10 @@ static const struct kobj_type fwat_attrs_ktype = {
>> */
>> struct fwat_device *
>> fwat_device_register(struct device *parent, const char *name, void *data,
>> + const struct fwat_dev_config *config,
>> const struct attribute_group **groups)
>> {
>> + const struct attribute_group **auto_groups;
>> struct fwat_device *fadev;
>> struct device *dev;
>> int ret;
>> @@ -97,19 +319,36 @@ fwat_device_register(struct device *parent, const char *name, void *data,
>> if (ret)
>> goto out_kobj_put;
>>
>> - if (groups) {
>> - ret = sysfs_create_groups(&fadev->attrs_kobj, groups);
>> + if (config) {
>> + auto_groups = fwat_create_auto_groups(dev, config);
>> + if (!auto_groups) {
>> + ret = -ENOMEM;
>> + goto out_kobj_unregister;
>> + }
>> +
>> + ret = sysfs_create_groups(&fadev->attrs_kobj, auto_groups);
>> if (ret)
>> goto out_kobj_unregister;
>> }
>>
>> + if (groups) {
>> + ret = sysfs_create_groups(&fadev->attrs_kobj, groups);
>> + if (ret)
>> + goto out_remove_auto_groups;
>> + }
>> +
>> fadev->dev = dev;
>> fadev->groups = groups;
>> + fadev->auto_groups = groups;
>>
>> kobject_uevent(&fadev->attrs_kobj, KOBJ_ADD);
>>
>> return fadev;
>>
>> +out_remove_auto_groups:
>> + if (config)
>> + sysfs_remove_groups(&fadev->attrs_kobj, auto_groups);
>> +
>> out_kobj_unregister:
>> kobject_del(&fadev->attrs_kobj);
>>
>> @@ -125,6 +364,8 @@ void fwat_device_unregister(struct fwat_device *fwadev)
>> {
>> if (fwadev->groups)
>> sysfs_remove_groups(&fwadev->attrs_kobj, fwadev->groups);
>> + if (fwadev->auto_groups)
>> + sysfs_remove_groups(&fwadev->attrs_kobj, fwadev->auto_groups);
>> kobject_del(&fwadev->attrs_kobj);
>> kobject_put(&fwadev->attrs_kobj);
>> device_unregister(fwadev->dev);
>> @@ -143,6 +384,7 @@ static void devm_fwat_device_release(void *data)
>> * device
>> * @parent: Parent device
>> * @name: Name of the class device
>> + * @config: Device configuration
>> * @data: Drvdata of the class device
>> * @groups: Sysfs groups for the custom `fwat_attrs_ktype` kobj_type
>> *
>> @@ -156,12 +398,13 @@ static void devm_fwat_device_release(void *data)
>> */
>> struct fwat_device *
>> devm_fwat_device_register(struct device *parent, const char *name, void *data,
>> + const struct fwat_dev_config *config,
>> const struct attribute_group **groups)
>> {
>> struct fwat_device *fadev;
>> int ret;
>>
>> - fadev = fwat_device_register(parent, name, data, groups);
>> + fadev = fwat_device_register(parent, name, data, config, groups);
>> if (IS_ERR(fadev))
>> return fadev;
>>
>> diff --git a/drivers/platform/x86/firmware_attributes_class.h b/drivers/platform/x86/firmware_attributes_class.h
>> index ad94bf91e5af30a2b8feb9abf224ee6f0d17600a..f250875d785c3439682c43c693f98e1c20e9ff54 100644
>> --- a/drivers/platform/x86/firmware_attributes_class.h
>> +++ b/drivers/platform/x86/firmware_attributes_class.h
>> @@ -18,11 +18,14 @@ extern const struct class firmware_attributes_class;
>> * @dev: The class device.
>> * @attrs_kobj: The "attributes" root kobject.
>> * @groups: Sysfs groups attached to the @attrs_kobj.
>> + * @auto_groups: Sysgs groups generated from &struct fwat_attr_config attached.
>> + * to the @attrs_kobj
>> */
>> struct fwat_device {
>> struct device *dev;
>> struct kobject attrs_kobj;
>> const struct attribute_group **groups;
>> + const struct attribute_group **auto_groups;
>> };
>>
>> #define to_fwat_device(_k) container_of_const(_k, struct fwat_device, attrs_kobj)
>> @@ -30,6 +33,7 @@ struct fwat_device {
>> /**
>> * struct fwat_attribute - The firmware-attributes's custom attribute
>> * @attr: Embedded struct attribute.
>> + * @aux: Auxiliary number defined by the user.
>> * @show: Show method called by the "attributes" kobject's ktype.
>> * @store: Store method called by the "attributes" kobject's ktype.
>> */
>> @@ -43,14 +47,238 @@ struct fwat_attribute {
>>
>> #define to_fwat_attribute(_a) container_of_const(_a, struct fwat_attribute, attr)
>>
>> +enum fwat_attr_type {
>> + fwat_type_integer,
>> + fwat_type_string,
>> + fwat_type_enumeration,
>> +};
>> +
>> +enum fwat_property {
>> + FWAT_PROP_DISPLAY_NAME,
>> + FWAT_PROP_LANGUAGE_CODE,
>> + FWAT_PROP_DEFAULT,
>> +
>> + FWAT_INT_PROP_MIN,
>> + FWAT_INT_PROP_MAX,
>> + FWAT_INT_PROP_INCREMENT,
>> +
>> + FWAT_STR_PROP_MIN,
>> + FWAT_STR_PROP_MAX,
>> +
>> + FWAT_ENUM_PROP_POSSIBLE_VALUES,
>> +};
>> +
>> +struct fwat_attr_config;
>> +
>> +/**
>> + * struct fwat_attr_ops - Operations for a firmware *attribute*
>> + * @prop_read: Callback for retrieving each configured property of an attribute.
>> + * @integer_read: Callback for reading the current_value of an attribute of
>> + * type *integer*.
>> + * @integer_write: Callback for writing the current_value of an attribute of
>> + * type *integer*.
>> + * @string_read: Callback for reading the current_value of an attribute of type
>> + * *string*.
>> + * @string_write: Callback for writing the current_value of an attribute of type
>> + * *string*.
>> + * @enumeration_read: Callback for reading the current_value of an attribute of
>> + * type *enumeration*.
>> + * @enumeration_write: Callback for writing the current_value of an attribute
>> + * of type *enumeration*.
>> + */
>> +struct fwat_attr_ops {
>> + ssize_t (*prop_read)(struct device *dev, long aux,
>> + enum fwat_property prop, const char *buf);
>> + union {
>> + struct {
>> + int (*integer_read)(struct device *dev, long aux,
>> + long *val);
>> + int (*integer_write)(struct device *dev, long aux,
>> + long val);
>> + };
>> + struct {
>> + int (*string_read)(struct device *dev, long aux,
>> + const char **str);
>> + int (*string_write)(struct device *dev, long aux,
>> + const char *str);
>> + };
>> + struct {
>> + int (*enumeration_read)(struct device *dev, long aux,
>> + const char **str);
>> + int (*enumeration_write)(struct device *dev, long aux,
>> + const char *str);
>> + };
>> + };
>> +};
>> +
>> +/**
>> + * struct fwat_attr_config - Configuration for a single firmware *attribute*
>> + * @name: Name of the sysfs group associated with this *attribute*.
>> + * @type: Type of this *attribute*.
>> + * @aux: Auxiliary number defined by the user, which will be passed to
>> + * read/write callbacks.
>> + * @ops: Operations for this *attribute*.
>> + * @props: Array of properties of this *attribute*.
>> + * @num_props: Size of the props array.
>> + */
>> +struct fwat_attr_config {
>> + const char *name;
>> + enum fwat_attr_type type;
>> + long aux;
>> + const struct fwat_attr_ops *ops;
>> + const enum fwat_property *props;
>> + size_t num_props;
>> +};
>> +
>> +/**
>> + * DEFINE_SIMPLE_FWAT_OPS() - Define static &struct fwat_attr_ops for a simple
>> + * *attribute* with no properties, i.e. No
>> + * &fwat_attr_ops.read_prop callback
>> + * @_name: Prefix of the `read` and `write` callbacks.
>> + * @_type: Firmware *attribute* type.
>> + *
>> + * Example:
>> + *
>> + * static int example_read(...) {...}
>> + * static int example_write(...) {...}
>> + *
>> + * DEFINE_SIMPLE_FWAT_OPS(example, ...);
>> + */
>> +#define DEFINE_SIMPLE_FWAT_OPS(_name, _type) \
>> + static const struct fwat_attr_ops _name##_ops = { \
>> + ._type##_read = _name##_read, \
>> + ._type##_write = _name##_write, \
>> + }
>> +
>> +/**
>> + * DEFINE_FWAT_OPS() - Define static &struct fwat_attr_ops with all callbacks.
>> + * @_name: Prefix of the `read` and `write` callbacks.
>> + * @_type: Firmware *attribute* type.
>> + *
>> + * Example:
>> + *
>> + * static int example_read(...) {...}
>> + * static int example_write(...) {...}
>> + * static int example_prop_read(...) {...}
>> + *
>> + * DEFINE_FWAT_OPS(example, ...);
>> + */
>> +#define DEFINE_FWAT_OPS(_name, _type) \
>> + static const struct fwat_attr_ops _name##_ops = { \
>> + .prop_read = _name##_prop_read, \
>> + ._type##_read = _name##_read, \
>> + ._type##_write = _name##_write, \
>> + }
>> +
>> +/**
>> + * FWAT_CONFIG() - Configuration pointer for a single firmware *attribute*.
>> + * @_name: String name of this *attribute*.
>> + * @_type: Firmware *attribute* type.
>> + * @_ops: Pointer to &struct fwat_attr_ops.
>> + * @_props: Pointer to a enum fwat_property array.
>> + * @_num_props: Size of the @_props array.
>> + *
>> + * This is a convenience macro to quickly construct a &struct fwat_attr_config
>> + * array, which will be passed to &struct fwat_dev_config.
>> + *
>> + * Example:
>> + *
>> + * static int example_read(...) {...}
>> + * static int example_write(...) {...}
>> + * static int example_prop_read(...) {...}
>> + *
>> + * DEFINE_FWAT_OPS(example, ...);
>> + *
>> + * static const enum fwat_property props[] = {...};
>> + *
>> + * static const struct fwat_attr_config * const attrs_config[] = {
>> + * FWAT_CONFIG(example, ..., &example_fwat_ops, props,
>> + * ARRAY_SIZE(props)),
>> + * ...
>> + * NULL
>> + * };
>> + *
>> + * static const struct fwat_dev_config fdev_config = {
>> + * .attrs_config = attrs_config,
>> + * ...
>> + * }
>> + */
>> +#define FWAT_CONFIG(_name, _type, _ops, _props, _num_props) \
>> + (&(const struct fwat_attr_config) { \
>> + .name = _name, \
>> + .type = fwat_type_##_type, \
>> + .ops = _ops, \
>> + .num_props = _num_props, \
>> + .props = _props, \
>> + })
>> +
>> +/**
>> + * FWAT_CONFIG_AUX() - Configuration pointer for a single firmware *attribute*
>> + * with an auxiliary number defined by the user
>> + * @_name: String name of this *attribute*.
>> + * @_type: Firmware *attribute* type.
>> + * @_aux: Auxiliary number defined by the user.
>> + * @_ops: Pointer to &struct fwat_attr_ops.
>> + * @_props: Pointer to a enum fwat_property array.
>> + * @_num_props: Size of the @_props array.
>> + *
>> + * This is a convenience macro to quickly construct a &struct fwat_attr_config
>> + * array, which will be passed to &struct fwat_dev_config.
>> + *
>> + * Example:
>> + *
>> + * static int example_read(...) {...}
>> + * static int example_write(...) {...}
>> + * static int example_prop_read(...) {...}
>> + *
>> + * DEFINE_FWAT_OPS(example, ...);
>> + *
>> + * static const enum fwat_property props[] = {...};
>> + *
>> + * static const struct fwat_attr_config * const config[] = {
>> + * FWAT_CONFIG_AUX(example, ..., n, &example_fwat_ops, props,
>> + * ARRAY_SIZE(props)),
>> + * ...
>> + * NULL
>> + * };
>> + *
>> + * static const struct fwat_dev_config fdev_config = {
>> + * .attrs_config = attrs_config,
>> + * ...
>> + * }
>> + */
>> +#define FWAT_CONFIG_AUX(_name, _type, _aux, _ops, _props, _num_props) \
>> + (&(const struct fwat_attr_config) { \
>> + .name = _name, \
>> + .type = fwat_type_##_type, \
>> + .aux = _aux, \
>> + .ops = _ops, \
>> + .num_props = _num_props, \
>> + .props = _props, \
>> + })
>> +
>> +/**
>> + * struct fwat_dev_config - Configuration for this devices's
>> + * &fwat_device.auto_groups
>> + * @attrs_config: NULL terminated &struct fwat_attr_config array.
>> + * @is_visible: Optional visibility callback to determine the visibility
>> + * of each auto_group.
>> + */
>> +struct fwat_dev_config {
>> + const struct fwat_attr_config *const *attrs_config;
>> + bool (*is_visible)(struct device *dev, const struct fwat_attr_config *config);
>> +};
>> +
>> struct fwat_device * __must_check
>> fwat_device_register(struct device *parent, const char *name, void *data,
>> + const struct fwat_dev_config *config,
>> 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 fwat_dev_config *config,
>> const struct attribute_group **groups);
>>
>> #endif /* FW_ATTR_CLASS_H */
>>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH RFC 2/5] platform/x86: firmware_attributes_class: Add a high level API
2025-05-09 19:56 ` Kurt Borja
@ 2025-05-09 20:00 ` Mario Limonciello
0 siblings, 0 replies; 12+ messages in thread
From: Mario Limonciello @ 2025-05-09 20:00 UTC (permalink / raw)
To: Kurt Borja, Hans de Goede, Ilpo Järvinen,
Thomas Weißschuh, Joshua Grisham, Mark Pearson, Armin Wolf
Cc: Antheas Kapenekakis, Derek J. Clark, Prasanth Ksr, Jorge Lopez,
platform-driver-x86, linux-kernel, Dell.Client.Kernel
On 5/9/2025 2:56 PM, Kurt Borja wrote:
> Hi Mario,
>
> On Fri May 9, 2025 at 12:58 PM -03, Mario Limonciello wrote:
>> On 5/9/2025 2:48 AM, Kurt Borja wrote:
>>> Add an attribute configuration mechanism through the newly introduced
>>> `struct fwat_dev_config`, which makes use of other documented structs
>>> and callbacks.
>>>
>>> This API aims to be simple, yet flexible. In order to accomplish this,
>>> the following features were taken into account:
>>>
>>> * Ability to statically define attributes
>>> * Custom read/write callbacks for each attribute type
>>> * Ability to map attributes to numbers in order to differentiate them in
>>> callbacks (`aux` number)
>>> * Ability to reuse read/write callbacks in different attributes
>>> * Ability to reuse property selection in different attributes
>>> * Optional visibility callback for dynamic attribute visibility
>>>
>>> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
>>> ---
>>> drivers/platform/x86/firmware_attributes_class.c | 249 ++++++++++++++++++++++-
>>> drivers/platform/x86/firmware_attributes_class.h | 228 +++++++++++++++++++++
>>> 2 files changed, 474 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/platform/x86/firmware_attributes_class.c b/drivers/platform/x86/firmware_attributes_class.c
>>> index 58ab1495ba3bd449cfe17de2827a57a0c5937788..7cfb0f49f235728c7450a82a7e9d00b8963d3dea 100644
>>> --- a/drivers/platform/x86/firmware_attributes_class.c
>>> +++ b/drivers/platform/x86/firmware_attributes_class.c
>>> @@ -7,14 +7,233 @@
>>> #include <linux/kobject.h>
>>> #include <linux/module.h>
>>> #include <linux/slab.h>
>>> +#include <linux/sysfs.h>
>>> #include <linux/types.h>
>>> #include "firmware_attributes_class.h"
>>>
>>> +#define to_fwat_attribute_ext(_a) container_of_const(_a, struct fwat_attribute_ext, attr)
>>> +
>>> +struct fwat_attribute_ext {
>>> + struct fwat_attribute attr;
>>> + enum fwat_property prop;
>>> + const struct fwat_attr_config *config;
>>> +};
>>> +
>>> const struct class firmware_attributes_class = {
>>> .name = "firmware-attributes",
>>> };
>>> EXPORT_SYMBOL_GPL(firmware_attributes_class);
>>>
>>> +static const char * const fwat_type_labels[] = {
>>> + [fwat_type_integer] = "integer",
>>> + [fwat_type_string] = "string",
>>> + [fwat_type_enumeration] = "enumeration",
>>> +};
>>> +
>>> +static const char * const fwat_prop_labels[] = {
>>> + [FWAT_PROP_DISPLAY_NAME] = "display_name",
>>> + [FWAT_PROP_LANGUAGE_CODE] = "display_name_language_code",
>>> + [FWAT_PROP_DEFAULT] = "default",
>>> +
>>> + [FWAT_INT_PROP_MIN] = "min_value",
>>> + [FWAT_INT_PROP_MAX] = "max_value",
>>> + [FWAT_INT_PROP_INCREMENT] = "scalar_increment",
>>> +
>>> + [FWAT_STR_PROP_MIN] = "min_length",
>>> + [FWAT_STR_PROP_MAX] = "max_length",
>>> +
>>> + [FWAT_ENUM_PROP_POSSIBLE_VALUES] = "possible_values",
>>> +};
>>> +
>>> +static ssize_t
>>> +fwat_type_show(struct device *dev, const struct fwat_attribute *attr, char *buf)
>>> +{
>>> + const struct fwat_attribute_ext *ext = to_fwat_attribute_ext(attr);
>>> + const struct fwat_attr_config *config = ext->config;
>>> +
>>> + return sysfs_emit(buf, "%s\n", fwat_type_labels[config->type]);
>>> +}
>>> +
>>> +static ssize_t
>>> +fwat_property_show(struct device *dev, const struct fwat_attribute *attr, char *buf)
>>> +{
>>> + const struct fwat_attribute_ext *ext = to_fwat_attribute_ext(attr);
>>> + const struct fwat_attr_config *config = ext->config;
>>> +
>>> + if (!config->ops->prop_read)
>>> + return -EOPNOTSUPP;
>>> +
>>> + return config->ops->prop_read(dev, config->aux, ext->prop, buf);
>>> +}
>>> +
>>> +static ssize_t
>>> +fwat_current_value_show(struct device *dev, const struct fwat_attribute *attr, char *buf)
>>> +{
>>> + const struct fwat_attribute_ext *ext = to_fwat_attribute_ext(attr);
>>> + const struct fwat_attr_config *config = ext->config;
>>> + const char *str;
>>> + long int_val;
>>> + int ret;
>>> +
>>> + switch (config->type) {
>>> + case fwat_type_integer:
>>> + ret = config->ops->integer_read(dev, config->aux, &int_val);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + return sysfs_emit(buf, "%ld\n", int_val);
>>> + case fwat_type_string:
>>> + ret = config->ops->string_read(dev, config->aux, &str);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + return sysfs_emit(buf, "%s\n", str);
>>> + case fwat_type_enumeration:
>>> + ret = config->ops->enumeration_read(dev, config->aux, &str);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + return sysfs_emit(buf, "%s\n", str);
>>> + default:
>>> + return -EOPNOTSUPP;
>>> + }
>>> +}
>>> +
>>> +static ssize_t
>>> +fwat_current_value_store(struct device *dev, const struct fwat_attribute *attr,
>>> + const char *buf, size_t count)
>>> +{
>>> + const struct fwat_attribute_ext *ext = to_fwat_attribute_ext(attr);
>>> + const struct fwat_attr_config *config = ext->config;
>>> + long int_val;
>>> + int ret;
>>> +
>>> + switch (config->type) {
>>> + case fwat_type_integer:
>>> + ret = kstrtol(buf, 0, &int_val);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + ret = config->ops->integer_write(dev, config->aux, int_val);
>>> + break;
>>> + case fwat_type_string:
>>> + ret = config->ops->string_write(dev, config->aux, buf);
>>> + break;
>>> + case fwat_type_enumeration:
>>> + ret = config->ops->enumeration_write(dev, config->aux, buf);
>>> + break;
>>> + default:
>>> + return -EOPNOTSUPP;
>>> + }
>>> +
>>> + return ret ? ret : count;
>>> +}
>>> +
>>> +static struct attribute *
>>> +fwat_alloc_attr(struct device *dev, const struct fwat_attr_config *config,
>>> + const char *attr_name, umode_t mode, enum fwat_property prop,
>>> + ssize_t (*show)(struct device *dev, const struct fwat_attribute *attr,
>>> + char *buf),
>>> + ssize_t (*store)(struct device *dev, const struct fwat_attribute *attr,
>>> + const char *buf, size_t count))
>>> +{
>>> + struct fwat_attribute_ext *fattr;
>>> +
>>> + fattr = devm_kzalloc(dev, sizeof(*fattr), GFP_KERNEL);
>>> + if (!fattr)
>>> + return NULL;
>>> +
>>> + fattr->attr.attr.name = attr_name;
>>> + fattr->attr.attr.mode = mode;
>>> + fattr->attr.show = show;
>>> + fattr->attr.store = store;
>>> + fattr->prop = prop;
>>> + fattr->config = config;
>>> + sysfs_attr_init(&fattr->attr.attr);
>>> +
>>> + return &fattr->attr.attr;
>>> +}
>>> +
>>> +static struct attribute **
>>> +fwat_create_attrs(struct device *dev, const struct fwat_attr_config *config)
>>> +{
>>> + struct attribute **attrs;
>>> + enum fwat_property prop;
>>> + unsigned int index = 0;
>>> +
>>> + attrs = devm_kcalloc(dev, config->num_props + 3, sizeof(*attrs), GFP_KERNEL);
>>> + if (!attrs)
>>> + return NULL;
>>> +
>>> + /*
>>> + * Create optional attributes
>>> + */
>> Just a nit here; this probably doesn't need to be a multiline comment.
>> a single line like this is fine:
>>
>> /* optional attributes */
>>
>>> + for (; index < config->num_props; index++) {
>>> + prop = config->props[index];
>>> + attrs[index] = fwat_alloc_attr(dev, config, fwat_prop_labels[prop],
>>> + 0444, prop, fwat_property_show, NULL);
>>> + }
>>> +
>>> + /*
>>> + * Create mandatory attributes
>>> + */
>>
>> Same as above
>
> Ack for these two.
>
>>
>>> + attrs[index++] = fwat_alloc_attr(dev, config, "type", 0444, 0, fwat_type_show, NULL);
>>> + attrs[index++] = fwat_alloc_attr(dev, config, "current_value", 0644, 0,
>>
>> Is this permission right? Some attributes could be considered more
>> sensitive can't they?
>
> You are right. I assumed most drivers would want 0644 but think-lmi and
> dell sysman use 0600.
>
> I can add a mode_t mask to fwat_attr_config and use that for
> current_value, while other properties would be masked by it, i.e.
> `0444 & config->mode`
>
> What do you think?
Yeah; current_value is the only sensitive one.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH RFC 3/5] platform/x86: firmware_attributes_class: Add a boolean type
2025-05-09 7:48 [PATCH RFC 0/5] platform/x86: firmware_attributes_class: Add a high level API Kurt Borja
2025-05-09 7:48 ` [PATCH RFC 1/5] platform/x86: firmware_attributes_class: Add device initialization methods Kurt Borja
2025-05-09 7:48 ` [PATCH RFC 2/5] platform/x86: firmware_attributes_class: Add a high level API Kurt Borja
@ 2025-05-09 7:48 ` Kurt Borja
2025-05-09 7:48 ` [PATCH RFC 4/5] MAINTAINERS: Add FIRMWARE ATTRIBUTES CLASS entry Kurt Borja
2025-05-09 7:48 ` [PATCH RFC 5/5] platform/x86: samsung-galaxybook: Transition to new firmware_attributes API Kurt Borja
4 siblings, 0 replies; 12+ messages in thread
From: Kurt Borja @ 2025-05-09 7:48 UTC (permalink / raw)
To: Hans de Goede, Ilpo Järvinen, Thomas Weißschuh,
Joshua Grisham, Mark Pearson, Armin Wolf, Mario Limonciello
Cc: Antheas Kapenekakis, Derek J. Clark, Prasanth Ksr, Jorge Lopez,
platform-driver-x86, linux-kernel, Dell.Client.Kernel, Kurt Borja
Add a `boolean` attribute type to prevent the use of the `enumeration`
type for this simple specification.
Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
.../ABI/testing/sysfs-class-firmware-attributes | 1 +
drivers/platform/x86/firmware_attributes_class.c | 16 ++++++++++++++++
drivers/platform/x86/firmware_attributes_class.h | 11 +++++++++++
3 files changed, 28 insertions(+)
diff --git a/Documentation/ABI/testing/sysfs-class-firmware-attributes b/Documentation/ABI/testing/sysfs-class-firmware-attributes
index 2713efa509b465a39bf014180794bf487e5b42d6..dc117e694416aed3f1f7ba0ebb1d0c23337b2298 100644
--- a/Documentation/ABI/testing/sysfs-class-firmware-attributes
+++ b/Documentation/ABI/testing/sysfs-class-firmware-attributes
@@ -21,6 +21,7 @@ Description:
- enumeration: a set of pre-defined valid values
- integer: a range of numerical values
- string
+ - bool
HP specific types
-----------------
diff --git a/drivers/platform/x86/firmware_attributes_class.c b/drivers/platform/x86/firmware_attributes_class.c
index 7cfb0f49f235728c7450a82a7e9d00b8963d3dea..13b10a1209be726b00fa1ebad6148778e165101e 100644
--- a/drivers/platform/x86/firmware_attributes_class.c
+++ b/drivers/platform/x86/firmware_attributes_class.c
@@ -26,6 +26,7 @@ EXPORT_SYMBOL_GPL(firmware_attributes_class);
static const char * const fwat_type_labels[] = {
[fwat_type_integer] = "integer",
+ [fwat_type_boolean] = "boolean",
[fwat_type_string] = "string",
[fwat_type_enumeration] = "enumeration",
};
@@ -72,6 +73,7 @@ fwat_current_value_show(struct device *dev, const struct fwat_attribute *attr, c
const struct fwat_attribute_ext *ext = to_fwat_attribute_ext(attr);
const struct fwat_attr_config *config = ext->config;
const char *str;
+ bool bool_val;
long int_val;
int ret;
@@ -82,6 +84,12 @@ fwat_current_value_show(struct device *dev, const struct fwat_attribute *attr, c
return ret;
return sysfs_emit(buf, "%ld\n", int_val);
+ case fwat_type_boolean:
+ ret = config->ops->boolean_read(dev, config->aux, &bool_val);
+ if (ret)
+ return ret;
+
+ return sysfs_emit(buf, "%u\n", bool_val);
case fwat_type_string:
ret = config->ops->string_read(dev, config->aux, &str);
if (ret)
@@ -105,6 +113,7 @@ fwat_current_value_store(struct device *dev, const struct fwat_attribute *attr,
{
const struct fwat_attribute_ext *ext = to_fwat_attribute_ext(attr);
const struct fwat_attr_config *config = ext->config;
+ bool bool_val;
long int_val;
int ret;
@@ -116,6 +125,13 @@ fwat_current_value_store(struct device *dev, const struct fwat_attribute *attr,
ret = config->ops->integer_write(dev, config->aux, int_val);
break;
+ case fwat_type_boolean:
+ ret = kstrtobool(buf, &bool_val);
+ if (ret)
+ return ret;
+
+ ret = config->ops->boolean_write(dev, config->aux, bool_val);
+ break;
case fwat_type_string:
ret = config->ops->string_write(dev, config->aux, buf);
break;
diff --git a/drivers/platform/x86/firmware_attributes_class.h b/drivers/platform/x86/firmware_attributes_class.h
index f250875d785c3439682c43c693f98e1c20e9ff54..d95d2024b0d9630ee3750ec26e18874965ec5cff 100644
--- a/drivers/platform/x86/firmware_attributes_class.h
+++ b/drivers/platform/x86/firmware_attributes_class.h
@@ -49,6 +49,7 @@ struct fwat_attribute {
enum fwat_attr_type {
fwat_type_integer,
+ fwat_type_boolean,
fwat_type_string,
fwat_type_enumeration,
};
@@ -77,6 +78,10 @@ struct fwat_attr_config;
* type *integer*.
* @integer_write: Callback for writing the current_value of an attribute of
* type *integer*.
+ * @boolean_read: Callback for reading the current_value of an attribute of type
+ * *boolean*.
+ * @boolean_write: Callback for writing the current_value of an attribute of
+ * type *boolean*.
* @string_read: Callback for reading the current_value of an attribute of type
* *string*.
* @string_write: Callback for writing the current_value of an attribute of type
@@ -96,6 +101,12 @@ struct fwat_attr_ops {
int (*integer_write)(struct device *dev, long aux,
long val);
};
+ struct {
+ int (*boolean_read)(struct device *dev, long aux,
+ bool *val);
+ int (*boolean_write)(struct device *dev, long aux,
+ bool val);
+ };
struct {
int (*string_read)(struct device *dev, long aux,
const char **str);
--
2.49.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH RFC 4/5] MAINTAINERS: Add FIRMWARE ATTRIBUTES CLASS entry
2025-05-09 7:48 [PATCH RFC 0/5] platform/x86: firmware_attributes_class: Add a high level API Kurt Borja
` (2 preceding siblings ...)
2025-05-09 7:48 ` [PATCH RFC 3/5] platform/x86: firmware_attributes_class: Add a boolean type Kurt Borja
@ 2025-05-09 7:48 ` Kurt Borja
2025-05-09 7:48 ` [PATCH RFC 5/5] platform/x86: samsung-galaxybook: Transition to new firmware_attributes API Kurt Borja
4 siblings, 0 replies; 12+ messages in thread
From: Kurt Borja @ 2025-05-09 7:48 UTC (permalink / raw)
To: Hans de Goede, Ilpo Järvinen, Thomas Weißschuh,
Joshua Grisham, Mark Pearson, Armin Wolf, Mario Limonciello
Cc: Antheas Kapenekakis, Derek J. Clark, Prasanth Ksr, Jorge Lopez,
platform-driver-x86, linux-kernel, Dell.Client.Kernel, Kurt Borja
Add entry for the FIRMWARE ATTRIBUTES CLASS.
Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
MAINTAINERS | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index f9417b4e9bafd1a4033bba2fdb4a4e6fb68cf219..c4adad420a8ffee7bc7624d3edc8ba36aec5cb40 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9190,6 +9190,13 @@ F: include/linux/firewire.h
F: include/uapi/linux/firewire*.h
F: tools/firewire/
+FIRMWARE ATTRIBUTES CLASS
+M: Kurt Borja <kuurtb@gmail.com>
+L: platform-driver-x86@vger.kernel.org
+S: Maintained
+F: drivers/platform/x86/firmware_attributes_class.*
+K: (devm_)?fwat_device_(un)?register
+
FIRMWARE FRAMEWORK FOR ARMV8-A
M: Sudeep Holla <sudeep.holla@arm.com>
L: linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
--
2.49.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH RFC 5/5] platform/x86: samsung-galaxybook: Transition to new firmware_attributes API
2025-05-09 7:48 [PATCH RFC 0/5] platform/x86: firmware_attributes_class: Add a high level API Kurt Borja
` (3 preceding siblings ...)
2025-05-09 7:48 ` [PATCH RFC 4/5] MAINTAINERS: Add FIRMWARE ATTRIBUTES CLASS entry Kurt Borja
@ 2025-05-09 7:48 ` Kurt Borja
2025-05-12 7:12 ` Thomas Weißschuh
4 siblings, 1 reply; 12+ messages in thread
From: Kurt Borja @ 2025-05-09 7:48 UTC (permalink / raw)
To: Hans de Goede, Ilpo Järvinen, Thomas Weißschuh,
Joshua Grisham, Mark Pearson, Armin Wolf, Mario Limonciello
Cc: Antheas Kapenekakis, Derek J. Clark, Prasanth Ksr, Jorge Lopez,
platform-driver-x86, linux-kernel, Dell.Client.Kernel, Kurt Borja
Transition to new firmware_attributes API and replace `enumeration` types
with the simpler `boolean` type.
Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
drivers/platform/x86/samsung-galaxybook.c | 299 ++++++++++++------------------
1 file changed, 114 insertions(+), 185 deletions(-)
diff --git a/drivers/platform/x86/samsung-galaxybook.c b/drivers/platform/x86/samsung-galaxybook.c
index 5878a351993eb05a4c5c2c75b4915d972ce9becc..4fe3601fa5d282e7f8ae0a9b7e973da0998a5a72 100644
--- a/drivers/platform/x86/samsung-galaxybook.c
+++ b/drivers/platform/x86/samsung-galaxybook.c
@@ -36,8 +36,6 @@ struct samsung_galaxybook {
struct platform_device *platform;
struct acpi_device *acpi;
- struct device *fw_attrs_dev;
- struct kset *fw_attrs_kset;
/* block in case firmware attributes are updated in multiple threads */
struct mutex fw_attr_lock;
@@ -66,13 +64,7 @@ enum galaxybook_fw_attr_id {
GB_ATTR_BLOCK_RECORDING,
};
-static const char * const galaxybook_fw_attr_name[] = {
- [GB_ATTR_POWER_ON_LID_OPEN] = "power_on_lid_open",
- [GB_ATTR_USB_CHARGING] = "usb_charging",
- [GB_ATTR_BLOCK_RECORDING] = "block_recording",
-};
-
-static const char * const galaxybook_fw_attr_desc[] = {
+static const char * const galaxybook_fwat_desc[] = {
[GB_ATTR_POWER_ON_LID_OPEN] = "Power On Lid Open",
[GB_ATTR_USB_CHARGING] = "USB Charging",
[GB_ATTR_BLOCK_RECORDING] = "Block Recording",
@@ -908,209 +900,146 @@ static int galaxybook_block_recording_init(struct samsung_galaxybook *galaxybook
/* Firmware Attributes setup */
-static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+static int galaxybook_fwat_read(struct device *dev, long aux, bool *val)
{
- return sysfs_emit(buf, "enumeration\n");
-}
-
-static struct kobj_attribute fw_attr_type = __ATTR_RO(type);
-
-static ssize_t default_value_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
-{
- return sysfs_emit(buf, "0\n");
-}
-
-static struct kobj_attribute fw_attr_default_value = __ATTR_RO(default_value);
-
-static ssize_t possible_values_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
-{
- return sysfs_emit(buf, "0;1\n");
-}
-
-static struct kobj_attribute fw_attr_possible_values = __ATTR_RO(possible_values);
-
-static ssize_t display_name_language_code_show(struct kobject *kobj, struct kobj_attribute *attr,
- char *buf)
-{
- return sysfs_emit(buf, "%s\n", GB_ATTR_LANGUAGE_CODE);
-}
-
-static struct kobj_attribute fw_attr_display_name_language_code =
- __ATTR_RO(display_name_language_code);
-
-static ssize_t display_name_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
-{
- struct galaxybook_fw_attr *fw_attr =
- container_of(attr, struct galaxybook_fw_attr, display_name);
-
- return sysfs_emit(buf, "%s\n", galaxybook_fw_attr_desc[fw_attr->fw_attr_id]);
-}
-
-static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
-{
- struct galaxybook_fw_attr *fw_attr =
- container_of(attr, struct galaxybook_fw_attr, current_value);
+ struct samsung_galaxybook *galaxybook = dev_get_drvdata(dev);
bool value;
int err;
- err = fw_attr->get_value(fw_attr->galaxybook, &value);
+ switch (aux) {
+ case GB_ATTR_POWER_ON_LID_OPEN:
+ err = power_on_lid_open_acpi_get(galaxybook, &value);
+ break;
+ case GB_ATTR_USB_CHARGING:
+ err = usb_charging_acpi_get(galaxybook, &value);
+ break;
+ case GB_ATTR_BLOCK_RECORDING:
+ err = block_recording_acpi_get(galaxybook, &value);
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
+
if (err)
return err;
- return sysfs_emit(buf, "%u\n", value);
+ *val = value;
+
+ return 0;
}
-static ssize_t current_value_store(struct kobject *kobj, struct kobj_attribute *attr,
- const char *buf, size_t count)
+static int galaxybook_fwat_write(struct device *dev, long aux, bool val)
{
- struct galaxybook_fw_attr *fw_attr =
- container_of(attr, struct galaxybook_fw_attr, current_value);
- struct samsung_galaxybook *galaxybook = fw_attr->galaxybook;
+ struct samsung_galaxybook *galaxybook = dev_get_drvdata(dev);
+ int err;
+
+ switch (aux) {
+ case GB_ATTR_POWER_ON_LID_OPEN:
+ err = power_on_lid_open_acpi_set(galaxybook, val);
+ break;
+ case GB_ATTR_USB_CHARGING:
+ err = usb_charging_acpi_set(galaxybook, val);
+ break;
+ case GB_ATTR_BLOCK_RECORDING:
+ err = block_recording_acpi_set(galaxybook, val);
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
+
+ return err;
+}
+
+static ssize_t galaxybook_fwat_prop_read(struct device *dev, long aux,
+ enum fwat_property prop, const char *buf)
+{
+ switch (prop) {
+ case FWAT_PROP_DISPLAY_NAME:
+ return sysfs_emit("%s\n", galaxybook_fwat_desc[aux]);
+ case FWAT_PROP_LANGUAGE_CODE:
+ return sysfs_emit("%s\n", GB_ATTR_LANGUAGE_CODE);
+ case FWAT_PROP_DEFAULT:
+ return sysfs_emit("%d\n", 0);
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+DEFINE_FWAT_OPS(galaxybook_fwat, boolean);
+
+static bool galaxybook_fwat_is_visible(struct device *dev,
+ const struct fwat_attr_config *config)
+{
+ struct samsung_galaxybook *galaxybook = dev_get_drvdata(dev);
bool value;
int err;
- if (!count)
- return -EINVAL;
+ switch (config->aux) {
+ case GB_ATTR_POWER_ON_LID_OPEN:
+ err = power_on_lid_open_acpi_get(galaxybook, &value);
+ break;
+ case GB_ATTR_USB_CHARGING:
+ err = usb_charging_acpi_get(galaxybook, &value);
+ break;
+ case GB_ATTR_BLOCK_RECORDING:
+ return galaxybook->has_block_recording;
+ default:
+ return false;
+ }
- err = kstrtobool(buf, &value);
- if (err)
- return err;
-
- guard(mutex)(&galaxybook->fw_attr_lock);
-
- err = fw_attr->set_value(galaxybook, value);
- if (err)
- return err;
-
- return count;
+ return !err;
}
-#define NUM_FW_ATTR_ENUM_ATTRS 6
+static const enum fwat_property galaxybook_fwat_props[] = {
+ FWAT_PROP_DISPLAY_NAME,
+ FWAT_PROP_LANGUAGE_CODE,
+ FWAT_PROP_DEFAULT,
+};
-static int galaxybook_fw_attr_init(struct samsung_galaxybook *galaxybook,
- const enum galaxybook_fw_attr_id fw_attr_id,
- int (*get_value)(struct samsung_galaxybook *galaxybook,
- bool *value),
- int (*set_value)(struct samsung_galaxybook *galaxybook,
- const bool value))
+static const struct fwat_attr_config * const galaxybook_fwat_config[] = {
+ FWAT_CONFIG_AUX("power_on_lid_open", boolean,
+ GB_ATTR_POWER_ON_LID_OPEN,
+ &galaxybook_fwat_ops,
+ galaxybook_fwat_props,
+ ARRAY_SIZE(galaxybook_fwat_props)),
+ FWAT_CONFIG_AUX("usb_charging", boolean,
+ GB_ATTR_USB_CHARGING,
+ &galaxybook_fwat_ops,
+ galaxybook_fwat_props,
+ ARRAY_SIZE(galaxybook_fwat_props)),
+ FWAT_CONFIG_AUX("block_recording", boolean,
+ GB_ATTR_BLOCK_RECORDING,
+ &galaxybook_fwat_ops,
+ galaxybook_fwat_props,
+ ARRAY_SIZE(galaxybook_fwat_props)),
+ NULL
+};
+
+static const struct fwat_dev_config galaxybook_fwat_dev_config = {
+ .attrs_config = galaxybook_fwat_config,
+ .is_visible = galaxybook_fwat_is_visible,
+};
+
+static int galaxybook_fwat_init(struct samsung_galaxybook *galaxybook)
{
- struct galaxybook_fw_attr *fw_attr;
- struct attribute **attrs;
-
- fw_attr = devm_kzalloc(&galaxybook->platform->dev, sizeof(*fw_attr), GFP_KERNEL);
- if (!fw_attr)
- return -ENOMEM;
-
- attrs = devm_kcalloc(&galaxybook->platform->dev, NUM_FW_ATTR_ENUM_ATTRS + 1,
- sizeof(*attrs), GFP_KERNEL);
- if (!attrs)
- return -ENOMEM;
-
- attrs[0] = &fw_attr_type.attr;
- attrs[1] = &fw_attr_default_value.attr;
- attrs[2] = &fw_attr_possible_values.attr;
- attrs[3] = &fw_attr_display_name_language_code.attr;
-
- sysfs_attr_init(&fw_attr->display_name.attr);
- fw_attr->display_name.attr.name = "display_name";
- fw_attr->display_name.attr.mode = 0444;
- fw_attr->display_name.show = display_name_show;
- attrs[4] = &fw_attr->display_name.attr;
-
- sysfs_attr_init(&fw_attr->current_value.attr);
- fw_attr->current_value.attr.name = "current_value";
- fw_attr->current_value.attr.mode = 0644;
- fw_attr->current_value.show = current_value_show;
- fw_attr->current_value.store = current_value_store;
- attrs[5] = &fw_attr->current_value.attr;
-
- attrs[6] = NULL;
-
- fw_attr->galaxybook = galaxybook;
- fw_attr->fw_attr_id = fw_attr_id;
- fw_attr->attr_group.name = galaxybook_fw_attr_name[fw_attr_id];
- fw_attr->attr_group.attrs = attrs;
- fw_attr->get_value = get_value;
- fw_attr->set_value = set_value;
-
- return sysfs_create_group(&galaxybook->fw_attrs_kset->kobj, &fw_attr->attr_group);
-}
-
-static void galaxybook_kset_unregister(void *data)
-{
- struct kset *kset = data;
-
- kset_unregister(kset);
-}
-
-static void galaxybook_fw_attrs_dev_unregister(void *data)
-{
- struct device *fw_attrs_dev = data;
-
- device_unregister(fw_attrs_dev);
-}
-
-static int galaxybook_fw_attrs_init(struct samsung_galaxybook *galaxybook)
-{
- bool value;
+ struct fwat_device *fwdev;
int err;
err = devm_mutex_init(&galaxybook->platform->dev, &galaxybook->fw_attr_lock);
if (err)
return err;
- galaxybook->fw_attrs_dev = device_create(&firmware_attributes_class, NULL, MKDEV(0, 0),
- NULL, "%s", DRIVER_NAME);
- if (IS_ERR(galaxybook->fw_attrs_dev))
- return PTR_ERR(galaxybook->fw_attrs_dev);
-
- err = devm_add_action_or_reset(&galaxybook->platform->dev,
- galaxybook_fw_attrs_dev_unregister,
- galaxybook->fw_attrs_dev);
- if (err)
- return err;
-
- galaxybook->fw_attrs_kset = kset_create_and_add("attributes", NULL,
- &galaxybook->fw_attrs_dev->kobj);
- if (!galaxybook->fw_attrs_kset)
- return -ENOMEM;
- err = devm_add_action_or_reset(&galaxybook->platform->dev,
- galaxybook_kset_unregister, galaxybook->fw_attrs_kset);
- if (err)
- return err;
-
- err = power_on_lid_open_acpi_get(galaxybook, &value);
- if (!err) {
- err = galaxybook_fw_attr_init(galaxybook,
- GB_ATTR_POWER_ON_LID_OPEN,
- &power_on_lid_open_acpi_get,
- &power_on_lid_open_acpi_set);
- if (err)
- return err;
- }
-
- err = usb_charging_acpi_get(galaxybook, &value);
- if (!err) {
- err = galaxybook_fw_attr_init(galaxybook,
- GB_ATTR_USB_CHARGING,
- &usb_charging_acpi_get,
- &usb_charging_acpi_set);
- if (err)
- return err;
- }
-
err = galaxybook_block_recording_init(galaxybook);
- if (err == GB_NOT_SUPPORTED)
- return 0;
- else if (err)
+ if (!err)
+ galaxybook->has_block_recording = true;
+ else if (err != GB_NOT_SUPPORTED)
return err;
- galaxybook->has_block_recording = true;
+ fwdev = devm_fwat_device_register(&galaxybook->platform->dev,
+ "samsung-galaxybook", galaxybook,
+ &galaxybook_fwat_dev_config, NULL);
- return galaxybook_fw_attr_init(galaxybook,
- GB_ATTR_BLOCK_RECORDING,
- &block_recording_acpi_get,
- &block_recording_acpi_set);
+ return PTR_ERR_OR_ZERO(fwdev);
}
/*
@@ -1389,7 +1318,7 @@ static int galaxybook_probe(struct platform_device *pdev)
return dev_err_probe(&galaxybook->platform->dev, err,
"failed to initialize kbd_backlight\n");
- err = galaxybook_fw_attrs_init(galaxybook);
+ err = galaxybook_fwat_init(galaxybook);
if (err)
return dev_err_probe(&galaxybook->platform->dev, err,
"failed to initialize firmware-attributes\n");
--
2.49.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH RFC 5/5] platform/x86: samsung-galaxybook: Transition to new firmware_attributes API
2025-05-09 7:48 ` [PATCH RFC 5/5] platform/x86: samsung-galaxybook: Transition to new firmware_attributes API Kurt Borja
@ 2025-05-12 7:12 ` Thomas Weißschuh
2025-05-12 19:39 ` Kurt Borja
0 siblings, 1 reply; 12+ messages in thread
From: Thomas Weißschuh @ 2025-05-12 7:12 UTC (permalink / raw)
To: Kurt Borja
Cc: Hans de Goede, Ilpo Järvinen, Joshua Grisham, Mark Pearson,
Armin Wolf, Mario Limonciello, Antheas Kapenekakis,
Derek J. Clark, Prasanth Ksr, Jorge Lopez, platform-driver-x86,
linux-kernel, Dell.Client.Kernel
On 2025-05-09 04:48:37-0300, Kurt Borja wrote:
> Transition to new firmware_attributes API and replace `enumeration` types
> with the simpler `boolean` type.
This is an ABI change, using a newly introduced ABI.
Some elaboration why it does not break userspace would be good.
>
> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
> ---
> drivers/platform/x86/samsung-galaxybook.c | 299 ++++++++++++------------------
> 1 file changed, 114 insertions(+), 185 deletions(-)
<snip>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH RFC 5/5] platform/x86: samsung-galaxybook: Transition to new firmware_attributes API
2025-05-12 7:12 ` Thomas Weißschuh
@ 2025-05-12 19:39 ` Kurt Borja
0 siblings, 0 replies; 12+ messages in thread
From: Kurt Borja @ 2025-05-12 19:39 UTC (permalink / raw)
To: Thomas Weißschuh
Cc: Hans de Goede, Ilpo Järvinen, Joshua Grisham, Mark Pearson,
Armin Wolf, Mario Limonciello, Antheas Kapenekakis,
Derek J. Clark, Prasanth Ksr, Jorge Lopez, platform-driver-x86,
linux-kernel, Dell.Client.Kernel
[-- Attachment #1: Type: text/plain, Size: 806 bytes --]
Hi Thomas,
On Mon May 12, 2025 at 4:12 AM -03, Thomas Weißschuh wrote:
> On 2025-05-09 04:48:37-0300, Kurt Borja wrote:
>> Transition to new firmware_attributes API and replace `enumeration` types
>> with the simpler `boolean` type.
>
> This is an ABI change, using a newly introduced ABI.
> Some elaboration why it does not break userspace would be good.
I thought it was not a big deal since the driver was still new. But TIL
fwupd actually interfaces with the firmware-attributes class.
I won't break the ABI of this driver in the next revision.
--
~ Kurt
>
>>
>> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
>> ---
>> drivers/platform/x86/samsung-galaxybook.c | 299 ++++++++++++------------------
>> 1 file changed, 114 insertions(+), 185 deletions(-)
>
> <snip>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread