| 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: <bb054a10-1020-4eb4-a09d-219b71bad5ff@gmx.de>
Date: Wed, 6 Nov 2024 04:53:14 +0100
From: Armin Wolf <W_Armin@....de>
To: Mario Limonciello <mario.limonciello@....com>,
Hans de Goede <hdegoede@...hat.com>,
Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: "Rafael J . Wysocki" <rafael@...nel.org>, Len Brown <lenb@...nel.org>,
Maximilian Luz <luzmaximilian@...il.com>, Lee Chun-Yi <jlee@...e.com>,
Shyam Sundar S K <Shyam-sundar.S-k@....com>,
Corentin Chary <corentin.chary@...il.com>, "Luke D . Jones"
<luke@...nes.dev>, Ike Panhc <ike.pan@...onical.com>,
Henrique de Moraes Holschuh <hmh@....eng.br>,
Alexis Belmonte <alexbelm48@...il.com>,
Uwe Kleine-König <u.kleine-koenig@...gutronix.de>,
Ai Chao <aichao@...inos.cn>, Gergo Koteles <soyer@....hu>,
open list <linux-kernel@...r.kernel.org>,
"open list:ACPI" <linux-acpi@...r.kernel.org>,
"open list:MICROSOFT SURFACE PLATFORM PROFILE DRIVER"
<platform-driver-x86@...r.kernel.org>,
"open list:THINKPAD ACPI EXTRAS DRIVER"
<ibm-acpi-devel@...ts.sourceforge.net>,
Mark Pearson <mpearson-lenovo@...ebb.ca>,
Matthew Schwartz <matthew.schwartz@...ux.dev>
Subject: Re: [PATCH v4 09/20] ACPI: platform_profile: Create class for ACPI
platform profile
Am 05.11.24 um 16:33 schrieb Mario Limonciello:
> When registering a platform profile handler create a class device
> that will allow changing a single platform profile handler.
>
> Signed-off-by: Mario Limonciello <mario.limonciello@....com>
> ---
> drivers/acpi/platform_profile.c | 42 +++++++++++++++++++++++++++++---
> include/linux/platform_profile.h | 2 ++
> 2 files changed, 41 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> index fd1c4e9dccf0a..f8e9dc9a10d54 100644
> --- a/drivers/acpi/platform_profile.c
> +++ b/drivers/acpi/platform_profile.c
> @@ -5,6 +5,7 @@
> #include <linux/acpi.h>
> #include <linux/bits.h>
> #include <linux/init.h>
> +#include <linux/kdev_t.h>
> #include <linux/mutex.h>
> #include <linux/platform_profile.h>
> #include <linux/sysfs.h>
> @@ -22,6 +23,12 @@ static const char * const profile_names[] = {
> };
> static_assert(ARRAY_SIZE(profile_names) == PLATFORM_PROFILE_LAST);
>
> +static DEFINE_IDR(platform_profile_minor_idr);
Please use a IDA instead, you can take a look at the hwmon subsystem for an example.
> +
> +static const struct class platform_profile_class = {
> + .name = "platform-profile",
> +};
> +
> static ssize_t platform_profile_choices_show(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> @@ -113,6 +120,8 @@ void platform_profile_notify(void)
> {
> if (!cur_profile)
> return;
> + if (!class_is_registered(&platform_profile_class))
> + return;
> sysfs_notify(acpi_kobj, NULL, "platform_profile");
> }
> EXPORT_SYMBOL_GPL(platform_profile_notify);
> @@ -123,6 +132,9 @@ int platform_profile_cycle(void)
> enum platform_profile_option next;
> int err;
>
> + if (!class_is_registered(&platform_profile_class))
> + return -ENODEV;
> +
> scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
> if (!cur_profile)
> return -ENODEV;
> @@ -167,12 +179,33 @@ int platform_profile_register(struct platform_profile_handler *pprof)
> if (cur_profile)
> return -EEXIST;
>
> - err = sysfs_create_group(acpi_kobj, &platform_profile_group);
> - if (err)
> - return err;
> + if (!class_is_registered(&platform_profile_class)) {
> + /* class for individual handlers */
> + err = class_register(&platform_profile_class);
> + if (err)
> + return err;
I think it would be better to register the class during module initialization. This way userspace
applications can depend on the class directory being present if the driver has been loaded (even if
it might be empty).
> + /* legacy sysfs files */
> + err = sysfs_create_group(acpi_kobj, &platform_profile_group);
> + if (err)
> + goto cleanup_class;
> + }
> +
> + /* create class interface for individual handler */
> + pprof->minor = idr_alloc(&platform_profile_minor_idr, pprof, 0, 0, GFP_KERNEL);
> + pprof->class_dev = device_create(&platform_profile_class, pprof->dev,
> + MKDEV(0, pprof->minor), NULL, "platform-profile-%d",
> + pprof->minor);
Please use MKDEV(0, 0), as we have no associated character device.
> + if (IS_ERR(pprof->class_dev))
> + return PTR_ERR(pprof->class_dev);
You still need to free "minor" in case of an error.
> + dev_set_drvdata(pprof->class_dev, pprof);
Please use device_create() for setting the drvdata of the class device.
>
> cur_profile = pprof;
> return 0;
> +
> +cleanup_class:
> + class_unregister(&platform_profile_class);
> +
> + return err;
> }
> EXPORT_SYMBOL_GPL(platform_profile_register);
>
> @@ -181,6 +214,9 @@ int platform_profile_remove(struct platform_profile_handler *pprof)
> guard(mutex)(&profile_lock);
>
> sysfs_remove_group(acpi_kobj, &platform_profile_group);
> +
> + device_destroy(&platform_profile_class, MKDEV(0, pprof->minor));
Using device_unregister() would be more suitable here. Also keep in mind that "minor" still
needs to be freed. Again drivers/hwmon/hwmon.c could be useful in this regard.
Thanks,
Armin Wolf
> +
> cur_profile = NULL;
> return 0;
> }
> diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h
> index bcaf3aa39160f..f81c3afcb7575 100644
> --- a/include/linux/platform_profile.h
> +++ b/include/linux/platform_profile.h
> @@ -29,6 +29,8 @@ enum platform_profile_option {
> struct platform_profile_handler {
> const char *name;
> struct device *dev;
> + struct device *class_dev;
> + int minor;
> unsigned long choices[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
> int (*profile_get)(struct platform_profile_handler *pprof,
> enum platform_profile_option *profile);
Powered by blists - more mailing lists