| Message ID | 1513071596-17506-1-git-send-email-wright.feng@cypress.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
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?
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
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;
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(-)