| Message ID | 20240719231158.50416-1-heinrich.schuchardt@canonical.com |
|---|---|
| State | New |
| Delegated to: | Andes |
| Headers | show |
| Series | [1/1] board: fix compatible property Milk-V Mars CM | expand |
On Fri, Jul 19, 2024 at 4:12 PM Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote: > > For the Milk-V Mars CM (lite) we have only been copying sizeof(void *) > bytes to the compatible property instead of the whole string list. This const char array must be so that we may get an accurate data length with sizeof() but it highlights there are helper functions for get of fdt stringlist and not for set of fdt stringlist. > > Fixes: de3229599d4f ("board: add support for Milk-V Mars CM") > Reported-by: E Shattow <lucent@gmail.com> > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > --- > board/starfive/visionfive2/spl.c | 15 ++++++++++++--- > 1 file changed, 12 insertions(+), 3 deletions(-) > > diff --git a/board/starfive/visionfive2/spl.c b/board/starfive/visionfive2/spl.c > index b794b73b6bd..f55c6b5d34c 100644 > --- a/board/starfive/visionfive2/spl.c > +++ b/board/starfive/visionfive2/spl.c > @@ -170,23 +170,32 @@ void spl_fdt_fixup_mars_cm(void *fdt) > { > const char *compat; > const char *model; > + int compat_size; > > spl_fdt_fixup_mars(fdt); > > if (!get_mmc_size_from_eeprom()) { > int offset; > + static const char > + compat_cm_lite[] = "milkv,mars-cm-lite\0starfive,jh7110"; > > model = "Milk-V Mars CM Lite"; > - compat = "milkv,mars-cm-lite\0starfive,jh7110"; > + compat = compat_cm_lite; > + compat_size = sizeof(compat_cm_lite); > > offset = fdt_path_offset(fdt, "/soc/pinctrl/mmc0-pins/mmc0-pins-rest"); > /* GPIOMUX(22, GPOUT_SYS_SDIO0_RST, GPOEN_ENABLE, GPI_NONE) */ > fdt_setprop_u32(fdt, offset, "pinmux", 0xff130016); > } else { > + static const char > + compat_cm[] = "milkv,mars-cm\0starfive,jh7110"; > + > model = "Milk-V Mars CM"; > - compat = "milkv,mars-cm\0starfive,jh7110"; > + compat = compat_cm; > + compat_size = sizeof(compat_cm); > } > - fdt_setprop(fdt, fdt_path_offset(fdt, "/"), "compatible", compat, sizeof(compat)); > + fdt_setprop(fdt, fdt_path_offset(fdt, "/"), > + "compatible", compat, compat_size); > fdt_setprop_string(fdt, fdt_path_offset(fdt, "/"), "model", model); > } > > -- > 2.45.2 > -E
P.S. my suggestion below On Fri, Jul 19, 2024 at 5:06 PM E Shattow <lucent@gmail.com> wrote: > > On Fri, Jul 19, 2024 at 4:12 PM Heinrich Schuchardt > <heinrich.schuchardt@canonical.com> wrote: > > > > For the Milk-V Mars CM (lite) we have only been copying sizeof(void *) > > bytes to the compatible property instead of the whole string list. > > This const char array must be so that we may get an accurate data > length with sizeof() but it highlights there are helper functions for > get of fdt stringlist and not for set of fdt stringlist. > > > > > Fixes: de3229599d4f ("board: add support for Milk-V Mars CM") > > Reported-by: E Shattow <lucent@gmail.com> > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > > --- > > board/starfive/visionfive2/spl.c | 15 ++++++++++++--- > > 1 file changed, 12 insertions(+), 3 deletions(-) > > > > diff --git a/board/starfive/visionfive2/spl.c b/board/starfive/visionfive2/spl.c > > index b794b73b6bd..f55c6b5d34c 100644 > > --- a/board/starfive/visionfive2/spl.c > > +++ b/board/starfive/visionfive2/spl.c > > @@ -170,23 +170,32 @@ void spl_fdt_fixup_mars_cm(void *fdt) > > { > > const char *compat; > > const char *model; > > + int compat_size; > > > > spl_fdt_fixup_mars(fdt); > > > > if (!get_mmc_size_from_eeprom()) { > > int offset; > > + static const char > > + compat_cm_lite[] = "milkv,mars-cm-lite\0starfive,jh7110"; > > > > model = "Milk-V Mars CM Lite"; > > - compat = "milkv,mars-cm-lite\0starfive,jh7110"; > > + compat = compat_cm_lite; > > + compat_size = sizeof(compat_cm_lite); > > > > offset = fdt_path_offset(fdt, "/soc/pinctrl/mmc0-pins/mmc0-pins-rest"); > > /* GPIOMUX(22, GPOUT_SYS_SDIO0_RST, GPOEN_ENABLE, GPI_NONE) */ > > fdt_setprop_u32(fdt, offset, "pinmux", 0xff130016); > > } else { > > + static const char > > + compat_cm[] = "milkv,mars-cm\0starfive,jh7110"; > > + > > model = "Milk-V Mars CM"; > > - compat = "milkv,mars-cm\0starfive,jh7110"; > > + compat = compat_cm; > > + compat_size = sizeof(compat_cm); > > } > > - fdt_setprop(fdt, fdt_path_offset(fdt, "/"), "compatible", compat, sizeof(compat)); > > + fdt_setprop(fdt, fdt_path_offset(fdt, "/"), > > + "compatible", compat, compat_size); > > fdt_setprop_string(fdt, fdt_path_offset(fdt, "/"), "model", model); > > } > > > > -- > > 2.45.2 > > > > -E What about something like as follows? void spl_fdt_fixup_mars(void *fdt) { - static const char compat[] = "milkv,mars\0starfive,jh7110"; u32 phandle; u8 i; int offset; int ret; - fdt_setprop(fdt, fdt_path_offset(fdt, "/"), "compatible", compat, sizeof(compat)); - fdt_setprop_string(fdt, fdt_path_offset(fdt, "/"), "model", - "Milk-V Mars"); + offset = fdt_path_offset(fdt, "/"); + fdt_setprop_string(fdt, offset, "compatible", "milkv,mars"); + fdt_appendprop_string(fdt, offset, "compatible", "starfive,jh7110"); + fdt_setprop_string(fdt, offset, "model", "Milk-V Mars"); /* gmac0 */ offset = fdt_path_offset(fdt, "/soc/clock-controller@17000000"); @@ -164,30 +226,31 @@ void spl_fdt_fixup_mars(void *fdt) break; } } } void spl_fdt_fixup_mars_cm(void *fdt) { - const char *compat; - const char *model; + int offset; spl_fdt_fixup_mars(fdt); if (!get_mmc_size_from_eeprom()) { - int offset; - - model = "Milk-V Mars CM Lite"; - compat = "milkv,mars-cm-lite\0starfive,jh7110"; + offset = fdt_path_offset(fdt, "/"); + fdt_setprop_string(fdt, offset, "compatible", "milkv,mars-cm-lite"); + fdt_appendprop_string(fdt, offset, "compatible", "starfive,jh7110"); + fdt_setprop_string(fdt, offset, "model", "Milk-V Mars CM Lite"); offset = fdt_path_offset(fdt, "/soc/pinctrl/mmc0-pins/mmc0-pins-rest"); /* GPIOMUX(22, GPOUT_SYS_SDIO0_RST, GPOEN_ENABLE, GPI_NONE) */ fdt_setprop_u32(fdt, offset, "pinmux", 0xff130016); } else { - model = "Milk-V Mars CM"; - compat = "milkv,mars-cm\0starfive,jh7110"; + offset = fdt_path_offset(fdt, "/"); + fdt_setprop_string(fdt, offset, "compatible", "milkv,mars-cm"); + fdt_appendprop_string(fdt, offset, "compatible", "starfive,jh7110"); + fdt_setprop_string(fdt, offset, "model", "Milk-V Mars CM"); } - fdt_setprop(fdt, fdt_path_offset(fdt, "/"), "compatible", compat, sizeof(compat)); - fdt_setprop_string(fdt, fdt_path_offset(fdt, "/"), "model", model); } void spl_fdt_fixup_version_a(void *fdt) @@ -235,6 +298,7 @@ void spl_fdt_fixup_version_a(void *fdt) break; } } + } And similar changes to the other "string1\0string2" settings in the same source file. -E
On 7/21/24 23:27, E Shattow wrote: > P.S. my suggestion below > > On Fri, Jul 19, 2024 at 5:06 PM E Shattow <lucent@gmail.com> wrote: >> >> On Fri, Jul 19, 2024 at 4:12 PM Heinrich Schuchardt >> <heinrich.schuchardt@canonical.com> wrote: >>> >>> For the Milk-V Mars CM (lite) we have only been copying sizeof(void *) >>> bytes to the compatible property instead of the whole string list. >> >> This const char array must be so that we may get an accurate data >> length with sizeof() but it highlights there are helper functions for >> get of fdt stringlist and not for set of fdt stringlist. >> >>> >>> Fixes: de3229599d4f ("board: add support for Milk-V Mars CM") >>> Reported-by: E Shattow <lucent@gmail.com> >>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> >>> --- >>> board/starfive/visionfive2/spl.c | 15 ++++++++++++--- >>> 1 file changed, 12 insertions(+), 3 deletions(-) >>> >>> diff --git a/board/starfive/visionfive2/spl.c b/board/starfive/visionfive2/spl.c >>> index b794b73b6bd..f55c6b5d34c 100644 >>> --- a/board/starfive/visionfive2/spl.c >>> +++ b/board/starfive/visionfive2/spl.c >>> @@ -170,23 +170,32 @@ void spl_fdt_fixup_mars_cm(void *fdt) >>> { >>> const char *compat; >>> const char *model; >>> + int compat_size; >>> >>> spl_fdt_fixup_mars(fdt); >>> >>> if (!get_mmc_size_from_eeprom()) { >>> int offset; >>> + static const char >>> + compat_cm_lite[] = "milkv,mars-cm-lite\0starfive,jh7110"; >>> >>> model = "Milk-V Mars CM Lite"; >>> - compat = "milkv,mars-cm-lite\0starfive,jh7110"; >>> + compat = compat_cm_lite; >>> + compat_size = sizeof(compat_cm_lite); >>> >>> offset = fdt_path_offset(fdt, "/soc/pinctrl/mmc0-pins/mmc0-pins-rest"); >>> /* GPIOMUX(22, GPOUT_SYS_SDIO0_RST, GPOEN_ENABLE, GPI_NONE) */ >>> fdt_setprop_u32(fdt, offset, "pinmux", 0xff130016); >>> } else { >>> + static const char >>> + compat_cm[] = "milkv,mars-cm\0starfive,jh7110"; >>> + >>> model = "Milk-V Mars CM"; >>> - compat = "milkv,mars-cm\0starfive,jh7110"; >>> + compat = compat_cm; >>> + compat_size = sizeof(compat_cm); >>> } >>> - fdt_setprop(fdt, fdt_path_offset(fdt, "/"), "compatible", compat, sizeof(compat)); >>> + fdt_setprop(fdt, fdt_path_offset(fdt, "/"), >>> + "compatible", compat, compat_size); >>> fdt_setprop_string(fdt, fdt_path_offset(fdt, "/"), "model", model); >>> } >>> >>> -- >>> 2.45.2 >>> >> >> -E > > What about something like as follows? > > void spl_fdt_fixup_mars(void *fdt) > { > - static const char compat[] = "milkv,mars\0starfive,jh7110"; > u32 phandle; > u8 i; > int offset; > int ret; > > - fdt_setprop(fdt, fdt_path_offset(fdt, "/"), "compatible", > compat, sizeof(compat)); > - fdt_setprop_string(fdt, fdt_path_offset(fdt, "/"), "model", > - "Milk-V Mars"); > + offset = fdt_path_offset(fdt, "/"); > + fdt_setprop_string(fdt, offset, "compatible", "milkv,mars"); > + fdt_appendprop_string(fdt, offset, "compatible", "starfive,jh7110"); > + fdt_setprop_string(fdt, offset, "model", "Milk-V Mars"); > > /* gmac0 */ > offset = fdt_path_offset(fdt, "/soc/clock-controller@17000000"); > @@ -164,30 +226,31 @@ void spl_fdt_fixup_mars(void *fdt) > break; > } > } > } > > void spl_fdt_fixup_mars_cm(void *fdt) > { > - const char *compat; > - const char *model; > + int offset; > > spl_fdt_fixup_mars(fdt); > > if (!get_mmc_size_from_eeprom()) { > - int offset; > - > - model = "Milk-V Mars CM Lite"; > - compat = "milkv,mars-cm-lite\0starfive,jh7110"; > + offset = fdt_path_offset(fdt, "/"); > + fdt_setprop_string(fdt, offset, "compatible", > "milkv,mars-cm-lite"); > + fdt_appendprop_string(fdt, offset, "compatible", > "starfive,jh7110"); > + fdt_setprop_string(fdt, offset, "model", "Milk-V Mars CM Lite"); > > offset = fdt_path_offset(fdt, > "/soc/pinctrl/mmc0-pins/mmc0-pins-rest"); > /* GPIOMUX(22, GPOUT_SYS_SDIO0_RST, GPOEN_ENABLE, GPI_NONE) */ > fdt_setprop_u32(fdt, offset, "pinmux", 0xff130016); > } else { > - model = "Milk-V Mars CM"; > - compat = "milkv,mars-cm\0starfive,jh7110"; > + offset = fdt_path_offset(fdt, "/"); > + fdt_setprop_string(fdt, offset, "compatible", "milkv,mars-cm"); > + fdt_appendprop_string(fdt, offset, "compatible", > "starfive,jh7110"); > + fdt_setprop_string(fdt, offset, "model", "Milk-V Mars CM"); > } > - fdt_setprop(fdt, fdt_path_offset(fdt, "/"), "compatible", > compat, sizeof(compat)); > - fdt_setprop_string(fdt, fdt_path_offset(fdt, "/"), "model", model); > } > > void spl_fdt_fixup_version_a(void *fdt) > @@ -235,6 +298,7 @@ void spl_fdt_fixup_version_a(void *fdt) > break; > } > } > + > } > > > And similar changes to the other "string1\0string2" settings in the > same source file. > > -E You did not mention why you would prefer three function calls (including strlen()) instead of one. Your code should work too but results in a larger code size. Probably the shortest code would use a common function with parameters 'model' and 'compatible_1st' for each of the boards and then use fdt_appendprop_string() to append the common 2nd compatible string. Best regards Heinrich
On Sun, Jul 21, 2024 at 2:42 PM Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote: > > On 7/21/24 23:27, E Shattow wrote: > > P.S. my suggestion below > > > > On Fri, Jul 19, 2024 at 5:06 PM E Shattow <lucent@gmail.com> wrote: > >> > >> On Fri, Jul 19, 2024 at 4:12 PM Heinrich Schuchardt > >> <heinrich.schuchardt@canonical.com> wrote: > >>> > >>> For the Milk-V Mars CM (lite) we have only been copying sizeof(void *) > >>> bytes to the compatible property instead of the whole string list. > >> > >> This const char array must be so that we may get an accurate data > >> length with sizeof() but it highlights there are helper functions for > >> get of fdt stringlist and not for set of fdt stringlist. > >> > >>> > >>> Fixes: de3229599d4f ("board: add support for Milk-V Mars CM") > >>> Reported-by: E Shattow <lucent@gmail.com> > >>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > >>> --- > >>> board/starfive/visionfive2/spl.c | 15 ++++++++++++--- > >>> 1 file changed, 12 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/board/starfive/visionfive2/spl.c b/board/starfive/visionfive2/spl.c > >>> index b794b73b6bd..f55c6b5d34c 100644 > >>> --- a/board/starfive/visionfive2/spl.c > >>> +++ b/board/starfive/visionfive2/spl.c > >>> @@ -170,23 +170,32 @@ void spl_fdt_fixup_mars_cm(void *fdt) > >>> { > >>> const char *compat; > >>> const char *model; > >>> + int compat_size; > >>> > >>> spl_fdt_fixup_mars(fdt); > >>> > >>> if (!get_mmc_size_from_eeprom()) { > >>> int offset; > >>> + static const char > >>> + compat_cm_lite[] = "milkv,mars-cm-lite\0starfive,jh7110"; > >>> > >>> model = "Milk-V Mars CM Lite"; > >>> - compat = "milkv,mars-cm-lite\0starfive,jh7110"; > >>> + compat = compat_cm_lite; > >>> + compat_size = sizeof(compat_cm_lite); > >>> > >>> offset = fdt_path_offset(fdt, "/soc/pinctrl/mmc0-pins/mmc0-pins-rest"); > >>> /* GPIOMUX(22, GPOUT_SYS_SDIO0_RST, GPOEN_ENABLE, GPI_NONE) */ > >>> fdt_setprop_u32(fdt, offset, "pinmux", 0xff130016); > >>> } else { > >>> + static const char > >>> + compat_cm[] = "milkv,mars-cm\0starfive,jh7110"; > >>> + > >>> model = "Milk-V Mars CM"; > >>> - compat = "milkv,mars-cm\0starfive,jh7110"; > >>> + compat = compat_cm; > >>> + compat_size = sizeof(compat_cm); > >>> } > >>> - fdt_setprop(fdt, fdt_path_offset(fdt, "/"), "compatible", compat, sizeof(compat)); > >>> + fdt_setprop(fdt, fdt_path_offset(fdt, "/"), > >>> + "compatible", compat, compat_size); > >>> fdt_setprop_string(fdt, fdt_path_offset(fdt, "/"), "model", model); > >>> } > >>> > >>> -- > >>> 2.45.2 > >>> > >> > >> -E > > > > What about something like as follows? > > > > void spl_fdt_fixup_mars(void *fdt) > > { > > - static const char compat[] = "milkv,mars\0starfive,jh7110"; > > u32 phandle; > > u8 i; > > int offset; > > int ret; > > > > - fdt_setprop(fdt, fdt_path_offset(fdt, "/"), "compatible", > > compat, sizeof(compat)); > > - fdt_setprop_string(fdt, fdt_path_offset(fdt, "/"), "model", > > - "Milk-V Mars"); > > + offset = fdt_path_offset(fdt, "/"); > > + fdt_setprop_string(fdt, offset, "compatible", "milkv,mars"); > > + fdt_appendprop_string(fdt, offset, "compatible", "starfive,jh7110"); > > + fdt_setprop_string(fdt, offset, "model", "Milk-V Mars"); > > > > /* gmac0 */ > > offset = fdt_path_offset(fdt, "/soc/clock-controller@17000000"); > > @@ -164,30 +226,31 @@ void spl_fdt_fixup_mars(void *fdt) > > break; > > } > > } > > } > > > > void spl_fdt_fixup_mars_cm(void *fdt) > > { > > - const char *compat; > > - const char *model; > > + int offset; > > > > spl_fdt_fixup_mars(fdt); > > > > if (!get_mmc_size_from_eeprom()) { > > - int offset; > > - > > - model = "Milk-V Mars CM Lite"; > > - compat = "milkv,mars-cm-lite\0starfive,jh7110"; > > + offset = fdt_path_offset(fdt, "/"); > > + fdt_setprop_string(fdt, offset, "compatible", > > "milkv,mars-cm-lite"); > > + fdt_appendprop_string(fdt, offset, "compatible", > > "starfive,jh7110"); > > + fdt_setprop_string(fdt, offset, "model", "Milk-V Mars CM Lite"); > > > > offset = fdt_path_offset(fdt, > > "/soc/pinctrl/mmc0-pins/mmc0-pins-rest"); > > /* GPIOMUX(22, GPOUT_SYS_SDIO0_RST, GPOEN_ENABLE, GPI_NONE) */ > > fdt_setprop_u32(fdt, offset, "pinmux", 0xff130016); > > } else { > > - model = "Milk-V Mars CM"; > > - compat = "milkv,mars-cm\0starfive,jh7110"; > > + offset = fdt_path_offset(fdt, "/"); > > + fdt_setprop_string(fdt, offset, "compatible", "milkv,mars-cm"); > > + fdt_appendprop_string(fdt, offset, "compatible", > > "starfive,jh7110"); > > + fdt_setprop_string(fdt, offset, "model", "Milk-V Mars CM"); > > } > > - fdt_setprop(fdt, fdt_path_offset(fdt, "/"), "compatible", > > compat, sizeof(compat)); > > - fdt_setprop_string(fdt, fdt_path_offset(fdt, "/"), "model", model); > > } > > > > void spl_fdt_fixup_version_a(void *fdt) > > @@ -235,6 +298,7 @@ void spl_fdt_fixup_version_a(void *fdt) > > break; > > } > > } > > + > > } > > > > > > And similar changes to the other "string1\0string2" settings in the > > same source file. > > > > -E > > You did not mention why you would prefer three function calls (including > strlen()) instead of one. > > Your code should work too but results in a larger code size. > > Probably the shortest code would use a common function with parameters > 'model' and 'compatible_1st' for each of the boards and then use > fdt_appendprop_string() to append the common 2nd compatible string. > > Best regards > > Heinrich You are correct and your patch does efficiently fix the bug. We can try to reduce code size and function calls further by consolidation of the duplicate bytes of static data "starfive,jh7110" in several locations and calling that as an fdt_appendprop_string() only once after the selected specialized fixup function has returned. But this would be clever and being clever is a reason why the bug is here. With OF_UPSTREAM most of the fdt setting function calls will be washed out. If just writing easy-to-read code using the helper functions and static data to parameters then this type of bug is not possible. But if you prefer your efficient fix then that is not any problem for me. -E
diff --git a/board/starfive/visionfive2/spl.c b/board/starfive/visionfive2/spl.c index b794b73b6bd..f55c6b5d34c 100644 --- a/board/starfive/visionfive2/spl.c +++ b/board/starfive/visionfive2/spl.c @@ -170,23 +170,32 @@ void spl_fdt_fixup_mars_cm(void *fdt) { const char *compat; const char *model; + int compat_size; spl_fdt_fixup_mars(fdt); if (!get_mmc_size_from_eeprom()) { int offset; + static const char + compat_cm_lite[] = "milkv,mars-cm-lite\0starfive,jh7110"; model = "Milk-V Mars CM Lite"; - compat = "milkv,mars-cm-lite\0starfive,jh7110"; + compat = compat_cm_lite; + compat_size = sizeof(compat_cm_lite); offset = fdt_path_offset(fdt, "/soc/pinctrl/mmc0-pins/mmc0-pins-rest"); /* GPIOMUX(22, GPOUT_SYS_SDIO0_RST, GPOEN_ENABLE, GPI_NONE) */ fdt_setprop_u32(fdt, offset, "pinmux", 0xff130016); } else { + static const char + compat_cm[] = "milkv,mars-cm\0starfive,jh7110"; + model = "Milk-V Mars CM"; - compat = "milkv,mars-cm\0starfive,jh7110"; + compat = compat_cm; + compat_size = sizeof(compat_cm); } - fdt_setprop(fdt, fdt_path_offset(fdt, "/"), "compatible", compat, sizeof(compat)); + fdt_setprop(fdt, fdt_path_offset(fdt, "/"), + "compatible", compat, compat_size); fdt_setprop_string(fdt, fdt_path_offset(fdt, "/"), "model", model); }
For the Milk-V Mars CM (lite) we have only been copying sizeof(void *) bytes to the compatible property instead of the whole string list. Fixes: de3229599d4f ("board: add support for Milk-V Mars CM") Reported-by: E Shattow <lucent@gmail.com> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> --- board/starfive/visionfive2/spl.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)