led can not be turned off normally when rfkill is blocked, the cause is
the led_turn_off() function exit as not expected:
drivers/net/wireless/rtl818x/rtl8187/leds.c:
static void led_turn_off(struct work_struct *work)
{
[...]
/* Don't change the LED, when the device is down. */
if (!priv->vif || priv->vif->type == NL80211_IFTYPE_UNSPECIFIED)
return ;
[...]
When rfkill is blocked, the function calling order looks like this:
net/mac80211/iface.c:
ieee80211_do_stop
-> drv_remove_interface = rtl8187_remove_interface
-> ieee80211_stop_device
-> led_set_brightness
-> led_turn_off
-> drv_stop = rtl8187_stop
rtl8187_remove_interface() set priv->vif to NULL, so, led_turn_off()
will return and will not be able to turn off the led. delay the setting
of priv->vif to rtl8187_stop() can solve it.
Signed-off-by: Wu Zhangjin <[email protected]>
---
drivers/net/wireless/rtl818x/rtl8187/dev.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/rtl818x/rtl8187/dev.c b/drivers/net/wireless/rtl818x/rtl8187/dev.c
index 2bb5297..1fea0cd 100644
--- a/drivers/net/wireless/rtl818x/rtl8187/dev.c
+++ b/drivers/net/wireless/rtl818x/rtl8187/dev.c
@@ -1007,6 +1007,9 @@ static void rtl8187_stop(struct ieee80211_hw *dev)
u32 reg;
mutex_lock(&priv->conf_mutex);
+
+ priv->vif = NULL;
+
rtl818x_iowrite16(priv, &priv->map->INT_MASK, 0);
reg = rtl818x_ioread8(priv, &priv->map->CMD);
@@ -1067,10 +1070,7 @@ exit:
static void rtl8187_remove_interface(struct ieee80211_hw *dev,
struct ieee80211_vif *vif)
{
- struct rtl8187_priv *priv = dev->priv;
- mutex_lock(&priv->conf_mutex);
- priv->vif = NULL;
- mutex_unlock(&priv->conf_mutex);
+ /* Nothing to do */
}
static int rtl8187_config(struct ieee80211_hw *dev, u32 changed)
--
1.7.1
On Fri, Apr 01, 2011 at 12:22:51AM +0800, wu zhangjin wrote:
> On Mon, Mar 28, 2011 at 11:40 PM, Herton Ronaldo Krzesinski
> <[email protected]> wrote:
> [...]
> >>
> >> My rtl8187 devices are both external USB sticks, thus they have no
> >> interaction with a radio-kill switch. I will test your patch to make
> >> sure it does no harm to my system.
> >>
> >> I think the commit message should be revised. A simple statement
> >> like "the LED does not turn off when the rfkill switch is off"
> >> should be sufficient.
> >
> > The patch should work, but I wonder if we should be fiddling with priv->vif
> > for this, perhaps we should not assume vif to be valid after
> > rtl8187_remove_interface (I don't see problems with current
> > rtl8187/mac80211 code on a quick look, but...)
> >
>
> Yes, but seems only two places have touched the vif pointer.
>
> $ grep "vif =" -ur drivers/net/wireless/rtl818x/rtl8187/
> drivers/net/wireless/rtl818x/rtl8187/dev.c: priv->vif = NULL;
> drivers/net/wireless/rtl818x/rtl8187/dev.c: priv->vif = vif;
>
> > I cleaner solution may be to use a priv->mode like p54.
>
> Yep, then, we may need to add a 'mode' member to rtl8187_priv and
> update it properly like p54.
>
> So, What do you prefer? I will prepare a new patch asap.
In the led code we dereference priv->vif, checking for priv->vif->type.
vif is a pointer from mac80211, and after rtl8187_remove_interface is
called by it, at least from my POV can do anything it wants with vif,
like freeing the sdata vif points to. From what I checked, it doesn't
free currently and your patch don't have issues, but may be it's better
to be safe and add a new "future proof" mode field.
And as your bug shows, the led code access priv->vif after
rtl8187_remove_interface, so we hit the case where vif could be
considered invalid.
So because of this and being cleaner I would go adding 'mode'.
>
> Best Regards,
> Wu Zhangjin
>
> >
> >>
> >> Larry
> >>
> >
> > --
> > []'s
> > Herton
> >
>
--
[]'s
Herton
On Mon, Mar 28, 2011 at 11:40 PM, Herton Ronaldo Krzesinski
<[email protected]> wrote:
[...]
>>
>> My rtl8187 devices are both external USB sticks, thus they have no
>> interaction with a radio-kill switch. I will test your patch to make
>> sure it does no harm to my system.
>>
>> I think the commit message should be revised. A simple statement
>> like "the LED does not turn off when the rfkill switch is off"
>> should be sufficient.
>
> The patch should work, but I wonder if we should be fiddling with priv->vif
> for this, perhaps we should not assume vif to be valid after
> rtl8187_remove_interface (I don't see problems with current
> rtl8187/mac80211 code on a quick look, but...)
>
Yes, but seems only two places have touched the vif pointer.
$ grep "vif =" -ur drivers/net/wireless/rtl818x/rtl8187/
drivers/net/wireless/rtl818x/rtl8187/dev.c: priv->vif = NULL;
drivers/net/wireless/rtl818x/rtl8187/dev.c: priv->vif = vif;
> I cleaner solution may be to use a priv->mode like p54.
Yep, then, we may need to add a 'mode' member to rtl8187_priv and
update it properly like p54.
So, What do you prefer? I will prepare a new patch asap.
Best Regards,
Wu Zhangjin
>
>>
>> Larry
>>
>
> --
> []'s
> Herton
>
Wu Zhangjin wrote:
> led can not be turned off normally when rfkill is blocked, the cause is
> the led_turn_off() function exit as not expected:
Hmm. While this sounds more sensible, is it needed? And what does the windows
driver do?
I think there are two kind of LEDs - one that comes on and off with the rfkill
switch; Larry or Herton makes the 2nd one, if there is one, blink while there is
traffic (and stay steady on otherwise).
>
> drivers/net/wireless/rtl818x/rtl8187/leds.c:
>
> static void led_turn_off(struct work_struct *work)
> {
> [...]
> /* Don't change the LED, when the device is down. */
> if (!priv->vif || priv->vif->type == NL80211_IFTYPE_UNSPECIFIED)
> return ;
> [...]
>
> When rfkill is blocked, the function calling order looks like this:
>
> net/mac80211/iface.c:
>
> ieee80211_do_stop
> -> drv_remove_interface = rtl8187_remove_interface
> -> ieee80211_stop_device
> -> led_set_brightness
> -> led_turn_off
> -> drv_stop = rtl8187_stop
>
> rtl8187_remove_interface() set priv->vif to NULL, so, led_turn_off()
> will return and will not be able to turn off the led. delay the setting
> of priv->vif to rtl8187_stop() can solve it.
>
> Signed-off-by: Wu Zhangjin <[email protected]>
> ---
> drivers/net/wireless/rtl818x/rtl8187/dev.c | 8 ++++----
> 1 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/wireless/rtl818x/rtl8187/dev.c b/drivers/net/wireless/rtl818x/rtl8187/dev.c
> index 2bb5297..1fea0cd 100644
> --- a/drivers/net/wireless/rtl818x/rtl8187/dev.c
> +++ b/drivers/net/wireless/rtl818x/rtl8187/dev.c
> @@ -1007,6 +1007,9 @@ static void rtl8187_stop(struct ieee80211_hw *dev)
> u32 reg;
>
> mutex_lock(&priv->conf_mutex);
> +
> + priv->vif = NULL;
> +
> rtl818x_iowrite16(priv, &priv->map->INT_MASK, 0);
>
> reg = rtl818x_ioread8(priv, &priv->map->CMD);
> @@ -1067,10 +1070,7 @@ exit:
> static void rtl8187_remove_interface(struct ieee80211_hw *dev,
> struct ieee80211_vif *vif)
> {
> - struct rtl8187_priv *priv = dev->priv;
> - mutex_lock(&priv->conf_mutex);
> - priv->vif = NULL;
> - mutex_unlock(&priv->conf_mutex);
> + /* Nothing to do */
> }
>
> static int rtl8187_config(struct ieee80211_hw *dev, u32 changed)
On 03/26/2011 07:55 PM, Hin-Tak Leung wrote:
> wu zhangjin wrote:
>> On Sun, Mar 27, 2011 at 5:33 AM, Hin-Tak Leung
>> <[email protected]> wrote:
>>> Wu Zhangjin wrote:
>>>> led can not be turned off normally when rfkill is blocked, the cause is
>>>> the led_turn_off() function exit as not expected:
>>> Hmm. While this sounds more sensible, is it needed? And what does the
>>> windows driver do?
>>>
>>> I think there are two kind of LEDs - one that comes on and off with the
>>> rfkill switch; Larry or Herton makes the 2nd one, if there is one, blink
>>> while there is traffic (and stay steady on otherwise).
>>
>> I have used this driver on the YeeLoong netbook(only support Linux),
>> there is only one LED for rtl8187, when there is traffic, it blinks
>> perfectly, but If press the hotkey to turn off the rf, the network
>> interface is down, but the LED is still light, then, the users may
>> mistakenly think the hotkey or rfkill support doesn't work or simply
>> think the interface is still on. So, that's why we may need to fix it.
>
> My laptop (a Toshiba one) has the other kind of LED - the LED only comes on and
> off in relation to the rfkill switch, and does not blink with traffic. I think
> Herton or Larry has some devices with both types.
>
> Hmm, I seem to have the impression that there is code somewhere for switching a
> singular LED's behavior of the driver between one or the other, by echo'ing into
> sysfs or some other trickery? Or would that be a desired approach if that's not
> done at the moment?
My rtl8187 devices are both external USB sticks, thus they have no interaction
with a radio-kill switch. I will test your patch to make sure it does no harm to
my system.
I think the commit message should be revised. A simple statement like "the LED
does not turn off when the rfkill switch is off" should be sufficient.
Larry
On Sun, Mar 27, 2011 at 05:47:36PM -0500, Larry Finger wrote:
> On 03/26/2011 07:55 PM, Hin-Tak Leung wrote:
> >wu zhangjin wrote:
> >>On Sun, Mar 27, 2011 at 5:33 AM, Hin-Tak Leung
> >><[email protected]> wrote:
> >>>Wu Zhangjin wrote:
> >>>>led can not be turned off normally when rfkill is blocked, the cause is
> >>>>the led_turn_off() function exit as not expected:
> >>>Hmm. While this sounds more sensible, is it needed? And what does the
> >>>windows driver do?
> >>>
> >>>I think there are two kind of LEDs - one that comes on and off with the
> >>>rfkill switch; Larry or Herton makes the 2nd one, if there is one, blink
> >>>while there is traffic (and stay steady on otherwise).
> >>
> >>I have used this driver on the YeeLoong netbook(only support Linux),
> >>there is only one LED for rtl8187, when there is traffic, it blinks
> >>perfectly, but If press the hotkey to turn off the rf, the network
> >>interface is down, but the LED is still light, then, the users may
> >>mistakenly think the hotkey or rfkill support doesn't work or simply
> >>think the interface is still on. So, that's why we may need to fix it.
> >
> >My laptop (a Toshiba one) has the other kind of LED - the LED only comes on and
> >off in relation to the rfkill switch, and does not blink with traffic. I think
> >Herton or Larry has some devices with both types.
I had one via based netbook with proper led, but don't have it anymore.
> >
> >Hmm, I seem to have the impression that there is code somewhere for switching a
> >singular LED's behavior of the driver between one or the other, by echo'ing into
> >sysfs or some other trickery? Or would that be a desired approach if that's not
> >done at the moment?
>
> My rtl8187 devices are both external USB sticks, thus they have no
> interaction with a radio-kill switch. I will test your patch to make
> sure it does no harm to my system.
>
> I think the commit message should be revised. A simple statement
> like "the LED does not turn off when the rfkill switch is off"
> should be sufficient.
The patch should work, but I wonder if we should be fiddling with priv->vif
for this, perhaps we should not assume vif to be valid after
rtl8187_remove_interface (I don't see problems with current
rtl8187/mac80211 code on a quick look, but...)
I cleaner solution may be to use a priv->mode like p54.
>
> Larry
>
--
[]'s
Herton
On Sun, Mar 27, 2011 at 8:55 AM, Hin-Tak Leung
<[email protected]> wrote:
[...]
>
> Hmm, I seem to have the impression that there is code somewhere for
> switching a singular LED's behavior of the driver between one or the other,
> by echo'ing into sysfs or some other trickery? Or would that be a desired
> approach if that's not done at the moment?
Do you mean the interface: /sys/class/rfkill/rfkill0/state? or
something like /sys/class/leds/...::/brightness?
Seems the LED behavior can be triggered automatically by the rfkill
input event with the kernel config option(MAC80211_LEDS, RFKILL_LEDS),
so, echo'ing to such interface may be not a good idea ;-)
Best Regards,
Wu Zhangjin
wu zhangjin wrote:
> On Sun, Mar 27, 2011 at 5:33 AM, Hin-Tak Leung
> <[email protected]> wrote:
>> Wu Zhangjin wrote:
>>> led can not be turned off normally when rfkill is blocked, the cause is
>>> the led_turn_off() function exit as not expected:
>> Hmm. While this sounds more sensible, is it needed? And what does the
>> windows driver do?
>>
>> I think there are two kind of LEDs - one that comes on and off with the
>> rfkill switch; Larry or Herton makes the 2nd one, if there is one, blink
>> while there is traffic (and stay steady on otherwise).
>
> I have used this driver on the YeeLoong netbook(only support Linux),
> there is only one LED for rtl8187, when there is traffic, it blinks
> perfectly, but If press the hotkey to turn off the rf, the network
> interface is down, but the LED is still light, then, the users may
> mistakenly think the hotkey or rfkill support doesn't work or simply
> think the interface is still on. So, that's why we may need to fix it.
My laptop (a Toshiba one) has the other kind of LED - the LED only comes on and
off in relation to the rfkill switch, and does not blink with traffic. I think
Herton or Larry has some devices with both types.
Hmm, I seem to have the impression that there is code somewhere for switching a
singular LED's behavior of the driver between one or the other, by echo'ing into
sysfs or some other trickery? Or would that be a desired approach if that's not
done at the moment?
>
> Regards,
> Wu Zhangjin
>
>>> drivers/net/wireless/rtl818x/rtl8187/leds.c:
>>>
>>> static void led_turn_off(struct work_struct *work)
>>> {
>>> [...]
>>> /* Don't change the LED, when the device is down. */
>>> if (!priv->vif || priv->vif->type == NL80211_IFTYPE_UNSPECIFIED)
>>> return ;
>>> [...]
>>>
>>> When rfkill is blocked, the function calling order looks like this:
>>>
>>> net/mac80211/iface.c:
>>>
>>> ieee80211_do_stop
>>> -> drv_remove_interface = rtl8187_remove_interface
>>> -> ieee80211_stop_device
>>> -> led_set_brightness
>>> -> led_turn_off
>>> -> drv_stop = rtl8187_stop
>>>
>>> rtl8187_remove_interface() set priv->vif to NULL, so, led_turn_off()
>>> will return and will not be able to turn off the led. delay the setting
>>> of priv->vif to rtl8187_stop() can solve it.
>>>
>>> Signed-off-by: Wu Zhangjin <[email protected]>
>>> ---
>>> drivers/net/wireless/rtl818x/rtl8187/dev.c | 8 ++++----
>>> 1 files changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/net/wireless/rtl818x/rtl8187/dev.c
>>> b/drivers/net/wireless/rtl818x/rtl8187/dev.c
>>> index 2bb5297..1fea0cd 100644
>>> --- a/drivers/net/wireless/rtl818x/rtl8187/dev.c
>>> +++ b/drivers/net/wireless/rtl818x/rtl8187/dev.c
>>> @@ -1007,6 +1007,9 @@ static void rtl8187_stop(struct ieee80211_hw *dev)
>>> u32 reg;
>>> mutex_lock(&priv->conf_mutex);
>>> +
>>> + priv->vif = NULL;
>>> +
>>> rtl818x_iowrite16(priv, &priv->map->INT_MASK, 0);
>>> reg = rtl818x_ioread8(priv, &priv->map->CMD);
>>> @@ -1067,10 +1070,7 @@ exit:
>>> static void rtl8187_remove_interface(struct ieee80211_hw *dev,
>>> struct ieee80211_vif *vif)
>>> {
>>> - struct rtl8187_priv *priv = dev->priv;
>>> - mutex_lock(&priv->conf_mutex);
>>> - priv->vif = NULL;
>>> - mutex_unlock(&priv->conf_mutex);
>>> + /* Nothing to do */
>>> }
>>> static int rtl8187_config(struct ieee80211_hw *dev, u32 changed)
>
On Mon, Mar 28, 2011 at 6:47 AM, Larry Finger <[email protected]> wrote:
[...]
>
> I think the commit message should be revised. A simple statement like "the
> LED does not turn off when the rfkill switch is off" should be sufficient.
Yeah, the above statement is simpler ;-)
Regards,
Wu Zhangjin
>
> Larry
>
>
On Sun, Mar 27, 2011 at 5:33 AM, Hin-Tak Leung
<[email protected]> wrote:
> Wu Zhangjin wrote:
>>
>> led can not be turned off normally when rfkill is blocked, the cause is
>> the led_turn_off() function exit as not expected:
>
> Hmm. While this sounds more sensible, is it needed? And what does the
> windows driver do?
>
> I think there are two kind of LEDs - one that comes on and off with the
> rfkill switch; Larry or Herton makes the 2nd one, if there is one, blink
> while there is traffic (and stay steady on otherwise).
I have used this driver on the YeeLoong netbook(only support Linux),
there is only one LED for rtl8187, when there is traffic, it blinks
perfectly, but If press the hotkey to turn off the rf, the network
interface is down, but the LED is still light, then, the users may
mistakenly think the hotkey or rfkill support doesn't work or simply
think the interface is still on. So, that's why we may need to fix it.
Regards,
Wu Zhangjin
>
>>
>> drivers/net/wireless/rtl818x/rtl8187/leds.c:
>>
>> static void led_turn_off(struct work_struct *work)
>> {
>> ? ? ? ?[...]
>> ? ? ? ?/* Don't change the LED, when the device is down. */
>> ? ? ? ?if (!priv->vif || priv->vif->type == NL80211_IFTYPE_UNSPECIFIED)
>> ? ? ? ? ? ? ? ?return ;
>> ? ? ? ?[...]
>>
>> When rfkill is blocked, the function calling order looks like this:
>>
>> net/mac80211/iface.c:
>>
>> ieee80211_do_stop
>> ? ? ? ?-> drv_remove_interface = rtl8187_remove_interface
>> ? ? ? ?-> ieee80211_stop_device
>> ? ? ? ? ? ? ? ?-> led_set_brightness
>> ? ? ? ? ? ? ? ? ? ? ? ?-> led_turn_off
>> ? ? ? ?-> drv_stop = rtl8187_stop
>>
>> rtl8187_remove_interface() set priv->vif to NULL, so, led_turn_off()
>> will return and will not be able to turn off the led. delay the setting
>> of priv->vif to rtl8187_stop() can solve it.
>>
>> Signed-off-by: Wu Zhangjin <[email protected]>
>> ---
>> ?drivers/net/wireless/rtl818x/rtl8187/dev.c | ? ?8 ++++----
>> ?1 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/wireless/rtl818x/rtl8187/dev.c
>> b/drivers/net/wireless/rtl818x/rtl8187/dev.c
>> index 2bb5297..1fea0cd 100644
>> --- a/drivers/net/wireless/rtl818x/rtl8187/dev.c
>> +++ b/drivers/net/wireless/rtl818x/rtl8187/dev.c
>> @@ -1007,6 +1007,9 @@ static void rtl8187_stop(struct ieee80211_hw *dev)
>> ? ? ? ?u32 reg;
>> ? ? ? ?mutex_lock(&priv->conf_mutex);
>> +
>> + ? ? ? priv->vif = NULL;
>> +
>> ? ? ? ?rtl818x_iowrite16(priv, &priv->map->INT_MASK, 0);
>> ? ? ? ?reg = rtl818x_ioread8(priv, &priv->map->CMD);
>> @@ -1067,10 +1070,7 @@ exit:
>> ?static void rtl8187_remove_interface(struct ieee80211_hw *dev,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct ieee80211_vif *vif)
>> ?{
>> - ? ? ? struct rtl8187_priv *priv = dev->priv;
>> - ? ? ? mutex_lock(&priv->conf_mutex);
>> - ? ? ? priv->vif = NULL;
>> - ? ? ? mutex_unlock(&priv->conf_mutex);
>> + ? ? ? /* Nothing to do */
>> ?}
>> ?static int rtl8187_config(struct ieee80211_hw *dev, u32 changed)
>
On 4/1/11, Herton Ronaldo Krzesinski <[email protected]> wrote:
> On Fri, Apr 01, 2011 at 12:22:51AM +0800, wu zhangjin wrote:
>> On Mon, Mar 28, 2011 at 11:40 PM, Herton Ronaldo Krzesinski
>> <[email protected]> wrote:
>> [...]
>> >>
>> >> My rtl8187 devices are both external USB sticks, thus they have no
>> >> interaction with a radio-kill switch. I will test your patch to make
>> >> sure it does no harm to my system.
>> >>
>> >> I think the commit message should be revised. A simple statement
>> >> like "the LED does not turn off when the rfkill switch is off"
>> >> should be sufficient.
>> >
>> > The patch should work, but I wonder if we should be fiddling with
>> > priv->vif
>> > for this, perhaps we should not assume vif to be valid after
>> > rtl8187_remove_interface (I don't see problems with current
>> > rtl8187/mac80211 code on a quick look, but...)
>> >
>>
>> Yes, but seems only two places have touched the vif pointer.
>>
>> $ grep "vif =" -ur drivers/net/wireless/rtl818x/rtl8187/
>> drivers/net/wireless/rtl818x/rtl8187/dev.c: priv->vif = NULL;
>> drivers/net/wireless/rtl818x/rtl8187/dev.c: priv->vif = vif;
>>
>> > I cleaner solution may be to use a priv->mode like p54.
>>
>> Yep, then, we may need to add a 'mode' member to rtl8187_priv and
>> update it properly like p54.
>>
>> So, What do you prefer? I will prepare a new patch asap.
>
> In the led code we dereference priv->vif, checking for priv->vif->type.
> vif is a pointer from mac80211, and after rtl8187_remove_interface is
> called by it, at least from my POV can do anything it wants with vif,
> like freeing the sdata vif points to. From what I checked, it doesn't
> free currently and your patch don't have issues, but may be it's better
> to be safe and add a new "future proof" mode field.
>
> And as your bug shows, the led code access priv->vif after
> rtl8187_remove_interface, so we hit the case where vif could be
> considered invalid.
>
> So because of this and being cleaner I would go adding 'mode'.
Ok, thanks ;-)
Best Regards,
Wu Zhangjin
>
>>
>> Best Regards,
>> Wu Zhangjin
>>
>> >
>> >>
>> >> Larry
>> >>
>> >
>> > --
>> > []'s
>> > Herton
>> >
>>
>
> --
> []'s
> Herton
>