| 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: <05563b0c-861f-4046-9d50-87296d1bf6a2@t-8ch.de>
Date: Sun, 6 Jul 2025 08:42:01 +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>, 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
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.
> +
> + 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.
> +
> + 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.
> +
> +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.
> + 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.
> +
> 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>
Powered by blists - more mailing lists