[PATCH 1/2] Revert "mwifiex: handle race during mwifiex_usb_disconnect"

[PATCH 1/2] Revert "mwifiex: handle race during mwifiex_usb_disconnect"

[Ganapathi Bhat]

This reverts commit b817047ae70c0bd67b677b65d0d69d72cd6e9728.

We have a better fix for this issue, which will be sent on top
of this revert.

Signed-off-by: Ganapathi Bhat <gbhat@marvell.com>
---
 drivers/net/wireless/marvell/mwifiex/usb.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/usb.c b/drivers/net/wireless/marvell/mwifiex/usb.c
index 6e3cf98..bc475b8 100644
--- a/drivers/net/wireless/marvell/mwifiex/usb.c
+++ b/drivers/net/wireless/marvell/mwifiex/usb.c
@@ -644,9 +644,6 @@ static void mwifiex_usb_disconnect(struct usb_interface *intf)
 					 MWIFIEX_FUNC_SHUTDOWN);
 	}
 
-	if (adapter->workqueue)
-		flush_workqueue(adapter->workqueue);
-
 	mwifiex_usb_free(card);
 
 	mwifiex_dbg(adapter, FATAL,
-- 
1.9.1

[PATCH 2/2] mwifiex: handle race during mwifiex_usb_disconnect

[Ganapathi Bhat]

Race condition is observed during rmmod of mwifiex_usb:

1. The rmmod thread will call mwifiex_usb_disconnect(), download
   SHUTDOWN command and do wait_event_interruptible_timeout(),
   waiting for response.

2. The main thread will handle the response and will do a
   wake_up_interruptible(), unblocking rmmod thread.

3. On getting unblocked, rmmod thread  will make rx_cmd.urb = NULL in
   mwifiex_usb_free().

4. The main thread will try to resubmit rx_cmd.urb in
   mwifiex_usb_submit_rx_urb(), which is NULL.

To fix this, move mwifiex_usb_free() from mwifiex_usb_disconnect
to mwifiex_unregister_dev(). Function mwifiex_unregister_dev() is
called after flushing the command and RX work queues.

Signed-off-by: Brian Norris <briannorris@chromium.org>
Signed-off-by: Ganapathi Bhat <gbhat@marvell.com>
---
 drivers/net/wireless/marvell/mwifiex/usb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/usb.c b/drivers/net/wireless/marvell/mwifiex/usb.c
index bc475b8..88f4c89 100644
--- a/drivers/net/wireless/marvell/mwifiex/usb.c
+++ b/drivers/net/wireless/marvell/mwifiex/usb.c
@@ -644,8 +644,6 @@ static void mwifiex_usb_disconnect(struct usb_interface *intf)
 					 MWIFIEX_FUNC_SHUTDOWN);
 	}
 
-	mwifiex_usb_free(card);
-
 	mwifiex_dbg(adapter, FATAL,
 		    "%s: removing card\n", __func__);
 	mwifiex_remove_card(adapter);
@@ -1353,6 +1351,8 @@ static void mwifiex_unregister_dev(struct mwifiex_adapter *adapter)
 {
 	struct usb_card_rec *card = (struct usb_card_rec *)adapter->card;
 
+	mwifiex_usb_free(card);
+
 	mwifiex_usb_cleanup_tx_aggr(adapter);
 
 	card->adapter = NULL;
-- 
1.9.1

Re: [PATCH 1/2] Revert "mwifiex: handle race during mwifiex_usb_disconnect"

[Kalle Valo]

Ganapathi Bhat <gbhat@marvell.com> writes:

> This reverts commit b817047ae70c0bd67b677b65d0d69d72cd6e9728.
>
> We have a better fix for this issue, which will be sent on top
> of this revert.
>
> Signed-off-by: Ganapathi Bhat <gbhat@marvell.com>

I'll queue both of these patches to 4.18.

-- 
Kalle Valo

Re: [PATCH 2/2] mwifiex: handle race during mwifiex_usb_disconnect

[Brian Norris]

Hi Ganapathi,

On Fri, Jun 01, 2018 at 04:11:20PM +0530, Ganapathi Bhat wrote:
> Race condition is observed during rmmod of mwifiex_usb:
> 
> 1. The rmmod thread will call mwifiex_usb_disconnect(), download
>    SHUTDOWN command and do wait_event_interruptible_timeout(),
>    waiting for response.
> 
> 2. The main thread will handle the response and will do a
>    wake_up_interruptible(), unblocking rmmod thread.
> 
> 3. On getting unblocked, rmmod thread  will make rx_cmd.urb = NULL in
>    mwifiex_usb_free().
> 
> 4. The main thread will try to resubmit rx_cmd.urb in
>    mwifiex_usb_submit_rx_urb(), which is NULL.
> 
> To fix this, move mwifiex_usb_free() from mwifiex_usb_disconnect
> to mwifiex_unregister_dev(). Function mwifiex_unregister_dev() is
> called after flushing the command and RX work queues.
> 
> Signed-off-by: Brian Norris <briannorris@chromium.org>

^^ I'm not sure if that line is quite accurate. While I nearly spelled
out what the patch would look like, you wrote it.

Anyway, patch seems good to me, assuming it tests out OK for you:

Reviewed-by: Brian Norris <briannorris@chromium.org>

and if Kalle hasn't applied this yet, an alternative to Signed-off-by:

Suggested-by: Brian Norris <briannorris@chromium.org>

> Signed-off-by: Ganapathi Bhat <gbhat@marvell.com>
> ---
>  drivers/net/wireless/marvell/mwifiex/usb.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/usb.c b/drivers/net/wireless/marvell/mwifiex/usb.c
> index bc475b8..88f4c89 100644
> --- a/drivers/net/wireless/marvell/mwifiex/usb.c
> +++ b/drivers/net/wireless/marvell/mwifiex/usb.c
> @@ -644,8 +644,6 @@ static void mwifiex_usb_disconnect(struct usb_interface *intf)
>  					 MWIFIEX_FUNC_SHUTDOWN);
>  	}
>  
> -	mwifiex_usb_free(card);
> -
>  	mwifiex_dbg(adapter, FATAL,
>  		    "%s: removing card\n", __func__);
>  	mwifiex_remove_card(adapter);
> @@ -1353,6 +1351,8 @@ static void mwifiex_unregister_dev(struct mwifiex_adapter *adapter)
>  {
>  	struct usb_card_rec *card = (struct usb_card_rec *)adapter->card;
>  
> +	mwifiex_usb_free(card);
> +
>  	mwifiex_usb_cleanup_tx_aggr(adapter);
>  
>  	card->adapter = NULL;
> -- 
> 1.9.1
> 

Re: [PATCH 2/2] mwifiex: handle race during mwifiex_usb_disconnect

[Kalle Valo]

Brian Norris <briannorris@chromium.org> writes:

> Hi Ganapathi,
>
> On Fri, Jun 01, 2018 at 04:11:20PM +0530, Ganapathi Bhat wrote:
>> Race condition is observed during rmmod of mwifiex_usb:
>> 
>> 1. The rmmod thread will call mwifiex_usb_disconnect(), download
>>    SHUTDOWN command and do wait_event_interruptible_timeout(),
>>    waiting for response.
>> 
>> 2. The main thread will handle the response and will do a
>>    wake_up_interruptible(), unblocking rmmod thread.
>> 
>> 3. On getting unblocked, rmmod thread  will make rx_cmd.urb = NULL in
>>    mwifiex_usb_free().
>> 
>> 4. The main thread will try to resubmit rx_cmd.urb in
>>    mwifiex_usb_submit_rx_urb(), which is NULL.
>> 
>> To fix this, move mwifiex_usb_free() from mwifiex_usb_disconnect
>> to mwifiex_unregister_dev(). Function mwifiex_unregister_dev() is
>> called after flushing the command and RX work queues.
>> 
>> Signed-off-by: Brian Norris <briannorris@chromium.org>
>
> ^^ I'm not sure if that line is quite accurate. While I nearly spelled
> out what the patch would look like, you wrote it.
>
> Anyway, patch seems good to me, assuming it tests out OK for you:
>
> Reviewed-by: Brian Norris <briannorris@chromium.org>
>
> and if Kalle hasn't applied this yet, an alternative to Signed-off-by:
>
> Suggested-by: Brian Norris <briannorris@chromium.org>

Ok, I'll change that.

-- 
Kalle Valo

Re: [1/2] Revert "mwifiex: handle race during mwifiex_usb_disconnect"

[Kalle Valo]

Ganapathi Bhat <gbhat@marvell.com> wrote:

> This reverts commit b817047ae70c0bd67b677b65d0d69d72cd6e9728.
> 
> We have a better fix for this issue, which will be sent on top
> of this revert.
> 
> Signed-off-by: Ganapathi Bhat <gbhat@marvell.com>

2 patches applied to wireless-drivers.git, thanks.

7e58e741c001 Revert "mwifiex: handle race during mwifiex_usb_disconnect"
f8c095f67962 mwifiex: handle race during mwifiex_usb_disconnect

-- 
https://patchwork.kernel.org/patch/10442949/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches