Iwlwifi and LEDS?

Iwlwifi and LEDS?

[Gregy]

Hello, I noticed you removed support for led subsystem from iwlwifi. I
have used it to control wifi led from userspace. Is it still possible
somehow?

Otherwise thank you for your work.

Gregy

Re: Iwlwifi and LEDS?

[Abhijeet Kolekar]

On Fri, 2010-05-21 at 02:11 -0700, Gregy wrote:
> Hello, I noticed you removed support for led subsystem from iwlwifi. I
> have used it to control wifi led from userspace. Is it still possible
> somehow?
> 
There is module parameter for iwlcore called 'led_mode'. 

Abhijeet
> Otherwise thank you for your work.
> 
> Gregy
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Iwlwifi and LEDS?

[Gregy]

On 21 May 2010 20:07, Abhijeet Kolekar <abhijeet.kolekar@intel.com> wrote:
> On Fri, 2010-05-21 at 02:11 -0700, Gregy wrote:
>> Hello, I noticed you removed support for led subsystem from iwlwifi. I
>> have used it to control wifi led from userspace. Is it still possible
>> somehow?
>>
> There is module parameter for iwlcore called 'led_mode'.

Yes but I cannot control the led through it. I can only set one of two
modes. Before I could  use it as "connected to internet" indicator or
I could turn it off completely.

Gregy

Re: Iwlwifi and LEDS?

[reinette chatre]

On Fri, 2010-05-21 at 13:49 -0700, Gregy wrote:
> On 21 May 2010 20:07, Abhijeet Kolekar <abhijeet.kolekar@intel.com> wrote:
> > On Fri, 2010-05-21 at 02:11 -0700, Gregy wrote:
> >> Hello, I noticed you removed support for led subsystem from iwlwifi. I
> >> have used it to control wifi led from userspace. Is it still possible
> >> somehow?
> >>
> > There is module parameter for iwlcore called 'led_mode'.
> 
> Yes but I cannot control the led through it. I can only set one of two
> modes. Before I could  use it as "connected to internet" indicator or
> I could turn it off completely.

No - we did not remove support for LEDs from iwlwifi. The led_mode
module parameter is just to change the blinking behavior (0=blinking,
1=On(RF On)/Off(RF Off)).

What did you do before that does not work anymore?

Reinette

Re: Iwlwifi and LEDS?

[Gregy]

> What did you do before that does not work anymore?
>
> Reinette

No, led never gets registered in /sys/class/leds. Also I found this:
http://article.gmane.org/gmane.linux.kernel.wireless.general/40328/

Re: Iwlwifi and LEDS?

[reinette chatre]

On Thu, 2010-05-27 at 13:14 -0700, Gregy wrote:
> > What did you do before that does not work anymore?
> No, led never gets registered in /sys/class/leds.

Right, and the patch you reference explains why. At no point was support
for LEDs removed.

>  Also I found this:
> http://article.gmane.org/gmane.linux.kernel.wireless.general/40328/

The intention and supported LED behavior has always been for a single
LED to blink according to data. The led_mode module parameter was added
to support the additional RF status indication capability. 

If you want to be able to manipulate the LED in other ways than the
driver supports you could look into adding a debugfs file that
manipulates the driver's led variables (allow_blinking,
last_blink_time, ...). There is already a readable led file in debugfs,
but not writable. 

Reinette

Re: Iwlwifi and LEDS?

[Gregy]

> If you want to be able to manipulate the LED in other ways than the
> driver supports you could look into adding a debugfs file that
> manipulates the driver's led variables (allow_blinking,
> last_blink_time, ...). There is already a readable led file in debugfs,
> but not writable.
>
> Reinette

Ok, I have tried that with partial success. It allows me to change led
status "mid-flight" but if led is blinking it sometimes works only
after second try. Also my understanding of C and kernel programming is
very limited so I probably made some mistakes. Could you please look
at it?

Thank you.

--- iwl-debugfs.c.old	2010-05-14 15:20:16.000000000 +0200
+++ iwl-debugfs.c	2010-05-28 12:37:47.706991952 +0200
@@ -40,6 +40,7 @@
 #include "iwl-core.h"
 #include "iwl-io.h"
 #include "iwl-calib.h"
+#include "iwl-agn-led.c"

 /* create and remove of files */
 #define DEBUGFS_ADD_FILE(name, parent, mode) do {			\
@@ -702,6 +703,34 @@
 	return ret;
 }

+static ssize_t iwl_dbgfs_ledw_write(struct file *file,
+					 const char __user *user_buf,
+					 size_t count, loff_t *ppos)
+{
+	
+	struct iwl_priv *priv = file->private_data;
+	char buf[8];
+	int buf_size;
+	int status;
+
+	memset(buf, 0, sizeof(buf));
+	buf_size = min(count, sizeof(buf) -  1);
+	if (copy_from_user(buf, user_buf, buf_size))
+		return -EFAULT;
+	if (sscanf(buf, "%d", &status) != 1)
+		return -EFAULT;
+	
+	priv->last_blink_time = 0;
+	priv->allow_blinking = 0;  // i set the mode manually -> stop
blinking (doesn't work very well)
+	
+	if(status == 0)
+		iwl_led_off_reg(priv);
+	else
+		iwl_led_on_reg(priv);
+
+	return count;
+}
+
 static ssize_t iwl_dbgfs_thermal_throttling_read(struct file *file,
 				char __user *user_buf,
 				size_t count, loff_t *ppos)
@@ -872,6 +901,7 @@
 DEBUGFS_READ_WRITE_FILE_OPS(interrupt);
 DEBUGFS_READ_FILE_OPS(qos);
 DEBUGFS_READ_FILE_OPS(led);
+DEBUGFS_WRITE_FILE_OPS(ledw);
 DEBUGFS_READ_FILE_OPS(thermal_throttling);
 DEBUGFS_READ_WRITE_FILE_OPS(disable_ht40);
 DEBUGFS_READ_WRITE_FILE_OPS(sleep_level_override);
@@ -2334,6 +2364,7 @@
 	DEBUGFS_ADD_FILE(interrupt, dir_data, S_IWUSR | S_IRUSR);
 	DEBUGFS_ADD_FILE(qos, dir_data, S_IRUSR);
 	DEBUGFS_ADD_FILE(led, dir_data, S_IRUSR);
+	DEBUGFS_ADD_FILE(ledw, dir_data, S_IWUSR);
 	DEBUGFS_ADD_FILE(sleep_level_override, dir_data, S_IWUSR | S_IRUSR);
 	DEBUGFS_ADD_FILE(current_sleep_command, dir_data, S_IRUSR);
 	DEBUGFS_ADD_FILE(thermal_throttling, dir_data, S_IRUSR);

Re: Iwlwifi and LEDS?

[Johannes Berg]

Richard, please see below after the *** -- I have a question for you.

On Fri, 2010-05-28 at 13:01 +0200, Gregy wrote:
> > If you want to be able to manipulate the LED in other ways than the
> > driver supports you could look into adding a debugfs file that
> > manipulates the driver's led variables (allow_blinking,
> > last_blink_time, ...). There is already a readable led file in debugfs,
> > but not writable.
> >
> > Reinette
> 
> Ok, I have tried that with partial success. It allows me to change led
> status "mid-flight" but if led is blinking it sometimes works only
> after second try. Also my understanding of C and kernel programming is
> very limited so I probably made some mistakes. Could you please look
> at it?

I don't think this is the correct approach. I guess since the patch you
referenced is mine, I should comment.

Prior to my patch, iwlwifi would register LEDs in the LED subsystem, but
it didn't really properly do that since a lot of internal code just
updates the LEDs. Additionally, by default the requirement is that the
LED blinks according to traffic. Also, the blinking itself is done by
the device, it is not done by the host CPU.

At the time of the patch, the LED subsystem didn't support blinking.
This has changed now, I think, but previously it was not possible to set
the LED to "blinking" like we need to have it. Additionally the LED
trigger registration code that I removed was way more code for much less
functionality than it should have been.

There's one proper way to fix this, but it is somewhat involved. Yes, I
could have implemented that, but under the given time constraints, I
opted for the cleanup that simply removed functionality that wasn't
fully functional.

(*** mark for Richard)

The proper way to do this now would be to rewrite the LED code in
iwlwifi to:

1) register an LED again, which implements the blink_set() callback and
programs the hw accordingly. This is essentially implementing
iwl_led_pattern(), but with on_time/off_time parameters.

2) Convert the current caller of iwl_led_pattern() to be an LED trigger
that registers with the LED system. It would call the blink_set() for
the LED it is connected to. Ideally, there should be some common
software emulation if the LED has no blink_set(). Richard, is there a
"make LED blink" callback that would start a timer for that LED? The
timer trigger uses it but doesn't seem to be usable itself for other
triggers?

3) start the LED on device start, stop it on device stop

4) depending on the module's led_mode, connect the LED to the
appropriate default trigger, e.g. mac80211's assoc trigger or normally
of course the new iwlwifi-blink trigger.

johannes

Re: Iwlwifi and LEDS?

[Richard Purdie]

On Fri, 2010-05-28 at 13:22 +0200, Johannes Berg wrote:
> Richard, please see below after the *** -- I have a question for you.
> 
> On Fri, 2010-05-28 at 13:01 +0200, Gregy wrote:
> > > If you want to be able to manipulate the LED in other ways than the
> > > driver supports you could look into adding a debugfs file that
> > > manipulates the driver's led variables (allow_blinking,
> > > last_blink_time, ...). There is already a readable led file in debugfs,
> > > but not writable.
> > >
> > > Reinette
> > 
> > Ok, I have tried that with partial success. It allows me to change led
> > status "mid-flight" but if led is blinking it sometimes works only
> > after second try. Also my understanding of C and kernel programming is
> > very limited so I probably made some mistakes. Could you please look
> > at it?
> 
> I don't think this is the correct approach. I guess since the patch you
> referenced is mine, I should comment.
> 
> Prior to my patch, iwlwifi would register LEDs in the LED subsystem, but
> it didn't really properly do that since a lot of internal code just
> updates the LEDs. Additionally, by default the requirement is that the
> LED blinks according to traffic. Also, the blinking itself is done by
> the device, it is not done by the host CPU.
> 
> At the time of the patch, the LED subsystem didn't support blinking.
> This has changed now, I think, but previously it was not possible to set
> the LED to "blinking" like we need to have it. Additionally the LED
> trigger registration code that I removed was way more code for much less
> functionality than it should have been.
> 
> There's one proper way to fix this, but it is somewhat involved. Yes, I
> could have implemented that, but under the given time constraints, I
> opted for the cleanup that simply removed functionality that wasn't
> fully functional.
> 
> (*** mark for Richard)
> 
> The proper way to do this now would be to rewrite the LED code in
> iwlwifi to:
> 
> 1) register an LED again, which implements the blink_set() callback and
> programs the hw accordingly. This is essentially implementing
> iwl_led_pattern(), but with on_time/off_time parameters.
> 
> 2) Convert the current caller of iwl_led_pattern() to be an LED trigger
> that registers with the LED system. It would call the blink_set() for
> the LED it is connected to. Ideally, there should be some common
> software emulation if the LED has no blink_set(). Richard, is there a
> "make LED blink" callback that would start a timer for that LED? The
> timer trigger uses it but doesn't seem to be usable itself for other
> triggers?
> 
> 3) start the LED on device start, stop it on device stop
> 
> 4) depending on the module's led_mode, connect the LED to the
> appropriate default trigger, e.g. mac80211's assoc trigger or normally
> of course the new iwlwifi-blink trigger.

I have to admit I don't fully understand what the capabilities of your
hardware are.

Certainly registering a trigger with the LED system, maybe only
appearing for this specific hardware sounds like the way to go. Since
this will only appear as a trigger for this hardware, the trigger can
then poke whatever it needs into the hardware to work as a traffic
indication?

Cheers,

Richard

Re: Iwlwifi and LEDS?

[Johannes Berg]

On Fri, 2010-05-28 at 15:58 +0100, Richard Purdie wrote:

> > The proper way to do this now would be to rewrite the LED code in
> > iwlwifi to:
> > 
> > 1) register an LED again, which implements the blink_set() callback and
> > programs the hw accordingly. This is essentially implementing
> > iwl_led_pattern(), but with on_time/off_time parameters.
> > 
> > 2) Convert the current caller of iwl_led_pattern() to be an LED trigger
> > that registers with the LED system. It would call the blink_set() for
> > the LED it is connected to. Ideally, there should be some common
> > software emulation if the LED has no blink_set(). Richard, is there a
> > "make LED blink" callback that would start a timer for that LED? The
> > timer trigger uses it but doesn't seem to be usable itself for other
> > triggers?
> > 
> > 3) start the LED on device start, stop it on device stop
> > 
> > 4) depending on the module's led_mode, connect the LED to the
> > appropriate default trigger, e.g. mac80211's assoc trigger or normally
> > of course the new iwlwifi-blink trigger.
> 
> I have to admit I don't fully understand what the capabilities of your
> hardware are.

Sorry, I didn't mean to confuse you with the hardware details. The
hardware can blink, essentially.

> Certainly registering a trigger with the LED system, maybe only
> appearing for this specific hardware sounds like the way to go. Since
> this will only appear as a trigger for this hardware, the trigger can
> then poke whatever it needs into the hardware to work as a traffic
> indication?

Yes, that would be one way. However, that would seem to unduly restrict
the trigger's use. It can be used with any LED that has a blink_set()
callback since it doesn't need to poke the hardware directly.

What I was wondering is if there is (or should be) a common "blink
emulation" for LEDs that don't have a blink_set, so instead of calling
	led->blink_set(...)
I would call
	led_blink_set(...)
and that would start the timer etc.

johannes