diff mbox series

[v2,06/35] brcmfmac: firmware: Support passing in multiple board_types

Message ID 20220104072658.69756-7-marcan@marcan.st
State New
Headers show
Series brcmfmac: Support Apple T2 and M1 platforms | expand

Commit Message

Hector Martin Jan. 4, 2022, 7:26 a.m. UTC
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.

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(-)

Comments

Hector Martin Jan. 4, 2022, 10:30 a.m. UTC | #1
On 2022/01/04 19:22, 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.
>>
>> 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;
>>   
>>   	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;
> 
> Indentation error

I knew I had a feeling I was forgetting to do something... that was
running v2 through checkpatch. Sigh. Thanks for catching that, fixed :)
Andy Shevchenko Jan. 4, 2022, 11:28 a.m. UTC | #2
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().
Arend van Spriel Jan. 6, 2022, 12:16 p.m. UTC | #3
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 ;-) ).

> +	}
Hector Martin Jan. 7, 2022, 4:02 a.m. UTC | #4
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? ;-)
Hector Martin Jan. 7, 2022, 7:12 a.m. UTC | #5
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 mbox series

Patch

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;
 }