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
Hi Johannes,
> > > 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
Hi Larry,
> > 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) {
> > + 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.
>
> 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.
I am under the impression that all these limitations are going away. Did
you talk to Kay or Greg about this?
Regards
Marcel
On Thu, 2009-04-02 at 13:42 -0500, Larry Finger wrote:
> Johannes Berg wrote:
> >
> > 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
On Thu, 2009-04-02 at 13:18 -0500, Larry Finger wrote:
> Johannes Berg wrote:
> > 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
Johannes Berg wrote:
> On Thu, 2009-04-02 at 16:24 -0500, Larry Finger wrote:
>
>>> 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.
>
>> 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!
>
>> 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.
>
>> 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]>
On Thu, 2009-04-02 at 15:36 -0500, Larry Finger wrote:
> > 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.
>
> Have I told you that I hate things that appear to be intermittent?
I share that feeling.
> 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.
Fun.
> 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(-)
--- wireless-testing.orig/net/rfkill/core.c 2009-04-02 22:47:05.000000000 +0200
+++ wireless-testing/net/rfkill/core.c 2009-04-02 22:49:53.000000000 +0200
@@ -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);
On Thu, 2009-04-02 at 20:28 +0200, Johannes Berg wrote:
> On Thu, 2009-04-02 at 13:18 -0500, Larry Finger wrote:
>
> > 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?
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
--
Richard Purdie
Intel Open Source Technology Centre
On Fri, Apr 3, 2009 at 06:37, Marcel Holtmann <[email protected]> wro=
te:
>> > 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
>> >
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 list_for_each_entry(trig, &trigger_lis=
t, next_trig) {
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 printk(KERN_DEBUG "available =
trigger: %s\n", trig->name);
>> > =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,
>> > =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))
>> > =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);
>> > =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);
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
>> >
>> > could you do the modification, with a kernel that has the TRIG_NAM=
E_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 bracke=
ts, 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 reboot=
ed. This
>> time I got
>>
>> ~/wireless-testing> cat /sys/class/leds/b43legacy-phy0\:\:rad/trigge=
r
>> none ide-disk ADP1-online BAT0-charging-or-full BAT0-charging BAT0-f=
ull 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 inva=
sive, 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?
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
Hi Marcel,
> > > 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
On Thu, 2009-04-02 at 09:44 -0500, Larry Finger wrote:
> > 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.
> 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
Marcel Holtmann wrote:
> Hi Larry,
>
>>> 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) {
>>> + 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.
>> 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.
>
> 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
On Thu, 2009-04-02 at 20:48 +0200, Johannes Berg wrote:
> On Thu, 2009-04-02 at 13:42 -0500, Larry Finger wrote:
> > Johannes Berg wrote:
> > >
> > > 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.
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
Johannes Berg wrote:
>
> 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
On Thu, 2009-04-02 at 16:24 -0500, Larry Finger wrote:
> > 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.
> 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!
> 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.
> 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
Johannes Berg wrote:
> Larry,
>=20
>> 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 "b43legac=
y-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 activit=
y, but only
>> a single call for LED index 1 when the LED's are still being registe=
red. 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 eve=
nt. Could
>> 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)=
);
> 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=
io
> 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
Larry,
> 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
Johannes Berg wrote:
> On Thu, 2009-04-02 at 09:44 -0500, Larry Finger wrote:
>=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", t=
hus the file
>> 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=
es
> in led core code.
>=20
>> with contents "none ide-disk
>> ADP1-online BAT0-charging-or-full BAT0-charging BAT0-full phy0rx phy=
0tx
>> phy0assoc phy0radio [(]". I unloaded and reloaded the driver and fou=
nd that the
>> "[(]" characters changed to "[=EF=BF=BDW[=EF=BF=BD]". It appears tha=
t something is putting
>> 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
On Thu, 2009-04-02 at 13:18 -0500, Larry Finger wrote:
> 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?
> 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
On Fri, 2009-04-03 at 16:15 +0100, Richard Purdie wrote:
> > 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
On Thu, 2009-04-02 at 10:22 -0500, Larry Finger wrote:
> 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
On Thu, 2009-04-02 at 16:59 -0500, Larry Finger wrote:
> > 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
Johannes Berg wrote:
> 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 wrote:
> 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) {
> + 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.
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 wrote:
> 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. :)
> net/rfkill/core.c | 22 +++++++++++++++++++---
> 1 file changed, 19 insertions(+), 3 deletions(-)
>
> --- wireless-testing.orig/net/rfkill/core.c 2009-04-02 22:47:05.000000000 +0200
> +++ wireless-testing/net/rfkill/core.c 2009-04-02 22:49:53.000000000 +0200
> @@ -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);
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
On Thu, 2009-04-02 at 10:22 -0500, Larry Finger wrote:
> 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