In http://marc.info/?l=linux-wireless&m=125916285209175&w=2, Michael
Buesch reports a problem with rtl8187 queuing LED on/off requests after
the suspend has begun. On my system this is present during a suspend
to disk.
This solution involves adding the power management entries to the
driver to set a flag indicating that the system is suspending. When the
flag is set, no LED on/off events are queued.
Signed-off-by: Larry Finger <[email protected]>
---
Index: wireless-testing/drivers/net/wireless/rtl818x/rtl8187.h
===================================================================
--- wireless-testing.orig/drivers/net/wireless/rtl818x/rtl8187.h
+++ wireless-testing/drivers/net/wireless/rtl818x/rtl8187.h
@@ -138,6 +138,7 @@ struct rtl8187_priv {
__le32 bits32;
} *io_dmabuf;
bool rfkill_off;
+ bool suspending; /* true if shutting down */
};
void rtl8187_write_phy(struct ieee80211_hw *dev, u8 addr, u32 data);
Index: wireless-testing/drivers/net/wireless/rtl818x/rtl8187_dev.c
===================================================================
--- wireless-testing.orig/drivers/net/wireless/rtl818x/rtl8187_dev.c
+++ wireless-testing/drivers/net/wireless/rtl818x/rtl8187_dev.c
@@ -1521,6 +1521,7 @@ static int __devinit rtl8187_probe(struc
wiphy_name(dev->wiphy), dev->wiphy->perm_addr,
chip_name, priv->asic_rev, priv->rf->name, priv->rfkill_mask);
+ priv->suspending = 0;
#ifdef CONFIG_RTL8187_LEDS
eeprom_93cx6_read(&eeprom, 0x3F, ®);
reg &= 0xFF;
@@ -1539,6 +1540,38 @@ static int __devinit rtl8187_probe(struc
return err;
}
+#ifdef CONFIG_PM
+static int rtl8187_suspend(struct usb_interface *intf, pm_message_t message)
+{
+ struct ieee80211_hw *dev = usb_get_intfdata(intf);
+ struct rtl8187_priv *priv;
+
+ if (!dev)
+ return -ENODEV;
+
+ priv = dev->priv;
+ priv->suspending = 1;
+#ifdef CONFIG_RTL8187_LEDS
+ cancel_delayed_work_sync(&priv->led_off);
+ cancel_delayed_work_sync(&priv->led_on);
+#endif
+ return 0;
+}
+
+static int rtl8187_resume(struct usb_interface *intf)
+{
+ struct ieee80211_hw *dev = usb_get_intfdata(intf);
+ struct rtl8187_priv *priv;
+
+ if (!dev)
+ return -ENODEV;
+
+ priv = dev->priv;
+ priv->suspending = 0;
+ return 0;
+}
+#endif
+
static void __devexit rtl8187_disconnect(struct usb_interface *intf)
{
struct ieee80211_hw *dev = usb_get_intfdata(intf);
@@ -1564,6 +1597,10 @@ static struct usb_driver rtl8187_driver
.name = KBUILD_MODNAME,
.id_table = rtl8187_table,
.probe = rtl8187_probe,
+#ifdef CONFIG_PM
+ .suspend = rtl8187_suspend,
+ .resume = rtl8187_resume,
+#endif
.disconnect = __devexit_p(rtl8187_disconnect),
};
Index: wireless-testing/drivers/net/wireless/rtl818x/rtl8187_leds.c
===================================================================
--- wireless-testing.orig/drivers/net/wireless/rtl818x/rtl8187_leds.c
+++ wireless-testing/drivers/net/wireless/rtl818x/rtl8187_leds.c
@@ -107,6 +107,8 @@ static void rtl8187_led_brightness_set(s
struct ieee80211_hw *hw = led->dev;
struct rtl8187_priv *priv = hw->priv;
+ if (priv->suspending)
+ return;
if (brightness == LED_OFF) {
ieee80211_queue_delayed_work(hw, &priv->led_off, 0);
/* The LED is off for 1/20 sec so that it just blinks. */
@@ -209,11 +211,12 @@ void rtl8187_leds_exit(struct ieee80211_
struct rtl8187_priv *priv = dev->priv;
/* turn the LED off before exiting */
- ieee80211_queue_delayed_work(dev, &priv->led_off, 0);
- rtl8187_unregister_led(&priv->led_rx);
- rtl8187_unregister_led(&priv->led_tx);
+ if (!priv->suspending)
+ ieee80211_queue_delayed_work(dev, &priv->led_off, 0);
cancel_delayed_work_sync(&priv->led_off);
cancel_delayed_work_sync(&priv->led_on);
+ rtl8187_unregister_led(&priv->led_rx);
+ rtl8187_unregister_led(&priv->led_tx);
}
#endif /* def CONFIG_RTL8187_LED */
Em Ter 08 Dez 2009, ?s 09:02:02, Michael Buesch escreveu:
> On Tuesday 08 December 2009 03:08:02 Larry Finger wrote:
> > In http://marc.info/?l=linux-wireless&m=125916285209175&w=2, Michael
> > Buesch reports a problem with rtl8187 queuing LED on/off requests after
> > the suspend has begun. On my system this is present during a suspend
> > to disk.
> >
> > This solution involves adding the power management entries to the
> > driver to set a flag indicating that the system is suspending. When the
> > flag is set, no LED on/off events are queued.
> >
> > Signed-off-by: Larry Finger <[email protected]>
> > ---
> >
> > Index: wireless-testing/drivers/net/wireless/rtl818x/rtl8187.h
> > ===================================================================
> > --- wireless-testing.orig/drivers/net/wireless/rtl818x/rtl8187.h
> > +++ wireless-testing/drivers/net/wireless/rtl818x/rtl8187.h
> > @@ -138,6 +138,7 @@ struct rtl8187_priv {
> > __le32 bits32;
> > } *io_dmabuf;
> > bool rfkill_off;
> > + bool suspending; /* true if shutting down */
> > };
> >
> > void rtl8187_write_phy(struct ieee80211_hw *dev, u8 addr, u32 data);
> > Index: wireless-testing/drivers/net/wireless/rtl818x/rtl8187_dev.c
> > ===================================================================
> > --- wireless-testing.orig/drivers/net/wireless/rtl818x/rtl8187_dev.c
> > +++ wireless-testing/drivers/net/wireless/rtl818x/rtl8187_dev.c
> > @@ -1521,6 +1521,7 @@ static int __devinit rtl8187_probe(struc
> > wiphy_name(dev->wiphy), dev->wiphy->perm_addr,
> > chip_name, priv->asic_rev, priv->rf->name, priv-
>rfkill_mask);
> >
> > + priv->suspending = 0;
> > #ifdef CONFIG_RTL8187_LEDS
> > eeprom_93cx6_read(&eeprom, 0x3F, ®);
> > reg &= 0xFF;
> > @@ -1539,6 +1540,38 @@ static int __devinit rtl8187_probe(struc
> > return err;
> > }
> >
> > +#ifdef CONFIG_PM
> > +static int rtl8187_suspend(struct usb_interface *intf, pm_message_t
> > message) +{
> > + struct ieee80211_hw *dev = usb_get_intfdata(intf);
> > + struct rtl8187_priv *priv;
> > +
> > + if (!dev)
> > + return -ENODEV;
> > +
> > + priv = dev->priv;
> > + priv->suspending = 1;
> > +#ifdef CONFIG_RTL8187_LEDS
> > + cancel_delayed_work_sync(&priv->led_off);
> > + cancel_delayed_work_sync(&priv->led_on);
> > +#endif
> > + return 0;
> > +}
> > +
> > +static int rtl8187_resume(struct usb_interface *intf)
> > +{
> > + struct ieee80211_hw *dev = usb_get_intfdata(intf);
> > + struct rtl8187_priv *priv;
> > +
> > + if (!dev)
> > + return -ENODEV;
> > +
> > + priv = dev->priv;
> > + priv->suspending = 0;
> > + return 0;
> > +}
> > +#endif
> > +
> > static void __devexit rtl8187_disconnect(struct usb_interface *intf)
> > {
> > struct ieee80211_hw *dev = usb_get_intfdata(intf);
> > @@ -1564,6 +1597,10 @@ static struct usb_driver rtl8187_driver
> > .name = KBUILD_MODNAME,
> > .id_table = rtl8187_table,
> > .probe = rtl8187_probe,
> > +#ifdef CONFIG_PM
> > + .suspend = rtl8187_suspend,
> > + .resume = rtl8187_resume,
> > +#endif
> > .disconnect = __devexit_p(rtl8187_disconnect),
> > };
> >
> > Index: wireless-testing/drivers/net/wireless/rtl818x/rtl8187_leds.c
> > ===================================================================
> > --- wireless-testing.orig/drivers/net/wireless/rtl818x/rtl8187_leds.c
> > +++ wireless-testing/drivers/net/wireless/rtl818x/rtl8187_leds.c
> > @@ -107,6 +107,8 @@ static void rtl8187_led_brightness_set(s
> > struct ieee80211_hw *hw = led->dev;
> > struct rtl8187_priv *priv = hw->priv;
> >
> > + if (priv->suspending)
> > + return;
> > if (brightness == LED_OFF) {
> > ieee80211_queue_delayed_work(hw, &priv->led_off, 0);
> > /* The LED is off for 1/20 sec so that it just blinks. */
> > @@ -209,11 +211,12 @@ void rtl8187_leds_exit(struct ieee80211_
> > struct rtl8187_priv *priv = dev->priv;
> >
> > /* turn the LED off before exiting */
> > - ieee80211_queue_delayed_work(dev, &priv->led_off, 0);
> > - rtl8187_unregister_led(&priv->led_rx);
> > - rtl8187_unregister_led(&priv->led_tx);
> > + if (!priv->suspending)
> > + ieee80211_queue_delayed_work(dev, &priv->led_off, 0);
> > cancel_delayed_work_sync(&priv->led_off);
> > cancel_delayed_work_sync(&priv->led_on);
> > + rtl8187_unregister_led(&priv->led_rx);
> > + rtl8187_unregister_led(&priv->led_tx);
> > }
> > #endif /* def CONFIG_RTL8187_LED */
>
> Did you test this? I think it can't work.
> mac80211's suspend calls stop operation before rtl8187_suspend() is run. So
> the actual suspend operations runs without suspend flag set.
> I think setting a flag is wrong. I also did that mistake in the broadcom
> driver and it's wrong. IMO the LED register/unregister code must be
> completely removed from the start/stop suspend/resume and hibernate paths.
> Instead the LEDs must be registered at device attach phase (that's where
> the device is registered to mac80211).
That's why in my last patch I added code to register the radio led, we have to
go that way to fix properly the issue (also this way we turn the led on/off only
when mac80211 wants it, which is the right way), so we can avoid having to
place code in start/stop or creating suspend/resume hooks only because the
issue.
The problem here is not simple: we have leds code triggering work and mac80211
scheduling led tx work even after radio led stop on device shutdown:
- On unregistering led device, even after mac80211 did the work to turn off the
led, the unregister code of led subsystem calls again led_brightness_set to
turn off the led, which causes the original warning without suspend/resume
hooks (also in leds_exit code we were scheduling work which triggered more
warnings).
- When I was testing, sometimes mac80211 on device shutdown (modprobe -r)
wasn't calling led stop code in the right order, so sometimes after removing
module the led was kept turned on (scheduling led tx code after led radio off).
This happens because ieee80211_tx_status can call ieee80211_led_tx after
mac80211 calls ieee80211_led_radio to turn the led off, and as we are
"emulating" a radio led (we have only one real led to signal radio and tx/rx,
no differentiation on hardware) we are affected by this.
So besides registering the radio led, I had to add that radio_on variable
check on last patch, to handle these two problems. The patch worked, but Larry
reported a crash, I'll try to review it more and see what could cause a crash
on removal, may be some delayed_work related thing missing, unfortunately I'm
a bit busy here since then, but will try to get to it soon.
--
[]'s
Herton
--- On Tue, 8/12/09, Herton Ronaldo Krzesinski <[email protected]> wrote:
> The problem here is not simple: we have leds code
> triggering work and mac80211
> scheduling led tx work even after radio led stop on device
> shutdown:
> - On unregistering led device, even after mac80211 did the
> work to turn off the
> led, the unregister code of led subsystem calls again
> led_brightness_set to
> turn off the led, which causes the original warning without
> suspend/resume
> hooks (also in leds_exit code we were scheduling work which
> triggered more
> warnings).
> - When I was testing, sometimes mac80211 on device shutdown
> (modprobe -r)
> wasn't calling led stop code in the right order, so
> sometimes after removing
> module the led was kept turned on (scheduling led tx code
> after led radio off).
> This happens because ieee80211_tx_status can call
> ieee80211_led_tx after
> mac80211 calls ieee80211_led_radio to turn the led off, and
> as we are
> "emulating" a radio led (we have only one real led to
> signal radio and tx/rx,
> no differentiation on hardware) we are affected by this.
>
> So besides registering the radio led, I had to add that
> radio_on variable
> check on last patch, to handle these two problems. The
> patch worked, but Larry
> reported a crash, I'll try to review it more and see what
> could cause a crash
> on removal, may be some delayed_work related thing missing,
> unfortunately I'm
> a bit busy here since then, but will try to get to it
> soon.
Sorry I haven't been paying too much attention due to other commitments... that sounds like race condition somewhere. Besides adding flag variables, would mutex help?
On 12/08/2009 09:41 AM, Herton Ronaldo Krzesinski wrote:
> That's why in my last patch I added code to register the radio led, we have to
> go that way to fix properly the issue (also this way we turn the led on/off only
> when mac80211 wants it, which is the right way), so we can avoid having to
> place code in start/stop or creating suspend/resume hooks only because the
> issue.
I do not understand the reason for adding another LED device, nor why
you think it helps. When I added the LED code, I wanted to get the LED
to indicate that there was I/O activity, which is why I chose the TX
and RX LEDs. Choosing "on" as the default state when there was no
activity and blinking it "off" with I/O seems to give exactly the
right behavior. Unfortunately, the asynchronous behavior of the USB
operations makes it very tricky to get the shutdown right.
Larry
On 12/08/2009 05:02 AM, Michael Buesch wrote:
> Did you test this? I think it can't work.
> mac80211's suspend calls stop operation before rtl8187_suspend() is run. So the
> actual suspend operations runs without suspend flag set.
> I think setting a flag is wrong. I also did that mistake in the broadcom driver
> and it's wrong. IMO the LED register/unregister code must be completely removed
> from the start/stop suspend/resume and hibernate paths. Instead the LEDs must be
> registered at device attach phase (that's where the device is registered to mac80211).
Yes, I did test this. Thus far, I have done 10 suspend to disk/resume
cycles without any problems. That is not a lot of cycles, but I got
the warning every time before this patch, and none since. In this
section, we also have to worry about kernel panics during rmmod or
device removal. The new changes have survived about 400 rmmod/insmod
cycles, and 20 device removal/reinsertion tests.
As part of the suspend/resume testing, I added printk() statements at
the entry to the new suspend routine and the corresponding routine in
mac80211. In my tests, the mac80211 routine was called first only one
time. In all other cases, the call to the rtl8187 routine was logged
first; however, I don't think that matters. The critical part is that
the flag be set to prevent queuing another LED on/off task.
I will see if I can move the LED register/unregister calls as you suggest.
Larry
On Tuesday 08 December 2009 03:08:02 Larry Finger wrote:
> In http://marc.info/?l=linux-wireless&m=125916285209175&w=2, Michael
> Buesch reports a problem with rtl8187 queuing LED on/off requests after
> the suspend has begun. On my system this is present during a suspend
> to disk.
>
> This solution involves adding the power management entries to the
> driver to set a flag indicating that the system is suspending. When the
> flag is set, no LED on/off events are queued.
>
> Signed-off-by: Larry Finger <[email protected]>
> ---
>
> Index: wireless-testing/drivers/net/wireless/rtl818x/rtl8187.h
> ===================================================================
> --- wireless-testing.orig/drivers/net/wireless/rtl818x/rtl8187.h
> +++ wireless-testing/drivers/net/wireless/rtl818x/rtl8187.h
> @@ -138,6 +138,7 @@ struct rtl8187_priv {
> __le32 bits32;
> } *io_dmabuf;
> bool rfkill_off;
> + bool suspending; /* true if shutting down */
> };
>
> void rtl8187_write_phy(struct ieee80211_hw *dev, u8 addr, u32 data);
> Index: wireless-testing/drivers/net/wireless/rtl818x/rtl8187_dev.c
> ===================================================================
> --- wireless-testing.orig/drivers/net/wireless/rtl818x/rtl8187_dev.c
> +++ wireless-testing/drivers/net/wireless/rtl818x/rtl8187_dev.c
> @@ -1521,6 +1521,7 @@ static int __devinit rtl8187_probe(struc
> wiphy_name(dev->wiphy), dev->wiphy->perm_addr,
> chip_name, priv->asic_rev, priv->rf->name, priv->rfkill_mask);
>
> + priv->suspending = 0;
> #ifdef CONFIG_RTL8187_LEDS
> eeprom_93cx6_read(&eeprom, 0x3F, ®);
> reg &= 0xFF;
> @@ -1539,6 +1540,38 @@ static int __devinit rtl8187_probe(struc
> return err;
> }
>
> +#ifdef CONFIG_PM
> +static int rtl8187_suspend(struct usb_interface *intf, pm_message_t message)
> +{
> + struct ieee80211_hw *dev = usb_get_intfdata(intf);
> + struct rtl8187_priv *priv;
> +
> + if (!dev)
> + return -ENODEV;
> +
> + priv = dev->priv;
> + priv->suspending = 1;
> +#ifdef CONFIG_RTL8187_LEDS
> + cancel_delayed_work_sync(&priv->led_off);
> + cancel_delayed_work_sync(&priv->led_on);
> +#endif
> + return 0;
> +}
> +
> +static int rtl8187_resume(struct usb_interface *intf)
> +{
> + struct ieee80211_hw *dev = usb_get_intfdata(intf);
> + struct rtl8187_priv *priv;
> +
> + if (!dev)
> + return -ENODEV;
> +
> + priv = dev->priv;
> + priv->suspending = 0;
> + return 0;
> +}
> +#endif
> +
> static void __devexit rtl8187_disconnect(struct usb_interface *intf)
> {
> struct ieee80211_hw *dev = usb_get_intfdata(intf);
> @@ -1564,6 +1597,10 @@ static struct usb_driver rtl8187_driver
> .name = KBUILD_MODNAME,
> .id_table = rtl8187_table,
> .probe = rtl8187_probe,
> +#ifdef CONFIG_PM
> + .suspend = rtl8187_suspend,
> + .resume = rtl8187_resume,
> +#endif
> .disconnect = __devexit_p(rtl8187_disconnect),
> };
>
> Index: wireless-testing/drivers/net/wireless/rtl818x/rtl8187_leds.c
> ===================================================================
> --- wireless-testing.orig/drivers/net/wireless/rtl818x/rtl8187_leds.c
> +++ wireless-testing/drivers/net/wireless/rtl818x/rtl8187_leds.c
> @@ -107,6 +107,8 @@ static void rtl8187_led_brightness_set(s
> struct ieee80211_hw *hw = led->dev;
> struct rtl8187_priv *priv = hw->priv;
>
> + if (priv->suspending)
> + return;
> if (brightness == LED_OFF) {
> ieee80211_queue_delayed_work(hw, &priv->led_off, 0);
> /* The LED is off for 1/20 sec so that it just blinks. */
> @@ -209,11 +211,12 @@ void rtl8187_leds_exit(struct ieee80211_
> struct rtl8187_priv *priv = dev->priv;
>
> /* turn the LED off before exiting */
> - ieee80211_queue_delayed_work(dev, &priv->led_off, 0);
> - rtl8187_unregister_led(&priv->led_rx);
> - rtl8187_unregister_led(&priv->led_tx);
> + if (!priv->suspending)
> + ieee80211_queue_delayed_work(dev, &priv->led_off, 0);
> cancel_delayed_work_sync(&priv->led_off);
> cancel_delayed_work_sync(&priv->led_on);
> + rtl8187_unregister_led(&priv->led_rx);
> + rtl8187_unregister_led(&priv->led_tx);
> }
> #endif /* def CONFIG_RTL8187_LED */
>
>
>
Did you test this? I think it can't work.
mac80211's suspend calls stop operation before rtl8187_suspend() is run. So the
actual suspend operations runs without suspend flag set.
I think setting a flag is wrong. I also did that mistake in the broadcom driver
and it's wrong. IMO the LED register/unregister code must be completely removed
from the start/stop suspend/resume and hibernate paths. Instead the LEDs must be
registered at device attach phase (that's where the device is registered to mac80211).
--
Greetings, Michael.
Em Ter 08 Dez 2009, ?s 18:16:59, Larry Finger escreveu:
> On 12/08/2009 09:41 AM, Herton Ronaldo Krzesinski wrote:
> > That's why in my last patch I added code to register the radio led, we
> > have to go that way to fix properly the issue (also this way we turn the
> > led on/off only when mac80211 wants it, which is the right way), so we
> > can avoid having to place code in start/stop or creating suspend/resume
> > hooks only because the issue.
>
> I do not understand the reason for adding another LED device, nor why
> you think it helps. When I added the LED code, I wanted to get the LED
> to indicate that there was I/O activity, which is why I chose the TX
> and RX LEDs. Choosing "on" as the default state when there was no
> activity and blinking it "off" with I/O seems to give exactly the
> right behavior. Unfortunately, the asynchronous behavior of the USB
> operations makes it very tricky to get the shutdown right.
It's because without registering a radio led the current behaviour is wrong
for some cases, the led stays on when it shouldn't. For example, if you bring
the interface down (ifconfig <if> down) the led stays on, when it shouldn't.
The led should only be active while interface is active, between start and
stop (or in other words, the radio is on). After mac80211 stops the device,
any i/o (led activity) shouldn't happen.
>
> Larry
--
[]'s
Herton
On 12/08/2009 02:57 PM, Herton Ronaldo Krzesinski wrote:
> It's because without registering a radio led the current behaviour is wrong
> for some cases, the led stays on when it shouldn't. For example, if you bring
> the interface down (ifconfig <if> down) the led stays on, when it shouldn't.
> The led should only be active while interface is active, between start and
> stop (or in other words, the radio is on). After mac80211 stops the device,
> any i/o (led activity) shouldn't happen.
OK, then the turn off of the radio LED eliminates the "turn-off" that
I put in the shutdown process.
Larry
On 12/08/2009 04:18 PM, Hin-Tak Leung wrote:
>
> Sorry I haven't been paying too much attention due to other commitments... that sounds like race condition somewhere. Besides adding flag variables, would mutex help?
It is a race in the sense that mac80211 may queue a LED change during
the shut down process. So far, it seems that the slight change that I
posted to Herton's patch is going to work. It keeps the brightness
setting routine from queuing any more on/off sequences after the LED
has been removed.
Larry
Em Ter 08 Dez 2009, ?s 19:00:54, Larry Finger escreveu:
> On 12/08/2009 02:57 PM, Herton Ronaldo Krzesinski wrote:
> > It's because without registering a radio led the current behaviour is
> > wrong for some cases, the led stays on when it shouldn't. For example, if
> > you bring the interface down (ifconfig <if> down) the led stays on, when
> > it shouldn't. The led should only be active while interface is active,
> > between start and stop (or in other words, the radio is on). After
> > mac80211 stops the device, any i/o (led activity) shouldn't happen.
>
> OK, then the turn off of the radio LED eliminates the "turn-off" that
> I put in the shutdown process.
Yes, but note it isn't the same thing, only if the turn off happened at
rtl8187_stop, which doesn't happen at the moment, the led should be turned off
at _stop.
>
> Larry
>
--
[]'s
Herton