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

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

[Hill Ma]

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.

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

Signed-off-by: Hill Ma <maahiuzeon@gmail.com>
---
 Documentation/admin-guide/kernel-parameters.txt |  6 ++++++
 drivers/macintosh/Kconfig                       | 10 ----------
 drivers/macintosh/via-pmu-led.c                 | 11 ++++++++---
 3 files changed, 14 insertions(+), 13 deletions(-)

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.
-
 config PMAC_SMU
 	bool "Support for SMU  based PowerMacs"
 	depends on PPC_PMAC64
diff --git a/drivers/macintosh/via-pmu-led.c b/drivers/macintosh/via-pmu-led.c
index ae067ab2373d..faf39a5962aa 100644
--- a/drivers/macintosh/via-pmu-led.c
+++ b/drivers/macintosh/via-pmu-led.c
@@ -25,6 +25,7 @@
 #include <linux/leds.h>
 #include <linux/adb.h>
 #include <linux/pmu.h>
+#include <linux/moduleparam.h>
 #include <asm/prom.h>
 
 static spinlock_t pmu_blink_lock;
@@ -71,11 +72,10 @@ static void pmu_led_set(struct led_classdev *led_cdev,
  	spin_unlock_irqrestore(&pmu_blink_lock, flags);
 }
 
+bool adb_pmu_led_disk;
+
 static struct led_classdev pmu_led = {
 	.name = "pmu-led::front",
-#ifdef CONFIG_ADB_PMU_LED_DISK
-	.default_trigger = "disk-activity",
-#endif
 	.brightness_set = pmu_led_set,
 };
 
@@ -106,6 +106,9 @@ static int __init via_pmu_led_init(void)
 	}
 	of_node_put(dt);
 
+	if (adb_pmu_led_disk)
+		pmu_led.default_trigger = "disk-activity";
+
 	spin_lock_init(&pmu_blink_lock);
 	/* no outstanding req */
 	pmu_blink_req.complete = 1;
@@ -114,4 +117,6 @@ static int __init via_pmu_led_init(void)
 	return led_classdev_register(NULL, &pmu_led);
 }
 
+core_param(adb_pmu_led_disk, adb_pmu_led_disk, bool, 0644);
+
 late_initcall(via_pmu_led_init);
-- 
2.33.1

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

[Nathan Lynch]

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?

> 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.

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

[Hill Ma]

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.

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

[Michael Ellerman]

Hill Ma <maahiuzeon@gmail.com> writes:
> 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

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