[PATCH 0/2] Bluetooth: btmrvl: Fix concurrent FW download

[PATCH 0/2] Bluetooth: btmrvl: Fix concurrent FW download

[Andreas Fenkart]

Fixes known bug, when mwifiex / btmrvl are competing for downloading the FW.

http://lists.infradead.org/pipermail/linux-arm-kernel/2012-December/137985.html

Andreas Fenkart (2):
  Bluetooth: btmrv: release lock while waiting for fw download
    complete.
  Bluetooth: btmrvl: report error if verify_fw_download times out.

 drivers/bluetooth/btmrvl_sdio.c |   31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

-- 
1.7.10.4

[PATCH 1/2] Bluetooth: btmrvl: release lock while waiting for fw download complete.

[Andreas Fenkart]

If not winner, driver must release the sdio host lock, so the fw
download can progress. While holding the lock fw download is stalled
and the following error is produced:

[  235.746015] Bluetooth: FW failed to be active in time!
[  235.752799] Bluetooth: Downloading firmware failed!

Signed-off-by: Andreas Fenkart <andreas.fenkart@streamunlimited.com>
---
 drivers/bluetooth/btmrvl_sdio.c |   26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/bluetooth/btmrvl_sdio.c b/drivers/bluetooth/btmrvl_sdio.c
index 9959d4c..6b6abb2 100644
--- a/drivers/bluetooth/btmrvl_sdio.c
+++ b/drivers/bluetooth/btmrvl_sdio.c
@@ -234,7 +234,10 @@ static int btmrvl_sdio_verify_fw_download(struct btmrvl_sdio_card *card,
 
 	 /* Wait for firmware to become ready */
 	for (tries = 0; tries < pollnum; tries++) {
-		if (btmrvl_sdio_read_fw_status(card, &firmwarestat) < 0)
+		sdio_claim_host(card->func);
+		ret = btmrvl_sdio_read_fw_status(card, &firmwarestat);
+		sdio_release_host(card->func);
+		if (ret < 0)
 			continue;
 
 		if (firmwarestat == FIRMWARE_READY) {
@@ -882,19 +885,20 @@ static int btmrvl_sdio_download_fw(struct btmrvl_sdio_card *card)
 		BT_ERR("card or function is NULL!");
 		return -EINVAL;
 	}
-	sdio_claim_host(card->func);
 
 	if (!btmrvl_sdio_verify_fw_download(card, 1)) {
 		BT_DBG("Firmware already downloaded!");
-		goto done;
+		return 0;
 	}
 
+	sdio_claim_host(card->func);
+
 	/* Check if other function driver is downloading the firmware */
 	fws0 = sdio_readb(card->func, card->reg->card_fw_status0, &ret);
 	if (ret) {
 		BT_ERR("Failed to read FW downloading status!");
 		ret = -EIO;
-		goto done;
+		goto release_host;
 	}
 	if (fws0) {
 		BT_DBG("BT not the winner (%#x). Skip FW downloading", fws0);
@@ -907,26 +911,28 @@ static int btmrvl_sdio_download_fw(struct btmrvl_sdio_card *card)
 			if (ret) {
 				BT_ERR("Failed to download helper!");
 				ret = -EIO;
-				goto done;
+				goto release_host;
 			}
 		}
 
 		if (btmrvl_sdio_download_fw_w_helper(card)) {
 			BT_ERR("Failed to download firmware!");
 			ret = -EIO;
-			goto done;
+			goto release_host;
 		}
 	}
 
+	sdio_release_host(card->func);
+
 	if (btmrvl_sdio_verify_fw_download(card, pollnum)) {
 		BT_ERR("FW failed to be active in time!");
-		ret = -ETIMEDOUT;
-		goto done;
+		return -ETIMEDOUT;
 	}
 
-done:
-	sdio_release_host(card->func);
+	return 0;
 
+release_host:
+	sdio_release_host(card->func);
 	return ret;
 }
 
-- 
1.7.10.4

Re: [PATCH 1/2] Bluetooth: btmrvl: release lock while waiting for fw download complete.

[Gustavo Padovan]

Hi Andreas,

* Andreas Fenkart <andreas.fenkart@streamunlimited.com> [2013-04-18 18:18:08 +0200]:

> If not winner, driver must release the sdio host lock, so the fw
> download can progress. While holding the lock fw download is stalled
> and the following error is produced:
> 
> [  235.746015] Bluetooth: FW failed to be active in time!
> [  235.752799] Bluetooth: Downloading firmware failed!
> 
> Signed-off-by: Andreas Fenkart <andreas.fenkart@streamunlimited.com>
> ---
>  drivers/bluetooth/btmrvl_sdio.c |   26 ++++++++++++++++----------
>  1 file changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/bluetooth/btmrvl_sdio.c b/drivers/bluetooth/btmrvl_sdio.c
> index 9959d4c..6b6abb2 100644
> --- a/drivers/bluetooth/btmrvl_sdio.c
> +++ b/drivers/bluetooth/btmrvl_sdio.c
> @@ -234,7 +234,10 @@ static int btmrvl_sdio_verify_fw_download(struct btmrvl_sdio_card *card,
>  
>  	 /* Wait for firmware to become ready */
>  	for (tries = 0; tries < pollnum; tries++) {
> -		if (btmrvl_sdio_read_fw_status(card, &firmwarestat) < 0)
> +		sdio_claim_host(card->func);
> +		ret = btmrvl_sdio_read_fw_status(card, &firmwarestat);
> +		sdio_release_host(card->func);
> +		if (ret < 0)
>  			continue;
>  
>  		if (firmwarestat == FIRMWARE_READY) {
> @@ -882,19 +885,20 @@ static int btmrvl_sdio_download_fw(struct btmrvl_sdio_card *card)
>  		BT_ERR("card or function is NULL!");
>  		return -EINVAL;
>  	}
> -	sdio_claim_host(card->func);
>  
>  	if (!btmrvl_sdio_verify_fw_download(card, 1)) {
>  		BT_DBG("Firmware already downloaded!");
> -		goto done;
> +		return 0;
>  	}
>  
> +	sdio_claim_host(card->func);
> +
>  	/* Check if other function driver is downloading the firmware */
>  	fws0 = sdio_readb(card->func, card->reg->card_fw_status0, &ret);
>  	if (ret) {
>  		BT_ERR("Failed to read FW downloading status!");
>  		ret = -EIO;
> -		goto done;
> +		goto release_host;

No need to rename this label, it is doing almost the same thing as before.

	Gustavo

[PATCH 2/2] Bluetooth: btmrvl: report error if verify_fw_download times out.

[Andreas Fenkart]

Fw does the synchronization of the different modules during init.
It will report different modules, that it is ready at different times.
The download 'winner' will be reported 'fw ready' first. Without this
patch, btmrvl is continuing before the fw tells it to. Probably on behalf
of the 'winner' which then never sees the fw as ready and times out.

Signed-off-by: Andreas Fenkart <andreas.fenkart@streamunlimited.com>
---
 drivers/bluetooth/btmrvl_sdio.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/bluetooth/btmrvl_sdio.c b/drivers/bluetooth/btmrvl_sdio.c
index 6b6abb2..2ec0841 100644
--- a/drivers/bluetooth/btmrvl_sdio.c
+++ b/drivers/bluetooth/btmrvl_sdio.c
@@ -245,6 +245,7 @@ static int btmrvl_sdio_verify_fw_download(struct btmrvl_sdio_card *card,
 			break;
 		} else {
 			msleep(10);
+			ret = -1;
 		}
 	}
 
@@ -924,6 +925,10 @@ static int btmrvl_sdio_download_fw(struct btmrvl_sdio_card *card)
 
 	sdio_release_host(card->func);
 
+	/*
+	 * winner or not, with this test the FW synchronizes when the
+	 * module can continue its initialization
+	 */
 	if (btmrvl_sdio_verify_fw_download(card, pollnum)) {
 		BT_ERR("FW failed to be active in time!");
 		return -ETIMEDOUT;
-- 
1.7.10.4

Re: [PATCH 2/2] Bluetooth: btmrvl: report error if verify_fw_download times out.

[Gustavo Padovan]

Hi Andreas,

* Andreas Fenkart <andreas.fenkart@streamunlimited.com> [2013-04-18 18:18:09 +0200]:

> Fw does the synchronization of the different modules during init.
> It will report different modules, that it is ready at different times.
> The download 'winner' will be reported 'fw ready' first. Without this
> patch, btmrvl is continuing before the fw tells it to. Probably on behalf
> of the 'winner' which then never sees the fw as ready and times out.
> 
> Signed-off-by: Andreas Fenkart <andreas.fenkart@streamunlimited.com>
> ---
>  drivers/bluetooth/btmrvl_sdio.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/bluetooth/btmrvl_sdio.c b/drivers/bluetooth/btmrvl_sdio.c
> index 6b6abb2..2ec0841 100644
> --- a/drivers/bluetooth/btmrvl_sdio.c
> +++ b/drivers/bluetooth/btmrvl_sdio.c
> @@ -245,6 +245,7 @@ static int btmrvl_sdio_verify_fw_download(struct btmrvl_sdio_card *card,
>  			break;
>  		} else {
>  			msleep(10);
> +			ret = -1;

Can we have a proper error code here?

	Gustavo

[PATCH 0/2] Bluetooth: btmrvl: Fix concurrent FW download

[Andreas Fenkart]

Fixes known bug, when mwifiex / btmrvl are competing for downloading the FW.

http://lists.infradead.org/pipermail/linux-arm-kernel/2012-December/137985.html

Andreas Fenkart (2):
  Bluetooth: btmrv: release lock while waiting for fw download
    complete.
  Bluetooth: btmrvl: report error if verify_fw_download times out.

 drivers/bluetooth/btmrvl_sdio.c |   31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

-- 
1.7.10.4

[PATCH v2 0/2] Bluetooth: btmrvl: Fix concurrent FW download

[Andreas Fenkart]

Resend. Addresses issues raised by Gustavo.

Andreas Fenkart (2):
  Bluetooth: btmrv: release lock while waiting for fw download
    complete.
  Bluetooth: btmrvl: report error if verify_fw_download times out.

 drivers/bluetooth/btmrvl_sdio.c |   39 ++++++++++++++++++++++-----------------
 1 file changed, 22 insertions(+), 17 deletions(-)

-- 
1.7.10.4

[PATCH v2 1/2] Bluetooth: btmrv: release lock while waiting for fw download complete.

[Andreas Fenkart]

If not winner, driver must release the sdio host lock, so the fw
download can progress. While holding the lock fw download is stalled
and the following error is produced:

[  235.746015] Bluetooth: FW failed to be active in time!
[  235.752799] Bluetooth: Downloading firmware failed!

Signed-off-by: Andreas Fenkart <andreas.fenkart@streamunlimited.com>

diff --git a/drivers/bluetooth/btmrvl_sdio.c b/drivers/bluetooth/btmrvl_sdio.c
index 9959d4c..c7ec727 100644
--- a/drivers/bluetooth/btmrvl_sdio.c
+++ b/drivers/bluetooth/btmrvl_sdio.c
@@ -234,7 +234,10 @@ static int btmrvl_sdio_verify_fw_download(struct btmrvl_sdio_card *card,
 
 	 /* Wait for firmware to become ready */
 	for (tries = 0; tries < pollnum; tries++) {
-		if (btmrvl_sdio_read_fw_status(card, &firmwarestat) < 0)
+		sdio_claim_host(card->func);
+		ret = btmrvl_sdio_read_fw_status(card, &firmwarestat);
+		sdio_release_host(card->func);
+		if (ret < 0)
 			continue;
 
 		if (firmwarestat == FIRMWARE_READY) {
@@ -882,13 +885,14 @@ static int btmrvl_sdio_download_fw(struct btmrvl_sdio_card *card)
 		BT_ERR("card or function is NULL!");
 		return -EINVAL;
 	}
-	sdio_claim_host(card->func);
 
 	if (!btmrvl_sdio_verify_fw_download(card, 1)) {
 		BT_DBG("Firmware already downloaded!");
-		goto done;
+		return 0;
 	}
 
+	sdio_claim_host(card->func);
+
 	/* Check if other function driver is downloading the firmware */
 	fws0 = sdio_readb(card->func, card->reg->card_fw_status0, &ret);
 	if (ret) {
@@ -918,15 +922,17 @@ static int btmrvl_sdio_download_fw(struct btmrvl_sdio_card *card)
 		}
 	}
 
+	sdio_release_host(card->func);
+
 	if (btmrvl_sdio_verify_fw_download(card, pollnum)) {
 		BT_ERR("FW failed to be active in time!");
-		ret = -ETIMEDOUT;
-		goto done;
+		return -ETIMEDOUT;
 	}
 
+	return 0;
+
 done:
 	sdio_release_host(card->func);
-
 	return ret;
 }
 
-- 
1.7.10.4

[PATCH v2 2/2] Bluetooth: btmrvl: report error if verify_fw_download times out.

[Andreas Fenkart]

FW does the synchronization of the different modules during init.
It will report different modules, that it is ready at different times.
The fw download 'winner' will be reported fw ready first. Without this
patch, btmrvl was already continuing before the FW told it too. Probably
on behalf of the 'winner' which then never sees FW ready and times out.

Signed-off-by: Andreas Fenkart <andreas.fenkart@streamunlimited.com>

diff --git a/drivers/bluetooth/btmrvl_sdio.c b/drivers/bluetooth/btmrvl_sdio.c
index c7ec727..3d8305c 100644
--- a/drivers/bluetooth/btmrvl_sdio.c
+++ b/drivers/bluetooth/btmrvl_sdio.c
@@ -228,9 +228,8 @@ failed:
 static int btmrvl_sdio_verify_fw_download(struct btmrvl_sdio_card *card,
 								int pollnum)
 {
-	int ret = -ETIMEDOUT;
 	u16 firmwarestat;
-	unsigned int tries;
+	int tries, ret;
 
 	 /* Wait for firmware to become ready */
 	for (tries = 0; tries < pollnum; tries++) {
@@ -240,15 +239,13 @@ static int btmrvl_sdio_verify_fw_download(struct btmrvl_sdio_card *card,
 		if (ret < 0)
 			continue;
 
-		if (firmwarestat == FIRMWARE_READY) {
-			ret = 0;
-			break;
-		} else {
-			msleep(10);
-		}
+		if (firmwarestat == FIRMWARE_READY)
+			return 0;
+
+		msleep(10);
 	}
 
-	return ret;
+	return -ETIMEDOUT;
 }
 
 static int btmrvl_sdio_download_helper(struct btmrvl_sdio_card *card)
@@ -924,6 +921,10 @@ static int btmrvl_sdio_download_fw(struct btmrvl_sdio_card *card)
 
 	sdio_release_host(card->func);
 
+	/*
+	 * winner or not, with this test the FW synchronizes when the
+	 * module can continue its initialization
+	 */
 	if (btmrvl_sdio_verify_fw_download(card, pollnum)) {
 		BT_ERR("FW failed to be active in time!");
 		return -ETIMEDOUT;
@@ -995,8 +996,6 @@ static int btmrvl_sdio_probe(struct sdio_func *func,
 		goto unreg_dev;
 	}
 
-	msleep(100);
-
 	btmrvl_sdio_enable_host_int(card);
 
 	priv = btmrvl_add_card(card);
-- 
1.7.10.4