Re: [PATCH] rtl8187: Fix kernel oops when device is removed when LEDS enabled (Bugzilla #14539)

[Christian Lamparter <chunkeey@googlemail.com>]

On Wednesday 04 November 2009 16:11:33 John W. Linville wrote:
> On Wed, Nov 04, 2009 at 12:00:25AM -0600, Larry Finger wrote:
> > As reported by Rick Farina (sidhayn@gmail.com), removing the RTL8187 USB
> > stick, or unloading the driver rtl8187 using rmmod will cause a kernel oops.
> > There are at least two forms of the failure, (1) BUG: Scheduling while atomic,
> > and (2) a fatal kernel page fault. This problem is reported in Bugzilla #14539.
> > 
> > This problem does not occur for kernel 2.6.31, but does for 2.6.32-rc2, thus
> > it is technically a regression; however, bisection did not locate any faulty
> > patch. The fix was found by comparing the faulty code in rtl8187 with p54usb.
> > My interpretation is that the handling of work queues in mac80211 changed
> > enough to the LEDs to be unregistered before tasks on the work queues are
> > cancelled. Previously, these actions could be done in either order.
> > 
> > Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
> > Reported-and-tested by: Rick Farina <sidhayn@gmail.com>
> > ---
> > 
> > John,
> > 
> > This is 2.6.32 material. Sorry to take so long to get a patch, but it was
> > difficult for me to locate the problem. Fortunately, I had the postings of the
> > two flame wars to amuse me while all the kernel compilations were happening.
> > 
> > Larry
> > ---
> > 
> > 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
> > @@ -210,10 +210,10 @@ void rtl8187_leds_exit(struct ieee80211_
> >  
> >  	/* turn the LED off before exiting */
> >  	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);
> > +	cancel_delayed_work_sync(&priv->led_off);
> > +	cancel_delayed_work_sync(&priv->led_on);
> >  }
> >  #endif /* def CONFIG_RTL8187_LED */
> >  
> 
> This seems like a band-aid.  If anything, the original order would
> seem to make more sense.

really?

take this code from led-class.c

void led_classdev_unregister(struct led_classdev *led_cdev)
{
        device_remove_file(led_cdev->dev, &dev_attr_max_brightness);
        device_remove_file(led_cdev->dev, &dev_attr_brightness);
#ifdef CONFIG_LEDS_TRIGGERS
        device_remove_file(led_cdev->dev, &dev_attr_trigger);
        down_write(&led_cdev->trigger_lock);
        if (led_cdev->trigger)
                led_trigger_set(led_cdev, NULL);
        up_write(&led_cdev->trigger_lock);
#endif

        device_unregister(led_cdev->dev);

        down_write(&leds_list_lock);
        list_del(&led_cdev->node);
        up_write(&leds_list_lock);
}

as you can see the led is switched-off right before the device is unregistered.
but rtl8187, p54 & ar9170 led-triggers are timed & asynchronous. So
we really need a cancel_delayed_work_sync after the unregister routine
finished... else the timed trigger might fire when the device/module
is _faded_ from memory.