Subject: Re: [PATCH v2 04/35] brcmfmac: firmware: Support having multiple alt paths

From: Dmitry Osipenko <[email-masked]>
Date: Thu, 6 Jan 2022 20:40:34 +0300

05.01.2022 16:22, Hector Martin пишет:
> On 05/01/2022 07.09, Dmitry Osipenko wrote:
>> 04.01.2022 11:43, Hector Martin пишет:
>>>>> +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?
> 
>>
>> 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.