Re: [PATCH v2] macintosh/via-pmu-led: make disk activity usage a parameter.

Thanks for the review.

On Tue, Oct 26, 2021 at 6:08 AM Nathan Lynch [off-list ref] wrote:

Hello,

Hill Ma [off-list ref] writes:

quoted

Whether to use the LED as a disk activity is a user preference.
Some like this usage while others find the LED too bright. So it
might be a good idea to make this choice a runtime parameter rather
than compile-time config.

Users already have the ability to change the LED behavior at runtime
already, correct? I.e. they can do:

  echo none > /sys/class/leds/pmu-led::front/trigger

in their boot scripts. Granted, a kernel built with ADB_PMU_LED_DISK=y
will blink the LED on disk activity until user space is running. Is this
unsatisfactory?

Yes, indeed. As someone who does not like this behavior on iBooks.

quoted

The default is set to disabled as OS X does not use the LED as a
disk activity indicator.

This is long-standing behavior in Linux and OS X has been EOL on this
architecture for a decade, so this isn't much of a consideration at this
point. Seems more important to avoid surprising existing users and
distributions with a behavior change that makes additional work for
them. See below.

quoted

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 43dc35fe5bc0..a656a51ba0a8 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt

@@ -250,6 +250,12 @@
                      Use timer override. For some broken Nvidia NF5 boards
                      that require a timer override, but don't have HPET

+     adb_pmu_led_disk [PPC]
+                     Use front LED as disk LED by default. Only applies to
+                     PowerBook, iBook, PowerMac 7,2/7,3.
+                     Format: <bool>  (1/Y/y=enable, 0/N/n=disable)
+                     Default: disabled
+
      add_efi_memmap  [EFI; X86] Include EFI memory map in
                      kernel's map of available physical RAM.

diff --git a/drivers/macintosh/Kconfig b/drivers/macintosh/Kconfig
index 5cdc361da37c..243215de563c 100644
--- a/drivers/macintosh/Kconfig
+++ b/drivers/macintosh/Kconfig

@@ -78,16 +78,6 @@ config ADB_PMU_LED
        behaviour of the old CONFIG_BLK_DEV_IDE_PMAC_BLINK, select this
        and the disk LED trigger and configure appropriately through sysfs.

-config ADB_PMU_LED_DISK
-     bool "Use front LED as DISK LED by default"
-     depends on ADB_PMU_LED
-     depends on LEDS_CLASS
-     select LEDS_TRIGGERS
-     select LEDS_TRIGGER_DISK
-     help
-       This option makes the front LED default to the disk trigger
-       so that it blinks on disk activity.
-

So, if I've been relying on CONFIG_ADB_PMU_LED_DISK=y and I upgrade to a
newer kernel with the proposed change, from my point of view the disk
activity LED has stopped working and I need to alter the bootloader
config or init scripts to restore the expected behavior. That seems
undesirable to me.

I don't think we rigidly enforce Kconfig backward compatibility, but
when it comes to a user-visible function on a legacy platform where
users and distros likely have their configurations figured out already,
it's probably best to avoid such changes.

I actually asked some distributions that still ship PowerPC BE
architectures to unset it.
https://github.com/void-ppc/void-packages/pull/48
https://github.com/void-linux/void-packages/pull/33275
https://git.adelielinux.org/adelie/packages/-/merge_requests/607

And Debian, which still has PowerPC BE architectures as ports, does
not turn it on.
https://salsa.debian.org/kernel-team/linux/-/blob/master/debian/config/kernelarch-powerpc/config

The problem I see is the following:
- A distribution might accidentally turn it back on, which happened
with Void already.
- For people like the disk activity behavior, they need to recompile
the kernel to regain the exact previous behavior.

I think we could retain backward compatibility by adding back the
Kconfig and setting the initial value of adb_pmu_led_disk based on the
config. I am not sure if we need two mechanisms for this single
preference though.