[PATCH v2] mwifiex: fix race condition when downloading firmware

[PATCH v2] mwifiex: fix race condition when downloading firmware

[Wei-Ning Huang]

The action 'check for winner' and 'download firmware' should be an
atomic action. This is true for btmrvl driver but not mwmfiex, which
cause firmware download to fail when the following scenario happens:

1) mwifiex check winner status: true
2) btmrvl check winner status: true, and start downloading firmware
3) mwifiex tries to download firmware, but failed because btmrvl is
already downloading.

This won't happen if 1) and 3) is an atomic action. This patch adds
sdio_claim/release_host call around those two actions to make sure it's
atomic.

Signed-off-by: Wei-Ning Huang <wnhuang@chromium.org>
---
 drivers/net/wireless/marvell/mwifiex/init.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/init.c b/drivers/net/wireless/marvell/mwifiex/init.c
index 78c532f..7703438 100644
--- a/drivers/net/wireless/marvell/mwifiex/init.c
+++ b/drivers/net/wireless/marvell/mwifiex/init.c
@@ -24,6 +24,7 @@
 #include "main.h"
 #include "wmm.h"
 #include "11n.h"
+#include "sdio.h"
 
 /*
  * This function adds a BSS priority table to the table list.
@@ -737,16 +738,20 @@ mwifiex_shutdown_drv(struct mwifiex_adapter *adapter)
 int mwifiex_dnld_fw(struct mwifiex_adapter *adapter,
 		    struct mwifiex_fw_image *pmfw)
 {
+	struct sdio_mmc_card *card = adapter->card;
 	int ret;
 	u32 poll_num = 1;
 
+	if (adapter->iface_type == MWIFIEX_SDIO)
+		sdio_claim_host(card->func);
+
 	if (adapter->if_ops.check_fw_status) {
 		/* check if firmware is already running */
 		ret = adapter->if_ops.check_fw_status(adapter, poll_num);
 		if (!ret) {
 			mwifiex_dbg(adapter, MSG,
 				    "WLAN FW already running! Skip FW dnld\n");
-			return 0;
+			goto release_host;
 		}
 	}
 
@@ -759,7 +764,7 @@ int mwifiex_dnld_fw(struct mwifiex_adapter *adapter,
 		if (ret) {
 			mwifiex_dbg(adapter, MSG,
 				    "WLAN read winner status failed!\n");
-			return ret;
+			goto release_host;
 		}
 
 		if (!adapter->winner) {
@@ -775,7 +780,7 @@ int mwifiex_dnld_fw(struct mwifiex_adapter *adapter,
 		if (ret) {
 			mwifiex_dbg(adapter, ERROR,
 				    "prog_fw failed ret=%#x\n", ret);
-			return ret;
+			goto release_host;
 		}
 	}
 
@@ -785,6 +790,9 @@ poll_fw:
 	if (ret)
 		mwifiex_dbg(adapter, ERROR,
 			    "FW failed to be active in time\n");
+release_host:
+	if (adapter->iface_type == MWIFIEX_SDIO)
+		sdio_release_host(card->func);
 
 	return ret;
 }
-- 
2.1.2

RE: [PATCH v2] mwifiex: fix race condition when downloading firmware

[Amitkumar Karwar]

> From: Wei-Ning Huang [mailto:wnhuang@chromium.org]
> Sent: Friday, May 27, 2016 5:58 PM
> To: Linux-Wireless
> Cc: LKML; Amitkumar Karwar; djkurtz@chromium.org; Wei-Ning Huang
> Subject: [PATCH v2] mwifiex: fix race condition when downloading
> firmware
> 
> The action 'check for winner' and 'download firmware' should be an
> atomic action. This is true for btmrvl driver but not mwmfiex, which
> cause firmware download to fail when the following scenario happens:
> 
> 1) mwifiex check winner status: true
> 2) btmrvl check winner status: true, and start downloading firmware
> 3) mwifiex tries to download firmware, but failed because btmrvl is
> already downloading.
> 
> This won't happen if 1) and 3) is an atomic action. This patch adds
> sdio_claim/release_host call around those two actions to make sure it's
> atomic.
> 
> Signed-off-by: Wei-Ning Huang <wnhuang@chromium.org>
> ---
>  drivers/net/wireless/marvell/mwifiex/init.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/init.c
> b/drivers/net/wireless/marvell/mwifiex/init.c
> index 78c532f..7703438 100644
> --- a/drivers/net/wireless/marvell/mwifiex/init.c
> +++ b/drivers/net/wireless/marvell/mwifiex/init.c
> @@ -24,6 +24,7 @@
>  #include "main.h"
>  #include "wmm.h"
>  #include "11n.h"
> +#include "sdio.h"
> 
>  /*
>   * This function adds a BSS priority table to the table list.
> @@ -737,16 +738,20 @@ mwifiex_shutdown_drv(struct mwifiex_adapter
> *adapter)  int mwifiex_dnld_fw(struct mwifiex_adapter *adapter,
>  		    struct mwifiex_fw_image *pmfw)
>  {
> +	struct sdio_mmc_card *card = adapter->card;
>  	int ret;
>  	u32 poll_num = 1;
> 
> +	if (adapter->iface_type == MWIFIEX_SDIO)
> +		sdio_claim_host(card->func);
> +
>  	if (adapter->if_ops.check_fw_status) {
>  		/* check if firmware is already running */
>  		ret = adapter->if_ops.check_fw_status(adapter, poll_num);
>  		if (!ret) {
>  			mwifiex_dbg(adapter, MSG,
>  				    "WLAN FW already running! Skip FW dnld\n");
> -			return 0;
> +			goto release_host;
>  		}
>  	}
> 
> @@ -759,7 +764,7 @@ int mwifiex_dnld_fw(struct mwifiex_adapter *adapter,
>  		if (ret) {
>  			mwifiex_dbg(adapter, MSG,
>  				    "WLAN read winner status failed!\n");
> -			return ret;
> +			goto release_host;
>  		}
> 
>  		if (!adapter->winner) {
> @@ -775,7 +780,7 @@ int mwifiex_dnld_fw(struct mwifiex_adapter *adapter,
>  		if (ret) {
>  			mwifiex_dbg(adapter, ERROR,
>  				    "prog_fw failed ret=%#x\n", ret);
> -			return ret;
> +			goto release_host;
>  		}
>  	}
> 
> @@ -785,6 +790,9 @@ poll_fw:
>  	if (ret)
>  		mwifiex_dbg(adapter, ERROR,
>  			    "FW failed to be active in time\n");
> +release_host:
> +	if (adapter->iface_type == MWIFIEX_SDIO)
> +		sdio_release_host(card->func);
> 
>  	return ret;
>  }
> --
> 2.1.2

Patch looks fine to me.

Acked-by: Amitkumar Karwar <akarwar@marvell.com>

Regards,
Amitkumar

Re: [PATCH v2] mwifiex: fix race condition when downloading firmware

[Kalle Valo]

Wei-Ning Huang <wnhuang@chromium.org> writes:

> The action 'check for winner' and 'download firmware' should be an
> atomic action. This is true for btmrvl driver but not mwmfiex, which
> cause firmware download to fail when the following scenario happens:
>
> 1) mwifiex check winner status: true
> 2) btmrvl check winner status: true, and start downloading firmware
> 3) mwifiex tries to download firmware, but failed because btmrvl is
> already downloading.
>
> This won't happen if 1) and 3) is an atomic action. This patch adds
> sdio_claim/release_host call around those two actions to make sure it's
> atomic.
>
> Signed-off-by: Wei-Ning Huang <wnhuang@chromium.org>

As I accidentally applied v1, I need to drop this one.

-- 
Kalle Valo