Re: [PATCH RFC 2/5] platform/x86: firmware_attributes_class: Add a high level API

[Mario Limonciello <mario.limonciello@amd.com>]

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

Yeah; current_value is the only sensitive one.