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 >> --- >> 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 >> #include >> #include >> +#include >> #include >> #include "firmware_attributes_class.h" >> >> +#define to_fwat_attribute_ext(_a) container_of_const(_a, struct fwat_attribute_ext, attr) >> + >> +struct fwat_attribute_ext { >> + struct fwat_attribute attr; >> + enum fwat_property prop; >> + const struct fwat_attr_config *config; >> +}; >> + >> const struct class firmware_attributes_class = { >> .name = "firmware-attributes", >> }; >> EXPORT_SYMBOL_GPL(firmware_attributes_class); >> >> +static const char * const fwat_type_labels[] = { >> + [fwat_type_integer] = "integer", >> + [fwat_type_string] = "string", >> + [fwat_type_enumeration] = "enumeration", >> +}; >> + >> +static const char * const fwat_prop_labels[] = { >> + [FWAT_PROP_DISPLAY_NAME] = "display_name", >> + [FWAT_PROP_LANGUAGE_CODE] = "display_name_language_code", >> + [FWAT_PROP_DEFAULT] = "default", >> + >> + [FWAT_INT_PROP_MIN] = "min_value", >> + [FWAT_INT_PROP_MAX] = "max_value", >> + [FWAT_INT_PROP_INCREMENT] = "scalar_increment", >> + >> + [FWAT_STR_PROP_MIN] = "min_length", >> + [FWAT_STR_PROP_MAX] = "max_length", >> + >> + [FWAT_ENUM_PROP_POSSIBLE_VALUES] = "possible_values", >> +}; >> + >> +static ssize_t >> +fwat_type_show(struct device *dev, const struct fwat_attribute *attr, char *buf) >> +{ >> + const struct fwat_attribute_ext *ext = to_fwat_attribute_ext(attr); >> + const struct fwat_attr_config *config = ext->config; >> + >> + return sysfs_emit(buf, "%s\n", fwat_type_labels[config->type]); >> +} >> + >> +static ssize_t >> +fwat_property_show(struct device *dev, const struct fwat_attribute *attr, char *buf) >> +{ >> + const struct fwat_attribute_ext *ext = to_fwat_attribute_ext(attr); >> + const struct fwat_attr_config *config = ext->config; >> + >> + if (!config->ops->prop_read) >> + return -EOPNOTSUPP; >> + >> + return config->ops->prop_read(dev, config->aux, ext->prop, buf); >> +} >> + >> +static ssize_t >> +fwat_current_value_show(struct device *dev, const struct fwat_attribute *attr, char *buf) >> +{ >> + const struct fwat_attribute_ext *ext = to_fwat_attribute_ext(attr); >> + const struct fwat_attr_config *config = ext->config; >> + const char *str; >> + long int_val; >> + int ret; >> + >> + switch (config->type) { >> + case fwat_type_integer: >> + ret = config->ops->integer_read(dev, config->aux, &int_val); >> + if (ret) >> + return ret; >> + >> + return sysfs_emit(buf, "%ld\n", int_val); >> + case fwat_type_string: >> + ret = config->ops->string_read(dev, config->aux, &str); >> + if (ret) >> + return ret; >> + >> + return sysfs_emit(buf, "%s\n", str); >> + case fwat_type_enumeration: >> + ret = config->ops->enumeration_read(dev, config->aux, &str); >> + if (ret) >> + return ret; >> + >> + return sysfs_emit(buf, "%s\n", str); >> + default: >> + return -EOPNOTSUPP; >> + } >> +} >> + >> +static ssize_t >> +fwat_current_value_store(struct device *dev, const struct fwat_attribute *attr, >> + const char *buf, size_t count) >> +{ >> + const struct fwat_attribute_ext *ext = to_fwat_attribute_ext(attr); >> + const struct fwat_attr_config *config = ext->config; >> + long int_val; >> + int ret; >> + >> + switch (config->type) { >> + case fwat_type_integer: >> + ret = kstrtol(buf, 0, &int_val); >> + if (ret) >> + return ret; >> + >> + ret = config->ops->integer_write(dev, config->aux, int_val); >> + break; >> + case fwat_type_string: >> + ret = config->ops->string_write(dev, config->aux, buf); >> + break; >> + case fwat_type_enumeration: >> + ret = config->ops->enumeration_write(dev, config->aux, buf); >> + break; >> + default: >> + return -EOPNOTSUPP; >> + } >> + >> + return ret ? ret : count; >> +} >> + >> +static struct attribute * >> +fwat_alloc_attr(struct device *dev, const struct fwat_attr_config *config, >> + const char *attr_name, umode_t mode, enum fwat_property prop, >> + ssize_t (*show)(struct device *dev, const struct fwat_attribute *attr, >> + char *buf), >> + ssize_t (*store)(struct device *dev, const struct fwat_attribute *attr, >> + const char *buf, size_t count)) >> +{ >> + struct fwat_attribute_ext *fattr; >> + >> + fattr = devm_kzalloc(dev, sizeof(*fattr), GFP_KERNEL); >> + if (!fattr) >> + return NULL; >> + >> + fattr->attr.attr.name = attr_name; >> + fattr->attr.attr.mode = mode; >> + fattr->attr.show = show; >> + fattr->attr.store = store; >> + fattr->prop = prop; >> + fattr->config = config; >> + sysfs_attr_init(&fattr->attr.attr); >> + >> + return &fattr->attr.attr; >> +} >> + >> +static struct attribute ** >> +fwat_create_attrs(struct device *dev, const struct fwat_attr_config *config) >> +{ >> + struct attribute **attrs; >> + enum fwat_property prop; >> + unsigned int index = 0; >> + >> + attrs = devm_kcalloc(dev, config->num_props + 3, sizeof(*attrs), GFP_KERNEL); >> + if (!attrs) >> + return NULL; >> + >> + /* >> + * Create optional attributes >> + */ > Just a nit here; this probably doesn't need to be a multiline comment. > a single line like this is fine: > > /* optional attributes */ > >> + for (; index < config->num_props; index++) { >> + prop = config->props[index]; >> + attrs[index] = fwat_alloc_attr(dev, config, fwat_prop_labels[prop], >> + 0444, prop, fwat_property_show, NULL); >> + } >> + >> + /* >> + * Create mandatory attributes >> + */ > > Same as above Ack for these two. > >> + attrs[index++] = fwat_alloc_attr(dev, config, "type", 0444, 0, fwat_type_show, NULL); >> + attrs[index++] = fwat_alloc_attr(dev, config, "current_value", 0644, 0, > > Is this permission right? Some attributes could be considered more > sensitive can't they? You are right. I assumed most drivers would want 0644 but think-lmi and dell sysman use 0600. I can add a mode_t mask to fwat_attr_config and use that for current_value, while other properties would be masked by it, i.e. `0444 & config->mode` What do you think? -- ~ Kurt > >> + fwat_current_value_show, fwat_current_value_store); >> + >> + return attrs; >> +} >> + >> +static const struct attribute_group * >> +fwat_create_group(struct device *dev, const struct fwat_attr_config *config) >> +{ >> + struct attribute_group *group; >> + struct attribute **attrs; >> + >> + group = devm_kzalloc(dev, sizeof(*group), GFP_KERNEL); >> + if (!group) >> + return NULL; >> + >> + attrs = fwat_create_attrs(dev, config); >> + if (!attrs) >> + return NULL; >> + >> + group->name = config->name; >> + group->attrs = attrs; >> + >> + return group; >> +} >> + >> +static const struct attribute_group ** >> +fwat_create_auto_groups(struct device *dev, const struct fwat_dev_config *config) >> +{ >> + const struct attribute_group **groups; >> + const struct attribute_group *grp; >> + unsigned int index = 0; >> + size_t ngroups = 0; >> + >> + while (config->attrs_config[ngroups]) >> + ngroups++; >> + >> + groups = devm_kcalloc(dev, ngroups + 1, sizeof(*groups), GFP_KERNEL); >> + if (!groups) >> + return NULL; >> + >> + for (unsigned int i = 0; i < ngroups; i++) { >> + if (config->is_visible && >> + !config->is_visible(dev, config->attrs_config[i])) >> + continue; >> + >> + grp = fwat_create_group(dev, config->attrs_config[i]); >> + if (!grp) >> + return NULL; >> + >> + groups[index++] = grp; >> + } >> + >> + return groups; >> +} >> + >> static ssize_t fwat_attrs_kobj_show(struct kobject *kobj, struct attribute *attr, >> char *buf) >> { >> @@ -61,6 +280,7 @@ static const struct kobj_type fwat_attrs_ktype = { >> * device >> * @parent: Parent device >> * @name: Name of the class device >> + * @config: Device configuration >> * @data: Drvdata of the class device >> * @groups: Sysfs groups for the custom `fwat_attrs_ktype` kobj_type >> * >> @@ -72,8 +292,10 @@ static const struct kobj_type fwat_attrs_ktype = { >> */ >> struct fwat_device * >> fwat_device_register(struct device *parent, const char *name, void *data, >> + const struct fwat_dev_config *config, >> const struct attribute_group **groups) >> { >> + const struct attribute_group **auto_groups; >> struct fwat_device *fadev; >> struct device *dev; >> int ret; >> @@ -97,19 +319,36 @@ fwat_device_register(struct device *parent, const char *name, void *data, >> if (ret) >> goto out_kobj_put; >> >> - if (groups) { >> - ret = sysfs_create_groups(&fadev->attrs_kobj, groups); >> + if (config) { >> + auto_groups = fwat_create_auto_groups(dev, config); >> + if (!auto_groups) { >> + ret = -ENOMEM; >> + goto out_kobj_unregister; >> + } >> + >> + ret = sysfs_create_groups(&fadev->attrs_kobj, auto_groups); >> if (ret) >> goto out_kobj_unregister; >> } >> >> + if (groups) { >> + ret = sysfs_create_groups(&fadev->attrs_kobj, groups); >> + if (ret) >> + goto out_remove_auto_groups; >> + } >> + >> fadev->dev = dev; >> fadev->groups = groups; >> + fadev->auto_groups = groups; >> >> kobject_uevent(&fadev->attrs_kobj, KOBJ_ADD); >> >> return fadev; >> >> +out_remove_auto_groups: >> + if (config) >> + sysfs_remove_groups(&fadev->attrs_kobj, auto_groups); >> + >> out_kobj_unregister: >> kobject_del(&fadev->attrs_kobj); >> >> @@ -125,6 +364,8 @@ void fwat_device_unregister(struct fwat_device *fwadev) >> { >> if (fwadev->groups) >> sysfs_remove_groups(&fwadev->attrs_kobj, fwadev->groups); >> + if (fwadev->auto_groups) >> + sysfs_remove_groups(&fwadev->attrs_kobj, fwadev->auto_groups); >> kobject_del(&fwadev->attrs_kobj); >> kobject_put(&fwadev->attrs_kobj); >> device_unregister(fwadev->dev); >> @@ -143,6 +384,7 @@ static void devm_fwat_device_release(void *data) >> * device >> * @parent: Parent device >> * @name: Name of the class device >> + * @config: Device configuration >> * @data: Drvdata of the class device >> * @groups: Sysfs groups for the custom `fwat_attrs_ktype` kobj_type >> * >> @@ -156,12 +398,13 @@ static void devm_fwat_device_release(void *data) >> */ >> struct fwat_device * >> devm_fwat_device_register(struct device *parent, const char *name, void *data, >> + const struct fwat_dev_config *config, >> const struct attribute_group **groups) >> { >> struct fwat_device *fadev; >> int ret; >> >> - fadev = fwat_device_register(parent, name, data, groups); >> + fadev = fwat_device_register(parent, name, data, config, groups); >> if (IS_ERR(fadev)) >> return fadev; >> >> diff --git a/drivers/platform/x86/firmware_attributes_class.h b/drivers/platform/x86/firmware_attributes_class.h >> index ad94bf91e5af30a2b8feb9abf224ee6f0d17600a..f250875d785c3439682c43c693f98e1c20e9ff54 100644 >> --- a/drivers/platform/x86/firmware_attributes_class.h >> +++ b/drivers/platform/x86/firmware_attributes_class.h >> @@ -18,11 +18,14 @@ extern const struct class firmware_attributes_class; >> * @dev: The class device. >> * @attrs_kobj: The "attributes" root kobject. >> * @groups: Sysfs groups attached to the @attrs_kobj. >> + * @auto_groups: Sysgs groups generated from &struct fwat_attr_config attached. >> + * to the @attrs_kobj >> */ >> struct fwat_device { >> struct device *dev; >> struct kobject attrs_kobj; >> const struct attribute_group **groups; >> + const struct attribute_group **auto_groups; >> }; >> >> #define to_fwat_device(_k) container_of_const(_k, struct fwat_device, attrs_kobj) >> @@ -30,6 +33,7 @@ struct fwat_device { >> /** >> * struct fwat_attribute - The firmware-attributes's custom attribute >> * @attr: Embedded struct attribute. >> + * @aux: Auxiliary number defined by the user. >> * @show: Show method called by the "attributes" kobject's ktype. >> * @store: Store method called by the "attributes" kobject's ktype. >> */ >> @@ -43,14 +47,238 @@ struct fwat_attribute { >> >> #define to_fwat_attribute(_a) container_of_const(_a, struct fwat_attribute, attr) >> >> +enum fwat_attr_type { >> + fwat_type_integer, >> + fwat_type_string, >> + fwat_type_enumeration, >> +}; >> + >> +enum fwat_property { >> + FWAT_PROP_DISPLAY_NAME, >> + FWAT_PROP_LANGUAGE_CODE, >> + FWAT_PROP_DEFAULT, >> + >> + FWAT_INT_PROP_MIN, >> + FWAT_INT_PROP_MAX, >> + FWAT_INT_PROP_INCREMENT, >> + >> + FWAT_STR_PROP_MIN, >> + FWAT_STR_PROP_MAX, >> + >> + FWAT_ENUM_PROP_POSSIBLE_VALUES, >> +}; >> + >> +struct fwat_attr_config; >> + >> +/** >> + * struct fwat_attr_ops - Operations for a firmware *attribute* >> + * @prop_read: Callback for retrieving each configured property of an attribute. >> + * @integer_read: Callback for reading the current_value of an attribute of >> + * type *integer*. >> + * @integer_write: Callback for writing the current_value of an attribute of >> + * type *integer*. >> + * @string_read: Callback for reading the current_value of an attribute of type >> + * *string*. >> + * @string_write: Callback for writing the current_value of an attribute of type >> + * *string*. >> + * @enumeration_read: Callback for reading the current_value of an attribute of >> + * type *enumeration*. >> + * @enumeration_write: Callback for writing the current_value of an attribute >> + * of type *enumeration*. >> + */ >> +struct fwat_attr_ops { >> + ssize_t (*prop_read)(struct device *dev, long aux, >> + enum fwat_property prop, const char *buf); >> + union { >> + struct { >> + int (*integer_read)(struct device *dev, long aux, >> + long *val); >> + int (*integer_write)(struct device *dev, long aux, >> + long val); >> + }; >> + struct { >> + int (*string_read)(struct device *dev, long aux, >> + const char **str); >> + int (*string_write)(struct device *dev, long aux, >> + const char *str); >> + }; >> + struct { >> + int (*enumeration_read)(struct device *dev, long aux, >> + const char **str); >> + int (*enumeration_write)(struct device *dev, long aux, >> + const char *str); >> + }; >> + }; >> +}; >> + >> +/** >> + * struct fwat_attr_config - Configuration for a single firmware *attribute* >> + * @name: Name of the sysfs group associated with this *attribute*. >> + * @type: Type of this *attribute*. >> + * @aux: Auxiliary number defined by the user, which will be passed to >> + * read/write callbacks. >> + * @ops: Operations for this *attribute*. >> + * @props: Array of properties of this *attribute*. >> + * @num_props: Size of the props array. >> + */ >> +struct fwat_attr_config { >> + const char *name; >> + enum fwat_attr_type type; >> + long aux; >> + const struct fwat_attr_ops *ops; >> + const enum fwat_property *props; >> + size_t num_props; >> +}; >> + >> +/** >> + * DEFINE_SIMPLE_FWAT_OPS() - Define static &struct fwat_attr_ops for a simple >> + * *attribute* with no properties, i.e. No >> + * &fwat_attr_ops.read_prop callback >> + * @_name: Prefix of the `read` and `write` callbacks. >> + * @_type: Firmware *attribute* type. >> + * >> + * Example: >> + * >> + * static int example_read(...) {...} >> + * static int example_write(...) {...} >> + * >> + * DEFINE_SIMPLE_FWAT_OPS(example, ...); >> + */ >> +#define DEFINE_SIMPLE_FWAT_OPS(_name, _type) \ >> + static const struct fwat_attr_ops _name##_ops = { \ >> + ._type##_read = _name##_read, \ >> + ._type##_write = _name##_write, \ >> + } >> + >> +/** >> + * DEFINE_FWAT_OPS() - Define static &struct fwat_attr_ops with all callbacks. >> + * @_name: Prefix of the `read` and `write` callbacks. >> + * @_type: Firmware *attribute* type. >> + * >> + * Example: >> + * >> + * static int example_read(...) {...} >> + * static int example_write(...) {...} >> + * static int example_prop_read(...) {...} >> + * >> + * DEFINE_FWAT_OPS(example, ...); >> + */ >> +#define DEFINE_FWAT_OPS(_name, _type) \ >> + static const struct fwat_attr_ops _name##_ops = { \ >> + .prop_read = _name##_prop_read, \ >> + ._type##_read = _name##_read, \ >> + ._type##_write = _name##_write, \ >> + } >> + >> +/** >> + * FWAT_CONFIG() - Configuration pointer for a single firmware *attribute*. >> + * @_name: String name of this *attribute*. >> + * @_type: Firmware *attribute* type. >> + * @_ops: Pointer to &struct fwat_attr_ops. >> + * @_props: Pointer to a enum fwat_property array. >> + * @_num_props: Size of the @_props array. >> + * >> + * This is a convenience macro to quickly construct a &struct fwat_attr_config >> + * array, which will be passed to &struct fwat_dev_config. >> + * >> + * Example: >> + * >> + * static int example_read(...) {...} >> + * static int example_write(...) {...} >> + * static int example_prop_read(...) {...} >> + * >> + * DEFINE_FWAT_OPS(example, ...); >> + * >> + * static const enum fwat_property props[] = {...}; >> + * >> + * static const struct fwat_attr_config * const attrs_config[] = { >> + * FWAT_CONFIG(example, ..., &example_fwat_ops, props, >> + * ARRAY_SIZE(props)), >> + * ... >> + * NULL >> + * }; >> + * >> + * static const struct fwat_dev_config fdev_config = { >> + * .attrs_config = attrs_config, >> + * ... >> + * } >> + */ >> +#define FWAT_CONFIG(_name, _type, _ops, _props, _num_props) \ >> + (&(const struct fwat_attr_config) { \ >> + .name = _name, \ >> + .type = fwat_type_##_type, \ >> + .ops = _ops, \ >> + .num_props = _num_props, \ >> + .props = _props, \ >> + }) >> + >> +/** >> + * FWAT_CONFIG_AUX() - Configuration pointer for a single firmware *attribute* >> + * with an auxiliary number defined by the user >> + * @_name: String name of this *attribute*. >> + * @_type: Firmware *attribute* type. >> + * @_aux: Auxiliary number defined by the user. >> + * @_ops: Pointer to &struct fwat_attr_ops. >> + * @_props: Pointer to a enum fwat_property array. >> + * @_num_props: Size of the @_props array. >> + * >> + * This is a convenience macro to quickly construct a &struct fwat_attr_config >> + * array, which will be passed to &struct fwat_dev_config. >> + * >> + * Example: >> + * >> + * static int example_read(...) {...} >> + * static int example_write(...) {...} >> + * static int example_prop_read(...) {...} >> + * >> + * DEFINE_FWAT_OPS(example, ...); >> + * >> + * static const enum fwat_property props[] = {...}; >> + * >> + * static const struct fwat_attr_config * const config[] = { >> + * FWAT_CONFIG_AUX(example, ..., n, &example_fwat_ops, props, >> + * ARRAY_SIZE(props)), >> + * ... >> + * NULL >> + * }; >> + * >> + * static const struct fwat_dev_config fdev_config = { >> + * .attrs_config = attrs_config, >> + * ... >> + * } >> + */ >> +#define FWAT_CONFIG_AUX(_name, _type, _aux, _ops, _props, _num_props) \ >> + (&(const struct fwat_attr_config) { \ >> + .name = _name, \ >> + .type = fwat_type_##_type, \ >> + .aux = _aux, \ >> + .ops = _ops, \ >> + .num_props = _num_props, \ >> + .props = _props, \ >> + }) >> + >> +/** >> + * struct fwat_dev_config - Configuration for this devices's >> + * &fwat_device.auto_groups >> + * @attrs_config: NULL terminated &struct fwat_attr_config array. >> + * @is_visible: Optional visibility callback to determine the visibility >> + * of each auto_group. >> + */ >> +struct fwat_dev_config { >> + const struct fwat_attr_config *const *attrs_config; >> + bool (*is_visible)(struct device *dev, const struct fwat_attr_config *config); >> +}; >> + >> struct fwat_device * __must_check >> fwat_device_register(struct device *parent, const char *name, void *data, >> + const struct fwat_dev_config *config, >> const struct attribute_group **groups); >> >> void fwat_device_unregister(struct fwat_device *fwadev); >> >> struct fwat_device * __must_check >> devm_fwat_device_register(struct device *parent, const char *name, void *data, >> + const struct fwat_dev_config *config, >> const struct attribute_group **groups); >> >> #endif /* FW_ATTR_CLASS_H */ >>