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
I think this is the one to use. It's true it's not exactly covered by
the current definition of the error, but I think it's close enough and
not ambiguous. A value of ncells > 2 can't be handled by this
interface, so in that sense it's a "bad" value.
> 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.
Yes, I realize that. But my point is that you have to explicitly
check for the cells==1 and cells==2 options at both levels of the call
stack, which means that cpu64_to_fdt_cells() isn't really abstracting
very much at all. It might be clearer to simply open code the two
available versions of it 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?
For this case of blatantly bad data being passed into the function, I
think it's ok to just ignore (i.e. probably SEGV).
--
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
Attachment:
signature.asc
Description: PGP signature
