[PATCH] brcmfmac: fix CLM load error for legacy chips when user helper is enabled

[PATCH] brcmfmac: fix CLM load error for legacy chips when user helper is enabled

[Wright Feng]

For legacy chips w/o CLM blob files, kernel with user helper function
enables returns -EAGAIN when we request_firmware() for blob file:
"request_firmware() -> _request_firmware() -> fw_load_from_user_helper()
-> _request_firmware_load() -> retval=-EAGAIN"
We should do one more retry and continue brcmf_c_process_clm_blob if
getting -EAGAIN from request_firmware function.

Signed-off-by: Wright Feng <wright.feng@cypress.com>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
index 6a59d06..56e2654 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
@@ -45,6 +45,8 @@
 
 #define BRCMF_DEFAULT_TXGLOM_SIZE	32  /* max tx frames in glom chain */
 
+#define CLM_LOAD_RETRIES 1 /* # of retries to load clm_blob file */
+
 static int brcmf_sdiod_txglomsz = BRCMF_DEFAULT_TXGLOM_SIZE;
 module_param_named(txglomsz, brcmf_sdiod_txglomsz, int, 0);
 MODULE_PARM_DESC(txglomsz, "Maximum tx packet chain size [SDIO]");
@@ -170,6 +172,7 @@ static int brcmf_c_process_clm_blob(struct brcmf_if *ifp)
 	u16 dl_flag = DL_BEGIN;
 	u32 status;
 	s32 err;
+	uint retries = 0;
 
 	brcmf_dbg(TRACE, "Enter\n");
 
@@ -180,11 +183,18 @@ static int brcmf_c_process_clm_blob(struct brcmf_if *ifp)
 		return err;
 	}
 
-	err = request_firmware(&clm, clm_name, dev);
+	do {
+		err = request_firmware(&clm, clm_name, dev);
+	} while (err == -EAGAIN && retries++ < CLM_LOAD_RETRIES);
 	if (err) {
 		if (err == -ENOENT) {
 			brcmf_dbg(INFO, "continue with CLM data currently present in firmware\n");
 			return 0;
+		} else if (err == -EAGAIN) {
+			brcmf_dbg(INFO, "reached maximum retries(%d)\n",
+				  CLM_LOAD_RETRIES);
+			brcmf_dbg(INFO, "continue with CLM data in firmware\n");
+			return 0;
 		}
 		brcmf_err("request CLM blob file failed (%d)\n", err);
 		return err;
-- 
1.9.1

Re: [PATCH] brcmfmac: fix CLM load error for legacy chips when user helper is enabled

[Kalle Valo]

Wright Feng <wright.feng@cypress.com> writes:

> For legacy chips w/o CLM blob files, kernel with user helper function
> enables returns -EAGAIN when we request_firmware() for blob file:
> "request_firmware() -> _request_firmware() -> fw_load_from_user_helper()
> -> _request_firmware_load() -> retval=-EAGAIN"
> We should do one more retry

Who says that we should do one more retry?

> and continue brcmf_c_process_clm_blob if getting -EAGAIN from
> request_firmware function.
>
> Signed-off-by: Wright Feng <wright.feng@cypress.com>

[...]

> @@ -180,11 +183,18 @@ static int brcmf_c_process_clm_blob(struct brcmf_if *ifp)
>  		return err;
>  	}
>  
> -	err = request_firmware(&clm, clm_name, dev);
> +	do {
> +		err = request_firmware(&clm, clm_name, dev);
> +	} while (err == -EAGAIN && retries++ < CLM_LOAD_RETRIES);

This looks like a REALLY ugly workaround, please think three times
before submitting something like this. And if you still decide to submit
it, put a good effort on the commit log to explain why the hack would be
acceptable.

Have you investigated why you are getting the -EGAIN, user space not
ready during boot or something like that?

-- 
Kalle Valo

Re: [PATCH] brcmfmac: fix CLM load error for legacy chips when user helper is enabled

[Wright Feng]

Hi Kalle,

On 2017/12/12 下午 08:57, Kalle Valo wrote:
> Wright Feng <wright.feng@cypress.com> writes:
> 
>> For legacy chips w/o CLM blob files, kernel with user helper function
>> enables returns -EAGAIN when we request_firmware() for blob file:
>> "request_firmware() -> _request_firmware() -> fw_load_from_user_helper()
>> -> _request_firmware_load() -> retval=-EAGAIN"
>> We should do one more retry
> 
> Who says that we should do one more retry?
> 
>> and continue brcmf_c_process_clm_blob if getting -EAGAIN from
>> request_firmware function.
>>
>> Signed-off-by: Wright Feng <wright.feng@cypress.com>
> 
> [...]
> 
>> @@ -180,11 +183,18 @@ static int brcmf_c_process_clm_blob(struct brcmf_if *ifp)
>>   		return err;
>>   	}
>>   
>> -	err = request_firmware(&clm, clm_name, dev);
>> +	do {
>> +		err = request_firmware(&clm, clm_name, dev);
>> +	} while (err == -EAGAIN && retries++ < CLM_LOAD_RETRIES);
> 
> This looks like a REALLY ugly workaround, please think three times
> before submitting something like this. And if you still decide to submit
> it, put a good effort on the commit log to explain why the hack would be
> acceptable.
> 
> Have you investigated why you are getting the -EGAIN, user space not
> ready during boot or something like that?
I didn't notice below commit 76098b36b5db ("firmware: send -EINTR on 
signal abort on fallback mechanism") has been merged on July 2017, sorry 
for that.

Before commit 76098b36b5db, user helper sent -EAGAIN for all errors 
including "getting interrupted" and "file not found". The one more retry 
was for the sysfs got interrupted, and what we expected was the driver 
should get "-EAGAIN" when clm blob file was not found for legacy wifi chip.
Since commit 76098b36b5db changed the error code for interrupted, I will 
remove the retry mechanism and keep remaining change for Patch v2. 
Thanks for the review.

commit 76098b36b5db1a509e5af94128b08f950692c7f8
Author: Luis R. Rodriguez <mcgrof@kernel.org>
Date:   Thu Jul 20 13:13:39 2017 -0700

     firmware: send -EINTR on signal abort on fallback mechanism

     Right now we send -EAGAIN to a syfs write which got interrupted.
     Userspace can't tell what happened though, send -EINTR if we
     were killed due to a signal so userspace can tell things apart.

     This is only applicable to the fallback mechanism.

Regards,
Wright