diff mbox series

[03/34] brcmfmac: firmware: Support having multiple alt paths

Message ID 20211226153624.162281-4-marcan@marcan.st (mailing list archive)
State Superseded
Delegated to: Kalle Valo
Headers show
Series brcmfmac: Support Apple T2 and M1 platforms | expand

Commit Message

Hector Martin Dec. 26, 2021, 3:35 p.m. UTC
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(-)

Comments

Linus Walleij Jan. 2, 2022, 5:31 a.m. UTC | #1
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.

Yours,
Linus Walleij
Dmitry Osipenko Jan. 2, 2022, 6:38 a.m. UTC | #2
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
Dmitry Osipenko Jan. 2, 2022, 6:45 a.m. UTC | #3
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?

> +}
Dmitry Osipenko Jan. 2, 2022, 6:55 a.m. UTC | #4
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
Dmitry Osipenko Jan. 2, 2022, 7:08 a.m. UTC | #5
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.

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.
Dmitry Osipenko Jan. 2, 2022, 7:10 a.m. UTC | #6
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.
Dmitry Osipenko Jan. 2, 2022, 7:20 a.m. UTC | #7
02.01.2022 10:08, Dmitry Osipenko пишет:
> 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.
> 
> 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];

Although, the const should be kept.

const 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.
>
Hector Martin Jan. 2, 2022, 2:18 p.m. UTC | #8
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?

> 
>> +	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

But it's fine in this case since the non-const pointer disappears so
nothing can ever write through it again.
Hector Martin Jan. 2, 2022, 2:25 p.m. UTC | #9
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.
Dmitry Osipenko Jan. 2, 2022, 8:11 p.m. UTC | #10
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.
Dmitry Osipenko Jan. 2, 2022, 8:12 p.m. UTC | #11
02.01.2022 17:25, Hector Martin пишет:
> 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.

It should be okay in the particular case then. But this is not obvious
without taking a closer look at the code, which is a sign that there is
some room for improvement.

>> 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.

Feel free to CC me on v2. I'll take a closer look and give a test to the
patches on older hardware, checking for regressions.
Hector Martin Jan. 3, 2022, 12:41 a.m. UTC | #12
On 03/01/2022 05.11, Dmitry Osipenko wrote:
> 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)

Heh, TIL. I thought you meant ARRAY_SIZE. First time I see the lowercase
macro. That's a confusing name collision...

>>>> +	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.

Which is why that conversion is illegal without a cast and you need to
explicitly choose to shoot yourself in the foot :-)

>> 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.

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.
Dmitry Osipenko Jan. 3, 2022, 1:26 a.m. UTC | #13
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.
Hector Martin Jan. 3, 2022, 6:17 a.m. UTC | #14
On 2022/01/03 10:26, Dmitry Osipenko wrote:
> 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.

Sure, in this case that works since the string is just strduped and
never mutated. Either way this will change to an argument instead of a
return value, since I'll change it to be statically sized as you said
and allocated by the caller (or in the struct).
Hector Martin Jan. 3, 2022, 6:18 a.m. UTC | #15
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 mbox series

Patch

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,