* [PATCH v2 1/5] platform/x86: firmware_attributes_class: Add device initialization methods
2025-05-17 8:51 [PATCH v2 0/5] platform/x86: firmware_attributes_class: Add a high level API Kurt Borja
@ 2025-05-17 8:51 ` Kurt Borja
2025-05-17 8:51 ` [PATCH v2 2/5] platform/x86: firmware_attributes_class: Add a high level API Kurt Borja
` (4 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: Kurt Borja @ 2025-05-17 8:51 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().
Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
Co-developed-by: Kurt Borja <kuurtb@gmail.com>
Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
drivers/platform/x86/firmware_attributes_class.c | 166 +++++++++++++++++++++++
drivers/platform/x86/firmware_attributes_class.h | 44 ++++++
2 files changed, 210 insertions(+)
diff --git a/drivers/platform/x86/firmware_attributes_class.c b/drivers/platform/x86/firmware_attributes_class.c
index 736e96c186d9dc6d945517f090e9af903e93bbf4..e2dcf2781513b5b033b019780d3e28115e83a1a5 100644
--- a/drivers/platform/x86/firmware_attributes_class.c
+++ b/drivers/platform/x86/firmware_attributes_class.c
@@ -2,7 +2,13 @@
/* Firmware attributes class helper module */
+#include <linux/device.h>
+#include <linux/device/class.h>
+#include <linux/kdev_t.h>
+#include <linux/kobject.h>
#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/types.h>
#include "firmware_attributes_class.h"
const struct class firmware_attributes_class = {
@@ -10,6 +16,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 +187,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* [PATCH v2 2/5] platform/x86: firmware_attributes_class: Add a high level API
2025-05-17 8:51 [PATCH v2 0/5] platform/x86: firmware_attributes_class: Add a high level API Kurt Borja
2025-05-17 8:51 ` [PATCH v2 1/5] platform/x86: firmware_attributes_class: Add device initialization methods Kurt Borja
@ 2025-05-17 8:51 ` Kurt Borja
2025-05-17 8:51 ` [PATCH v2 3/5] platform/x86: firmware_attributes_class: Add a boolean type Kurt Borja
` (3 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: Kurt Borja @ 2025-05-17 8:51 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 | 275 ++++++++++++++++++++++-
drivers/platform/x86/firmware_attributes_class.h | 224 ++++++++++++++++++
2 files changed, 496 insertions(+), 3 deletions(-)
diff --git a/drivers/platform/x86/firmware_attributes_class.c b/drivers/platform/x86/firmware_attributes_class.c
index e2dcf2781513b5b033b019780d3e28115e83a1a5..29401b81b25b9bb1332dbe56eadf96ff81e91c2f 100644
--- a/drivers/platform/x86/firmware_attributes_class.c
+++ b/drivers/platform/x86/firmware_attributes_class.c
@@ -8,14 +8,259 @@
#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->ops->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 struct fwat_attr_ops *ops = config->ops;
+ const char *str;
+ long int_val;
+ int ret;
+
+ switch (ops->type) {
+ case fwat_type_integer:
+ if (!ops->integer.read)
+ return -EOPNOTSUPP;
+
+ ret = ops->integer.read(dev, config->aux, &int_val);
+ if (ret)
+ return ret;
+
+ return sysfs_emit(buf, "%ld\n", int_val);
+ case fwat_type_string:
+ if (!ops->string.read)
+ return -EOPNOTSUPP;
+
+ ret = ops->string.read(dev, config->aux, &str);
+ if (ret)
+ return ret;
+
+ return sysfs_emit(buf, "%s\n", str);
+ case fwat_type_enumeration:
+ if (!ops->enumeration.read)
+ return -EOPNOTSUPP;
+
+ ret = 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;
+ const struct fwat_attr_ops *ops = config->ops;
+ long int_val;
+ int ret;
+
+ switch (ops->type) {
+ case fwat_type_integer:
+ if (!ops->integer.write)
+ return -EOPNOTSUPP;
+
+ ret = kstrtol(buf, 0, &int_val);
+ if (ret)
+ return ret;
+
+ ret = ops->integer.write(dev, config->aux, int_val);
+ break;
+ case fwat_type_string:
+ if (!ops->string.write)
+ return -EOPNOTSUPP;
+
+ ret = ops->string.write(dev, config->aux, buf, count);
+ break;
+ case fwat_type_enumeration:
+ if (!ops->enumeration.write)
+ return -EOPNOTSUPP;
+
+ ret = ops->enumeration.write(dev, config->aux, buf, count);
+ 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;
+ struct attribute *attr;
+ unsigned int index = 0;
+
+ attrs = devm_kcalloc(dev, config->num_props + 3, sizeof(*attrs), GFP_KERNEL);
+ if (!attrs)
+ return NULL;
+
+ /* Optional attributes */
+ for (; index < config->num_props; index++) {
+ prop = config->props[index];
+ attr = fwat_alloc_attr(dev, config, fwat_prop_labels[prop],
+ 0444, prop, fwat_property_show, NULL);
+ if (!attr)
+ return NULL;
+ attrs[index] = attr;
+ }
+
+ /* Mandatory attributes */
+ attr = fwat_alloc_attr(dev, config, "type", 0444, 0, fwat_type_show, NULL);
+ if (!attr)
+ return NULL;
+ attrs[index++] = attr;
+ attr = fwat_alloc_attr(dev, config, "current_value", config->mode, 0,
+ fwat_current_value_show, fwat_current_value_store);
+ if (!attr)
+ return NULL;
+ attrs[index++] = attr;
+
+ 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)
{
@@ -62,6 +307,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
*
@@ -73,8 +319,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;
@@ -98,19 +346,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);
@@ -126,6 +391,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);
@@ -144,6 +411,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
*
@@ -157,12 +425,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..3a13c69ca6a0f993cf7c6bfae6e43f1eeaa002ab 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,234 @@ 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 long_ops {
+ int (*read)(struct device *dev, long aux, long *val);
+ int (*write)(struct device *dev, long aux, long val);
+};
+
+struct str_ops {
+ int (*read)(struct device *dev, long aux, const char **str);
+ int (*write)(struct device *dev, long aux, const char *str, size_t count);
+};
+
+/**
+ * struct fwat_attr_ops - Operations for a firmware *attribute*
+ * @type: Type of callbacks.
+ * @prop_read: Callback for retrieving each configured property of an attribute.
+ * @integer: Integer type callbacks.
+ * @string: String type callbacks.
+ * @enumeration: Enumeration type callbacks.
+ */
+struct fwat_attr_ops {
+ enum fwat_attr_type type;
+ ssize_t (*prop_read)(struct device *dev, long aux,
+ enum fwat_property prop, char *buf);
+ union {
+ struct long_ops integer;
+ struct str_ops string;
+ struct str_ops enumeration;
+ };
+};
+
+/**
+ * struct fwat_attr_config - Configuration for a single firmware *attribute*
+ * @name: Name of the sysfs group associated with this *attribute*.
+ * @mode: Mode assigned to current_value
+ * @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;
+ umode_t mode;
+ 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 = fwat_type_##_type, \
+ ._type = { \
+ .read = _name##_read, \
+ .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 = { \
+ .type = fwat_type_##_type, \
+ .prop_read = _name##_prop_read, \
+ ._type = { \
+ .read = _name##_read, \
+ .write = _name##_write, \
+ } \
+ }
+
+/**
+ * FWAT_CONFIG() - Configuration pointer for a single firmware *attribute*.
+ * @_name: String name of this *attribute*.
+ * @_mode: Mode assigned to current_value
+ * @_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, _mode, _ops, _props, _num_props) \
+ (&(const struct fwat_attr_config) { \
+ .name = _name, \
+ .mode = _mode, \
+ .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*.
+ * @_mode: Mode assigned to current_value
+ * @_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, _mode, _aux, _ops, _props, _num_props) \
+ (&(const struct fwat_attr_config) { \
+ .name = _name, \
+ .mode = _mode, \
+ .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* [PATCH v2 3/5] platform/x86: firmware_attributes_class: Add a boolean type
2025-05-17 8:51 [PATCH v2 0/5] platform/x86: firmware_attributes_class: Add a high level API Kurt Borja
2025-05-17 8:51 ` [PATCH v2 1/5] platform/x86: firmware_attributes_class: Add device initialization methods Kurt Borja
2025-05-17 8:51 ` [PATCH v2 2/5] platform/x86: firmware_attributes_class: Add a high level API Kurt Borja
@ 2025-05-17 8:51 ` Kurt Borja
2025-05-17 8:51 ` [PATCH v2 4/5] MAINTAINERS: Add FIRMWARE ATTRIBUTES CLASS entry Kurt Borja
` (2 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: Kurt Borja @ 2025-05-17 8:51 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 | 19 +++++++++++++++++++
drivers/platform/x86/firmware_attributes_class.h | 8 ++++++++
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 29401b81b25b9bb1332dbe56eadf96ff81e91c2f..6ff38b3be98bc8705ac29ce0afc11d5a4604dc8e 100644
--- a/drivers/platform/x86/firmware_attributes_class.c
+++ b/drivers/platform/x86/firmware_attributes_class.c
@@ -27,6 +27,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",
};
@@ -74,6 +75,7 @@ fwat_current_value_show(struct device *dev, const struct fwat_attribute *attr, c
const struct fwat_attr_config *config = ext->config;
const struct fwat_attr_ops *ops = config->ops;
const char *str;
+ bool bool_val;
long int_val;
int ret;
@@ -87,6 +89,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:
if (!ops->string.read)
return -EOPNOTSUPP;
@@ -117,6 +125,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;
const struct fwat_attr_ops *ops = config->ops;
+ bool bool_val;
long int_val;
int ret;
@@ -131,6 +140,16 @@ fwat_current_value_store(struct device *dev, const struct fwat_attribute *attr,
ret = ops->integer.write(dev, config->aux, int_val);
break;
+ case fwat_type_boolean:
+ if (!ops->boolean.write)
+ return -EOPNOTSUPP;
+
+ ret = kstrtobool(buf, &bool_val);
+ if (ret)
+ return ret;
+
+ ret = ops->boolean.write(dev, config->aux, bool_val);
+ break;
case fwat_type_string:
if (!ops->string.write)
return -EOPNOTSUPP;
diff --git a/drivers/platform/x86/firmware_attributes_class.h b/drivers/platform/x86/firmware_attributes_class.h
index 3a13c69ca6a0f993cf7c6bfae6e43f1eeaa002ab..b823d05d76e91efa40cd3623687b57c664176073 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,
};
@@ -80,11 +81,17 @@ struct str_ops {
int (*write)(struct device *dev, long aux, const char *str, size_t count);
};
+struct bool_ops {
+ int (*read)(struct device *dev, long aux, bool *val);
+ int (*write)(struct device *dev, long aux, bool val);
+};
+
/**
* struct fwat_attr_ops - Operations for a firmware *attribute*
* @type: Type of callbacks.
* @prop_read: Callback for retrieving each configured property of an attribute.
* @integer: Integer type callbacks.
+ * @boolean: Boolean type callbacks.
* @string: String type callbacks.
* @enumeration: Enumeration type callbacks.
*/
@@ -94,6 +101,7 @@ struct fwat_attr_ops {
enum fwat_property prop, char *buf);
union {
struct long_ops integer;
+ struct bool_ops boolean;
struct str_ops string;
struct str_ops enumeration;
};
--
2.49.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH v2 4/5] MAINTAINERS: Add FIRMWARE ATTRIBUTES CLASS entry
2025-05-17 8:51 [PATCH v2 0/5] platform/x86: firmware_attributes_class: Add a high level API Kurt Borja
` (2 preceding siblings ...)
2025-05-17 8:51 ` [PATCH v2 3/5] platform/x86: firmware_attributes_class: Add a boolean type Kurt Borja
@ 2025-05-17 8:51 ` Kurt Borja
2025-05-17 8:51 ` [PATCH v2 5/5] platform/x86: samsung-galaxybook: Transition to new firmware_attributes API Kurt Borja
2025-06-07 16:38 ` [PATCH v2 0/5] platform/x86: firmware_attributes_class: Add a high level API Joshua Grisham
5 siblings, 0 replies; 12+ messages in thread
From: Kurt Borja @ 2025-05-17 8:51 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 cfb3b51cec83d96e969c5b3be791948cb9d60bb9..48509d4da8b132f61ff2d5f90e121bf94ee30e24 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 v2 5/5] platform/x86: samsung-galaxybook: Transition to new firmware_attributes API
2025-05-17 8:51 [PATCH v2 0/5] platform/x86: firmware_attributes_class: Add a high level API Kurt Borja
` (3 preceding siblings ...)
2025-05-17 8:51 ` [PATCH v2 4/5] MAINTAINERS: Add FIRMWARE ATTRIBUTES CLASS entry Kurt Borja
@ 2025-05-17 8:51 ` Kurt Borja
2025-06-07 16:38 ` [PATCH v2 0/5] platform/x86: firmware_attributes_class: Add a high level API Joshua Grisham
5 siblings, 0 replies; 12+ messages in thread
From: Kurt Borja @ 2025-05-17 8:51 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 | 308 ++++++++++++------------------
1 file changed, 123 insertions(+), 185 deletions(-)
diff --git a/drivers/platform/x86/samsung-galaxybook.c b/drivers/platform/x86/samsung-galaxybook.c
index 5878a351993eb05a4c5c2c75b4915d972ce9becc..c69ac4b240ca2acb37ba751853ce33e10a91ebb3 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,155 @@ 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, const char **str)
{
- 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);
+ *str = value ? "1" : "0";
+
+ 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, const char *str,
+ size_t count)
{
- 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);
+ bool val;
+ int err;
+
+ err = kstrtobool(str, &val);
+ if (err)
+ return 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, char *buf)
+{
+ switch (prop) {
+ case FWAT_PROP_DISPLAY_NAME:
+ return sysfs_emit(buf, "%s\n", galaxybook_fwat_desc[aux]);
+ case FWAT_PROP_LANGUAGE_CODE:
+ return sysfs_emit(buf, "%s\n", GB_ATTR_LANGUAGE_CODE);
+ case FWAT_PROP_DEFAULT:
+ return sysfs_emit(buf, "%d\n", 0);
+ case FWAT_ENUM_PROP_POSSIBLE_VALUES:
+ return sysfs_emit(buf, "0;1\n");
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+DEFINE_FWAT_OPS(galaxybook_fwat, enumeration);
+
+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,
+ FWAT_ENUM_PROP_POSSIBLE_VALUES,
+};
-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", 0644,
+ GB_ATTR_POWER_ON_LID_OPEN,
+ &galaxybook_fwat_ops,
+ galaxybook_fwat_props,
+ ARRAY_SIZE(galaxybook_fwat_props)),
+ FWAT_CONFIG_AUX("usb_charging", 0644,
+ GB_ATTR_USB_CHARGING,
+ &galaxybook_fwat_ops,
+ galaxybook_fwat_props,
+ ARRAY_SIZE(galaxybook_fwat_props)),
+ FWAT_CONFIG_AUX("block_recording", 0644,
+ 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 +1327,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 v2 0/5] platform/x86: firmware_attributes_class: Add a high level API
2025-05-17 8:51 [PATCH v2 0/5] platform/x86: firmware_attributes_class: Add a high level API Kurt Borja
` (4 preceding siblings ...)
2025-05-17 8:51 ` [PATCH v2 5/5] platform/x86: samsung-galaxybook: Transition to new firmware_attributes API Kurt Borja
@ 2025-06-07 16:38 ` Joshua Grisham
2025-06-09 1:30 ` Kurt Borja
5 siblings, 1 reply; 12+ messages in thread
From: Joshua Grisham @ 2025-06-07 16:38 UTC (permalink / raw)
To: Kurt Borja
Cc: Hans de Goede, Ilpo Järvinen, Thomas Weißschuh,
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
Den lör 17 maj 2025 kl 10:52 skrev Kurt Borja <kuurtb@gmail.com>:
>
> Hi all,
>
> These series adds the _long awaited_ API for the Firmware Attributes
> class.
>
> You'll find all the details in the commit messages and kernel-doc.
>
> I think it's easier to understand by example, so I used the
> samsung-galaxybook driver for this purpose (last patch). IMO code
> readibility, simplicity, maintainability, etc. is greatly improved, but
> there is still room for improvement of the API itself. For this reason I
> submitted this as an RFC.
>
> As always, your feedback is very appreciated :)
>
> Overview
> ========
>
> Patch 1-2: New API with docs included.
> Patch 3: New firwmare attributes type
> Patch 4: Misc Maintainers patch
> Patch 5: samsung-galaxybook example
>
> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
> ---
> Changes in v2:
>
> [Patch 1]
> - Include kdev_t.h header
>
> [Patch 2]
> - Use one line comments in fwat_create_attrs()
> - Check propagate errors in fwat_create_attrs()
> - Add `mode` to fwat_attr_config and related macros to let users
> configure the `current_value` attribute mode
> - Use defined structs in fwat_attr_ops instead of anonymous ones
> - Move fwat_attr_type from config to ops
>
> [Patch 5]
> - Just transition to new API without chaing ABI
>
> - Link to v1: https://lore.kernel.org/r/20250509-fw-attrs-api-v1-0-258afed65bfa@gmail.com
>
> ---
> Kurt Borja (4):
> platform/x86: firmware_attributes_class: Add a high level API
> platform/x86: firmware_attributes_class: Add a boolean type
> MAINTAINERS: Add FIRMWARE ATTRIBUTES CLASS entry
> platform/x86: samsung-galaxybook: Transition to new firmware_attributes API
>
> Thomas Weißschuh (1):
> platform/x86: firmware_attributes_class: Add device initialization methods
>
> .../ABI/testing/sysfs-class-firmware-attributes | 1 +
> MAINTAINERS | 7 +
> drivers/platform/x86/firmware_attributes_class.c | 454 +++++++++++++++++++++
> drivers/platform/x86/firmware_attributes_class.h | 276 +++++++++++++
> drivers/platform/x86/samsung-galaxybook.c | 308 ++++++--------
> 5 files changed, 861 insertions(+), 185 deletions(-)
> ---
> base-commit: 9f080c9f2099b5a81c85b3b7f95fd11fad428cc8
> change-id: 20250326-fw-attrs-api-0eea7c0225b6
> --
> ~ Kurt
>
Hi Kurt! First let me begin by saying GREAT job in picking this up,
carrying on the work from Thomas, and really trying to glue all of the
various pieces together into a packaged solution that can finally see
the light of day :)
Sorry it has taken some time for me to get back to you--work and other
life stuff seemed to always get in the way and I wanted to make sure I
took enough time to really think about this before I were to give any
feedback myself.
First off, the quick and easy one: I applied all of your patches (on
top of 6.15.1), tested everything with samsung-galaxybook from my
device, and everything is still working without any failures and all
features work as I expect them to. I diffed everything under
/sys/class/firmware-attributes before vs after and everything is
exactly the same EXCEPT it looks like what is currently
"default_value" will now be called "default" with your patch. I assume
if the intention is to keep the ABI same as before then you would
probably want to change this? Specifically here:
> +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",
Assume the last line should instead be
[FWAT_PROP_DEFAULT] = "default_value",
or maybe even for consistency to rename the fwat_property to also
match and then it could be like this?
[FWAT_PROP_DEFAULT_VALUE] = "default_value",
FWIW I don't personally mind changing the ABI for samsung-galaxybook;
as you mentioned it is basically a brand new driver and the solutions
which exist "in the wild" for it are quite limited so better maybe
that it looks "right" going forward instead of carrying any
unnecessary baggage, but I can understand that this may not be the
case for all of the other drivers which have been using these
facilities for a longer time period.
Past that, I certainly think this is a big step forward as compared to
messing around with the lower level kset / kobj_attribute etc
facilities and trying to set everything up from scratch without so
many helper utilities. As you may have noticed, what I ended up doing
in samsung-galaxybook was essentially to create my own local
implementation of some kind of "standard" fw attributes (but only for
booleans), but it would be even better if this were standardized
across all drivers! There are a few things left over in
samsung-galaxybook that still need to be cleaned up from your
suggested change (e.g. the struct galaxybook_fw_attr can now be
totally removed, etc) which we can also address at some point, of
course!
But just to take a step back for a moment, and what I have been really
trying to think through and reflect on myself for a few hours with
this change...
(Please feel free to completely disregard the below if this has
already been brought up and ruled out, or anyone else has any opinions
against this; all of that feedback is welcome and most definitely
trumps my own meager opinions! ;) Also please remember that it is not
my intention at all to detract from any of the great work that has
already been done here -- just the stuff that my brain kind of gets
"stuck" on as I try to think through the bigger picture with this! )
If I think in terms of anyone who wants to come in and work on device
drivers in the kernel, then they will potentially need to learn
facilities for multiple different kind of "attributes" depending on
their use case: device attributes, driver attributes, hwmon's
sensor-related attributes, bus attributes, etc etc, and for the most
part, I think they ALL have basically the same kind of interface and
facilities. It feels very unified and relatively easy to work with all
of them once you have basically figured out the scheme and conventions
that have been implemented.
Now, when I look at the proposal from these patches, these "Firmware
Attributes" do not seem to have the same kind of "look, feel, and
smell" as the other type of attributes I just mentioned, but instead
feels like a totally new animal that must be learned separately. My
take on it would be that a desired/"dream" scenario for a device
driver developer is that all of these interfaces sort of look and
"smell" the same, it is just a matter of the name of the macro you
use, which device you attach the attributes to (which registration
function you need to execute??), and maybe some small subtle
differences in the facilities as appropriate to their context.
Specifically with firmware attributes vs the other kinds, I guess the
biggest differences are that:
1) There is a display_name with a language code
2) There are built-in restrictions on the input values depending on a
"type" (e.g. "enumeration" type has a predetermined list of values,
min/max values or str lengths for numeric or string values, etc)
3) There is a default_value
4) *Maybe* there should be some kind of inheritance and/or sub-groups
(e.g. the "authentication" and similar extensions that create a group
under the parent group...)
But at the end of the day, my hope as a developer would be to be able
to create these firmware attributes in much the same way as the other
types. E.g. maybe something like this quick and dirty pseudo example:
static ssize_t power_on_lid_open_show(struct device *dev,
struct device_attribute *attr,
char *buf)
{
// ...
}
static ssize_t power_on_lid_open_store(struct device *dev,
struct device_attribute *attr,
const char *buf, size_t count)
{
// ...
}
static FW_BOOL_ATTR_RW(power_on_lid_open, "Power On Lid Open");
static struct attribute *galaxybook_fw_attrs[] = {
// ... other fw attrs not shown above ...
&fw_attr_power_on_lid_open.attr,
NULL
};
static const struct attribute_group galaxybook_fw_attrs_group = {
.attrs = galaxybook_fw_attrs,
.is_visible = galaxybook_fw_attr_visible,
};
static int galaxybook_fw_attrs_init(struct samsung_galaxybook *galaxybook)
{
// ...
/* something like "devm_fw_attr_device_register" could be sort
of similar to
how devm_hwmon_device_register_with_groups works ? */
ret = devm_fw_attr_device_register(&galaxybook->platform->dev,
DRIVER_NAME, galaxybook,
&galaxybook_fw_attrs_group);
return PTR_ERR_OR_ZERO(ret);
}
Or in other words:
- I create my callback functions for "show" and "store" with a certain
named prefix and then use a macro to create the struct for this fw
attr that relies on that these functions exist (e.g. in the above
example the macro would create this "fw_attr_power_on_lid_open" fw
attr structure instance) -- note here it might need to be a macro per
type and/or to include the type-related stuff (including value
constraints/enumeration arrays/default values/etc) as parameters to
the macro, plus maybe I would want to provide some kind of context
parameter e.g. I would maybe want a pointer to my samsung_galaxybook
ideally somehow to get to come along?? (that might affect the
signature of my above examples of course! they were just a
quick-and-dirty example...),
- put all of my desired attrs together in a group where I can specify
their is_visible callback (just like you do with DEVICE_ATTRs),
- and then register my fw attr device with my attribute_group (the
register function would take care of all the rest..)
And as sort of shown in the above example I certainly think it would
be nice if the naming convention lined up nicely with how the naming
convention works for the existing attribute stuff (e.g. DEVICE_ATTR_RW
vs DRIVER_ATTR_RW vs something like "FW_ATTR_RW" or "FIRMWARE_ATTR_RW"
seems like it falls into the same convention??)
Again I am not trying to "rock the boat" here, and I have not
necessarily *really* thought through all of the implications to the
existing fw attrs extensions and how they might be able to be
implemented with the above kind of example, ... I'm just taking a step
back and sharing my observations of the patch compared to how it
actually looks in the driver with the example vs how most of the other
existing attribute facilities have been designed.
One more final thing which I always felt a little "off" about -- is it
not the case that other type of platforms might could use firmware
attributes as well? Or is this considered ONLY an x86 thing (e.g. that
"firmware" relates to "BIOS" or something) ? Because I always thought
it a bit strange that the header file was only local to
./drivers/platform/x86/ instead of being part of the Linux headers
under ./include ..
And in the same vein as that, is it not the case that other attributes
could benefit from this "typing" mechanism with constraints (e.g.
DEVICE_ATTR of type "enumeration" that only allows for one of a list
of values ? or a number with min/max value / a string with min/max
length etc etc??). I understand this poses an even bigger question and
much larger change (now we are really talking a HUGE impact! ;) ), but
my first guess is that it would probably be sort of nice to have these
types and this automatic constraints mechanism to be somewhat
universal across other type of attributes, otherwise every single
driver author/maintainer has to write their own manual code for the
same kinds of verifications in every function of every driver (e.g.
write an if statement to check the value in every store function of
every attribute they create, and then otherwise return -EINVAL or
similar... this kind of code exists over and over and over in the
kernel today!).
Anyway I hope this all was of some use, and, as mentioned, please feel
free to take all I have said here "with a pinch of salt" -- I would
definitely hope and encourage that others with longer service records
here could chime in regarding this!
Thanks again for the contribution, great work, and have a nice weekend!
Best regards,
Joshua
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2 0/5] platform/x86: firmware_attributes_class: Add a high level API
2025-06-07 16:38 ` [PATCH v2 0/5] platform/x86: firmware_attributes_class: Add a high level API Joshua Grisham
@ 2025-06-09 1:30 ` Kurt Borja
2025-06-09 9:29 ` Ilpo Järvinen
2025-06-09 13:03 ` Joshua Grisham
0 siblings, 2 replies; 12+ messages in thread
From: Kurt Borja @ 2025-06-09 1:30 UTC (permalink / raw)
To: Joshua Grisham
Cc: Hans de Goede, Ilpo Järvinen, Thomas Weißschuh,
Mark Pearson, Armin Wolf, Mario Limonciello, Antheas Kapenekakis,
Derek J. Clark, Prasanth Ksr, Jorge Lopez, platform-driver-x86,
linux-kernel, Dell.Client.Kernel
Hi Joshua,
On Sat Jun 7, 2025 at 1:38 PM -03, Joshua Grisham wrote:
> Den lör 17 maj 2025 kl 10:52 skrev Kurt Borja <kuurtb@gmail.com>:
>>
>> Hi all,
>>
>> These series adds the _long awaited_ API for the Firmware Attributes
>> class.
>>
>> You'll find all the details in the commit messages and kernel-doc.
>>
>> I think it's easier to understand by example, so I used the
>> samsung-galaxybook driver for this purpose (last patch). IMO code
>> readibility, simplicity, maintainability, etc. is greatly improved, but
>> there is still room for improvement of the API itself. For this reason I
>> submitted this as an RFC.
>>
>> As always, your feedback is very appreciated :)
>>
>> Overview
>> ========
>>
>> Patch 1-2: New API with docs included.
>> Patch 3: New firwmare attributes type
>> Patch 4: Misc Maintainers patch
>> Patch 5: samsung-galaxybook example
>>
>> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
>> ---
>> Changes in v2:
>>
>> [Patch 1]
>> - Include kdev_t.h header
>>
>> [Patch 2]
>> - Use one line comments in fwat_create_attrs()
>> - Check propagate errors in fwat_create_attrs()
>> - Add `mode` to fwat_attr_config and related macros to let users
>> configure the `current_value` attribute mode
>> - Use defined structs in fwat_attr_ops instead of anonymous ones
>> - Move fwat_attr_type from config to ops
>>
>> [Patch 5]
>> - Just transition to new API without chaing ABI
>>
>> - Link to v1: https://lore.kernel.org/r/20250509-fw-attrs-api-v1-0-258afed65bfa@gmail.com
>>
>> ---
>> Kurt Borja (4):
>> platform/x86: firmware_attributes_class: Add a high level API
>> platform/x86: firmware_attributes_class: Add a boolean type
>> MAINTAINERS: Add FIRMWARE ATTRIBUTES CLASS entry
>> platform/x86: samsung-galaxybook: Transition to new firmware_attributes API
>>
>> Thomas Weißschuh (1):
>> platform/x86: firmware_attributes_class: Add device initialization methods
>>
>> .../ABI/testing/sysfs-class-firmware-attributes | 1 +
>> MAINTAINERS | 7 +
>> drivers/platform/x86/firmware_attributes_class.c | 454 +++++++++++++++++++++
>> drivers/platform/x86/firmware_attributes_class.h | 276 +++++++++++++
>> drivers/platform/x86/samsung-galaxybook.c | 308 ++++++--------
>> 5 files changed, 861 insertions(+), 185 deletions(-)
>> ---
>> base-commit: 9f080c9f2099b5a81c85b3b7f95fd11fad428cc8
>> change-id: 20250326-fw-attrs-api-0eea7c0225b6
>> --
>> ~ Kurt
>>
>
> Hi Kurt! First let me begin by saying GREAT job in picking this up,
> carrying on the work from Thomas, and really trying to glue all of the
> various pieces together into a packaged solution that can finally see
> the light of day :)
>
> Sorry it has taken some time for me to get back to you--work and other
> life stuff seemed to always get in the way and I wanted to make sure I
> took enough time to really think about this before I were to give any
> feedback myself.
>
> First off, the quick and easy one: I applied all of your patches (on
> top of 6.15.1), tested everything with samsung-galaxybook from my
> device, and everything is still working without any failures and all
> features work as I expect them to. I diffed everything under
> /sys/class/firmware-attributes before vs after and everything is
> exactly the same EXCEPT it looks like what is currently
> "default_value" will now be called "default" with your patch. I assume
> if the intention is to keep the ABI same as before then you would
> probably want to change this? Specifically here:
>
>> +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",
>
> Assume the last line should instead be
>
> [FWAT_PROP_DEFAULT] = "default_value",
>
> or maybe even for consistency to rename the fwat_property to also
> match and then it could be like this?
>
> [FWAT_PROP_DEFAULT_VALUE] = "default_value",
Yes! You are correct. I completely missed this.
>
> FWIW I don't personally mind changing the ABI for samsung-galaxybook;
> as you mentioned it is basically a brand new driver and the solutions
> which exist "in the wild" for it are quite limited so better maybe
> that it looks "right" going forward instead of carrying any
> unnecessary baggage, but I can understand that this may not be the
> case for all of the other drivers which have been using these
> facilities for a longer time period.
This was my first thought but I found out fwupd uses this interface.
I'll leave the ABI as is to not incur in regressions.
>
> Past that, I certainly think this is a big step forward as compared to
> messing around with the lower level kset / kobj_attribute etc
> facilities and trying to set everything up from scratch without so
> many helper utilities. As you may have noticed, what I ended up doing
> in samsung-galaxybook was essentially to create my own local
> implementation of some kind of "standard" fw attributes (but only for
> booleans), but it would be even better if this were standardized
> across all drivers! There are a few things left over in
> samsung-galaxybook that still need to be cleaned up from your
> suggested change (e.g. the struct galaxybook_fw_attr can now be
> totally removed, etc) which we can also address at some point, of
> course!
Thanks! I'll clean them in the next revision.
>
> But just to take a step back for a moment, and what I have been really
> trying to think through and reflect on myself for a few hours with
> this change...
>
> (Please feel free to completely disregard the below if this has
> already been brought up and ruled out, or anyone else has any opinions
> against this; all of that feedback is welcome and most definitely
> trumps my own meager opinions! ;) Also please remember that it is not
> my intention at all to detract from any of the great work that has
> already been done here -- just the stuff that my brain kind of gets
> "stuck" on as I try to think through the bigger picture with this! )
Don't worry, feedback is always appreciated :)
>
> If I think in terms of anyone who wants to come in and work on device
> drivers in the kernel, then they will potentially need to learn
> facilities for multiple different kind of "attributes" depending on
> their use case: device attributes, driver attributes, hwmon's
> sensor-related attributes, bus attributes, etc etc, and for the most
> part, I think they ALL have basically the same kind of interface and
> facilities. It feels very unified and relatively easy to work with all
> of them once you have basically figured out the scheme and conventions
> that have been implemented.
>
> Now, when I look at the proposal from these patches, these "Firmware
> Attributes" do not seem to have the same kind of "look, feel, and
> smell" as the other type of attributes I just mentioned, but instead
> feels like a totally new animal that must be learned separately. My
> take on it would be that a desired/"dream" scenario for a device
> driver developer is that all of these interfaces sort of look and
> "smell" the same, it is just a matter of the name of the macro you
> use, which device you attach the attributes to (which registration
> function you need to execute??), and maybe some small subtle
> differences in the facilities as appropriate to their context.
>
> Specifically with firmware attributes vs the other kinds, I guess the
> biggest differences are that:
> 1) There is a display_name with a language code
> 2) There are built-in restrictions on the input values depending on a
> "type" (e.g. "enumeration" type has a predetermined list of values,
> min/max values or str lengths for numeric or string values, etc)
> 3) There is a default_value
> 4) *Maybe* there should be some kind of inheritance and/or sub-groups
> (e.g. the "authentication" and similar extensions that create a group
> under the parent group...)
I'm not sure what you mean by this. If you mean this API should also
offer a way to create the Authentication group, I agree!
I was just hoping to get feedback from other maintainers before doing
that. I want to know if this approach passes the "smell" test for
everyone.
>
> But at the end of the day, my hope as a developer would be to be able
> to create these firmware attributes in much the same way as the other
> types. E.g. maybe something like this quick and dirty pseudo example:
>
>
> static ssize_t power_on_lid_open_show(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> {
> // ...
> }
>
> static ssize_t power_on_lid_open_store(struct device *dev,
> struct device_attribute *attr,
> const char *buf, size_t count)
> {
> // ...
> }
>
> static FW_BOOL_ATTR_RW(power_on_lid_open, "Power On Lid Open");
>
> static struct attribute *galaxybook_fw_attrs[] = {
> // ... other fw attrs not shown above ...
> &fw_attr_power_on_lid_open.attr,
> NULL
> };
>
> static const struct attribute_group galaxybook_fw_attrs_group = {
> .attrs = galaxybook_fw_attrs,
> .is_visible = galaxybook_fw_attr_visible,
> };
>
> static int galaxybook_fw_attrs_init(struct samsung_galaxybook *galaxybook)
> {
> // ...
>
> /* something like "devm_fw_attr_device_register" could be sort
> of similar to
> how devm_hwmon_device_register_with_groups works ? */
>
> ret = devm_fw_attr_device_register(&galaxybook->platform->dev,
> DRIVER_NAME, galaxybook,
> &galaxybook_fw_attrs_group);
> return PTR_ERR_OR_ZERO(ret);
> }
>
>
> Or in other words:
> - I create my callback functions for "show" and "store" with a certain
> named prefix and then use a macro to create the struct for this fw
> attr that relies on that these functions exist (e.g. in the above
> example the macro would create this "fw_attr_power_on_lid_open" fw
> attr structure instance) -- note here it might need to be a macro per
> type and/or to include the type-related stuff (including value
> constraints/enumeration arrays/default values/etc) as parameters to
> the macro, plus maybe I would want to provide some kind of context
> parameter e.g. I would maybe want a pointer to my samsung_galaxybook
> ideally somehow to get to come along?? (that might affect the
> signature of my above examples of course! they were just a
> quick-and-dirty example...),
I agree and I believe this API has this capability. You can do this:
static int power_on_lid_open_read(struct device *dev, long aux, const char **str)
{
...
}
static int power_on_lid_open_write(struct device *dev, long aux, const char *str, size_t count)
{
...
}
static ssize_t power_on_lid_open_prop_read(struct device *dev, long aux, enum fwat_property prop,
char *buf)
{
...
}
DEFINE_FWAT_OPS(power_on_lid_open, enumeration);
...
static const struct fwat_attr_config * const galaxybook_fwat_config[] = {
FWAT_CONFIG_AUX("power_on_lid_open", 0644,
GB_ATTR_POWER_ON_LID_OPEN,
&power_on_lid_open_ops,
galaxybook_fwat_props,
ARRAY_SIZE(galaxybook_fwat_props)),
...
NULL
}
I.e, you can define ops for each "firmware attribute" (aka
attribute_group).
I feel the _props approach is currently a bit ugly though, and there is
room for improvement in the boilerplate department.
In the samsung-galaxybook case I decided to define a single struct
fwat_attr_ops because I didn't want to make the diff too ugly. The
*_acpi_{get,set}() functions that already exist are used in other parts
of the driver, and I would have to change a few lines to make it work.
BTW, you can pass a drvdata pointer to devm_fwat_device_register().
> - put all of my desired attrs together in a group where I can specify
> their is_visible callback (just like you do with DEVICE_ATTRs),
I decided to make this a single callback defined in struct
fwat_dev_config. I went for this because I didn't like the idea of a
different function for each attribute_group because it would just be a
bunch of functions.
> - and then register my fw attr device with my attribute_group (the
> register function would take care of all the rest..)
Do you think the struct fwat_attr_config * list achieves this? Could it
be improved in some way?
>
> And as sort of shown in the above example I certainly think it would
> be nice if the naming convention lined up nicely with how the naming
> convention works for the existing attribute stuff (e.g. DEVICE_ATTR_RW
> vs DRIVER_ATTR_RW vs something like "FW_ATTR_RW" or "FIRMWARE_ATTR_RW"
> seems like it falls into the same convention??)
I can certainly add these macros, but they would be for "firmware
attributes" defined entirely manually, using struct fwat_attribute.
Actually I thought of adding these, but I didn't do it because I wanted
to get something working at first and then add some of these extra
helpers.
>
> Again I am not trying to "rock the boat" here, and I have not
> necessarily *really* thought through all of the implications to the
> existing fw attrs extensions and how they might be able to be
> implemented with the above kind of example, ... I'm just taking a step
> back and sharing my observations of the patch compared to how it
> actually looks in the driver with the example vs how most of the other
> existing attribute facilities have been designed.
Thank you! As I said before, feedback is always welcome.
I feel this API already accomplishes the requirements (which I agree
with) you listed, albeit with some (maybe a bit too much) boilerplate.
However your questions make me realise documentation is still lacking, I
will make it better for the next revision.
If you have more concrete areas of improvement, please let me know! I
know there is room for improvement. Especially with naming.
>
> One more final thing which I always felt a little "off" about -- is it
> not the case that other type of platforms might could use firmware
> attributes as well? Or is this considered ONLY an x86 thing (e.g. that
> "firmware" relates to "BIOS" or something) ? Because I always thought
> it a bit strange that the header file was only local to
> ./drivers/platform/x86/ instead of being part of the Linux headers
> under ./include ..
I agree! I'd like to know maintainers opinion on this.
>
> And in the same vein as that, is it not the case that other attributes
> could benefit from this "typing" mechanism with constraints (e.g.
> DEVICE_ATTR of type "enumeration" that only allows for one of a list
> of values ? or a number with min/max value / a string with min/max
> length etc etc??). I understand this poses an even bigger question and
> much larger change (now we are really talking a HUGE impact! ;) ), but
> my first guess is that it would probably be sort of nice to have these
> types and this automatic constraints mechanism to be somewhat
> universal across other type of attributes, otherwise every single
> driver author/maintainer has to write their own manual code for the
> same kinds of verifications in every function of every driver (e.g.
> write an if statement to check the value in every store function of
> every attribute they create, and then otherwise return -EINVAL or
> similar... this kind of code exists over and over and over in the
> kernel today!).
Device attributes already have a lot of helpers for creating some
common attributes, see [1].
I feel like every driver, subsystem, interface, etc. Has VERY different
requirements for how sysfs attributes/groups should work. IMO there
wouldn't be a lot of benefit in providing this infrastructure for other
subsystems, either because they already have something in place or
because it wouldn't exactly fit their needs. Kernel ABI is very diverse.
These syfs interfaces are very old and there are good reasons why they
are the way they are now. I don't think is a bad think to have to
develop infrastructures for each subsystem!
>
> Anyway I hope this all was of some use, and, as mentioned, please feel
> free to take all I have said here "with a pinch of salt" -- I would
> definitely hope and encourage that others with longer service records
> here could chime in regarding this!
I hope so!
>
> Thanks again for the contribution, great work, and have a nice weekend!
You too :)
>
> Best regards,
> Joshua
--
~ Kurt
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2 0/5] platform/x86: firmware_attributes_class: Add a high level API
2025-06-09 1:30 ` Kurt Borja
@ 2025-06-09 9:29 ` Ilpo Järvinen
2025-06-09 19:03 ` Kurt Borja
2025-06-09 13:03 ` Joshua Grisham
1 sibling, 1 reply; 12+ messages in thread
From: Ilpo Järvinen @ 2025-06-09 9:29 UTC (permalink / raw)
To: Kurt Borja
Cc: Joshua Grisham, Hans de Goede, Thomas Weißschuh,
Mark Pearson, Armin Wolf, Mario Limonciello, Antheas Kapenekakis,
Derek J. Clark, Prasanth Ksr, Jorge Lopez, platform-driver-x86,
LKML, Dell.Client.Kernel
[-- Attachment #1: Type: text/plain, Size: 18215 bytes --]
On Sun, 8 Jun 2025, Kurt Borja wrote:
> On Sat Jun 7, 2025 at 1:38 PM -03, Joshua Grisham wrote:
> > Den lör 17 maj 2025 kl 10:52 skrev Kurt Borja <kuurtb@gmail.com>:
> >>
> >> These series adds the _long awaited_ API for the Firmware Attributes
> >> class.
> >>
> >> You'll find all the details in the commit messages and kernel-doc.
> >>
> >> I think it's easier to understand by example, so I used the
> >> samsung-galaxybook driver for this purpose (last patch). IMO code
> >> readibility, simplicity, maintainability, etc. is greatly improved, but
> >> there is still room for improvement of the API itself. For this reason I
> >> submitted this as an RFC.
> >>
> >> As always, your feedback is very appreciated :)
> >>
> >> Overview
> >> ========
> >>
> >> Patch 1-2: New API with docs included.
> >> Patch 3: New firwmare attributes type
> >> Patch 4: Misc Maintainers patch
> >> Patch 5: samsung-galaxybook example
> >>
> >> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
> >> ---
> >> Changes in v2:
> >>
> >> [Patch 1]
> >> - Include kdev_t.h header
> >>
> >> [Patch 2]
> >> - Use one line comments in fwat_create_attrs()
> >> - Check propagate errors in fwat_create_attrs()
> >> - Add `mode` to fwat_attr_config and related macros to let users
> >> configure the `current_value` attribute mode
> >> - Use defined structs in fwat_attr_ops instead of anonymous ones
> >> - Move fwat_attr_type from config to ops
> >>
> >> [Patch 5]
> >> - Just transition to new API without chaing ABI
> >>
> >> - Link to v1: https://lore.kernel.org/r/20250509-fw-attrs-api-v1-0-258afed65bfa@gmail.com
> >>
> >> ---
> >> Kurt Borja (4):
> >> platform/x86: firmware_attributes_class: Add a high level API
> >> platform/x86: firmware_attributes_class: Add a boolean type
> >> MAINTAINERS: Add FIRMWARE ATTRIBUTES CLASS entry
> >> platform/x86: samsung-galaxybook: Transition to new firmware_attributes API
> >>
> >> Thomas Weißschuh (1):
> >> platform/x86: firmware_attributes_class: Add device initialization methods
> >>
> >> .../ABI/testing/sysfs-class-firmware-attributes | 1 +
> >> MAINTAINERS | 7 +
> >> drivers/platform/x86/firmware_attributes_class.c | 454 +++++++++++++++++++++
> >> drivers/platform/x86/firmware_attributes_class.h | 276 +++++++++++++
> >> drivers/platform/x86/samsung-galaxybook.c | 308 ++++++--------
> >> 5 files changed, 861 insertions(+), 185 deletions(-)
> >> ---
> >> base-commit: 9f080c9f2099b5a81c85b3b7f95fd11fad428cc8
> >> change-id: 20250326-fw-attrs-api-0eea7c0225b6
> >> --
> >> ~ Kurt
> >>
> >
> > Hi Kurt! First let me begin by saying GREAT job in picking this up,
> > carrying on the work from Thomas, and really trying to glue all of the
> > various pieces together into a packaged solution that can finally see
> > the light of day :)
> >
> > Sorry it has taken some time for me to get back to you--work and other
> > life stuff seemed to always get in the way and I wanted to make sure I
> > took enough time to really think about this before I were to give any
> > feedback myself.
> >
> > First off, the quick and easy one: I applied all of your patches (on
> > top of 6.15.1), tested everything with samsung-galaxybook from my
> > device, and everything is still working without any failures and all
> > features work as I expect them to. I diffed everything under
> > /sys/class/firmware-attributes before vs after and everything is
> > exactly the same EXCEPT it looks like what is currently
> > "default_value" will now be called "default" with your patch. I assume
> > if the intention is to keep the ABI same as before then you would
> > probably want to change this? Specifically here:
> >
> >> +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",
> >
> > Assume the last line should instead be
> >
> > [FWAT_PROP_DEFAULT] = "default_value",
> >
> > or maybe even for consistency to rename the fwat_property to also
> > match and then it could be like this?
> >
> > [FWAT_PROP_DEFAULT_VALUE] = "default_value",
>
> Yes! You are correct. I completely missed this.
>
> >
> > FWIW I don't personally mind changing the ABI for samsung-galaxybook;
> > as you mentioned it is basically a brand new driver and the solutions
> > which exist "in the wild" for it are quite limited so better maybe
> > that it looks "right" going forward instead of carrying any
> > unnecessary baggage, but I can understand that this may not be the
> > case for all of the other drivers which have been using these
> > facilities for a longer time period.
>
> This was my first thought but I found out fwupd uses this interface.
> I'll leave the ABI as is to not incur in regressions.
>
> >
> > Past that, I certainly think this is a big step forward as compared to
> > messing around with the lower level kset / kobj_attribute etc
> > facilities and trying to set everything up from scratch without so
> > many helper utilities. As you may have noticed, what I ended up doing
> > in samsung-galaxybook was essentially to create my own local
> > implementation of some kind of "standard" fw attributes (but only for
> > booleans), but it would be even better if this were standardized
> > across all drivers! There are a few things left over in
> > samsung-galaxybook that still need to be cleaned up from your
> > suggested change (e.g. the struct galaxybook_fw_attr can now be
> > totally removed, etc) which we can also address at some point, of
> > course!
>
> Thanks! I'll clean them in the next revision.
>
> >
> > But just to take a step back for a moment, and what I have been really
> > trying to think through and reflect on myself for a few hours with
> > this change...
> >
> > (Please feel free to completely disregard the below if this has
> > already been brought up and ruled out, or anyone else has any opinions
> > against this; all of that feedback is welcome and most definitely
> > trumps my own meager opinions! ;) Also please remember that it is not
> > my intention at all to detract from any of the great work that has
> > already been done here -- just the stuff that my brain kind of gets
> > "stuck" on as I try to think through the bigger picture with this! )
>
> Don't worry, feedback is always appreciated :)
>
> >
> > If I think in terms of anyone who wants to come in and work on device
> > drivers in the kernel, then they will potentially need to learn
> > facilities for multiple different kind of "attributes" depending on
> > their use case: device attributes, driver attributes, hwmon's
> > sensor-related attributes, bus attributes, etc etc, and for the most
> > part, I think they ALL have basically the same kind of interface and
> > facilities. It feels very unified and relatively easy to work with all
> > of them once you have basically figured out the scheme and conventions
> > that have been implemented.
> >
> > Now, when I look at the proposal from these patches, these "Firmware
> > Attributes" do not seem to have the same kind of "look, feel, and
> > smell" as the other type of attributes I just mentioned, but instead
> > feels like a totally new animal that must be learned separately. My
> > take on it would be that a desired/"dream" scenario for a device
> > driver developer is that all of these interfaces sort of look and
> > "smell" the same, it is just a matter of the name of the macro you
> > use, which device you attach the attributes to (which registration
> > function you need to execute??), and maybe some small subtle
> > differences in the facilities as appropriate to their context.
> >
> > Specifically with firmware attributes vs the other kinds, I guess the
> > biggest differences are that:
> > 1) There is a display_name with a language code
> > 2) There are built-in restrictions on the input values depending on a
> > "type" (e.g. "enumeration" type has a predetermined list of values,
> > min/max values or str lengths for numeric or string values, etc)
> > 3) There is a default_value
> > 4) *Maybe* there should be some kind of inheritance and/or sub-groups
> > (e.g. the "authentication" and similar extensions that create a group
> > under the parent group...)
>
> I'm not sure what you mean by this. If you mean this API should also
> offer a way to create the Authentication group, I agree!
>
> I was just hoping to get feedback from other maintainers before doing
> that. I want to know if this approach passes the "smell" test for
> everyone.
>
> >
> > But at the end of the day, my hope as a developer would be to be able
> > to create these firmware attributes in much the same way as the other
> > types. E.g. maybe something like this quick and dirty pseudo example:
> >
> >
> > static ssize_t power_on_lid_open_show(struct device *dev,
> > struct device_attribute *attr,
> > char *buf)
> > {
> > // ...
> > }
> >
> > static ssize_t power_on_lid_open_store(struct device *dev,
> > struct device_attribute *attr,
> > const char *buf, size_t count)
> > {
> > // ...
> > }
> >
> > static FW_BOOL_ATTR_RW(power_on_lid_open, "Power On Lid Open");
> >
> > static struct attribute *galaxybook_fw_attrs[] = {
> > // ... other fw attrs not shown above ...
> > &fw_attr_power_on_lid_open.attr,
> > NULL
> > };
> >
> > static const struct attribute_group galaxybook_fw_attrs_group = {
> > .attrs = galaxybook_fw_attrs,
> > .is_visible = galaxybook_fw_attr_visible,
> > };
> >
> > static int galaxybook_fw_attrs_init(struct samsung_galaxybook *galaxybook)
> > {
> > // ...
> >
> > /* something like "devm_fw_attr_device_register" could be sort
> > of similar to
> > how devm_hwmon_device_register_with_groups works ? */
> >
> > ret = devm_fw_attr_device_register(&galaxybook->platform->dev,
> > DRIVER_NAME, galaxybook,
> > &galaxybook_fw_attrs_group);
> > return PTR_ERR_OR_ZERO(ret);
> > }
> >
> >
> > Or in other words:
> > - I create my callback functions for "show" and "store" with a certain
> > named prefix and then use a macro to create the struct for this fw
> > attr that relies on that these functions exist (e.g. in the above
> > example the macro would create this "fw_attr_power_on_lid_open" fw
> > attr structure instance) -- note here it might need to be a macro per
> > type and/or to include the type-related stuff (including value
> > constraints/enumeration arrays/default values/etc) as parameters to
> > the macro, plus maybe I would want to provide some kind of context
> > parameter e.g. I would maybe want a pointer to my samsung_galaxybook
> > ideally somehow to get to come along?? (that might affect the
> > signature of my above examples of course! they were just a
> > quick-and-dirty example...),
>
> I agree and I believe this API has this capability. You can do this:
>
> static int power_on_lid_open_read(struct device *dev, long aux, const char **str)
> {
> ...
> }
>
> static int power_on_lid_open_write(struct device *dev, long aux, const char *str, size_t count)
> {
> ...
> }
>
> static ssize_t power_on_lid_open_prop_read(struct device *dev, long aux, enum fwat_property prop,
> char *buf)
> {
> ...
> }
>
> DEFINE_FWAT_OPS(power_on_lid_open, enumeration);
>
> ...
>
> static const struct fwat_attr_config * const galaxybook_fwat_config[] = {
> FWAT_CONFIG_AUX("power_on_lid_open", 0644,
> GB_ATTR_POWER_ON_LID_OPEN,
> &power_on_lid_open_ops,
> galaxybook_fwat_props,
> ARRAY_SIZE(galaxybook_fwat_props)),
> ...
> NULL
> }
>
> I.e, you can define ops for each "firmware attribute" (aka
> attribute_group).
>
> I feel the _props approach is currently a bit ugly though, and there is
> room for improvement in the boilerplate department.
>
> In the samsung-galaxybook case I decided to define a single struct
> fwat_attr_ops because I didn't want to make the diff too ugly. The
> *_acpi_{get,set}() functions that already exist are used in other parts
> of the driver, and I would have to change a few lines to make it work.
>
> BTW, you can pass a drvdata pointer to devm_fwat_device_register().
>
> > - put all of my desired attrs together in a group where I can specify
> > their is_visible callback (just like you do with DEVICE_ATTRs),
>
> I decided to make this a single callback defined in struct
> fwat_dev_config. I went for this because I didn't like the idea of a
> different function for each attribute_group because it would just be a
> bunch of functions.
>
> > - and then register my fw attr device with my attribute_group (the
> > register function would take care of all the rest..)
>
> Do you think the struct fwat_attr_config * list achieves this? Could it
> be improved in some way?
>
> >
> > And as sort of shown in the above example I certainly think it would
> > be nice if the naming convention lined up nicely with how the naming
> > convention works for the existing attribute stuff (e.g. DEVICE_ATTR_RW
> > vs DRIVER_ATTR_RW vs something like "FW_ATTR_RW" or "FIRMWARE_ATTR_RW"
> > seems like it falls into the same convention??)
>
> I can certainly add these macros, but they would be for "firmware
> attributes" defined entirely manually, using struct fwat_attribute.
> Actually I thought of adding these, but I didn't do it because I wanted
> to get something working at first and then add some of these extra
> helpers.
>
> >
> > Again I am not trying to "rock the boat" here, and I have not
> > necessarily *really* thought through all of the implications to the
> > existing fw attrs extensions and how they might be able to be
> > implemented with the above kind of example, ... I'm just taking a step
> > back and sharing my observations of the patch compared to how it
> > actually looks in the driver with the example vs how most of the other
> > existing attribute facilities have been designed.
>
> Thank you! As I said before, feedback is always welcome.
>
> I feel this API already accomplishes the requirements (which I agree
> with) you listed, albeit with some (maybe a bit too much) boilerplate.
> However your questions make me realise documentation is still lacking, I
> will make it better for the next revision.
>
> If you have more concrete areas of improvement, please let me know! I
> know there is room for improvement. Especially with naming.
>
> >
> > One more final thing which I always felt a little "off" about -- is it
> > not the case that other type of platforms might could use firmware
> > attributes as well? Or is this considered ONLY an x86 thing (e.g. that
> > "firmware" relates to "BIOS" or something) ? Because I always thought
> > it a bit strange that the header file was only local to
> > ./drivers/platform/x86/ instead of being part of the Linux headers
> > under ./include ..
>
> I agree! I'd like to know maintainers opinion on this.
We do move code and headers around whenever we find out the initial
placement isn't good any more, it's business as usual.
Usually when something feels off to you, it is off. But I understand
in these situations there's often the nagging voice telling inside one's
head 'Am I missing something obvious here?'; which rarely is the case ;-).
There's no need to assume the existing code is 'perfect' (including its
placement). :-)
--
i.
> > And in the same vein as that, is it not the case that other attributes
> > could benefit from this "typing" mechanism with constraints (e.g.
> > DEVICE_ATTR of type "enumeration" that only allows for one of a list
> > of values ? or a number with min/max value / a string with min/max
> > length etc etc??). I understand this poses an even bigger question and
> > much larger change (now we are really talking a HUGE impact! ;) ), but
> > my first guess is that it would probably be sort of nice to have these
> > types and this automatic constraints mechanism to be somewhat
> > universal across other type of attributes, otherwise every single
> > driver author/maintainer has to write their own manual code for the
> > same kinds of verifications in every function of every driver (e.g.
> > write an if statement to check the value in every store function of
> > every attribute they create, and then otherwise return -EINVAL or
> > similar... this kind of code exists over and over and over in the
> > kernel today!).
>
> Device attributes already have a lot of helpers for creating some
> common attributes, see [1].
>
> I feel like every driver, subsystem, interface, etc. Has VERY different
> requirements for how sysfs attributes/groups should work. IMO there
> wouldn't be a lot of benefit in providing this infrastructure for other
> subsystems, either because they already have something in place or
> because it wouldn't exactly fit their needs. Kernel ABI is very diverse.
>
> These syfs interfaces are very old and there are good reasons why they
> are the way they are now. I don't think is a bad think to have to
> develop infrastructures for each subsystem!
>
> >
> > Anyway I hope this all was of some use, and, as mentioned, please feel
> > free to take all I have said here "with a pinch of salt" -- I would
> > definitely hope and encourage that others with longer service records
> > here could chime in regarding this!
>
> I hope so!
>
> >
> > Thanks again for the contribution, great work, and have a nice weekend!
>
> You too :)
>
> >
> > Best regards,
> > Joshua
>
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2 0/5] platform/x86: firmware_attributes_class: Add a high level API
2025-06-09 9:29 ` Ilpo Järvinen
@ 2025-06-09 19:03 ` Kurt Borja
0 siblings, 0 replies; 12+ messages in thread
From: Kurt Borja @ 2025-06-09 19:03 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: Joshua Grisham, Hans de Goede, Thomas Weißschuh,
Mark Pearson, Armin Wolf, Mario Limonciello, Antheas Kapenekakis,
Derek J. Clark, Prasanth Ksr, Jorge Lopez, platform-driver-x86,
LKML, Dell.Client.Kernel
[-- Attachment #1: Type: text/plain, Size: 16539 bytes --]
On Mon Jun 9, 2025 at 6:29 AM -03, Ilpo Järvinen wrote:
> On Sun, 8 Jun 2025, Kurt Borja wrote:
>> On Sat Jun 7, 2025 at 1:38 PM -03, Joshua Grisham wrote:
>> > Den lör 17 maj 2025 kl 10:52 skrev Kurt Borja <kuurtb@gmail.com>:
>> >>
>> >> These series adds the _long awaited_ API for the Firmware Attributes
>> >> class.
>> >>
>> >> You'll find all the details in the commit messages and kernel-doc.
>> >>
>> >> I think it's easier to understand by example, so I used the
>> >> samsung-galaxybook driver for this purpose (last patch). IMO code
>> >> readibility, simplicity, maintainability, etc. is greatly improved, but
>> >> there is still room for improvement of the API itself. For this reason I
>> >> submitted this as an RFC.
>> >>
>> >> As always, your feedback is very appreciated :)
>> >>
>> >> Overview
>> >> ========
>> >>
>> >> Patch 1-2: New API with docs included.
>> >> Patch 3: New firwmare attributes type
>> >> Patch 4: Misc Maintainers patch
>> >> Patch 5: samsung-galaxybook example
>> >>
>> >> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
>> >> ---
>> >> Changes in v2:
>> >>
>> >> [Patch 1]
>> >> - Include kdev_t.h header
>> >>
>> >> [Patch 2]
>> >> - Use one line comments in fwat_create_attrs()
>> >> - Check propagate errors in fwat_create_attrs()
>> >> - Add `mode` to fwat_attr_config and related macros to let users
>> >> configure the `current_value` attribute mode
>> >> - Use defined structs in fwat_attr_ops instead of anonymous ones
>> >> - Move fwat_attr_type from config to ops
>> >>
>> >> [Patch 5]
>> >> - Just transition to new API without chaing ABI
>> >>
>> >> - Link to v1: https://lore.kernel.org/r/20250509-fw-attrs-api-v1-0-258afed65bfa@gmail.com
>> >>
>> >> ---
>> >> Kurt Borja (4):
>> >> platform/x86: firmware_attributes_class: Add a high level API
>> >> platform/x86: firmware_attributes_class: Add a boolean type
>> >> MAINTAINERS: Add FIRMWARE ATTRIBUTES CLASS entry
>> >> platform/x86: samsung-galaxybook: Transition to new firmware_attributes API
>> >>
>> >> Thomas Weißschuh (1):
>> >> platform/x86: firmware_attributes_class: Add device initialization methods
>> >>
>> >> .../ABI/testing/sysfs-class-firmware-attributes | 1 +
>> >> MAINTAINERS | 7 +
>> >> drivers/platform/x86/firmware_attributes_class.c | 454 +++++++++++++++++++++
>> >> drivers/platform/x86/firmware_attributes_class.h | 276 +++++++++++++
>> >> drivers/platform/x86/samsung-galaxybook.c | 308 ++++++--------
>> >> 5 files changed, 861 insertions(+), 185 deletions(-)
>> >> ---
>> >> base-commit: 9f080c9f2099b5a81c85b3b7f95fd11fad428cc8
>> >> change-id: 20250326-fw-attrs-api-0eea7c0225b6
>> >> --
>> >> ~ Kurt
>> >>
>> >
>> > Hi Kurt! First let me begin by saying GREAT job in picking this up,
>> > carrying on the work from Thomas, and really trying to glue all of the
>> > various pieces together into a packaged solution that can finally see
>> > the light of day :)
>> >
>> > Sorry it has taken some time for me to get back to you--work and other
>> > life stuff seemed to always get in the way and I wanted to make sure I
>> > took enough time to really think about this before I were to give any
>> > feedback myself.
>> >
>> > First off, the quick and easy one: I applied all of your patches (on
>> > top of 6.15.1), tested everything with samsung-galaxybook from my
>> > device, and everything is still working without any failures and all
>> > features work as I expect them to. I diffed everything under
>> > /sys/class/firmware-attributes before vs after and everything is
>> > exactly the same EXCEPT it looks like what is currently
>> > "default_value" will now be called "default" with your patch. I assume
>> > if the intention is to keep the ABI same as before then you would
>> > probably want to change this? Specifically here:
>> >
>> >> +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",
>> >
>> > Assume the last line should instead be
>> >
>> > [FWAT_PROP_DEFAULT] = "default_value",
>> >
>> > or maybe even for consistency to rename the fwat_property to also
>> > match and then it could be like this?
>> >
>> > [FWAT_PROP_DEFAULT_VALUE] = "default_value",
>>
>> Yes! You are correct. I completely missed this.
>>
>> >
>> > FWIW I don't personally mind changing the ABI for samsung-galaxybook;
>> > as you mentioned it is basically a brand new driver and the solutions
>> > which exist "in the wild" for it are quite limited so better maybe
>> > that it looks "right" going forward instead of carrying any
>> > unnecessary baggage, but I can understand that this may not be the
>> > case for all of the other drivers which have been using these
>> > facilities for a longer time period.
>>
>> This was my first thought but I found out fwupd uses this interface.
>> I'll leave the ABI as is to not incur in regressions.
>>
>> >
>> > Past that, I certainly think this is a big step forward as compared to
>> > messing around with the lower level kset / kobj_attribute etc
>> > facilities and trying to set everything up from scratch without so
>> > many helper utilities. As you may have noticed, what I ended up doing
>> > in samsung-galaxybook was essentially to create my own local
>> > implementation of some kind of "standard" fw attributes (but only for
>> > booleans), but it would be even better if this were standardized
>> > across all drivers! There are a few things left over in
>> > samsung-galaxybook that still need to be cleaned up from your
>> > suggested change (e.g. the struct galaxybook_fw_attr can now be
>> > totally removed, etc) which we can also address at some point, of
>> > course!
>>
>> Thanks! I'll clean them in the next revision.
>>
>> >
>> > But just to take a step back for a moment, and what I have been really
>> > trying to think through and reflect on myself for a few hours with
>> > this change...
>> >
>> > (Please feel free to completely disregard the below if this has
>> > already been brought up and ruled out, or anyone else has any opinions
>> > against this; all of that feedback is welcome and most definitely
>> > trumps my own meager opinions! ;) Also please remember that it is not
>> > my intention at all to detract from any of the great work that has
>> > already been done here -- just the stuff that my brain kind of gets
>> > "stuck" on as I try to think through the bigger picture with this! )
>>
>> Don't worry, feedback is always appreciated :)
>>
>> >
>> > If I think in terms of anyone who wants to come in and work on device
>> > drivers in the kernel, then they will potentially need to learn
>> > facilities for multiple different kind of "attributes" depending on
>> > their use case: device attributes, driver attributes, hwmon's
>> > sensor-related attributes, bus attributes, etc etc, and for the most
>> > part, I think they ALL have basically the same kind of interface and
>> > facilities. It feels very unified and relatively easy to work with all
>> > of them once you have basically figured out the scheme and conventions
>> > that have been implemented.
>> >
>> > Now, when I look at the proposal from these patches, these "Firmware
>> > Attributes" do not seem to have the same kind of "look, feel, and
>> > smell" as the other type of attributes I just mentioned, but instead
>> > feels like a totally new animal that must be learned separately. My
>> > take on it would be that a desired/"dream" scenario for a device
>> > driver developer is that all of these interfaces sort of look and
>> > "smell" the same, it is just a matter of the name of the macro you
>> > use, which device you attach the attributes to (which registration
>> > function you need to execute??), and maybe some small subtle
>> > differences in the facilities as appropriate to their context.
>> >
>> > Specifically with firmware attributes vs the other kinds, I guess the
>> > biggest differences are that:
>> > 1) There is a display_name with a language code
>> > 2) There are built-in restrictions on the input values depending on a
>> > "type" (e.g. "enumeration" type has a predetermined list of values,
>> > min/max values or str lengths for numeric or string values, etc)
>> > 3) There is a default_value
>> > 4) *Maybe* there should be some kind of inheritance and/or sub-groups
>> > (e.g. the "authentication" and similar extensions that create a group
>> > under the parent group...)
>>
>> I'm not sure what you mean by this. If you mean this API should also
>> offer a way to create the Authentication group, I agree!
>>
>> I was just hoping to get feedback from other maintainers before doing
>> that. I want to know if this approach passes the "smell" test for
>> everyone.
>>
>> >
>> > But at the end of the day, my hope as a developer would be to be able
>> > to create these firmware attributes in much the same way as the other
>> > types. E.g. maybe something like this quick and dirty pseudo example:
>> >
>> >
>> > static ssize_t power_on_lid_open_show(struct device *dev,
>> > struct device_attribute *attr,
>> > char *buf)
>> > {
>> > // ...
>> > }
>> >
>> > static ssize_t power_on_lid_open_store(struct device *dev,
>> > struct device_attribute *attr,
>> > const char *buf, size_t count)
>> > {
>> > // ...
>> > }
>> >
>> > static FW_BOOL_ATTR_RW(power_on_lid_open, "Power On Lid Open");
>> >
>> > static struct attribute *galaxybook_fw_attrs[] = {
>> > // ... other fw attrs not shown above ...
>> > &fw_attr_power_on_lid_open.attr,
>> > NULL
>> > };
>> >
>> > static const struct attribute_group galaxybook_fw_attrs_group = {
>> > .attrs = galaxybook_fw_attrs,
>> > .is_visible = galaxybook_fw_attr_visible,
>> > };
>> >
>> > static int galaxybook_fw_attrs_init(struct samsung_galaxybook *galaxybook)
>> > {
>> > // ...
>> >
>> > /* something like "devm_fw_attr_device_register" could be sort
>> > of similar to
>> > how devm_hwmon_device_register_with_groups works ? */
>> >
>> > ret = devm_fw_attr_device_register(&galaxybook->platform->dev,
>> > DRIVER_NAME, galaxybook,
>> > &galaxybook_fw_attrs_group);
>> > return PTR_ERR_OR_ZERO(ret);
>> > }
>> >
>> >
>> > Or in other words:
>> > - I create my callback functions for "show" and "store" with a certain
>> > named prefix and then use a macro to create the struct for this fw
>> > attr that relies on that these functions exist (e.g. in the above
>> > example the macro would create this "fw_attr_power_on_lid_open" fw
>> > attr structure instance) -- note here it might need to be a macro per
>> > type and/or to include the type-related stuff (including value
>> > constraints/enumeration arrays/default values/etc) as parameters to
>> > the macro, plus maybe I would want to provide some kind of context
>> > parameter e.g. I would maybe want a pointer to my samsung_galaxybook
>> > ideally somehow to get to come along?? (that might affect the
>> > signature of my above examples of course! they were just a
>> > quick-and-dirty example...),
>>
>> I agree and I believe this API has this capability. You can do this:
>>
>> static int power_on_lid_open_read(struct device *dev, long aux, const char **str)
>> {
>> ...
>> }
>>
>> static int power_on_lid_open_write(struct device *dev, long aux, const char *str, size_t count)
>> {
>> ...
>> }
>>
>> static ssize_t power_on_lid_open_prop_read(struct device *dev, long aux, enum fwat_property prop,
>> char *buf)
>> {
>> ...
>> }
>>
>> DEFINE_FWAT_OPS(power_on_lid_open, enumeration);
>>
>> ...
>>
>> static const struct fwat_attr_config * const galaxybook_fwat_config[] = {
>> FWAT_CONFIG_AUX("power_on_lid_open", 0644,
>> GB_ATTR_POWER_ON_LID_OPEN,
>> &power_on_lid_open_ops,
>> galaxybook_fwat_props,
>> ARRAY_SIZE(galaxybook_fwat_props)),
>> ...
>> NULL
>> }
>>
>> I.e, you can define ops for each "firmware attribute" (aka
>> attribute_group).
>>
>> I feel the _props approach is currently a bit ugly though, and there is
>> room for improvement in the boilerplate department.
>>
>> In the samsung-galaxybook case I decided to define a single struct
>> fwat_attr_ops because I didn't want to make the diff too ugly. The
>> *_acpi_{get,set}() functions that already exist are used in other parts
>> of the driver, and I would have to change a few lines to make it work.
>>
>> BTW, you can pass a drvdata pointer to devm_fwat_device_register().
>>
>> > - put all of my desired attrs together in a group where I can specify
>> > their is_visible callback (just like you do with DEVICE_ATTRs),
>>
>> I decided to make this a single callback defined in struct
>> fwat_dev_config. I went for this because I didn't like the idea of a
>> different function for each attribute_group because it would just be a
>> bunch of functions.
>>
>> > - and then register my fw attr device with my attribute_group (the
>> > register function would take care of all the rest..)
>>
>> Do you think the struct fwat_attr_config * list achieves this? Could it
>> be improved in some way?
>>
>> >
>> > And as sort of shown in the above example I certainly think it would
>> > be nice if the naming convention lined up nicely with how the naming
>> > convention works for the existing attribute stuff (e.g. DEVICE_ATTR_RW
>> > vs DRIVER_ATTR_RW vs something like "FW_ATTR_RW" or "FIRMWARE_ATTR_RW"
>> > seems like it falls into the same convention??)
>>
>> I can certainly add these macros, but they would be for "firmware
>> attributes" defined entirely manually, using struct fwat_attribute.
>> Actually I thought of adding these, but I didn't do it because I wanted
>> to get something working at first and then add some of these extra
>> helpers.
>>
>> >
>> > Again I am not trying to "rock the boat" here, and I have not
>> > necessarily *really* thought through all of the implications to the
>> > existing fw attrs extensions and how they might be able to be
>> > implemented with the above kind of example, ... I'm just taking a step
>> > back and sharing my observations of the patch compared to how it
>> > actually looks in the driver with the example vs how most of the other
>> > existing attribute facilities have been designed.
>>
>> Thank you! As I said before, feedback is always welcome.
>>
>> I feel this API already accomplishes the requirements (which I agree
>> with) you listed, albeit with some (maybe a bit too much) boilerplate.
>> However your questions make me realise documentation is still lacking, I
>> will make it better for the next revision.
>>
>> If you have more concrete areas of improvement, please let me know! I
>> know there is room for improvement. Especially with naming.
>>
>> >
>> > One more final thing which I always felt a little "off" about -- is it
>> > not the case that other type of platforms might could use firmware
>> > attributes as well? Or is this considered ONLY an x86 thing (e.g. that
>> > "firmware" relates to "BIOS" or something) ? Because I always thought
>> > it a bit strange that the header file was only local to
>> > ./drivers/platform/x86/ instead of being part of the Linux headers
>> > under ./include ..
>>
>> I agree! I'd like to know maintainers opinion on this.
>
> We do move code and headers around whenever we find out the initial
> placement isn't good any more, it's business as usual.
>
> Usually when something feels off to you, it is off. But I understand
> in these situations there's often the nagging voice telling inside one's
> head 'Am I missing something obvious here?'; which rarely is the case ;-).
> There's no need to assume the existing code is 'perfect' (including its
> placement). :-)
Then I'll move it to ./include/. Thanks Ilpo :)
--
~ Kurt
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 0/5] platform/x86: firmware_attributes_class: Add a high level API
2025-06-09 1:30 ` Kurt Borja
2025-06-09 9:29 ` Ilpo Järvinen
@ 2025-06-09 13:03 ` Joshua Grisham
2025-06-09 19:00 ` Kurt Borja
1 sibling, 1 reply; 12+ messages in thread
From: Joshua Grisham @ 2025-06-09 13:03 UTC (permalink / raw)
To: Kurt Borja
Cc: Hans de Goede, Ilpo Järvinen, Thomas Weißschuh,
Mark Pearson, Armin Wolf, Mario Limonciello, Antheas Kapenekakis,
Derek J. Clark, Prasanth Ksr, Jorge Lopez, platform-driver-x86,
linux-kernel, Dell.Client.Kernel
Den mån 9 juni 2025 kl 03:30 skrev Kurt Borja <kuurtb@gmail.com>:
>
> Hi Joshua,
>
> On Sat Jun 7, 2025 at 1:38 PM -03, Joshua Grisham wrote:
> > Den lör 17 maj 2025 kl 10:52 skrev Kurt Borja <kuurtb@gmail.com>:
> >>
> >> Hi all,
> >>
> >> These series adds the _long awaited_ API for the Firmware Attributes
> >> class.
> >>
> >> You'll find all the details in the commit messages and kernel-doc.
> >>
> >> I think it's easier to understand by example, so I used the
> >> samsung-galaxybook driver for this purpose (last patch). IMO code
> >> readibility, simplicity, maintainability, etc. is greatly improved, but
> >> there is still room for improvement of the API itself. For this reason I
> >> submitted this as an RFC.
> >>
> >> As always, your feedback is very appreciated :)
> >>
> >> Overview
> >> ========
> >>
> >> Patch 1-2: New API with docs included.
> >> Patch 3: New firwmare attributes type
> >> Patch 4: Misc Maintainers patch
> >> Patch 5: samsung-galaxybook example
> >>
> >> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
> >> ---
> >> Changes in v2:
> >>
> >> [Patch 1]
> >> - Include kdev_t.h header
> >>
> >> [Patch 2]
> >> - Use one line comments in fwat_create_attrs()
> >> - Check propagate errors in fwat_create_attrs()
> >> - Add `mode` to fwat_attr_config and related macros to let users
> >> configure the `current_value` attribute mode
> >> - Use defined structs in fwat_attr_ops instead of anonymous ones
> >> - Move fwat_attr_type from config to ops
> >>
> >> [Patch 5]
> >> - Just transition to new API without chaing ABI
> >>
> >> - Link to v1: https://lore.kernel.org/r/20250509-fw-attrs-api-v1-0-258afed65bfa@gmail.com
> >>
> >> ---
> >> Kurt Borja (4):
> >> platform/x86: firmware_attributes_class: Add a high level API
> >> platform/x86: firmware_attributes_class: Add a boolean type
> >> MAINTAINERS: Add FIRMWARE ATTRIBUTES CLASS entry
> >> platform/x86: samsung-galaxybook: Transition to new firmware_attributes API
> >>
> >> Thomas Weißschuh (1):
> >> platform/x86: firmware_attributes_class: Add device initialization methods
> >>
> >> .../ABI/testing/sysfs-class-firmware-attributes | 1 +
> >> MAINTAINERS | 7 +
> >> drivers/platform/x86/firmware_attributes_class.c | 454 +++++++++++++++++++++
> >> drivers/platform/x86/firmware_attributes_class.h | 276 +++++++++++++
> >> drivers/platform/x86/samsung-galaxybook.c | 308 ++++++--------
> >> 5 files changed, 861 insertions(+), 185 deletions(-)
> >> ---
> >> base-commit: 9f080c9f2099b5a81c85b3b7f95fd11fad428cc8
> >> change-id: 20250326-fw-attrs-api-0eea7c0225b6
> >> --
> >> ~ Kurt
> >>
> >
> > Hi Kurt! First let me begin by saying GREAT job in picking this up,
> > carrying on the work from Thomas, and really trying to glue all of the
> > various pieces together into a packaged solution that can finally see
> > the light of day :)
> >
> > Sorry it has taken some time for me to get back to you--work and other
> > life stuff seemed to always get in the way and I wanted to make sure I
> > took enough time to really think about this before I were to give any
> > feedback myself.
> >
> > First off, the quick and easy one: I applied all of your patches (on
> > top of 6.15.1), tested everything with samsung-galaxybook from my
> > device, and everything is still working without any failures and all
> > features work as I expect them to. I diffed everything under
> > /sys/class/firmware-attributes before vs after and everything is
> > exactly the same EXCEPT it looks like what is currently
> > "default_value" will now be called "default" with your patch. I assume
> > if the intention is to keep the ABI same as before then you would
> > probably want to change this? Specifically here:
> >
> >> +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",
> >
> > Assume the last line should instead be
> >
> > [FWAT_PROP_DEFAULT] = "default_value",
> >
> > or maybe even for consistency to rename the fwat_property to also
> > match and then it could be like this?
> >
> > [FWAT_PROP_DEFAULT_VALUE] = "default_value",
>
> Yes! You are correct. I completely missed this.
>
> >
> > FWIW I don't personally mind changing the ABI for samsung-galaxybook;
> > as you mentioned it is basically a brand new driver and the solutions
> > which exist "in the wild" for it are quite limited so better maybe
> > that it looks "right" going forward instead of carrying any
> > unnecessary baggage, but I can understand that this may not be the
> > case for all of the other drivers which have been using these
> > facilities for a longer time period.
>
> This was my first thought but I found out fwupd uses this interface.
> I'll leave the ABI as is to not incur in regressions.
>
> >
> > Past that, I certainly think this is a big step forward as compared to
> > messing around with the lower level kset / kobj_attribute etc
> > facilities and trying to set everything up from scratch without so
> > many helper utilities. As you may have noticed, what I ended up doing
> > in samsung-galaxybook was essentially to create my own local
> > implementation of some kind of "standard" fw attributes (but only for
> > booleans), but it would be even better if this were standardized
> > across all drivers! There are a few things left over in
> > samsung-galaxybook that still need to be cleaned up from your
> > suggested change (e.g. the struct galaxybook_fw_attr can now be
> > totally removed, etc) which we can also address at some point, of
> > course!
>
> Thanks! I'll clean them in the next revision.
>
> >
> > But just to take a step back for a moment, and what I have been really
> > trying to think through and reflect on myself for a few hours with
> > this change...
> >
> > (Please feel free to completely disregard the below if this has
> > already been brought up and ruled out, or anyone else has any opinions
> > against this; all of that feedback is welcome and most definitely
> > trumps my own meager opinions! ;) Also please remember that it is not
> > my intention at all to detract from any of the great work that has
> > already been done here -- just the stuff that my brain kind of gets
> > "stuck" on as I try to think through the bigger picture with this! )
>
> Don't worry, feedback is always appreciated :)
>
> >
> > If I think in terms of anyone who wants to come in and work on device
> > drivers in the kernel, then they will potentially need to learn
> > facilities for multiple different kind of "attributes" depending on
> > their use case: device attributes, driver attributes, hwmon's
> > sensor-related attributes, bus attributes, etc etc, and for the most
> > part, I think they ALL have basically the same kind of interface and
> > facilities. It feels very unified and relatively easy to work with all
> > of them once you have basically figured out the scheme and conventions
> > that have been implemented.
> >
> > Now, when I look at the proposal from these patches, these "Firmware
> > Attributes" do not seem to have the same kind of "look, feel, and
> > smell" as the other type of attributes I just mentioned, but instead
> > feels like a totally new animal that must be learned separately. My
> > take on it would be that a desired/"dream" scenario for a device
> > driver developer is that all of these interfaces sort of look and
> > "smell" the same, it is just a matter of the name of the macro you
> > use, which device you attach the attributes to (which registration
> > function you need to execute??), and maybe some small subtle
> > differences in the facilities as appropriate to their context.
> >
> > Specifically with firmware attributes vs the other kinds, I guess the
> > biggest differences are that:
> > 1) There is a display_name with a language code
> > 2) There are built-in restrictions on the input values depending on a
> > "type" (e.g. "enumeration" type has a predetermined list of values,
> > min/max values or str lengths for numeric or string values, etc)
> > 3) There is a default_value
> > 4) *Maybe* there should be some kind of inheritance and/or sub-groups
> > (e.g. the "authentication" and similar extensions that create a group
> > under the parent group...)
>
> I'm not sure what you mean by this. If you mean this API should also
> offer a way to create the Authentication group, I agree!
>
> I was just hoping to get feedback from other maintainers before doing
> that. I want to know if this approach passes the "smell" test for
> everyone.
>
> >
> > But at the end of the day, my hope as a developer would be to be able
> > to create these firmware attributes in much the same way as the other
> > types. E.g. maybe something like this quick and dirty pseudo example:
> >
> >
> > static ssize_t power_on_lid_open_show(struct device *dev,
> > struct device_attribute *attr,
> > char *buf)
> > {
> > // ...
> > }
> >
> > static ssize_t power_on_lid_open_store(struct device *dev,
> > struct device_attribute *attr,
> > const char *buf, size_t count)
> > {
> > // ...
> > }
> >
> > static FW_BOOL_ATTR_RW(power_on_lid_open, "Power On Lid Open");
> >
> > static struct attribute *galaxybook_fw_attrs[] = {
> > // ... other fw attrs not shown above ...
> > &fw_attr_power_on_lid_open.attr,
> > NULL
> > };
> >
> > static const struct attribute_group galaxybook_fw_attrs_group = {
> > .attrs = galaxybook_fw_attrs,
> > .is_visible = galaxybook_fw_attr_visible,
> > };
> >
> > static int galaxybook_fw_attrs_init(struct samsung_galaxybook *galaxybook)
> > {
> > // ...
> >
> > /* something like "devm_fw_attr_device_register" could be sort
> > of similar to
> > how devm_hwmon_device_register_with_groups works ? */
> >
> > ret = devm_fw_attr_device_register(&galaxybook->platform->dev,
> > DRIVER_NAME, galaxybook,
> > &galaxybook_fw_attrs_group);
> > return PTR_ERR_OR_ZERO(ret);
> > }
> >
> >
> > Or in other words:
> > - I create my callback functions for "show" and "store" with a certain
> > named prefix and then use a macro to create the struct for this fw
> > attr that relies on that these functions exist (e.g. in the above
> > example the macro would create this "fw_attr_power_on_lid_open" fw
> > attr structure instance) -- note here it might need to be a macro per
> > type and/or to include the type-related stuff (including value
> > constraints/enumeration arrays/default values/etc) as parameters to
> > the macro, plus maybe I would want to provide some kind of context
> > parameter e.g. I would maybe want a pointer to my samsung_galaxybook
> > ideally somehow to get to come along?? (that might affect the
> > signature of my above examples of course! they were just a
> > quick-and-dirty example...),
>
> I agree and I believe this API has this capability. You can do this:
>
> static int power_on_lid_open_read(struct device *dev, long aux, const char **str)
> {
> ...
> }
>
> static int power_on_lid_open_write(struct device *dev, long aux, const char *str, size_t count)
> {
> ...
> }
>
> static ssize_t power_on_lid_open_prop_read(struct device *dev, long aux, enum fwat_property prop,
> char *buf)
> {
> ...
> }
>
> DEFINE_FWAT_OPS(power_on_lid_open, enumeration);
>
> ...
>
> static const struct fwat_attr_config * const galaxybook_fwat_config[] = {
> FWAT_CONFIG_AUX("power_on_lid_open", 0644,
> GB_ATTR_POWER_ON_LID_OPEN,
> &power_on_lid_open_ops,
> galaxybook_fwat_props,
> ARRAY_SIZE(galaxybook_fwat_props)),
> ...
> NULL
> }
>
> I.e, you can define ops for each "firmware attribute" (aka
> attribute_group).
>
> I feel the _props approach is currently a bit ugly though, and there is
> room for improvement in the boilerplate department.
>
> In the samsung-galaxybook case I decided to define a single struct
> fwat_attr_ops because I didn't want to make the diff too ugly. The
> *_acpi_{get,set}() functions that already exist are used in other parts
> of the driver, and I would have to change a few lines to make it work.
>
> BTW, you can pass a drvdata pointer to devm_fwat_device_register().
>
> > - put all of my desired attrs together in a group where I can specify
> > their is_visible callback (just like you do with DEVICE_ATTRs),
>
> I decided to make this a single callback defined in struct
> fwat_dev_config. I went for this because I didn't like the idea of a
> different function for each attribute_group because it would just be a
> bunch of functions.
>
> > - and then register my fw attr device with my attribute_group (the
> > register function would take care of all the rest..)
>
> Do you think the struct fwat_attr_config * list achieves this? Could it
> be improved in some way?
>
Hi again Kurt!
In case it helps for me to restate that, from my perspective, I can
say that I was maybe not thinking too terribly deeply, more at a
shallow level about the higher level workflow/process for a developer.
For example, with device attributes, it is loosely like this:
1. create functions to handle the actual getting and setting logic
which can become my show/store callback(s)
2. use DEVICE_ATTR_RW or similar macro to create my
device_attribute(s) -- the macro will expect callback functions that
follow specific naming convention from step 1
3. put one of more of my device_attribute(s) in a group (with optional
is_visible callback that also should be created)
4. my "device" is registered (through various mechanisms) while
passing in this attribute group as a parameter and everything is
created for me via the registration process
And this process is roughly the same with device attributes, driver
attributes, bus attributes, hwmon sensor attributes etc etc. so it is
very "familiar" to me (not saying that is "good" or "bad", but it is
nice that once you learn this process, you can sort of more quickly
understand what is happening as you encounter these various types used
in different places).
And from what I can tell as you mentioned in this patch, the process
for FW attributes would instead be something like .. ?
1. create functions or some way to handle the actual getting and
setting logic (but these are maybe not directly used as callbacks, but
instead used as one case within the logic of a more general
"show/store any property under the fw attr" callback)
2. create (or use an existing "standard"?) enum with all properties
that should exist under my fw attr (display_name, current_value,
default_value, etc)
3. create said general show/store callback function for all properties
within the fw attr (display_name, current_value, default_value, etc)
4. use DEFINE_FWAT_OPS macro to create some fw attr ops for a specific
fw attr (passing the name and type)
5. build an array of fwat_attr_config whose entry is another struct,
but that struct can/will be built by another macro FWAT_CONFIG_AUX (or
similar) with all of its own properties for things like mode, the ops
from step 4, the properties enum from step 2, etc
6. put the attrs config in a new fwat_dev_config (with optional
is_visible callback that also should be created)
7. register the fw attr device with devm_fwat_device_register, passing
the fwat_dev_config and optional drvdata
(of course I could have easily missed something obvious or gotten
something totally wrong here, but hopefully I have captured the gist
of it!)
So I guess what I was trying to say was, the first process seems
easier and more intuitive for my simple mind to understand, whereas
the second process is a fair bit different from the others. Not that
one is necessarily better or worse than the other, but I guess I would
say that the first does feel more "simple" to me (though admittedly
these other attributes are more "simple" by design, of course!).
What I tried to convey before was more that my own personal "dream
scenario" with this is that I as a developer could essentially still
follow the first process I mentioned from above (the one for other
device attrs) and then in a "happy path" scenario I would only need to
focus on callback functions for my getting and setting logic (show
and/or store callbacks) and not have to write ANY other code.
Basically like this:
1. create functions to handle the actual getting and setting logic
which can become my show/store callback(s)
2. use FW_BOOL_ATTR_RW(name, display_name, default_value) or similar
macro to create my fw_attribute(s) --> NOTE that because I chose
"bool" here (in that I used the "BOOL" variant of the macro) then I
will automatically get all of the standard attributes like
display_name, current_value, language_code (maybe even that it
defaults to English unless I want to change it??), fixed choice of
possible_values (as the possible values are always the same for
booleans, I don't need to do anything with this part..) etc etc i.e.
there are standard callback functions for these standard things like
display_name etc and the developer does not have to implement them
(and similar variants could exist for enum, string, and numeric fw
attrs that take care of min/max properties etc)
3. put one of more of my fw_attribute(s) in a group (with optional
is_visible callback that also should be created)
4. my fw attr "device" is registered and the group is passed and
everything is created for me via the registration process
So *essentially* the process of this kind of fw attr implementation
would still be roughly the same as other attribute types, the code
would "look/feel/smell" the same as other attribute types, but I would
just need to pass a few extra parameters to the attribute macro in
order to set the display_name, default_value, etc properties that
would then get picked up by the "standard" show callback functions for
those attributes. The only thing I as an implementer really bother
with is the show/store functions and their actual getting/setting
logic on the actual device.
And bonus if the "standard" logic also enforces the various
constraints (e.g. it should always block to store something that is
not one of my given possible_values, or enforce the various min/max
rules, etc) so that I do not even have to write any of the code for
that part, either!
Maybe I am thinking of this too simply? I have not as mentioned
thought through all of the implementation details of this, but my
first guess is at least for these standard types (enumeration, string,
boolean, etc) is that it does seem like it is *probably* possible??
(not the extensions e.g. "authentication" etc -- needs more thinking
on that!)
Does this make sense, or are we saying the same thing, or maybe
"talking past each other" as they say in Swedish? :)
Thanks again and keep up the good fight!
Joshua
> >
> > And as sort of shown in the above example I certainly think it would
> > be nice if the naming convention lined up nicely with how the naming
> > convention works for the existing attribute stuff (e.g. DEVICE_ATTR_RW
> > vs DRIVER_ATTR_RW vs something like "FW_ATTR_RW" or "FIRMWARE_ATTR_RW"
> > seems like it falls into the same convention??)
>
> I can certainly add these macros, but they would be for "firmware
> attributes" defined entirely manually, using struct fwat_attribute.
> Actually I thought of adding these, but I didn't do it because I wanted
> to get something working at first and then add some of these extra
> helpers.
>
> >
> > Again I am not trying to "rock the boat" here, and I have not
> > necessarily *really* thought through all of the implications to the
> > existing fw attrs extensions and how they might be able to be
> > implemented with the above kind of example, ... I'm just taking a step
> > back and sharing my observations of the patch compared to how it
> > actually looks in the driver with the example vs how most of the other
> > existing attribute facilities have been designed.
>
> Thank you! As I said before, feedback is always welcome.
>
> I feel this API already accomplishes the requirements (which I agree
> with) you listed, albeit with some (maybe a bit too much) boilerplate.
> However your questions make me realise documentation is still lacking, I
> will make it better for the next revision.
>
> If you have more concrete areas of improvement, please let me know! I
> know there is room for improvement. Especially with naming.
>
> >
> > One more final thing which I always felt a little "off" about -- is it
> > not the case that other type of platforms might could use firmware
> > attributes as well? Or is this considered ONLY an x86 thing (e.g. that
> > "firmware" relates to "BIOS" or something) ? Because I always thought
> > it a bit strange that the header file was only local to
> > ./drivers/platform/x86/ instead of being part of the Linux headers
> > under ./include ..
>
> I agree! I'd like to know maintainers opinion on this.
>
> >
> > And in the same vein as that, is it not the case that other attributes
> > could benefit from this "typing" mechanism with constraints (e.g.
> > DEVICE_ATTR of type "enumeration" that only allows for one of a list
> > of values ? or a number with min/max value / a string with min/max
> > length etc etc??). I understand this poses an even bigger question and
> > much larger change (now we are really talking a HUGE impact! ;) ), but
> > my first guess is that it would probably be sort of nice to have these
> > types and this automatic constraints mechanism to be somewhat
> > universal across other type of attributes, otherwise every single
> > driver author/maintainer has to write their own manual code for the
> > same kinds of verifications in every function of every driver (e.g.
> > write an if statement to check the value in every store function of
> > every attribute they create, and then otherwise return -EINVAL or
> > similar... this kind of code exists over and over and over in the
> > kernel today!).
>
> Device attributes already have a lot of helpers for creating some
> common attributes, see [1].
>
> I feel like every driver, subsystem, interface, etc. Has VERY different
> requirements for how sysfs attributes/groups should work. IMO there
> wouldn't be a lot of benefit in providing this infrastructure for other
> subsystems, either because they already have something in place or
> because it wouldn't exactly fit their needs. Kernel ABI is very diverse.
>
> These syfs interfaces are very old and there are good reasons why they
> are the way they are now. I don't think is a bad think to have to
> develop infrastructures for each subsystem!
>
> >
> > Anyway I hope this all was of some use, and, as mentioned, please feel
> > free to take all I have said here "with a pinch of salt" -- I would
> > definitely hope and encourage that others with longer service records
> > here could chime in regarding this!
>
> I hope so!
>
> >
> > Thanks again for the contribution, great work, and have a nice weekend!
>
> You too :)
>
> >
> > Best regards,
> > Joshua
>
>
> --
> ~ Kurt
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2 0/5] platform/x86: firmware_attributes_class: Add a high level API
2025-06-09 13:03 ` Joshua Grisham
@ 2025-06-09 19:00 ` Kurt Borja
0 siblings, 0 replies; 12+ messages in thread
From: Kurt Borja @ 2025-06-09 19:00 UTC (permalink / raw)
To: Joshua Grisham
Cc: Hans de Goede, Ilpo Järvinen, Thomas Weißschuh,
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: 27466 bytes --]
On Mon Jun 9, 2025 at 10:03 AM -03, Joshua Grisham wrote:
> Den mån 9 juni 2025 kl 03:30 skrev Kurt Borja <kuurtb@gmail.com>:
>>
>> Hi Joshua,
>>
>> On Sat Jun 7, 2025 at 1:38 PM -03, Joshua Grisham wrote:
>> > Den lör 17 maj 2025 kl 10:52 skrev Kurt Borja <kuurtb@gmail.com>:
>> >>
>> >> Hi all,
>> >>
>> >> These series adds the _long awaited_ API for the Firmware Attributes
>> >> class.
>> >>
>> >> You'll find all the details in the commit messages and kernel-doc.
>> >>
>> >> I think it's easier to understand by example, so I used the
>> >> samsung-galaxybook driver for this purpose (last patch). IMO code
>> >> readibility, simplicity, maintainability, etc. is greatly improved, but
>> >> there is still room for improvement of the API itself. For this reason I
>> >> submitted this as an RFC.
>> >>
>> >> As always, your feedback is very appreciated :)
>> >>
>> >> Overview
>> >> ========
>> >>
>> >> Patch 1-2: New API with docs included.
>> >> Patch 3: New firwmare attributes type
>> >> Patch 4: Misc Maintainers patch
>> >> Patch 5: samsung-galaxybook example
>> >>
>> >> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
>> >> ---
>> >> Changes in v2:
>> >>
>> >> [Patch 1]
>> >> - Include kdev_t.h header
>> >>
>> >> [Patch 2]
>> >> - Use one line comments in fwat_create_attrs()
>> >> - Check propagate errors in fwat_create_attrs()
>> >> - Add `mode` to fwat_attr_config and related macros to let users
>> >> configure the `current_value` attribute mode
>> >> - Use defined structs in fwat_attr_ops instead of anonymous ones
>> >> - Move fwat_attr_type from config to ops
>> >>
>> >> [Patch 5]
>> >> - Just transition to new API without chaing ABI
>> >>
>> >> - Link to v1: https://lore.kernel.org/r/20250509-fw-attrs-api-v1-0-258afed65bfa@gmail.com
>> >>
>> >> ---
>> >> Kurt Borja (4):
>> >> platform/x86: firmware_attributes_class: Add a high level API
>> >> platform/x86: firmware_attributes_class: Add a boolean type
>> >> MAINTAINERS: Add FIRMWARE ATTRIBUTES CLASS entry
>> >> platform/x86: samsung-galaxybook: Transition to new firmware_attributes API
>> >>
>> >> Thomas Weißschuh (1):
>> >> platform/x86: firmware_attributes_class: Add device initialization methods
>> >>
>> >> .../ABI/testing/sysfs-class-firmware-attributes | 1 +
>> >> MAINTAINERS | 7 +
>> >> drivers/platform/x86/firmware_attributes_class.c | 454 +++++++++++++++++++++
>> >> drivers/platform/x86/firmware_attributes_class.h | 276 +++++++++++++
>> >> drivers/platform/x86/samsung-galaxybook.c | 308 ++++++--------
>> >> 5 files changed, 861 insertions(+), 185 deletions(-)
>> >> ---
>> >> base-commit: 9f080c9f2099b5a81c85b3b7f95fd11fad428cc8
>> >> change-id: 20250326-fw-attrs-api-0eea7c0225b6
>> >> --
>> >> ~ Kurt
>> >>
>> >
>> > Hi Kurt! First let me begin by saying GREAT job in picking this up,
>> > carrying on the work from Thomas, and really trying to glue all of the
>> > various pieces together into a packaged solution that can finally see
>> > the light of day :)
>> >
>> > Sorry it has taken some time for me to get back to you--work and other
>> > life stuff seemed to always get in the way and I wanted to make sure I
>> > took enough time to really think about this before I were to give any
>> > feedback myself.
>> >
>> > First off, the quick and easy one: I applied all of your patches (on
>> > top of 6.15.1), tested everything with samsung-galaxybook from my
>> > device, and everything is still working without any failures and all
>> > features work as I expect them to. I diffed everything under
>> > /sys/class/firmware-attributes before vs after and everything is
>> > exactly the same EXCEPT it looks like what is currently
>> > "default_value" will now be called "default" with your patch. I assume
>> > if the intention is to keep the ABI same as before then you would
>> > probably want to change this? Specifically here:
>> >
>> >> +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",
>> >
>> > Assume the last line should instead be
>> >
>> > [FWAT_PROP_DEFAULT] = "default_value",
>> >
>> > or maybe even for consistency to rename the fwat_property to also
>> > match and then it could be like this?
>> >
>> > [FWAT_PROP_DEFAULT_VALUE] = "default_value",
>>
>> Yes! You are correct. I completely missed this.
>>
>> >
>> > FWIW I don't personally mind changing the ABI for samsung-galaxybook;
>> > as you mentioned it is basically a brand new driver and the solutions
>> > which exist "in the wild" for it are quite limited so better maybe
>> > that it looks "right" going forward instead of carrying any
>> > unnecessary baggage, but I can understand that this may not be the
>> > case for all of the other drivers which have been using these
>> > facilities for a longer time period.
>>
>> This was my first thought but I found out fwupd uses this interface.
>> I'll leave the ABI as is to not incur in regressions.
>>
>> >
>> > Past that, I certainly think this is a big step forward as compared to
>> > messing around with the lower level kset / kobj_attribute etc
>> > facilities and trying to set everything up from scratch without so
>> > many helper utilities. As you may have noticed, what I ended up doing
>> > in samsung-galaxybook was essentially to create my own local
>> > implementation of some kind of "standard" fw attributes (but only for
>> > booleans), but it would be even better if this were standardized
>> > across all drivers! There are a few things left over in
>> > samsung-galaxybook that still need to be cleaned up from your
>> > suggested change (e.g. the struct galaxybook_fw_attr can now be
>> > totally removed, etc) which we can also address at some point, of
>> > course!
>>
>> Thanks! I'll clean them in the next revision.
>>
>> >
>> > But just to take a step back for a moment, and what I have been really
>> > trying to think through and reflect on myself for a few hours with
>> > this change...
>> >
>> > (Please feel free to completely disregard the below if this has
>> > already been brought up and ruled out, or anyone else has any opinions
>> > against this; all of that feedback is welcome and most definitely
>> > trumps my own meager opinions! ;) Also please remember that it is not
>> > my intention at all to detract from any of the great work that has
>> > already been done here -- just the stuff that my brain kind of gets
>> > "stuck" on as I try to think through the bigger picture with this! )
>>
>> Don't worry, feedback is always appreciated :)
>>
>> >
>> > If I think in terms of anyone who wants to come in and work on device
>> > drivers in the kernel, then they will potentially need to learn
>> > facilities for multiple different kind of "attributes" depending on
>> > their use case: device attributes, driver attributes, hwmon's
>> > sensor-related attributes, bus attributes, etc etc, and for the most
>> > part, I think they ALL have basically the same kind of interface and
>> > facilities. It feels very unified and relatively easy to work with all
>> > of them once you have basically figured out the scheme and conventions
>> > that have been implemented.
>> >
>> > Now, when I look at the proposal from these patches, these "Firmware
>> > Attributes" do not seem to have the same kind of "look, feel, and
>> > smell" as the other type of attributes I just mentioned, but instead
>> > feels like a totally new animal that must be learned separately. My
>> > take on it would be that a desired/"dream" scenario for a device
>> > driver developer is that all of these interfaces sort of look and
>> > "smell" the same, it is just a matter of the name of the macro you
>> > use, which device you attach the attributes to (which registration
>> > function you need to execute??), and maybe some small subtle
>> > differences in the facilities as appropriate to their context.
>> >
>> > Specifically with firmware attributes vs the other kinds, I guess the
>> > biggest differences are that:
>> > 1) There is a display_name with a language code
>> > 2) There are built-in restrictions on the input values depending on a
>> > "type" (e.g. "enumeration" type has a predetermined list of values,
>> > min/max values or str lengths for numeric or string values, etc)
>> > 3) There is a default_value
>> > 4) *Maybe* there should be some kind of inheritance and/or sub-groups
>> > (e.g. the "authentication" and similar extensions that create a group
>> > under the parent group...)
>>
>> I'm not sure what you mean by this. If you mean this API should also
>> offer a way to create the Authentication group, I agree!
>>
>> I was just hoping to get feedback from other maintainers before doing
>> that. I want to know if this approach passes the "smell" test for
>> everyone.
>>
>> >
>> > But at the end of the day, my hope as a developer would be to be able
>> > to create these firmware attributes in much the same way as the other
>> > types. E.g. maybe something like this quick and dirty pseudo example:
>> >
>> >
>> > static ssize_t power_on_lid_open_show(struct device *dev,
>> > struct device_attribute *attr,
>> > char *buf)
>> > {
>> > // ...
>> > }
>> >
>> > static ssize_t power_on_lid_open_store(struct device *dev,
>> > struct device_attribute *attr,
>> > const char *buf, size_t count)
>> > {
>> > // ...
>> > }
>> >
>> > static FW_BOOL_ATTR_RW(power_on_lid_open, "Power On Lid Open");
>> >
>> > static struct attribute *galaxybook_fw_attrs[] = {
>> > // ... other fw attrs not shown above ...
>> > &fw_attr_power_on_lid_open.attr,
>> > NULL
>> > };
>> >
>> > static const struct attribute_group galaxybook_fw_attrs_group = {
>> > .attrs = galaxybook_fw_attrs,
>> > .is_visible = galaxybook_fw_attr_visible,
>> > };
>> >
>> > static int galaxybook_fw_attrs_init(struct samsung_galaxybook *galaxybook)
>> > {
>> > // ...
>> >
>> > /* something like "devm_fw_attr_device_register" could be sort
>> > of similar to
>> > how devm_hwmon_device_register_with_groups works ? */
>> >
>> > ret = devm_fw_attr_device_register(&galaxybook->platform->dev,
>> > DRIVER_NAME, galaxybook,
>> > &galaxybook_fw_attrs_group);
>> > return PTR_ERR_OR_ZERO(ret);
>> > }
>> >
>> >
>> > Or in other words:
>> > - I create my callback functions for "show" and "store" with a certain
>> > named prefix and then use a macro to create the struct for this fw
>> > attr that relies on that these functions exist (e.g. in the above
>> > example the macro would create this "fw_attr_power_on_lid_open" fw
>> > attr structure instance) -- note here it might need to be a macro per
>> > type and/or to include the type-related stuff (including value
>> > constraints/enumeration arrays/default values/etc) as parameters to
>> > the macro, plus maybe I would want to provide some kind of context
>> > parameter e.g. I would maybe want a pointer to my samsung_galaxybook
>> > ideally somehow to get to come along?? (that might affect the
>> > signature of my above examples of course! they were just a
>> > quick-and-dirty example...),
>>
>> I agree and I believe this API has this capability. You can do this:
>>
>> static int power_on_lid_open_read(struct device *dev, long aux, const char **str)
>> {
>> ...
>> }
>>
>> static int power_on_lid_open_write(struct device *dev, long aux, const char *str, size_t count)
>> {
>> ...
>> }
>>
>> static ssize_t power_on_lid_open_prop_read(struct device *dev, long aux, enum fwat_property prop,
>> char *buf)
>> {
>> ...
>> }
>>
>> DEFINE_FWAT_OPS(power_on_lid_open, enumeration);
>>
>> ...
>>
>> static const struct fwat_attr_config * const galaxybook_fwat_config[] = {
>> FWAT_CONFIG_AUX("power_on_lid_open", 0644,
>> GB_ATTR_POWER_ON_LID_OPEN,
>> &power_on_lid_open_ops,
>> galaxybook_fwat_props,
>> ARRAY_SIZE(galaxybook_fwat_props)),
>> ...
>> NULL
>> }
>>
>> I.e, you can define ops for each "firmware attribute" (aka
>> attribute_group).
>>
>> I feel the _props approach is currently a bit ugly though, and there is
>> room for improvement in the boilerplate department.
>>
>> In the samsung-galaxybook case I decided to define a single struct
>> fwat_attr_ops because I didn't want to make the diff too ugly. The
>> *_acpi_{get,set}() functions that already exist are used in other parts
>> of the driver, and I would have to change a few lines to make it work.
>>
>> BTW, you can pass a drvdata pointer to devm_fwat_device_register().
>>
>> > - put all of my desired attrs together in a group where I can specify
>> > their is_visible callback (just like you do with DEVICE_ATTRs),
>>
>> I decided to make this a single callback defined in struct
>> fwat_dev_config. I went for this because I didn't like the idea of a
>> different function for each attribute_group because it would just be a
>> bunch of functions.
>>
>> > - and then register my fw attr device with my attribute_group (the
>> > register function would take care of all the rest..)
>>
>> Do you think the struct fwat_attr_config * list achieves this? Could it
>> be improved in some way?
>>
>
> Hi again Kurt!
Hi Joshua,
>
> In case it helps for me to restate that, from my perspective, I can
> say that I was maybe not thinking too terribly deeply, more at a
> shallow level about the higher level workflow/process for a developer.
> For example, with device attributes, it is loosely like this:
>
> 1. create functions to handle the actual getting and setting logic
> which can become my show/store callback(s)
>
> 2. use DEVICE_ATTR_RW or similar macro to create my
> device_attribute(s) -- the macro will expect callback functions that
> follow specific naming convention from step 1
>
> 3. put one of more of my device_attribute(s) in a group (with optional
> is_visible callback that also should be created)
>
> 4. my "device" is registered (through various mechanisms) while
> passing in this attribute group as a parameter and everything is
> created for me via the registration process
>
> And this process is roughly the same with device attributes, driver
> attributes, bus attributes, hwmon sensor attributes etc etc. so it is
> very "familiar" to me (not saying that is "good" or "bad", but it is
> nice that once you learn this process, you can sort of more quickly
> understand what is happening as you encounter these various types used
> in different places).
>
> And from what I can tell as you mentioned in this patch, the process
> for FW attributes would instead be something like .. ?
>
> 1. create functions or some way to handle the actual getting and
> setting logic (but these are maybe not directly used as callbacks, but
> instead used as one case within the logic of a more general
> "show/store any property under the fw attr" callback)
I don't really see how this steps differs from your dream scenario. If
you want you could define read/write/read_prop for each "firmware
attribute" (which is actually an attribute_group).
The only difference is that you can (optionally) reuse these
read/write/read_prop callbacks for multiple attribute groups. I think
this is necessary if you want to define a bunch of attribute_groups with
the same behavior without having to define a bunch of callbacks.
>
> 2. create (or use an existing "standard"?) enum with all properties
> that should exist under my fw attr (display_name, current_value,
> default_value, etc)
You have to provide a list of sysfs attributes (not fw attributes!) you
want for each of your "fw attributes" (attribute_groups). The enum is
already provided.
>
> 3. create said general show/store callback function for all properties
> within the fw attr (display_name, current_value, default_value, etc)
>
> 4. use DEFINE_FWAT_OPS macro to create some fw attr ops for a specific
> fw attr (passing the name and type)
Or multiple fw attributes. Users can decide.
>
> 5. build an array of fwat_attr_config whose entry is another struct,
> but that struct can/will be built by another macro FWAT_CONFIG_AUX (or
> similar) with all of its own properties for things like mode, the ops
> from step 4, the properties enum from step 2, etc
>
> 6. put the attrs config in a new fwat_dev_config (with optional
> is_visible callback that also should be created)
>
> 7. register the fw attr device with devm_fwat_device_register, passing
> the fwat_dev_config and optional drvdata
>
> (of course I could have easily missed something obvious or gotten
> something totally wrong here, but hopefully I have captured the gist
> of it!)
>
> So I guess what I was trying to say was, the first process seems
> easier and more intuitive for my simple mind to understand, whereas
> the second process is a fair bit different from the others. Not that
> one is necessarily better or worse than the other, but I guess I would
> say that the first does feel more "simple" to me (though admittedly
> these other attributes are more "simple" by design, of course!).
This is not a minor detail :p
In the first example you are creating single attributes. In the second
process you are creating attribute_group(s) with a few "type" specific
attributes, which are all optional too! (per ABI specification).
>
> What I tried to convey before was more that my own personal "dream
> scenario" with this is that I as a developer could essentially still
> follow the first process I mentioned from above (the one for other
> device attrs) and then in a "happy path" scenario I would only need to
> focus on callback functions for my getting and setting logic (show
> and/or store callbacks) and not have to write ANY other code.
> Basically like this:
>
> 1. create functions to handle the actual getting and setting logic
> which can become my show/store callback(s)
I think we can agree this API already accomplishes this? I might be
misunderstanding something though.
>
> 2. use FW_BOOL_ATTR_RW(name, display_name, default_value) or similar
IMO using that name would actually break intution. As a user I would
expect that macro to create a sysfs attribute, not an attribute group.
But I agree, display_name can/should be statically defined. I don't know
about default_value though. Some drivers may obtain this value
dynamically.
> macro to create my fw_attribute(s) --> NOTE that because I chose
> "bool" here (in that I used the "BOOL" variant of the macro) then I
> will automatically get all of the standard attributes like
> display_name, current_value, language_code (maybe even that it
I agree, this could be the default behavior. I think most drivers
already do this anyway, so it would eliminate some boilerplate code.
> defaults to English unless I want to change it??), fixed choice of
> possible_values (as the possible values are always the same for
> booleans, I don't need to do anything with this part..) etc etc i.e.
IMO possible_values (or any of the other props) should not be statically
defined. I think this is too restrictive for drivers that probe for this
data.
> there are standard callback functions for these standard things like
> display_name etc and the developer does not have to implement them
> (and similar variants could exist for enum, string, and numeric fw
> attrs that take care of min/max properties etc)
I like this, it could work for drivers that do want this values
statically defined. I'll think a way of accomplishing this without
introducing too much complexity.
>
> 3. put one of more of my fw_attribute(s) in a group (with optional
> is_visible callback that also should be created)
>
> 4. my fw attr "device" is registered and the group is passed and
> everything is created for me via the registration process
I think this API already accomplishes this two? What do you think?
>
> So *essentially* the process of this kind of fw attr implementation
> would still be roughly the same as other attribute types, the code
> would "look/feel/smell" the same as other attribute types, but I would
> just need to pass a few extra parameters to the attribute macro in
> order to set the display_name, default_value, etc properties that
> would then get picked up by the "standard" show callback functions for
> those attributes. The only thing I as an implementer really bother
> with is the show/store functions and their actual getting/setting
> logic on the actual device.
I agree. This should be the workflow for drivers that want everything
statically defined (such as samsung-galaxybook).
>
> And bonus if the "standard" logic also enforces the various
> constraints (e.g. it should always block to store something that is
> not one of my given possible_values, or enforce the various min/max
> rules, etc) so that I do not even have to write any of the code for
> that part, either!
IMO APIs like this should enforce the least possible constraints and let
drivers handle that appropiately.
Also as I said before, some of these values are dynamically obtained. We
can't assume they don't change, etc.
>
> Maybe I am thinking of this too simply? I have not as mentioned
> thought through all of the implementation details of this, but my
> first guess is at least for these standard types (enumeration, string,
> boolean, etc) is that it does seem like it is *probably* possible??
> (not the extensions e.g. "authentication" etc -- needs more thinking
> on that!)
>
> Does this make sense, or are we saying the same thing, or maybe
> "talking past each other" as they say in Swedish? :)
I think I understand your major concerns/suggestions. For v3 I'll do the
following:
- Make display name and language code statically defined
- Create all attributes for a "fw attribute" type by default and offer
an option to select just the ones you want
- Offer a way to define these props statically (I have to think more
about this though)
- Improve documentation!
Would you add something extra?
--
~ Kurt
>
> Thanks again and keep up the good fight!
>
> Joshua
>
>> >
>> > And as sort of shown in the above example I certainly think it would
>> > be nice if the naming convention lined up nicely with how the naming
>> > convention works for the existing attribute stuff (e.g. DEVICE_ATTR_RW
>> > vs DRIVER_ATTR_RW vs something like "FW_ATTR_RW" or "FIRMWARE_ATTR_RW"
>> > seems like it falls into the same convention??)
>>
>> I can certainly add these macros, but they would be for "firmware
>> attributes" defined entirely manually, using struct fwat_attribute.
>> Actually I thought of adding these, but I didn't do it because I wanted
>> to get something working at first and then add some of these extra
>> helpers.
>>
>> >
>> > Again I am not trying to "rock the boat" here, and I have not
>> > necessarily *really* thought through all of the implications to the
>> > existing fw attrs extensions and how they might be able to be
>> > implemented with the above kind of example, ... I'm just taking a step
>> > back and sharing my observations of the patch compared to how it
>> > actually looks in the driver with the example vs how most of the other
>> > existing attribute facilities have been designed.
>>
>> Thank you! As I said before, feedback is always welcome.
>>
>> I feel this API already accomplishes the requirements (which I agree
>> with) you listed, albeit with some (maybe a bit too much) boilerplate.
>> However your questions make me realise documentation is still lacking, I
>> will make it better for the next revision.
>>
>> If you have more concrete areas of improvement, please let me know! I
>> know there is room for improvement. Especially with naming.
>>
>> >
>> > One more final thing which I always felt a little "off" about -- is it
>> > not the case that other type of platforms might could use firmware
>> > attributes as well? Or is this considered ONLY an x86 thing (e.g. that
>> > "firmware" relates to "BIOS" or something) ? Because I always thought
>> > it a bit strange that the header file was only local to
>> > ./drivers/platform/x86/ instead of being part of the Linux headers
>> > under ./include ..
>>
>> I agree! I'd like to know maintainers opinion on this.
>>
>> >
>> > And in the same vein as that, is it not the case that other attributes
>> > could benefit from this "typing" mechanism with constraints (e.g.
>> > DEVICE_ATTR of type "enumeration" that only allows for one of a list
>> > of values ? or a number with min/max value / a string with min/max
>> > length etc etc??). I understand this poses an even bigger question and
>> > much larger change (now we are really talking a HUGE impact! ;) ), but
>> > my first guess is that it would probably be sort of nice to have these
>> > types and this automatic constraints mechanism to be somewhat
>> > universal across other type of attributes, otherwise every single
>> > driver author/maintainer has to write their own manual code for the
>> > same kinds of verifications in every function of every driver (e.g.
>> > write an if statement to check the value in every store function of
>> > every attribute they create, and then otherwise return -EINVAL or
>> > similar... this kind of code exists over and over and over in the
>> > kernel today!).
>>
>> Device attributes already have a lot of helpers for creating some
>> common attributes, see [1].
>>
>> I feel like every driver, subsystem, interface, etc. Has VERY different
>> requirements for how sysfs attributes/groups should work. IMO there
>> wouldn't be a lot of benefit in providing this infrastructure for other
>> subsystems, either because they already have something in place or
>> because it wouldn't exactly fit their needs. Kernel ABI is very diverse.
>>
>> These syfs interfaces are very old and there are good reasons why they
>> are the way they are now. I don't think is a bad think to have to
>> develop infrastructures for each subsystem!
>>
>> >
>> > Anyway I hope this all was of some use, and, as mentioned, please feel
>> > free to take all I have said here "with a pinch of salt" -- I would
>> > definitely hope and encourage that others with longer service records
>> > here could chime in regarding this!
>>
>> I hope so!
>>
>> >
>> > Thanks again for the contribution, great work, and have a nice weekend!
>>
>> You too :)
>>
>> >
>> > Best regards,
>> > Joshua
>>
>>
>> --
>> ~ Kurt
>>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread