Re: [PATCH v3 3/3] dtc: Add support for variable sized cells

[David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>]

On Fri, Sep 23, 2011 at 11:02:54AM -0700, Anton Staaf wrote:
> Cells of size 8, 16, 32, and 64 bits are supported.  The new
> /bits/ syntax was selected so as to not pollute the reserved
> keyword space with uint8/uint16/... type names.
> 
> With this patch the following property assignment:
> 
>     property = /bits/ 16 <0x1234 0x5678 0x0 0xffff>;
> 
> is equivalent to:
> 
>     property = <0x12345678 0x0000ffff>;
> 
> It is now also possible to directly specify a 64 bit literal in a
> cell list using:
> 
>     property = /bits/ 64 <0xdeadbeef00000000>;
> 
> It is an error to attempt to store a literal into a cell that is too
> small to hold the literal, and the compiler will generate an error
> when it detects this.  For instance:
> 
>     property = /bits/ 8 <256>;
> 
> Will fail to compile.  It is also an error to attempt to place a
> reference in a non 32-bit cell.

So, there's one small but serious error left in the patch, see below.

Other than that, only two things concern me:

	1) Exactly what to call the keyword.  /bits/ is better than
/size/, but I'd still like to stew a bit on it to see if we can come
up with anything better.

	2) In the documentation and internal naming, we're using
the term "cell" ambiguously.  In the discussion of this extension
we've been using "cell" to mean one array element, which is fair
enough since it's more-or-less standard CS terminology.  However, in
previous FDT and OF terminology "cell" refers to an exactly 32-bit
quantity [well, to be exact OF old-timers say this isn't strictly
right in old OF-ese, but it's close enough to meaning that for most
purposes].
	So, I think we need to find a different name for array cells
to clarify the terminology.  "elements" maybe?

[snip]
> +	| celllistprefix DT_REF
>  		{
> -			$$ = eval_literal($1, 0, 32);
> +			uint64_t val = ~0ULL >> (64 - $1.bits);
> +
> +			if ($1.bits != 32)
> +				print_error("References only allowed in 32-bit"
> +					    " cell lists");
> +
> +			$$.data = data_add_marker($1.data, REF_PHANDLE, $2);
> +			$$.data = data_append_integer($$.data, val, $1.bits);

Uh, so, this is actually worse than what you had before.  Remember
print_error() just prints an error, without aborting the parse.  So
now, if bits != 32 it will print the error, then add a phandle marker
*and* a bits-sized -1.  So later, when we go through and fix up the
REF_PHANDLE markers, we'll assume we're replacing 32-bits of data,
which could overrun the end of the data if the element size is less
than 32.

So, basically the data_add_marker() needs to be in an else clause.

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