[PATCH] Make of_read_number() handle unaligned data

[PATCH] Make of_read_number() handle unaligned data

[David VomLehn]

Numeric values in properties can be unaligned, so introduce get_unaligned()
in of_read_number() to make this work correctly on all processors.

Signed-off-by: David VomLehn <dvomlehn-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org>
---
 include/linux/of.h |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/include/linux/of.h b/include/linux/of.h
index cad7cf0..a0a6925 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -22,6 +22,7 @@
 #include <linux/spinlock.h>
 
 #include <asm/byteorder.h>
+#include <asm/unaligned.h>
 
 #ifdef CONFIG_OF
 
@@ -110,8 +111,11 @@ extern void of_node_put(struct device_node *node);
 static inline u64 of_read_number(const __be32 *cell, int size)
 {
 	u64 r = 0;
-	while (size--)
-		r = (r << 32) | be32_to_cpu(*(cell++));
+	while (size--) {
+		r = (r << 32) | be32_to_cpu(get_unaligned(cell));
+		cell++;
+	}
+
 	return r;
 }

Re: [PATCH] Make of_read_number() handle unaligned data

[Grant Likely]

On Tue, Feb 22, 2011 at 02:36:47PM -0800, David VomLehn wrote:
> Numeric values in properties can be unaligned, so introduce get_unaligned()
> in of_read_number() to make this work correctly on all processors.
> 
> Signed-off-by: David VomLehn <dvomlehn-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org>

Hi David,

I've thought a lot about this change, (which is a big part of why it
has taken me so long to reply) and while the change itself doesn't
look problematic, I'm really bothered by the implications.  I really
don't like the binding that makes this change necessary.  Mixing cell
and string values is already troublesome.  Placing the cell after the
string so that alignment is wonky is doubly so.  (however, I am
working from memory here about the binding that requires this change.
Can you send it to me again, and I'll have a fresh look).

g.

> ---
>  include/linux/of.h |    8 ++++++--
>  1 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/of.h b/include/linux/of.h
> index cad7cf0..a0a6925 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -22,6 +22,7 @@
>  #include <linux/spinlock.h>
>  
>  #include <asm/byteorder.h>
> +#include <asm/unaligned.h>
>  
>  #ifdef CONFIG_OF
>  
> @@ -110,8 +111,11 @@ extern void of_node_put(struct device_node *node);
>  static inline u64 of_read_number(const __be32 *cell, int size)
>  {
>  	u64 r = 0;
> -	while (size--)
> -		r = (r << 32) | be32_to_cpu(*(cell++));
> +	while (size--) {
> +		r = (r << 32) | be32_to_cpu(get_unaligned(cell));
> +		cell++;
> +	}
> +
>  	return r;
>  }
>  

Re: [PATCH] Make of_read_number() handle unaligned data

[David VomLehn]

On Fri, Mar 04, 2011 at 10:20:45PM -0700, Grant Likely wrote:
> On Tue, Feb 22, 2011 at 02:36:47PM -0800, David VomLehn wrote:
> > Numeric values in properties can be unaligned, so introduce get_unaligned()
> > in of_read_number() to make this work correctly on all processors.
> > 
> > Signed-off-by: David VomLehn <dvomlehn-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org>
> 
> Hi David,
> 
> I've thought a lot about this change, (which is a big part of why it
> has taken me so long to reply) and while the change itself doesn't
> look problematic, I'm really bothered by the implications.  I really
> don't like the binding that makes this change necessary.  Mixing cell
> and string values is already troublesome.  Placing the cell after the
> string so that alignment is wonky is doubly so.  (however, I am
> working from memory here about the binding that requires this change.
> Can you send it to me again, and I'll have a fresh look).
> 
> g.
> 
> > diff --git a/include/linux/of.h b/include/linux/of.h
> > index cad7cf0..a0a6925 100644
> > --- a/include/linux/of.h
> > +++ b/include/linux/of.h
> > @@ -22,6 +22,7 @@
> >  #include <linux/spinlock.h>
> >  
> >  #include <asm/byteorder.h>
> > +#include <asm/unaligned.h>
> >  
> >  #ifdef CONFIG_OF
> >  
> > @@ -110,8 +111,11 @@ extern void of_node_put(struct device_node *node);
> >  static inline u64 of_read_number(const __be32 *cell, int size)
> >  {
> >  	u64 r = 0;
> > -	while (size--)
> > -		r = (r << 32) | be32_to_cpu(*(cell++));
> > +	while (size--) {
> > +		r = (r << 32) | be32_to_cpu(get_unaligned(cell));
> > +		cell++;
> > +	}
> > +
> >  	return r;
> >  }
> >  

Several devices on our system need to use very large, physically
contiguous, buffers. These buffers can be up to 80 MB long. Hardware
constrains some of the buffer to be at statically determined addresses
and the others have addresses dynamically assigned within a specific
range of memory. Both the size of buffers and the requirement to use
specific addresses imply that the buffers must be set up before or
during the time the bootmem allocator is active.

A given device can have buffers with both statically and dynamically
assigned addresses and may have more than one of each type. Drivers
get the address via tag, which is a string.

The specific syntax for a buffer with a dynamically assigned address
(ignoring optional whitespace) is:

	<dbuf-property> ::= "cisco,dynamic-bufs" "=" <dbuf-specs> ";"
	<dbufs-specs> ::= <dbuf-spec> | <dbuf-spec> "," <dbuf-specs>
	<dbuf-spec> ::= <name> "," "<" <size> <alignment> <range-start>
		<range-size> ">"
	<name> ::= '"' <non-quote-chars> '"'
	<size>, <alignment>, <range-start>, and <range-size> are all hex
		constants

Example: cisco,dynamic-bufs =
		"DisplayBins0",<0x00101000 0x1000 0x10000000 0x0fc00000>,
		"DisplayBins1",<0x00101000 0x1000 0x60000000 0x10000000>;

Buffers with statically assigned addresses have the following syntax
(again, ignoring optional whitespace):

	<sbuf-property> ::= "cisco,static-bufs" "=" <sbuf-spec> ";"
	<sbuf-specs ::= <sbuf-spec> | <sbuf-spec> "," <sbuf-specs>
	<sbuf-spec> ::= <name> "," "<" <start-address> <size> ">"
	<start-address> and <size> are hex constants

Example: cisco,static-bufs = "AvfsDmaMem",<0x67c00000 0x00ea0000>;

Since I may have a string sandwiched betweeen cells, and property values
have no padding for alignment, cells are generally not aligned in this
property.

This syntax gives me something pretty readable from the human standpoint
and easy to find in the device tree. Any other option I tried looked
less desirable, but I'm open to suggestions.

-- 
David VL

Re: [PATCH] Make of_read_number() handle unaligned data

[Grant Likely]

On Tue, Mar 08, 2011 at 09:55:09AM -0800, David VomLehn wrote:
> On Fri, Mar 04, 2011 at 10:20:45PM -0700, Grant Likely wrote:
> > On Tue, Feb 22, 2011 at 02:36:47PM -0800, David VomLehn wrote:
> > > Numeric values in properties can be unaligned, so introduce get_unaligned()
> > > in of_read_number() to make this work correctly on all processors.
> > > 
> > > Signed-off-by: David VomLehn <dvomlehn-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org>
> > 
> > Hi David,
> > 
> > I've thought a lot about this change, (which is a big part of why it
> > has taken me so long to reply) and while the change itself doesn't
> > look problematic, I'm really bothered by the implications.  I really
> > don't like the binding that makes this change necessary.  Mixing cell
> > and string values is already troublesome.  Placing the cell after the
> > string so that alignment is wonky is doubly so.  (however, I am
> > working from memory here about the binding that requires this change.
> > Can you send it to me again, and I'll have a fresh look).
> > 
> > g.
> > 
> > > diff --git a/include/linux/of.h b/include/linux/of.h
> > > index cad7cf0..a0a6925 100644
> > > --- a/include/linux/of.h
> > > +++ b/include/linux/of.h
> > > @@ -22,6 +22,7 @@
> > >  #include <linux/spinlock.h>
> > >  
> > >  #include <asm/byteorder.h>
> > > +#include <asm/unaligned.h>
> > >  
> > >  #ifdef CONFIG_OF
> > >  
> > > @@ -110,8 +111,11 @@ extern void of_node_put(struct device_node *node);
> > >  static inline u64 of_read_number(const __be32 *cell, int size)
> > >  {
> > >  	u64 r = 0;
> > > -	while (size--)
> > > -		r = (r << 32) | be32_to_cpu(*(cell++));
> > > +	while (size--) {
> > > +		r = (r << 32) | be32_to_cpu(get_unaligned(cell));
> > > +		cell++;
> > > +	}
> > > +
> > >  	return r;
> > >  }
> > >  
> 
> Several devices on our system need to use very large, physically
> contiguous, buffers. These buffers can be up to 80 MB long. Hardware
> constrains some of the buffer to be at statically determined addresses
> and the others have addresses dynamically assigned within a specific
> range of memory. Both the size of buffers and the requirement to use
> specific addresses imply that the buffers must be set up before or
> during the time the bootmem allocator is active.
> 
> A given device can have buffers with both statically and dynamically
> assigned addresses and may have more than one of each type. Drivers
> get the address via tag, which is a string.
> 
> The specific syntax for a buffer with a dynamically assigned address
> (ignoring optional whitespace) is:
> 
> 	<dbuf-property> ::= "cisco,dynamic-bufs" "=" <dbuf-specs> ";"
> 	<dbufs-specs> ::= <dbuf-spec> | <dbuf-spec> "," <dbuf-specs>
> 	<dbuf-spec> ::= <name> "," "<" <size> <alignment> <range-start>
> 		<range-size> ">"
> 	<name> ::= '"' <non-quote-chars> '"'
> 	<size>, <alignment>, <range-start>, and <range-size> are all hex
> 		constants
> 
> Example: cisco,dynamic-bufs =
> 		"DisplayBins0",<0x00101000 0x1000 0x10000000 0x0fc00000>,
> 		"DisplayBins1",<0x00101000 0x1000 0x60000000 0x10000000>;
> 
> Buffers with statically assigned addresses have the following syntax
> (again, ignoring optional whitespace):
> 
> 	<sbuf-property> ::= "cisco,static-bufs" "=" <sbuf-spec> ";"
> 	<sbuf-specs ::= <sbuf-spec> | <sbuf-spec> "," <sbuf-specs>
> 	<sbuf-spec> ::= <name> "," "<" <start-address> <size> ">"
> 	<start-address> and <size> are hex constants
> 
> Example: cisco,static-bufs = "AvfsDmaMem",<0x67c00000 0x00ea0000>;
> 
> Since I may have a string sandwiched betweeen cells, and property values
> have no padding for alignment, cells are generally not aligned in this
> property.

Right, and it is this bit, plus the mixed string+cell values, that
bothers me.  I'd be much happier with a binding that uses the natural
structure to provide the delineation.  Something like:

	cisco,dynamic-buffers {
		display-bins-0 {
			cisco,size = <size>;
			cisco,alignment = <alignment>;
			cisco,range = <range-start range-size>;
		};
		display-bins-1 {
			cisco,size = <size>;
			cisco,alignment = <alignment>;
			cisco,range = <start size>;
		};
	};
	cisco,static-buffers {
		avfs-dma-mem {
			cisco,size = <size>;
			cisco,range = <start size>;
		};
	};

Which uses the existing dt functions for parsing the data, and is
easy for a driver to find the specific data it needs.  Plus it stays
out of the territory of mixed data types in properties which raises
eyebrows.

g.