| Message ID | 20160624142757.32735-8-maxime.ripard@free-electrons.com |
|---|---|
| State | Superseded |
| Delegated to: | Pantelis Antoniou |
| Headers | show |
On 24 June 2016 at 08:27, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > Add a function to modify inplace a property starting from a given index. > > This is especially useful when the property is an array of values, and you > want to update one of them without changing the DT size. > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > --- > include/libfdt.h | 34 +++++++++++++++++++++++++++++++--- > lib/libfdt/fdt_wip.c | 13 ++++++++----- > 2 files changed, 39 insertions(+), 8 deletions(-) Acked-by: Simon Glass <sjg@chromium.org> I need to get fdt_setprop_inplace() upstream. - Simon
Hi Simon, On Sat, Jun 25, 2016 at 08:53:50PM -0600, Simon Glass wrote: > On 24 June 2016 at 08:27, Maxime Ripard > <maxime.ripard@free-electrons.com> wrote: > > Add a function to modify inplace a property starting from a given index. > > > > This is especially useful when the property is an array of values, and you > > want to update one of them without changing the DT size. > > > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > > --- > > include/libfdt.h | 34 +++++++++++++++++++++++++++++++--- > > lib/libfdt/fdt_wip.c | 13 ++++++++----- > > 2 files changed, 39 insertions(+), 8 deletions(-) > > Acked-by: Simon Glass <sjg@chromium.org> Thanks for taking the time to review it. > I need to get fdt_setprop_inplace() upstream. It's already there (unless you mean another upstream than dtc?) Maxime
On Fri, Jun 24, 2016 at 04:27:52PM +0200, Maxime Ripard wrote: > Add a function to modify inplace a property starting from a given index. > > This is especially useful when the property is an array of values, and you > want to update one of them without changing the DT size. > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > --- > include/libfdt.h | 34 +++++++++++++++++++++++++++++++--- > lib/libfdt/fdt_wip.c | 13 ++++++++----- > 2 files changed, 39 insertions(+), 8 deletions(-) > > diff --git a/include/libfdt.h b/include/libfdt.h > index 4643be5adf39..2c8a42bcb667 100644 > --- a/include/libfdt.h > +++ b/include/libfdt.h > @@ -1032,6 +1032,27 @@ int fdt_size_cells(const void *fdt, int nodeoffset); > /* Write-in-place functions */ > /**********************************************************************/ > > +/** > + * fdt_setprop_inplace_namelen_by_index - change a property's value, > + * but not its size > + * @fdt: pointer to the device tree blob > + * @nodeoffset: offset of the node whose property to change > + * @name: name of the property to change > + * @namelen: number of characters of name to consider > + * @index: index of the property to change in the array > + * @val: pointer to data to replace the property value with > + * @len: length of the property value > + * > + * Identical to fdt_setprop_inplace(), but modifies the given property > + * starting from the given index, and using only the first characters > + * of the name. It is useful when you want to manipulate only one value of > + * an array and you have a string that doesn't end with \0. > + */ > +int fdt_setprop_inplace_namelen_by_index(void *fdt, int nodeoffset, > + const char *name, int namelen, > + uint32_t index, const void *val, > + int len); This looks like a good addition to upstream, but I don't like the name. I'd suggest fdt_setprop_inplace_namelen_partial() because it only overwrite part of the existing property value. > + > /** > * fdt_setprop_inplace - change a property's value, but not its size > * @fdt: pointer to the device tree blob > @@ -1060,8 +1081,13 @@ int fdt_size_cells(const void *fdt, int nodeoffset); > * -FDT_ERR_BADSTRUCTURE, > * -FDT_ERR_TRUNCATED, standard meanings > */ > -int fdt_setprop_inplace(void *fdt, int nodeoffset, const char *name, > - const void *val, int len); > +static inline int fdt_setprop_inplace(void *fdt, int nodeoffset, > + const char *name, const void *val, > + int len) > +{ > + return fdt_setprop_inplace_namelen_by_index(fdt, nodeoffset, name, > + strlen(name), 0, val, len); This effectively removes the error if len is not equal to the existing property length, so that needs to be put back. > +} > > /** > * fdt_setprop_inplace_u32 - change the value of a 32-bit integer property > @@ -1095,7 +1121,9 @@ static inline int fdt_setprop_inplace_u32(void *fdt, int nodeoffset, > const char *name, uint32_t val) > { > fdt32_t tmp = cpu_to_fdt32(val); > - return fdt_setprop_inplace(fdt, nodeoffset, name, &tmp, sizeof(tmp)); > + return fdt_setprop_inplace_namelen_by_index(fdt, nodeoffset, > + name, strlen(name), > + 0, &tmp, sizeof(tmp)); > } > > /** > diff --git a/lib/libfdt/fdt_wip.c b/lib/libfdt/fdt_wip.c > index 9fe988655fe3..737769fa59e2 100644 > --- a/lib/libfdt/fdt_wip.c > +++ b/lib/libfdt/fdt_wip.c > @@ -14,20 +14,23 @@ > > #include "libfdt_internal.h" > > -int fdt_setprop_inplace(void *fdt, int nodeoffset, const char *name, > - const void *val, int len) > +int fdt_setprop_inplace_namelen_by_index(void *fdt, int nodeoffset, > + const char *name, int namelen, > + uint32_t index, const void *val, > + int len) > { > void *propval; > int proplen; > > - propval = fdt_getprop_w(fdt, nodeoffset, name, &proplen); > + propval = fdt_getprop_namelen_w(fdt, nodeoffset, name, namelen, > + &proplen); > if (!propval) > return proplen; > > - if (proplen != len) > + if (proplen < (len + index)) > return -FDT_ERR_NOSPACE; > > - memcpy(propval, val, len); > + memcpy(propval + index, val, len); > return 0; > } >
On Mon, Jun 27, 2016 at 01:45:11AM +1000, David Gibson wrote: > On Fri, Jun 24, 2016 at 04:27:52PM +0200, Maxime Ripard wrote: > > Add a function to modify inplace a property starting from a given index. > > > > This is especially useful when the property is an array of values, and you > > want to update one of them without changing the DT size. > > > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > > --- > > include/libfdt.h | 34 +++++++++++++++++++++++++++++++--- > > lib/libfdt/fdt_wip.c | 13 ++++++++----- > > 2 files changed, 39 insertions(+), 8 deletions(-) > > > > diff --git a/include/libfdt.h b/include/libfdt.h > > index 4643be5adf39..2c8a42bcb667 100644 > > --- a/include/libfdt.h > > +++ b/include/libfdt.h > > @@ -1032,6 +1032,27 @@ int fdt_size_cells(const void *fdt, int nodeoffset); > > /* Write-in-place functions */ > > /**********************************************************************/ > > > > +/** > > + * fdt_setprop_inplace_namelen_by_index - change a property's value, > > + * but not its size > > + * @fdt: pointer to the device tree blob > > + * @nodeoffset: offset of the node whose property to change > > + * @name: name of the property to change > > + * @namelen: number of characters of name to consider > > + * @index: index of the property to change in the array > > + * @val: pointer to data to replace the property value with > > + * @len: length of the property value > > + * > > + * Identical to fdt_setprop_inplace(), but modifies the given property > > + * starting from the given index, and using only the first characters > > + * of the name. It is useful when you want to manipulate only one value of > > + * an array and you have a string that doesn't end with \0. > > + */ > > +int fdt_setprop_inplace_namelen_by_index(void *fdt, int nodeoffset, > > + const char *name, int namelen, > > + uint32_t index, const void *val, > > + int len); > > This looks like a good addition to upstream, but I don't like the > name. I'd suggest fdt_setprop_inplace_namelen_partial() because it > only overwrite part of the existing property value. Ack. > > + > > /** > > * fdt_setprop_inplace - change a property's value, but not its size > > * @fdt: pointer to the device tree blob > > @@ -1060,8 +1081,13 @@ int fdt_size_cells(const void *fdt, int nodeoffset); > > * -FDT_ERR_BADSTRUCTURE, > > * -FDT_ERR_TRUNCATED, standard meanings > > */ > > -int fdt_setprop_inplace(void *fdt, int nodeoffset, const char *name, > > - const void *val, int len); > > +static inline int fdt_setprop_inplace(void *fdt, int nodeoffset, > > + const char *name, const void *val, > > + int len) > > +{ > > + return fdt_setprop_inplace_namelen_by_index(fdt, nodeoffset, name, > > + strlen(name), 0, val, len); > > This effectively removes the error if len is not equal to the existing > property length, so that needs to be put back. So I'm not really sure what you want me to do. I can't check for the property length, since it would prevent that function from working, I can't check for the length + index, since we might update only a few bytes in the middle, and we want to keep that error case. Or should I just forgive about merging the two functions and just duplicate the two? Thanks, Maxime
On Mon, Jun 27, 2016 at 11:16:52AM +0200, Maxime Ripard wrote: > On Mon, Jun 27, 2016 at 01:45:11AM +1000, David Gibson wrote: > > On Fri, Jun 24, 2016 at 04:27:52PM +0200, Maxime Ripard wrote: > > > Add a function to modify inplace a property starting from a given index. > > > > > > This is especially useful when the property is an array of values, and you > > > want to update one of them without changing the DT size. > > > > > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > > > --- > > > include/libfdt.h | 34 +++++++++++++++++++++++++++++++--- > > > lib/libfdt/fdt_wip.c | 13 ++++++++----- > > > 2 files changed, 39 insertions(+), 8 deletions(-) > > > > > > diff --git a/include/libfdt.h b/include/libfdt.h > > > index 4643be5adf39..2c8a42bcb667 100644 > > > --- a/include/libfdt.h > > > +++ b/include/libfdt.h > > > @@ -1032,6 +1032,27 @@ int fdt_size_cells(const void *fdt, int nodeoffset); > > > /* Write-in-place functions */ > > > /**********************************************************************/ > > > > > > +/** > > > + * fdt_setprop_inplace_namelen_by_index - change a property's value, > > > + * but not its size > > > + * @fdt: pointer to the device tree blob > > > + * @nodeoffset: offset of the node whose property to change > > > + * @name: name of the property to change > > > + * @namelen: number of characters of name to consider > > > + * @index: index of the property to change in the array > > > + * @val: pointer to data to replace the property value with > > > + * @len: length of the property value > > > + * > > > + * Identical to fdt_setprop_inplace(), but modifies the given property > > > + * starting from the given index, and using only the first characters > > > + * of the name. It is useful when you want to manipulate only one value of > > > + * an array and you have a string that doesn't end with \0. > > > + */ > > > +int fdt_setprop_inplace_namelen_by_index(void *fdt, int nodeoffset, > > > + const char *name, int namelen, > > > + uint32_t index, const void *val, > > > + int len); > > > > This looks like a good addition to upstream, but I don't like the > > name. I'd suggest fdt_setprop_inplace_namelen_partial() because it > > only overwrite part of the existing property value. > > Ack. > > > > + > > > /** > > > * fdt_setprop_inplace - change a property's value, but not its size > > > * @fdt: pointer to the device tree blob > > > @@ -1060,8 +1081,13 @@ int fdt_size_cells(const void *fdt, int nodeoffset); > > > * -FDT_ERR_BADSTRUCTURE, > > > * -FDT_ERR_TRUNCATED, standard meanings > > > */ > > > -int fdt_setprop_inplace(void *fdt, int nodeoffset, const char *name, > > > - const void *val, int len); > > > +static inline int fdt_setprop_inplace(void *fdt, int nodeoffset, > > > + const char *name, const void *val, > > > + int len) > > > +{ > > > + return fdt_setprop_inplace_namelen_by_index(fdt, nodeoffset, name, > > > + strlen(name), 0, val, len); > > > > This effectively removes the error if len is not equal to the existing > > property length, so that needs to be put back. > > So I'm not really sure what you want me to do. > > I can't check for the property length, since it would prevent that > function from working, I can't check for the length + index, since we > might update only a few bytes in the middle, and we want to keep that > error case. > > Or should I just forgive about merging the two functions and just > duplicate the two? No, what I'm suggesting is that fdt_setprop_inplace(), whether or not it is implemented using fdt_setprop_inplace_partial() *also* checks that the existing property length exactly matches the replacement length, and fails otherwise. Doing this without too much code duplication might involve a private internal function, or maybe it's just simpler to duplicate. But those are the semantics they should have.
diff --git a/include/libfdt.h b/include/libfdt.h index 4643be5adf39..2c8a42bcb667 100644 --- a/include/libfdt.h +++ b/include/libfdt.h @@ -1032,6 +1032,27 @@ int fdt_size_cells(const void *fdt, int nodeoffset); /* Write-in-place functions */ /**********************************************************************/ +/** + * fdt_setprop_inplace_namelen_by_index - change a property's value, + * but not its size + * @fdt: pointer to the device tree blob + * @nodeoffset: offset of the node whose property to change + * @name: name of the property to change + * @namelen: number of characters of name to consider + * @index: index of the property to change in the array + * @val: pointer to data to replace the property value with + * @len: length of the property value + * + * Identical to fdt_setprop_inplace(), but modifies the given property + * starting from the given index, and using only the first characters + * of the name. It is useful when you want to manipulate only one value of + * an array and you have a string that doesn't end with \0. + */ +int fdt_setprop_inplace_namelen_by_index(void *fdt, int nodeoffset, + const char *name, int namelen, + uint32_t index, const void *val, + int len); + /** * fdt_setprop_inplace - change a property's value, but not its size * @fdt: pointer to the device tree blob @@ -1060,8 +1081,13 @@ int fdt_size_cells(const void *fdt, int nodeoffset); * -FDT_ERR_BADSTRUCTURE, * -FDT_ERR_TRUNCATED, standard meanings */ -int fdt_setprop_inplace(void *fdt, int nodeoffset, const char *name, - const void *val, int len); +static inline int fdt_setprop_inplace(void *fdt, int nodeoffset, + const char *name, const void *val, + int len) +{ + return fdt_setprop_inplace_namelen_by_index(fdt, nodeoffset, name, + strlen(name), 0, val, len); +} /** * fdt_setprop_inplace_u32 - change the value of a 32-bit integer property @@ -1095,7 +1121,9 @@ static inline int fdt_setprop_inplace_u32(void *fdt, int nodeoffset, const char *name, uint32_t val) { fdt32_t tmp = cpu_to_fdt32(val); - return fdt_setprop_inplace(fdt, nodeoffset, name, &tmp, sizeof(tmp)); + return fdt_setprop_inplace_namelen_by_index(fdt, nodeoffset, + name, strlen(name), + 0, &tmp, sizeof(tmp)); } /** diff --git a/lib/libfdt/fdt_wip.c b/lib/libfdt/fdt_wip.c index 9fe988655fe3..737769fa59e2 100644 --- a/lib/libfdt/fdt_wip.c +++ b/lib/libfdt/fdt_wip.c @@ -14,20 +14,23 @@ #include "libfdt_internal.h" -int fdt_setprop_inplace(void *fdt, int nodeoffset, const char *name, - const void *val, int len) +int fdt_setprop_inplace_namelen_by_index(void *fdt, int nodeoffset, + const char *name, int namelen, + uint32_t index, const void *val, + int len) { void *propval; int proplen; - propval = fdt_getprop_w(fdt, nodeoffset, name, &proplen); + propval = fdt_getprop_namelen_w(fdt, nodeoffset, name, namelen, + &proplen); if (!propval) return proplen; - if (proplen != len) + if (proplen < (len + index)) return -FDT_ERR_NOSPACE; - memcpy(propval, val, len); + memcpy(propval + index, val, len); return 0; }
Add a function to modify inplace a property starting from a given index. This is especially useful when the property is an array of values, and you want to update one of them without changing the DT size. Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> --- include/libfdt.h | 34 +++++++++++++++++++++++++++++++--- lib/libfdt/fdt_wip.c | 13 ++++++++----- 2 files changed, 39 insertions(+), 8 deletions(-)