| 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: <DB4SASRMA77T.JY8INTVOV1CW@gmail.com>
Date: Sun, 06 Jul 2025 04:12:49 -0300
From: "Kurt Borja" <kuurtb@...il.com>
To: Thomas Weißschuh <linux@...ssschuh.net>
Cc: "Hans de Goede" <hdegoede@...hat.com>, Ilpo Järvinen
<ilpo.jarvinen@...ux.intel.com>, "Joshua Grisham" <josh@...huagrisham.com>,
"Mark Pearson" <mpearson-lenovo@...ebb.ca>, "Armin Wolf" <W_Armin@....de>,
"Mario Limonciello" <mario.limonciello@....com>, "Antheas Kapenekakis"
<lkml@...heas.dev>, "Derek J. Clark" <derekjohn.clark@...il.com>, "Prasanth
Ksr" <prasanth.ksr@...l.com>, "Jorge Lopez" <jorge.lopez2@...com>,
<platform-driver-x86@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<Dell.Client.Kernel@...l.com>
Subject: Re: [PATCH v5 1/6] platform/x86: firmware_attributes_class: Add
device initialization methods
Hi Thomas,
On Sun Jul 6, 2025 at 3:42 AM -03, Thomas Weißschuh wrote:
> On 2025-07-05 00:33:56-0300, Kurt Borja wrote:
>> From: Thomas Weißschuh <linux@...ssschuh.net>
>>
>> Currently each user of firmware_attributes_class has to manually set up
>> kobjects, devices, etc.
>>
>> Provide this infrastructure out-of-the-box through the newly introduced
>> fwat_device_register().
>>
>> Reviewed-by: Mario Limonciello <mario.limonciello@....com>
>> Signed-off-by: Thomas Weißschuh <linux@...ssschuh.net>
>> Co-developed-by: Kurt Borja <kuurtb@...il.com>
>> Signed-off-by: Kurt Borja <kuurtb@...il.com>
>> ---
>> drivers/platform/x86/firmware_attributes_class.c | 125 +++++++++++++++++++++++
>> drivers/platform/x86/firmware_attributes_class.h | 28 +++++
>> 2 files changed, 153 insertions(+)
>>
>> diff --git a/drivers/platform/x86/firmware_attributes_class.c b/drivers/platform/x86/firmware_attributes_class.c
>> index 736e96c186d9dc6d945517f090e9af903e93bbf4..290364202cce64bb0e9046e0b2bbb8d85e2cbc6f 100644
>> --- a/drivers/platform/x86/firmware_attributes_class.c
>> +++ b/drivers/platform/x86/firmware_attributes_class.c
>> @@ -2,7 +2,14 @@
>>
>> /* Firmware attributes class helper module */
>>
>> +#include <linux/cleanup.h>
>> +#include <linux/device.h>
>> +#include <linux/device/class.h>
>> +#include <linux/kdev_t.h>
>> +#include <linux/kobject.h>
>> #include <linux/module.h>
>> +#include <linux/slab.h>
>> +#include <linux/types.h>
>> #include "firmware_attributes_class.h"
>>
>> const struct class firmware_attributes_class = {
>> @@ -10,6 +17,122 @@ const struct class firmware_attributes_class = {
>> };
>> EXPORT_SYMBOL_GPL(firmware_attributes_class);
>>
>> +static void fwat_device_release(struct device *dev)
>> +{
>> + struct fwat_device *fadev = to_fwat_device(dev);
>> +
>> + kfree(fadev);
>> +}
>> +
>> +/**
>> + * fwat_device_register - Create and register a firmware-attributes class
>> + * device
>> + * @parent: Parent device
>> + * @name: Name of the class device
>> + * @drvdata: Drvdata of the class device
>> + * @groups: Extra groups for the "attributes" directory
>> + *
>> + * Return: pointer to the new fwat_device on success, ERR_PTR on failure
>> + */
>> +struct fwat_device *
>> +fwat_device_register(struct device *parent, const char *name, void *drvdata,
>> + const struct attribute_group **groups)
>> +{
>> + struct fwat_device *fadev;
>> + int ret;
>> +
>> + if (!parent || !name)
>> + return ERR_PTR(-EINVAL);
>> +
>> + fadev = kzalloc(sizeof(*fadev), GFP_KERNEL);
>> + if (!fadev)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + fadev->groups = groups;
>> + fadev->dev.class = &firmware_attributes_class;
>> + fadev->dev.parent = parent;
>> + fadev->dev.release = fwat_device_release;
>> + dev_set_drvdata(&fadev->dev, drvdata);
>> + ret = dev_set_name(&fadev->dev, "%s", name);
>> + if (ret) {
>> + kfree(fadev);
>> + return ERR_PTR(ret);
>> + }
>> + ret = device_register(&fadev->dev);
>> + if (ret)
>> + return ERR_PTR(ret);
>
> I think we need a put_device() here.
Indeed. I'll fix it.
>
>> +
>> + fadev->attrs_kset = kset_create_and_add("attributes", NULL, &fadev->dev.kobj);
>> + if (!fadev->attrs_kset) {
>> + ret = -ENOMEM;
>> + goto out_device_unregister;
>> + }
>> +
>> + ret = sysfs_create_groups(&fadev->attrs_kset->kobj, groups);
>> + if (ret)
>> + goto out_kset_unregister;
>
> It would be nicer for userspace to add the device to the hierarchy
> only when it is set up fully.
> Replacing device_register() with a device_initialize() above and
> device_add() down here.
Sure!
In the end however, children kobjects corresponding to "firmware
attributes" will still be added after device_add(), with
fwat_create_group().
Obviously, that only applies if we can all agree on the approach of
Patch 2. If you could take a look I would be very grateful!
>
>> +
>> + return fadev;
>> +
>> +out_kset_unregister:
>> + kset_unregister(fadev->attrs_kset);
>
> I *think* the driver core should clean up any child objects
> automatically, so this is unnecessary.
Hmm - I think filesystem is automatically cleaned up. But not putting
down the kset ref would leak it's allocated memory.
I'll verify this.
>
>> +
>> +out_device_unregister:
>> + device_unregister(&fadev->dev);
>> +
>> + return ERR_PTR(ret);
>> +}
>> +EXPORT_SYMBOL_GPL(fwat_device_register);
>> +
>> +void fwat_device_unregister(struct fwat_device *fadev)
>> +{
>> + if (!fadev)
>> + return;
>> +
>> + sysfs_remove_groups(&fadev->attrs_kset->kobj, fadev->groups);
>> + kset_unregister(fadev->attrs_kset);
>
> The also the two lines above would be unnecessary.
For the reason above I think kset_unregister() is still required.
Removing groups manually, on the other hand, is not required (I think).
I can remove that call.
>
>> + device_unregister(&fadev->dev);
>> +}
>> +EXPORT_SYMBOL_GPL(fwat_device_unregister);
>> +
>> +static void devm_fwat_device_release(void *data)
>> +{
>> + struct fwat_device *fadev = data;
>> +
>> + fwat_device_unregister(fadev);
>> +}
>> +
>> +/**
>> + * devm_fwat_device_register - Create and register a firmware-attributes class
>> + * device
>> + * @parent: Parent device
>> + * @name: Name of the class device
>> + * @data: Drvdata of the class device
>> + * @groups: Extra groups for the class device (Optional)
>> + *
>> + * Device managed version of fwat_device_register().
>> + *
>> + * Return: pointer to the new fwat_device on success, ERR_PTR on failure
>> + */
>> +struct fwat_device *
>> +devm_fwat_device_register(struct device *parent, const char *name, void *data,
>> + const struct attribute_group **groups)
>> +{
>> + struct fwat_device *fadev;
>> + int ret;
>> +
>> + fadev = fwat_device_register(parent, name, data, groups);
>> + if (IS_ERR(fadev))
>> + return fadev;
>> +
>> + ret = devm_add_action_or_reset(parent, devm_fwat_device_release, fadev);
>> + if (ret)
>> + return ERR_PTR(ret);
>> +
>> + return fadev;
>> +}
>> +EXPORT_SYMBOL_GPL(devm_fwat_device_register);
>
> ... and also all of the devm stuff.
Could you elaborate on this?
>
>> +
>> static __init int fw_attributes_class_init(void)
>> {
>> return class_register(&firmware_attributes_class);
>> @@ -23,5 +146,7 @@ static __exit void fw_attributes_class_exit(void)
>> module_exit(fw_attributes_class_exit);
>>
>> MODULE_AUTHOR("Mark Pearson <markpearson@...ovo.com>");
>> +MODULE_AUTHOR("Thomas Weißschuh <linux@...ssschuh.net>");
>> +MODULE_AUTHOR("Kurt Borja <kuurtb@...il.com>");
>> MODULE_DESCRIPTION("Firmware attributes class helper module");
>> MODULE_LICENSE("GPL");
>
> <snip>
Thanks for reviewing this!
--
~ Kurt
Powered by blists - more mailing lists