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
