Hi David,
On Mon, Mar 04, 2019 at 05:41:36PM +1100, David Gibson wrote:
> 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.
Okay.
> > 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.
Well, there is a long history here.
When I initially introduced this function, named fill_property(),
as part of kernel code, it looked like:
if (cells == 1)
...
else /* cells == 2 */
...
Rob, however, suggested that it should be generalized using a while loop.
(He commented on the code in a different way recently though.)
Now that we have additional size checks, I agree to you and will follow
your suggestion here, but want to never revert it again :)
> > > > +
> > > > + 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).
Okay, we'd better defer it to fdt_appendprop().
Thanks,
-Takahiro Akashi
> --
> 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
