cmd/fdt: support single value replacement within an array

With this commit we can modify single values within an array of a dts property.

This is useful if we have for example a pwm-backlight where we want to modifiy the pwm frequency per u-boot script.

The pwm is described in dts like this:

backlight {
	pwms = <0x0000002b 0x00000000 0x004c4b40>;
};

For changing the frequency, here the 3rd parameter, we simply type:

fdt set /backlight pwms <? ? 0x1E8480>;

For doing all this we:
- backup the property content into our 'SCRATCHPAD'
- only modify the array-cell if the new content doesn't start with '?'

Signed-off-by Hannes Schmelzer <[email protected]>

---

 cmd/fdt.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

cmd/fdt.c +0 -0

index a21415d..e55102a 100644
@@ -257,6 +257,7 @@  static int do_fdt(cmd_tbl_t *cmdtp, int flag, int argc,

char * const argv[])>

cmd/fdt.c +1 -0

 		char *prop;		/* property */
 		int  nodeoffset;	/* node offset from libfdt */
 		static char data[SCRATCHPAD];	/* storage for the property */
+		const void *ptmp;
 		int  len;		/* new length of the property */
 		int  ret;		/* return value */

@@ -268,13 +269,6 @@  static int do_fdt(cmd_tbl_t *cmdtp, int flag, int

argc, char * const argv[])>

cmd/fdt.c +15 -7

 		pathp  = argv[2];
 		prop   = argv[3];
 
-		if (argc == 4) {
-			len = 0;
-		} else {
-			ret = fdt_parse_prop(&argv[4], argc - 4, data, &len);
-			if (ret != 0)
-				return ret;
-		}
 
 		nodeoffset = fdt_path_offset (working_fdt, pathp);
 		if (nodeoffset < 0) {

@@ -286,6 +280,21 @@  static int do_fdt(cmd_tbl_t *cmdtp, int flag, int
 argc, char * const argv[])
 			return 1;
 		}
 
+		if (argc == 4) {
+			len = 0;
+		} else {
+			ptmp = fdt_getprop(working_fdt, nodeoffset, prop,
 &len);
+			if (len > SCRATCHPAD) {
+				printf("prop (%d) doesn't fit in scratchpad!
 \n",
+				       len);
+				return 1;
+			}
+			memcpy(data, ptmp, len);
+			ret = fdt_parse_prop(&argv[4], argc - 4, data, &len);
+			if (ret != 0)
+				return ret;
+		}
+

cmd/fdt.c +0 -0

 		ret = fdt_setprop(working_fdt, nodeoffset, prop, data, len);
 		if (ret < 0) {
 			printf ("libfdt fdt_setprop(): %s\n",

fdt_strerror(ret));

cmd/fdt.c +5 -1

@@ -766,7 +775,11 @@  static int fdt_parse_prop(char * const *newval, int
 count, char *data, int *len)
 			cp = newp;
 			tmp = simple_strtoul(cp, &newp, 0);
 
-			*(fdt32_t *)data = cpu_to_fdt32(tmp);
+			if (*cp != '?')
+				*(fdt32_t *)data = cpu_to_fdt32(tmp);
+			else
+				newp++;
+
 
 			data  += 4;

*len += 4;

Von: Laurent Pinchart <[email protected]>
An: u-boot at lists.denx.de
Kopie: Hannes Schmelzer <[email protected]>, Simon

<[email protected]>
Datum: 04.08.2017 23:23
Betreff: Re: cmd/fdt: support single value replacement within an array

Hi Hannes,

(I'm not subscribed to the list, please keep me CC'ed on replies)

Quoted 2 lines · On Tuesday 30 May 2017 13:05:00 Hannes Schmelzer wrote:

With this commit we can modify single values within an array of a dts property.

But with this commit U-Boot crashes if you try to create a new property

the fdt set command :-/

I've tested v2017.07 with the commit reverted, and fdt set works again

The issue is that your fdt_getprop() call fails and return NULL with len

to -1. You can easily imagine what the memcpy() following it will do.

Quoted 21 lines

This is useful if we have for example a pwm-backlight where we want to modifiy the pwm frequency per u-boot script.

The pwm is described in dts like this:

backlight {
   pwms = <0x0000002b 0x00000000 0x004c4b40>;
};

For changing the frequency, here the 3rd parameter, we simply type:

fdt set /backlight pwms <? ? 0x1E8480>;

For doing all this we:
- backup the property content into our 'SCRATCHPAD'
- only modify the array-cell if the new content doesn't start with '?'

Signed-off-by Hannes Schmelzer <[email protected]>

---

cmd/fdt.c static int do_fdt(cmd_tbl_t *cmdtp, int flag, int

Quoted 8 lines

 cmd/fdt.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

cmd/fdt.c +0 -0

index a21415d..e55102a 100644
@@ -257,6 +257,7 @@  static int do_fdt(cmd_tbl_t *cmdtp, int flag, int 

cmd/fdt.c static int do_fdt(cmd_tbl_t *cmdtp, int flag,

Quoted 9 lines

char * const argv[])>
char *prop; /* property */
int nodeoffset; /* node offset from libfdt */
static char data[SCRATCHPAD]; /* storage for the property */

cmd/fdt.c +1 -0

+      const void *ptmp;
       int  len;      /* new length of the property */
       int  ret;      /* return value */

@@ -268,13 +269,6 @@  static int do_fdt(cmd_tbl_t *cmdtp, int flag, 

Quoted 12 lines

argc, char * const argv[])>

       pathp  = argv[2];
       prop   = argv[3];

cmd/fdt.c +0 -7

-      if (argc == 4) {
-         len = 0;
-      } else {
-         ret = fdt_parse_prop(&argv[4], argc - 4, data, &len);
-         if (ret != 0)
-            return ret;
-      }

cmd/fdt.c static int do_fdt(cmd_tbl_t *cmdtp, int flag,

Quoted 3 lines

       nodeoffset = fdt_path_offset (working_fdt, pathp);
       if (nodeoffset < 0) {

cmd/fdt.c +0 -0

@@ -286,6 +280,21 @@  static int do_fdt(cmd_tbl_t *cmdtp, int flag, 

Quoted 18 lines

cmd/fdt.c +12 -0

 argc, char * const argv[])
          return 1;
       }
 
+      if (argc == 4) {
+         len = 0;
+      } else {
+         ptmp = fdt_getprop(working_fdt, nodeoffset, prop,
 &len);
+         if (len > SCRATCHPAD) {
+            printf("prop (%d) doesn't fit in scratchpad!
 \n",
+                   len);
+            return 1;
+         }
+         memcpy(data, ptmp, len);
+         ret = fdt_parse_prop(&argv[4], argc - 4, data, &len);
+         if (ret != 0)

cmd/fdt.c static int fdt_parse_prop(char * const *newval,

Quoted 8 lines

cmd/fdt.c +3 -0

+            return ret;
+      }
+

       ret = fdt_setprop(working_fdt, nodeoffset, prop, data, len);
       if (ret < 0) {
          printf ("libfdt fdt_setprop(): %s\n",

fdt_strerror(ret));

cmd/fdt.c +0 -0

@@ -766,7 +775,11 @@  static int fdt_parse_prop(char * const *newval, 

Quoted 13 lines

cmd/fdt.c +5 -1

 count, char *data, int *len)
          cp = newp;
          tmp = simple_strtoul(cp, &newp, 0);
 
-         *(fdt32_t *)data = cpu_to_fdt32(tmp);
+         if (*cp != '?')
+            *(fdt32_t *)data = cpu_to_fdt32(tmp);
+         else
+            newp++;
+
 
          data  += 4;

          *len += 4;

Signature · 3 lines

Regards,

Laurent Pinchart

Hi Hannes,

(I'm not subscribed to the list, please keep me CC'ed on replies)

Quoted 2 lines

With this commit we can modify single values within an array of a dts property.

But with this commit U-Boot crashes if you try to create a new property

the fdt set command :-/

I've tested v2017.07 with the commit reverted, and fdt set works again

The issue is that your fdt_getprop() call fails and return NULL with len

to -1. You can easily imagine what the memcpy() following it will do.

cmd/fdt.c static int do_fdt(cmd_tbl_t *cmdtp, int flag, int

Quoted 10 lines

---

 cmd/fdt.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

cmd/fdt.c +0 -0

index a21415d..e55102a 100644
@@ -257,6 +257,7 @@  static int do_fdt(cmd_tbl_t *cmdtp, int flag, int 

cmd/fdt.c static int do_fdt(cmd_tbl_t *cmdtp, int flag,

Quoted 9 lines

char * const argv[])>
char *prop; /* property */
int nodeoffset; /* node offset from libfdt */
static char data[SCRATCHPAD]; /* storage for the property */

cmd/fdt.c +1 -0

+      const void *ptmp;
       int  len;      /* new length of the property */
       int  ret;      /* return value */

@@ -268,13 +269,6 @@  static int do_fdt(cmd_tbl_t *cmdtp, int flag, 

Quoted 12 lines

argc, char * const argv[])>

       pathp  = argv[2];
       prop   = argv[3];

cmd/fdt.c +0 -7

-      if (argc == 4) {
-         len = 0;
-      } else {
-         ret = fdt_parse_prop(&argv[4], argc - 4, data, &len);
-         if (ret != 0)
-            return ret;
-      }

cmd/fdt.c static int do_fdt(cmd_tbl_t *cmdtp, int flag,

Quoted 3 lines

       nodeoffset = fdt_path_offset (working_fdt, pathp);
       if (nodeoffset < 0) {

cmd/fdt.c +0 -0

@@ -286,6 +280,21 @@  static int do_fdt(cmd_tbl_t *cmdtp, int flag, 

Quoted 18 lines

cmd/fdt.c +12 -0

 argc, char * const argv[])
          return 1;
       }
 
+      if (argc == 4) {
+         len = 0;
+      } else {
+         ptmp = fdt_getprop(working_fdt, nodeoffset, prop,
 &len);
+         if (len > SCRATCHPAD) {
+            printf("prop (%d) doesn't fit in scratchpad!
 \n",
+                   len);
+            return 1;
+         }
+         memcpy(data, ptmp, len);
+         ret = fdt_parse_prop(&argv[4], argc - 4, data, &len);
+         if (ret != 0)

cmd/fdt.c static int fdt_parse_prop(char * const *newval,

Quoted 8 lines

cmd/fdt.c +3 -0

+            return ret;
+      }
+

       ret = fdt_setprop(working_fdt, nodeoffset, prop, data, len);
       if (ret < 0) {
          printf ("libfdt fdt_setprop(): %s\n",

fdt_strerror(ret));

cmd/fdt.c +0 -0

@@ -766,7 +775,11 @@  static int fdt_parse_prop(char * const *newval, 

Quoted 13 lines

cmd/fdt.c +5 -1

 count, char *data, int *len)
          cp = newp;
          tmp = simple_strtoul(cp, &newp, 0);
 
-         *(fdt32_t *)data = cpu_to_fdt32(tmp);
+         if (*cp != '?')
+            *(fdt32_t *)data = cpu_to_fdt32(tmp);
+         else
+            newp++;
+
 
          data  += 4;

          *len += 4;

Laurent Pinchart schrieb am 04.08.2017 23:23:19:

Hi Laurent,

as told a few days ago i'm now coming back to this issue.

Quoted 1 line

(I'm not subscribed to the list, please keep me CC'ed on replies)

subscribing to the list would be a great idea, so you're always up to date whats going on ;-)

Quoted 11 lines · On Tuesday 30 May 2017 13:05:00 Hannes Schmelzer wrote:

Quoted 2 lines

With this commit we can modify single values within an array of a dts property.

But with this commit U-Boot crashes if you try to create a new property with the fdt set command :-/

I've tested v2017.07 with the commit reverted, and fdt set works again for me. The issue is that your fdt_getprop() call fails and return NULL with len set to -1. You can easily imagine what the memcpy() following it will do.

Yes. Your'e right with that.

I just checked most current source (...), there were changes regarding this issue.
Tom introduced a check against the fail of fdt_getprop(...) call.

http://git.denx.de/?p=u-boot.git;a=commitdiff;h=99bb38e2cce9d99238458e0f6d18
80c6d2e80a4d

Can you please try with your testcase with most current source again?

please let us know if the problem is fixed with this.

Von: Laurent Pinchart <[email protected]>

Hi Hannes,

Quoted 5 lines

Laurent Pinchart schrieb am 04.08.2017 23:23:19:

Hi Laurent,

as told a few days ago i'm now coming back to this issue.

Thank you.

Quoted 3 lines

Quoted 1 line

(I'm not subscribed to the list, please keep me CC'ed on replies)

subscribing to the list would be a great idea, so you're always up to

Quoted 1 line

whats going on ;-)

I know, but I've already stopped reading mailing lists I'm subscribed to

to the amount of traffic, so I figured out that subscribing to new ones wouldn't be a good idea :-)

Quoted 2 lines

Quoted 2 lines · On Tuesday 30 May 2017 13:05:00 Hannes Schmelzer wrote:

Quoted 1 line

With this commit we can modify single values within an array of a

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

property.

But with this commit U-Boot crashes if you try to create a new

Quoted 3 lines

Quoted 3 lines

with the fdt set command :-/

I've tested v2017.07 with the commit reverted, and fdt set works

Quoted 1 line

Quoted 1 line

for me. The issue is that your fdt_getprop() call fails and return

Quoted 1 line

Quoted 1 line

with len set to -1. You can easily imagine what the memcpy()

Quoted 9 lines

Quoted 1 line

will do.

Yes. Your'e right with that.

I just checked most current source (...), there were changes regarding this issue.
Tom introduced a check against the fail of fdt_getprop(...) call.

Quoted 5 lines

80c6d2e80a4d

Can you please try with your testcase with most current source again?

please let us know if the problem is fixed with this.

I can't test that right now as I don't have access to my hardware at the

moment, but I doubt it will work.

The code now reads as

   ptmp = fdt_getprop(working_fdt, nodeoffset, prop, &len);
   if (!ptmp) {
      printf("prop (%s) not found!\n", prop);
      return 1;
   }

The new !ptmp check should prevent the crash, but it will also prevent

set command from operating correctly, as it will return an error if the property isn't found.

Signature · 3 lines

Regards,

Laurent Pinchart