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 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