| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <386c1272-774d-4f3f-ae42-a45dac285339@t-8ch.de>
Date: Wed, 23 Apr 2025 18:57:47 +0200
From: Thomas Weißschuh <linux@...ssschuh.net>
To: Kurt Borja <kuurtb@...il.com>
Cc: Hans de Goede <hdegoede@...hat.com>,
Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>, Armin Wolf <W_Armin@....de>, platform-driver-x86@...r.kernel.org,
linux-kernel@...r.kernel.org, Joshua Grisham <josh@...huagrisham.com>
Subject: Re: [PATCH 1/2] platform/x86: firmware_attributes_class: Provide a
highlevel interface
On 2025-04-23 05:05:35-0300, Kurt Borja wrote:
> 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@...ssschuh.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@...ovo.com>");
> > +MODULE_AUTHOR("Thomas Weißschuh <linux@...ssschuh.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,
Hi Kurt,
> Are you still working on this patchset?
I did some more work on this patchset, adding higher-level
functionality. But so far I was not satisfied with the result.
> 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.
You're welcome to take over.
> 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.
Nice to hear :-)
> Let me know what you think!
Please go ahead!
Thomas
Powered by blists - more mailing lists