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

[Michael Ellerman]

Hill Ma <maahiuzeon@xxxxxxxxx> writes:
> 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

Looks like all three distros have it disabled.

So let's drop the config option, make it disabled by default, and anyone
who wants to turn it on can do so in their initramfs or init scripts.

cheers