* [PATCH v2] brcmfmac: fix CLM load error for legacy chips when user helper is enabled
@ 2018-01-12 7:44 Wright Feng
2018-01-12 10:55 ` Arend van Spriel
0 siblings, 1 reply; 6+ messages in thread
From: Wright Feng @ 2018-01-12 7:44 UTC (permalink / raw)
To: arend.vanspriel, franky.lin, hante.meuleman, kvalo, chi-hsien.lin
Cc: wright.feng, linux-wireless, brcm80211-dev-list.pdl
For legacy chips without CLM blob files, kernel with user helper function
returns -EAGAIN when we request_firmware() for blob file. In this case,
brcmf_bus_started gets error and failed to bring up legacy chips.
Because of that, we should continue with CLM data currently present in
firmware if getting -EAGAIN when doing request_firmware().
Signed-off-by: Wright Feng <wright.feng@cypress.com>
---
v2: remove retry from patch v1
---
drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
index 6a59d06..0baab4c 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
@@ -182,7 +182,7 @@ static int brcmf_c_process_clm_blob(struct brcmf_if *ifp)
err = request_firmware(&clm, clm_name, dev);
if (err) {
- if (err == -ENOENT) {
+ if (err == -ENOENT || err == -EAGAIN) {
brcmf_dbg(INFO, "continue with CLM data currently present in firmware\n");
return 0;
}
--
1.9.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] brcmfmac: fix CLM load error for legacy chips when user helper is enabled
2018-01-12 7:44 [PATCH v2] brcmfmac: fix CLM load error for legacy chips when user helper is enabled Wright Feng
@ 2018-01-12 10:55 ` Arend van Spriel
2018-01-12 11:16 ` Kalle Valo
0 siblings, 1 reply; 6+ messages in thread
From: Arend van Spriel @ 2018-01-12 10:55 UTC (permalink / raw)
To: Wright Feng, franky.lin, hante.meuleman, kvalo, chi-hsien.lin
Cc: linux-wireless, brcm80211-dev-list.pdl
On 1/12/2018 8:44 AM, Wright Feng wrote:
> For legacy chips without CLM blob files, kernel with user helper function
> returns -EAGAIN when we request_firmware() for blob file. In this case,
> brcmf_bus_started gets error and failed to bring up legacy chips.
> Because of that, we should continue with CLM data currently present in
> firmware if getting -EAGAIN when doing request_firmware().
>
> Signed-off-by: Wright Feng <wright.feng@cypress.com>
> ---
> v2: remove retry from patch v1
> ---
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
> index 6a59d06..0baab4c 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
> @@ -182,7 +182,7 @@ static int brcmf_c_process_clm_blob(struct brcmf_if *ifp)
>
> err = request_firmware(&clm, clm_name, dev);
> if (err) {
> - if (err == -ENOENT) {
> + if (err == -ENOENT || err == -EAGAIN) {
> brcmf_dbg(INFO, "continue with CLM data currently present in firmware\n");
> return 0;
> }
Hi Wright,
Why don't we just fall-back to "CLM in firmware" regardless of the error
code? Also it might be better to use brcmf_info() instead of
brcmf_dbg(INFO, ..).
Regards,
Arend
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] brcmfmac: fix CLM load error for legacy chips when user helper is enabled
2018-01-12 10:55 ` Arend van Spriel
@ 2018-01-12 11:16 ` Kalle Valo
2018-01-15 10:09 ` Wright Feng
0 siblings, 1 reply; 6+ messages in thread
From: Kalle Valo @ 2018-01-12 11:16 UTC (permalink / raw)
To: Arend van Spriel
Cc: Wright Feng, franky.lin, hante.meuleman, chi-hsien.lin,
linux-wireless, brcm80211-dev-list.pdl
Arend van Spriel <arend.vanspriel@broadcom.com> writes:
> On 1/12/2018 8:44 AM, Wright Feng wrote:
>> For legacy chips without CLM blob files, kernel with user helper function
>> returns -EAGAIN when we request_firmware() for blob file.
_Why_ is the -EAGAIN returned? Is it because of user space, due to
timing when loading the brcmfmac module or what? You should explain the
problem in detail in the commit log and why this is the right approach
to fix the problem.
Based on the commit log to me this still looks like a random attempt to
workaround a bug, not a proper fix.
>> In this case, brcmf_bus_started gets error and failed to bring up
>> legacy chips. Because of that, we should continue with CLM data
>> currently present in firmware if getting -EAGAIN when doing
>> request_firmware().
>>
>> Signed-off-by: Wright Feng <wright.feng@cypress.com>
>> ---
>> v2: remove retry from patch v1
>> ---
>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
>> index 6a59d06..0baab4c 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
>> @@ -182,7 +182,7 @@ static int brcmf_c_process_clm_blob(struct brcmf_if *ifp)
>>
>> err = request_firmware(&clm, clm_name, dev);
>> if (err) {
>> - if (err == -ENOENT) {
>> + if (err == -ENOENT || err == -EAGAIN) {
>> brcmf_dbg(INFO, "continue with CLM data currently present in firmware\n");
>> return 0;
>> }
>
> Why don't we just fall-back to "CLM in firmware" regardless of the
> error code?
Indeed, I was thinking the same.
--
Kalle Valo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] brcmfmac: fix CLM load error for legacy chips when user helper is enabled
2018-01-12 11:16 ` Kalle Valo
@ 2018-01-15 10:09 ` Wright Feng
2018-01-15 19:54 ` Arend van Spriel
0 siblings, 1 reply; 6+ messages in thread
From: Wright Feng @ 2018-01-15 10:09 UTC (permalink / raw)
To: Kalle Valo, Arend van Spriel
Cc: franky.lin, hante.meuleman, chi-hsien.lin, linux-wireless,
brcm80211-dev-list.pdl
On 2018/1/12 下午 07:16, Kalle Valo wrote:
> Arend van Spriel <arend.vanspriel@broadcom.com> writes:
>
>> On 1/12/2018 8:44 AM, Wright Feng wrote:
>>> For legacy chips without CLM blob files, kernel with user helper function
>>> returns -EAGAIN when we request_firmware() for blob file.
>
> _Why_ is the -EAGAIN returned? Is it because of user space, due to
> timing when loading the brcmfmac module or what? You should explain the
> problem in detail in the commit log and why this is the right approach
> to fix the problem.
>
> Based on the commit log to me this still looks like a random attempt to
> workaround a bug, not a proper fix.
>
It is not about the timing issue, it is about "the clm blob is not
existence in firmware path for legacy firmware with CLM data built-in"
and the -ENOENT error is transferred to -EAGAIN in firmware_class.c.
Here is explanation of -EAGAIN returned in detail.
In drivers/base/firmware_class.c:__fw_state_wait_common, it returns
-ENOENT to indicate the clm_blob file is not found via user helper.
However, in drivers/base/firmware_class.c:_request_firmware_load, all
errors with fw status aborted are transferred to:
1. -ERESTARTSYS: The signal is pending and the task is interruptible.
Before 76098b36b5db ("firmware: send -EINTR on signal abort on fallback
mechanism"), all errors with fw status aborted are transferred to
-EAGAIN.
2. -EAGAIN: All others fw status aborted(include -ENOENT) except for
-ERESTARTSYS.
And that's why I handle -EAGAIN error to let driver using CLM data in
firmware.
>>> In this case, brcmf_bus_started gets error and failed to bring up
>>> legacy chips. Because of that, we should continue with CLM data
>>> currently present in firmware if getting -EAGAIN when doing
>>> request_firmware().
>>>
>>> Signed-off-by: Wright Feng <wright.feng@cypress.com>
>>> ---
>>> v2: remove retry from patch v1
>>> ---
>>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
>>> index 6a59d06..0baab4c 100644
>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
>>> @@ -182,7 +182,7 @@ static int brcmf_c_process_clm_blob(struct brcmf_if *ifp)
>>>
>>> err = request_firmware(&clm, clm_name, dev);
>>> if (err) {
>>> - if (err == -ENOENT) {
>>> + if (err == -ENOENT || err == -EAGAIN) {
>>> brcmf_dbg(INFO, "continue with CLM data currently present in firmware\n");
>>> return 0;
>>> }
>>
>> Why don't we just fall-back to "CLM in firmware" regardless of the
>> error code?
>
> Indeed, I was thinking the same.
>
For the firmwares with CLM data built-in, it is okay to continue the
bringing-up flow.
But when users put clm_blob file to firmware path, the corresponding
firmware may not have CLM data built-in. If we fall-back to use empty
CLM data in firmware when hitting other errors like "-ENOMEM" or
"-EINTR", the country code revision will be null and users will meet
error when trying to connect to access point.
It is fine to fall-back to "CLM in firmware" regardless of the error
code, but it would be better to print error log to indicate the error if
the returned error codes are not what we expected.
Please let me know your opinion of below commit log and diff, if it is
okay to you, I will post Patch v3 later.
For legacy chips without CLM blob files, kernel with user helper
function returns -EAGAIN when we request_firmware() for blob file. So,
it got failed when bring up legacy chips. We expect the CLM blob files
is not existence in firmware path, however the -ENOENT error is
transferred to -EAGAIN in firmware_class.c.
Because of that, we continue with CLM data currently present in
firmware if getting error in doing request_firmware().
---
drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c | 12
++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
index 6a59d06..aaab0e6 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
@@ -182,12 +182,12 @@ static int brcmf_c_process_clm_blob(struct
brcmf_if *ifp)
err = request_firmware(&clm, clm_name, dev);
if (err) {
- if (err == -ENOENT) {
- brcmf_dbg(INFO, "continue with CLM data currently present in
firmware\n");
- return 0;
- }
- brcmf_err("request CLM blob file failed (%d)\n", err);
- return err;
+ if (err == -ENOENT || err == -EAGAIN)
+ brcmf_info("continue with CLM data in FW\n");
+ else
+ brcmf_err("request clm_blob failed(%d) continue with CLM data in FW\n",
+ err);
+ return 0;
}
chunk_buf = kzalloc(sizeof(*chunk_buf) + MAX_CHUNK_LEN - 1, GFP_KERNEL);
--
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] brcmfmac: fix CLM load error for legacy chips when user helper is enabled
2018-01-15 10:09 ` Wright Feng
@ 2018-01-15 19:54 ` Arend van Spriel
2018-01-16 1:52 ` Wright Feng
0 siblings, 1 reply; 6+ messages in thread
From: Arend van Spriel @ 2018-01-15 19:54 UTC (permalink / raw)
To: Wright Feng, Kalle Valo
Cc: franky.lin, hante.meuleman, chi-hsien.lin, linux-wireless,
brcm80211-dev-list.pdl
On 1/15/2018 11:09 AM, Wright Feng wrote:
>
[...]
> ---
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c | 12
> ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
> index 6a59d06..aaab0e6 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
> @@ -182,12 +182,12 @@ static int brcmf_c_process_clm_blob(struct
> brcmf_if *ifp)
>
> err = request_firmware(&clm, clm_name, dev);
> if (err) {
> - if (err == -ENOENT) {
> - brcmf_dbg(INFO, "continue with CLM data currently present
> in firmware\n");
> - return 0;
> - }
> - brcmf_err("request CLM blob file failed (%d)\n", err);
> - return err;
> + if (err == -ENOENT || err == -EAGAIN)
> + brcmf_info("continue with CLM data in FW\n");
> + else
> + brcmf_err("request clm_blob failed(%d) continue with CLM
> data in FW\n",
> + err);
Don't see much value in doing it this way. Either way we need to inform
the user about the consequence of this, ie.:
brcmf_info("no clm_blob available (%d). device may have limited
channels available\n", err);
> + return 0;
> }
Regards,
Arend
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] brcmfmac: fix CLM load error for legacy chips when user helper is enabled
2018-01-15 19:54 ` Arend van Spriel
@ 2018-01-16 1:52 ` Wright Feng
0 siblings, 0 replies; 6+ messages in thread
From: Wright Feng @ 2018-01-16 1:52 UTC (permalink / raw)
To: Arend van Spriel, Kalle Valo
Cc: franky.lin, hante.meuleman, chi-hsien.lin, linux-wireless,
brcm80211-dev-list.pdl
On 2018/1/16 上午 03:54, Arend van Spriel wrote:
> On 1/15/2018 11:09 AM, Wright Feng wrote:
>>
>
> [...]
>
>> ---
>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c | 12
>> ++++++------
>> 1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
>> index 6a59d06..aaab0e6 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
>> @@ -182,12 +182,12 @@ static int brcmf_c_process_clm_blob(struct
>> brcmf_if *ifp)
>>
>> err = request_firmware(&clm, clm_name, dev);
>> if (err) {
>> - if (err == -ENOENT) {
>> - brcmf_dbg(INFO, "continue with CLM data currently present
>> in firmware\n");
>> - return 0;
>> - }
>> - brcmf_err("request CLM blob file failed (%d)\n", err);
>> - return err;
>> + if (err == -ENOENT || err == -EAGAIN)
>> + brcmf_info("continue with CLM data in FW\n");
>> + else
>> + brcmf_err("request clm_blob failed(%d) continue with CLM
>> data in FW\n",
>> + err);
>
> Don't see much value in doing it this way. Either way we need to inform
> the user about the consequence of this, ie.:
>
> brcmf_info("no clm_blob available (%d). device may have limited
> channels available\n", err);
>> + return 0;
>> }
>
> Regards,
> Arend
Thanks for the comment, I will post patch v3 with your suggestion later.
The patch will include one brcmf_info print and returning 0 regardless
of errors from request_firmware.
Regards,
Wright
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-01-16 1:52 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-12 7:44 [PATCH v2] brcmfmac: fix CLM load error for legacy chips when user helper is enabled Wright Feng
2018-01-12 10:55 ` Arend van Spriel
2018-01-12 11:16 ` Kalle Valo
2018-01-15 10:09 ` Wright Feng
2018-01-15 19:54 ` Arend van Spriel
2018-01-16 1:52 ` Wright Feng
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.