Re: [PATCH 1/2] platform/x86: firmware_attributes_class: Provide a highlevel interface

["Kurt Borja" <kuurtb@gmail.com>]

On Tue Jan 7, 2025 at 2:05 PM -03, Thomas Weißschuh wrote:
> Currently each user of firmware_attributes_class has to manually set up
> kobjects, devices, etc.
> Provide a higher level API which takes care of the low-level details.
>
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>  drivers/platform/x86/firmware_attributes_class.c | 146 +++++++++++++++++++++++
>  drivers/platform/x86/firmware_attributes_class.h |  37 ++++++
>  2 files changed, 183 insertions(+)
>
> diff --git a/drivers/platform/x86/firmware_attributes_class.c b/drivers/platform/x86/firmware_attributes_class.c
> index 736e96c186d9dc6d945517f090e9af903e93bbf4..70ceae5215820098b017bfda991a3c2a7824c98e 100644
> --- a/drivers/platform/x86/firmware_attributes_class.c
> +++ b/drivers/platform/x86/firmware_attributes_class.c
> @@ -2,6 +2,9 @@
>  
>  /* Firmware attributes class helper module */
>  
> +#include <linux/device/class.h>
> +#include <linux/device.h>
> +#include <linux/kobject.h>
>  #include <linux/module.h>
>  #include "firmware_attributes_class.h"
>  
> @@ -22,6 +25,149 @@ static __exit void fw_attributes_class_exit(void)
>  }
>  module_exit(fw_attributes_class_exit);
>  
> +static ssize_t fw_attributes_sysfs_show(struct kobject *kobj, struct attribute *attr, char *buf)
> +{
> +	struct firmware_attributes_device *fwadev = to_firmware_attribute_device(kobj);
> +	const struct firmware_attribute *fw_attr = to_firmware_attribute(attr);
> +
> +	if (!fw_attr->show)
> +		return -EIO;
> +
> +	return fw_attr->show(fwadev, fw_attr, buf);
> +}
> +
> +static ssize_t fw_attributes_sysfs_store(struct kobject *kobj, struct attribute *attr,
> +					 const char *buf, size_t count)
> +{
> +	struct firmware_attributes_device *fwadev = to_firmware_attribute_device(kobj);
> +	const struct firmware_attribute *fw_attr = to_firmware_attribute(attr);
> +
> +	if (!fw_attr->store)
> +		return -EIO;
> +
> +	return fw_attr->store(fwadev, fw_attr, buf, count);
> +}
> +
> +static const struct sysfs_ops fw_attributes_sysfs_ops = {
> +	.show	= fw_attributes_sysfs_show,
> +	.store	= fw_attributes_sysfs_store,
> +};
> +
> +static void fw_attributes_attr_release(struct kobject *kobj)
> +{
> +	struct firmware_attributes_device *fwadev = to_firmware_attribute_device(kobj);
> +	struct device *cdev;
> +
> +	cdev = fwadev->dev;
> +
> +	kfree(fwadev);
> +	device_unregister(cdev);
> +}
> +
> +static const struct kobj_type fw_attributes_attr_type = {
> +	.sysfs_ops	= &fw_attributes_sysfs_ops,
> +	.release	= fw_attributes_attr_release,
> +};
> +
> +DEFINE_FREE(firmware_attributes_device_unregister, struct firmware_attributes_device *,
> +	    if (_T) firmware_attributes_device_unregister(_T))
> +
> +struct firmware_attributes_device *
> +firmware_attributes_device_register(struct device *parent, const char *name,
> +				    const struct attribute_group **groups, void *data)
> +{
> +	struct firmware_attributes_device *fwadev = NULL;
> +	struct device *cdev = NULL;
> +	int ret;
> +
> +	fwadev = kzalloc(sizeof(*fwadev), GFP_KERNEL);
> +	if (!fwadev)
> +		return ERR_PTR(-ENOMEM);
> +
> +	cdev = device_create(&firmware_attributes_class, parent, MKDEV(0, 0), "%s", name);
> +	if (IS_ERR(cdev))
> +		return ERR_CAST(cdev);
> +
> +	fwadev->data = data;
> +	fwadev->dev = cdev;
> +
> +	ret = kobject_init_and_add(&fwadev->attributes, &fw_attributes_attr_type, &cdev->kobj,
> +				   "attributes");
> +	if (ret) {
> +		device_del(cdev);
> +		return ERR_PTR(ret);
> +	}
> +
> +	if (groups) {
> +		ret = sysfs_create_groups(&fwadev->attributes, groups);
> +		if (ret) {
> +			firmware_attributes_device_unregister(fwadev);
> +			return ERR_PTR(ret);
> +		}
> +
> +		kobject_uevent(&fwadev->dev->kobj, KOBJ_CHANGE);
> +	}
> +
> +	return fwadev;
> +}
> +EXPORT_SYMBOL_GPL(firmware_attributes_device_register);
> +
> +void firmware_attributes_device_unregister(struct firmware_attributes_device *fwadev)
> +{
> +	kobject_del(&fwadev->attributes);
> +	kobject_put(&fwadev->attributes);
> +}
> +EXPORT_SYMBOL_GPL(firmware_attributes_device_unregister);
> +
> +static void devm_firmware_attributes_device_release(void *data)
> +{
> +	struct firmware_attributes_device *fwadev = data;
> +
> +	firmware_attributes_device_unregister(fwadev);
> +}
> +
> +struct firmware_attributes_device *
> +devm_firmware_attributes_device_register(struct device *parent, const char *name,
> +					 const struct attribute_group **groups, void *data)
> +{
> +	struct firmware_attributes_device *fwadev;
> +	int ret;
> +
> +	fwadev = firmware_attributes_device_register(parent, name, groups, data);
> +	if (IS_ERR(fwadev))
> +		return fwadev;
> +
> +	ret = devm_add_action_or_reset(parent, devm_firmware_attributes_device_release, fwadev);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	return fwadev;
> +}
> +EXPORT_SYMBOL_GPL(devm_firmware_attributes_device_register);
> +
> +static ssize_t firmware_attributes_type_show(struct firmware_attributes_device *fwadev,
> +					     const struct firmware_attribute *attr, char *buf)
> +{
> +	if (attr == &_firmware_attribute_type_string)
> +		return sysfs_emit(buf, "string\n");
> +	else if (attr == &_firmware_attribute_type_enumeration)
> +		return sysfs_emit(buf, "enumeration\n");
> +	else if (attr == &_firmware_attribute_type_integer)
> +		return sysfs_emit(buf, "integer\n");
> +	else
> +		return -EIO;
> +}
> +
> +#define __FW_TYPE_ATTR	__ATTR(type, 0444, firmware_attributes_type_show, NULL)
> +
> +const struct firmware_attribute _firmware_attribute_type_string = __FW_TYPE_ATTR;
> +EXPORT_SYMBOL_GPL(_firmware_attribute_type_string);
> +const struct firmware_attribute _firmware_attribute_type_enumeration = __FW_TYPE_ATTR;
> +EXPORT_SYMBOL_GPL(_firmware_attribute_type_enumeration);
> +const struct firmware_attribute _firmware_attribute_type_integer = __FW_TYPE_ATTR;
> +EXPORT_SYMBOL_GPL(_firmware_attribute_type_integer);
> +
>  MODULE_AUTHOR("Mark Pearson <markpearson@lenovo.com>");
> +MODULE_AUTHOR("Thomas Weißschuh <linux@weissschuh.net>");
>  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..66837ad9f65b8ca501dee73f48c01f2710d86bf5 100644
> --- a/drivers/platform/x86/firmware_attributes_class.h
> +++ b/drivers/platform/x86/firmware_attributes_class.h
> @@ -5,8 +5,45 @@
>  #ifndef FW_ATTR_CLASS_H
>  #define FW_ATTR_CLASS_H
>  
> +#include <linux/device.h>
>  #include <linux/device/class.h>
> +#include <linux/sysfs.h>
>  
>  extern const struct class firmware_attributes_class;
>  
> +struct firmware_attributes_device {
> +	struct device *dev;
> +	struct kobject attributes;
> +	void *data;
> +};
> +
> +struct firmware_attribute {
> +	struct attribute attr;
> +	ssize_t (*show)(struct firmware_attributes_device *fwadev,
> +			const struct firmware_attribute *attr, char *buf);
> +	ssize_t (*store)(struct firmware_attributes_device *fwadev,
> +			 const struct firmware_attribute *attr, const char *buf, size_t count);
> +};
> +
> +#define to_firmware_attribute(_a) container_of_const(_a, struct firmware_attribute, attr)
> +#define to_firmware_attribute_device(_s) \
> +	container_of_const(_s, struct firmware_attributes_device, attributes)
> +
> +extern const struct firmware_attribute _firmware_attribute_type_string;
> +#define firmware_attribute_type_string ((struct attribute *)&_firmware_attribute_type_string.attr)
> +extern const struct firmware_attribute _firmware_attribute_type_enumeration;
> +#define firmware_attribute_type_enumeration ((struct attribute *)&_firmware_attribute_type_enumeration.attr)
> +extern const struct firmware_attribute _firmware_attribute_type_integer;
> +#define firmware_attribute_type_integer ((struct attribute *)&_firmware_attribute_type_integer.attr)
> +
> +struct firmware_attributes_device * __must_check
> +firmware_attributes_device_register(struct device *parent, const char *name,
> +				    const struct attribute_group **groups, void *data);
> +
> +void firmware_attributes_device_unregister(struct firmware_attributes_device *fwadev);
> +
> +struct firmware_attributes_device * __must_check
> +devm_firmware_attributes_device_register(struct device *parent, const char *name,
> +					 const struct attribute_group **groups, void *data);
> +
>  #endif /* FW_ATTR_CLASS_H */

Hi Thomas,

Are you still working on this patchset?

If you don't mind I can take it from here. I'm planning on fixing a
couple memory leaks of this patch and extend it a bit more with some
helper macros for ABI compliant drivers. I might drop the test driver
though.

I ask this because I want to use this class in another series I'm
to do, and I think this patch is a great starting point.

Let me know what you think!

-- 
 ~ Kurt