Patchwork [02/11] powerpc/nvram: More flexible nvram_create_partition()

login
register
mail settings
Submitter Benjamin Herrenschmidt
Date Aug. 2, 2010, 12:55 a.m.
Message ID <1280710522-6362-3-git-send-email-benh@kernel.crashing.org>
Download mbox | patch
Permalink /patch/60496/
State Rejected
Headers show

Comments

Benjamin Herrenschmidt - Aug. 2, 2010, 12:55 a.m.
Replace nvram_create_os_partition() with a variant that takes
the partition name, signature and size as arguments.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 arch/powerpc/kernel/nvram_64.c         |   46 +++++++++++++++++++------------
 arch/powerpc/platforms/pseries/nvram.c |    6 ++--
 2 files changed, 31 insertions(+), 21 deletions(-)
Michael Ellerman - Aug. 2, 2010, 3:43 a.m.
On Mon, 2010-08-02 at 10:55 +1000, Benjamin Herrenschmidt wrote:
> Replace nvram_create_os_partition() with a variant that takes
> the partition name, signature and size as arguments.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
>  arch/powerpc/kernel/nvram_64.c         |   46 +++++++++++++++++++------------
>  arch/powerpc/platforms/pseries/nvram.c |    6 ++--
>  2 files changed, 31 insertions(+), 21 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/nvram_64.c b/arch/powerpc/kernel/nvram_64.c
> index a8154f1..974b3ec 100644
> --- a/arch/powerpc/kernel/nvram_64.c
> +++ b/arch/powerpc/kernel/nvram_64.c
> @@ -307,13 +307,15 @@ static int __init nvram_remove_os_partition(void)
>  	return 0;
>  }
>  
> -/* nvram_create_os_partition
> - *
> - * Create a OS linux partition to buffer error logs.
> - * Will create a partition starting at the first free
> - * space found if space has enough room.
> +/**
> + * nvram_create_partition - Create a partition in nvram
> + * @name: name of the partition to create
> + * @sig: signature of the partition to create
> + * @req_size: size to allocate preferrably
> + * @min_size: minimum acceptable size (0 means req_size)
>   */
> -static int __init nvram_create_os_partition(void)
> +static int __init nvram_create_partition(const char *name, int sig,
> +					 int req_size, int min_size)
>  {
>  	struct nvram_partition *part;
>  	struct nvram_partition *new_part;
> @@ -322,20 +324,27 @@ static int __init nvram_create_os_partition(void)
>  	loff_t tmp_index;
>  	long size = 0;
>  	int rc;
> -	
> +
> +	/* If no minimum size specified, make it the same as the
> +	 * requested size
> +	 */
> +	if (min_size == 0)
> +		min_size = req_size;
> +
>  	/* Find a free partition that will give us the maximum needed size 
>  	   If can't find one that will give us the minimum size needed */
>  	list_for_each_entry(part, &nvram_part->partition, partition) {
>  		if (part->header.signature != NVRAM_SIG_FREE)
>  			continue;
>  
> -		if (part->header.length >= NVRAM_MAX_REQ) {
> -			size = NVRAM_MAX_REQ;
> +		if (part->header.length >= req_size) {
> +			size = req_size;
>  			free_part = part;
>  			break;
>  		}
> -		if (!size && part->header.length >= NVRAM_MIN_REQ) {
> -			size = NVRAM_MIN_REQ;
> +		if (part->header.length > size &&
> +		    part->header.length >= min_size) {
> +			size = part->header.length;
>  			free_part = part;
>  		}

I can't quite convince myself that this is right, but perhaps I just
need a coffee.

> @@ -350,9 +359,9 @@ static int __init nvram_create_os_partition(void)
>  	}
>  
>  	new_part->index = free_part->index;
> -	new_part->header.signature = NVRAM_SIG_OS;
> +	new_part->header.signature = sig;
>  	new_part->header.length = size;
> -	strcpy(new_part->header.name, "ppc64,linux");
> +	strncpy(new_part->header.name, name, 12);

Should we be checking the name fits?

>  	new_part->header.checksum = nvram_checksum(&new_part->header);
>  
>  	rc = nvram_write_header(new_part);
> @@ -451,10 +460,10 @@ static int __init nvram_setup_partition(void)
>  	}
>  	
>  	/* try creating a partition with the free space we have */
> -	rc = nvram_create_partition("ppc64,linux", );

That looks odd? Trailing comma.

> -	if (!rc) {
> +	rc = 	nvram_create_partition("ppc64,linux", NVRAM_SIG_OS,
> +				       NVRAM_MIN_REQ, NVRAM_MAX_REQ);

Odd whitespace.

> +	if (!rc)
>  		return 0;
> -	}
>  		
>  	/* need to free up some space */
>  	rc = nvram_remove_os_partition();
> @@ -463,9 +472,10 @@ static int __init nvram_setup_partition(void)
>  	}
>  	
>  	/* create a partition in this new space */
> -	rc = nvram_create_os_partition();
> +	rc = 	nvram_create_partition("ppc64,linux", NVRAM_SIG_OS,
> +				       NVRAM_MIN_REQ, NVRAM_MAX_REQ);

And again.

>  	if (rc) {
> -		printk(KERN_ERR "nvram_create_os_partition: Could not find a "
> +		printk(KERN_ERR "nvram_create_partition: Could not find a "
>  		       "NVRAM partition large enough\n");

pr_err("%s: ..", __func__, ..) if you're touching it anyway?

> diff --git a/arch/powerpc/platforms/pseries/nvram.c b/arch/powerpc/platforms/pseries/nvram.c
> index f4e4c06..2a1ef5c 100644
> --- a/arch/powerpc/platforms/pseries/nvram.c
> +++ b/arch/powerpc/platforms/pseries/nvram.c
> @@ -22,14 +22,14 @@
>  #include <asm/prom.h>
>  #include <asm/machdep.h>
>  
> +/* Max bytes to read/write in one go */
> +#define NVRW_CNT 0x20
> +
>  static unsigned int nvram_size;
>  static int nvram_fetch, nvram_store;
>  static char nvram_buf[NVRW_CNT];	/* assume this is in the first 4GB */
>  static DEFINE_SPINLOCK(nvram_lock);
>  
> -/* Max bytes to read/write in one go */
> -#define NVRW_CNT 0x20
> -

Churn alert, you just moved that hunk there in the previous patch.

cheers
Benjamin Herrenschmidt - Aug. 2, 2010, 3:54 a.m.
On Mon, 2010-08-02 at 13:43 +1000, Michael Ellerman wrote:

> >  	
> >  	/* try creating a partition with the free space we have */
> > -	rc = nvram_create_partition("ppc64,linux", );
> 
> That looks odd? Trailing comma.

Yeah, that and...

> > diff --git a/arch/powerpc/platforms/pseries/nvram.c b/arch/powerpc/platforms/pseries/nvram.c
> > index f4e4c06..2a1ef5c 100644
> > --- a/arch/powerpc/platforms/pseries/nvram.c
> > +++ b/arch/powerpc/platforms/pseries/nvram.c
> > @@ -22,14 +22,14 @@
> >  #include <asm/prom.h>
> >  #include <asm/machdep.h>
> >  
> > +/* Max bytes to read/write in one go */
> > +#define NVRW_CNT 0x20
> > +
> >  static unsigned int nvram_size;
> >  static int nvram_fetch, nvram_store;
> >  static char nvram_buf[NVRW_CNT];	/* assume this is in the first 4GB */
> >  static DEFINE_SPINLOCK(nvram_lock);
> >  
> > -/* Max bytes to read/write in one go */
> > -#define NVRW_CNT 0x20
> > -
> 
> Churn alert, you just moved that hunk there in the previous patch.

 ... that are local rebasing fu*kage.

I'll sort all that out in the next version.

Thanks,
Ben.
Benjamin Herrenschmidt - Aug. 2, 2010, 7:02 a.m.
On Mon, 2010-08-02 at 10:55 +1000, Benjamin Herrenschmidt wrote:
> Replace nvram_create_os_partition() with a variant that takes
> the partition name, signature and size as arguments.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
>  arch/powerpc/kernel/nvram_64.c         |   46 +++++++++++++++++++------------
>  arch/powerpc/platforms/pseries/nvram.c |    6 ++--
>  2 files changed, 31 insertions(+), 21 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/nvram_64.c b/arch/powerpc/kernel/nvram_64.c
> index a8154f1..974b3ec 100644
> --- a/arch/powerpc/kernel/nvram_64.c
> +++ b/arch/powerpc/kernel/nvram_64.c
> @@ -307,13 +307,15 @@ static int __init nvram_remove_os_partition(void)
>  	return 0;
>  }
>  
> -/* nvram_create_os_partition
> - *
> - * Create a OS linux partition to buffer error logs.
> - * Will create a partition starting at the first free
> - * space found if space has enough room.
> +/**
> + * nvram_create_partition - Create a partition in nvram
> + * @name: name of the partition to create
> + * @sig: signature of the partition to create
> + * @req_size: size to allocate preferrably
> + * @min_size: minimum acceptable size (0 means req_size)
>   */
> -static int __init nvram_create_os_partition(void)
> +static int __init nvram_create_partition(const char *name, int sig,
> +					 int req_size, int min_size)
>  {

Order of args is req , min but...

> +	rc = 	nvram_create_partition("ppc64,linux", NVRAM_SIG_OS,
> +				       NVRAM_MIN_REQ, NVRAM_MAX_REQ);
>  	if (rc) {

I call it the other way around :-)

I should have tested this more before sending for RFC :-) Anyways, I
found more crap, like the way the list head is allocated etc...

I'll do another pass this week after I've dealt with the merge window.

Cheers,
Ben.

Patch

diff --git a/arch/powerpc/kernel/nvram_64.c b/arch/powerpc/kernel/nvram_64.c
index a8154f1..974b3ec 100644
--- a/arch/powerpc/kernel/nvram_64.c
+++ b/arch/powerpc/kernel/nvram_64.c
@@ -307,13 +307,15 @@  static int __init nvram_remove_os_partition(void)
 	return 0;
 }
 
-/* nvram_create_os_partition
- *
- * Create a OS linux partition to buffer error logs.
- * Will create a partition starting at the first free
- * space found if space has enough room.
+/**
+ * nvram_create_partition - Create a partition in nvram
+ * @name: name of the partition to create
+ * @sig: signature of the partition to create
+ * @req_size: size to allocate preferrably
+ * @min_size: minimum acceptable size (0 means req_size)
  */
-static int __init nvram_create_os_partition(void)
+static int __init nvram_create_partition(const char *name, int sig,
+					 int req_size, int min_size)
 {
 	struct nvram_partition *part;
 	struct nvram_partition *new_part;
@@ -322,20 +324,27 @@  static int __init nvram_create_os_partition(void)
 	loff_t tmp_index;
 	long size = 0;
 	int rc;
-	
+
+	/* If no minimum size specified, make it the same as the
+	 * requested size
+	 */
+	if (min_size == 0)
+		min_size = req_size;
+
 	/* Find a free partition that will give us the maximum needed size 
 	   If can't find one that will give us the minimum size needed */
 	list_for_each_entry(part, &nvram_part->partition, partition) {
 		if (part->header.signature != NVRAM_SIG_FREE)
 			continue;
 
-		if (part->header.length >= NVRAM_MAX_REQ) {
-			size = NVRAM_MAX_REQ;
+		if (part->header.length >= req_size) {
+			size = req_size;
 			free_part = part;
 			break;
 		}
-		if (!size && part->header.length >= NVRAM_MIN_REQ) {
-			size = NVRAM_MIN_REQ;
+		if (part->header.length > size &&
+		    part->header.length >= min_size) {
+			size = part->header.length;
 			free_part = part;
 		}
 	}
@@ -350,9 +359,9 @@  static int __init nvram_create_os_partition(void)
 	}
 
 	new_part->index = free_part->index;
-	new_part->header.signature = NVRAM_SIG_OS;
+	new_part->header.signature = sig;
 	new_part->header.length = size;
-	strcpy(new_part->header.name, "ppc64,linux");
+	strncpy(new_part->header.name, name, 12);
 	new_part->header.checksum = nvram_checksum(&new_part->header);
 
 	rc = nvram_write_header(new_part);
@@ -451,10 +460,10 @@  static int __init nvram_setup_partition(void)
 	}
 	
 	/* try creating a partition with the free space we have */
-	rc = nvram_create_partition("ppc64,linux", );
-	if (!rc) {
+	rc = 	nvram_create_partition("ppc64,linux", NVRAM_SIG_OS,
+				       NVRAM_MIN_REQ, NVRAM_MAX_REQ);
+	if (!rc)
 		return 0;
-	}
 		
 	/* need to free up some space */
 	rc = nvram_remove_os_partition();
@@ -463,9 +472,10 @@  static int __init nvram_setup_partition(void)
 	}
 	
 	/* create a partition in this new space */
-	rc = nvram_create_os_partition();
+	rc = 	nvram_create_partition("ppc64,linux", NVRAM_SIG_OS,
+				       NVRAM_MIN_REQ, NVRAM_MAX_REQ);
 	if (rc) {
-		printk(KERN_ERR "nvram_create_os_partition: Could not find a "
+		printk(KERN_ERR "nvram_create_partition: Could not find a "
 		       "NVRAM partition large enough\n");
 		return rc;
 	}
diff --git a/arch/powerpc/platforms/pseries/nvram.c b/arch/powerpc/platforms/pseries/nvram.c
index f4e4c06..2a1ef5c 100644
--- a/arch/powerpc/platforms/pseries/nvram.c
+++ b/arch/powerpc/platforms/pseries/nvram.c
@@ -22,14 +22,14 @@ 
 #include <asm/prom.h>
 #include <asm/machdep.h>
 
+/* Max bytes to read/write in one go */
+#define NVRW_CNT 0x20
+
 static unsigned int nvram_size;
 static int nvram_fetch, nvram_store;
 static char nvram_buf[NVRW_CNT];	/* assume this is in the first 4GB */
 static DEFINE_SPINLOCK(nvram_lock);
 
-/* Max bytes to read/write in one go */
-#define NVRW_CNT 0x20
-
 static ssize_t pSeries_nvram_read(char *buf, size_t count, loff_t *index)
 {
 	unsigned int i;