[kvm-unit-tests PATCH v2 0/4] devicetree: use correct #address/#size cells

[kvm-unit-tests PATCH v2 0/4] devicetree: use correct #address/#size cells

[Andrew Jones]

I recently noticed that when using the default bus translation (pbus)
we'd always use the root nodes #address/#size cells, which may not be
correct.

v1 fixed this quite wrongly, but Thomas straightened me out. v2 does
the straight-forward (correct) fix in patch 1/4, and then the following
patches are devicetree cleanups.

Thanks,
drew

Andrew Jones (4):
  devicetree: translate with parent node's #address/size-cells
  devicetree: don't cache #address/#size-cells
  devicetree: remove unused globals
  devicetree: dt_get_nr_cells output robustness

 lib/devicetree.c | 38 ++++++++++++++++++++------------------
 lib/devicetree.h |  9 ++-------
 2 files changed, 22 insertions(+), 25 deletions(-)

-- 
2.4.11

[kvm-unit-tests PATCH v2 1/4] devicetree: translate with parent node's #address/size-cells

[Andrew Jones]

Don't assume all pbus (cpu physical bus) translations will use the
same number of address and size cells as are defined in the root
node. Use the parent's (which may be the root node), as the spec
tells us to do.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 lib/devicetree.c | 13 +++++++++++--
 lib/devicetree.h |  4 ++--
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/lib/devicetree.c b/lib/devicetree.c
index a5c7f7c69ddfd..7859968c7a8d2 100644
--- a/lib/devicetree.c
+++ b/lib/devicetree.c
@@ -82,9 +82,18 @@ int dt_pbus_translate_node(int fdtnode, int regidx,
 			   struct dt_pbus_reg *pbus_reg)
 {
 	struct dt_reg raw_reg;
-	int ret;
+	u32 nac, nsc;
+	int parent, ret;
+
+	parent = fdt_parent_offset(fdt, fdtnode);
+	if (parent < 0)
+		return parent;
 
-	dt_reg_init(&raw_reg, root_nr_address_cells, root_nr_size_cells);
+	ret = dt_get_nr_cells(parent, &nac, &nsc);
+	if (ret != 0)
+		return ret;
+
+	dt_reg_init(&raw_reg, nac, nsc);
 
 	ret = dt_get_reg(fdtnode, regidx, &raw_reg);
 	if (ret < 0)
diff --git a/lib/devicetree.h b/lib/devicetree.h
index c8c86eeae28b6..d40243a603925 100644
--- a/lib/devicetree.h
+++ b/lib/devicetree.h
@@ -92,8 +92,8 @@ static inline dt_pbus_addr_t dt_pbus_read_cells(u32 nr_cells, u32 *cells)
 
 /*
  * dt_pbus_translate translates device node regs for the
- * processor bus using the root node's #address-cells and
- * #size-cells and dt_pbus_read_cells()
+ * processor bus using the parent node's #address-cells
+ * and #size-cells and dt_pbus_read_cells()
  * returns
  *  - zero on success
  *  - a negative FDT_ERR_* value on failure
-- 
2.4.11

[kvm-unit-tests PATCH v2 2/4] devicetree: don't cache #address/#size-cells

[Andrew Jones]

The bus structure allows caching of #address/#size-cells. This
is an unnecessary complication, as we can always find that
information in the FDT (and don't care about the overhead).
Anyway, it's currently unused, so let's just remove the code.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 lib/devicetree.c | 4 ----
 lib/devicetree.h | 5 -----
 2 files changed, 9 deletions(-)

diff --git a/lib/devicetree.c b/lib/devicetree.c
index 7859968c7a8d2..2da7d22339a64 100644
--- a/lib/devicetree.c
+++ b/lib/devicetree.c
@@ -278,7 +278,6 @@ int dt_get_default_console_node(void)
 
 int dt_init(const void *fdt_ptr)
 {
-	struct dt_bus *defbus = (struct dt_bus *)&dt_default_bus;
 	int root, ret;
 
 	ret = fdt_check_header(fdt_ptr);
@@ -295,8 +294,5 @@ int dt_init(const void *fdt_ptr)
 	if (ret < 0)
 		return ret;
 
-	defbus->nr_address_cells = root_nr_address_cells;
-	defbus->nr_size_cells = root_nr_size_cells;
-
 	return 0;
 }
diff --git a/lib/devicetree.h b/lib/devicetree.h
index d40243a603925..315ba948e7cc2 100644
--- a/lib/devicetree.h
+++ b/lib/devicetree.h
@@ -66,9 +66,6 @@ struct dt_bus {
 	 *  - a negative FDT_ERR_* value on failure
 	 */
 	int (*translate)(const struct dt_device *dev, int regidx, void *reg);
-
-	/* the bus #address-cells and #size-cells properties */
-	u32 nr_address_cells, nr_size_cells;
 };
 
 /* dt_bus_match_any matches any fdt node, i.e. it always returns true */
@@ -125,8 +122,6 @@ static inline int dt_pbus_get_base(const struct dt_device *dev,
  * dt_bus_init_defaults initializes @bus with
  *  match		<- dt_bus_match_any
  *  translate		<- dt_pbus_translate
- *  nr_address_cells	<- #address-cells of the root node
- *  nr_size_cells	<- #size-cells of the root node
  */
 extern void dt_bus_init_defaults(struct dt_bus *bus);
 
-- 
2.4.11

[kvm-unit-tests PATCH v2 3/4] devicetree: remove unused globals

[Andrew Jones]

root_nr_address_cells and root_nr_size_cells are unused,
thanks to the last two patches. Remove them, and tidy-up
dt_init while at it.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 lib/devicetree.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/lib/devicetree.c b/lib/devicetree.c
index 2da7d22339a64..c091459a94e27 100644
--- a/lib/devicetree.c
+++ b/lib/devicetree.c
@@ -8,7 +8,6 @@
 #include "devicetree.h"
 
 static const void *fdt;
-static u32 root_nr_address_cells, root_nr_size_cells;
 
 const void *dt_fdt(void)
 {
@@ -278,21 +277,16 @@ int dt_get_default_console_node(void)
 
 int dt_init(const void *fdt_ptr)
 {
-	int root, ret;
+	int ret;
 
 	ret = fdt_check_header(fdt_ptr);
 	if (ret < 0)
 		return ret;
-	fdt = fdt_ptr;
-
-	root = fdt_path_offset(fdt, "/");
-	if (root < 0)
-		return root;
 
-	ret = dt_get_nr_cells(root, &root_nr_address_cells,
-				    &root_nr_size_cells);
+	ret = fdt_path_offset(fdt_ptr, "/");
 	if (ret < 0)
 		return ret;
 
+	fdt = fdt_ptr;
 	return 0;
 }
-- 
2.4.11

[kvm-unit-tests PATCH v2 4/4] devicetree: dt_get_nr_cells output robustness

[Andrew Jones]

Only assign outputs if both address and size are found.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 lib/devicetree.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/lib/devicetree.c b/lib/devicetree.c
index c091459a94e27..6ebd4ff5b2a9c 100644
--- a/lib/devicetree.c
+++ b/lib/devicetree.c
@@ -23,21 +23,24 @@ int dt_get_nr_cells(int fdtnode, u32 *nr_address_cells, u32 *nr_size_cells)
 {
 	const struct fdt_property *prop;
 	u32 *nr_cells;
-	int len;
+	int len, nac, nsc;
 
 	prop = fdt_get_property(fdt, fdtnode, "#address-cells", &len);
 	if (prop == NULL)
 		return len;
 
 	nr_cells = (u32 *)prop->data;
-	*nr_address_cells = fdt32_to_cpu(*nr_cells);
+	nac = fdt32_to_cpu(*nr_cells);
 
 	prop = fdt_get_property(fdt, fdtnode, "#size-cells", &len);
 	if (prop == NULL)
 		return len;
 
 	nr_cells = (u32 *)prop->data;
-	*nr_size_cells = fdt32_to_cpu(*nr_cells);
+	nsc = fdt32_to_cpu(*nr_cells);
+
+	*nr_address_cells = nac;
+	*nr_size_cells = nsc;
 
 	return 0;
 }
-- 
2.4.11

Re: [kvm-unit-tests PATCH v2 1/4] devicetree: translate with parent node's #address/size-cells

[Thomas Huth]

On 11.05.2016 15:25, Andrew Jones wrote:
> Don't assume all pbus (cpu physical bus) translations will use the
> same number of address and size cells as are defined in the root
> node. Use the parent's (which may be the root node), as the spec
> tells us to do.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  lib/devicetree.c | 13 +++++++++++--
>  lib/devicetree.h |  4 ++--
>  2 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/devicetree.c b/lib/devicetree.c
> index a5c7f7c69ddfd..7859968c7a8d2 100644
> --- a/lib/devicetree.c
> +++ b/lib/devicetree.c
> @@ -82,9 +82,18 @@ int dt_pbus_translate_node(int fdtnode, int regidx,
>  			   struct dt_pbus_reg *pbus_reg)
>  {
>  	struct dt_reg raw_reg;
> -	int ret;
> +	u32 nac, nsc;
> +	int parent, ret;
> +
> +	parent = fdt_parent_offset(fdt, fdtnode);
> +	if (parent < 0)
> +		return parent;
>  
> -	dt_reg_init(&raw_reg, root_nr_address_cells, root_nr_size_cells);
> +	ret = dt_get_nr_cells(parent, &nac, &nsc);
> +	if (ret != 0)
> +		return ret;
> +
> +	dt_reg_init(&raw_reg, nac, nsc);
>  
>  	ret = dt_get_reg(fdtnode, regidx, &raw_reg);
>  	if (ret < 0)
> diff --git a/lib/devicetree.h b/lib/devicetree.h
> index c8c86eeae28b6..d40243a603925 100644
> --- a/lib/devicetree.h
> +++ b/lib/devicetree.h
> @@ -92,8 +92,8 @@ static inline dt_pbus_addr_t dt_pbus_read_cells(u32 nr_cells, u32 *cells)
>  
>  /*
>   * dt_pbus_translate translates device node regs for the
> - * processor bus using the root node's #address-cells and
> - * #size-cells and dt_pbus_read_cells()
> + * processor bus using the parent node's #address-cells
> + * and #size-cells and dt_pbus_read_cells()
>   * returns
>   *  - zero on success
>   *  - a negative FDT_ERR_* value on failure
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [kvm-unit-tests PATCH v2 2/4] devicetree: don't cache #address/#size-cells

[Thomas Huth]

On 11.05.2016 15:25, Andrew Jones wrote:
> The bus structure allows caching of #address/#size-cells. This
> is an unnecessary complication, as we can always find that
> information in the FDT (and don't care about the overhead).
> Anyway, it's currently unused, so let's just remove the code.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  lib/devicetree.c | 4 ----
>  lib/devicetree.h | 5 -----
>  2 files changed, 9 deletions(-)
> 
> diff --git a/lib/devicetree.c b/lib/devicetree.c
> index 7859968c7a8d2..2da7d22339a64 100644
> --- a/lib/devicetree.c
> +++ b/lib/devicetree.c
> @@ -278,7 +278,6 @@ int dt_get_default_console_node(void)
>  
>  int dt_init(const void *fdt_ptr)
>  {
> -	struct dt_bus *defbus = (struct dt_bus *)&dt_default_bus;
>  	int root, ret;
>  
>  	ret = fdt_check_header(fdt_ptr);
> @@ -295,8 +294,5 @@ int dt_init(const void *fdt_ptr)
>  	if (ret < 0)
>  		return ret;
>  
> -	defbus->nr_address_cells = root_nr_address_cells;
> -	defbus->nr_size_cells = root_nr_size_cells;
> -
>  	return 0;
>  }
> diff --git a/lib/devicetree.h b/lib/devicetree.h
> index d40243a603925..315ba948e7cc2 100644
> --- a/lib/devicetree.h
> +++ b/lib/devicetree.h
> @@ -66,9 +66,6 @@ struct dt_bus {
>  	 *  - a negative FDT_ERR_* value on failure
>  	 */
>  	int (*translate)(const struct dt_device *dev, int regidx, void *reg);
> -
> -	/* the bus #address-cells and #size-cells properties */
> -	u32 nr_address_cells, nr_size_cells;
>  };
>  
>  /* dt_bus_match_any matches any fdt node, i.e. it always returns true */
> @@ -125,8 +122,6 @@ static inline int dt_pbus_get_base(const struct dt_device *dev,
>   * dt_bus_init_defaults initializes @bus with
>   *  match		<- dt_bus_match_any
>   *  translate		<- dt_pbus_translate
> - *  nr_address_cells	<- #address-cells of the root node
> - *  nr_size_cells	<- #size-cells of the root node
>   */
>  extern void dt_bus_init_defaults(struct dt_bus *bus);
>  
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [kvm-unit-tests PATCH v2 4/4] devicetree: dt_get_nr_cells output robustness

[Thomas Huth]

On 11.05.2016 15:25, Andrew Jones wrote:
> Only assign outputs if both address and size are found.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  lib/devicetree.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/devicetree.c b/lib/devicetree.c
> index c091459a94e27..6ebd4ff5b2a9c 100644
> --- a/lib/devicetree.c
> +++ b/lib/devicetree.c
> @@ -23,21 +23,24 @@ int dt_get_nr_cells(int fdtnode, u32 *nr_address_cells, u32 *nr_size_cells)
>  {
>  	const struct fdt_property *prop;
>  	u32 *nr_cells;
> -	int len;
> +	int len, nac, nsc;
>  
>  	prop = fdt_get_property(fdt, fdtnode, "#address-cells", &len);
>  	if (prop == NULL)
>  		return len;
>  
>  	nr_cells = (u32 *)prop->data;
> -	*nr_address_cells = fdt32_to_cpu(*nr_cells);
> +	nac = fdt32_to_cpu(*nr_cells);
>  
>  	prop = fdt_get_property(fdt, fdtnode, "#size-cells", &len);
>  	if (prop == NULL)
>  		return len;
>  
>  	nr_cells = (u32 *)prop->data;
> -	*nr_size_cells = fdt32_to_cpu(*nr_cells);
> +	nsc = fdt32_to_cpu(*nr_cells);
> +
> +	*nr_address_cells = nac;
> +	*nr_size_cells = nsc;
>  
>  	return 0;
>  }

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [kvm-unit-tests PATCH v2 3/4] devicetree: remove unused globals

[Thomas Huth]

On 11.05.2016 15:25, Andrew Jones wrote:
> root_nr_address_cells and root_nr_size_cells are unused,
> thanks to the last two patches. Remove them, and tidy-up
> dt_init while at it.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  lib/devicetree.c | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/lib/devicetree.c b/lib/devicetree.c
> index 2da7d22339a64..c091459a94e27 100644
> --- a/lib/devicetree.c
> +++ b/lib/devicetree.c
> @@ -8,7 +8,6 @@
>  #include "devicetree.h"
>  
>  static const void *fdt;
> -static u32 root_nr_address_cells, root_nr_size_cells;
>  
>  const void *dt_fdt(void)
>  {
> @@ -278,21 +277,16 @@ int dt_get_default_console_node(void)
>  
>  int dt_init(const void *fdt_ptr)
>  {
> -	int root, ret;
> +	int ret;
>  
>  	ret = fdt_check_header(fdt_ptr);
>  	if (ret < 0)
>  		return ret;
> -	fdt = fdt_ptr;
> -
> -	root = fdt_path_offset(fdt, "/");
> -	if (root < 0)
> -		return root;
>  
> -	ret = dt_get_nr_cells(root, &root_nr_address_cells,
> -				    &root_nr_size_cells);
> +	ret = fdt_path_offset(fdt_ptr, "/");

That line is now only a sanity check, right? ... in case you respin, you
could maybe add a comment here what this is good for.

>  	if (ret < 0)
>  		return ret;
>  
> +	fdt = fdt_ptr;
>  	return 0;
>  }
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [kvm-unit-tests PATCH v2 3/4] devicetree: remove unused globals

[Paolo Bonzini]

On 11/05/2016 17:38, Thomas Huth wrote:
> On 11.05.2016 15:25, Andrew Jones wrote:
>> root_nr_address_cells and root_nr_size_cells are unused,
>> thanks to the last two patches. Remove them, and tidy-up
>> dt_init while at it.
>>
>> Signed-off-by: Andrew Jones <drjones@redhat.com>
>> ---
>>  lib/devicetree.c | 12 +++---------
>>  1 file changed, 3 insertions(+), 9 deletions(-)
>>
>> diff --git a/lib/devicetree.c b/lib/devicetree.c
>> index 2da7d22339a64..c091459a94e27 100644
>> --- a/lib/devicetree.c
>> +++ b/lib/devicetree.c
>> @@ -8,7 +8,6 @@
>>  #include "devicetree.h"
>>  
>>  static const void *fdt;
>> -static u32 root_nr_address_cells, root_nr_size_cells;
>>  
>>  const void *dt_fdt(void)
>>  {
>> @@ -278,21 +277,16 @@ int dt_get_default_console_node(void)
>>  
>>  int dt_init(const void *fdt_ptr)
>>  {
>> -	int root, ret;
>> +	int ret;
>>  
>>  	ret = fdt_check_header(fdt_ptr);
>>  	if (ret < 0)
>>  		return ret;
>> -	fdt = fdt_ptr;
>> -
>> -	root = fdt_path_offset(fdt, "/");
>> -	if (root < 0)
>> -		return root;
>>  
>> -	ret = dt_get_nr_cells(root, &root_nr_address_cells,
>> -				    &root_nr_size_cells);
>> +	ret = fdt_path_offset(fdt_ptr, "/");
> 
> That line is now only a sanity check, right? ... in case you respin, you
> could maybe add a comment here what this is good for.

I can do that like this:

diff --git a/lib/devicetree.c b/lib/devicetree.c
index c091459..b9f1d3d 100644
--- a/lib/devicetree.c
+++ b/lib/devicetree.c
@@ -283,6 +283,7 @@ int dt_init(const void *fdt_ptr)
 	if (ret < 0)
 		return ret;

+	/* Sanity check the path.  */
 	ret = fdt_path_offset(fdt_ptr, "/");
 	if (ret < 0)
 		return ret;


Andrew, is this okay?

Thanks,

Paolo

Re: [kvm-unit-tests PATCH v2 3/4] devicetree: remove unused globals

[Andrew Jones]

On Wed, May 11, 2016 at 05:46:30PM +0200, Paolo Bonzini wrote:
> 
> 
> On 11/05/2016 17:38, Thomas Huth wrote:
> > On 11.05.2016 15:25, Andrew Jones wrote:
> >> root_nr_address_cells and root_nr_size_cells are unused,
> >> thanks to the last two patches. Remove them, and tidy-up
> >> dt_init while at it.
> >>
> >> Signed-off-by: Andrew Jones <drjones@redhat.com>
> >> ---
> >>  lib/devicetree.c | 12 +++---------
> >>  1 file changed, 3 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/lib/devicetree.c b/lib/devicetree.c
> >> index 2da7d22339a64..c091459a94e27 100644
> >> --- a/lib/devicetree.c
> >> +++ b/lib/devicetree.c
> >> @@ -8,7 +8,6 @@
> >>  #include "devicetree.h"
> >>  
> >>  static const void *fdt;
> >> -static u32 root_nr_address_cells, root_nr_size_cells;
> >>  
> >>  const void *dt_fdt(void)
> >>  {
> >> @@ -278,21 +277,16 @@ int dt_get_default_console_node(void)
> >>  
> >>  int dt_init(const void *fdt_ptr)
> >>  {
> >> -	int root, ret;
> >> +	int ret;
> >>  
> >>  	ret = fdt_check_header(fdt_ptr);
> >>  	if (ret < 0)
> >>  		return ret;
> >> -	fdt = fdt_ptr;
> >> -
> >> -	root = fdt_path_offset(fdt, "/");
> >> -	if (root < 0)
> >> -		return root;
> >>  
> >> -	ret = dt_get_nr_cells(root, &root_nr_address_cells,
> >> -				    &root_nr_size_cells);
> >> +	ret = fdt_path_offset(fdt_ptr, "/");
> > 
> > That line is now only a sanity check, right? ... in case you respin, you
> > could maybe add a comment here what this is good for.
> 
> I can do that like this:
> 
> diff --git a/lib/devicetree.c b/lib/devicetree.c
> index c091459..b9f1d3d 100644
> --- a/lib/devicetree.c
> +++ b/lib/devicetree.c
> @@ -283,6 +283,7 @@ int dt_init(const void *fdt_ptr)
>  	if (ret < 0)
>  		return ret;
> 
> +	/* Sanity check the path.  */
>  	ret = fdt_path_offset(fdt_ptr, "/");
>  	if (ret < 0)
>  		return ret;
> 
> 
> Andrew, is this okay?

Yup, that's exactly what I had in mind.

Thanks,
drew
> 
> Thanks,
> 
> Paolo
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html