All,
Ping. Any further comments?
Thanks,
-Takahiro Akashi
On Fri, Feb 15, 2019 at 03:12:22PM +0900, AKASHI Takahiro wrote:
> Hi David,
>
> On Wed, Feb 06, 2019 at 03:39:46PM +1100, David Gibson wrote:
> > On Thu, Jan 24, 2019 at 07:20:48PM +0900, AKASHI Takahiro wrote:
> > > This function will append an address range property using parent node's
> > > "#address-cells" and "#size-cells" properties.
> > >
> > > It will be used in implementing kdump with kexec_file_load system call
> > > at linux kernel for arm64 once it is merged into kernel tree.
> > >
> > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@xxxxxxxxxx>
> >
> > Sorry I've taken so long to look at this.
>
> My apologies for not responding quickly.
>
> > > ---
> > > libfdt/fdt_addresses.c | 43 ++++++++++++++++++++++++++++++++++++++++++
> > > libfdt/libfdt.h | 37 ++++++++++++++++++++++++++++++++++++
> > > 2 files changed, 80 insertions(+)
> > >
> > > diff --git a/libfdt/fdt_addresses.c b/libfdt/fdt_addresses.c
> > > index f13a87dfa068..6f13a7bbb6a3 100644
> > > --- a/libfdt/fdt_addresses.c
> > > +++ b/libfdt/fdt_addresses.c
> > > @@ -95,3 +95,46 @@ int fdt_size_cells(const void *fdt, int nodeoffset)
> > > return 1;
> > > return val;
> > > }
> > > +
> > > +static void cpu64_to_fdt_cells(uint8_t *prop, uint64_t val, int cells)
> > > +{
> > > + fdt32_t val32;
> > > +
> > > + while (cells) {
> > > + val32 = cpu_to_fdt32(val >> (32 * (--cells)));
> >
> > I'm not sure if you're intending this to work for cells > 2 - with the
>
> No, but
>
> > upper cells being 0-filled. If so, this will result in a (shift >
> > width of type) which can trip undefined behaviour.
>
> it would be nice to be able to handle "> 2" case even though this function
> is "static" and called only from fdt_appendprop_addrrange().
> I will fix it.
>
> > > + memcpy(prop, &val32, sizeof(val32));
> >
> > It'd be nicer to add fdt{32,64}_st() helpers for this
> >
> > > + prop += sizeof(val32);
> > > + }
> > > +}
> > > +
> > > +/* This function assumes that [address|size]_cells is 1 or 2 */
> > > +int fdt_appendprop_addrrange(void *fdt, int parent, int nodeoffset,
> > > + const char *name, uint64_t addr, uint64_t size)
> > > +{
> > > + int addr_cells, size_cells;
> > > + uint8_t data[sizeof(fdt64_t) * 2];
> >
> > This is only safe for cells <= 2, but you don't actually check for
> > that and error otherwise.
>
> Will add a check.
> What concerned me is an error code. I don't find any suitable value
> according to descriptions in libfdt.h:
>
> FDT_ERR_NOSPACE -> actually we don't suffer from memory allocation
> FDT_ERR_BADNCELLS -> The value itself is NOT bad
> FDT_ERR_BADVALUE -> This is for device tree
> FDT_ERR_INTERNAL -> internal?
>
> > > + addr_cells = fdt_address_cells(fdt, parent);
> > > + if (addr_cells < 0)
> > > + return addr_cells;
> > > + size_cells = fdt_size_cells(fdt, parent);
> > > + if (size_cells < 0)
> > > + return size_cells;
> > > +
> > > + if ((addr_cells == 1) &&
> > > + ((addr > UINT32_MAX) || ((addr + size) > UINT32_MAX)))
> > > + return -FDT_ERR_BADVALUE;
> > > +
> > > + if ((size_cells == 1) && (size > UINT32_MAX))
> > > + return -FDT_ERR_BADVALUE;
> >
> > Given you already have a test for cells == 1 specifically, it's barely
> > worth it to have the quasi-general cpu64_to_fdt_cells() helper.
>
> I'm afraid you misunderstand something.
> We still allow "cells == 2" case here.
>
> > > +
> > > + if (!name)
> > > + name = "reg";
> >
> > Drop this - allowing the user to pass NULL instead of "reg" has no
> > real benefit and makes the interface more complex.
>
> Okay, but gain error code?
>
> FDT_ERR_BADPATH -> It's about property name here, not path
> FDT_ERR_BADVALUE -> This is for device tree
>
> Or can we just ignore this error case?
>
> Once all set, I will send out a new version promptly.
>
> Thanks,
> -Takahiro Akashi
>
> > > +
> > > + cpu64_to_fdt_cells(data, addr, addr_cells);
> > > + cpu64_to_fdt_cells(data + addr_cells * sizeof(fdt32_t), size,
> > > + size_cells);
> > > +
> > > + return fdt_appendprop(fdt, nodeoffset, name, data,
> > > + (addr_cells + size_cells) * sizeof(fdt32_t));
> > > +}
> > > diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
> > > index a470d1df6d2a..c25d980c2547 100644
> > > --- a/libfdt/libfdt.h
> > > +++ b/libfdt/libfdt.h
> > > @@ -1831,6 +1831,43 @@ static inline int fdt_appendprop_cell(void *fdt, int nodeoffset,
> > > #define fdt_appendprop_string(fdt, nodeoffset, name, str) \
> > > fdt_appendprop((fdt), (nodeoffset), (name), (str), strlen(str)+1)
> > >
> > > +/**
> > > + * fdt_appendprop_addrrange - append a address range property
> > > + * @fdt: pointer to the device tree blob
> > > + * @parent: offset of the parent node
> > > + * @nodeoffset: offset of the node to add a property at
> > > + * @name: name of property
> > > + * @addr: start address of a given range
> > > + * @size: size of a given range
> > > + *
> > > + * fdt_appendprop_addrrange() appends an address range value (start
> > > + * address and size) to the value of the named property in the given
> > > + * node, or creates a new property with that value if it does not
> > > + * already exist.
> > > + * If "name" is not specified, a default "reg" is used.
> > > + * Cell sizes are determined by parent's #address-cells and #size-cells.
> > > + *
> > > + * This function may insert data into the blob, and will therefore
> > > + * change the offsets of some existing nodes.
> > > + *
> > > + * returns:
> > > + * 0, on success
> > > + * -FDT_ERR_BADLAYOUT,
> > > + * -FDT_ERR_BADMAGIC,
> > > + * -FDT_ERR_BADNCELLS, if the node has a badly formatted or invalid
> > > + * #address-cells property
> > > + * -FDT_ERR_BADOFFSET, nodeoffset did not point to FDT_BEGIN_NODE tag
> > > + * -FDT_ERR_BADSTATE,
> > > + * -FDT_ERR_BADSTRUCTURE,
> > > + * -FDT_ERR_BADVERSION,
> > > + * -FDT_ERR_BADVALUE, addr or size doesn't fit to respective cells size
> > > + * -FDT_ERR_NOSPACE, there is insufficient free space in the blob to
> > > + * contain a new property
> > > + * -FDT_ERR_TRUNCATED, standard meanings
> > > + */
> > > +int fdt_appendprop_addrrange(void *fdt, int parent, int nodeoffset,
> > > + const char *name, uint64_t addr, uint64_t size);
> > > +
> > > /**
> > > * fdt_delprop - delete a property
> > > * @fdt: pointer to the device tree blob
> >
> > --
> > David Gibson | 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
>
>
