| Message ID | 20241109044151.29804-11-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 09.11.24 um 05:41 schrieb Mario Limonciello: > When registering a platform profile handler create a class device > that will allow changing a single platform profile handler. > > The class and sysfs group are no longer needed when the platform profile > core is a module and unloaded, so remove them at that time as well. > > Tested-by: Mark Pearson <mpearson-lenovo@squebb.ca> > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > --- > v6: > * Catch failures in ida_alloc > * Use 4th argument of device_create instead of dev_set_drvdata() > * Squash unregister patch > * Add module init callback > * Move class creation to module init > * Update visibility based on group presence > * Add back parent device > v5: > * Use ida instead of idr > * Use device_unregister instead of device_destroy() > * MKDEV (0, 0) > --- > drivers/acpi/platform_profile.c | 88 ++++++++++++++++++++++++++++++-- > include/linux/platform_profile.h | 2 + > 2 files changed, 85 insertions(+), 5 deletions(-) > > diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c > index 32affb75e782d..ef6af2c655524 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) > @@ -105,8 +112,25 @@ static struct attribute *platform_profile_attrs[] = { > NULL > }; > > +static int profile_class_registered(struct device *dev, const void *data) > +{ > + return 1; > +} > + > +static umode_t profile_class_is_visible(struct kobject *kobj, struct attribute *attr, int idx) > +{ > + if (!class_find_device(&platform_profile_class, NULL, NULL, profile_class_registered)) > + return 0; > + if (attr == &dev_attr_platform_profile_choices.attr) > + return 0444; > + if (attr == &dev_attr_platform_profile.attr) > + return 0644; > + return 0; > +} > + > static const struct attribute_group platform_profile_group = { > - .attrs = platform_profile_attrs > + .attrs = platform_profile_attrs, > + .is_visible = profile_class_is_visible, > }; > > void platform_profile_notify(struct platform_profile_handler *pprof) > @@ -123,6 +147,9 @@ int platform_profile_cycle(void) > enum platform_profile_option next; > int err; > > + if (!class_is_registered(&platform_profile_class)) > + return -ENODEV; This check is pointless since the platform profile class will always be registered during module initialization. Please remove it. > + > scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) { > if (!cur_profile) > return -ENODEV; > @@ -164,25 +191,76 @@ 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; > + /* create class interface for individual handler */ > + pprof->minor = ida_alloc(&platform_profile_ida, GFP_KERNEL); > + if (pprof->minor < 0) > + return pprof->minor; > + pprof->class_dev = device_create(&platform_profile_class, pprof->dev, > + MKDEV(0, 0), pprof, "platform-profile-%d", > + pprof->minor); > + if (IS_ERR(pprof->class_dev)) { > + err = PTR_ERR(pprof->class_dev); > + goto cleanup_ida; > + } > > cur_profile = pprof; > + > + err = sysfs_update_group(acpi_kobj, &platform_profile_group); > + if (err) > + goto cleanup_cur; > + > return 0; > + > +cleanup_cur: > + cur_profile = NULL; > + device_unregister(pprof->class_dev); > + > +cleanup_ida: > + ida_free(&platform_profile_ida, pprof->minor); > + > + 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; > + > + sysfs_update_group(acpi_kobj, &platform_profile_group); > + > return 0; > } > EXPORT_SYMBOL_GPL(platform_profile_remove); > > +static int __init platform_profile_init(void) > +{ > + int err; > + > + err = class_register(&platform_profile_class); > + if (err) > + return err; > + > + err = sysfs_create_group(acpi_kobj, &platform_profile_group); > + if (err) > + class_unregister(&platform_profile_class); > + > + return err; > +} Please use a blank line after function/struct/union/enum declarations. Apart from those minor issues the patch looks quite nice, so with those issues being fixed: Reviewed-by: Armin Wolf <W_Armin@gmx.de> > +static void __exit platform_profile_exit(void) > +{ > + class_unregister(&platform_profile_class); > + sysfs_remove_group(acpi_kobj, &platform_profile_group); > +} > +module_init(platform_profile_init); > +module_exit(platform_profile_exit); > + > MODULE_AUTHOR("Mark Pearson <markpearson@lenovo.com>"); > MODULE_DESCRIPTION("ACPI platform profile sysfs interface"); > MODULE_LICENSE("GPL"); > diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h > index 8ec0b8da56db5..a888fd085c513 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);
Am 17.11.24 um 20:11 schrieb Armin Wolf: > Am 09.11.24 um 05:41 schrieb Mario Limonciello: > >> When registering a platform profile handler create a class device >> that will allow changing a single platform profile handler. >> >> The class and sysfs group are no longer needed when the platform profile >> core is a module and unloaded, so remove them at that time as well. >> >> Tested-by: Mark Pearson <mpearson-lenovo@squebb.ca> >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >> --- >> v6: >> * Catch failures in ida_alloc >> * Use 4th argument of device_create instead of dev_set_drvdata() >> * Squash unregister patch >> * Add module init callback >> * Move class creation to module init >> * Update visibility based on group presence >> * Add back parent device >> v5: >> * Use ida instead of idr >> * Use device_unregister instead of device_destroy() >> * MKDEV (0, 0) >> --- >> drivers/acpi/platform_profile.c | 88 ++++++++++++++++++++++++++++++-- >> include/linux/platform_profile.h | 2 + >> 2 files changed, 85 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/acpi/platform_profile.c >> b/drivers/acpi/platform_profile.c >> index 32affb75e782d..ef6af2c655524 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) >> @@ -105,8 +112,25 @@ static struct attribute >> *platform_profile_attrs[] = { >> NULL >> }; >> >> +static int profile_class_registered(struct device *dev, const void >> *data) >> +{ >> + return 1; >> +} >> + >> +static umode_t profile_class_is_visible(struct kobject *kobj, struct >> attribute *attr, int idx) >> +{ >> + if (!class_find_device(&platform_profile_class, NULL, NULL, >> profile_class_registered)) >> + return 0; >> + if (attr == &dev_attr_platform_profile_choices.attr) >> + return 0444; >> + if (attr == &dev_attr_platform_profile.attr) >> + return 0644; >> + return 0; >> +} >> + >> static const struct attribute_group platform_profile_group = { >> - .attrs = platform_profile_attrs >> + .attrs = platform_profile_attrs, >> + .is_visible = profile_class_is_visible, >> }; >> >> void platform_profile_notify(struct platform_profile_handler *pprof) >> @@ -123,6 +147,9 @@ int platform_profile_cycle(void) >> enum platform_profile_option next; >> int err; >> >> + if (!class_is_registered(&platform_profile_class)) >> + return -ENODEV; > > This check is pointless since the platform profile class will always > be registered during module initialization. > Please remove it. > >> + >> scoped_cond_guard(mutex_intr, return -ERESTARTSYS, >> &profile_lock) { >> if (!cur_profile) >> return -ENODEV; >> @@ -164,25 +191,76 @@ 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; >> + /* create class interface for individual handler */ >> + pprof->minor = ida_alloc(&platform_profile_ida, GFP_KERNEL); >> + if (pprof->minor < 0) >> + return pprof->minor; >> + pprof->class_dev = device_create(&platform_profile_class, >> pprof->dev, >> + MKDEV(0, 0), pprof, "platform-profile-%d", >> + pprof->minor); >> + if (IS_ERR(pprof->class_dev)) { >> + err = PTR_ERR(pprof->class_dev); >> + goto cleanup_ida; >> + } >> >> cur_profile = pprof; >> + >> + err = sysfs_update_group(acpi_kobj, &platform_profile_group); >> + if (err) >> + goto cleanup_cur; >> + >> return 0; >> + >> +cleanup_cur: >> + cur_profile = NULL; >> + device_unregister(pprof->class_dev); >> + >> +cleanup_ida: >> + ida_free(&platform_profile_ida, pprof->minor); >> + >> + 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; >> + >> + sysfs_update_group(acpi_kobj, &platform_profile_group); >> + >> return 0; >> } >> EXPORT_SYMBOL_GPL(platform_profile_remove); >> >> +static int __init platform_profile_init(void) >> +{ >> + int err; >> + >> + err = class_register(&platform_profile_class); >> + if (err) >> + return err; >> + >> + err = sysfs_create_group(acpi_kobj, &platform_profile_group); >> + if (err) >> + class_unregister(&platform_profile_class); >> + >> + return err; >> +} > > Please use a blank line after function/struct/union/enum declarations. > > Apart from those minor issues the patch looks quite nice, so with > those issues being fixed: > > Reviewed-by: Armin Wolf <W_Armin@gmx.de> > >> +static void __exit platform_profile_exit(void) >> +{ >> + class_unregister(&platform_profile_class); >> + sysfs_remove_group(acpi_kobj, &platform_profile_group); I just noticed that the legacy sysfs group needs to be removed before the class, otherwise there might be a short time window during module exit where the legacy sysfs interface might try to use the already unregistered class. Thanks, Armin Wolf >> +} >> +module_init(platform_profile_init); >> +module_exit(platform_profile_exit); >> + >> MODULE_AUTHOR("Mark Pearson <markpearson@lenovo.com>"); >> MODULE_DESCRIPTION("ACPI platform profile sysfs interface"); >> MODULE_LICENSE("GPL"); >> diff --git a/include/linux/platform_profile.h >> b/include/linux/platform_profile.h >> index 8ec0b8da56db5..a888fd085c513 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); >
diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c index 32affb75e782d..ef6af2c655524 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) @@ -105,8 +112,25 @@ static struct attribute *platform_profile_attrs[] = { NULL }; +static int profile_class_registered(struct device *dev, const void *data) +{ + return 1; +} + +static umode_t profile_class_is_visible(struct kobject *kobj, struct attribute *attr, int idx) +{ + if (!class_find_device(&platform_profile_class, NULL, NULL, profile_class_registered)) + return 0; + if (attr == &dev_attr_platform_profile_choices.attr) + return 0444; + if (attr == &dev_attr_platform_profile.attr) + return 0644; + return 0; +} + static const struct attribute_group platform_profile_group = { - .attrs = platform_profile_attrs + .attrs = platform_profile_attrs, + .is_visible = profile_class_is_visible, }; void platform_profile_notify(struct platform_profile_handler *pprof) @@ -123,6 +147,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; @@ -164,25 +191,76 @@ 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; + /* create class interface for individual handler */ + pprof->minor = ida_alloc(&platform_profile_ida, GFP_KERNEL); + if (pprof->minor < 0) + return pprof->minor; + pprof->class_dev = device_create(&platform_profile_class, pprof->dev, + MKDEV(0, 0), pprof, "platform-profile-%d", + pprof->minor); + if (IS_ERR(pprof->class_dev)) { + err = PTR_ERR(pprof->class_dev); + goto cleanup_ida; + } cur_profile = pprof; + + err = sysfs_update_group(acpi_kobj, &platform_profile_group); + if (err) + goto cleanup_cur; + return 0; + +cleanup_cur: + cur_profile = NULL; + device_unregister(pprof->class_dev); + +cleanup_ida: + ida_free(&platform_profile_ida, pprof->minor); + + 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; + + sysfs_update_group(acpi_kobj, &platform_profile_group); + return 0; } EXPORT_SYMBOL_GPL(platform_profile_remove); +static int __init platform_profile_init(void) +{ + int err; + + err = class_register(&platform_profile_class); + if (err) + return err; + + err = sysfs_create_group(acpi_kobj, &platform_profile_group); + if (err) + class_unregister(&platform_profile_class); + + return err; +} +static void __exit platform_profile_exit(void) +{ + class_unregister(&platform_profile_class); + sysfs_remove_group(acpi_kobj, &platform_profile_group); +} +module_init(platform_profile_init); +module_exit(platform_profile_exit); + MODULE_AUTHOR("Mark Pearson <markpearson@lenovo.com>"); MODULE_DESCRIPTION("ACPI platform profile sysfs interface"); MODULE_LICENSE("GPL"); diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h index 8ec0b8da56db5..a888fd085c513 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);