[PATCH 1/3] parisc: led: Reduce CPU overhead for disk & lan LED computation

[PATCH 1/3] parisc: led: Reduce CPU overhead for disk & lan LED computation

[deller]

From: Helge Deller <deller@gmx.de>

Older PA-RISC machines have LEDs which show the disk- and LAN-activity.
The computation is done in software and takes quite some time, e.g. on a
J6500 this may take up to 60% time of one CPU if the machine is loaded
via network traffic.

Since most people don't care about the LEDs, start with LEDs disabled and
just show a CPU heartbeat LED. The disk and LAN LEDs can be turned on
manually via /proc/pdc/led.

Signed-off-by: Helge Deller <deller@gmx.de>
Cc: <stable@vger.kernel.org>
---
 drivers/parisc/led.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/parisc/led.c b/drivers/parisc/led.c
index 8bdc5e043831..765f19608f60 100644
--- a/drivers/parisc/led.c
+++ b/drivers/parisc/led.c
@@ -56,8 +56,8 @@
 static int led_type __read_mostly = -1;
 static unsigned char lastleds;	/* LED state from most recent update */
 static unsigned int led_heartbeat __read_mostly = 1;
-static unsigned int led_diskio    __read_mostly = 1;
-static unsigned int led_lanrxtx   __read_mostly = 1;
+static unsigned int led_diskio    __read_mostly;
+static unsigned int led_lanrxtx   __read_mostly;
 static char lcd_text[32]          __read_mostly;
 static char lcd_text_default[32]  __read_mostly;
 static int  lcd_no_led_support    __read_mostly = 0; /* KittyHawk doesn't support LED on its LCD */
@@ -589,6 +589,9 @@ int __init register_led_driver(int model, unsigned long cmd_reg, unsigned long d
 		return 1;
 	}
 	
+	pr_info("LED: Enable disk and LAN activity LEDs "
+		"via /proc/pdc/led\n");
+
 	/* mark the LCD/LED driver now as initialized and 
 	 * register to the reboot notifier chain */
 	initialized++;
-- 
2.41.0

Re: [PATCH 1/3] parisc: led: Reduce CPU overhead for disk & lan LED computation

[Greg KH]

On Fri, Aug 25, 2023 at 08:09:26PM +0200, deller@kernel.org wrote:
> From: Helge Deller <deller@gmx.de>
> 
> Older PA-RISC machines have LEDs which show the disk- and LAN-activity.
> The computation is done in software and takes quite some time, e.g. on a
> J6500 this may take up to 60% time of one CPU if the machine is loaded
> via network traffic.
> 
> Since most people don't care about the LEDs, start with LEDs disabled and
> just show a CPU heartbeat LED. The disk and LAN LEDs can be turned on
> manually via /proc/pdc/led.
> 
> Signed-off-by: Helge Deller <deller@gmx.de>
> Cc: <stable@vger.kernel.org>
> ---
>  drivers/parisc/led.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/parisc/led.c b/drivers/parisc/led.c
> index 8bdc5e043831..765f19608f60 100644
> --- a/drivers/parisc/led.c
> +++ b/drivers/parisc/led.c
> @@ -56,8 +56,8 @@
>  static int led_type __read_mostly = -1;
>  static unsigned char lastleds;	/* LED state from most recent update */
>  static unsigned int led_heartbeat __read_mostly = 1;
> -static unsigned int led_diskio    __read_mostly = 1;
> -static unsigned int led_lanrxtx   __read_mostly = 1;
> +static unsigned int led_diskio    __read_mostly;
> +static unsigned int led_lanrxtx   __read_mostly;
>  static char lcd_text[32]          __read_mostly;
>  static char lcd_text_default[32]  __read_mostly;
>  static int  lcd_no_led_support    __read_mostly = 0; /* KittyHawk doesn't support LED on its LCD */
> @@ -589,6 +589,9 @@ int __init register_led_driver(int model, unsigned long cmd_reg, unsigned long d
>  		return 1;
>  	}
>  	
> +	pr_info("LED: Enable disk and LAN activity LEDs "
> +		"via /proc/pdc/led\n");

When drivers are working properly, they should be quiet.  Who is going
to see this message?

I don't even understand it, are you saying that you now need to go
enable the led through proc?  And why are leds in proc, I thought they
had a real class for them?  Why not use that instead?

And finally, you shouldn't split strings across lines :)

thanks,

greg k-h

Re: [PATCH 1/3] parisc: led: Reduce CPU overhead for disk & lan LED computation

[Helge Deller]

On 8/26/23 09:34, Greg KH wrote:
> On Fri, Aug 25, 2023 at 08:09:26PM +0200, deller@kernel.org wrote:
>> From: Helge Deller <deller@gmx.de>
>>
>> Older PA-RISC machines have LEDs which show the disk- and LAN-activity.
>> The computation is done in software and takes quite some time, e.g. on a
>> J6500 this may take up to 60% time of one CPU if the machine is loaded
>> via network traffic.
>>
>> Since most people don't care about the LEDs, start with LEDs disabled and
>> just show a CPU heartbeat LED. The disk and LAN LEDs can be turned on
>> manually via /proc/pdc/led.
>>
>> Signed-off-by: Helge Deller <deller@gmx.de>
>> Cc: <stable@vger.kernel.org>
>> ---
>>   drivers/parisc/led.c | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/parisc/led.c b/drivers/parisc/led.c
>> index 8bdc5e043831..765f19608f60 100644
>> --- a/drivers/parisc/led.c
>> +++ b/drivers/parisc/led.c
>> @@ -56,8 +56,8 @@
>>   static int led_type __read_mostly = -1;
>>   static unsigned char lastleds;	/* LED state from most recent update */
>>   static unsigned int led_heartbeat __read_mostly = 1;
>> -static unsigned int led_diskio    __read_mostly = 1;
>> -static unsigned int led_lanrxtx   __read_mostly = 1;
>> +static unsigned int led_diskio    __read_mostly;
>> +static unsigned int led_lanrxtx   __read_mostly;
>>   static char lcd_text[32]          __read_mostly;
>>   static char lcd_text_default[32]  __read_mostly;
>>   static int  lcd_no_led_support    __read_mostly = 0; /* KittyHawk doesn't support LED on its LCD */
>> @@ -589,6 +589,9 @@ int __init register_led_driver(int model, unsigned long cmd_reg, unsigned long d
>>   		return 1;
>>   	}
>>
>> +	pr_info("LED: Enable disk and LAN activity LEDs "
>> +		"via /proc/pdc/led\n");
>
> When drivers are working properly, they should be quiet.  Who is going
> to see this message?

That patch shouldn't have gone to stable@ yet... git-send-patch just
pulled the CC in and I didn't noticed.
So, please don't apply it yet.

Anyway, yes, I can drop this info.

> I don't even understand it, are you saying that you now need to go
> enable the led through proc?  And why are leds in proc, I thought they
> had a real class for them?  Why not use that instead?

This is an old driver from the very beginning, and I don't want to change
much in it for old kernels other than to reduce the CPU overhead it generates.

Additionally I've started a rewrite of the driver (in my for-next tree),
and I did look at the led class and led trigger classes. If it fits, I'll
convert to those, but at least for hdd activity it seems only IDE/ATA activity
is monitored and SCSI activity is missing. Anyway, this is still work-in-progress...

> And finally, you shouldn't split strings across lines :)

Ok.

Thanks!
Helge

Re: [PATCH 1/3] parisc: led: Reduce CPU overhead for disk & lan LED computation

[Greg KH]

On Sat, Aug 26, 2023 at 09:54:55AM +0200, Helge Deller wrote:
> On 8/26/23 09:34, Greg KH wrote:
> > On Fri, Aug 25, 2023 at 08:09:26PM +0200, deller@kernel.org wrote:
> > > From: Helge Deller <deller@gmx.de>
> > > 
> > > Older PA-RISC machines have LEDs which show the disk- and LAN-activity.
> > > The computation is done in software and takes quite some time, e.g. on a
> > > J6500 this may take up to 60% time of one CPU if the machine is loaded
> > > via network traffic.
> > > 
> > > Since most people don't care about the LEDs, start with LEDs disabled and
> > > just show a CPU heartbeat LED. The disk and LAN LEDs can be turned on
> > > manually via /proc/pdc/led.
> > > 
> > > Signed-off-by: Helge Deller <deller@gmx.de>
> > > Cc: <stable@vger.kernel.org>
> > > ---
> > >   drivers/parisc/led.c | 7 +++++--
> > >   1 file changed, 5 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/parisc/led.c b/drivers/parisc/led.c
> > > index 8bdc5e043831..765f19608f60 100644
> > > --- a/drivers/parisc/led.c
> > > +++ b/drivers/parisc/led.c
> > > @@ -56,8 +56,8 @@
> > >   static int led_type __read_mostly = -1;
> > >   static unsigned char lastleds;	/* LED state from most recent update */
> > >   static unsigned int led_heartbeat __read_mostly = 1;
> > > -static unsigned int led_diskio    __read_mostly = 1;
> > > -static unsigned int led_lanrxtx   __read_mostly = 1;
> > > +static unsigned int led_diskio    __read_mostly;
> > > +static unsigned int led_lanrxtx   __read_mostly;
> > >   static char lcd_text[32]          __read_mostly;
> > >   static char lcd_text_default[32]  __read_mostly;
> > >   static int  lcd_no_led_support    __read_mostly = 0; /* KittyHawk doesn't support LED on its LCD */
> > > @@ -589,6 +589,9 @@ int __init register_led_driver(int model, unsigned long cmd_reg, unsigned long d
> > >   		return 1;
> > >   	}
> > > 
> > > +	pr_info("LED: Enable disk and LAN activity LEDs "
> > > +		"via /proc/pdc/led\n");
> > 
> > When drivers are working properly, they should be quiet.  Who is going
> > to see this message?
> 
> That patch shouldn't have gone to stable@ yet... git-send-patch just
> pulled the CC in and I didn't noticed.
> So, please don't apply it yet.

I will not, and it's fine to cc: stable@ on stuff that is still making
it's way into the kernel tree.  It gives us a heads-up on stuff, AND
sometimes it gives you an additional free review :)

> > I don't even understand it, are you saying that you now need to go
> > enable the led through proc?  And why are leds in proc, I thought they
> > had a real class for them?  Why not use that instead?
> 
> This is an old driver from the very beginning, and I don't want to change
> much in it for old kernels other than to reduce the CPU overhead it generates.

Ah, makes sense, that is a crazy amount of cpu time for a blinking led.

How about just default to it off (like the first chunk you have here),
which will go to stable trees, and then a rewrite to use the proper LED
api?  Or not, your call, I doubt many people actually have this hardware
to care about it...

thanks,

greg k-h

Re: [PATCH 1/3] parisc: led: Reduce CPU overhead for disk & lan LED computation

[Helge Deller]

On 8/26/23 09:58, Greg KH wrote:
> On Sat, Aug 26, 2023 at 09:54:55AM +0200, Helge Deller wrote:
>> On 8/26/23 09:34, Greg KH wrote:
>>> On Fri, Aug 25, 2023 at 08:09:26PM +0200, deller@kernel.org wrote:
>>>> From: Helge Deller <deller@gmx.de>
>>>>
>>>> Older PA-RISC machines have LEDs which show the disk- and LAN-activity.
>>>> The computation is done in software and takes quite some time, e.g. on a
>>>> J6500 this may take up to 60% time of one CPU if the machine is loaded
>>>> via network traffic.
>>>>
>>>> Since most people don't care about the LEDs, start with LEDs disabled and
>>>> just show a CPU heartbeat LED. The disk and LAN LEDs can be turned on
>>>> manually via /proc/pdc/led.
>>>>
>>>> Signed-off-by: Helge Deller <deller@gmx.de>
>>>> Cc: <stable@vger.kernel.org>
>>>> ---
>>>>    drivers/parisc/led.c | 7 +++++--
>>>>    1 file changed, 5 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/parisc/led.c b/drivers/parisc/led.c
>>>> index 8bdc5e043831..765f19608f60 100644
>>>> --- a/drivers/parisc/led.c
>>>> +++ b/drivers/parisc/led.c
>>>> @@ -56,8 +56,8 @@
>>>>    static int led_type __read_mostly = -1;
>>>>    static unsigned char lastleds;	/* LED state from most recent update */
>>>>    static unsigned int led_heartbeat __read_mostly = 1;
>>>> -static unsigned int led_diskio    __read_mostly = 1;
>>>> -static unsigned int led_lanrxtx   __read_mostly = 1;
>>>> +static unsigned int led_diskio    __read_mostly;
>>>> +static unsigned int led_lanrxtx   __read_mostly;
>>>>    static char lcd_text[32]          __read_mostly;
>>>>    static char lcd_text_default[32]  __read_mostly;
>>>>    static int  lcd_no_led_support    __read_mostly = 0; /* KittyHawk doesn't support LED on its LCD */
>>>> @@ -589,6 +589,9 @@ int __init register_led_driver(int model, unsigned long cmd_reg, unsigned long d
>>>>    		return 1;
>>>>    	}
>>>>
>>>> +	pr_info("LED: Enable disk and LAN activity LEDs "
>>>> +		"via /proc/pdc/led\n");
>>>
>>> When drivers are working properly, they should be quiet.  Who is going
>>> to see this message?
>>
>> That patch shouldn't have gone to stable@ yet... git-send-patch just
>> pulled the CC in and I didn't noticed.
>> So, please don't apply it yet.
>
> I will not, and it's fine to cc: stable@ on stuff that is still making
> it's way into the kernel tree.  It gives us a heads-up on stuff, AND
> sometimes it gives you an additional free review :)

Yes, thanks!  :-)

>>> I don't even understand it, are you saying that you now need to go
>>> enable the led through proc?  And why are leds in proc, I thought they
>>> had a real class for them?  Why not use that instead?
>>
>> This is an old driver from the very beginning, and I don't want to change
>> much in it for old kernels other than to reduce the CPU overhead it generates.
>
> Ah, makes sense, that is a crazy amount of cpu time for a blinking led.
>
> How about just default to it off (like the first chunk you have here),
> which will go to stable trees,

Yes, that was the idea of this patch. I'll drop the pr_info() next time.

> and then a rewrite to use the proper LED api?

Yes.

Helge