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.
