| Message ID | 20220104072658.69756-7-marcan@marcan.st |
|---|---|
| State | New |
| Headers | show |
| Series | brcmfmac: Support Apple T2 and M1 platforms | expand |
On 1/4/2022 8:26 AM, Hector Martin wrote: > In order to make use of the multiple alt_path functionality, change > board_type to an array. Bus drivers can pass in a NULL-terminated list > of board type strings to try for the firmware fetch. 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 | 35 ++++++++++++------- > .../broadcom/brcm80211/brcmfmac/firmware.h | 2 +- > .../broadcom/brcm80211/brcmfmac/pcie.c | 4 ++- > .../broadcom/brcm80211/brcmfmac/sdio.c | 2 +- > 4 files changed, 27 insertions(+), 16 deletions(-) > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c > index 7570dbf22cdd..054ea3ed133e 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c > @@ -594,28 +594,39 @@ static int brcmf_fw_complete_request(const struct firmware *fw, > return (cur->flags & BRCMF_FW_REQF_OPTIONAL) ? 0 : ret; > } > > -static int brcm_alt_fw_paths(const char *path, const char *board_type, > +static int brcm_alt_fw_paths(const char *path, struct brcmf_fw *fwctx, > const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS]) > { > + const char **board_types = fwctx->req->board_types; > + unsigned int i; > char alt_path[BRCMF_FW_NAME_LEN]; > const char *suffix; [...] > + for (i = 0; i < BRCMF_FW_MAX_ALT_PATHS; i++) { > + if (!board_types[i]) > + break; > > - strlcat(alt_path, ".", BRCMF_FW_NAME_LEN); > - strlcat(alt_path, board_type, BRCMF_FW_NAME_LEN); > - strlcat(alt_path, suffix, BRCMF_FW_NAME_LEN); > + /* strip extension at the end */ > + strscpy(alt_path, path, BRCMF_FW_NAME_LEN); > + alt_path[suffix - path] = 0; > > - alt_paths[0] = kstrdup(alt_path, GFP_KERNEL); > + strlcat(alt_path, ".", BRCMF_FW_NAME_LEN); > + strlcat(alt_path, board_types[i], BRCMF_FW_NAME_LEN); > + strlcat(alt_path, suffix, BRCMF_FW_NAME_LEN); > + > + alt_paths[i] = kstrdup(alt_path, GFP_KERNEL); > + brcmf_dbg(TRACE, "FW alt path: %s\n", alt_paths[i]); Could use alt_path in the debug print thus avoiding additional array access (working hard to find those nits to pick ;-) ). > + }
On 2022/01/04 20:28, Andy Shevchenko wrote: > On Tue, Jan 4, 2022 at 9:28 AM Hector Martin <marcan@marcan.st> wrote: >> >> In order to make use of the multiple alt_path functionality, change >> board_type to an array. Bus drivers can pass in a NULL-terminated list >> of board type strings to try for the firmware fetch. > >> + /* strip extension at the end */ >> + strscpy(alt_path, path, BRCMF_FW_NAME_LEN); >> + alt_path[suffix - path] = 0; >> >> - alt_paths[0] = kstrdup(alt_path, GFP_KERNEL); >> + strlcat(alt_path, ".", BRCMF_FW_NAME_LEN); >> + strlcat(alt_path, board_types[i], BRCMF_FW_NAME_LEN); >> + strlcat(alt_path, suffix, BRCMF_FW_NAME_LEN); >> + >> + alt_paths[i] = kstrdup(alt_path, GFP_KERNEL); >> + brcmf_dbg(TRACE, "FW alt path: %s\n", alt_paths[i]); > > Consider replacing these string manipulations with kasprintf(). > Done, thanks!
On 2022/01/06 21:16, Arend van Spriel wrote: > On 1/4/2022 8:26 AM, Hector Martin wrote: >> In order to make use of the multiple alt_path functionality, change >> board_type to an array. Bus drivers can pass in a NULL-terminated list >> of board type strings to try for the firmware fetch. > > 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 | 35 ++++++++++++------- >> .../broadcom/brcm80211/brcmfmac/firmware.h | 2 +- >> .../broadcom/brcm80211/brcmfmac/pcie.c | 4 ++- >> .../broadcom/brcm80211/brcmfmac/sdio.c | 2 +- >> 4 files changed, 27 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c >> index 7570dbf22cdd..054ea3ed133e 100644 >> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c >> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c >> @@ -594,28 +594,39 @@ static int brcmf_fw_complete_request(const struct firmware *fw, >> return (cur->flags & BRCMF_FW_REQF_OPTIONAL) ? 0 : ret; >> } >> >> -static int brcm_alt_fw_paths(const char *path, const char *board_type, >> +static int brcm_alt_fw_paths(const char *path, struct brcmf_fw *fwctx, >> const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS]) >> { >> + const char **board_types = fwctx->req->board_types; >> + unsigned int i; >> char alt_path[BRCMF_FW_NAME_LEN]; >> const char *suffix; > > [...] > >> + for (i = 0; i < BRCMF_FW_MAX_ALT_PATHS; i++) { >> + if (!board_types[i]) >> + break; >> >> - strlcat(alt_path, ".", BRCMF_FW_NAME_LEN); >> - strlcat(alt_path, board_type, BRCMF_FW_NAME_LEN); >> - strlcat(alt_path, suffix, BRCMF_FW_NAME_LEN); >> + /* strip extension at the end */ >> + strscpy(alt_path, path, BRCMF_FW_NAME_LEN); >> + alt_path[suffix - path] = 0; >> >> - alt_paths[0] = kstrdup(alt_path, GFP_KERNEL); >> + strlcat(alt_path, ".", BRCMF_FW_NAME_LEN); >> + strlcat(alt_path, board_types[i], BRCMF_FW_NAME_LEN); >> + strlcat(alt_path, suffix, BRCMF_FW_NAME_LEN); >> + >> + alt_paths[i] = kstrdup(alt_path, GFP_KERNEL); >> + brcmf_dbg(TRACE, "FW alt path: %s\n", alt_paths[i]); > > Could use alt_path in the debug print thus avoiding additional array > access (working hard to find those nits to pick ;-) ). > So you're saying my code is so good you have to resort to nits on this level to make it clear you read it, right? ;-)
On January 7, 2022 5:02:13 AM Hector Martin <marcan@marcan.st> wrote: > On 2022/01/06 21:16, Arend van Spriel wrote: >> On 1/4/2022 8:26 AM, Hector Martin wrote: >>> In order to make use of the multiple alt_path functionality, change >>> board_type to an array. Bus drivers can pass in a NULL-terminated list >>> of board type strings to try for the firmware fetch. >> >> 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 | 35 ++++++++++++------- >>> .../broadcom/brcm80211/brcmfmac/firmware.h | 2 +- >>> .../broadcom/brcm80211/brcmfmac/pcie.c | 4 ++- >>> .../broadcom/brcm80211/brcmfmac/sdio.c | 2 +- >>> 4 files changed, 27 insertions(+), 16 deletions(-) >>> >>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c >>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c >>> index 7570dbf22cdd..054ea3ed133e 100644 >>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c >>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c >>> @@ -594,28 +594,39 @@ static int brcmf_fw_complete_request(const struct >>> firmware *fw, >>> return (cur->flags & BRCMF_FW_REQF_OPTIONAL) ? 0 : ret; >>> } >>> >>> -static int brcm_alt_fw_paths(const char *path, const char *board_type, >>> +static int brcm_alt_fw_paths(const char *path, struct brcmf_fw *fwctx, >>> const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS]) >>> { >>> + const char **board_types = fwctx->req->board_types; >>> + unsigned int i; >>> char alt_path[BRCMF_FW_NAME_LEN]; >>> const char *suffix; >> >> [...] >> >>> + for (i = 0; i < BRCMF_FW_MAX_ALT_PATHS; i++) { >>> + if (!board_types[i]) >>> + break; >>> >>> - strlcat(alt_path, ".", BRCMF_FW_NAME_LEN); >>> - strlcat(alt_path, board_type, BRCMF_FW_NAME_LEN); >>> - strlcat(alt_path, suffix, BRCMF_FW_NAME_LEN); >>> + /* strip extension at the end */ >>> + strscpy(alt_path, path, BRCMF_FW_NAME_LEN); >>> + alt_path[suffix - path] = 0; >>> >>> - alt_paths[0] = kstrdup(alt_path, GFP_KERNEL); >>> + strlcat(alt_path, ".", BRCMF_FW_NAME_LEN); >>> + strlcat(alt_path, board_types[i], BRCMF_FW_NAME_LEN); >>> + strlcat(alt_path, suffix, BRCMF_FW_NAME_LEN); >>> + >>> + alt_paths[i] = kstrdup(alt_path, GFP_KERNEL); >>> + brcmf_dbg(TRACE, "FW alt path: %s\n", alt_paths[i]); >> >> Could use alt_path in the debug print thus avoiding additional array >> access (working hard to find those nits to pick ;-) ). > > So you're saying my code is so good you have to resort to nits on this > level to make it clear you read it, right? ;-) Don't read too much into this :-p Actually never liked the alt_path approach, but didn't come up with a better solution. Regards, Arend
On 2022/01/07 15:17, Arend Van Spriel wrote: > On January 7, 2022 5:02:13 AM Hector Martin <marcan@marcan.st> wrote: > >> On 2022/01/06 21:16, Arend van Spriel wrote: >>> On 1/4/2022 8:26 AM, Hector Martin wrote: >>>> In order to make use of the multiple alt_path functionality, change >>>> board_type to an array. Bus drivers can pass in a NULL-terminated list >>>> of board type strings to try for the firmware fetch. >>> >>> 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 | 35 ++++++++++++------- >>>> .../broadcom/brcm80211/brcmfmac/firmware.h | 2 +- >>>> .../broadcom/brcm80211/brcmfmac/pcie.c | 4 ++- >>>> .../broadcom/brcm80211/brcmfmac/sdio.c | 2 +- >>>> 4 files changed, 27 insertions(+), 16 deletions(-) >>>> >>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c >>>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c >>>> index 7570dbf22cdd..054ea3ed133e 100644 >>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c >>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c >>>> @@ -594,28 +594,39 @@ static int brcmf_fw_complete_request(const struct >>>> firmware *fw, >>>> return (cur->flags & BRCMF_FW_REQF_OPTIONAL) ? 0 : ret; >>>> } >>>> >>>> -static int brcm_alt_fw_paths(const char *path, const char *board_type, >>>> +static int brcm_alt_fw_paths(const char *path, struct brcmf_fw *fwctx, >>>> const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS]) >>>> { >>>> + const char **board_types = fwctx->req->board_types; >>>> + unsigned int i; >>>> char alt_path[BRCMF_FW_NAME_LEN]; >>>> const char *suffix; >>> >>> [...] >>> >>>> + for (i = 0; i < BRCMF_FW_MAX_ALT_PATHS; i++) { >>>> + if (!board_types[i]) >>>> + break; >>>> >>>> - strlcat(alt_path, ".", BRCMF_FW_NAME_LEN); >>>> - strlcat(alt_path, board_type, BRCMF_FW_NAME_LEN); >>>> - strlcat(alt_path, suffix, BRCMF_FW_NAME_LEN); >>>> + /* strip extension at the end */ >>>> + strscpy(alt_path, path, BRCMF_FW_NAME_LEN); >>>> + alt_path[suffix - path] = 0; >>>> >>>> - alt_paths[0] = kstrdup(alt_path, GFP_KERNEL); >>>> + strlcat(alt_path, ".", BRCMF_FW_NAME_LEN); >>>> + strlcat(alt_path, board_types[i], BRCMF_FW_NAME_LEN); >>>> + strlcat(alt_path, suffix, BRCMF_FW_NAME_LEN); >>>> + >>>> + alt_paths[i] = kstrdup(alt_path, GFP_KERNEL); >>>> + brcmf_dbg(TRACE, "FW alt path: %s\n", alt_paths[i]); >>> >>> Could use alt_path in the debug print thus avoiding additional array >>> access (working hard to find those nits to pick ;-) ). >> >> So you're saying my code is so good you have to resort to nits on this >> level to make it clear you read it, right? ;-) > > Don't read too much into this :-p Actually never liked the alt_path > approach, but didn't come up with a better solution. Yeah, it's not the prettiest approach. I redid this part of the patchset for v3 though, as I mentioned to Dmitry. Now I just iterate over board_types, which ends up being a lot cleaner as far as the changes required.
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c index 7570dbf22cdd..054ea3ed133e 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c @@ -594,28 +594,39 @@ static int brcmf_fw_complete_request(const struct firmware *fw, return (cur->flags & BRCMF_FW_REQF_OPTIONAL) ? 0 : ret; } -static int brcm_alt_fw_paths(const char *path, const char *board_type, +static int brcm_alt_fw_paths(const char *path, struct brcmf_fw *fwctx, const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS]) { + const char **board_types = fwctx->req->board_types; + unsigned int i; char alt_path[BRCMF_FW_NAME_LEN]; const char *suffix; memset(alt_paths, 0, array_size(sizeof(*alt_paths), BRCMF_FW_MAX_ALT_PATHS)); + if (!board_types[0]) + return -ENOENT; + suffix = strrchr(path, '.'); if (!suffix || suffix == path) return -EINVAL; - /* strip extension at the end */ - strscpy(alt_path, path, BRCMF_FW_NAME_LEN); - alt_path[suffix - path] = 0; + for (i = 0; i < BRCMF_FW_MAX_ALT_PATHS; i++) { + if (!board_types[i]) + break; - strlcat(alt_path, ".", BRCMF_FW_NAME_LEN); - strlcat(alt_path, board_type, BRCMF_FW_NAME_LEN); - strlcat(alt_path, suffix, BRCMF_FW_NAME_LEN); + /* strip extension at the end */ + strscpy(alt_path, path, BRCMF_FW_NAME_LEN); + alt_path[suffix - path] = 0; - alt_paths[0] = kstrdup(alt_path, GFP_KERNEL); + strlcat(alt_path, ".", BRCMF_FW_NAME_LEN); + strlcat(alt_path, board_types[i], BRCMF_FW_NAME_LEN); + strlcat(alt_path, suffix, BRCMF_FW_NAME_LEN); + + alt_paths[i] = kstrdup(alt_path, GFP_KERNEL); + brcmf_dbg(TRACE, "FW alt path: %s\n", alt_paths[i]); + } return 0; } @@ -637,11 +648,10 @@ static int brcmf_fw_request_firmware(const struct firmware **fw, unsigned int i; /* Files can be board-specific, first try a board-specific path */ - if (fwctx->req->board_type) { + if (fwctx->req->board_types[0]) { const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS]; - if (brcm_alt_fw_paths(cur->path, fwctx->req->board_type, - alt_paths) != 0) + if (brcm_alt_fw_paths(cur->path, fwctx, alt_paths) != 0) goto fallback; for (i = 0; i < BRCMF_FW_MAX_ALT_PATHS && alt_paths[i]; i++) { @@ -750,8 +760,7 @@ 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 */ - if (brcm_alt_fw_paths(first->path, req->board_type, - fwctx->alt_paths) == 0) { + if (brcm_alt_fw_paths(first->path, fwctx, fwctx->alt_paths) == 0) { fwctx->alt_index = 0; ret = request_firmware_nowait(THIS_MODULE, true, fwctx->alt_paths[0], diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h index 7f4e6e359c82..3b60a0e290b0 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h @@ -68,7 +68,7 @@ struct brcmf_fw_request { u16 domain_nr; u16 bus_nr; u32 n_items; - const char *board_type; + const char *board_types[BRCMF_FW_MAX_ALT_PATHS]; struct brcmf_fw_item items[]; }; diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c index 591f870d1e47..a52a6f8081eb 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c @@ -1877,11 +1877,13 @@ brcmf_pcie_prepare_fw_request(struct brcmf_pciedev_info *devinfo) fwreq->items[BRCMF_PCIE_FW_NVRAM].flags = BRCMF_FW_REQF_OPTIONAL; fwreq->items[BRCMF_PCIE_FW_CLM].type = BRCMF_FW_TYPE_BINARY; fwreq->items[BRCMF_PCIE_FW_CLM].flags = BRCMF_FW_REQF_OPTIONAL; - fwreq->board_type = devinfo->settings->board_type; /* NVRAM reserves PCI domain 0 for Broadcom's SDK faked bus */ fwreq->domain_nr = pci_domain_nr(devinfo->pdev->bus) + 1; fwreq->bus_nr = devinfo->pdev->bus->number; + brcmf_dbg(PCIE, "Board: %s\n", devinfo->settings->board_type); + fwreq->board_types[0] = devinfo->settings->board_type; + return fwreq; } diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c index 7466e6fd6eca..ed944764f6ea 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c @@ -4432,7 +4432,7 @@ brcmf_sdio_prepare_fw_request(struct brcmf_sdio *bus) fwreq->items[BRCMF_SDIO_FW_NVRAM].type = BRCMF_FW_TYPE_NVRAM; fwreq->items[BRCMF_SDIO_FW_CLM].type = BRCMF_FW_TYPE_BINARY; fwreq->items[BRCMF_SDIO_FW_CLM].flags = BRCMF_FW_REQF_OPTIONAL; - fwreq->board_type = bus->sdiodev->settings->board_type; + fwreq->board_types[0] = bus->sdiodev->settings->board_type; return fwreq; }