Re: [PATCH] libfdt: add helpers to read address and size from reg

[Benjamin Fair <b-fair-l0cyMroinI0@public.gmane.org>]

On 12/01/2016 09:12 PM, David Gibson wrote:
> 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().
>

Thanks for the suggestion. I changed it to 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.
>

That seems like a good idea. I implemented this for the next revision.

>>>> +	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.
>>

Would it be OK to pass in the number of address cells and number of 
size cells directly instead of the parent offset?

I was thinking about this in order to minimize repeated work. Plus, 
the calling function needs to know this information in order to 
interpret the results anyway.

>>>
>>>> +
>>>> +	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[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(&reg[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.
>

Thanks, I changed it to use uint64_t

-- 
Benjamin Fair