| Message ID | 20211226153624.162281-4-marcan@marcan.st |
|---|---|
| State | New |
| Headers | show |
| Series | brcmfmac: Support Apple T2 and M1 platforms | expand |
26.12.2021 18:35, 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. > > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > .../broadcom/brcm80211/brcmfmac/firmware.c | 73 ++++++++++++++----- > 1 file changed, 55 insertions(+), 18 deletions(-) ... > -static char *brcm_alt_fw_path(const char *path, const char *board_type) > +static const char **brcm_alt_fw_paths(const char *path, const char *board_type) ... > static int brcmf_fw_request_firmware(const struct firmware **fw, > struct brcmf_fw *fwctx) > { > struct brcmf_fw_item *cur = &fwctx->req->items[fwctx->curpos]; > - int ret; > + int ret, i; > > /* Files can be board-specific, first try a board-specific path */ > if (cur->type == BRCMF_FW_TYPE_NVRAM && fwctx->req->board_type) { > - char *alt_path; > + const char **alt_paths = brcm_alt_fw_paths(cur->path, fwctx); The brcm_alt_fw_paths() takes "board_type" argument, while you're passing the "fwctx" to it. This patch doesn't compile. If this code is changed by a further patch, then please use "git rebase --exec" to compile-test all the patches. drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c: In function ‘brcmf_fw_request_firmware’: drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c:642:71: error: passing argument 2 of ‘brcm_alt_fw_paths’ from incompatible pointer type [-Werror=incompatible-pointer-types] 642 | const char **alt_paths = brcm_alt_fw_paths(cur->path, fwctx); | ^~~~~ | | | struct brcmf_fw * drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c:597:69: note: expected ‘const char *’ but argument is of type ‘struct brcmf_fw *’ 597 | static const char **brcm_alt_fw_paths(const char *path, const char *board_type) | ~~~~~~~~~~~~^~~~~~~~~~ drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c: In function ‘brcmf_fw_get_firmwares’: drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c:752:59: error: passing argument 2 of ‘brcm_alt_fw_paths’ from incompatible pointer type [-Werror=incompatible-pointer-types] 752 | fwctx->alt_paths = brcm_alt_fw_paths(first->path, fwctx); | ^~~~~ | | | struct brcmf_fw * drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c:597:69: note: expected ‘const char *’ but argument is of type ‘struct brcmf_fw *’ 597 | static const char **brcm_alt_fw_paths(const char *path, const char *board_type) | ~~~~~~~~~~~~^~~~~~~~~~ cc1: some warnings being treated as errors
26.12.2021 18:35, Hector Martin пишет: > -static char *brcm_alt_fw_path(const char *path, const char *board_type) > +static const char **brcm_alt_fw_paths(const char *path, const char *board_type) > { > char alt_path[BRCMF_FW_NAME_LEN]; > + char **alt_paths; > char suffix[5]; > > strscpy(alt_path, path, BRCMF_FW_NAME_LEN); > @@ -609,27 +612,46 @@ 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 = kzalloc(sizeof(char *) * 2, GFP_KERNEL); array_size()? > + alt_paths[0] = kstrdup(alt_path, GFP_KERNEL); > + > + return (const char **)alt_paths; Why this casting is needed? > +}
26.12.2021 18:35, Hector Martin пишет: > struct brcmf_fw { > struct device *dev; > struct brcmf_fw_request *req; > + const char **alt_paths; > + int alt_index; ... > +static void brcm_free_alt_fw_paths(const char **alt_paths) > +{ > + int i; ... > static int brcmf_fw_request_firmware(const struct firmware **fw, > struct brcmf_fw *fwctx) > { > struct brcmf_fw_item *cur = &fwctx->req->items[fwctx->curpos]; > - int ret; > + int ret, i; unsigned int
02.01.2022 08:31, Linus Walleij пишет: > On Sun, Dec 26, 2021 at 4:37 PM Hector Martin <marcan@marcan.st> 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. >> >> Signed-off-by: Hector Martin <marcan@marcan.st> > > This looks OK to me so FWIW: > Acked-by: Linus Walleij <linus.walleij@linaro.org> > > Make sure Dmitry Osipenko gets to review this though, he has many > valuable insights about how the FW is loaded and helped me out a > lot when I patched this. Thanks, Linus. I took a brief look at the patch and may give it a test next time, once it will compile without errors and warnings.
On 2022/01/02 16:08, Dmitry Osipenko wrote: > 26.12.2021 18:35, Hector Martin пишет: >> +static void brcm_free_alt_fw_paths(const char **alt_paths) >> +{ >> + int i; >> + >> + if (!alt_paths) >> + return; >> + >> + for (i = 0; alt_paths[i]; i++) >> + kfree(alt_paths[i]); >> + >> + kfree(alt_paths); >> } >> >> static int brcmf_fw_request_firmware(const struct firmware **fw, >> struct brcmf_fw *fwctx) >> { >> struct brcmf_fw_item *cur = &fwctx->req->items[fwctx->curpos]; >> - int ret; >> + int ret, i; >> >> /* Files can be board-specific, first try a board-specific path */ >> if (cur->type == BRCMF_FW_TYPE_NVRAM && fwctx->req->board_type) { >> - char *alt_path; >> + const char **alt_paths = brcm_alt_fw_paths(cur->path, fwctx); >> >> - alt_path = brcm_alt_fw_path(cur->path, fwctx->req->board_type); >> - if (!alt_path) >> + if (!alt_paths) >> goto fallback; >> >> - ret = request_firmware(fw, alt_path, fwctx->dev); >> - kfree(alt_path); >> - if (ret == 0) >> - return ret; >> + for (i = 0; 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 +663,9 @@ 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); >> + fwctx->alt_paths = NULL; > > It looks suspicious that fwctx->alt_paths isn't zero'ed by other code > paths. The brcm_free_alt_fw_paths() should take fwctx for the argument > and fwctx->alt_paths should be set to NULL there. There are multiple code paths for alt_paths; the initial firmware lookup uses fwctx->alt_paths, and once we know the firmware load succeeded we use blocking firmware requests for NVRAM/CLM/etc and those do not use the fwctx member, but rather just keep alt_paths in function scope (brcmf_fw_request_firmware). You're right that there was a rebase SNAFU there though, I'll compile test each patch before sending v2. Sorry about that. In this series the code should build again by patch #6. Are you thinking of any particular code paths? As far as I saw when writing this, brcmf_fw_request_done() should always get called whether things succeed or fail. There are no other code paths that free fwctx->alt_paths. > On the other hand, I'd change the **alt_paths to a fixed-size array. > This should simplify the code, making it easier to follow and maintain. > > - const char **alt_paths; > + char *alt_paths[BRCM_MAX_ALT_FW_PATHS]; > > Then you also won't need to NULL-terminate the array, which is a common > source of bugs in kernel. That sounds reasonable, it'll certainly make the code simpler. I'll do that for v2.
02.01.2022 17:18, Hector Martin пишет: > On 2022/01/02 15:45, Dmitry Osipenko wrote: >> 26.12.2021 18:35, Hector Martin пишет: >>> -static char *brcm_alt_fw_path(const char *path, const char *board_type) >>> +static const char **brcm_alt_fw_paths(const char *path, const char *board_type) >>> { >>> char alt_path[BRCMF_FW_NAME_LEN]; >>> + char **alt_paths; >>> char suffix[5]; >>> >>> strscpy(alt_path, path, BRCMF_FW_NAME_LEN); >>> @@ -609,27 +612,46 @@ 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 = kzalloc(sizeof(char *) * 2, GFP_KERNEL); >> >> array_size()? > > Of what array? array_size(sizeof(*alt_paths), 2) >>> + alt_paths[0] = kstrdup(alt_path, GFP_KERNEL); >>> + >>> + return (const char **)alt_paths; >> >> Why this casting is needed? > > Because implicit conversion from char ** to const char ** is not legal > in C, as that could cause const unsoundness if you do this: > > char *foo[1]; > const char **bar = foo; > > bar[0] = "constant string"; > foo[0][0] = '!'; // clobbers constant string It's up to a programmer to decide what is right to do. C gives you flexibility, meanwhile it's easy to shoot yourself in the foot if you won't be careful. > But it's fine in this case since the non-const pointer disappears so > nothing can ever write through it again. > There is indeed no need for the castings in such cases, it's a typical code pattern in kernel. You would need to do the casting for the other way around, i.e. if char ** was returned and **alt_paths was a const.
03.01.2022 03:41, Hector Martin пишет: >> There is indeed no need for the castings in such cases, it's a typical >> code pattern in kernel. You would need to do the casting for the other >> way around, i.e. if char ** was returned and **alt_paths was a const. > You do need to do the cast. Try it. > > $ cat test.c > int main() { > char *foo[1]; > const char **bar = foo; > > return 0; > } > > $ gcc test.c > test.c: In function ‘main’: > test.c:4:28: warning: initialization of ‘const char **’ from > incompatible pointer type ‘char **’ [-Wincompatible-pointer-types] > 4 | const char **bar = foo; > | > > You can implicitly cast char* to const char*, but you *cannot* > impliclicitly cast char** to const char** for the reason I explained. It > requires a cast. Right, I read it as "char * const *". The "const char **" vs "char * const *" always confuses me. Hence you should've written "const char **alt_paths;" in brcm_alt_fw_paths() in the first place and then casting wouldn't have been needed.
On 2022/01/02 15:55, Dmitry Osipenko wrote: > 26.12.2021 18:35, Hector Martin пишет: >> struct brcmf_fw { >> struct device *dev; >> struct brcmf_fw_request *req; >> + const char **alt_paths; > >> + int alt_index; > ... >> +static void brcm_free_alt_fw_paths(const char **alt_paths) >> +{ >> + int i; > ... >> static int brcmf_fw_request_firmware(const struct firmware **fw, >> struct brcmf_fw *fwctx) >> { >> struct brcmf_fw_item *cur = &fwctx->req->items[fwctx->curpos]; >> - int ret; >> + int ret, i; > > unsigned int > Thanks, changed for v2!
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c index 0eb13e5df517..cc97cd1da44d 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; + int alt_index; u32 curpos; void (*done)(struct device *dev, int err, struct brcmf_fw_request *req); }; @@ -592,9 +594,10 @@ 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 const char **brcm_alt_fw_paths(const char *path, const char *board_type) { char alt_path[BRCMF_FW_NAME_LEN]; + char **alt_paths; char suffix[5]; strscpy(alt_path, path, BRCMF_FW_NAME_LEN); @@ -609,27 +612,46 @@ 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 = kzalloc(sizeof(char *) * 2, GFP_KERNEL); + alt_paths[0] = kstrdup(alt_path, GFP_KERNEL); + + return (const char **)alt_paths; +} + +static void brcm_free_alt_fw_paths(const char **alt_paths) +{ + int i; + + if (!alt_paths) + return; + + for (i = 0; alt_paths[i]; i++) + kfree(alt_paths[i]); + + kfree(alt_paths); } static int brcmf_fw_request_firmware(const struct firmware **fw, struct brcmf_fw *fwctx) { struct brcmf_fw_item *cur = &fwctx->req->items[fwctx->curpos]; - int ret; + int ret, i; /* Files can be board-specific, first try a board-specific path */ if (cur->type == BRCMF_FW_TYPE_NVRAM && fwctx->req->board_type) { - char *alt_path; + const char **alt_paths = brcm_alt_fw_paths(cur->path, fwctx); - alt_path = brcm_alt_fw_path(cur->path, fwctx->req->board_type); - if (!alt_path) + if (!alt_paths) goto fallback; - ret = request_firmware(fw, alt_path, fwctx->dev); - kfree(alt_path); - if (ret == 0) - return ret; + for (i = 0; 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 +663,9 @@ 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); + fwctx->alt_paths = NULL; + ret = brcmf_fw_complete_request(fw, fwctx); while (ret == 0 && ++fwctx->curpos < fwctx->req->n_items) { @@ -662,13 +687,26 @@ 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_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 +731,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 +749,12 @@ 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, + fwctx->alt_paths = brcm_alt_fw_paths(first->path, fwctx); + if (fwctx->alt_paths) { + 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,
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. Signed-off-by: Hector Martin <marcan@marcan.st> --- .../broadcom/brcm80211/brcmfmac/firmware.c | 73 ++++++++++++++----- 1 file changed, 55 insertions(+), 18 deletions(-)