| Message ID | 20220104072658.69756-5-marcan@marcan.st (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | brcmfmac: Support Apple T2 and M1 platforms | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
04.01.2022 10:26, Hector Martin пишет: > Apple platforms have firmware and config files identified with multiple > dimensions. We want to be able to find the most specific firmware > available for any given platform, progressively trying more general > firmwares. > > First, add support for having multiple alternate firmware paths. > > Acked-by: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > .../broadcom/brcm80211/brcmfmac/firmware.c | 75 ++++++++++++++----- > .../broadcom/brcm80211/brcmfmac/firmware.h | 2 + > 2 files changed, 59 insertions(+), 18 deletions(-) > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c > index 0497b721136a..7570dbf22cdd 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c > @@ -427,6 +427,8 @@ void brcmf_fw_nvram_free(void *nvram) > struct brcmf_fw { > struct device *dev; > struct brcmf_fw_request *req; > + const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS]; > + int alt_index; unsigned int > u32 curpos; > void (*done)(struct device *dev, int err, struct brcmf_fw_request *req); > }; > @@ -592,14 +594,18 @@ static int brcmf_fw_complete_request(const struct firmware *fw, > return (cur->flags & BRCMF_FW_REQF_OPTIONAL) ? 0 : ret; > } > > -static char *brcm_alt_fw_path(const char *path, const char *board_type) > +static int brcm_alt_fw_paths(const char *path, const char *board_type, > + const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS])> { > char alt_path[BRCMF_FW_NAME_LEN]; > const char *suffix; > > + memset(alt_paths, 0, array_size(sizeof(*alt_paths), > + BRCMF_FW_MAX_ALT_PATHS)); You don't need to use array_size() since size of a fixed array is already known. memset(alt_paths, 0, sizeof(alt_paths)); ... > +static void > +brcm_free_alt_fw_paths(const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS]) > +{ > + unsigned int i; > + > + for (i = 0; alt_paths[i]; i++) What if array is fully populated and there is no null in the end? Please don't do this, use BRCMF_FW_MAX_ALT_PATHS or ARRAY_SIZE(). > + kfree(alt_paths[i]); > } > > static int brcmf_fw_request_firmware(const struct firmware **fw, > @@ -617,19 +634,25 @@ static int brcmf_fw_request_firmware(const struct firmware **fw, > { > struct brcmf_fw_item *cur = &fwctx->req->items[fwctx->curpos]; > int ret; > + unsigned int i; Keep reverse Xmas tree coding style. ... > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h > @@ -11,6 +11,8 @@ > > #define BRCMF_FW_DEFAULT_PATH "brcm/" > > +#define BRCMF_FW_MAX_ALT_PATHS 8 Two tabs are needed here.
On 2022/01/04 17:26, Dmitry Osipenko wrote: > 04.01.2022 10:26, Hector Martin пишет: >> Apple platforms have firmware and config files identified with multiple >> dimensions. We want to be able to find the most specific firmware >> available for any given platform, progressively trying more general >> firmwares. >> >> First, add support for having multiple alternate firmware paths. >> >> Acked-by: Linus Walleij <linus.walleij@linaro.org> >> Signed-off-by: Hector Martin <marcan@marcan.st> >> --- >> .../broadcom/brcm80211/brcmfmac/firmware.c | 75 ++++++++++++++----- >> .../broadcom/brcm80211/brcmfmac/firmware.h | 2 + >> 2 files changed, 59 insertions(+), 18 deletions(-) >> >> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c >> index 0497b721136a..7570dbf22cdd 100644 >> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c >> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c >> @@ -427,6 +427,8 @@ void brcmf_fw_nvram_free(void *nvram) >> struct brcmf_fw { >> struct device *dev; >> struct brcmf_fw_request *req; >> + const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS]; >> + int alt_index; > > unsigned int Ack. > >> u32 curpos; >> void (*done)(struct device *dev, int err, struct brcmf_fw_request *req); >> }; >> @@ -592,14 +594,18 @@ static int brcmf_fw_complete_request(const struct firmware *fw, >> return (cur->flags & BRCMF_FW_REQF_OPTIONAL) ? 0 : ret; >> } >> >> -static char *brcm_alt_fw_path(const char *path, const char *board_type) >> +static int brcm_alt_fw_paths(const char *path, const char *board_type, >> + const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS])> { >> char alt_path[BRCMF_FW_NAME_LEN]; >> const char *suffix; >> >> + memset(alt_paths, 0, array_size(sizeof(*alt_paths), >> + BRCMF_FW_MAX_ALT_PATHS)); > You don't need to use array_size() since size of a fixed array is > already known. > > memset(alt_paths, 0, sizeof(alt_paths)); It's a function argument, so that doesn't work and actually throws a warning. Array function argument notation is informative only; they behave strictly equivalent to pointers. Try it: $ cat test.c #include <stdio.h> void foo(char x[42]) { printf("%ld\n", sizeof(x)); } int main() { char x[42]; foo(x); } $ gcc test.c test.c: In function ‘foo’: test.c:5:31: warning: ‘sizeof’ on array function parameter ‘x’ will return size of ‘char *’ [-Wsizeof-array-argument] 5 | printf("%ld\n", sizeof(x)); | ^ test.c:3:15: note: declared here 3 | void foo(char x[42]) | ~~~~~^~~~~ $ ./a.out 8 > > ... >> +static void >> +brcm_free_alt_fw_paths(const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS]) >> +{ >> + unsigned int i; >> + >> + for (i = 0; alt_paths[i]; i++) > > What if array is fully populated and there is no null in the end? Please > don't do this, use BRCMF_FW_MAX_ALT_PATHS or ARRAY_SIZE(). Argh, forgot to change this one. I used BRCMF_FW_MAX_ALT_PATHS elsewhere; ARRAY_SIZE won't work as I explained above. > >> + kfree(alt_paths[i]); >> } >> >> static int brcmf_fw_request_firmware(const struct firmware **fw, >> @@ -617,19 +634,25 @@ static int brcmf_fw_request_firmware(const struct firmware **fw, >> { >> struct brcmf_fw_item *cur = &fwctx->req->items[fwctx->curpos]; >> int ret; >> + unsigned int i; > > Keep reverse Xmas tree coding style. First time I hear this one, heh. Sure. > > ... >> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h >> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h >> @@ -11,6 +11,8 @@ >> >> #define BRCMF_FW_DEFAULT_PATH "brcm/" >> >> +#define BRCMF_FW_MAX_ALT_PATHS 8 > > Two tabs are needed here. Will do.
04.01.2022 11:43, Hector Martin пишет: >>> +static int brcm_alt_fw_paths(const char *path, const char *board_type, >>> + const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS])> { >>> char alt_path[BRCMF_FW_NAME_LEN]; >>> const char *suffix; >>> >>> + memset(alt_paths, 0, array_size(sizeof(*alt_paths), >>> + BRCMF_FW_MAX_ALT_PATHS)); >> You don't need to use array_size() since size of a fixed array is >> already known. >> >> memset(alt_paths, 0, sizeof(alt_paths)); > It's a function argument, so that doesn't work and actually throws a > warning. Array function argument notation is informative only; they > behave strictly equivalent to pointers. Try it: > > $ cat test.c > #include <stdio.h> > > void foo(char x[42]) > { > printf("%ld\n", sizeof(x)); > } > > int main() { > char x[42]; > > foo(x); > } > $ gcc test.c > test.c: In function ‘foo’: > test.c:5:31: warning: ‘sizeof’ on array function parameter ‘x’ will > return size of ‘char *’ [-Wsizeof-array-argument] > 5 | printf("%ld\n", sizeof(x)); > | ^ > test.c:3:15: note: declared here > 3 | void foo(char x[42]) > | ~~~~~^~~~~ > $ ./a.out > 8 Then please use "const char **alt_paths" for the function argument to make code cleaner and add another argument to pass the number of array elements. static int brcm_alt_fw_paths(const char *path, const char *board_type, const char **alt_paths, unsigned int num_paths) { size_t alt_paths_size = array_size(sizeof(*alt_paths), num_paths); memset(alt_paths, 0, alt_paths_size); } ... Maybe even better create a dedicated struct for the alt_paths: struct brcmf_fw_alt_paths { const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS]; unsigned int index; }; and then use the ".index" in the brcm_free_alt_fw_paths(). I suppose this will make code a bit nicer and easier to follow.
04.01.2022 11:43, Hector Martin пишет: >>> @@ -427,6 +427,8 @@ void brcmf_fw_nvram_free(void *nvram) >>> struct brcmf_fw { >>> struct device *dev; >>> struct brcmf_fw_request *req; >>> + const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS]; >>> + int alt_index; >> unsigned int > Ack. > The same applies to the rest of the patches. If value can't be negative, then please use unsigned type. This makes code more consistent.
04.01.2022 10:26, Hector Martin пишет: > +static int brcm_alt_fw_paths(const char *path, const char *board_type, > + const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS]) > { ... > +static void > +brcm_free_alt_fw_paths(const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS]) > +{ I'd rename this funcs to brcm_init/deinit_alt_fw_paths(), for consistency and clarity.
On 05/01/2022 07.09, Dmitry Osipenko wrote: > 04.01.2022 11:43, Hector Martin пишет: >>>> +static int brcm_alt_fw_paths(const char *path, const char *board_type, >>>> + const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS])> { >>>> char alt_path[BRCMF_FW_NAME_LEN]; >>>> const char *suffix; >>>> >>>> + memset(alt_paths, 0, array_size(sizeof(*alt_paths), >>>> + BRCMF_FW_MAX_ALT_PATHS)); >>> You don't need to use array_size() since size of a fixed array is >>> already known. >>> >>> memset(alt_paths, 0, sizeof(alt_paths)); >> It's a function argument, so that doesn't work and actually throws a >> warning. Array function argument notation is informative only; they >> behave strictly equivalent to pointers. Try it: >> >> $ cat test.c >> #include <stdio.h> >> >> void foo(char x[42]) >> { >> printf("%ld\n", sizeof(x)); >> } >> >> int main() { >> char x[42]; >> >> foo(x); >> } >> $ gcc test.c >> test.c: In function ‘foo’: >> test.c:5:31: warning: ‘sizeof’ on array function parameter ‘x’ will >> return size of ‘char *’ [-Wsizeof-array-argument] >> 5 | printf("%ld\n", sizeof(x)); >> | ^ >> test.c:3:15: note: declared here >> 3 | void foo(char x[42]) >> | ~~~~~^~~~~ >> $ ./a.out >> 8 > > Then please use "const char **alt_paths" for the function argument to > make code cleaner and add another argument to pass the number of array > elements. So you want me to do the ARRAY_SIZE at the caller side then? > > static int brcm_alt_fw_paths(const char *path, const char *board_type, > const char **alt_paths, unsigned int num_paths) > { > size_t alt_paths_size = array_size(sizeof(*alt_paths), num_paths); > > memset(alt_paths, 0, alt_paths_size); > } > > ... > > Maybe even better create a dedicated struct for the alt_paths: > > struct brcmf_fw_alt_paths { > const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS]; > unsigned int index; > }; > > and then use the ".index" in the brcm_free_alt_fw_paths(). I suppose > this will make code a bit nicer and easier to follow. > I'm confused; the array size is constant. What would index contain and why would would brcm_free_alt_fw_paths use it? Just as an iterator variable instead of using a local variable? Or do you mean count? Though, to be honest, at this point I'm considering rethinking the whole patch for this mechanism because I'm not terribly happy with the current approach and clearly you aren't either :-) Maybe it makes more sense to stop trying to compute all the alt_paths ahead of time, and just have the function compute a single one to be used just-in-time at firmware request time, and just iterate over board_types.
On 1/4/2022 8:26 AM, Hector Martin wrote: > Apple platforms have firmware and config files identified with multiple > dimensions. We want to be able to find the most specific firmware > available for any given platform, progressively trying more general > firmwares. > > First, add support for having multiple alternate firmware paths. Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com> > Acked-by: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > .../broadcom/brcm80211/brcmfmac/firmware.c | 75 ++++++++++++++----- > .../broadcom/brcm80211/brcmfmac/firmware.h | 2 + > 2 files changed, 59 insertions(+), 18 deletions(-) > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c > index 0497b721136a..7570dbf22cdd 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c > @@ -427,6 +427,8 @@ void brcmf_fw_nvram_free(void *nvram) > struct brcmf_fw { > struct device *dev; > struct brcmf_fw_request *req; > + const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS]; > + int alt_index; > u32 curpos; > void (*done)(struct device *dev, int err, struct brcmf_fw_request *req); > }; > @@ -592,14 +594,18 @@ static int brcmf_fw_complete_request(const struct firmware *fw, > return (cur->flags & BRCMF_FW_REQF_OPTIONAL) ? 0 : ret; > } > > -static char *brcm_alt_fw_path(const char *path, const char *board_type) > +static int brcm_alt_fw_paths(const char *path, const char *board_type, > + const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS]) > { > char alt_path[BRCMF_FW_NAME_LEN]; > const char *suffix; > > + memset(alt_paths, 0, array_size(sizeof(*alt_paths), > + BRCMF_FW_MAX_ALT_PATHS)); > + > suffix = strrchr(path, '.'); > if (!suffix || suffix == path) > - return NULL; > + return -EINVAL; > > /* strip extension at the end */ > strscpy(alt_path, path, BRCMF_FW_NAME_LEN); > @@ -609,7 +615,18 @@ static char *brcm_alt_fw_path(const char *path, const char *board_type) > strlcat(alt_path, board_type, BRCMF_FW_NAME_LEN); > strlcat(alt_path, suffix, BRCMF_FW_NAME_LEN); > > - return kstrdup(alt_path, GFP_KERNEL); > + alt_paths[0] = kstrdup(alt_path, GFP_KERNEL); > + > + return 0; > +} > + > +static void > +brcm_free_alt_fw_paths(const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS]) > +{ > + unsigned int i; > + > + for (i = 0; alt_paths[i]; i++) > + kfree(alt_paths[i]); > } > > static int brcmf_fw_request_firmware(const struct firmware **fw, > @@ -617,19 +634,25 @@ static int brcmf_fw_request_firmware(const struct firmware **fw, > { > struct brcmf_fw_item *cur = &fwctx->req->items[fwctx->curpos]; > int ret; > + unsigned int i; > > /* Files can be board-specific, first try a board-specific path */ > if (fwctx->req->board_type) { > - char *alt_path; > + const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS]; > > - alt_path = brcm_alt_fw_path(cur->path, fwctx->req->board_type); > - if (!alt_path) > + if (brcm_alt_fw_paths(cur->path, fwctx->req->board_type, > + alt_paths) != 0) > goto fallback; > > - ret = request_firmware(fw, alt_path, fwctx->dev); > - kfree(alt_path); > - if (ret == 0) > - return ret; > + for (i = 0; i < BRCMF_FW_MAX_ALT_PATHS && alt_paths[i]; i++) { > + ret = firmware_request_nowarn(fw, alt_paths[i], > + fwctx->dev); > + if (ret == 0) { > + brcm_free_alt_fw_paths(alt_paths); > + return ret; > + } > + } > + brcm_free_alt_fw_paths(alt_paths); > } > > fallback: > @@ -641,6 +664,8 @@ static void brcmf_fw_request_done(const struct firmware *fw, void *ctx) > struct brcmf_fw *fwctx = ctx; > int ret; > > + brcm_free_alt_fw_paths(fwctx->alt_paths); > + > ret = brcmf_fw_complete_request(fw, fwctx); > > while (ret == 0 && ++fwctx->curpos < fwctx->req->n_items) { > @@ -662,13 +687,27 @@ static void brcmf_fw_request_done_alt_path(const struct firmware *fw, void *ctx) > struct brcmf_fw_item *first = &fwctx->req->items[0]; > int ret = 0; > > - /* Fall back to canonical path if board firmware not found */ > - if (!fw) > + if (fw) { > + brcmf_fw_request_done(fw, ctx); > + return; > + } > + > + fwctx->alt_index++; > + if (fwctx->alt_index < BRCMF_FW_MAX_ALT_PATHS && > + fwctx->alt_paths[fwctx->alt_index]) { > + /* Try the next alt firmware */ > + ret = request_firmware_nowait(THIS_MODULE, true, > + fwctx->alt_paths[fwctx->alt_index], > + fwctx->dev, GFP_KERNEL, fwctx, > + brcmf_fw_request_done_alt_path); > + } else { > + /* Fall back to canonical path if board firmware not found */ > ret = request_firmware_nowait(THIS_MODULE, true, first->path, > fwctx->dev, GFP_KERNEL, fwctx, > brcmf_fw_request_done); > + } > > - if (fw || ret < 0) > + if (ret < 0) > brcmf_fw_request_done(fw, ctx); > } > > @@ -693,7 +732,6 @@ int brcmf_fw_get_firmwares(struct device *dev, struct brcmf_fw_request *req, > { > struct brcmf_fw_item *first = &req->items[0]; > struct brcmf_fw *fwctx; > - char *alt_path; > int ret; > > brcmf_dbg(TRACE, "enter: dev=%s\n", dev_name(dev)); > @@ -712,12 +750,13 @@ int brcmf_fw_get_firmwares(struct device *dev, struct brcmf_fw_request *req, > fwctx->done = fw_cb; > > /* First try alternative board-specific path if any */ > - alt_path = brcm_alt_fw_path(first->path, fwctx->req->board_type); > - if (alt_path) { > - ret = request_firmware_nowait(THIS_MODULE, true, alt_path, > + if (brcm_alt_fw_paths(first->path, req->board_type, > + fwctx->alt_paths) == 0) { > + fwctx->alt_index = 0; > + ret = request_firmware_nowait(THIS_MODULE, true, > + fwctx->alt_paths[0], > fwctx->dev, GFP_KERNEL, fwctx, > brcmf_fw_request_done_alt_path); > - kfree(alt_path); > } else { > ret = request_firmware_nowait(THIS_MODULE, true, first->path, > fwctx->dev, GFP_KERNEL, fwctx, > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h > index e290dec9c53d..7f4e6e359c82 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h > @@ -11,6 +11,8 @@ > > #define BRCMF_FW_DEFAULT_PATH "brcm/" > > +#define BRCMF_FW_MAX_ALT_PATHS 8 > + Any motivation to have 8 here today? In patch #9 I see a list of 6 paths in the commit message so you need 6 and rounded up here to power of 2? > /** > * struct brcmf_firmware_mapping - Used to map chipid/revmask to firmware > * filename and nvram filename. Each bus type implementation should create
On 2022/01/06 19:43, Arend van Spriel wrote: >> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h >> index e290dec9c53d..7f4e6e359c82 100644 >> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h >> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h >> @@ -11,6 +11,8 @@ >> >> #define BRCMF_FW_DEFAULT_PATH "brcm/" >> >> +#define BRCMF_FW_MAX_ALT_PATHS 8 >> + > > Any motivation to have 8 here today? In patch #9 I see a list of 6 paths > in the commit message so you need 6 and rounded up here to power of 2? > Heh, yeah, that's just my powers-of-two-are-nice-numbers habit. I can drop it down to 6 if you prefer.
05.01.2022 16:22, Hector Martin пишет: > On 05/01/2022 07.09, Dmitry Osipenko wrote: >> 04.01.2022 11:43, Hector Martin пишет: >>>>> +static int brcm_alt_fw_paths(const char *path, const char *board_type, >>>>> + const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS])> { >>>>> char alt_path[BRCMF_FW_NAME_LEN]; >>>>> const char *suffix; >>>>> >>>>> + memset(alt_paths, 0, array_size(sizeof(*alt_paths), >>>>> + BRCMF_FW_MAX_ALT_PATHS)); >>>> You don't need to use array_size() since size of a fixed array is >>>> already known. >>>> >>>> memset(alt_paths, 0, sizeof(alt_paths)); >>> It's a function argument, so that doesn't work and actually throws a >>> warning. Array function argument notation is informative only; they >>> behave strictly equivalent to pointers. Try it: >>> >>> $ cat test.c >>> #include <stdio.h> >>> >>> void foo(char x[42]) >>> { >>> printf("%ld\n", sizeof(x)); >>> } >>> >>> int main() { >>> char x[42]; >>> >>> foo(x); >>> } >>> $ gcc test.c >>> test.c: In function ‘foo’: >>> test.c:5:31: warning: ‘sizeof’ on array function parameter ‘x’ will >>> return size of ‘char *’ [-Wsizeof-array-argument] >>> 5 | printf("%ld\n", sizeof(x)); >>> | ^ >>> test.c:3:15: note: declared here >>> 3 | void foo(char x[42]) >>> | ~~~~~^~~~~ >>> $ ./a.out >>> 8 >> >> Then please use "const char **alt_paths" for the function argument to >> make code cleaner and add another argument to pass the number of array >> elements. > > So you want me to do the ARRAY_SIZE at the caller side then? > >> >> static int brcm_alt_fw_paths(const char *path, const char *board_type, >> const char **alt_paths, unsigned int num_paths) >> { >> size_t alt_paths_size = array_size(sizeof(*alt_paths), num_paths); >> >> memset(alt_paths, 0, alt_paths_size); >> } >> >> ... >> >> Maybe even better create a dedicated struct for the alt_paths: >> >> struct brcmf_fw_alt_paths { >> const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS]; >> unsigned int index; >> }; >> >> and then use the ".index" in the brcm_free_alt_fw_paths(). I suppose >> this will make code a bit nicer and easier to follow. >> > > I'm confused; the array size is constant. What would index contain and > why would would brcm_free_alt_fw_paths use it? Just as an iterator > variable instead of using a local variable? Or do you mean count? Yes, use index for the count of active entries in the alt_paths[]. for (i = 0; i < alt_paths.index; i++) kfree(alt_paths.path[i]); alt_paths.index = 0; or while (alt_paths.index) kfree(alt_paths.path[--alt_paths.index]); > Though, to be honest, at this point I'm considering rethinking the whole > patch for this mechanism because I'm not terribly happy with the current > approach and clearly you aren't either :-) Maybe it makes more sense to > stop trying to compute all the alt_paths ahead of time, and just have > the function compute a single one to be used just-in-time at firmware > request time, and just iterate over board_types. > The just-in-time approach sounds like a good idea.
On Thu, Jan 6, 2022 at 7:40 PM Dmitry Osipenko <digetx@gmail.com> wrote: > 05.01.2022 16:22, Hector Martin пишет: > > On 05/01/2022 07.09, Dmitry Osipenko wrote: ... > > I'm confused; the array size is constant. What would index contain and > > why would would brcm_free_alt_fw_paths use it? Just as an iterator > > variable instead of using a local variable? Or do you mean count? > > Yes, use index for the count of active entries in the alt_paths[]. > > for (i = 0; i < alt_paths.index; i++) > kfree(alt_paths.path[i]); > > alt_paths.index = 0; > > or > > while (alt_paths.index) > kfree(alt_paths.path[--alt_paths.index]); Usual pattern is while (x--) kfree(x); easier to read, extend (if needed).
06.01.2022 20:58, Andy Shevchenko пишет: > On Thu, Jan 6, 2022 at 7:40 PM Dmitry Osipenko <digetx@gmail.com> wrote: >> 05.01.2022 16:22, Hector Martin пишет: >>> On 05/01/2022 07.09, Dmitry Osipenko wrote: > > ... > >>> I'm confused; the array size is constant. What would index contain and >>> why would would brcm_free_alt_fw_paths use it? Just as an iterator >>> variable instead of using a local variable? Or do you mean count? >> >> Yes, use index for the count of active entries in the alt_paths[]. >> >> for (i = 0; i < alt_paths.index; i++) >> kfree(alt_paths.path[i]); >> >> alt_paths.index = 0; >> >> or >> >> while (alt_paths.index) >> kfree(alt_paths.path[--alt_paths.index]); > > Usual pattern is > > while (x--) > kfree(x); > > easier to read, extend (if needed). > That is indeed a usual patter for the driver removal code paths. I didn't like to have index of struct brcmf_fw underflowed, but I see now that fwctx is dynamically created and freed during driver probe, so it should be fine to use that usual pattern here too.
On Fri, Jan 7, 2022 at 5:12 AM Dmitry Osipenko <digetx@gmail.com> wrote: > 06.01.2022 20:58, Andy Shevchenko пишет: > > On Thu, Jan 6, 2022 at 7:40 PM Dmitry Osipenko <digetx@gmail.com> wrote: > >> 05.01.2022 16:22, Hector Martin пишет: ... > >> while (alt_paths.index) > >> kfree(alt_paths.path[--alt_paths.index]); > > > > Usual pattern is > > > > while (x--) > > kfree(x); I have to elaborate that my point is to have postdecrement in the while() instead of doing predecrement in its body. So the above example will look while (alt_paths.index--) kfree(alt_paths.path[alt_paths.index]); > > easier to read, extend (if needed). > > That is indeed a usual patter for the driver removal code paths. I > didn't like to have index of struct brcmf_fw underflowed, but I see now > that fwctx is dynamically created and freed during driver probe, so it > should be fine to use that usual pattern here too.
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c index 0497b721136a..7570dbf22cdd 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c @@ -427,6 +427,8 @@ void brcmf_fw_nvram_free(void *nvram) struct brcmf_fw { struct device *dev; struct brcmf_fw_request *req; + const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS]; + int alt_index; u32 curpos; void (*done)(struct device *dev, int err, struct brcmf_fw_request *req); }; @@ -592,14 +594,18 @@ static int brcmf_fw_complete_request(const struct firmware *fw, return (cur->flags & BRCMF_FW_REQF_OPTIONAL) ? 0 : ret; } -static char *brcm_alt_fw_path(const char *path, const char *board_type) +static int brcm_alt_fw_paths(const char *path, const char *board_type, + const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS]) { char alt_path[BRCMF_FW_NAME_LEN]; const char *suffix; + memset(alt_paths, 0, array_size(sizeof(*alt_paths), + BRCMF_FW_MAX_ALT_PATHS)); + suffix = strrchr(path, '.'); if (!suffix || suffix == path) - return NULL; + return -EINVAL; /* strip extension at the end */ strscpy(alt_path, path, BRCMF_FW_NAME_LEN); @@ -609,7 +615,18 @@ static char *brcm_alt_fw_path(const char *path, const char *board_type) strlcat(alt_path, board_type, BRCMF_FW_NAME_LEN); strlcat(alt_path, suffix, BRCMF_FW_NAME_LEN); - return kstrdup(alt_path, GFP_KERNEL); + alt_paths[0] = kstrdup(alt_path, GFP_KERNEL); + + return 0; +} + +static void +brcm_free_alt_fw_paths(const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS]) +{ + unsigned int i; + + for (i = 0; alt_paths[i]; i++) + kfree(alt_paths[i]); } static int brcmf_fw_request_firmware(const struct firmware **fw, @@ -617,19 +634,25 @@ static int brcmf_fw_request_firmware(const struct firmware **fw, { struct brcmf_fw_item *cur = &fwctx->req->items[fwctx->curpos]; int ret; + unsigned int i; /* Files can be board-specific, first try a board-specific path */ if (fwctx->req->board_type) { - char *alt_path; + const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS]; - alt_path = brcm_alt_fw_path(cur->path, fwctx->req->board_type); - if (!alt_path) + if (brcm_alt_fw_paths(cur->path, fwctx->req->board_type, + alt_paths) != 0) goto fallback; - ret = request_firmware(fw, alt_path, fwctx->dev); - kfree(alt_path); - if (ret == 0) - return ret; + for (i = 0; i < BRCMF_FW_MAX_ALT_PATHS && alt_paths[i]; i++) { + ret = firmware_request_nowarn(fw, alt_paths[i], + fwctx->dev); + if (ret == 0) { + brcm_free_alt_fw_paths(alt_paths); + return ret; + } + } + brcm_free_alt_fw_paths(alt_paths); } fallback: @@ -641,6 +664,8 @@ static void brcmf_fw_request_done(const struct firmware *fw, void *ctx) struct brcmf_fw *fwctx = ctx; int ret; + brcm_free_alt_fw_paths(fwctx->alt_paths); + ret = brcmf_fw_complete_request(fw, fwctx); while (ret == 0 && ++fwctx->curpos < fwctx->req->n_items) { @@ -662,13 +687,27 @@ static void brcmf_fw_request_done_alt_path(const struct firmware *fw, void *ctx) struct brcmf_fw_item *first = &fwctx->req->items[0]; int ret = 0; - /* Fall back to canonical path if board firmware not found */ - if (!fw) + if (fw) { + brcmf_fw_request_done(fw, ctx); + return; + } + + fwctx->alt_index++; + if (fwctx->alt_index < BRCMF_FW_MAX_ALT_PATHS && + fwctx->alt_paths[fwctx->alt_index]) { + /* Try the next alt firmware */ + ret = request_firmware_nowait(THIS_MODULE, true, + fwctx->alt_paths[fwctx->alt_index], + fwctx->dev, GFP_KERNEL, fwctx, + brcmf_fw_request_done_alt_path); + } else { + /* Fall back to canonical path if board firmware not found */ ret = request_firmware_nowait(THIS_MODULE, true, first->path, fwctx->dev, GFP_KERNEL, fwctx, brcmf_fw_request_done); + } - if (fw || ret < 0) + if (ret < 0) brcmf_fw_request_done(fw, ctx); } @@ -693,7 +732,6 @@ int brcmf_fw_get_firmwares(struct device *dev, struct brcmf_fw_request *req, { struct brcmf_fw_item *first = &req->items[0]; struct brcmf_fw *fwctx; - char *alt_path; int ret; brcmf_dbg(TRACE, "enter: dev=%s\n", dev_name(dev)); @@ -712,12 +750,13 @@ int brcmf_fw_get_firmwares(struct device *dev, struct brcmf_fw_request *req, fwctx->done = fw_cb; /* First try alternative board-specific path if any */ - alt_path = brcm_alt_fw_path(first->path, fwctx->req->board_type); - if (alt_path) { - ret = request_firmware_nowait(THIS_MODULE, true, alt_path, + if (brcm_alt_fw_paths(first->path, req->board_type, + fwctx->alt_paths) == 0) { + fwctx->alt_index = 0; + ret = request_firmware_nowait(THIS_MODULE, true, + fwctx->alt_paths[0], fwctx->dev, GFP_KERNEL, fwctx, brcmf_fw_request_done_alt_path); - kfree(alt_path); } else { ret = request_firmware_nowait(THIS_MODULE, true, first->path, fwctx->dev, GFP_KERNEL, fwctx, diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h index e290dec9c53d..7f4e6e359c82 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h @@ -11,6 +11,8 @@ #define BRCMF_FW_DEFAULT_PATH "brcm/" +#define BRCMF_FW_MAX_ALT_PATHS 8 + /** * struct brcmf_firmware_mapping - Used to map chipid/revmask to firmware * filename and nvram filename. Each bus type implementation should create