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@xxxxxxxxx> Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx> --- 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@xxxxxx>+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@xxxxxxxxxx>"); 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);
