Re: [PATCH v3] mmc: allow use of hardware partition names for mmc partconf

[Marek Vasut <marex@denx.de>]

On 4/27/24 2:06 PM, E Shattow wrote:
> On Sat, Apr 27, 2024 at 3:22 AM Marek Vasut <marex@denx.de> wrote:
>>
>> On 4/27/24 3:29 AM, E Shattow wrote:
>>> Hi Marek,
>>>
>>> On Fri, Apr 26, 2024 at 5:49 PM Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> [...]
>>>>
>>>>> diff --git a/include/mmc.h b/include/mmc.h
>>>>> index 4b8327f1f93b..7243bd761202 100644
>>>>> --- a/include/mmc.h
>>>>> +++ b/include/mmc.h
>>>>> @@ -381,6 +381,21 @@ enum mmc_voltage {
>>>>>     #define MMC_TIMING_MMC_HS200        9
>>>>>     #define MMC_TIMING_MMC_HS400        10
>>>>>
>>>>> +/* emmc hardware partition values */
>>>>> +enum emmc_hwpart {
>>>>> +     EMMC_HWPART_DEFAULT = 0,
>>>>> +     EMMC_HWPART_BOOT0 = 1,
>>>>> +     EMMC_HWPART_BOOT1 = 2,
>>>>> +     EMMC_HWPART_GP1 = 3,
>>>>> +     EMMC_HWPART_GP2 = 4,
>>>>> +     EMMC_HWPART_GP3 = 5,
>>>>> +     EMMC_HWPART_GP4 = 6,
>>>>> +     EMMC_HWPART_USER = 7,
>>>>> +};
>>>>> +
>>>>> +/* emmc hardware partition names */
>>>>> +extern const char *emmc_hwpart_names[];
>>>>
>>>> Maybe the array should have fixed size here, i.e. 8 ?
>>>
>>> Is there an ABI reason to do so? Can you explain further why it would
>>> be needed to do that?
>>
>> It has nothing to do with ABI, it is only to let the compiler validate
>> that nobody would index the array with index > 7 by accident.
> 
> At least GCC knows this without doing its work again ourselves. How
> about a const for the upper limit, where currently EMMC_HWPART_USER
> substitutes for expressing an upper limit? You may as well be writing
> EMMC_HWPART_MAX = 8 in the enum and using that for the initializer
> also any iterators with a less-than condition to make it more
> expressive.

You could use ARRAY_SIZE(emmc_hwpart_names) in iterators, no extra 
symbols should be necessary.

> $ gcc -o testobj test.c -Wall -Wextra -O2
> test.c: In function ‘main’:
> test.c:16:5: warning: array subscript 8 is above array bounds of
> ‘const char *[8]’ [-Warray-bounds=]
>     16 |     printf(emmc_hwpart_names[8]);
>        |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
> test.c:3:13: note: while referencing ‘emmc_hwpart_names’
>      3 | const char *emmc_hwpart_names[] = {
>        |             ^~~~~~~~~~~~~~~~~
> 
> #include <stdio.h>
> 
> const char *emmc_hwpart_names[] = {
>         "user",
>         "boot0",
>         "boot1",
>         "gp1",
>         "gp2",
>         "gp3",
>         "gp4",
>         "user",
> };
> 
> int main(int argc, char** argv) {
>      (void) argc; (void) argv;
>      printf(emmc_hwpart_names[8]);
> 
>      return 0;
> }

In case of this patched code here, the example is more like this:

a.c:
#include <stdio.h>
#include "h.h"

int main(void)
{
     printf("%d\n", arr[8]); // This code contains a bug here
     return 0;
}

b.c:
int arr[2] = { 1, 2 };

h.h:
extern int arr[];

Compile:
$ gcc -O2 -Wall -c -o a.o a.c
$ gcc -O2 -Wall -c -o b.o b.c
$ gcc -O2 -Wall -o out a.o b.o

You won't get warning when compiling a.c , because gcc does not know the 
size of the array until the linking step. You will get a warning if you 
change the header like this:

h.h:
-extern int arr[];
+extern int arr[2];