[PATCH] at76_usb: fix Oops on disconnect

[PATCH] at76_usb: fix Oops on disconnect

[Jason Andryuk]

flush_workqueue needs to be called instead of the generic one and the
associated functions need to be modified to prevent re-adding
themselves to the workqueue.

Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
--

diff --git a/drivers/staging/at76_usb/at76_usb.c
b/drivers/staging/at76_usb/at76_usb.c
index 185533e..211a6dd 100644
--- a/drivers/staging/at76_usb/at76_usb.c
+++ b/drivers/staging/at76_usb/at76_usb.c
@@ -1967,6 +1967,9 @@ static void at76_dwork_hw_scan(struct work_struct *work)
 					      dwork_hw_scan.work);
 	int ret;

+	if (priv->device_unplugged)
+		return;
+
 	ret = at76_get_cmd_status(priv->udev, CMD_SCAN);
 	at76_dbg(DBG_MAC80211, "%s: CMD_SCAN status 0x%02x", __func__, ret);

@@ -2000,6 +2003,9 @@ static int at76_hw_scan(struct ieee80211_hw *hw,
u8 *ssid, size_t len)
 	at76_dbg(DBG_MAC80211, "%s():", __func__);
 	at76_dbg_dump(DBG_MAC80211, ssid, len, "ssid %zd bytes:", len);

+	if (priv->device_unplugged)
+		return 0;
+
 	mutex_lock(&priv->mtx);

 	ieee80211_stop_queues(hw);
@@ -2102,6 +2108,12 @@ static void at76_configure_filter(struct
ieee80211_hw *hw,
 	flags = changed_flags & AT76_SUPPORTED_FILTERS;
 	*total_flags = AT76_SUPPORTED_FILTERS;

+	/* We bail out of the function to prevent work from being queued
+	 * in the case when the device is no longer present.  We do so
+	 * after updating total_flags to avoid a WARN_ON in mac80211. */
+	if (priv->device_unplugged)
+		return;
+
 	/* FIXME: access to priv->promisc should be protected with
 	 * priv->mtx, but it's impossible because this function needs to be
 	 * atomic */
@@ -2511,6 +2523,8 @@ static int at76_init_new_device(struct at76_priv *priv,
 	/* mac80211 initialisation */
 	priv->hw->wiphy->bands[IEEE80211_BAND_2GHZ] = &at76_supported_band;

+	priv->device_unplugged = 0;
+
 	if (FIRMWARE_IS_WPA(priv->fw_version) &&
 		(at76_is_503rfmd(priv->board_type) ||
 		 at76_is_505(priv->board_type)))
@@ -2561,13 +2575,11 @@ static void at76_delete_device(struct at76_priv *priv)
 	/* The device is gone, don't bother turning it off */
 	priv->device_unplugged = 1;

-	if (priv->mac80211_registered)
-		ieee80211_unregister_hw(priv->hw);
-
-	/* assuming we used keventd, it must quiesce too */
-	flush_scheduled_work();
+	if (priv->mac80211_registered) {
+		flush_workqueue(priv->hw->workqueue);

-	kfree(priv->bulk_out_buffer);
+		ieee80211_unregister_hw(priv->hw);
+	}

 	if (priv->tx_urb) {
 		usb_kill_urb(priv->tx_urb);
@@ -2580,8 +2592,12 @@ static void at76_delete_device(struct at76_priv *priv)

 	at76_dbg(DBG_PROC_ENTRY, "%s: unlinked urbs", __func__);

-	if (priv->rx_skb)
+	/* move me after urb unlink? */
+	kfree(priv->bulk_out_buffer);
+
+	if (priv->rx_skb) {
 		kfree_skb(priv->rx_skb);
+	}

 	usb_put_dev(priv->udev);

Re: [PATCH] at76_usb: fix Oops on disconnect

[Kalle Valo]

On Thu, Feb 5, 2009 at 6:03 AM, Jason Andryuk <jandryuk@gmail.com> wrote:
> flush_workqueue needs to be called instead of the generic one and the
> associated functions need to be modified to prevent re-adding
> themselves to the workqueue.

Good fix. Can you please port this to the at76c50x-usb driver I posted
this morning?

Kalle

Re: [PATCH] at76_usb: fix Oops on disconnect

[Jason Andryuk]

I'll take a look at at76c50x-usb in general and bringing over this
patch specifically.  Unfortunately, I still had one disconnect oops
with the patch installed.  Netconsole was not configured, so I did not
catch it.  There were several other successful disconnects without
Oops though.

It may need locking as none is currently present, but does a one way
transition device_unplugged 0 -> 1 need a lock?

Jason

On Sat, Feb 7, 2009 at 6:29 AM, Kalle Valo <kalle.valo@iki.fi> wrote:
> On Thu, Feb 5, 2009 at 6:03 AM, Jason Andryuk <jandryuk@gmail.com> wrote:
>> flush_workqueue needs to be called instead of the generic one and the
>> associated functions need to be modified to prevent re-adding
>> themselves to the workqueue.
>
> Good fix. Can you please port this to the at76c50x-usb driver I posted
> this morning?
>
> Kalle
>

Re: [PATCH] at76_usb: fix Oops on disconnect

[Kalle Valo]

Jason Andryuk <jandryuk@gmail.com> writes:

> I'll take a look at at76c50x-usb in general and bringing over this
> patch specifically.  Unfortunately, I still had one disconnect oops
> with the patch installed.  Netconsole was not configured, so I did not
> catch it.  There were several other successful disconnects without
> Oops though.

Progress is always good :)

> It may need locking as none is currently present, but does a one way
> transition device_unplugged 0 -> 1 need a lock?

Using int might be enough but I think it's not guarenteed anywhere.
Better to use atomic operations directly or use a proper lock. But I
think the issues are more than that. For example, the disconnect
handler is not taking the mutex.

-- 
Kalle Valo

[PATCH] at76c50x-usb: additional disconnect fixes

[Jason Andryuk]

Additional attempts to fix Oops on disconnect, that appear to be successful.  However, 
some may be extraneous.

The cancel_delayed_work call is probably the most necessary.  The device_unplugged
check may not be necessary.  del_timer_sync may not be necessary either, but the Oops 
I was receiving was related to timers.  Hence the addition.

Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
--

diff --git a/drivers/net/wireless/at76c50x-usb.c b/drivers/net/wireless/at76c50x-usb.c
index 7a03251..52b0be5 100644
--- a/drivers/net/wireless/at76c50x-usb.c
+++ b/drivers/net/wireless/at76c50x-usb.c
@@ -1497,6 +1497,9 @@ static void at76_work_set_promisc(struct work_struct *work)
 					      work_set_promisc);
 	int ret = 0;
 
+	if (priv->device_unplugged)
+		return;
+
 	mutex_lock(&priv->mtx);
 
 	priv->mib_buf.type = MIB_LOCAL;
@@ -2297,6 +2300,7 @@ static void at76_delete_device(struct at76_priv *priv)
 	tasklet_kill(&priv->rx_tasklet);
 
 	if (priv->mac80211_registered) {
+		cancel_delayed_work(&priv->dwork_hw_scan);
 		flush_workqueue(priv->hw->workqueue);
 		ieee80211_unregister_hw(priv->hw);
 	}
@@ -2314,6 +2318,8 @@ static void at76_delete_device(struct at76_priv *priv)
 
 	kfree(priv->bulk_out_buffer);
 
+	del_timer_sync(&ledtrig_tx_timer);
+
 	if (priv->rx_skb)
 		kfree_skb(priv->rx_skb);
 

Re: [PATCH] at76c50x-usb: additional disconnect fixes

[Kalle Valo]

Jason Andryuk <jandryuk@gmail.com> writes:

> Additional attempts to fix Oops on disconnect, that appear to be
> successful. However, some may be extraneous.
>
> The cancel_delayed_work call is probably the most necessary. The
> device_unplugged check may not be necessary. del_timer_sync may not be
> necessary either, but the Oops I was receiving was related to timers.
> Hence the addition.

Thanks, looks good. I'll take this and do similarly as with the
previous patches.

-- 
Kalle Valo