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 <[email protected]>
---
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
Wright Feng <[email protected]> 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 <[email protected]>
[...]
> @@ -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
Hi Kalle,
On 2017/12/12 下午 08:57, Kalle Valo wrote:
> Wright Feng <[email protected]> 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 <[email protected]>
>
> [...]
>
>> @@ -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 <[email protected]>
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