Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20211026033254.1052-1-maahiuzeon@gmail.com> <87fssox7ah.fsf@linux.ibm.com>
In-Reply-To: <87fssox7ah.fsf@linux.ibm.com>
From:   Hill Ma <maahiuzeon@gmail.com>
Date:   Tue, 26 Oct 2021 07:50:12 -0700
Message-ID: <CABpQrUMcCKbgSTnTB4BeUUVwq5jkOw7pGbUC53SGe-4DEVnUag@mail.gmail.com>
Subject: Re: [PATCH v2] macintosh/via-pmu-led: make disk activity usage a parameter.
To:     Nathan Lynch <nathanl@linux.ibm.com>
Cc:     linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org,
        benh@kernel.crashing.org, linuxppc-dev@lists.ozlabs.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Thanks for the review.

On Tue, Oct 26, 2021 at 6:08 AM Nathan Lynch <nathanl@linux.ibm.com> wrote:
>
> Hello,
>
> Hill Ma <maahiuzeon@gmail.com> 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.