Re: iwlegacy: Please change led_mode default to _LED_RF_STATE

Re: iwlegacy: Please change led_mode default to _LED_RF_STATE

[Luciano Coelho]

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

Emmanuel and I are not the maintainers of iwlegacy, so I'm adding the
linux-wireless mailing list and Stanislaw, who is the actual
maintainer.

--
Cheers,
Luca.

Re: iwlegacy: Please change led_mode default to _LED_RF_STATE

[Stanislaw Gruszka]

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
 

Re: iwlegacy: Please change led_mode default to _LED_RF_STATE

[Johannes Berg]

On Tue, 2018-01-23 at 15:07 +0100, Stanislaw Gruszka wrote:
> 
> 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. 

Not really. As I remember, the reason was basically marketing having
sold the devices with different defaults :-)

I for one have no objection to changing the default, but OTOH the OP
could just set a module configuration file and be done with it.

johannes

Fwd: iwlegacy: Please change led_mode default to _LED_RF_STATE

[cyp]

> Emmanuel and I are not the maintainers of iwlegacy, so I'm adding
> the linux-wireless mailing list and Stanislaw, who is the actual
> maintainer.

The issue is the same for iwlwifi:
while most iwiwifi device cfgs have  led_mode = IWL_LED_RF_STATE
iwlwifi/cfg/1000.c and iwlwifi/cfg/5000.c   have led_mode = IWL_LED_BLINK,
which in turn causes the led to be configured to blink *by default*
(up to 20x/sec!),
which in turn is a human factor problem if the led is bright and/or in
the user's face.

The (potential) solution is also the same as for iwlegacy:
In iwlwifi/dvm/led.c : iwl_leds_init() : line 183

- if (mode == IWL_LED_DEFAULT)
-              mode = priv->cfg->led_mode;

+  if (mode != IWL_LED_BLINK) /* if user does not explicitly request
blink ... */
+             mode = IWL_LED_RF_STATE; /* then use a stable indicator
for sanity and consistency */


Sebastian's response that I should then update modprobe options is
missing the point.
The point is: defaults ought to be sane to begin with, and (ideally) consistent.
*I* know what to do. Many (most?) users will not, hence the dozens of
requests for help (since 2008!).

Regards,
Cyrus