| Message ID | 20230825180928.205499-1-deller@kernel.org (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | [1/3] parisc: led: Reduce CPU overhead for disk & lan LED computation | expand |
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
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
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
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
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++;