Re: [PATCH] of/address: fix dt reading on LE machines

Re: [PATCH] of/address: fix dt reading on LE machines

[Grant Likely]

On Mon, Apr 5, 2010 at 7:23 AM, Herring Robert-RA7055
<ra7055-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> Grant,
>
>> -----Original Message-----
>> From: glikely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org [mailto:glikely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org] On
>> Behalf Of Grant Likely
>> Sent: Friday, April 02, 2010 1:46 AM
>> To: Herring Robert-RA7055
>> Cc: Jeremy Kerr
>> Subject: Re: [PATCH] of/address: fix dt reading on LE machines
>>
>> On Thu, Apr 1, 2010 at 6:55 PM, Rob Herring
>> <r.herring-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
>> > The debug prints are still wrong, but the result is correct.
>> >
>> > Signed-off-by: Rob Herring <r.herring-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>> > ---
>> >  drivers/of/address.c |    2 +-
>> >  1 files changed, 1 insertions(+), 1 deletions(-)
>> >
>> > diff --git a/drivers/of/address.c b/drivers/of/address.c
>> > index c3c7c7d..1b285ef 100644
>> > --- a/drivers/of/address.c
>> > +++ b/drivers/of/address.c
>> > @@ -430,7 +430,7 @@ u64 __of_translate_address(struct
>> device_node *dev, const u32 *in_addr,
>> >                /* If root, we have finished */
>> >                if (parent == NULL) {
>> >                        pr_debug("OF: reached root node\n");
>> > -                       result = of_read_number(addr, na);
>> > +                       result = *((u64*)addr);
>>
>> It's too late at night for me to think clearly.  I could use a better
>> explanation on what you found when you investigated this.
>
> Tracing back the source of addr, it does not point to the DTB and is already in LE format. So the conversion is not needed. On BE machine, the conversion is a nop, so it happens to work.
>
> This fix really is just a bandaid on the problem as the debug printk's show. The real problem is all DTB accesses need to go thru of_read_number or other accessors. Also, the accesors should be restricted to DTB accesses only.

Okay, I think I found the problem.  The bus translation functions take
in raw device tree data, but put back out cpu endian data into the
same data buffer.  The kicker is these functions get called multiple
times as addresses get translated between addressing domains; so
endianess can flip back and forth every time translation occurs.  Each
translation hook really needs to emit the same form it accepts.  I
seem to have fixed it now.  Here's the patch (and some unrelated
stuff, just because this is wip).  I'll test is a bit and push it out
to my tree.

It's not a perfect fix because it is still prone to coding errors.  I
should really look at creating a new pointer type instead of void* for
holding raw device tree property data so that I can get the compiler
to catch a lot of the errors for me; but I suspect that will be a lot
of work before I get it fixed.  I need to think about it more.

g.

diff --git a/drivers/of/address.c b/drivers/of/address.c
index 95e4d3d..d81a370 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -24,7 +24,7 @@ static void of_dump_addr(const char *s, const u32
*addr, int na)
 {
 	printk(KERN_DEBUG "%s", s);
 	while (na--)
-		printk(" %08x", *(addr++));
+		printk(" %08x", be32_to_cpu(*(addr++)));
 	printk("\n");
 }
 #else
@@ -81,8 +81,8 @@ static int of_bus_default_translate(u32 *addr, u64
offset, int na)
 	memset(addr, 0, na * 4);
 	a += offset;
 	if (na > 1)
-		addr[na - 2] = a >> 32;
-	addr[na - 1] = a & 0xffffffffu;
+		addr[na - 2] = cpu_to_be32(a >> 32);
+	addr[na - 1] = cpu_to_be32(a & 0xffffffffu);

 	return 0;
 }
@@ -337,6 +337,8 @@ static int of_translate_one(struct device_node
*parent, struct of_bus *bus,
 	int rone;
 	u64 offset = OF_BAD_ADDR;

+	ranges = of_get_property(parent, rprop, &rlen);
+
 	/* Normally, an absence of a "ranges" property means we are
 	 * crossing a non-translatable boundary, and thus the addresses
 	 * below the current not cannot be converted to CPU physical ones.
@@ -348,12 +350,21 @@ static int of_translate_one(struct device_node
*parent, struct of_bus *bus,
 	 * a 1:1 translation at that level. It's up to the caller not to try
 	 * to translate addresses that aren't supposed to be translated in
 	 * the first place. --BenH.
+	 *
+	 * As far as we know, this damage only exists on Apple machines, so
+	 * This code is only enabled on powerpc. --gcl
 	 */
-	ranges = of_get_property(parent, rprop, &rlen);
+#if !defined(CONFIG_PPC)
+	if (ranges == NULL) {
+		pr_err("OF: no ranges; cannot translate\n");
+		return 1;
+	}
+#endif /* !defined(CONFIG_PPC) */
+
 	if (ranges == NULL || rlen == 0) {
 		offset = of_read_number(addr, na);
 		memset(addr, 0, pna * 4);
-		pr_debug("OF: no ranges, 1:1 translation\n");
+		pr_debug("OF: empty ranges; 1:1 translation\n");
 		goto finish;
 	}