| 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: <84a647ba-50ec-4d60-b4be-758ff50335bd@gmx.de>
Date: Thu, 7 Nov 2024 09:16:25 +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 v5 08/20] ACPI: platform_profile: Create class for ACPI
platform profile
Am 07.11.24 um 07:02 schrieb Mario Limonciello:
> When registering a platform profile handler create a class device
> that will allow changing a single platform profile handler.
>
> Tested-by: Mark Pearson <mpearson-lenovo@...ebb.ca>
> Signed-off-by: Mario Limonciello <mario.limonciello@....com>
> ---
> v5:
> * Use ida instead of idr
> * Use device_unregister instead of device_destroy()
> * MKDEV (0, 0)
> ---
> drivers/acpi/platform_profile.c | 50 +++++++++++++++++++++++++++++---
> include/linux/platform_profile.h | 2 ++
> 2 files changed, 48 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> index 0450bdae7c88b..652034b71ee9b 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_IDA(platform_profile_ida);
> +
> +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;
> @@ -163,20 +175,50 @@ 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;
> + /* 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 = ida_alloc(&platform_profile_ida, GFP_KERNEL);
Missing error handling.
> + pprof->class_dev = device_create(&platform_profile_class, NULL,
> + MKDEV(0, 0), NULL, "platform-profile-%d",
> + pprof->minor);
Two things:
1. Please allow drivers to pass in their struct device so the resulting class device
has a parent device. This would allow userspace applications to determine which device
handles which platform profile device. This parameter is optional and can be NULL.
2. Please use the fourth argument of device_create() instead of dev_set_drvdata().
Thanks,
Armin Wolf
> + if (IS_ERR(pprof->class_dev)) {
> + err = PTR_ERR(pprof->class_dev);
> + goto cleanup_ida;
> + }
> + dev_set_drvdata(pprof->class_dev, pprof);
>
> cur_profile = pprof;
> return 0;
> +
> +cleanup_ida:
> + ida_free(&platform_profile_ida, pprof->minor);
> +
> +cleanup_class:
> + class_unregister(&platform_profile_class);
> +
> + return err;
> }
> EXPORT_SYMBOL_GPL(platform_profile_register);
>
> int platform_profile_remove(struct platform_profile_handler *pprof)
> {
> + int id;
> guard(mutex)(&profile_lock);
>
> - sysfs_remove_group(acpi_kobj, &platform_profile_group);
> + id = pprof->minor;
> + device_unregister(pprof->class_dev);
> + ida_free(&platform_profile_ida, id);
> +
> cur_profile = NULL;
> return 0;
> }
> diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h
> index 58279b76d740e..d92a035e6ba6a 100644
> --- a/include/linux/platform_profile.h
> +++ b/include/linux/platform_profile.h
> @@ -28,6 +28,8 @@ enum platform_profile_option {
>
> struct platform_profile_handler {
> const char *name;
> + 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