Thanks for the review. On Tue, Oct 26, 2021 at 6:08 AM Nathan Lynch <nathanl@xxxxxxxxxxxxx> wrote: > > Hello, > > Hill Ma <maahiuzeon@xxxxxxxxx> writes: > > 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. > > 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. > > > 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.
