Re: [PATCH RFC 2/5] platform/x86: firmware_attributes_class: Add a high level API
From: Mario Limonciello
Date: Fri May 09 2025 - 16:01:40 EST
On 5/9/2025 2:56 PM, Kurt Borja wrote:
Hi Mario,
On Fri May 9, 2025 at 12:58 PM -03, Mario Limonciello wrote:
On 5/9/2025 2:48 AM, Kurt Borja wrote:
Add an attribute configuration mechanism through the newly introduced
`struct fwat_dev_config`, which makes use of other documented structs
and callbacks.
This API aims to be simple, yet flexible. In order to accomplish this,
the following features were taken into account:
* Ability to statically define attributes
* Custom read/write callbacks for each attribute type
* Ability to map attributes to numbers in order to differentiate them in
callbacks (`aux` number)
* Ability to reuse read/write callbacks in different attributes
* Ability to reuse property selection in different attributes
* Optional visibility callback for dynamic attribute visibility
Signed-off-by: Kurt Borja <kuurtb@xxxxxxxxx>
---
drivers/platform/x86/firmware_attributes_class.c | 249 ++++++++++++++++++++++-
drivers/platform/x86/firmware_attributes_class.h | 228 +++++++++++++++++++++
2 files changed, 474 insertions(+), 3 deletions(-)
diff --git a/drivers/platform/x86/firmware_attributes_class.c b/drivers/platform/x86/firmware_attributes_class.c
index 58ab1495ba3bd449cfe17de2827a57a0c5937788..7cfb0f49f235728c7450a82a7e9d00b8963d3dea 100644
--- a/drivers/platform/x86/firmware_attributes_class.c
+++ b/drivers/platform/x86/firmware_attributes_class.c
@@ -7,14 +7,233 @@
#include <linux/kobject.h>
#include <linux/module.h>
#include <linux/slab.h>
+#include <linux/sysfs.h>
#include <linux/types.h>
#include "firmware_attributes_class.h"
+#define to_fwat_attribute_ext(_a) container_of_const(_a, struct fwat_attribute_ext, attr)
+
+struct fwat_attribute_ext {
+ struct fwat_attribute attr;
+ enum fwat_property prop;
+ const struct fwat_attr_config *config;
+};
+
const struct class firmware_attributes_class = {
.name = "firmware-attributes",
};
EXPORT_SYMBOL_GPL(firmware_attributes_class);
+static const char * const fwat_type_labels[] = {
+ [fwat_type_integer] = "integer",
+ [fwat_type_string] = "string",
+ [fwat_type_enumeration] = "enumeration",
+};
+
+static const char * const fwat_prop_labels[] = {
+ [FWAT_PROP_DISPLAY_NAME] = "display_name",
+ [FWAT_PROP_LANGUAGE_CODE] = "display_name_language_code",
+ [FWAT_PROP_DEFAULT] = "default",
+
+ [FWAT_INT_PROP_MIN] = "min_value",
+ [FWAT_INT_PROP_MAX] = "max_value",
+ [FWAT_INT_PROP_INCREMENT] = "scalar_increment",
+
+ [FWAT_STR_PROP_MIN] = "min_length",
+ [FWAT_STR_PROP_MAX] = "max_length",
+
+ [FWAT_ENUM_PROP_POSSIBLE_VALUES] = "possible_values",
+};
+
+static ssize_t
+fwat_type_show(struct device *dev, const struct fwat_attribute *attr, char *buf)
+{
+ const struct fwat_attribute_ext *ext = to_fwat_attribute_ext(attr);
+ const struct fwat_attr_config *config = ext->config;
+
+ return sysfs_emit(buf, "%s\n", fwat_type_labels[config->type]);
+}
+
+static ssize_t
+fwat_property_show(struct device *dev, const struct fwat_attribute *attr, char *buf)
+{
+ const struct fwat_attribute_ext *ext = to_fwat_attribute_ext(attr);
+ const struct fwat_attr_config *config = ext->config;
+
+ if (!config->ops->prop_read)
+ return -EOPNOTSUPP;
+
+ return config->ops->prop_read(dev, config->aux, ext->prop, buf);
+}
+
+static ssize_t
+fwat_current_value_show(struct device *dev, const struct fwat_attribute *attr, char *buf)
+{
+ const struct fwat_attribute_ext *ext = to_fwat_attribute_ext(attr);
+ const struct fwat_attr_config *config = ext->config;
+ const char *str;
+ long int_val;
+ int ret;
+
+ switch (config->type) {
+ case fwat_type_integer:
+ ret = config->ops->integer_read(dev, config->aux, &int_val);
+ if (ret)
+ return ret;
+
+ return sysfs_emit(buf, "%ld\n", int_val);
+ case fwat_type_string:
+ ret = config->ops->string_read(dev, config->aux, &str);
+ if (ret)
+ return ret;
+
+ return sysfs_emit(buf, "%s\n", str);
+ case fwat_type_enumeration:
+ ret = config->ops->enumeration_read(dev, config->aux, &str);
+ if (ret)
+ return ret;
+
+ return sysfs_emit(buf, "%s\n", str);
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+static ssize_t
+fwat_current_value_store(struct device *dev, const struct fwat_attribute *attr,
+ const char *buf, size_t count)
+{
+ const struct fwat_attribute_ext *ext = to_fwat_attribute_ext(attr);
+ const struct fwat_attr_config *config = ext->config;
+ long int_val;
+ int ret;
+
+ switch (config->type) {
+ case fwat_type_integer:
+ ret = kstrtol(buf, 0, &int_val);
+ if (ret)
+ return ret;
+
+ ret = config->ops->integer_write(dev, config->aux, int_val);
+ break;
+ case fwat_type_string:
+ ret = config->ops->string_write(dev, config->aux, buf);
+ break;
+ case fwat_type_enumeration:
+ ret = config->ops->enumeration_write(dev, config->aux, buf);
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
+
+ return ret ? ret : count;
+}
+
+static struct attribute *
+fwat_alloc_attr(struct device *dev, const struct fwat_attr_config *config,
+ const char *attr_name, umode_t mode, enum fwat_property prop,
+ ssize_t (*show)(struct device *dev, const struct fwat_attribute *attr,
+ char *buf),
+ ssize_t (*store)(struct device *dev, const struct fwat_attribute *attr,
+ const char *buf, size_t count))
+{
+ struct fwat_attribute_ext *fattr;
+
+ fattr = devm_kzalloc(dev, sizeof(*fattr), GFP_KERNEL);
+ if (!fattr)
+ return NULL;
+
+ fattr->attr.attr.name = attr_name;
+ fattr->attr.attr.mode = mode;
+ fattr->attr.show = show;
+ fattr->attr.store = store;
+ fattr->prop = prop;
+ fattr->config = config;
+ sysfs_attr_init(&fattr->attr.attr);
+
+ return &fattr->attr.attr;
+}
+
+static struct attribute **
+fwat_create_attrs(struct device *dev, const struct fwat_attr_config *config)
+{
+ struct attribute **attrs;
+ enum fwat_property prop;
+ unsigned int index = 0;
+
+ attrs = devm_kcalloc(dev, config->num_props + 3, sizeof(*attrs), GFP_KERNEL);
+ if (!attrs)
+ return NULL;
+
+ /*
+ * Create optional attributes
+ */
Just a nit here; this probably doesn't need to be a multiline comment.
a single line like this is fine:
/* optional attributes */
+ for (; index < config->num_props; index++) {
+ prop = config->props[index];
+ attrs[index] = fwat_alloc_attr(dev, config, fwat_prop_labels[prop],
+ 0444, prop, fwat_property_show, NULL);
+ }
+
+ /*
+ * Create mandatory attributes
+ */
Same as above
Ack for these two.
+ attrs[index++] = fwat_alloc_attr(dev, config, "type", 0444, 0, fwat_type_show, NULL);
+ attrs[index++] = fwat_alloc_attr(dev, config, "current_value", 0644, 0,
Is this permission right? Some attributes could be considered more
sensitive can't they?
You are right. I assumed most drivers would want 0644 but think-lmi and
dell sysman use 0600.
I can add a mode_t mask to fwat_attr_config and use that for
current_value, while other properties would be masked by it, i.e.
`0444 & config->mode`
What do you think?
Yeah; current_value is the only sensitive one.