[RFC PATCH 00/34] brcmfmac: Support Apple T2 and M1 platforms

On Sun, Dec 26, 2021 at 4:37 PM Hector Martin [off-list ref] wrote:

Teach brcm_alt_fw_paths to correctly split off variable length
extensions, and enable alt firmware lookups for the CLM blob firmware
requests.

Apple platforms have per-board CLM blob files.

Signed-off-by: Hector Martin <redacted>

Acked-by: Linus Walleij <redacted>

Yours,
Linus Walleij

On Sun, Dec 26, 2021 at 4:36 PM Hector Martin [off-list ref] wrote:

Merry Christmas! This year Santa brings us a 34-patch series to add
proper support for the Broadcom FullMAC chips used on Apple T2 and M1
platforms:

I tried to review as best I could, when I think I know what I'm doing I state
Reviewed-by and when I think it just LooksGoodToMe(TM) I replied
Acked-by. If I missed some patch you can assume Acked-by from me
on these as well.

Thanks for doing this, some really old bugs and code improvements long
overdue is in the series, much appreciated.

Yours,
Linus Walleij

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 <redacted>
---
 .../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)

...

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 пишет:

array_size()?

Why this casting is needed?

+}

26.12.2021 18:35, Hector Martin пишет:

+	int alt_index;

...

...

unsigned int

26.12.2021 18:35, Hector Martin пишет:

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.

02.01.2022 08:31, Linus Walleij пишет:

On Sun, Dec 26, 2021 at 4:37 PM Hector Martin [off-list ref] wrote:

quoted

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

This looks OK to me so FWIW:
Acked-by: Linus Walleij <redacted>

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.

02.01.2022 10:08, Dmitry Osipenko пишет:

26.12.2021 18:35, Hector Martin пишет:

quoted

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

On 2022/01/02 15:45, Dmitry Osipenko wrote:

26.12.2021 18:35, Hector Martin пишет:

quoted

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

quoted

+	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 (marcan@marcan.st)
Public Key: https://mrcn.st/pub

On 2022/01/02 16:08, Dmitry Osipenko wrote:

26.12.2021 18:35, Hector Martin пишет:

quoted

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


-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub

02.01.2022 17:18, Hector Martin пишет:

On 2022/01/02 15:45, Dmitry Osipenko wrote:

quoted

26.12.2021 18:35, Hector Martin пишет:

quoted

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

quoted

quoted

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

02.01.2022 17:25, Hector Martin пишет:

On 2022/01/02 16:08, Dmitry Osipenko wrote:

quoted

26.12.2021 18:35, Hector Martin пишет:

quoted

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

quoted

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.

On 03/01/2022 05.11, Dmitry Osipenko wrote:

02.01.2022 17:18, Hector Martin пишет:

quoted

On 2022/01/02 15:45, Dmitry Osipenko wrote:

quoted

26.12.2021 18:35, Hector Martin пишет:

quoted

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

quoted

quoted

quoted

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

quoted

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.

-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub

03.01.2022 03:41, Hector Martin пишет:

quoted

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 14:50, Linus Walleij wrote:

On Sun, Dec 26, 2021 at 4:37 PM Hector Martin [off-list ref] wrote:

quoted

On Device Tree platforms, it is customary to be able to set the MAC
address via the Device Tree, as it is often stored in system firmware.
This is particularly relevant for Apple ARM64 platforms, where this
information comes from system configuration and passed through by the
bootloader into the DT.

Implement support for this by fetching the platform MAC address and
adding or replacing the macaddr= property in nvram. This becomes the
dongle's default MAC address.

On platforms with an SROM MAC address, this overrides it. On platforms
without one, such as Apple ARM64 devices, this is required for the
firmware to boot (it will fail if it does not have a valid MAC at all).

Signed-off-by: Hector Martin <redacted>

This looks very helpful.

quoted

+       /* Add space for properties we may add */
+       size += strlen(BRCMF_FW_DEFAULT_BOARDREV) + 1;
+       size += BRCMF_FW_MACADDR_LEN + 1;

Add some note to the commit log why you also make space for
boardrev? (Looks useful.) Is the boardrev spacing in the right
patch?

Ah, that was a drive-by fix. While adding the MACADDR space I noticed we
weren't allocating space for BOARDREV... not sure if any platforms hit
this; it would cause an overflow if there are platforms with no
board_rev in the nvram that also don't have enough comments/junk to
otherwise make space for it.

I'll move it to another patch so it's more evident, and it should get a
Fixes: too.

-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub

On 2022/01/02 14:38, Linus Walleij wrote:

On Sun, Dec 26, 2021 at 4:37 PM Hector Martin [off-list ref] wrote:

quoted

On Apple platforms, the One Time Programmable ROM in the Broadcom chips
contains information about the specific board design (module, vendor,
version) that is required to select the correct NVRAM file. Parse this
OTP ROM and extract the required strings.

Note that the user OTP offset/size is per-chip. This patch does not add
any chips yet.

Signed-off-by: Hector Martin <redacted>

Overall looks fine!

quoted

+       const char *chip_params;
+       const char *module_params;

This variable name "module_params" is a bit confusing since loadable
kernel modules have params...

Can we think of another name and just put a comment that this
refers to the WiFi module building block?

Sometimes people talk about SoM:s (system-on-modules), so
maybe som_params or brcm_som_params?

Yours,
Linus Walleij

How about board_params, since we're already calling those things boards
elsewhere in the driver? That could refer to the board of a standalone
module, or an integrated board, which should cover all cases.

-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub

On 2022/01/02 14:58, Linus Walleij wrote:

On Sun, Dec 26, 2021 at 4:38 PM Hector Martin [off-list ref] wrote:

quoted

On DT platforms, the module-instance and antenna-sku-info properties
are passed in the DT. On ACPI platforms, module-instance is passed via
the analogous Apple device property mechanism, while the antenna SKU
info is instead obtained via an ACPI method that grabs it from
non-volatile storage.

Add support for this, to allow proper firmware selection on Apple
platforms.

Signed-off-by: Hector Martin <redacted>

If the strings treated here are exactly the same as for the device tree,
you should be able to just use "devprops" (firmware node) to handle it
abstractly, and then the respective DT and ACPI backend will provide
the properties.

I don't know if this patch I made recently is enough of an examples:
https://lore.kernel.org/linux-hwmon/20211206020423.62402-2-linus.walleij@linaro.org/

If the ACPI and DT differs a lot in format and strings etc it may not
be worth it.

It's not quite the same; module-instance is the same from macOS'
perspective, but we don't use Apple's device tree directly but rather
roll our own DT which uses a different property name in this case.
antenna-sku-info uses an ACPI method on x86, so that one is completely
different. So in the end nothing is actually shared.

-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub

On 2022/01/03 10:26, Dmitry Osipenko wrote:

03.01.2022 03:41, Hector Martin пишет:

quoted

quoted

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 (marcan@marcan.st)
Public Key: https://mrcn.st/pub

On 2022/01/02 15:55, Dmitry Osipenko wrote:

26.12.2021 18:35, Hector Martin пишет:

quoted

 struct brcmf_fw {
 	struct device *dev;
 	struct brcmf_fw_request *req;
+	const char **alt_paths;

quoted

+	int alt_index;

...

quoted

+static void brcm_free_alt_fw_paths(const char **alt_paths)
+{
+	int i;

...

quoted

 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!

-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub

On 2022/01/02 15:25, Linus Walleij wrote:

On Sun, Dec 26, 2021 at 4:36 PM Hector Martin [off-list ref] wrote:

quoted

Merry Christmas! This year Santa brings us a 34-patch series to add
proper support for the Broadcom FullMAC chips used on Apple T2 and M1
platforms:

I tried to review as best I could, when I think I know what I'm doing I state
Reviewed-by and when I think it just LooksGoodToMe(TM) I replied
Acked-by. If I missed some patch you can assume Acked-by from me
on these as well.

Thanks for doing this, some really old bugs and code improvements long
overdue is in the series, much appreciated.

Yours,
Linus Walleij

Thanks for the comprehensive review! I'm glad this all makes some sense
and I'm not crazy about the approach :)

I'll wait a bit for any other feedback that might come in and then
submit a v2 with the fixes/changes mentioned so far.

Cheers,
-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub

On 1/3/2022 7:27 AM, Hector Martin wrote:

On 2022/01/02 15:25, Linus Walleij wrote:

quoted

On Sun, Dec 26, 2021 at 4:36 PM Hector Martin [off-list ref] wrote:

quoted

Merry Christmas! This year Santa brings us a 34-patch series to add
proper support for the Broadcom FullMAC chips used on Apple T2 and M1
platforms:

I tried to review as best I could, when I think I know what I'm doing I state
Reviewed-by and when I think it just LooksGoodToMe(TM) I replied
Acked-by. If I missed some patch you can assume Acked-by from me
on these as well.

Thanks for doing this, some really old bugs and code improvements long
overdue is in the series, much appreciated.

Yours,
Linus Walleij

Thanks for the comprehensive review! I'm glad this all makes some sense
and I'm not crazy about the approach :)

I'll wait a bit for any other feedback that might come in and then
submit a v2 with the fixes/changes mentioned so far.

Happy new year to you all. I wanted to start the new year relaxing by reviewing this series, but with the comments already given I prefer to do that with v2 so don't wait for me :-)

Regards,
Arend

On Mon, Jan 3, 2022 at 6:52 AM Hector Martin [off-list ref] wrote:

On 2022/01/02 14:38, Linus Walleij wrote:

quoted

On Sun, Dec 26, 2021 at 4:37 PM Hector Martin [off-list ref] wrote:

quoted

On Apple platforms, the One Time Programmable ROM in the Broadcom chips
contains information about the specific board design (module, vendor,
version) that is required to select the correct NVRAM file. Parse this
OTP ROM and extract the required strings.

Note that the user OTP offset/size is per-chip. This patch does not add
any chips yet.

Signed-off-by: Hector Martin <redacted>

Overall looks fine!

quoted

+       const char *chip_params;
+       const char *module_params;

This variable name "module_params" is a bit confusing since loadable
kernel modules have params...

Can we think of another name and just put a comment that this
refers to the WiFi module building block?

Sometimes people talk about SoM:s (system-on-modules), so
maybe som_params or brcm_som_params?

Yours,
Linus Walleij

How about board_params, since we're already calling those things boards
elsewhere in the driver? That could refer to the board of a standalone
module, or an integrated board, which should cover all cases.

Fair enough, go for board_params!

Yours,
Linus Walleij

On Mon, Jan 3, 2022 at 7:03 AM Hector Martin [off-list ref] wrote:

On 2022/01/02 14:58, Linus Walleij wrote:

quoted

On Sun, Dec 26, 2021 at 4:38 PM Hector Martin [off-list ref] wrote:

quoted

On DT platforms, the module-instance and antenna-sku-info properties
are passed in the DT. On ACPI platforms, module-instance is passed via
the analogous Apple device property mechanism, while the antenna SKU
info is instead obtained via an ACPI method that grabs it from
non-volatile storage.

Add support for this, to allow proper firmware selection on Apple
platforms.

Signed-off-by: Hector Martin <redacted>

If the strings treated here are exactly the same as for the device tree,
you should be able to just use "devprops" (firmware node) to handle it
abstractly, and then the respective DT and ACPI backend will provide
the properties.

I don't know if this patch I made recently is enough of an examples:
https://lore.kernel.org/linux-hwmon/20211206020423.62402-2-linus.walleij@linaro.org/

If the ACPI and DT differs a lot in format and strings etc it may not
be worth it.

It's not quite the same; module-instance is the same from macOS'
perspective, but we don't use Apple's device tree directly but rather
roll our own DT which uses a different property name in this case.
antenna-sku-info uses an ACPI method on x86, so that one is completely
different. So in the end nothing is actually shared.

OK then!
Acked-by: Linus Walleij <redacted>

Yours,
Linus Walleij