[PATCH] powerpc: force 4k update_flash block and list sizes

[PATCH] powerpc: force 4k update_flash block and list sizes

[John Rose]

The enablement of 64k pages on POWER platforms exposed a quirk in the RTAS
mechanism for updating firmware.  RTAS assumes 4k for flash block and list
sizes, and use of any other sizes results in a failure.

This patch changes the rtas_flash module to force the use of 4k memory block
and list sizes when preparing and sending a firmware image to RTAS.  It also
changes rtas_flash to use a slab cache to ensure 4k alignment of flash block
data.  Such alignment is a documented requirement.

Signed-off-by: John Rose <johnrose@austin.ibm.com>

___

Intended for inclusion for 2.6.20, if appropriate.  Thanks!

John

---

 uf_4k-johnrose/arch/powerpc/kernel/rtas_flash.c |   52 +++++++++++++++++++-----
 1 files changed, 42 insertions(+), 10 deletions(-)

diff -puN arch/powerpc/kernel/rtas_flash.c~uf_4k_alt2 arch/powerpc/kernel/rtas_flash.c
--- uf_4k/arch/powerpc/kernel/rtas_flash.c~uf_4k_alt2	2006-11-06 16:59:27.000000000 -0600
+++ uf_4k-johnrose/arch/powerpc/kernel/rtas_flash.c	2006-11-06 17:02:31.000000000 -0600
@@ -72,6 +72,10 @@
 #define VALIDATE_BUF_SIZE 4096    
 #define RTAS_MSG_MAXLEN   64
 
+/* Quirk - RTAS requires 4k list length and block size */
+#define RTAS_BLKLIST_LENGTH 4096
+#define RTAS_BLK_SIZE 4096
+
 struct flash_block {
 	char *data;
 	unsigned long length;
@@ -83,7 +87,7 @@ struct flash_block {
  * into a version/length and translate the pointers
  * to absolute.
  */
-#define FLASH_BLOCKS_PER_NODE ((PAGE_SIZE - 16) / sizeof(struct flash_block))
+#define FLASH_BLOCKS_PER_NODE ((RTAS_BLKLIST_LENGTH - 16) / sizeof(struct flash_block))
 struct flash_block_list {
 	unsigned long num_blocks;
 	struct flash_block_list *next;
@@ -96,6 +100,9 @@ struct flash_block_list_header { /* just
 
 static struct flash_block_list_header rtas_firmware_flash_list = {0, NULL};
 
+/* Use slab cache to guarantee 4k alignment */
+static kmem_cache_t *flash_block_cache = NULL;
+
 #define FLASH_BLOCK_LIST_VERSION (1UL)
 
 /* Local copy of the flash block list.
@@ -153,7 +160,7 @@ static int flash_list_valid(struct flash
 				return FLASH_IMG_NULL_DATA;
 			}
 			block_size = f->blocks[i].length;
-			if (block_size <= 0 || block_size > PAGE_SIZE) {
+			if (block_size <= 0 || block_size > RTAS_BLK_SIZE) {
 				return FLASH_IMG_BAD_LEN;
 			}
 			image_size += block_size;
@@ -177,9 +184,9 @@ static void free_flash_list(struct flash
 
 	while (f) {
 		for (i = 0; i < f->num_blocks; i++)
-			free_page((unsigned long)(f->blocks[i].data));
+			kmem_cache_free(flash_block_cache, f->blocks[i].data);
 		next = f->next;
-		free_page((unsigned long)f);
+		kmem_cache_free(flash_block_cache, f);
 		f = next;
 	}
 }
@@ -278,6 +285,12 @@ static ssize_t rtas_flash_read(struct fi
 	return msglen;
 }
 
+/* constructor for flash_block_cache */
+void rtas_block_ctor(void *ptr, kmem_cache_t *cache, unsigned long flags)
+{
+	memset(ptr, 0, RTAS_BLK_SIZE);
+}
+
 /* We could be much more efficient here.  But to keep this function
  * simple we allocate a page to the block list no matter how small the
  * count is.  If the system is low on memory it will be just as well
@@ -302,7 +315,7 @@ static ssize_t rtas_flash_write(struct f
 	 * proc file
 	 */
 	if (uf->flist == NULL) {
-		uf->flist = (struct flash_block_list *) get_zeroed_page(GFP_KERNEL);
+		uf->flist = kmem_cache_alloc(flash_block_cache, GFP_KERNEL);
 		if (!uf->flist)
 			return -ENOMEM;
 	}
@@ -313,21 +326,21 @@ static ssize_t rtas_flash_write(struct f
 	next_free = fl->num_blocks;
 	if (next_free == FLASH_BLOCKS_PER_NODE) {
 		/* Need to allocate another block_list */
-		fl->next = (struct flash_block_list *)get_zeroed_page(GFP_KERNEL);
+		fl->next = kmem_cache_alloc(flash_block_cache, GFP_KERNEL);
 		if (!fl->next)
 			return -ENOMEM;
 		fl = fl->next;
 		next_free = 0;
 	}
 
-	if (count > PAGE_SIZE)
-		count = PAGE_SIZE;
-	p = (char *)get_zeroed_page(GFP_KERNEL);
+	if (count > RTAS_BLK_SIZE)
+		count = RTAS_BLK_SIZE;
+	p = kmem_cache_alloc(flash_block_cache, GFP_KERNEL);
 	if (!p)
 		return -ENOMEM;
 	
 	if(copy_from_user(p, buffer, count)) {
-		free_page((unsigned long)p);
+		kmem_cache_free(flash_block_cache, p);
 		return -EFAULT;
 	}
 	fl->blocks[next_free].data = p;
@@ -791,6 +804,16 @@ int __init rtas_flash_init(void)
 		goto cleanup;
 
 	rtas_flash_term_hook = rtas_flash_firmware;
+
+	flash_block_cache = kmem_cache_create("rtas_flash_cache",
+				RTAS_BLK_SIZE, RTAS_BLK_SIZE, 0,
+				rtas_block_ctor, NULL);
+	if (!flash_block_cache) {
+		printk(KERN_ERR "%s: failed to create block cache\n",
+				__FUNCTION__);
+		rc = -ENOMEM;
+		goto cleanup;
+	}
 	return 0;
 
 cleanup:
@@ -804,7 +827,16 @@ cleanup:
 
 void __exit rtas_flash_cleanup(void)
 {
+	int rc;
+
 	rtas_flash_term_hook = NULL;
+
+	if (flash_block_cache) {
+		rc = kmem_cache_destroy(flash_block_cache);
+		if (rc)
+			printk(KERN_WARNING "%s: cache memory leak\n",
+					__FUNCTION__);
+	}
 	remove_flash_pde(firmware_flash_pde);
 	remove_flash_pde(firmware_update_pde);
 	remove_flash_pde(validate_pde);

_

Re: [PATCH] powerpc: force 4k update_flash block and list sizes

[Michael Neuling]

> +
> +	if (flash_block_cache) {
> +		rc = kmem_cache_destroy(flash_block_cache);
> +		if (rc)
> +			printk(KERN_WARNING "%s: cache memory leak\n",
> +					__FUNCTION__);
> +	}

kmem_cache_destroy doesn't return anything.  Hence we get:

arch/powerpc/kernel/rtas_flash.c: In function 'rtas_flash_cleanup':
arch/powerpc/kernel/rtas_flash.c:835: error: void value not ignored as it ought to be

Mikey

Re: [PATCH] powerpc: force 4k update_flash block and list sizes

[John Rose]

> kmem_cache_destroy doesn't return anything.

Good call!  How's this:

The enablement of 64k pages on POWER platforms exposed a quirk in the RTAS
mechanism for updating firmware.  RTAS assumes 4k for flash block and list
sizes, and use of any other sizes results in a failure.

This patch changes the rtas_flash module to force the use of 4k memory block
and list sizes when preparing and sending a firmware image to RTAS.  It also
changes rtas_flash to use a slab cache to ensure 4k alignment of flash block
data.  Such alignment is a documented requirement.

Signed-off-by: John Rose <johnrose@austin.ibm.com>

---

---

 uf_4k-johnrose/arch/powerpc/kernel/rtas_flash.c |   52 +++++++++++++++++++-----
 1 files changed, 42 insertions(+), 10 deletions(-)

diff -puN arch/powerpc/kernel/rtas_flash.c~uf_4k_alt2 arch/powerpc/kernel/rtas_flash.c
--- uf_4k/arch/powerpc/kernel/rtas_flash.c~uf_4k_alt2	2006-11-06 16:59:27.000000000 -0600
+++ uf_4k-johnrose/arch/powerpc/kernel/rtas_flash.c	2006-11-06 17:02:31.000000000 -0600
@@ -72,6 +72,10 @@
 #define VALIDATE_BUF_SIZE 4096    
 #define RTAS_MSG_MAXLEN   64
 
+/* Quirk - RTAS requires 4k list length and block size */
+#define RTAS_BLKLIST_LENGTH 4096
+#define RTAS_BLK_SIZE 4096
+
 struct flash_block {
 	char *data;
 	unsigned long length;
@@ -83,7 +87,7 @@ struct flash_block {
  * into a version/length and translate the pointers
  * to absolute.
  */
-#define FLASH_BLOCKS_PER_NODE ((PAGE_SIZE - 16) / sizeof(struct flash_block))
+#define FLASH_BLOCKS_PER_NODE ((RTAS_BLKLIST_LENGTH - 16) / sizeof(struct flash_block))
 struct flash_block_list {
 	unsigned long num_blocks;
 	struct flash_block_list *next;
@@ -96,6 +100,9 @@ struct flash_block_list_header { /* just
 
 static struct flash_block_list_header rtas_firmware_flash_list = {0, NULL};
 
+/* Use slab cache to guarantee 4k alignment */
+static kmem_cache_t *flash_block_cache = NULL;
+
 #define FLASH_BLOCK_LIST_VERSION (1UL)
 
 /* Local copy of the flash block list.
@@ -153,7 +160,7 @@ static int flash_list_valid(struct flash
 				return FLASH_IMG_NULL_DATA;
 			}
 			block_size = f->blocks[i].length;
-			if (block_size <= 0 || block_size > PAGE_SIZE) {
+			if (block_size <= 0 || block_size > RTAS_BLK_SIZE) {
 				return FLASH_IMG_BAD_LEN;
 			}
 			image_size += block_size;
@@ -177,9 +184,9 @@ static void free_flash_list(struct flash
 
 	while (f) {
 		for (i = 0; i < f->num_blocks; i++)
-			free_page((unsigned long)(f->blocks[i].data));
+			kmem_cache_free(flash_block_cache, f->blocks[i].data);
 		next = f->next;
-		free_page((unsigned long)f);
+		kmem_cache_free(flash_block_cache, f);
 		f = next;
 	}
 }
@@ -278,6 +285,12 @@ static ssize_t rtas_flash_read(struct fi
 	return msglen;
 }
 
+/* constructor for flash_block_cache */
+void rtas_block_ctor(void *ptr, kmem_cache_t *cache, unsigned long flags)
+{
+	memset(ptr, 0, RTAS_BLK_SIZE);
+}
+
 /* We could be much more efficient here.  But to keep this function
  * simple we allocate a page to the block list no matter how small the
  * count is.  If the system is low on memory it will be just as well
@@ -302,7 +315,7 @@ static ssize_t rtas_flash_write(struct f
 	 * proc file
 	 */
 	if (uf->flist == NULL) {
-		uf->flist = (struct flash_block_list *) get_zeroed_page(GFP_KERNEL);
+		uf->flist = kmem_cache_alloc(flash_block_cache, GFP_KERNEL);
 		if (!uf->flist)
 			return -ENOMEM;
 	}
@@ -313,21 +326,21 @@ static ssize_t rtas_flash_write(struct f
 	next_free = fl->num_blocks;
 	if (next_free == FLASH_BLOCKS_PER_NODE) {
 		/* Need to allocate another block_list */
-		fl->next = (struct flash_block_list *)get_zeroed_page(GFP_KERNEL);
+		fl->next = kmem_cache_alloc(flash_block_cache, GFP_KERNEL);
 		if (!fl->next)
 			return -ENOMEM;
 		fl = fl->next;
 		next_free = 0;
 	}
 
-	if (count > PAGE_SIZE)
-		count = PAGE_SIZE;
-	p = (char *)get_zeroed_page(GFP_KERNEL);
+	if (count > RTAS_BLK_SIZE)
+		count = RTAS_BLK_SIZE;
+	p = kmem_cache_alloc(flash_block_cache, GFP_KERNEL);
 	if (!p)
 		return -ENOMEM;
 	
 	if(copy_from_user(p, buffer, count)) {
-		free_page((unsigned long)p);
+		kmem_cache_free(flash_block_cache, p);
 		return -EFAULT;
 	}
 	fl->blocks[next_free].data = p;
@@ -791,6 +804,16 @@ int __init rtas_flash_init(void)
 		goto cleanup;
 
 	rtas_flash_term_hook = rtas_flash_firmware;
+
+	flash_block_cache = kmem_cache_create("rtas_flash_cache",
+				RTAS_BLK_SIZE, RTAS_BLK_SIZE, 0,
+				rtas_block_ctor, NULL);
+	if (!flash_block_cache) {
+		printk(KERN_ERR "%s: failed to create block cache\n",
+				__FUNCTION__);
+		rc = -ENOMEM;
+		goto cleanup;
+	}
 	return 0;
 
 cleanup:
@@ -804,7 +827,16 @@ cleanup:
 
 void __exit rtas_flash_cleanup(void)
 {
+	int rc;
+
 	rtas_flash_term_hook = NULL;
+
+	if (flash_block_cache) {
+		rc = kmem_cache_destroy(flash_block_cache);
+		if (rc)
+			printk(KERN_WARNING "%s: cache memory leak\n",
+					__FUNCTION__);
+	}
 	remove_flash_pde(firmware_flash_pde);
 	remove_flash_pde(firmware_update_pde);
 	remove_flash_pde(validate_pde);

_

Re: [PATCH] powerpc: force 4k update_flash block and list sizes

[Michael Neuling]

In message <1162960202.14254.3.camel@sinatra.austin.ibm.com> you wrote:
> > kmem_cache_destroy doesn't return anything.
> 
> Good call!  How's this:
<snip>
> +	if (flash_block_cache) {
> +		rc = kmem_cache_destroy(flash_block_cache);
> +		if (rc)
> +			printk(KERN_WARNING "%s: cache memory leak\n",
> +					__FUNCTION__);
> +	}

Arrh, did anything change?

Mikey

Re: [PATCH] powerpc: force 4k update_flash block and list sizes

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 1165 bytes --]

On Tue, 2006-11-07 at 22:30 -0600, John Rose wrote:
> > kmem_cache_destroy doesn't return anything.
> 
> Good call!  How's this:
> 
> The enablement of 64k pages on POWER platforms exposed a quirk in the RTAS
> mechanism for updating firmware.  RTAS assumes 4k for flash block and list
> sizes, and use of any other sizes results in a failure.
> 
> This patch changes the rtas_flash module to force the use of 4k memory block
> and list sizes when preparing and sending a firmware image to RTAS.  It also
> changes rtas_flash to use a slab cache to ensure 4k alignment of flash block
> data.  Such alignment is a documented requirement.

Just being picky .. but why did you decide to use the slab cache? Does
it make the code neater? I would have thought you could guarantee 4k
alignment some other way, and it seems slightly odd to use the slab
cache for something you only do once ..

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] powerpc: force 4k update_flash block and list sizes

[Arnd Bergmann]

On Wednesday 08 November 2006 05:41, Michael Ellerman wrote:
> Just being picky .. but why did you decide to use the slab cache? Does
> it make the code neater? I would have thought you could guarantee 4k
> alignment some other way, and it seems slightly odd to use the slab
> cache for something you only do once ..

There are not so many allocators available. kmalloc does not guarantee
alignment beyond a few bytes and the buddy allocator and vmalloc don't
give you allocations smaller than PAGE_SIZE.

The ehca people had the same problem, and I could imagine there are
others with similar issues. How about adding a special slab allocator
architecture-wide that gives out aligned 4k chunks independent of
page size?

	Arnd <>z

Re: [PATCH] powerpc: force 4k update_flash block and list sizes

[John Rose]

> Arrh, did anything change?

Sorry, attached the wrong thing!  That's what I get for trying to submit
code after 10PM :)

The enablement of 64k pages on POWER platforms exposed a quirk in the RTAS
mechanism for updating firmware.  RTAS assumes 4k for flash block and list
sizes, and use of any other sizes results in a failure.

This patch changes the rtas_flash module to force the use of 4k memory block
and list sizes when preparing and sending a firmware image to RTAS.  It also
changes rtas_flash to use a slab cache to ensure 4k alignment of flash block
data.  Such alignment is a documented requirement.

Signed-off-by: John Rose <johnrose@austin.ibm.com>

---

 uf_4k-johnrose/arch/powerpc/kernel/rtas_flash.c |   47 ++++++++++++++++++------
 1 files changed, 37 insertions(+), 10 deletions(-)

diff -puN arch/powerpc/kernel/rtas_flash.c~uf_4k_alt2 arch/powerpc/kernel/rtas_flash.c
--- uf_4k/arch/powerpc/kernel/rtas_flash.c~uf_4k_alt2	2006-11-06 16:59:27.000000000 -0600
+++ uf_4k-johnrose/arch/powerpc/kernel/rtas_flash.c	2006-11-07 22:29:52.000000000 -0600
@@ -72,6 +72,10 @@
 #define VALIDATE_BUF_SIZE 4096    
 #define RTAS_MSG_MAXLEN   64
 
+/* Quirk - RTAS requires 4k list length and block size */
+#define RTAS_BLKLIST_LENGTH 4096
+#define RTAS_BLK_SIZE 4096
+
 struct flash_block {
 	char *data;
 	unsigned long length;
@@ -83,7 +87,7 @@ struct flash_block {
  * into a version/length and translate the pointers
  * to absolute.
  */
-#define FLASH_BLOCKS_PER_NODE ((PAGE_SIZE - 16) / sizeof(struct flash_block))
+#define FLASH_BLOCKS_PER_NODE ((RTAS_BLKLIST_LENGTH - 16) / sizeof(struct flash_block))
 struct flash_block_list {
 	unsigned long num_blocks;
 	struct flash_block_list *next;
@@ -96,6 +100,9 @@ struct flash_block_list_header { /* just
 
 static struct flash_block_list_header rtas_firmware_flash_list = {0, NULL};
 
+/* Use slab cache to guarantee 4k alignment */
+static kmem_cache_t *flash_block_cache = NULL;
+
 #define FLASH_BLOCK_LIST_VERSION (1UL)
 
 /* Local copy of the flash block list.
@@ -153,7 +160,7 @@ static int flash_list_valid(struct flash
 				return FLASH_IMG_NULL_DATA;
 			}
 			block_size = f->blocks[i].length;
-			if (block_size <= 0 || block_size > PAGE_SIZE) {
+			if (block_size <= 0 || block_size > RTAS_BLK_SIZE) {
 				return FLASH_IMG_BAD_LEN;
 			}
 			image_size += block_size;
@@ -177,9 +184,9 @@ static void free_flash_list(struct flash
 
 	while (f) {
 		for (i = 0; i < f->num_blocks; i++)
-			free_page((unsigned long)(f->blocks[i].data));
+			kmem_cache_free(flash_block_cache, f->blocks[i].data);
 		next = f->next;
-		free_page((unsigned long)f);
+		kmem_cache_free(flash_block_cache, f);
 		f = next;
 	}
 }
@@ -278,6 +285,12 @@ static ssize_t rtas_flash_read(struct fi
 	return msglen;
 }
 
+/* constructor for flash_block_cache */
+void rtas_block_ctor(void *ptr, kmem_cache_t *cache, unsigned long flags)
+{
+	memset(ptr, 0, RTAS_BLK_SIZE);
+}
+
 /* We could be much more efficient here.  But to keep this function
  * simple we allocate a page to the block list no matter how small the
  * count is.  If the system is low on memory it will be just as well
@@ -302,7 +315,7 @@ static ssize_t rtas_flash_write(struct f
 	 * proc file
 	 */
 	if (uf->flist == NULL) {
-		uf->flist = (struct flash_block_list *) get_zeroed_page(GFP_KERNEL);
+		uf->flist = kmem_cache_alloc(flash_block_cache, GFP_KERNEL);
 		if (!uf->flist)
 			return -ENOMEM;
 	}
@@ -313,21 +326,21 @@ static ssize_t rtas_flash_write(struct f
 	next_free = fl->num_blocks;
 	if (next_free == FLASH_BLOCKS_PER_NODE) {
 		/* Need to allocate another block_list */
-		fl->next = (struct flash_block_list *)get_zeroed_page(GFP_KERNEL);
+		fl->next = kmem_cache_alloc(flash_block_cache, GFP_KERNEL);
 		if (!fl->next)
 			return -ENOMEM;
 		fl = fl->next;
 		next_free = 0;
 	}
 
-	if (count > PAGE_SIZE)
-		count = PAGE_SIZE;
-	p = (char *)get_zeroed_page(GFP_KERNEL);
+	if (count > RTAS_BLK_SIZE)
+		count = RTAS_BLK_SIZE;
+	p = kmem_cache_alloc(flash_block_cache, GFP_KERNEL);
 	if (!p)
 		return -ENOMEM;
 	
 	if(copy_from_user(p, buffer, count)) {
-		free_page((unsigned long)p);
+		kmem_cache_free(flash_block_cache, p);
 		return -EFAULT;
 	}
 	fl->blocks[next_free].data = p;
@@ -791,6 +804,16 @@ int __init rtas_flash_init(void)
 		goto cleanup;
 
 	rtas_flash_term_hook = rtas_flash_firmware;
+
+	flash_block_cache = kmem_cache_create("rtas_flash_cache",
+				RTAS_BLK_SIZE, RTAS_BLK_SIZE, 0,
+				rtas_block_ctor, NULL);
+	if (!flash_block_cache) {
+		printk(KERN_ERR "%s: failed to create block cache\n",
+				__FUNCTION__);
+		rc = -ENOMEM;
+		goto cleanup;
+	}
 	return 0;
 
 cleanup:
@@ -805,6 +828,10 @@ cleanup:
 void __exit rtas_flash_cleanup(void)
 {
 	rtas_flash_term_hook = NULL;
+
+	if (flash_block_cache)
+		kmem_cache_destroy(flash_block_cache);
+
 	remove_flash_pde(firmware_flash_pde);
 	remove_flash_pde(firmware_update_pde);
 	remove_flash_pde(validate_pde);

_

Re: [PATCH] powerpc: force 4k update_flash block and list sizes

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 1376 bytes --]

On Wed, 2006-11-08 at 10:28 +0100, Arnd Bergmann wrote:
> On Wednesday 08 November 2006 05:41, Michael Ellerman wrote:
> > Just being picky .. but why did you decide to use the slab cache? Does
> > it make the code neater? I would have thought you could guarantee 4k
> > alignment some other way, and it seems slightly odd to use the slab
> > cache for something you only do once ..
> 
> There are not so many allocators available. kmalloc does not guarantee
> alignment beyond a few bytes and the buddy allocator and vmalloc don't
> give you allocations smaller than PAGE_SIZE.

Yeah, I assumed kmalloc aligned on the size of the allocation, but it
doesn't looking at the code. We could set ARCH_KMALLOC_MINALIGN to 4k,
but that aligns all caches which is probably not what we want.

> The ehca people had the same problem, and I could imagine there are
> others with similar issues. How about adding a special slab allocator
> architecture-wide that gives out aligned 4k chunks independent of
> page size?

Yeah that sounds like a plan, I'm sure there's other code around that
could use it.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]