* [U-Boot] cmd/fdt: support single value replacement within an array
@ 2017-08-04 21:23 Laurent Pinchart
2017-08-16 7:05 ` Hannes Schmelzer
2017-08-18 8:07 ` Hannes Schmelzer
0 siblings, 2 replies; 5+ messages in thread
From: Laurent Pinchart @ 2017-08-04 21:23 UTC (permalink / raw)
To: u-boot
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);
> + else
> + newp++;
> +
>
> data += 4;
> *len += 4;
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 5+ messages in thread* [U-Boot] cmd/fdt: support single value replacement within an array
2017-08-04 21:23 [U-Boot] cmd/fdt: support single value replacement within an array Laurent Pinchart
@ 2017-08-16 7:05 ` Hannes Schmelzer
2017-08-18 8:07 ` Hannes Schmelzer
1 sibling, 0 replies; 5+ messages in thread
From: Hannes Schmelzer @ 2017-08-16 7:05 UTC (permalink / raw)
To: u-boot
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 at 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);
> > + else
> > + newp++;
> > +
> >
> > data += 4;
> > *len += 4;
> --
> Regards,
>
> Laurent Pinchart
>
^ permalink raw reply [flat|nested] 5+ messages in thread* [U-Boot] cmd/fdt: support single value replacement within an array
2017-08-04 21:23 [U-Boot] cmd/fdt: support single value replacement within an array Laurent Pinchart
2017-08-16 7:05 ` Hannes Schmelzer
@ 2017-08-18 8:07 ` Hannes Schmelzer
2017-08-18 8:14 ` Laurent Pinchart
1 sibling, 1 reply; 5+ messages in thread
From: Hannes Schmelzer @ 2017-08-18 8:07 UTC (permalink / raw)
To: u-boot
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=99bb38e2cce9d99238458e0f6d1880c6d2e80a4d
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);
> > + else
> > + newp++;
> > +
> >
> > data += 4;
> > *len += 4;
^ permalink raw reply [flat|nested] 5+ messages in thread* [U-Boot] cmd/fdt: support single value replacement within an array
2017-08-18 8:07 ` Hannes Schmelzer
@ 2017-08-18 8:14 ` Laurent Pinchart
2017-08-18 12:49 ` [U-Boot] Antwort: " Hannes Schmelzer
0 siblings, 1 reply; 5+ messages in thread
From: Laurent Pinchart @ 2017-08-18 8:14 UTC (permalink / raw)
To: u-boot
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.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 5+ messages in thread* [U-Boot] Antwort: Re: cmd/fdt: support single value replacement within an array
2017-08-18 8:14 ` Laurent Pinchart
@ 2017-08-18 12:49 ` Hannes Schmelzer
0 siblings, 0 replies; 5+ messages in thread
From: Hannes Schmelzer @ 2017-08-18 12:49 UTC (permalink / raw)
To: u-boot
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-08-18 12:49 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-08-04 21:23 [U-Boot] cmd/fdt: support single value replacement within an array Laurent Pinchart
2017-08-16 7:05 ` Hannes Schmelzer
2017-08-18 8:07 ` Hannes Schmelzer
2017-08-18 8:14 ` Laurent Pinchart
2017-08-18 12:49 ` [U-Boot] Antwort: " Hannes Schmelzer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox