| Message ID | 20241107060254.17615-9-mario.limonciello@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for binding ACPI platform profile to multiple drivers | expand |
On 11/7/2024 02:16, Armin Wolf wrote: > 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@squebb.ca> >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.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. Ack. > >> + 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. > I previously did this indirectly by letting them set it in the "struct platform_profile_handler *pprof" and then used that value. You had said that wasn't necessary so I dropped that patch. I would rather go back to including that then having another argument to platform_profile_register(). > 2. Please use the fourth argument of device_create() instead of > dev_set_drvdata(). OK. > > 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);
Am 07.11.24 um 22:09 schrieb Mario Limonciello: > On 11/7/2024 02:16, Armin Wolf wrote: >> 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@squebb.ca> >>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.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. > > Ack. > >> >>> + 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. >> > > I previously did this indirectly by letting them set it in the > "struct platform_profile_handler *pprof" and then used that value. > > You had said that wasn't necessary so I dropped that patch. I would > rather go back to including that then having another argument to > platform_profile_register(). > I meant that requiring "dev" to be set is not necessary. Having a "dev" field inside struct platform_profile_handler is fine. Thanks, Armin Wolf >> 2. Please use the fourth argument of device_create() instead of >> dev_set_drvdata(). > > OK. > >> >> 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); >
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); + pprof->class_dev = device_create(&platform_profile_class, NULL, + MKDEV(0, 0), NULL, "platform-profile-%d", + pprof->minor); + 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);