From: David Gibson <david@gibson.dropbear.id.au>
To: Ayush Singh <ayush@beagleboard.org>
Cc: d-gole@ti.com, lorforlinux@beagleboard.org,
jkridner@beagleboard.org, robertcnelson@beagleboard.org,
nenad.marinkovic@mikroe.com, Andrew Davis <afd@ti.com>,
Geert Uytterhoeven <geert@linux-m68k.org>,
Robert Nelson <robertcnelson@gmail.com>,
devicetree-compiler@vger.kernel.org
Subject: Re: [PATCH 3/4] libfdt: Add fdt_setprop_namelen()
Date: Thu, 5 Dec 2024 10:45:38 +1100 [thread overview]
Message-ID: <Z1DpojG4q9-bD68G@zatzit> (raw)
In-Reply-To: <125c0168-e6ba-4dc6-8ad7-fa9f283bfbf4@beagleboard.org>
[-- Attachment #1: Type: text/plain, Size: 11836 bytes --]
On Wed, Dec 04, 2024 at 01:09:13PM +0530, Ayush Singh wrote:
>
>
> On 04/12/24 05:59, David Gibson wrote:
> > On Wed, Dec 04, 2024 at 12:00:01AM +0530, Ayush Singh wrote:
> > > Allow specifying name length in setprop similar to
> > > `fdt_get_property_namelen` functions.
> > >
> > > Signed-off-by: Ayush Singh <ayush@beagleboard.org>
> >
> > Two really minor revisions suggested below, otherwise this looks good.
> >
> > > ---
> > > libfdt/fdt_rw.c | 41 +++++++++++++++------------
> > > libfdt/libfdt.h | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++---
> > > libfdt/version.lds | 2 ++
> > > 3 files changed, 103 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/libfdt/fdt_rw.c b/libfdt/fdt_rw.c
> > > index 3621d3651d3f4bd82b7af66c60d023e3139add03..33c60063cd721a147a3c6c0e70450c98d8dbd772 100644
> > > --- a/libfdt/fdt_rw.c
> > > +++ b/libfdt/fdt_rw.c
> > > @@ -124,31 +124,33 @@ static int fdt_splice_string_(void *fdt, int newlen)
> > > * allocated. Ignored if can_assume(NO_ROLLBACK)
> > > * @return offset of string in the string table (whether found or added)
> > > */
> > > -static int fdt_find_add_string_(void *fdt, const char *s, int *allocated)
> > > +static int fdt_find_add_string_(void *fdt, const char *s, int slen,
> > > + int *allocated)
> > > {
> > > char *strtab = (char *)fdt + fdt_off_dt_strings(fdt);
> > > const char *p;
> > > char *new;
> > > - int len = strlen(s) + 1;
> > > int err;
> > > if (!can_assume(NO_ROLLBACK))
> > > *allocated = 0;
> > > - p = fdt_find_string_(strtab, fdt_size_dt_strings(fdt), s);
> > > + p = fdt_find_string_len_(strtab, fdt_size_dt_strings(fdt), s, slen);
> > > if (p)
> > > /* found it */
> > > return (p - strtab);
> > > new = strtab + fdt_size_dt_strings(fdt);
> > > - err = fdt_splice_string_(fdt, len);
> > > + err = fdt_splice_string_(fdt, slen + 1);
> > > if (err)
> > > return err;
> > > if (!can_assume(NO_ROLLBACK))
> > > *allocated = 1;
> > > - memcpy(new, s, len);
> > > + memcpy(new, s, slen);
> > > + new[slen] = '\0';
> > > +
> > > return (new - strtab);
> > > }
> > > @@ -182,12 +184,14 @@ int fdt_del_mem_rsv(void *fdt, int n)
> > > }
> > > static int fdt_resize_property_(void *fdt, int nodeoffset, const char *name,
> > > - int len, struct fdt_property **prop)
> > > + int namelen, int len,
> > > + struct fdt_property **prop)
> >
> > Nit: I'd prefer to see name and namelen grouped together on a line,
> > then len and prop can go on the next.
>
> That exceeds 80 column limit. I am fine with 100 column limit to be honest,
> but well, I would like that limit to apply everywhere (in any new changes),
> instead of just here.
Sorry, I was unclear. I meant:
static int fdt_resize_property_(void *fdt, int nodeoffset,
const char *name, int namelen,
int len, struct fdt_property **prop)
>
> >
> > > {
> > > int oldlen;
> > > int err;
> > > - *prop = fdt_get_property_w(fdt, nodeoffset, name, &oldlen);
> > > + *prop = fdt_get_property_namelen_w(fdt, nodeoffset, name, namelen,
> > > + &oldlen);
> > > if (!*prop)
> > > return oldlen;
> > > @@ -200,7 +204,7 @@ static int fdt_resize_property_(void *fdt, int nodeoffset, const char *name,
> > > }
> > > static int fdt_add_property_(void *fdt, int nodeoffset, const char *name,
> > > - int len, struct fdt_property **prop)
> > > + int namelen, int len, struct fdt_property **prop)
> > > {
> > > int proplen;
> > > int nextoffset;
> > > @@ -211,7 +215,7 @@ static int fdt_add_property_(void *fdt, int nodeoffset, const char *name,
> > > if ((nextoffset = fdt_check_node_offset_(fdt, nodeoffset)) < 0)
> > > return nextoffset;
> > > - namestroff = fdt_find_add_string_(fdt, name, &allocated);
> > > + namestroff = fdt_find_add_string_(fdt, name, namelen, &allocated);
> > > if (namestroff < 0)
> > > return namestroff;
> > > @@ -255,17 +259,18 @@ int fdt_set_name(void *fdt, int nodeoffset, const char *name)
> > > return 0;
> > > }
> > > -int fdt_setprop_placeholder(void *fdt, int nodeoffset, const char *name,
> > > - int len, void **prop_data)
> > > +int fdt_setprop_namelen_placeholder(void *fdt, int nodeoffset, const char *name,
> > > + int namelen, int len, void **prop_data)
> >
> > Sorry, I gave you bad advice in my earlier message. I'd prefer this
> > to be "fdt_setprop_placeholder_namelen()" (compare
> > "fdt_add_subnode_namelen()").
> >
> > > {
> > > struct fdt_property *prop;
> > > int err;
> > > FDT_RW_PROBE(fdt);
> > > - err = fdt_resize_property_(fdt, nodeoffset, name, len, &prop);
> > > + err = fdt_resize_property_(fdt, nodeoffset, name, namelen, len, &prop);
> > > if (err == -FDT_ERR_NOTFOUND)
> > > - err = fdt_add_property_(fdt, nodeoffset, name, len, &prop);
> > > + err = fdt_add_property_(fdt, nodeoffset, name, namelen, len,
> > > + &prop);
> > > if (err)
> > > return err;
> > > @@ -273,13 +278,14 @@ int fdt_setprop_placeholder(void *fdt, int nodeoffset, const char *name,
> > > return 0;
> > > }
> > > -int fdt_setprop(void *fdt, int nodeoffset, const char *name,
> > > - const void *val, int len)
> > > +int fdt_setprop_namelen(void *fdt, int nodeoffset, const char *name,
> > > + int namelen, const void *val, int len)
> > > {
> > > void *prop_data;
> > > int err;
> > > - err = fdt_setprop_placeholder(fdt, nodeoffset, name, len, &prop_data);
> > > + err = fdt_setprop_namelen_placeholder(fdt, nodeoffset, name, namelen,
> > > + len, &prop_data);
> > > if (err)
> > > return err;
> > > @@ -307,7 +313,8 @@ int fdt_appendprop(void *fdt, int nodeoffset, const char *name,
> > > prop->len = cpu_to_fdt32(newlen);
> > > memcpy(prop->data + oldlen, val, len);
> > > } else {
> > > - err = fdt_add_property_(fdt, nodeoffset, name, len, &prop);
> > > + err = fdt_add_property_(fdt, nodeoffset, name, strlen(name),
> > > + len, &prop);
> > > if (err)
> > > return err;
> > > memcpy(prop->data, val, len);
> > > diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
> > > index 7d0c252a34970f4ca841935f8088410a3970127a..c9c81f039365b6d244932786a84a05f065e67c33 100644
> > > --- a/libfdt/libfdt.h
> > > +++ b/libfdt/libfdt.h
> > > @@ -1666,6 +1666,38 @@ int fdt_del_mem_rsv(void *fdt, int n);
> > > */
> > > int fdt_set_name(void *fdt, int nodeoffset, const char *name);
> > > +/**
> > > + * fdt_setprop_namelen - create or change a property
> > > + * @fdt: pointer to the device tree blob
> > > + * @nodeoffset: offset of the node whose property to change
> > > + * @name: name of the property to change
> > > + * @namelen: length of the name
> > > + * @val: pointer to data to set the property value to
> > > + * @len: length of the property value
> > > + *
> > > + * fdt_setprop_namelen() sets the value of the named property in the given
> > > + * node to the given value and length, creating the property if it
> > > + * does not already exist.
> > > + *
> > > + * This function may insert or delete data from the blob, and will
> > > + * therefore change the offsets of some existing nodes.
> > > + *
> > > + * returns:
> > > + * 0, on success
> > > + * -FDT_ERR_NOSPACE, there is insufficient free space in the blob to
> > > + * contain the new property value
> > > + * -FDT_ERR_BADOFFSET, nodeoffset did not point to FDT_BEGIN_NODE tag
> > > + * -FDT_ERR_BADLAYOUT,
> > > + * -FDT_ERR_BADMAGIC,
> > > + * -FDT_ERR_BADVERSION,
> > > + * -FDT_ERR_BADSTATE,
> > > + * -FDT_ERR_BADSTRUCTURE,
> > > + * -FDT_ERR_BADLAYOUT,
> > > + * -FDT_ERR_TRUNCATED, standard meanings
> > > + */
> > > +int fdt_setprop_namelen(void *fdt, int nodeoffset, const char *name,
> > > + int namelen, const void *val, int len);
> > > +
> > > /**
> > > * fdt_setprop - create or change a property
> > > * @fdt: pointer to the device tree blob
> > > @@ -1694,8 +1726,44 @@ int fdt_set_name(void *fdt, int nodeoffset, const char *name);
> > > * -FDT_ERR_BADLAYOUT,
> > > * -FDT_ERR_TRUNCATED, standard meanings
> > > */
> > > -int fdt_setprop(void *fdt, int nodeoffset, const char *name,
> > > - const void *val, int len);
> > > +static inline int fdt_setprop(void *fdt, int nodeoffset, const char *name,
> > > + const void *val, int len)
> > > +{
> > > + return fdt_setprop_namelen(fdt, nodeoffset, name, strlen(name), val,
> > > + len);
> > > +}
> > > +
> > > +/**
> > > + * fdt_setprop_namelen_placeholder - allocate space for a property
> > > + * @fdt: pointer to the device tree blob
> > > + * @nodeoffset: offset of the node whose property to change
> > > + * @name: name of the property to change
> > > + * @namelen: length of the name
> > > + * @len: length of the property value
> > > + * @prop_data: return pointer to property data
> > > + *
> > > + * fdt_setprop_namelen_placeholder() allocates the named property in the given node.
> > > + * If the property exists it is resized. In either case a pointer to the
> > > + * property data is returned.
> > > + *
> > > + * This function may insert or delete data from the blob, and will
> > > + * therefore change the offsets of some existing nodes.
> > > + *
> > > + * returns:
> > > + * 0, on success
> > > + * -FDT_ERR_NOSPACE, there is insufficient free space in the blob to
> > > + * contain the new property value
> > > + * -FDT_ERR_BADOFFSET, nodeoffset did not point to FDT_BEGIN_NODE tag
> > > + * -FDT_ERR_BADLAYOUT,
> > > + * -FDT_ERR_BADMAGIC,
> > > + * -FDT_ERR_BADVERSION,
> > > + * -FDT_ERR_BADSTATE,
> > > + * -FDT_ERR_BADSTRUCTURE,
> > > + * -FDT_ERR_BADLAYOUT,
> > > + * -FDT_ERR_TRUNCATED, standard meanings
> > > + */
> > > +int fdt_setprop_namelen_placeholder(void *fdt, int nodeoffset, const char *name,
> > > + int namelen, int len, void **prop_data);
> > > /**
> > > * fdt_setprop_placeholder - allocate space for a property
> > > @@ -1725,8 +1793,13 @@ int fdt_setprop(void *fdt, int nodeoffset, const char *name,
> > > * -FDT_ERR_BADLAYOUT,
> > > * -FDT_ERR_TRUNCATED, standard meanings
> > > */
> > > -int fdt_setprop_placeholder(void *fdt, int nodeoffset, const char *name,
> > > - int len, void **prop_data);
> > > +static inline int fdt_setprop_placeholder(void *fdt, int nodeoffset,
> > > + const char *name, int len,
> > > + void **prop_data)
> > > +{
> > > + return fdt_setprop_namelen_placeholder(fdt, nodeoffset, name,
> > > + strlen(name), len, prop_data);
> > > +}
> > > /**
> > > * fdt_setprop_u32 - set a property to a 32-bit integer
> > > diff --git a/libfdt/version.lds b/libfdt/version.lds
> > > index 989cd89f1051ce59255a3b3e60493be4e5e985cc..76ba8f6758cef16efbad2813464e824de594b646 100644
> > > --- a/libfdt/version.lds
> > > +++ b/libfdt/version.lds
> > > @@ -43,6 +43,7 @@ LIBFDT_1.2 {
> > > fdt_add_mem_rsv;
> > > fdt_del_mem_rsv;
> > > fdt_set_name;
> > > + fdt_setprop_namelen;
> > > fdt_setprop;
> > > fdt_delprop;
> > > fdt_add_subnode_namelen;
> > > @@ -71,6 +72,7 @@ LIBFDT_1.2 {
> > > fdt_find_max_phandle;
> > > fdt_generate_phandle;
> > > fdt_check_full;
> > > + fdt_setprop_namelen_placeholder;
> > > fdt_setprop_placeholder;
> > > fdt_property_placeholder;
> > > fdt_header_size_;
> > >
> >
>
> Ayush Singh
>
>
--
David Gibson (he or they) | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you, not the other way
| around.
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2024-12-04 23:45 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-12-03 18:29 [PATCH 0/4] libfdt: Add namelen variants for setprop Ayush Singh
2024-12-03 18:29 ` [PATCH 1/4] libfdt: add fdt_get_property_namelen_w() Ayush Singh
2024-12-04 0:21 ` David Gibson
2024-12-03 18:30 ` [PATCH 2/4] libfdt_internal: fdt_find_string_len_() Ayush Singh
2024-12-04 0:22 ` David Gibson
2024-12-03 18:30 ` [PATCH 3/4] libfdt: Add fdt_setprop_namelen() Ayush Singh
2024-12-04 0:29 ` David Gibson
2024-12-04 7:39 ` Ayush Singh
2024-12-04 23:45 ` David Gibson [this message]
2024-12-03 18:30 ` [PATCH 4/4] libfdt: Add fdt_setprop_namelen_string() Ayush Singh
2024-12-04 0:32 ` David Gibson
2024-12-04 0:34 ` [PATCH 0/4] libfdt: Add namelen variants for setprop David Gibson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Z1DpojG4q9-bD68G@zatzit \
--to=david@gibson.dropbear.id.au \
--cc=afd@ti.com \
--cc=ayush@beagleboard.org \
--cc=d-gole@ti.com \
--cc=devicetree-compiler@vger.kernel.org \
--cc=geert@linux-m68k.org \
--cc=jkridner@beagleboard.org \
--cc=lorforlinux@beagleboard.org \
--cc=nenad.marinkovic@mikroe.com \
--cc=robertcnelson@beagleboard.org \
--cc=robertcnelson@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).