On Tue, Jan 23, 2018 at 03:24:15PM +0200, Luciano Coelho wrote: > On Tue, 2018-01-23 at 12:55 +0100, cyp@xxxxxxxxxxx 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
