| Message ID | 1301174394-12642-1-git-send-email-wuzhangjin@gmail.com |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
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 <wuzhangjin@gmail.com> > --- > 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) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Mar 27, 2011 at 5:33 AM, Hin-Tak Leung <htl10@users.sourceforge.net> 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 <wuzhangjin@gmail.com> >> --- >> 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) > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
wu zhangjin wrote: > On Sun, Mar 27, 2011 at 5:33 AM, Hin-Tak Leung > <htl10@users.sourceforge.net> 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 <wuzhangjin@gmail.com> >>> --- >>> 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) > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 >> <htl10@users.sourceforge.net> 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > >><htl10@users.sourceforge.net> 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 >
On Sun, Mar 27, 2011 at 8:55 AM, Hin-Tak Leung <htl10@users.sourceforge.net> 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 28, 2011 at 6:47 AM, Larry Finger <Larry.Finger@lwfinger.net> 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 > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 28, 2011 at 11:40 PM, Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com> 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 > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > <herton.krzesinski@canonical.com> 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 > > >
On 4/1/11, Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com> 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 >> <herton.krzesinski@canonical.com> 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 > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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)
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 <wuzhangjin@gmail.com> --- drivers/net/wireless/rtl818x/rtl8187/dev.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-)