Re: iwlegacy: Please change led_mode default to _LED_RF_STATE

[Stanislaw Gruszka <sgruszka@redhat.com>]

On Tue, Jan 23, 2018 at 03:24:15PM +0200, Luciano Coelho wrote:
> On Tue, 2018-01-23 at 12:55 +0100, cyp@abwesend.de wrote:
> > Good morning, :-)
> > 
> > In iwlegacy/common.c, when module_param(led_mode) is not set by the
> > user (i.e. it is 0=IL_LED_DEFAULT), then il_leds_init() uses the
> > device's cfg->led_mode as the default. That inheritance is ok for
> > devices that have cfg->led_mode = IL_LED_RF_STATE. But there are also
> > .cfgs in which cfg->led_mode = IL_LED_BLINK. Then the inheritance is
> > not so good. :-)
> > 
> > A blinking wlan led is a human factor problem when the wlan led lies
> > within a user's field of vision, for instance on the keyboard or  on
> > the display bevel. In those cases, the blinking is literally in-your-
> > face, and therefore a distraction. Or annoyance. Or even drives
> > people insane if they happen to have an HP device with a bright blue
> > led on the wlan "media" key. :-)
> > 
> > In dozens (hundreds?) of posts dating back to at least 2008 and found
> > all over the 'net, users have been seeking workarounds for a blinking
> > wlan led. (search for: linux blinking wifi|wlan led)
> > * One of those workarounds is of course to define led_mode=1 via
> > /etc/modprobe.d/<whatever>.  But many of those posts are for older
> > versions of the driver, and the solutions no longer work because the
> > name of the driver has changed since then. (https://askubuntu.com/que
> > stions/12069/how-to-stop-constantly-blinking-wifi-led has a list)

So you need to update your modprobe config to reflect correct name.

> > * Another suggested workaround is to echo phyXradio >
> > /sys/class/led/<whatever>/trigger and then stick that in a script in
> > /etc/NetworkManager/dispatcher.d. Well, the led interface names have
> > changed too (e.g. 'iwl-phyX:{assoc|radio|RX|TX}' is now 'phyX-led',
> > so many of those suggestions no longer work either. Of course, it
> > also breaks if phyX becomes phyY when the driver is reloaded.
> > * Another workaround is to paste a piece of opaque tape over the led.
> > I was recently a visitor at a high school where the "administrator"
> > had done that for the laptops there. But kids will be kids, and most
> > of the machines had "lost" the tape.  :-)
> > 
> > My point is, these "workarounds" are not solutions. They would also
> > be unnecessary if the driver used a sane default to begin with, just
> > as the newer iwlwifi devices have. You know the code and the design

Some of iwlwifi devices have BLINK and some other have RF_STATE
as default. I don't know why is that, but I assume there is reason
for it. 

> > choices better than anyone else, but perhaps cfg->led_mode is just
> > code cruft that is long obsolete. But perhaps the following change to
> > iwlegacy/common.c would also be ok?:
> > - /* default: IL_LED_BLINK(0) using blinking idx table */
> > + /* module_param(led_mode) is evaluated in il_leds_init() below */
> >   static int led_mode;
> >   module_param(led_mode, int, S_IRUGO);
> >   MODULE_PARM_DESC(led_mode,
> > -                 "0=system default, " "1=On(RF On)/Off(RF Off),
> > 2=blinking");
> > +               "0=system default, 1=show RF on/off state, 2=blink on
> > TX/RX");
> > + /* previously (< Jan 2018) "system default" meant "inherit from
> > device .cfg."
> > + * Now, "system default" means "driver default" which is '1' for
> > user sanity 
> > + * and for consistency with newer intel wifi devices.
> > + */
> > 
> >   void
> >   il_leds_init(struct il_priv *il)
> >   {
> >         int mode = led_mode;
> >         int ret;
> > 
> > -      if (mode == IL_LED_DEFAULT) 
> > -             mode = il->cfg->led_mode;
> > +      if (mode != IL_LED_BLINK) /* if user does not explicitly ask
> > for blink ... */
> > +             mode = IL_LED_RF_STATE; /* use stable (i.e. RF on/off)
> > state */
> > 
> > 
> > A non-blinking default would be great.

I'm not enthusiastic for this change. We have this setting for ages
and I do not see the point of changing it right now for few people
who still use iwlegacy.

Thanks
Stanislaw