05.01.2022 16:22, Hector Martin пишет:

On 05/01/2022 07.09, Dmitry Osipenko wrote:

quoted

04.01.2022 11:43, Hector Martin пишет:

quoted

quoted

quoted

+static int brcm_alt_fw_paths(const char *path, const char *board_type,
+			     const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS])>  {
 	char alt_path[BRCMF_FW_NAME_LEN];
 	const char *suffix;
 
+	memset(alt_paths, 0, array_size(sizeof(*alt_paths),
+					BRCMF_FW_MAX_ALT_PATHS));

You don't need to use array_size() since size of a fixed array is
already known.

memset(alt_paths, 0, sizeof(alt_paths));

It's a function argument, so that doesn't work and actually throws a
warning. Array function argument notation is informative only; they
behave strictly equivalent to pointers. Try it:

$ cat test.c
#include <stdio.h>

void foo(char x[42])
{
	printf("%ld\n", sizeof(x));
}

int main() {
	char x[42];

	foo(x);
}
$ gcc test.c
test.c: In function ‘foo’:
test.c:5:31: warning: ‘sizeof’ on array function parameter ‘x’ will
return size of ‘char *’ [-Wsizeof-array-argument]
    5 |         printf("%ld\n", sizeof(x));
      |                               ^
test.c:3:15: note: declared here
    3 | void foo(char x[42])
      |          ~~~~~^~~~~
$ ./a.out
8

Then please use "const char **alt_paths" for the function argument to
make code cleaner and add another argument to pass the number of array
elements.

So you want me to do the ARRAY_SIZE at the caller side then?

quoted

static int brcm_alt_fw_paths(const char *path, const char *board_type,
			     const char **alt_paths, unsigned int num_paths)
{
	size_t alt_paths_size = array_size(sizeof(*alt_paths), num_paths);
	
	memset(alt_paths, 0, alt_paths_size);
}

...

Maybe even better create a dedicated struct for the alt_paths:

struct brcmf_fw_alt_paths {
	const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS];
	unsigned int index;
};

and then use the ".index" in the brcm_free_alt_fw_paths(). I suppose
this will make code a bit nicer and easier to follow.

I'm confused; the array size is constant. What would index contain and
why would would brcm_free_alt_fw_paths use it? Just as an iterator
variable instead of using a local variable? Or do you mean count?

Yes, use index for the count of active entries in the alt_paths[].

for (i = 0; i < alt_paths.index; i++)
	kfree(alt_paths.path[i]);

alt_paths.index = 0;

or

while (alt_paths.index)
	kfree(alt_paths.path[--alt_paths.index]);

Though, to be honest, at this point I'm considering rethinking the whole
patch for this mechanism because I'm not terribly happy with the current
approach and clearly you aren't either :-) Maybe it makes more sense to
stop trying to compute all the alt_paths ahead of time, and just have
the function compute a single one to be used just-in-time at firmware
request time, and just iterate over board_types.

The just-in-time approach sounds like a good idea.