| Message ID | 20241107060254.17615-9-mario.limonciello@amd.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Add support for binding ACPI platform profile to multiple drivers | expand |
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. > + 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);
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);