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

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

[-- Attachment #1: Type: text/plain, Size: 5413 bytes --]

On Wed, Nov 09, 2016 at 10:58:32AM -0600, Benjamin Fair wrote:
> This patch extends the capability of libfdt to parse the contents of device
> trees in a similar manner to fdt_address_cells and fdt_size_cells.
> 
> It adds a helper function which reads the address and size of a device from
> the reg property and performs basic sanity checks.
> 
> It does not perform translation to a physical address using the ranges
> properties of parents, but this enhancement may be added as a separate
> function in the future.

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.

> Signed-off-by: Benjamin Fair <b-fair-l0cyMroinI0@public.gmane.org>
> ---
> 
> The intent of this patch is similar to the commit "libfdt: Add helpers to read
> #address-cells and #size-cells" [1].
> 
> It is related to "libfdt: add address translation support" [2] but does not
> attempt to perform address translation and was written from scratch rather than
> reusing GPL code. If the issues with that patch are resolved, that
> functionality will complement what is added in this patch.
> 
> [1] http://www.spinics.net/lists/devicetree-compiler/msg00113.html
> [2] http://www.spinics.net/lists/devicetree-compiler/msg00093.html
> 
>  libfdt/fdt_addresses.c | 62 ++++++++++++++++++++++++++++++++++++++++++
>  libfdt/libfdt.h        | 29 ++++++++++++++++++++
>  libfdt/version.lds     |  1 +
>  tests/.gitignore       |  1 +
>  tests/Makefile.tests   |  2 +-
>  tests/addr_size.c      | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/addresses.dts    | 11 ++++++++
>  tests/run_tests.sh     |  1 +
>  8 files changed, 180 insertions(+), 1 deletion(-)
>  create mode 100644 tests/addr_size.c
> 
> 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).

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

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

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

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

> +	return 0;
> +}

-- 
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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]