| Message ID | 20250518185111.3560-1-W_Armin@gmx.de |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] ACPI: platform_profile: Add support for non-ACPI platforms | expand |
On Sun, May 18, 2025 at 8:51 PM Armin Wolf <W_Armin@gmx.de> wrote: > > Currently the platform profile subsystem assumes that all supported > (i.e. ACPI-capable) platforms always run with ACPI being enabled. > However some ARM64 notebooks do not support ACPI and are instead > using devicetree for booting. > > Do not register the legacy sysfs interface on such devices as it > depends on the acpi_kobj (/sys/firmware/acpi/) being present. Users > are encouraged to use the new platform-profile class interface > instead. So how does it work in this case? > Reviewed-by: Janne Grunau <j@jannau.net> > Tested-by: Janne Grunau <j@jannau.net> > Signed-off-by: Armin Wolf <W_Armin@gmx.de> > --- > drivers/acpi/platform_profile.c | 68 ++++++++++++++++++++++++++------- > 1 file changed, 55 insertions(+), 13 deletions(-) > > diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c > index ffbfd32f4cf1..c5a5da7d03f1 100644 > --- a/drivers/acpi/platform_profile.c > +++ b/drivers/acpi/platform_profile.c > @@ -190,6 +190,20 @@ static ssize_t profile_show(struct device *dev, > return sysfs_emit(buf, "%s\n", profile_names[profile]); > } > > +/** > + * profile_notify_legacy - Notify the legacy sysfs interface > + * > + * This wrapper takes care of only notifying the legacy sysfs interface > + * if it was registered during module initialization. > + */ > +static void profile_notify_legacy(void) > +{ > + if (!acpi_kobj) > + return; > + > + sysfs_notify(acpi_kobj, NULL, "platform_profile"); > +} > + > /** > * profile_store - Set the profile for a class device > * @dev: The device > @@ -215,7 +229,7 @@ static ssize_t profile_store(struct device *dev, > return ret; > } > > - sysfs_notify(acpi_kobj, NULL, "platform_profile"); > + profile_notify_legacy(); > > return count; > } > @@ -435,7 +449,7 @@ static ssize_t platform_profile_store(struct kobject *kobj, > return ret; > } > > - sysfs_notify(acpi_kobj, NULL, "platform_profile"); > + profile_notify_legacy(); > > return count; > } > @@ -472,6 +486,22 @@ static const struct attribute_group platform_profile_group = { > .is_visible = profile_class_is_visible, > }; > > +/** > + * profile_update_legacy - Update the legacy sysfs interface > + * > + * This wrapper takes care of only updating the legacy sysfs interface > + * if it was registered during module initialization. > + * > + * Return: 0 on success or error code on failure. > + */ > +static int profile_update_legacy(void) > +{ > + if (!acpi_kobj) > + return 0; > + > + return sysfs_update_group(acpi_kobj, &platform_profile_group); > +} > + > /** > * platform_profile_notify - Notify class device and legacy sysfs interface > * @dev: The class device > @@ -481,7 +511,7 @@ void platform_profile_notify(struct device *dev) > scoped_cond_guard(mutex_intr, return, &profile_lock) { > _notify_class_profile(dev, NULL); > } > - sysfs_notify(acpi_kobj, NULL, "platform_profile"); > + profile_notify_legacy(); > } > EXPORT_SYMBOL_GPL(platform_profile_notify); > > @@ -529,7 +559,7 @@ int platform_profile_cycle(void) > return err; > } > > - sysfs_notify(acpi_kobj, NULL, "platform_profile"); > + profile_notify_legacy(); > > return 0; > } > @@ -603,9 +633,9 @@ struct device *platform_profile_register(struct device *dev, const char *name, > goto cleanup_ida; > } > > - sysfs_notify(acpi_kobj, NULL, "platform_profile"); > + profile_notify_legacy(); > > - err = sysfs_update_group(acpi_kobj, &platform_profile_group); > + err = profile_update_legacy(); > if (err) > goto cleanup_cur; > > @@ -639,8 +669,8 @@ void platform_profile_remove(struct device *dev) > ida_free(&platform_profile_ida, pprof->minor); > device_unregister(&pprof->dev); > > - sysfs_notify(acpi_kobj, NULL, "platform_profile"); > - sysfs_update_group(acpi_kobj, &platform_profile_group); > + profile_notify_legacy(); > + profile_update_legacy(); > } > EXPORT_SYMBOL_GPL(platform_profile_remove); > > @@ -692,16 +722,28 @@ static int __init platform_profile_init(void) > if (err) > return err; > > - err = sysfs_create_group(acpi_kobj, &platform_profile_group); > - if (err) > - class_unregister(&platform_profile_class); > + /* > + * The ACPI kobject can be missing if ACPI was disabled during booting. > + * We thus skip the initialization of the legacy sysfs interface in such > + * cases to allow the platform profile class to work on ARM64 notebooks > + * without ACPI support. > + */ > + if (acpi_kobj) { > + err = sysfs_create_group(acpi_kobj, &platform_profile_group); > + if (err < 0) { > + class_unregister(&platform_profile_class); > + return err; > + } > + } > > - return err; > + return 0; > } > > static void __exit platform_profile_exit(void) > { > - sysfs_remove_group(acpi_kobj, &platform_profile_group); > + if (acpi_kobj) > + sysfs_remove_group(acpi_kobj, &platform_profile_group); > + > class_unregister(&platform_profile_class); > } > module_init(platform_profile_init); > -- > 2.39.5 > >
Am 21.05.25 um 22:17 schrieb Rafael J. Wysocki: > On Sun, May 18, 2025 at 8:51 PM Armin Wolf <W_Armin@gmx.de> wrote: >> Currently the platform profile subsystem assumes that all supported >> (i.e. ACPI-capable) platforms always run with ACPI being enabled. >> However some ARM64 notebooks do not support ACPI and are instead >> using devicetree for booting. >> >> Do not register the legacy sysfs interface on such devices as it >> depends on the acpi_kobj (/sys/firmware/acpi/) being present. Users >> are encouraged to use the new platform-profile class interface >> instead. > So how does it work in this case? > The platform profile subsystem also exposes a more modern class-based sysfs interface, see Documentation/ABI/testing/sysfs-class-platform-profile for details. This interface does not depend on /sys/firmware/acpi being present, so userspace programs can still control the platform profiles using the class-based interface. This will become very important once we have platform profile drivers not depending on some sort of ACPI interface. I suspect that sooner or later some drivers for the embedded controllers on ARM64 notebooks (devicetree!) will register with the platform profile subsystem. Apart from that this allows input drivers using platform_profile_cycle() to work on non-ACPI platforms (like ARm64 devices using devicetree). Thanks, Armin Wolf >> Reviewed-by: Janne Grunau <j@jannau.net> >> Tested-by: Janne Grunau <j@jannau.net> >> Signed-off-by: Armin Wolf <W_Armin@gmx.de> >> --- >> drivers/acpi/platform_profile.c | 68 ++++++++++++++++++++++++++------- >> 1 file changed, 55 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c >> index ffbfd32f4cf1..c5a5da7d03f1 100644 >> --- a/drivers/acpi/platform_profile.c >> +++ b/drivers/acpi/platform_profile.c >> @@ -190,6 +190,20 @@ static ssize_t profile_show(struct device *dev, >> return sysfs_emit(buf, "%s\n", profile_names[profile]); >> } >> >> +/** >> + * profile_notify_legacy - Notify the legacy sysfs interface >> + * >> + * This wrapper takes care of only notifying the legacy sysfs interface >> + * if it was registered during module initialization. >> + */ >> +static void profile_notify_legacy(void) >> +{ >> + if (!acpi_kobj) >> + return; >> + >> + sysfs_notify(acpi_kobj, NULL, "platform_profile"); >> +} >> + >> /** >> * profile_store - Set the profile for a class device >> * @dev: The device >> @@ -215,7 +229,7 @@ static ssize_t profile_store(struct device *dev, >> return ret; >> } >> >> - sysfs_notify(acpi_kobj, NULL, "platform_profile"); >> + profile_notify_legacy(); >> >> return count; >> } >> @@ -435,7 +449,7 @@ static ssize_t platform_profile_store(struct kobject *kobj, >> return ret; >> } >> >> - sysfs_notify(acpi_kobj, NULL, "platform_profile"); >> + profile_notify_legacy(); >> >> return count; >> } >> @@ -472,6 +486,22 @@ static const struct attribute_group platform_profile_group = { >> .is_visible = profile_class_is_visible, >> }; >> >> +/** >> + * profile_update_legacy - Update the legacy sysfs interface >> + * >> + * This wrapper takes care of only updating the legacy sysfs interface >> + * if it was registered during module initialization. >> + * >> + * Return: 0 on success or error code on failure. >> + */ >> +static int profile_update_legacy(void) >> +{ >> + if (!acpi_kobj) >> + return 0; >> + >> + return sysfs_update_group(acpi_kobj, &platform_profile_group); >> +} >> + >> /** >> * platform_profile_notify - Notify class device and legacy sysfs interface >> * @dev: The class device >> @@ -481,7 +511,7 @@ void platform_profile_notify(struct device *dev) >> scoped_cond_guard(mutex_intr, return, &profile_lock) { >> _notify_class_profile(dev, NULL); >> } >> - sysfs_notify(acpi_kobj, NULL, "platform_profile"); >> + profile_notify_legacy(); >> } >> EXPORT_SYMBOL_GPL(platform_profile_notify); >> >> @@ -529,7 +559,7 @@ int platform_profile_cycle(void) >> return err; >> } >> >> - sysfs_notify(acpi_kobj, NULL, "platform_profile"); >> + profile_notify_legacy(); >> >> return 0; >> } >> @@ -603,9 +633,9 @@ struct device *platform_profile_register(struct device *dev, const char *name, >> goto cleanup_ida; >> } >> >> - sysfs_notify(acpi_kobj, NULL, "platform_profile"); >> + profile_notify_legacy(); >> >> - err = sysfs_update_group(acpi_kobj, &platform_profile_group); >> + err = profile_update_legacy(); >> if (err) >> goto cleanup_cur; >> >> @@ -639,8 +669,8 @@ void platform_profile_remove(struct device *dev) >> ida_free(&platform_profile_ida, pprof->minor); >> device_unregister(&pprof->dev); >> >> - sysfs_notify(acpi_kobj, NULL, "platform_profile"); >> - sysfs_update_group(acpi_kobj, &platform_profile_group); >> + profile_notify_legacy(); >> + profile_update_legacy(); >> } >> EXPORT_SYMBOL_GPL(platform_profile_remove); >> >> @@ -692,16 +722,28 @@ static int __init platform_profile_init(void) >> if (err) >> return err; >> >> - err = sysfs_create_group(acpi_kobj, &platform_profile_group); >> - if (err) >> - class_unregister(&platform_profile_class); >> + /* >> + * The ACPI kobject can be missing if ACPI was disabled during booting. >> + * We thus skip the initialization of the legacy sysfs interface in such >> + * cases to allow the platform profile class to work on ARM64 notebooks >> + * without ACPI support. >> + */ >> + if (acpi_kobj) { >> + err = sysfs_create_group(acpi_kobj, &platform_profile_group); >> + if (err < 0) { >> + class_unregister(&platform_profile_class); >> + return err; >> + } >> + } >> >> - return err; >> + return 0; >> } >> >> static void __exit platform_profile_exit(void) >> { >> - sysfs_remove_group(acpi_kobj, &platform_profile_group); >> + if (acpi_kobj) >> + sysfs_remove_group(acpi_kobj, &platform_profile_group); >> + >> class_unregister(&platform_profile_class); >> } >> module_init(platform_profile_init); >> -- >> 2.39.5 >> >>
On Thu, May 22, 2025 at 6:34 AM Armin Wolf <W_Armin@gmx.de> wrote: > > Am 21.05.25 um 22:17 schrieb Rafael J. Wysocki: > > > On Sun, May 18, 2025 at 8:51 PM Armin Wolf <W_Armin@gmx.de> wrote: > >> Currently the platform profile subsystem assumes that all supported > >> (i.e. ACPI-capable) platforms always run with ACPI being enabled. > >> However some ARM64 notebooks do not support ACPI and are instead > >> using devicetree for booting. > >> > >> Do not register the legacy sysfs interface on such devices as it > >> depends on the acpi_kobj (/sys/firmware/acpi/) being present. Users > >> are encouraged to use the new platform-profile class interface > >> instead. > > So how does it work in this case? > > > The platform profile subsystem also exposes a more modern class-based sysfs interface, > see Documentation/ABI/testing/sysfs-class-platform-profile for details. > > This interface does not depend on /sys/firmware/acpi being present, so userspace > programs can still control the platform profiles using the class-based interface. > > This will become very important once we have platform profile drivers not depending on > some sort of ACPI interface. I suspect that sooner or later some drivers for the embedded > controllers on ARM64 notebooks (devicetree!) will register with the platform profile subsystem. > > Apart from that this allows input drivers using platform_profile_cycle() to work on non-ACPI > platforms (like ARm64 devices using devicetree). This driver though is located in drivers/acpi/ and depends on CONFIG_ACPI. Moreover, the platform profile provider drivers need to select ACPI_PLATFORM_PROFILE so it gets built. This means that there are no non-ACPI platform profile providers currently in the tree. While the observation that the code in the driver, other than the legacy sysfs interface, doesn't really depend on ACPI is valid, if you want it to be used on systems without ACPI, it needs to be properly converted to a generic driver. For now, it is better to simply make it fail to initialize without ACPI, so I'm going to apply this patch: https://patchwork.kernel.org/project/linux-acpi/patch/20250522141410.31315-1-alexghiti@rivosinc.com/ Thanks!
On Fri, May 23, 2025 at 04:39:59PM +0200, Rafael J. Wysocki wrote: > On Thu, May 22, 2025 at 6:34 AM Armin Wolf <W_Armin@gmx.de> wrote: > > > > Am 21.05.25 um 22:17 schrieb Rafael J. Wysocki: > > > > > On Sun, May 18, 2025 at 8:51 PM Armin Wolf <W_Armin@gmx.de> wrote: > > >> Currently the platform profile subsystem assumes that all supported > > >> (i.e. ACPI-capable) platforms always run with ACPI being enabled. > > >> However some ARM64 notebooks do not support ACPI and are instead > > >> using devicetree for booting. > > >> > > >> Do not register the legacy sysfs interface on such devices as it > > >> depends on the acpi_kobj (/sys/firmware/acpi/) being present. Users > > >> are encouraged to use the new platform-profile class interface > > >> instead. > > > So how does it work in this case? > > > > > The platform profile subsystem also exposes a more modern class-based sysfs interface, > > see Documentation/ABI/testing/sysfs-class-platform-profile for details. > > > > This interface does not depend on /sys/firmware/acpi being present, so userspace > > programs can still control the platform profiles using the class-based interface. > > > > This will become very important once we have platform profile drivers not depending on > > some sort of ACPI interface. I suspect that sooner or later some drivers for the embedded > > controllers on ARM64 notebooks (devicetree!) will register with the platform profile subsystem. > > > > Apart from that this allows input drivers using platform_profile_cycle() to work on non-ACPI > > platforms (like ARm64 devices using devicetree). > > This driver though is located in drivers/acpi/ and depends on > CONFIG_ACPI. Moreover, the platform profile provider drivers need to > select ACPI_PLATFORM_PROFILE so it gets built. This means that there > are no non-ACPI platform profile providers currently in the tree. > > While the observation that the code in the driver, other than the > legacy sysfs interface, doesn't really depend on ACPI is valid, if you > want it to be used on systems without ACPI, it needs to be properly > converted to a generic driver. > > For now, it is better to simply make it fail to initialize without > ACPI, so I'm going to apply this patch: > > https://patchwork.kernel.org/project/linux-acpi/patch/20250522141410.31315-1-alexghiti@rivosinc.com/ That unfortunately does not help with the hid-lenovo regression. Commit 84c9d2a968c8 ("HID: lenovo: Support for ThinkPad-X12-TAB-1/2 Kbd Fn keys") added a platform_profile_cycle() call and thus a dependency on platform_profile. hid-lenovo is used for USB and Bluetooth devices and not just for Lenovo laptop/tablet specific devices. Above patch just avoids the warning splat but still prevents loading hid-lenovo when ACPI is enabled in the kernel (like Distro kernels) on a non-ACPI system. This breaks devices like "Lenovo ThinkPad Compact Keyboard with TrackPoint" on such systems. I will send a patch to remove platform_profile_cycle() call as short term regression fix and tell the original author to to resubmit once the platform_profile dependency on non-ACPI systems is worked out. Janne
diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c index ffbfd32f4cf1..c5a5da7d03f1 100644 --- a/drivers/acpi/platform_profile.c +++ b/drivers/acpi/platform_profile.c @@ -190,6 +190,20 @@ static ssize_t profile_show(struct device *dev, return sysfs_emit(buf, "%s\n", profile_names[profile]); } +/** + * profile_notify_legacy - Notify the legacy sysfs interface + * + * This wrapper takes care of only notifying the legacy sysfs interface + * if it was registered during module initialization. + */ +static void profile_notify_legacy(void) +{ + if (!acpi_kobj) + return; + + sysfs_notify(acpi_kobj, NULL, "platform_profile"); +} + /** * profile_store - Set the profile for a class device * @dev: The device @@ -215,7 +229,7 @@ static ssize_t profile_store(struct device *dev, return ret; } - sysfs_notify(acpi_kobj, NULL, "platform_profile"); + profile_notify_legacy(); return count; } @@ -435,7 +449,7 @@ static ssize_t platform_profile_store(struct kobject *kobj, return ret; } - sysfs_notify(acpi_kobj, NULL, "platform_profile"); + profile_notify_legacy(); return count; } @@ -472,6 +486,22 @@ static const struct attribute_group platform_profile_group = { .is_visible = profile_class_is_visible, }; +/** + * profile_update_legacy - Update the legacy sysfs interface + * + * This wrapper takes care of only updating the legacy sysfs interface + * if it was registered during module initialization. + * + * Return: 0 on success or error code on failure. + */ +static int profile_update_legacy(void) +{ + if (!acpi_kobj) + return 0; + + return sysfs_update_group(acpi_kobj, &platform_profile_group); +} + /** * platform_profile_notify - Notify class device and legacy sysfs interface * @dev: The class device @@ -481,7 +511,7 @@ void platform_profile_notify(struct device *dev) scoped_cond_guard(mutex_intr, return, &profile_lock) { _notify_class_profile(dev, NULL); } - sysfs_notify(acpi_kobj, NULL, "platform_profile"); + profile_notify_legacy(); } EXPORT_SYMBOL_GPL(platform_profile_notify); @@ -529,7 +559,7 @@ int platform_profile_cycle(void) return err; } - sysfs_notify(acpi_kobj, NULL, "platform_profile"); + profile_notify_legacy(); return 0; } @@ -603,9 +633,9 @@ struct device *platform_profile_register(struct device *dev, const char *name, goto cleanup_ida; } - sysfs_notify(acpi_kobj, NULL, "platform_profile"); + profile_notify_legacy(); - err = sysfs_update_group(acpi_kobj, &platform_profile_group); + err = profile_update_legacy(); if (err) goto cleanup_cur; @@ -639,8 +669,8 @@ void platform_profile_remove(struct device *dev) ida_free(&platform_profile_ida, pprof->minor); device_unregister(&pprof->dev); - sysfs_notify(acpi_kobj, NULL, "platform_profile"); - sysfs_update_group(acpi_kobj, &platform_profile_group); + profile_notify_legacy(); + profile_update_legacy(); } EXPORT_SYMBOL_GPL(platform_profile_remove); @@ -692,16 +722,28 @@ static int __init platform_profile_init(void) if (err) return err; - err = sysfs_create_group(acpi_kobj, &platform_profile_group); - if (err) - class_unregister(&platform_profile_class); + /* + * The ACPI kobject can be missing if ACPI was disabled during booting. + * We thus skip the initialization of the legacy sysfs interface in such + * cases to allow the platform profile class to work on ARM64 notebooks + * without ACPI support. + */ + if (acpi_kobj) { + err = sysfs_create_group(acpi_kobj, &platform_profile_group); + if (err < 0) { + class_unregister(&platform_profile_class); + return err; + } + } - return err; + return 0; } static void __exit platform_profile_exit(void) { - sysfs_remove_group(acpi_kobj, &platform_profile_group); + if (acpi_kobj) + sysfs_remove_group(acpi_kobj, &platform_profile_group); + class_unregister(&platform_profile_class); } module_init(platform_profile_init);