Re: [Qemu-devel] [PATCH 4/6] device_tree: Add support for reading device tree properties

[Peter Crosthwaite]

On Sat, Jul 7, 2012 at 1:34 AM, Peter Maydell <address@hidden> wrote:
> On 6 July 2012 02:56, Peter Crosthwaite <address@hidden> wrote:
>> Can we generalise and get functionality for reading cells with offsets
>> as well? Your function assumes (and asserts) that the property is a
>> single cell, but can we add a index parameter for reading a non-0th
>> property out of a multi-cell prop? Needed for reading things like
>> ranges, regs and interrupt properties.
>
> I was playing about with this and I'm really not sure that we should
> be providing a "read a single u32 from a u32 array property" at the
> device_tree.c layer. For example, for handling the ranges property
> what you really want to do is treat it as a list of tuples

The tuples concept layers on top of the cells concept. The meaning of
cells will differ from property to property but cells are cells. The
setter API manages cells but not tuples, so the same should be true of
the getter API. So if you are dealing with tuples, the device tree
layer should provide access to the cells (which is essentially reading
u32 from u32 array) then you can interpret them as tuples if you wish.

I guess what i'm trying to say is that tuples and cells should be
managed quite separately, the former in the client and the latter in
the device tree API.

 (including
> doing something sensible if it doesn't have the right length to be
> a complete list), so the code that knows the structure of the ranges
> property is better off calling qemu_devtree_getprop to get a uint32_t*
> for the whole array.

The logic for the byte reversal should still be in device tree
however. You have to be_to_cpu each read value after reading that
array which is tedious and an easy omission to make. I think if we are
going to wrap for single cell props then it makes sense for multi cell
as well.

Then it has the whole thing as a straightforward
> C array which will be much easier and more efficient to handle than

Efficient yes, but easier no because of the endian issue. For my few
use cases out of tree I only ever get the one property at a time. I
never parse an entire cell array.

> constantly bouncing back into the fdt layer to read each uint32_t.
>

Constantly bouncing back is safer however. If you hang on to an
in-place pointer into the FDT (as returned by get_prop) and someone
comes along and set_props() then your pointer is corrupted. Ive been
snagged before by doing exactly this and eventually came to the
brute-force approach of just requerying the DTB every touch rather
than try to work with pointers to arrays. duping the property could
work, but its a bit of a mess trying to free the returned copies.

(As an aside this is one of the reasons why my chicken-and-egg machine
model uses coroutines instead of threads. I need the get_prop()
followed by its immediate usage to be atomic).

If user care about efficiency over safety, then your get_prop + cast +
endian_swap approach will always be available to them. I just think a
single idx arg at the end creates more options for users. We could
vararg it and if its omitted it just takes the 0th element to keep the
call sites for the 90% case a little more concise.

Regards,
Peter

> I've also just realised that I'm assuming that the pointer returned
> by fdt_getprop() is naturally aligned for a 32 bit integer if the
> property is a 32 bit integer -- is that valid?
>
> -- PMM