[PATCH 2/2] powerpc/mm: Fix memory_block_size_bytes() for non-pseries

[PATCH 2/2] powerpc/mm: Fix memory_block_size_bytes() for non-pseries

[Benjamin Herrenschmidt]

Just compiling pseries in the kernel causes it to override
memory_block_size_bytes() regardless of what is the runtime
platform.

This cleans up the implementation of that function, fixing
a bug or two while at it, so that it's harmless (and potentially
useful) for other platforms. Without this, bugs in that code
would trigger a WARN_ON() in drivers/base/memory.c when
booting some different platforms. 

If/when we have another platform supporting memory hotplug we
might want to either move that out to a generic place or
make it a ppc_md. callback.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---

diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
index 33867ec..9d6a8ef 100644
--- a/arch/powerpc/platforms/pseries/hotplug-memory.c
+++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
@@ -12,6 +12,8 @@
 #include <linux/of.h>
 #include <linux/memblock.h>
 #include <linux/vmalloc.h>
+#include <linux/memory.h>
+
 #include <asm/firmware.h>
 #include <asm/machdep.h>
 #include <asm/pSeries_reconfig.h>
@@ -20,24 +22,25 @@
 static unsigned long get_memblock_size(void)
 {
 	struct device_node *np;
-	unsigned int memblock_size = 0;
+	unsigned int memblock_size = MIN_MEMORY_BLOCK_SIZE;
+	struct resource r;
 
 	np = of_find_node_by_path("/ibm,dynamic-reconfiguration-memory");
 	if (np) {
-		const unsigned long *size;
+		const __be64 *size;
 
 		size = of_get_property(np, "ibm,lmb-size", NULL);
-		memblock_size = size ? *size : 0;
-
+		if (size)
+			memblock_size = be64_to_cpup(size);
 		of_node_put(np);
-	} else {
+	} else  if (machine_is(pseries)) {
+		/* This fallback really only applies to pseries */
 		unsigned int memzero_size = 0;
-		const unsigned int *regs;
 
 		np = of_find_node_by_path("/memory@0");
 		if (np) {
-			regs = of_get_property(np, "reg", NULL);
-			memzero_size = regs ? regs[3] : 0;
+			if (!of_address_to_resource(np, 0, &r))
+				memzero_size = resource_size(&r);
 			of_node_put(np);
 		}
 
@@ -50,16 +53,21 @@ static unsigned long get_memblock_size(void)
 			sprintf(buf, "/memory@%x", memzero_size);
 			np = of_find_node_by_path(buf);
 			if (np) {
-				regs = of_get_property(np, "reg", NULL);
-				memblock_size = regs ? regs[3] : 0;
+				if (!of_address_to_resource(np, 0, &r))
+					memblock_size = resource_size(&r);
 				of_node_put(np);
 			}
 		}
 	}
-
 	return memblock_size;
 }
 
+/* WARNING: This is going to override the generic definition whenever
+ * pseries is built-in regardless of what platform is active at boot
+ * time. This is fine for now as this is the only "option" and it
+ * should work everywhere. If not, we'll have to turn this into a
+ * ppc_md. callback
+ */
 unsigned long memory_block_size_bytes(void)
 {
 	return get_memblock_size();

Re: [PATCH 2/2] powerpc/mm: Fix memory_block_size_bytes() for non-pseries

[Ingo Molnar]

* Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> Just compiling pseries in the kernel causes it to override
> memory_block_size_bytes() regardless of what is the runtime
> platform.
> 
> This cleans up the implementation of that function, fixing
> a bug or two while at it, so that it's harmless (and potentially
> useful) for other platforms. Without this, bugs in that code
> would trigger a WARN_ON() in drivers/base/memory.c when
> booting some different platforms. 
> 
> If/when we have another platform supporting memory hotplug we
> might want to either move that out to a generic place or
> make it a ppc_md. callback.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
> 
> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
> index 33867ec..9d6a8ef 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
> @@ -12,6 +12,8 @@
>  #include <linux/of.h>
>  #include <linux/memblock.h>
>  #include <linux/vmalloc.h>
> +#include <linux/memory.h>
> +
>  #include <asm/firmware.h>
>  #include <asm/machdep.h>
>  #include <asm/pSeries_reconfig.h>
> @@ -20,24 +22,25 @@
>  static unsigned long get_memblock_size(void)
>  {
>  	struct device_node *np;
> -	unsigned int memblock_size = 0;
> +	unsigned int memblock_size = MIN_MEMORY_BLOCK_SIZE;
> +	struct resource r;
>  
>  	np = of_find_node_by_path("/ibm,dynamic-reconfiguration-memory");
>  	if (np) {
> -		const unsigned long *size;
> +		const __be64 *size;
>  
>  		size = of_get_property(np, "ibm,lmb-size", NULL);
> -		memblock_size = size ? *size : 0;
> -
> +		if (size)
> +			memblock_size = be64_to_cpup(size);
>  		of_node_put(np);
> -	} else {
> +	} else  if (machine_is(pseries)) {
> +		/* This fallback really only applies to pseries */
>  		unsigned int memzero_size = 0;
> -		const unsigned int *regs;
>  
>  		np = of_find_node_by_path("/memory@0");
>  		if (np) {
> -			regs = of_get_property(np, "reg", NULL);
> -			memzero_size = regs ? regs[3] : 0;
> +			if (!of_address_to_resource(np, 0, &r))
> +				memzero_size = resource_size(&r);
>  			of_node_put(np);
>  		}
>  
> @@ -50,16 +53,21 @@ static unsigned long get_memblock_size(void)
>  			sprintf(buf, "/memory@%x", memzero_size);
>  			np = of_find_node_by_path(buf);
>  			if (np) {
> -				regs = of_get_property(np, "reg", NULL);
> -				memblock_size = regs ? regs[3] : 0;
> +				if (!of_address_to_resource(np, 0, &r))
> +					memblock_size = resource_size(&r);
>  				of_node_put(np);
>  			}
>  		}
>  	}
> -
>  	return memblock_size;
>  }
>  
> +/* WARNING: This is going to override the generic definition whenever
> + * pseries is built-in regardless of what platform is active at boot
> + * time. This is fine for now as this is the only "option" and it
> + * should work everywhere. If not, we'll have to turn this into a
> + * ppc_md. callback
> + */

Just a small nit, please use the customary (multi-line) comment 
style:

  /*
   * Comment .....
   * ...... goes here.
   */

specified in Documentation/CodingStyle.

Thanks,

        Ingo

Re: [PATCH 2/2] powerpc/mm: Fix memory_block_size_bytes() for non-pseries

[Benjamin Herrenschmidt]

On Fri, 2011-07-01 at 14:15 +0200, Ingo Molnar wrote:

> > +/* WARNING: This is going to override the generic definition whenever
> > + * pseries is built-in regardless of what platform is active at boot
> > + * time. This is fine for now as this is the only "option" and it
> > + * should work everywhere. If not, we'll have to turn this into a
> > + * ppc_md. callback
> > + */
> 
> Just a small nit, please use the customary (multi-line) comment 
> style:
> 
>   /*
>    * Comment .....
>    * ...... goes here.
>    */
> 
> specified in Documentation/CodingStyle.

Ah ! Here goes my sneak attempts at violating coding style while nobody
notices :-)

No seriously, that sort of stuff shouldn't be such a hard rule... In
some cases the "official" way looks nicer, on some cases it's just a
waste of space, and I've grown to prefer my slightly more compact form,
at least depending on how the surrounding code looks like.

Since that's all powerpc arch code, I believe I'm entitled to that
little bit of flexibility in how the code looks like :-) It's not
like I'm GoingToPlayWithCaps() or switching to 3-char tabs :-)

Cheers,
Ben.

Re: [PATCH 2/2] powerpc/mm: Fix memory_block_size_bytes() for non-pseries

[Ingo Molnar]

* Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> On Fri, 2011-07-01 at 14:15 +0200, Ingo Molnar wrote:
> 
> > > +/* WARNING: This is going to override the generic definition whenever
> > > + * pseries is built-in regardless of what platform is active at boot
> > > + * time. This is fine for now as this is the only "option" and it
> > > + * should work everywhere. If not, we'll have to turn this into a
> > > + * ppc_md. callback
> > > + */
> > 
> > Just a small nit, please use the customary (multi-line) comment 
> > style:
> > 
> >   /*
> >    * Comment .....
> >    * ...... goes here.
> >    */
> > 
> > specified in Documentation/CodingStyle.
> 
> Ah ! Here goes my sneak attempts at violating coding style while 
> nobody notices :-)
> 
> No seriously, that sort of stuff shouldn't be such a hard rule... 
> In some cases the "official" way looks nicer, on some cases it's 
> just a waste of space, and I've grown to prefer my slightly more 
> compact form, at least depending on how the surrounding code looks 
> like.
>
> Since that's all powerpc arch code, I believe I'm entitled to that 
> little bit of flexibility in how the code looks like :-) It's not 
> like I'm GoingToPlayWithCaps() or switching to 3-char tabs :-)

It's certainly not a hard rule - but note that the file in question 
(arch/powerpc/platforms/pseries/hotplug-memory.c) has a rather 
inconsistent comment style, sometimes even within the same function:

        /*
         * Remove htab bolted mappings for this section of memory
         */
...

        /* Ensure all vmalloc mappings are flushed in case they also
         * hit that section of memory
         */

That kind of inconsistency within the same .c file and within the 
same function is not defensible with a "style is a matter of taste" 
argument.

As i said, it's just a small nit.

Thanks,

	Ingo

Re: [PATCH 2/2] powerpc/mm: Fix memory_block_size_bytes() for non-pseries

[Benjamin Herrenschmidt]

On Sat, 2011-07-02 at 12:23 +0200, Ingo Molnar wrote:
> It's certainly not a hard rule - but note that the file in question 
> (arch/powerpc/platforms/pseries/hotplug-memory.c) has a rather 
> inconsistent comment style, sometimes even within the same function:
> 
>         /*
>          * Remove htab bolted mappings for this section of memory
>          */
> ...
> 
>         /* Ensure all vmalloc mappings are flushed in case they also
>          * hit that section of memory
>          */
> 
> That kind of inconsistency within the same .c file and within the 
> same function is not defensible with a "style is a matter of taste" 
> argument.

Right, that's a matter of different people with different taste mucking
around with the same file I suppose.

Most of this probably predates my involvement as a maintainer but even
if not (and I really can't be bothered digging into the history), it
might very well be something I didn't pay attention to while reviewing.

Seriously, it's so low on the scale of what matters ... I'm sure we both
have more valuable stuff to spend our time and energy on :-)

Cheers,
Ben.