Re: [U-Boot] 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)
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 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.
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 hannes.schmelzer@br-automation.com
cmd/fdt.c | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-)
diff --git a/cmd/fdt.c b/cmd/fdt.c index a21415d..e55102a 100644 --- a/cmd/fdt.c +++ b/cmd/fdt.c @@ -257,6 +257,7 @@ static int do_fdt(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])> char *prop; /* property */ int nodeoffset; /* node offset from libfdt */ static char data[SCRATCHPAD]; /* storage for the property */
const void *ptmp;@@ -268,13 +269,6 @@ static int do_fdt(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])> 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;}- ret = fdt_setprop(working_fdt, nodeoffset, prop, data, len); if (ret < 0) { printf ("libfdt fdt_setprop(): %s\n",
fdt_strerror(ret)); @@ -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);elsenewp++;data += 4; *len += 4;
Hi Laurent,
thanks for reporting this. I just came back from holiday. I will check / fix this as soon as possible.
cheers, Hannes
Laurent Pinchart laurent.pinchart@ideasonboard.com schrieb am 04.08.2017 23:23:19:
Von: Laurent Pinchart laurent.pinchart@ideasonboard.com An: u-boot@lists.denx.de Kopie: Hannes Schmelzer hannes.schmelzer@br-automation.com, Simon
Glass
sjg@chromium.org 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)
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
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.
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 hannes.schmelzer@br-automation.com
cmd/fdt.c | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-)
diff --git a/cmd/fdt.c b/cmd/fdt.c index a21415d..e55102a 100644 --- a/cmd/fdt.c +++ b/cmd/fdt.c @@ -257,6 +257,7 @@ static int do_fdt(cmd_tbl_t *cmdtp, int flag, int
argc,
char * const argv[])> 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[])> 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;}ret = fdt_setprop(working_fdt, nodeoffset, prop, data, len); if (ret < 0) { printf ("libfdt fdt_setprop(): %s\n",fdt_strerror(ret)); @@ -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);elsenewp++;data += 4; *len += 4;-- Regards,
Laurent Pinchart
Laurent Pinchart laurent.pinchart@ideasonboard.com schrieb am 04.08.2017 23:23:19:
Hi Hannes,
Hi Laurent,
as told a few days ago i'm now coming back to this issue.
(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 ;-)
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
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...
Can you please try with your testcase with most current source again?
please let us know if the problem is fixed with this.
cheers, Hannes
(....)
cmd/fdt.c | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-)
diff --git a/cmd/fdt.c b/cmd/fdt.c index a21415d..e55102a 100644 --- a/cmd/fdt.c +++ b/cmd/fdt.c @@ -257,6 +257,7 @@ static int do_fdt(cmd_tbl_t *cmdtp, int flag, int
argc,
char * const argv[])> 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[])> 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;}ret = fdt_setprop(working_fdt, nodeoffset, prop, data, len); if (ret < 0) { printf ("libfdt fdt_setprop(): %s\n",fdt_strerror(ret)); @@ -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);elsenewp++;data += 4; *len += 4;
Hi Hannes,
On Friday 18 Aug 2017 10:07:19 Hannes Schmelzer wrote:
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.
(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 ;-)
I know, but I've already stopped reading mailing lists I'm subscribed to due to the amount of traffic, so I figured out that subscribing to new ones wouldn't be a good idea :-)
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 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.
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 the fdt set command from operating correctly, as it will return an error if the property isn't found.
Laurent Pinchart laurent.pinchart@ideasonboard.com schrieb am 18.08.2017 10:14:39:
Von: Laurent Pinchart laurent.pinchart@ideasonboard.com
Hi Hannes,
Hi Laurent,
i will provide some patch for fixing this issue.
cheers, Hannes
On Friday 18 Aug 2017 10:07:19 Hannes Schmelzer wrote:
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.
(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 ;-)
I know, but I've already stopped reading mailing lists I'm subscribed to
due
to the amount of traffic, so I figured out that subscribing to new ones wouldn't be a good idea :-)
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
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.
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
the fdt
set command from operating correctly, as it will return an error if the property isn't found.
-- Regards,
Laurent Pinchart
participants (2)
-
Hannes Schmelzer -
Laurent Pinchart