On Wed, Nov 30, 2016 at 01:35:04PM -0600, Benjamin Fair wrote:
> On 11/25/2016 04:51 AM, David Gibson wrote:
> > On Wed, Nov 09, 2016 at 10:58:32AM -0600, Benjamin Fair wrote:
> [...]
> >
> > I like the concept of a helper to read entries from reg, but there
> > some things about the execution of it I think need some more thought.
> >
>
> Thanks for the review.
>
> [...]
> > > diff --git a/libfdt/fdt_addresses.c b/libfdt/fdt_addresses.c
> > > index eff4dbc..92cbed9 100644
> > > --- a/libfdt/fdt_addresses.c
> > > +++ b/libfdt/fdt_addresses.c
> > > @@ -55,6 +55,9 @@
> > >
> > > #include "libfdt_internal.h"
> > >
> > > +#define BYTES_PER_CELL 4
> > > +#define BITS_PER_CELL 32
> >
> > You shouldn't need these. BYTES_PER_CELL == sizeof(fdt32_t).
> >
>
> Of course. Thanks. I'll get rid of them.
>
> > > +
> > > int fdt_address_cells(const void *fdt, int nodeoffset)
> > > {
> > > const fdt32_t *ac;
> > > @@ -94,3 +97,62 @@ int fdt_size_cells(const void *fdt, int nodeoffset)
> > >
> > > return val;
> > > }
> > > +
> > > +static uint64_t _fdt_read_cells(const fdt32_t *cells, int n)
> >
> > This is a reasonable helper, but the name is bad. "read_cells"
> > suggests it can read some arbitrary number of cells, but in fact all
> > it can do is read a 32-bit int or a 64-bit int. Plus everything is
> > made up of cells, but more specifically what you're doing here is
> > interpreting several cells as an integer in the usual encoding.
> >
>
> Would "cells_to_integer" be a better name? Or would you recommend something
> else for this?
cells_to_integer would be ok. Or just fdt_read_integer().
>
> > > +{
> > > + int i;
> > > + uint64_t res;
> > > +
> > > + /* TODO: Support values larger than 2 cells */
> >
> > I don't really see any way you could support >2 cells without
> > completely changing the interface.
> >
>
> True. I wanted to have the result be a 128 bit integer, but couldn't find a
> portable way to do so. Is there a better way to go about this? Or is it fine
> to only support at most 2 cells, even though the rest of libfdt supports 4?
I think just supporting 2 cells is ok - it's useful in enough
practical cases. However, I'd suggest thinking of this in a more
layered way.
First, create a helper which just locates the relevant pieces of reg
entries, returning addresses and sizes as (fdt32_t *). That's at
least somewhat useful for the >2 cells case, even though you have to
then parse the address/size yourself. In the most common case for
this, PCI (address-cells == 3), you probably want to do that anyway.
This may also be useful for cases which are <= 2 cells, but the
encoding of the address is not just a plain integer.
You can then polish up fdt_read_integer() a bit and export it.
Finally you can add another helper which combines these to directly
get you the addr+size as u64 for buses where that's suitable. Make
sure to return an error if #a or #s > 2, of course.
> > > + if (n > 2)
> > > + return -FDT_ERR_BADNCELLS;
> > > +
> > > + res = 0;
> > > + for (i = 0; i < n; i++) {
> > > + res <<= BITS_PER_CELL;
> > > + res |= fdt32_to_cpu(cells[i]);
> > > + }
> > > +
> > > + return res;
> > > +}
> > > +
> > > +int fdt_simple_addr_size(const void *fdt, int nodeoffset, int idx,
> > > + uintptr_t *addr, size_t *size)
> > > +{
> > > + int parent;
> > > + int ac, sc, reg_stride;
> > > + int res;
> > > + const fdt32_t *reg;
> > > +
> > > + reg = fdt_getprop(fdt, nodeoffset, "reg", &res);
> > > + if (res < 0)
> > > + return res;
> > > +
> > > + parent = fdt_parent_offset(fdt, nodeoffset);
> > > + if (parent < 0)
> > > + return res;
> >
> > So, fdt_parent_offset() is very expensive, I wouldn't recommend it in
> > a function that's likely to be called a lot like this. Instead I'd
> > suggest a function which takes the parent offset as a parameter, and
> > optionally a wrapper that uses fdt_parent_offset().
>
> Great idea, I'll do this in the next revision once we have a solution for
> the rest of the comments.
>
> >
> > > +
> > > + ac = fdt_address_cells(fdt, parent);
> > > + if (ac < 0)
> > > + return ac;
> > > +
> > > + sc = fdt_size_cells(fdt, parent);
> > > + if (sc < 0)
> > > + return sc;
> > > +
> > > + reg_stride = ac + sc;
> > > + /*
> > > + * res is the number of bytes read and must be an even multiple of the
> > > + * sum of ac and sc.
> > > + */
> > > + if ((res % (reg_stride * BYTES_PER_CELL)) != 0)
> > > + return -FDT_ERR_BADVALUE;
> > > +
> > > + if (addr)
> > > + *addr = (uintptr_t) _fdt_read_cells(®[reg_stride * idx], ac);
> >
> > I don't think uintptr_t makes sense here. The addresses in the device
> > tree are in whatever bus they're in, and there are a whole stack of
> > reasons that could be unrelated to the pointer size of environment
> > libfdt is running in:
> > - The device may be on a subordinate bus whose addresses need
> > to be translated
> > - Even at the top-level, the reg properties represent
> > *physical* addresses, which may not be the same as virtual
> > addresses in code running on the system
> > - libfdt may be running on a completely different system from the
> > one the device tree in question is aimed at (bootloaders are
> > only one use case for libfdt).
> >
> > > + if (size)
> > > + *size = (size_t) _fdt_read_cells(®[ac + reg_stride * idx],
> > > + sc);
> >
> > Likewise size_t isn't necessarily right here, although I suspect it's
> > less likely to break in practice.
> >
>
> Hmm... Is it fine to use uint64_t for both of these instead then?
Yes, I think that's reasonable. Or you could even use unsigned long
or unsigned long long - as long as you error whenever #cells is too
big for the size of that type. uint64_t is probably clearer and more
consistent, though.
--
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
