How does rfkill work?

DISCUSSION Larry Finger 5 participants 26 messages Apr 2, 2009 – Apr 3, 2009 Rfkill

Share & tools

Larry Finger

author

2009-04-02 05:00 UTC #0 lore raw

Johannes,

I'm having trouble getting the radio LED to work on b43legacy. When the LED's are registered, I get LED index 0 registered with names of "b43legacy-phy0::tx" and "b43legacy-phy0::rx", and LED index 1 with a name as of "b43legacy-phy0::radio". I placed printk's at the entrance to b43legacy_led_brightness_set(), which is the callback routine. I see a number of calls to modify LED index 0, which I assume are due to RX/TX activity, but only a single call for LED index 1 when the LED's are still being registered. There are no such calls generated when the radio switch is moved.

I don't see where/how a particular LED is attached to the rfkill event. Could you point me to some code that does that?

Thanks,

Larry

Johannes Berg

maintainer

2009-04-02 09:57 UTC #1 lore raw

Larry,

Quoted 11 lines

I'm having trouble getting the radio LED to work on b43legacy. When the LED's are registered, I get LED index 0 registered with names of "b43legacy-phy0::tx" and "b43legacy-phy0::rx", and LED index 1 with a name as of "b43legacy-phy0::radio". I placed printk's at the entrance to b43legacy_led_brightness_set(), which is the callback routine. I see a number of calls to modify LED index 0, which I assume are due to RX/TX activity, but only a single call for LED index 1 when the LED's are still being registered. There are no such calls generated when the radio switch is moved.
I don't see where/how a particular LED is attached to the rfkill event. Could you point me to some code that does that?

In theory, that is here:

                snprintf(name, sizeof(name),
                         "b43legacy-%s::radio", wiphy_name(hw->wiphy));
                b43legacy_register_led(dev, &dev->led_radio, name,
                                 b43legacy_rfkill_led_name(dev),
                                 led_index, activelow);

I actually thought this was attached, by default, to the mac80211 radio trigger, not the rfkill radio trigger.

Can you check the contents of

	/sys/class/leds/b43legacy-...::radio/trigger

please?

johannes

Larry Finger

author

2009-04-02 14:45 UTC #2 lore raw

Quoted 3 lines · Johannes Berg

Larry,
=20
Quoted 1 line
I'm having trouble getting the radio LED to work on b43legacy. When =

the LED's

Quoted 1 line

Quoted 1 line
are registered, I get LED index 0 registered with names of "b43legac=

y-phy0::tx"

Quoted 3 lines

Quoted 3 lines
and "b43legacy-phy0::rx", and LED index 1 with a name as of "b43legacy-phy0::radio". I placed printk's at the entrance to b43legacy_led_brightness_set(), which is the callback routine. I see=

a number of

Quoted 1 line

Quoted 1 line
calls to modify LED index 0, which I assume are due to RX/TX activit=

y, but only

Quoted 1 line

Quoted 1 line
a single call for LED index 1 when the LED's are still being registe=

red. There

Quoted 3 lines

Quoted 3 lines
are no such calls generated when the radio switch is moved.
I don't see where/how a particular LED is attached to the rfkill eve=

nt. Could

Quoted 6 lines

Quoted 1 line
you point me to some code that does that?
=20
In theory, that is here:
=20
                snprintf(name, sizeof(name),
                         "b43legacy-%s::radio", wiphy_name(hw->wiphy)=

);

Quoted 5 lines

                b43legacy_register_led(dev, &dev->led_radio, name,
                                 b43legacy_rfkill_led_name(dev),
                                 led_index, activelow);

=20
I actually thought this was attached, by default, to the mac80211 rad=

Quoted 5 lines

trigger, not the rfkill radio trigger.
=20
Can you check the contents of
	/sys/class/leds/b43legacy-...::radio/trigger
please?

The first thing I noticed is that "radio" gets truncated to "rad", thus=
the file
is /sys/class/leds/b43legacy-phy0\:\:rad/trigger, with contents "none i= de-disk
ADP1-online BAT0-charging-or-full BAT0-charging BAT0-full phy0rx phy0tx phy0assoc phy0radio [(]". I unloaded and reloaded the driver and found = that the
"[(]" characters changed to "[=EF=BF=BDW[=EF=BF=BD]". It appears that s= omething is putting
garbage in that file.

Larry

Larry
--
To unsubscribe from this list: send the line "unsubscribe linux-wireles= s" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Johannes Berg

maintainer

2009-04-02 15:02 UTC #3 lore raw

Quoted 6 lines · Larry Finger

Quoted 3 lines
Can you check the contents of
	/sys/class/leds/b43legacy-...::radio/trigger
please?
The first thing I noticed is that "radio" gets truncated to "rad", thus the file is /sys/class/leds/b43legacy-phy0\:\:rad/trigger,

Cute. Must be that the limit is 20 bytes (19+NUL) in the buffer it uses in led core code.

Quoted 5 lines

with contents "none ide-disk
ADP1-online BAT0-charging-or-full BAT0-charging BAT0-full phy0rx phy0tx phy0assoc phy0radio [(]". I unloaded and reloaded the driver and found that the "[(]" characters changed to "[�W[�]". It appears that something is putting garbage in that file.

Ouch. That's bad. I'll try to figure it out. I don't see how my patch changes this -- does this happen without my patch too?

johannes

Larry Finger

author

2009-04-02 15:23 UTC #4 lore raw

Quoted 6 lines · Johannes Berg

On Thu, 2009-04-02 at 09:44 -0500, Larry Finger wrote:
=20
Quoted 4 lines
Quoted 3 lines
Can you check the contents of
	/sys/class/leds/b43legacy-...::radio/trigger
please?
The first thing I noticed is that "radio" gets truncated to "rad", t=

hus the file

Quoted 3 lines

Quoted 1 line
is /sys/class/leds/b43legacy-phy0\:\:rad/trigger,
=20
Cute. Must be that the limit is 20 bytes (19+NUL) in the buffer it us=

Quoted 4 lines

in led core code.
=20
Quoted 2 lines
with contents "none ide-disk
ADP1-online BAT0-charging-or-full BAT0-charging BAT0-full phy0rx phy=

0tx

Quoted 1 line

Quoted 1 line
phy0assoc phy0radio [(]". I unloaded and reloaded the driver and fou=

nd that the

Quoted 1 line

Quoted 1 line
"[(]" characters changed to "[=EF=BF=BDW[=EF=BF=BD]". It appears tha=

t something is putting

Quoted 4 lines

Quoted 1 line
garbage in that file.
=20
Ouch. That's bad. I'll try to figure it out. I don't see how my patch changes this -- does this happen without my patch too?

Without your "rfkill rewrite" patch, but with "rfkill-remove-unused-cod= e",
"rfkill-remove-deprecated", and "rfkill-remove-user-claim" patches, the=
file
/sys/class/..../trigger contains "none ide-disk BAT0-charging-or-full BAT0-charging BAT0-full ADP1-online phy0rx phy0tx phy0assoc phy0radio [= rfkill0]"

Now I know what should be inside the [].

Larry
--
To unsubscribe from this list: send the line "unsubscribe linux-wireles= s" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Johannes Berg

maintainer

2009-04-02 15:56 UTC #5 lore raw

Quoted 6 lines · Larry Finger

Without your "rfkill rewrite" patch, but with "rfkill-remove-unused-code", "rfkill-remove-deprecated", and "rfkill-remove-user-claim" patches, the file /sys/class/..../trigger contains "none ide-disk BAT0-charging-or-full BAT0-charging BAT0-full ADP1-online phy0rx phy0tx phy0assoc phy0radio [rfkill0]"
Now I know what should be inside the [].

Indeed. But it confuses me that this isn't in there to start with... I haven't found it yet.

johannes

Johannes Berg

maintainer

2009-04-02 16:27 UTC #6 lore raw

In reply to Larry Finger

Quoted 4 lines · Larry Finger

Without your "rfkill rewrite" patch, but with "rfkill-remove-unused-code", "rfkill-remove-deprecated", and "rfkill-remove-user-claim" patches, the file /sys/class/..../trigger contains "none ide-disk BAT0-charging-or-full BAT0-charging BAT0-full ADP1-online phy0rx phy0tx phy0assoc phy0radio [rfkill0]"

On my system that's exactly what it looks like with my patch (well, rfkill4 right now). I'm stumped. What kernel are you running?

johannes

Larry Finger

author

2009-04-02 18:19 UTC #7 lore raw

Quoted 2 lines · Johannes Berg

On my system that's exactly what it looks like with my patch (well, rfkill4 right now). I'm stumped. What kernel are you running?

The problem is the statement #define TRIG_NAME_MAX 50 in include/linux/leds.h. When I make it 60, then the trigger file contains "none ide-disk ADP1-online BAT0-charging-or-full BAT0-charging BAT0-full phy0rx phy0tx phy0assoc phy0radio [rfkill0]" as expected.

I still have not found where radio gets truncated to rad, but I'm looking.

I'm running the master-2009-03-27 pull from wireless-testing with some added patches.

Larry

Johannes Berg

maintainer

2009-04-02 18:29 UTC #8 lore raw

Quoted 4 lines · Larry Finger

The problem is the statement #define TRIG_NAME_MAX 50 in include/linux/leds.h. When I make it 60, then the trigger file contains "none ide-disk ADP1-online BAT0-charging-or-full BAT0-charging BAT0-full phy0rx phy0tx phy0assoc phy0radio [rfkill0]" as expected.

Woah, that's strange. Richard? Larry is seeing problems where, oddly depending on a patch I have, he's getting garbage for the selected trigger when reading the trigger sysfs file.

The trigger's name, should be
led_trigger.name = <something that is NULL> ? : dev_name(&some_dev)

Does this make any sense to you?

Quoted 1 line

I still have not found where radio gets truncated to rad, but I'm looking.

Probably in sysfs or something, don't think it is important.

johannes

Richard Purdie

2009-04-03 15:36 UTC #9 lore raw

Quoted 7 lines · Johannes Berg

Quoted 4 lines · Larry Finger
The problem is the statement #define TRIG_NAME_MAX 50 in include/linux/leds.h. When I make it 60, then the trigger file contains "none ide-disk ADP1-online BAT0-charging-or-full BAT0-charging BAT0-full phy0rx phy0tx phy0assoc phy0radio [rfkill0]" as expected.

Quoted 8 lines

Woah, that's strange. Richard? Larry is seeing problems where, oddly depending on a patch I have, he's getting garbage for the selected trigger when reading the trigger sysfs file.
The trigger's name, should be
led_trigger.name = <something that is NULL> ? : dev_name(&some_dev)
Does this make any sense to you?

No, it doesn't make any sense and the 'fix' he posted isn't valid, just a workaround that happens to help. I suspect there is memory corruption going on somewhere...

Cheers,

Richard

Signature · 2 lines

Richard Purdie
Intel Open Source Technology Centre

Johannes Berg

maintainer

2009-04-03 19:23 UTC #10 lore raw

Quoted 8 lines · Richard Purdie

Quoted 4 lines
The trigger's name, should be
led_trigger.name = <something that is NULL> ? : dev_name(&some_dev)
Does this make any sense to you?
No, it doesn't make any sense and the 'fix' he posted isn't valid, just a workaround that happens to help. I suspect there is memory corruption going on somewhere...

That's what I suspected, thanks.

johannes

Johannes Berg

maintainer

2009-04-02 18:30 UTC #11 lore raw

In reply to Larry Finger

Quoted 8 lines · Larry Finger

Quoted 2 lines · Johannes Berg
On my system that's exactly what it looks like with my patch (well, rfkill4 right now). I'm stumped. What kernel are you running?
The problem is the statement #define TRIG_NAME_MAX 50 in include/linux/leds.h. When I make it 60, then the trigger file contains "none ide-disk ADP1-online BAT0-charging-or-full BAT0-charging BAT0-full phy0rx phy0tx phy0assoc phy0radio [rfkill0]" as expected.

Also, if increasing that helps, does it also work then? I mean, does the LED get turned on/off?

johannes

Larry Finger

author

2009-04-02 18:42 UTC #12 lore raw

Quoted 3 lines · Johannes Berg

Also, if increasing that helps, does it also work then? I mean, does the LED get turned on/off?

No. I'm still not getting the appropriate call to the set brightness callback. Actually, I didn't expect any change as the truncation from the too-short array length only occurs when the pseudo file is dumped.

Larry

Johannes Berg

maintainer

2009-04-02 18:48 UTC #13 lore raw

Quoted 8 lines · Larry Finger

Quoted 3 lines · Johannes Berg
Also, if increasing that helps, does it also work then? I mean, does the LED get turned on/off?
No. I'm still not getting the appropriate call to the set brightness callback. Actually, I didn't expect any change as the truncation from the too-short array length only occurs when the pseudo file is dumped.

Hmm. In my tree TRIG_NAME_MAX is only used when you try to /write/ something to the trigger sysfs file, so that garbling and the patch don't make much sense to me. There must be some other weirdness going on.

johannes

Johannes Berg

maintainer

2009-04-02 18:58 UTC #14 lore raw

Quoted 6 lines · Johannes Berg

Quoted 5 lines · Larry Finger
Quoted 3 lines · Johannes Berg
Also, if increasing that helps, does it also work then? I mean, does the LED get turned on/off?

Quoted 8 lines

Quoted 3 lines
No. I'm still not getting the appropriate call to the set brightness callback. Actually, I didn't expect any change as the truncation from the too-short array length only occurs when the pseudo file is dumped.
Hmm. In my tree TRIG_NAME_MAX is only used when you try to /write/ something to the trigger sysfs file, so that garbling and the patch don't make much sense to me. There must be some other weirdness going on.

Could you try something for me?

In drivers/leds/led-triggers.c in led_trigger_show you have

        list_for_each_entry(trig, &trigger_list, next_trig) {
+		printk(KERN_DEBUG "available trigger: %s\n", trig->name);
                if (led_cdev->trigger && !strcmp(led_cdev->trigger->name,
                                                        trig->name))
                        len += sprintf(buf+len, "[%s] ", trig->name);
                else
                        len += sprintf(buf+len, "%s ", trig->name);
        }

could you do the modification, with a kernel that has the TRIG_NAME_MAX set to 50, and see what it prints? I'm completely confused by this failure mode since the garbage you get is actually put into brackets, so it seems the strcmp() is returning 0 which seems odd.

johannes

Larry Finger

author

2009-04-02 20:37 UTC #15 lore raw

Quoted 15 lines · Johannes Berg

On Thu, 2009-04-02 at 20:48 +0200, Johannes Berg wrote:

Could you try something for me?

In drivers/leds/led-triggers.c in led_trigger_show you have

        list_for_each_entry(trig, &trigger_list, next_trig) {

(quoted diff) +1 -0

+		printk(KERN_DEBUG "available trigger: %s\n", trig->name);
                if (led_cdev->trigger && !strcmp(led_cdev->trigger->name,
                                                        trig->name))
                        len += sprintf(buf+len, "[%s] ", trig->name);
                else
                        len += sprintf(buf+len, "%s ", trig->name);
        }

Quoted 4 lines

could you do the modification, with a kernel that has the TRIG_NAME_MAX set to 50, and see what it prints? I'm completely confused by this failure mode since the garbage you get is actually put into brackets, so it seems the strcmp() is returning 0 which seems odd.

Have I told you that I hate things that appear to be intermittent?

I added the printk, returned TRIG_NAME_MAX to 50, rebuilt and rebooted. This time I got

~/wireless-testing> cat /sys/class/leds/b43legacy-phy0\:\:rad/trigger none ide-disk ADP1-online BAT0-charging-or-full BAT0-charging BAT0-full phy0rx phy0tx phy0assoc phy0radio [rfkill0]
~/wireless-testing> dmesg | grep available
available trigger: ide-disk
available trigger: ADP1-online
available trigger: BAT0-charging-or-full
available trigger: BAT0-charging
available trigger: BAT0-full
available trigger: phy0rx
available trigger: phy0tx
available trigger: phy0assoc
available trigger: phy0radio
available trigger: rfkill0

I'll leave the printk in place for the moment.

The name is being truncated by "#define BUS_ID_SIZE 20" in include/linux/device.h. As changing that define would be pretty invasive, I plan to use a shorter name when the LED is registered.

The bad news is that the set brightness callback routine is still not being called even though I tried BUS_ID_SIZE of 30..

Till later,

Larry

Johannes Berg

maintainer

2009-04-02 20:50 UTC #16 lore raw

Quoted 11 lines · Larry Finger

Quoted 11 lines

In drivers/leds/led-triggers.c in led_trigger_show you have

        list_for_each_entry(trig, &trigger_list, next_trig) {

(quoted diff) +1 -0

+		printk(KERN_DEBUG "available trigger: %s\n", trig->name);
                if (led_cdev->trigger && !strcmp(led_cdev->trigger->name,
                                                        trig->name))
                        len += sprintf(buf+len, "[%s] ", trig->name);
                else
                        len += sprintf(buf+len, "%s ", trig->name);
        }

Quoted 6 lines

Quoted 4 lines
could you do the modification, with a kernel that has the TRIG_NAME_MAX set to 50, and see what it prints? I'm completely confused by this failure mode since the garbage you get is actually put into brackets, so it seems the strcmp() is returning 0 which seems odd.
Have I told you that I hate things that appear to be intermittent?

I share that feeling.

Quoted 18 lines

I added the printk, returned TRIG_NAME_MAX to 50, rebuilt and rebooted. This time I got
~/wireless-testing> cat /sys/class/leds/b43legacy-phy0\:\:rad/trigger none ide-disk ADP1-online BAT0-charging-or-full BAT0-charging BAT0-full phy0rx phy0tx phy0assoc phy0radio [rfkill0]
~/wireless-testing> dmesg | grep available
available trigger: ide-disk
available trigger: ADP1-online
available trigger: BAT0-charging-or-full
available trigger: BAT0-charging
available trigger: BAT0-full
available trigger: phy0rx
available trigger: phy0tx
available trigger: phy0assoc
available trigger: phy0radio
available trigger: rfkill0

Quoted 1 line

I'll leave the printk in place for the moment.

Fun.

Quoted 6 lines

The name is being truncated by "#define BUS_ID_SIZE 20" in include/linux/device.h. As changing that define would be pretty invasive, I plan to use a shorter name when the LED is registered.
The bad news is that the set brightness callback routine is still not being called even though I tried BUS_ID_SIZE of 30..

Ah, yes. Using a shorter name is probably a good idea then. I don't think changing that define makes too much sense.

On my test with iwlwifi the rfkill LED trigger is definitely called -- I cannot pinpoint why it shouldn't be on your setup. :(

Actually. Are you positive it works without my patch? The confusing this is that this code never seems to call led_trigger_event() outside of rfkill_led_trigger_activate() which is only called once... Can you try this patch please?

johannes

---
 net/rfkill/core.c |   22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

net/rfkill/core.c +19 -3

@@ -83,12 +83,10 @@ static bool rfkill_epo_lock_active;
 
 
 #ifdef CONFIG_RFKILL_LEDS
-static void rfkill_led_trigger_activate(struct led_classdev *led)
+static void rfkill_led_trigger_event(struct rfkill *rfkill)
 {
-	struct rfkill *rfkill;
 	struct led_trigger *trigger;
 
-	rfkill = container_of(led->trigger, struct rfkill, led_trigger);
 	trigger = &rfkill->led_trigger;
 
 	if (rfkill->blocked)

@@ -97,6 +95,15 @@ static void rfkill_led_trigger_activate(
 		led_trigger_event(trigger, LED_FULL);
 }
 
+static void rfkill_led_trigger_activate(struct led_classdev *led)
+{
+	struct rfkill *rfkill;
+
+	rfkill = container_of(led->trigger, struct rfkill, led_trigger);
+
+	rfkill_led_trigger_event(rfkill);
+}
+
 const char *rfkill_get_led_trigger_name(struct rfkill *rfkill)
 {
 	return rfkill->led_trigger.name;

@@ -124,6 +131,10 @@ static void rfkill_led_trigger_unregiste
 	led_trigger_unregister(&rfkill->led_trigger);
 }
 #else
+static void rfkill_led_trigger_event(struct rfkill *rfkill)
+{
+}
+
 static inline int rfkill_led_trigger_register(struct rfkill *rfkill)
 {
 	return 0;

@@ -158,6 +169,8 @@ static bool __rfkill_set_hw_state(struct
 
 	*change = prev != blocked;
 
+	rfkill_led_trigger_event(rfkill);
+
 	return blocked || !!test_bit(RFKILL_BLOCK_SW_BIT, &rfkill->blocked);
 }

@@ -214,6 +227,7 @@ static void rfkill_set_block(struct rfki
 			clear_bit(RFKILL_BLOCK_SW_BIT, &rfkill->blocked);
 	}
 
+	rfkill_led_trigger_event(rfkill);
 	rfkill_uevent(rfkill);
 }

@@ -400,6 +414,8 @@ bool rfkill_set_sw_state(struct rfkill *
 	if (prev != blocked && !hwblock)
 		schedule_work(&rfkill->uevent_work);
 
+	rfkill_led_trigger_event(rfkill);
+
 	return blocked || hwblock;
 }
 EXPORT_SYMBOL(rfkill_set_sw_state);

Larry Finger

author

2009-04-02 21:25 UTC #17 lore raw

Quoted 10 lines · Johannes Berg

Ah, yes. Using a shorter name is probably a good idea then. I don't think changing that define makes too much sense.
On my test with iwlwifi the rfkill LED trigger is definitely called -- I cannot pinpoint why it shouldn't be on your setup. :(
Actually. Are you positive it works without my patch? The confusing this is that this code never seems to call led_trigger_event() outside of rfkill_led_trigger_activate() which is only called once... Can you try this patch please?

No, it hasn't worked for some time, but until you rationalized the rfkill code, I didn't want to mess with it. :)

Quoted 64 lines

 net/rfkill/core.c |   22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

net/rfkill/core.c +17 -3

@@ -83,12 +83,10 @@ static bool rfkill_epo_lock_active;
 
 
 #ifdef CONFIG_RFKILL_LEDS
-static void rfkill_led_trigger_activate(struct led_classdev *led)
+static void rfkill_led_trigger_event(struct rfkill *rfkill)
 {
-	struct rfkill *rfkill;
 	struct led_trigger *trigger;
 
-	rfkill = container_of(led->trigger, struct rfkill, led_trigger);
 	trigger = &rfkill->led_trigger;
 
 	if (rfkill->blocked)

@@ -97,6 +95,15 @@ static void rfkill_led_trigger_activate(
 		led_trigger_event(trigger, LED_FULL);
 }
 
+static void rfkill_led_trigger_activate(struct led_classdev *led)
+{
+	struct rfkill *rfkill;
+
+	rfkill = container_of(led->trigger, struct rfkill, led_trigger);
+
+	rfkill_led_trigger_event(rfkill);
+}
+
 const char *rfkill_get_led_trigger_name(struct rfkill *rfkill)
 {
 	return rfkill->led_trigger.name;

@@ -124,6 +131,10 @@ static void rfkill_led_trigger_unregiste
 	led_trigger_unregister(&rfkill->led_trigger);
 }
 #else
+static void rfkill_led_trigger_event(struct rfkill *rfkill)
+{
+}
+
 static inline int rfkill_led_trigger_register(struct rfkill *rfkill)
 {
 	return 0;

@@ -158,6 +169,8 @@ static bool __rfkill_set_hw_state(struct
 
 	*change = prev != blocked;
 
+	rfkill_led_trigger_event(rfkill);
+
 	return blocked || !!test_bit(RFKILL_BLOCK_SW_BIT, &rfkill->blocked);
 }

@@ -214,6 +227,7 @@ static void rfkill_set_block(struct rfki
 			clear_bit(RFKILL_BLOCK_SW_BIT, &rfkill->blocked);
 	}
 
+	rfkill_led_trigger_event(rfkill);
 	rfkill_uevent(rfkill);
 }

net/rfkill/core.c bool rfkill_set_sw_state(struct rfkill * · line 421

Quoted 8 lines

net/rfkill/core.c +2 -0

@@ -400,6 +414,8 @@ bool rfkill_set_sw_state(struct rfkill *
 	if (prev != blocked && !hwblock)
 		schedule_work(&rfkill->uevent_work);
 
+	rfkill_led_trigger_event(rfkill);
+
 	return blocked || hwblock;
 }
 EXPORT_SYMBOL(rfkill_set_sw_state);

This patch does the trick. Not only is the set brightness callback routine being called, but the LED is going on/off as expected. It even ends up in the off state when the module is loaded with the switch off. It toggles on/off in that case, but I'm not going to complain as long as it ends up off. That part broke first, then everything broke later..

Thanks for the help.

Larry

Johannes Berg

maintainer

2009-04-02 21:40 UTC #18 lore raw

Quoted 7 lines · Larry Finger

Quoted 4 lines
Actually. Are you positive it works without my patch? The confusing this is that this code never seems to call led_trigger_event() outside of rfkill_led_trigger_activate() which is only called once... Can you try this patch please?
No, it hasn't worked for some time, but until you rationalized the rfkill code, I didn't want to mess with it. :)

Ah, ok :) And I thought I broke it.

Quoted 2 lines

This patch does the trick. Not only is the set brightness callback routine being called, but the LED is going on/off as expected.

Wohoo!

Quoted 2 lines

It even ends up in the off
state when the module is loaded with the switch off.

Yeah I'd fixed that earlier by calling the right thing.

Quoted 3 lines

It toggles on/off in that
case, but I'm not going to complain as long as it ends up off. That part broke first, then everything broke later..

Hm, don't see a good way to fix that really. Nor am I sure why it happens, but if it just flashes once doesn't really matter I guess.

I'll roll this into my rework patch.

johannes

Larry Finger

author

2009-04-02 21:59 UTC #19 lore raw

Quoted 21 lines · Johannes Berg

Quoted 6 lines · Larry Finger
Quoted 4 lines
Actually. Are you positive it works without my patch? The confusing this is that this code never seems to call led_trigger_event() outside of rfkill_led_trigger_activate() which is only called once... Can you try this patch please?
No, it hasn't worked for some time, but until you rationalized the rfkill code, I didn't want to mess with it. :)
Ah, ok :) And I thought I broke it.
Quoted 2 lines
This patch does the trick. Not only is the set brightness callback routine being called, but the LED is going on/off as expected.
Wohoo!
Quoted 2 lines
It even ends up in the off
state when the module is loaded with the switch off.
Yeah I'd fixed that earlier by calling the right thing.

Quoted 8 lines

Quoted 3 lines
It toggles on/off in that
case, but I'm not going to complain as long as it ends up off. That part broke first, then everything broke later..
Hm, don't see a good way to fix that really. Nor am I sure why it happens, but if it just flashes once doesn't really matter I guess.
I'll roll this into my rework patch.

The whole thing gets a Tested-by: Larry Finger <[email protected]>

Johannes Berg

maintainer

2009-04-02 22:09 UTC #20 lore raw

Quoted 3 lines · Larry Finger

Quoted 1 line
I'll roll this into my rework patch.
The whole thing gets a Tested-by: Larry Finger <[email protected]>

Added; Thanks for your help and patience testing this!

Just to outline the plan at least once: I'll repost the patch after -rc1 (and after that trickles into wireless-testing), when a bunch more drivers (I know at least some sony thing) will have been added that use it, and I hopefully have toshiba and thinkpad conversions.

johannes

Marcel Holtmann

2009-04-03 04:37 UTC #21 lore raw

In reply to Larry Finger

Hi Larry,

Quoted 40 lines

Quoted 15 lines · Johannes Berg
Could you try something for me?
In drivers/leds/led-triggers.c in led_trigger_show you have
        list_for_each_entry(trig, &trigger_list, next_trig) {
(quoted diff) +1 -0
+		printk(KERN_DEBUG "available trigger: %s\n", trig->name);
                if (led_cdev->trigger && !strcmp(led_cdev->trigger->name,
                                                        trig->name))
                        len += sprintf(buf+len, "[%s] ", trig->name);
                else
                        len += sprintf(buf+len, "%s ", trig->name);
        }
Quoted 4 lines
could you do the modification, with a kernel that has the TRIG_NAME_MAX set to 50, and see what it prints? I'm completely confused by this failure mode since the garbage you get is actually put into brackets, so it seems the strcmp() is returning 0 which seems odd.
Have I told you that I hate things that appear to be intermittent?
I added the printk, returned TRIG_NAME_MAX to 50, rebuilt and rebooted. This time I got
~/wireless-testing> cat /sys/class/leds/b43legacy-phy0\:\:rad/trigger none ide-disk ADP1-online BAT0-charging-or-full BAT0-charging BAT0-full phy0rx phy0tx phy0assoc phy0radio [rfkill0]
~/wireless-testing> dmesg | grep available
available trigger: ide-disk
available trigger: ADP1-online
available trigger: BAT0-charging-or-full
available trigger: BAT0-charging
available trigger: BAT0-full
available trigger: phy0rx
available trigger: phy0tx
available trigger: phy0assoc
available trigger: phy0radio
available trigger: rfkill0

Quoted 5 lines

I'll leave the printk in place for the moment.
The name is being truncated by "#define BUS_ID_SIZE 20" in include/linux/device.h. As changing that define would be pretty invasive, I plan to use a shorter name when the LED is registered.

I am under the impression that all these limitations are going away. Did you talk to Kay or Greg about this?

Regards

Marcel

Larry Finger

author

2009-04-03 04:58 UTC #22 lore raw

Quoted 41 lines · Marcel Holtmann

Hi Larry,
Quoted 39 lines
Quoted 15 lines · Johannes Berg
Could you try something for me?
In drivers/leds/led-triggers.c in led_trigger_show you have
        list_for_each_entry(trig, &trigger_list, next_trig) {
(quoted diff) +1 -0
+		printk(KERN_DEBUG "available trigger: %s\n", trig->name);
                if (led_cdev->trigger && !strcmp(led_cdev->trigger->name,
                                                        trig->name))
                        len += sprintf(buf+len, "[%s] ", trig->name);
                else
                        len += sprintf(buf+len, "%s ", trig->name);
        }
Quoted 4 lines
could you do the modification, with a kernel that has the TRIG_NAME_MAX set to 50, and see what it prints? I'm completely confused by this failure mode since the garbage you get is actually put into brackets, so it seems the strcmp() is returning 0 which seems odd.
Have I told you that I hate things that appear to be intermittent?
I added the printk, returned TRIG_NAME_MAX to 50, rebuilt and rebooted. This time I got
~/wireless-testing> cat /sys/class/leds/b43legacy-phy0\:\:rad/trigger none ide-disk ADP1-online BAT0-charging-or-full BAT0-charging BAT0-full phy0rx phy0tx phy0assoc phy0radio [rfkill0]
~/wireless-testing> dmesg | grep available
available trigger: ide-disk
available trigger: ADP1-online
available trigger: BAT0-charging-or-full
available trigger: BAT0-charging
available trigger: BAT0-full
available trigger: phy0rx
available trigger: phy0tx
available trigger: phy0assoc
available trigger: phy0radio
available trigger: rfkill0

Quoted 8 lines

Quoted 5 lines
I'll leave the printk in place for the moment.
The name is being truncated by "#define BUS_ID_SIZE 20" in include/linux/device.h. As changing that define would be pretty invasive, I plan to use a shorter name when the LED is registered.
I am under the impression that all these limitations are going away. Did you talk to Kay or Greg about this?

No, I have not approached them about this matter. Based on this exchange, I will find out from them.

To answer your previous question, this is a 2.6.29 kernel from wireless-testing.

Thanks,

Larry

Kay Sievers

2009-04-03 05:09 UTC #23 lore raw

In reply to Marcel Holtmann

On Fri, Apr 3, 2009 at 06:37, Marcel Holtmann <[email protected]> wro= te:

Quoted 7 lines

Quoted 7 lines
Quoted 7 lines · Johannes Berg
Could you try something for me?
In drivers/leds/led-triggers.c in led_trigger_show you have
=C2=A0 =C2=A0 =C2=A0 =C2=A0 list_for_each_entry(trig, &trigger_lis=

t, next_trig) {

Quoted 1 line

Quoted 1 line
Quoted 1 line
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 printk(KERN_DEBUG "available =

trigger: %s\n", trig->name);

Quoted 1 line

Quoted 1 line
Quoted 1 line
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (led_cd=

ev->trigger && !strcmp(led_cdev->trigger->name,

Quoted 1 line

Quoted 1 line
Quoted 1 line
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=

=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 trig->name))

Quoted 1 line

Quoted 1 line
Quoted 1 line
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=

=A0 =C2=A0 =C2=A0 len +=3D sprintf(buf+len, "[%s] ", trig->name);

Quoted 2 lines

Quoted 2 lines
Quoted 2 lines
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 else =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=

=A0 =C2=A0 =C2=A0 len +=3D sprintf(buf+len, "%s ", trig->name);

Quoted 3 lines

Quoted 3 lines
Quoted 3 lines
=C2=A0 =C2=A0 =C2=A0 =C2=A0 }
could you do the modification, with a kernel that has the TRIG_NAM=

E_MAX

Quoted 2 lines

Quoted 2 lines
Quoted 2 lines
set to 50, and see what it prints? I'm completely confused by this failure mode since the garbage you get is actually put into bracke=

ts, so

Quoted 5 lines

Quoted 5 lines
Quoted 1 line
it seems the strcmp() is returning 0 which seems odd.
Have I told you that I hate things that appear to be intermittent?
I added the printk, returned TRIG_NAME_MAX to 50, rebuilt and reboot=

ed. This

Quoted 3 lines

Quoted 3 lines
time I got
~/wireless-testing> cat /sys/class/leds/b43legacy-phy0\:\:rad/trigge=

Quoted 1 line

Quoted 1 line
none ide-disk ADP1-online BAT0-charging-or-full BAT0-charging BAT0-f=

ull phy0rx

Quoted 13 lines

Quoted 13 lines
phy0tx phy0assoc phy0radio [rfkill0]
~/wireless-testing> dmesg | grep available
available trigger: ide-disk
available trigger: ADP1-online
available trigger: BAT0-charging-or-full
available trigger: BAT0-charging
available trigger: BAT0-full
available trigger: phy0rx
available trigger: phy0tx
available trigger: phy0assoc
available trigger: phy0radio
available trigger: rfkill0

Quoted 4 lines

Quoted 4 lines
I'll leave the printk in place for the moment.
The name is being truncated by "#define BUS_ID_SIZE 20" in include/linux/device.h. As changing that define would be pretty inva=

sive, I plan

Quoted 3 lines

Quoted 1 line
to use a shorter name when the LED is registered.
I am under the impression that all these limitations are going away. =

Did

Quoted 1 line

you talk to Kay or Greg about this?

The limit is already gone in the current tree:
http://git.kernel.org/?p=3Dlinux/kernel/git/torvalds/linux-2.6.git;a=3D=
commitdiff;h=3D1fa5ae857bb14f6046205171d98506d8112dd74e

Kay
--
To unsubscribe from this list: send the line "unsubscribe linux-wireles= s" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Marcel Holtmann

2009-04-03 04:35 UTC #24 lore raw

In reply to Johannes Berg

Hi Johannes,

Quoted 9 lines

Quoted 6 lines
Quoted 3 lines
Can you check the contents of
	/sys/class/leds/b43legacy-...::radio/trigger
please?
The first thing I noticed is that "radio" gets truncated to "rad", thus the file is /sys/class/leds/b43legacy-phy0\:\:rad/trigger,
Cute. Must be that the limit is 20 bytes (19+NUL) in the buffer it uses in led core code.

then this needs fixing in the LED core. We don't have a length limit in the driver core anymore. Is this an older kernel?

Regards

Marcel

Johannes Berg

maintainer

2009-04-03 09:02 UTC #25 lore raw

Hi Marcel,

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
The first thing I noticed is that "radio" gets truncated to "rad", thus the file is /sys/class/leds/b43legacy-phy0\:\:rad/trigger,
Cute. Must be that the limit is 20 bytes (19+NUL) in the buffer it uses in led core code.
then this needs fixing in the LED core. We don't have a length limit in the driver core anymore. Is this an older kernel?

No, I was mistaken, this is the limit Larry pointed out, and that was already lifted (bus_id).

johannes