Skip to contentSkip to conversation

of/flattree: Use common ALIGN() macro instead of arch specific _ALIGN

PATCH In review Grant Likely 3 participants 11 messages Jul 14, 2010 – Jul 19, 2010 2 messages not in the archive

Subsystems (3)
PowerPC Device tree Microblaze architecture
lore
Share & tools
lore

Grant Likely

author
#0 lore raw

PATCH 1/3 of/flattree: Use common ALIGN() macro instead of arch specific _ALIGN

There's no reason to use the powerpc-specific _ALIGN macro in the fdt code. Replace it with ALIGN() from kernel.h

Signed-off-by Grant Likely <grant.likely-s3s/[email protected]>
---
 arch/microblaze/include/asm/page.h |    7 -------
 drivers/of/fdt.c                   |   20 ++++++++++----------
 2 files changed, 10 insertions(+), 17 deletions(-)
arch/microblaze/include/asm/page.h +0 -7
index 464ff32..2fd4761 100644
@@ -39,13 +39,6 @@
 #define PAGE_UP(addr)	(((addr)+((PAGE_SIZE)-1))&(~((PAGE_SIZE)-1)))
 #define PAGE_DOWN(addr)	((addr)&(~((PAGE_SIZE)-1)))
 
-/* align addr on a size boundary - adjust address up/down if needed */
-#define _ALIGN_UP(addr, size)	(((addr)+((size)-1))&(~((size)-1)))
-#define _ALIGN_DOWN(addr, size)	((addr)&(~((size)-1)))
-
-/* align addr on a size boundary - adjust address up if needed */
-#define _ALIGN(addr, size)	_ALIGN_UP(addr, size)
-
 #ifndef CONFIG_MMU
 /*
  * PAGE_OFFSET -- the first address of the first page of memory. When not
drivers/of/fdt.c +10 -10
index b6987bb..d61fda8 100644
@@ -69,9 +69,9 @@ int __init of_scan_flat_dt(int (*it)(unsigned long node,
 			u32 sz = be32_to_cpup((__be32 *)p);
 			p += 8;
 			if (be32_to_cpu(initial_boot_params->version) < 0x10)
-				p = _ALIGN(p, sz >= 8 ? 8 : 4);
+				p = ALIGN(p, sz >= 8 ? 8 : 4);
 			p += sz;
-			p = _ALIGN(p, 4);
+			p = ALIGN(p, 4);
 			continue;
 		}
 		if (tag != OF_DT_BEGIN_NODE) {

@@ -80,7 +80,7 @@ int __init of_scan_flat_dt(int (*it)(unsigned long node,
 		}
 		depth++;
 		pathp = (char *)p;
-		p = _ALIGN(p + strlen(pathp) + 1, 4);
+		p = ALIGN(p + strlen(pathp) + 1, 4);
 		if ((*pathp) == '/') {
 			char *lp, *np;
 			for (lp = NULL, np = pathp; *np; np++)

@@ -109,7 +109,7 @@ unsigned long __init of_get_flat_dt_root(void)
 		p += 4;
 	BUG_ON(be32_to_cpup((__be32 *)p) != OF_DT_BEGIN_NODE);
 	p += 4;
-	return _ALIGN(p + strlen((char *)p) + 1, 4);
+	return ALIGN(p + strlen((char *)p) + 1, 4);
 }
 
 /**

@@ -138,7 +138,7 @@ void *__init of_get_flat_dt_prop(unsigned long node, const char *name,
 		noff = be32_to_cpup((__be32 *)(p + 4));
 		p += 8;
 		if (be32_to_cpu(initial_boot_params->version) < 0x10)
-			p = _ALIGN(p, sz >= 8 ? 8 : 4);
+			p = ALIGN(p, sz >= 8 ? 8 : 4);
 
 		nstr = find_flat_dt_string(noff);
 		if (nstr == NULL) {

@@ -151,7 +151,7 @@ void *__init of_get_flat_dt_prop(unsigned long node, const char *name,
 			return (void *)p;
 		}
 		p += sz;
-		p = _ALIGN(p, 4);
+		p = ALIGN(p, 4);
 	} while (1);
 }
 

@@ -184,7 +184,7 @@ static void *__init unflatten_dt_alloc(unsigned long *mem, unsigned long size,
 {
 	void *res;
 
-	*mem = _ALIGN(*mem, align);
+	*mem = ALIGN(*mem, align);
 	res = (void *)*mem;
 	*mem += size;
 

@@ -220,7 +220,7 @@ unsigned long __init unflatten_dt_node(unsigned long mem,
 	*p += 4;
 	pathp = (char *)*p;
 	l = allocl = strlen(pathp) + 1;
-	*p = _ALIGN(*p + l, 4);
+	*p = ALIGN(*p + l, 4);
 
 	/* version 0x10 has a more compact unit name here instead of the full
 	 * path. we accumulate the full path size using "fpsize", we'll rebuild

@@ -299,7 +299,7 @@ unsigned long __init unflatten_dt_node(unsigned long mem,
 		noff = be32_to_cpup((__be32 *)((*p) + 4));
 		*p += 8;
 		if (be32_to_cpu(initial_boot_params->version) < 0x10)
-			*p = _ALIGN(*p, sz >= 8 ? 8 : 4);
+			*p = ALIGN(*p, sz >= 8 ? 8 : 4);
 
 		pname = find_flat_dt_string(noff);
 		if (pname == NULL) {

@@ -333,7 +333,7 @@ unsigned long __init unflatten_dt_node(unsigned long mem,
 			*prev_pp = pp;
 			prev_pp = &pp->next;
 		}
-		*p = _ALIGN((*p) + sz, 4);
+		*p = ALIGN((*p) + sz, 4);
 	}
 	/* with version 0x10 we may not have the name property, recreate
 	 * it here from the unit name if absent
author
#1 lore raw

PATCH 2/3 of/flattree: Fix crash when device tree absent

This patch fixes the condition where device tree support is compiled in, but no device tree was proved by firmware. It makes of_platform_bus_probe() explicitly check for a NULL device tree pointer, and adds an error message if the device tree was unable to be flattened.

Signed-off-by Grant Likely <grant.likely-s3s/[email protected]>
---
 drivers/of/fdt.c      |    2 ++
 drivers/of/platform.c |    2 ++
 2 files changed, 4 insertions(+), 0 deletions(-)
drivers/of/fdt.c +2 -0
index d61fda8..66401bc 100644
@@ -94,6 +94,8 @@ int __init of_scan_flat_dt(int (*it)(unsigned long node,
 			break;
 	} while (1);
 
+	if (rc)
+		pr_err("Failed to unflatten device tree blob.  rc=%i\n", rc);
 	return rc;
 }
drivers/of/platform.c +2 -0
index f0ca906..9bc6999 100644
@@ -674,6 +674,8 @@ int of_platform_bus_probe(struct device_node *root,
 		root = of_find_node_by_path("/");
 	else
 		of_node_get(root);
+	if (root == NULL)
+		return -EINVAL;
 
 	pr_debug("of_platform_bus_probe()\n");
 	pr_debug(" starting at: %s\n", root->full_name);
#2 lore raw

Re: [PATCH 2/3] of/flattree: Fix crash when device tree absent

Hi Grant,

drivers/of/fdt.c int __init of_scan_flat_dt(int (*it)(unsigned long node, · line 100

Quoted 12 lines
drivers/of/fdt.c +2 -0
index d61fda8..66401bc 100644
@@ -94,6 +94,8 @@ int __init of_scan_flat_dt(int (*it)(unsigned long node,
 			break;
 	} while (1);
 
+	if (rc)
+		pr_err("Failed to unflatten device tree blob.  rc=%i\n", rc);
 	return rc;
 }

rc == 1 is perfectly valid - it indicates that the iterator callback stopped the scan early.

How about we just check for the presence of OF_DT_HEADER instead?

Cheers,

Jeremy

A message that is not in the archive (sent off-list or not yet received): 1279156262.15089.68.camel-353AF/[email protected]

author
#4 lore raw

Re: [PATCH 2/3] of/flattree: Fix crash when device tree absent

Quoted 10 lines · Jeremy Kerr

Hi Grant,

Quoted 8 lines
drivers/of/fdt.c +0 -0
index d61fda8..66401bc 100644
@@ -94,6 +94,8 @@ int __init of_scan_flat_dt(int (*it)(unsigned long node,
                       break;
       } while (1);
Quoted 7 lines

drivers/of/fdt.c int __init of_scan_flat_dt(int (*it)(unsigned long node, · line 100

Quoted 4 lines
drivers/of/fdt.c +2 -0
+     if (rc)
+             pr_err("Failed to unflatten device tree blob.  rc=%i\n", rc);
       return rc;
  }

rc == 1 is perfectly valid - it indicates that the iterator callback stopped the scan early.

This was just an informational output anyway. I'll just drop this hunk.

Quoted 1 line

How about we just check for the presence of OF_DT_HEADER instead?

Probably a good idea. We'll need more robust dtb pointer checking with ARM. I'll do that in a separate patch.

g.

A message that is not in the archive (sent off-list or not yet received): AANLkTikD3ZhhVg4PTuG5Zkc38oW1I6yaQyKqcv9rAaoR-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

#7 lore raw

Re: [PATCH 2/3] of/flattree: Fix crash when device tree absent

In reply to Grant Likely
Quoted 7 lines · Grant Likely

This patch fixes the condition where device tree support is compiled in, but no device tree was proved by firmware. It makes of_platform_bus_probe() explicitly check for a NULL device tree pointer, and adds an error message if the device tree was unable to be flattened.

Signed-off-by Grant Likely <grant.likely-s3s/[email protected]>

Not sure about the message..

Quoted 10 lines
---
 drivers/of/fdt.c      |    2 ++
 drivers/of/platform.c |    2 ++
 2 files changed, 4 insertions(+), 0 deletions(-)
drivers/of/fdt.c +0 -0
index d61fda8..66401bc 100644

drivers/of/fdt.c int __init of_scan_flat_dt(int (*it)(unsigned long node, · line 100

Quoted 7 lines
drivers/of/fdt.c +2 -0
@@ -94,6 +94,8 @@ int __init of_scan_flat_dt(int (*it)(unsigned long node,
 			break;
 	} while (1);
 
+	if (rc)
+		pr_err("Failed to unflatten device tree blob.  rc=%i\n", rc);
 	return rc;
 }

This is of_scan_flat_dt() ... might be better using something like

"Failed to scan the flat device-tree (error %d)". Might also want to rate limit it to 1.

drivers/of/platform.c int of_platform_bus_probe(struct device_node *root, · line 681

Quoted 13 lines
drivers/of/platform.c +2 -0
index f0ca906..9bc6999 100644
@@ -674,6 +674,8 @@ int of_platform_bus_probe(struct device_node *root,
 		root = of_find_node_by_path("/");
 	else
 		of_node_get(root);
+	if (root == NULL)
+		return -EINVAL;
 
 	pr_debug("of_platform_bus_probe()\n");
 	pr_debug(" starting at: %s\n", root->full_name);

Cheers,
Ben.

author
#8 lore raw

Re: [PATCH 2/3] of/flattree: Fix crash when device tree absent

Quoted 10 lines · Benjamin Herrenschmidt
Quoted 7 lines · Grant Likely

This patch fixes the condition where device tree support is compiled in, but no device tree was proved by firmware.  It makes of_platform_bus_probe() explicitly check for a NULL device tree pointer, and adds an error message if the device tree was unable to be flattened.

Signed-off-by Grant Likely <grant.likely-s3s/[email protected]>

Not sure about the message..

What do you mean? On the draft ARM code, CONFIG_OF can be set, but no device tree passed in. As for the error message, I've removed that from this patch (see below).

Quoted 21 lines
Quoted 11 lines

---
 drivers/of/fdt.c      |    2 ++
 drivers/of/platform.c |    2 ++

 2 files changed, 4 insertions(+), 0 deletions(-)
drivers/of/fdt.c +0 -0
index d61fda8..66401bc 100644

drivers/of/fdt.c int __init of_scan_flat_dt(int (*it)(unsigned long node, · line 100

Quoted 7 lines
drivers/of/fdt.c +2 -0
@@ -94,6 +94,8 @@ int __init of_scan_flat_dt(int (*it)(unsigned long node,
                       break;
       } while (1);
 
+     if (rc)
+             pr_err("Failed to unflatten device tree blob.  rc=%i\n", rc);
       return rc;
  }

This is of_scan_flat_dt() ... might be better using something like

Quoted 2 lines

"Failed to scan the flat device-tree (error %d)". Might also want to rate limit it to 1.

I've dropped this hunk anyway. IIRC, I added this message to the unflatten function, but it looks to have gotten mangled during a rebase. Anyway, there is no need for it right now, so I'll just write a separate patch later if that changes.

g.

author
#9 lore raw

PATCH 3/3 of: Remove unused of_find_device_by_phandle()

In reply to Grant Likely
Signed-off-by Grant Likely <grant.likely-s3s/[email protected]>
---
 arch/microblaze/include/asm/of_platform.h |    2 --
 arch/microblaze/kernel/of_platform.c      |   18 ------------------
 arch/powerpc/include/asm/of_platform.h    |    2 --
 arch/powerpc/kernel/of_platform.c         |   19 -------------------
 4 files changed, 0 insertions(+), 41 deletions(-)
arch/microblaze/include/asm/of_platform.h +0 -2
index 625003f..353d8f6 100644
@@ -14,8 +14,6 @@
 /* This is just here during the transition */
 #include <linux/of_platform.h>
 
-extern struct of_device *of_find_device_by_phandle(phandle ph);
-
 extern void of_instantiate_rtc(void);
 
 #endif /* _ASM_MICROBLAZE_OF_PLATFORM_H */
arch/microblaze/kernel/of_platform.c +0 -18
index 33212bb..80c9c49 100644
@@ -63,21 +63,3 @@ struct of_device *of_find_device_by_node(struct device_node *np)
 	return NULL;
 }
 EXPORT_SYMBOL(of_find_device_by_node);
-
-static int of_dev_phandle_match(struct device *dev, void *data)
-{
-	phandle *ph = data;
-	return to_of_device(dev)->dev.of_node->phandle == *ph;
-}
-
-struct of_device *of_find_device_by_phandle(phandle ph)
-{
-	struct device *dev;
-
-	dev = bus_find_device(&platform_bus_type,
-			      NULL, &ph, of_dev_phandle_match);
-	if (dev)
-		return to_of_device(dev);
-	return NULL;
-}
-EXPORT_SYMBOL(of_find_device_by_phandle);
arch/powerpc/include/asm/of_platform.h +0 -2
index b37d2dc..d506aa6 100644
@@ -11,8 +11,6 @@
  *
  */
 
-extern struct of_device *of_find_device_by_phandle(phandle ph);
-
 extern void of_instantiate_rtc(void);
 
 #endif	/* _ASM_POWERPC_OF_PLATFORM_H */
arch/powerpc/kernel/of_platform.c +0 -19
index a3d7154..b093d4b 100644
@@ -68,25 +68,6 @@ struct of_device *of_find_device_by_node(struct device_node *np)
 }
 EXPORT_SYMBOL(of_find_device_by_node);
 
-static int of_dev_phandle_match(struct device *dev, void *data)
-{
-	phandle *ph = data;
-	return to_of_device(dev)->dev.of_node->phandle == *ph;
-}
-
-struct of_device *of_find_device_by_phandle(phandle ph)
-{
-	struct device *dev;
-
-	dev = bus_find_device(&platform_bus_type,
-			      NULL, &ph, of_dev_phandle_match);
-	if (dev)
-		return to_of_device(dev);
-	return NULL;
-}
-EXPORT_SYMBOL(of_find_device_by_phandle);
-
-
 #ifdef CONFIG_PPC_OF_PLATFORM_PCI
 
 /* The probing of PCI controllers from of_platform is currently
Acked-by on v1 3/3
#10 lore raw

Re: [PATCH 3/3] of: Remove unused of_find_device_by_phandle()

Quoted 1 line · Grant Likely
Signed-off-by Grant Likely <grant.likely-s3s/[email protected]>
Acked-by Benjamin Herrenschmidt <[email protected]>
Quoted 89 lines
---
 arch/microblaze/include/asm/of_platform.h |    2 --
 arch/microblaze/kernel/of_platform.c      |   18 ------------------
 arch/powerpc/include/asm/of_platform.h    |    2 --
 arch/powerpc/kernel/of_platform.c         |   19 -------------------
 4 files changed, 0 insertions(+), 41 deletions(-)
arch/microblaze/include/asm/of_platform.h +0 -2
index 625003f..353d8f6 100644
@@ -14,8 +14,6 @@
 /* This is just here during the transition */
 #include <linux/of_platform.h>
 
-extern struct of_device *of_find_device_by_phandle(phandle ph);
-
 extern void of_instantiate_rtc(void);
 
 #endif /* _ASM_MICROBLAZE_OF_PLATFORM_H */
arch/microblaze/kernel/of_platform.c +0 -18
index 33212bb..80c9c49 100644
@@ -63,21 +63,3 @@ struct of_device *of_find_device_by_node(struct device_node *np)
 	return NULL;
 }
 EXPORT_SYMBOL(of_find_device_by_node);
-
-static int of_dev_phandle_match(struct device *dev, void *data)
-{
-	phandle *ph = data;
-	return to_of_device(dev)->dev.of_node->phandle == *ph;
-}
-
-struct of_device *of_find_device_by_phandle(phandle ph)
-{
-	struct device *dev;
-
-	dev = bus_find_device(&platform_bus_type,
-			      NULL, &ph, of_dev_phandle_match);
-	if (dev)
-		return to_of_device(dev);
-	return NULL;
-}
-EXPORT_SYMBOL(of_find_device_by_phandle);
arch/powerpc/include/asm/of_platform.h +0 -2
index b37d2dc..d506aa6 100644
@@ -11,8 +11,6 @@
  *
  */
 
-extern struct of_device *of_find_device_by_phandle(phandle ph);
-
 extern void of_instantiate_rtc(void);
 
 #endif	/* _ASM_POWERPC_OF_PLATFORM_H */
arch/powerpc/kernel/of_platform.c +0 -19
index a3d7154..b093d4b 100644
@@ -68,25 +68,6 @@ struct of_device *of_find_device_by_node(struct device_node *np)
 }
 EXPORT_SYMBOL(of_find_device_by_node);
 
-static int of_dev_phandle_match(struct device *dev, void *data)
-{
-	phandle *ph = data;
-	return to_of_device(dev)->dev.of_node->phandle == *ph;
-}
-
-struct of_device *of_find_device_by_phandle(phandle ph)
-{
-	struct device *dev;
-
-	dev = bus_find_device(&platform_bus_type,
-			      NULL, &ph, of_dev_phandle_match);
-	if (dev)
-		return to_of_device(dev);
-	return NULL;
-}
-EXPORT_SYMBOL(of_find_device_by_phandle);
-
-
 #ifdef CONFIG_PPC_OF_PLATFORM_PCI
 
 /* The probing of PCI controllers from of_platform is currently
Acked-by on v1 1/3
#12 lore raw
In reply to Grant Likely
Quoted 4 lines · Grant Likely

There's no reason to use the powerpc-specific _ALIGN macro in the fdt code. Replace it with ALIGN() from kernel.h

Signed-off-by Grant Likely <grant.likely-s3s/[email protected]>
Acked-by Benjamin Herrenschmidt <[email protected]>
Quoted 111 lines
---
 arch/microblaze/include/asm/page.h |    7 -------
 drivers/of/fdt.c                   |   20 ++++++++++----------
 2 files changed, 10 insertions(+), 17 deletions(-)
arch/microblaze/include/asm/page.h +0 -7
index 464ff32..2fd4761 100644
@@ -39,13 +39,6 @@
 #define PAGE_UP(addr)	(((addr)+((PAGE_SIZE)-1))&(~((PAGE_SIZE)-1)))
 #define PAGE_DOWN(addr)	((addr)&(~((PAGE_SIZE)-1)))
 
-/* align addr on a size boundary - adjust address up/down if needed */
-#define _ALIGN_UP(addr, size)	(((addr)+((size)-1))&(~((size)-1)))
-#define _ALIGN_DOWN(addr, size)	((addr)&(~((size)-1)))
-
-/* align addr on a size boundary - adjust address up if needed */
-#define _ALIGN(addr, size)	_ALIGN_UP(addr, size)
-
 #ifndef CONFIG_MMU
 /*
  * PAGE_OFFSET -- the first address of the first page of memory. When not
drivers/of/fdt.c +10 -10
index b6987bb..d61fda8 100644
@@ -69,9 +69,9 @@ int __init of_scan_flat_dt(int (*it)(unsigned long node,
 			u32 sz = be32_to_cpup((__be32 *)p);
 			p += 8;
 			if (be32_to_cpu(initial_boot_params->version) < 0x10)
-				p = _ALIGN(p, sz >= 8 ? 8 : 4);
+				p = ALIGN(p, sz >= 8 ? 8 : 4);
 			p += sz;
-			p = _ALIGN(p, 4);
+			p = ALIGN(p, 4);
 			continue;
 		}
 		if (tag != OF_DT_BEGIN_NODE) {

@@ -80,7 +80,7 @@ int __init of_scan_flat_dt(int (*it)(unsigned long node,
 		}
 		depth++;
 		pathp = (char *)p;
-		p = _ALIGN(p + strlen(pathp) + 1, 4);
+		p = ALIGN(p + strlen(pathp) + 1, 4);
 		if ((*pathp) == '/') {
 			char *lp, *np;
 			for (lp = NULL, np = pathp; *np; np++)

@@ -109,7 +109,7 @@ unsigned long __init of_get_flat_dt_root(void)
 		p += 4;
 	BUG_ON(be32_to_cpup((__be32 *)p) != OF_DT_BEGIN_NODE);
 	p += 4;
-	return _ALIGN(p + strlen((char *)p) + 1, 4);
+	return ALIGN(p + strlen((char *)p) + 1, 4);
 }
 
 /**

@@ -138,7 +138,7 @@ void *__init of_get_flat_dt_prop(unsigned long node, const char *name,
 		noff = be32_to_cpup((__be32 *)(p + 4));
 		p += 8;
 		if (be32_to_cpu(initial_boot_params->version) < 0x10)
-			p = _ALIGN(p, sz >= 8 ? 8 : 4);
+			p = ALIGN(p, sz >= 8 ? 8 : 4);
 
 		nstr = find_flat_dt_string(noff);
 		if (nstr == NULL) {

@@ -151,7 +151,7 @@ void *__init of_get_flat_dt_prop(unsigned long node, const char *name,
 			return (void *)p;
 		}
 		p += sz;
-		p = _ALIGN(p, 4);
+		p = ALIGN(p, 4);
 	} while (1);
 }
 

@@ -184,7 +184,7 @@ static void *__init unflatten_dt_alloc(unsigned long *mem, unsigned long size,
 {
 	void *res;
 
-	*mem = _ALIGN(*mem, align);
+	*mem = ALIGN(*mem, align);
 	res = (void *)*mem;
 	*mem += size;
 

@@ -220,7 +220,7 @@ unsigned long __init unflatten_dt_node(unsigned long mem,
 	*p += 4;
 	pathp = (char *)*p;
 	l = allocl = strlen(pathp) + 1;
-	*p = _ALIGN(*p + l, 4);
+	*p = ALIGN(*p + l, 4);
 
 	/* version 0x10 has a more compact unit name here instead of the full
 	 * path. we accumulate the full path size using "fpsize", we'll rebuild

@@ -299,7 +299,7 @@ unsigned long __init unflatten_dt_node(unsigned long mem,
 		noff = be32_to_cpup((__be32 *)((*p) + 4));
 		*p += 8;
 		if (be32_to_cpu(initial_boot_params->version) < 0x10)
-			*p = _ALIGN(*p, sz >= 8 ? 8 : 4);
+			*p = ALIGN(*p, sz >= 8 ? 8 : 4);
 
 		pname = find_flat_dt_string(noff);
 		if (pname == NULL) {

@@ -333,7 +333,7 @@ unsigned long __init unflatten_dt_node(unsigned long mem,
 			*prev_pp = pp;
 			prev_pp = &pp->next;
 		}
-		*p = _ALIGN((*p) + sz, 4);
+		*p = ALIGN((*p) + sz, 4);
 	}
 	/* with version 0x10 we may not have the name property, recreate
 	 * it here from the unit name if absent