linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] phram: Allow the user to set the erase page size.
@ 2020-11-25  7:11 Guohua Zhong
  2020-12-04  8:08 ` ping: " Guohua Zhong
  2020-12-04  8:52 ` Richard Weinberger
  0 siblings, 2 replies; 5+ messages in thread
From: Guohua Zhong @ 2020-11-25  7:11 UTC (permalink / raw)
  To: patrick, joern, miquel.raynal, richard, vigneshr
  Cc: linux-mtd, linux-kernel, nixiaoming, wangle6, young.liuyang

Permit the user to specify the erase page size as a parameter.
This solves two problems:

- phram can access images made by mkfs.jffs2.  mkfs.jffs2 won't
create images with erase sizes less than 8KiB; many architectures
define PAGE_SIZE as 4KiB.

- Allows more effective use of small capacity devices.  JFFS2
needs somewhere between 2 and 5 empty pages for garbage collection;
and for an NVRAM part with only 32KiB of space, a smaller erase page
allows much better utilization in applications where garbage collection
is important.

Signed-off-by: Patrick O'Grady <patrick@baymotion.com>
Reviewed-by: Joern Engel <joern@logfs.org>
Link: https://lore.kernel.org/lkml/CAJ7m5OqYv_=JB9NhHsqBsa8YU0DFRoP7C+W10PY22wonAGJK=A@mail.gmail.com/
[Guohua Zhong: fix token array index out of bounds and update patch for kernel master branch]
Signed-off-by: Guohua Zhong <zhongguohua1@huawei.com>
Reported-by: kernel test robot <lkp@intel.com>

-------
v2:
 fix build error which is reported by kernel test robot

v1: https://lore.kernel.org/lkml/20201124061053.10812-1-zhongguohua1@huawei.com/
 1.fix token array index out of bounds
 2.update patch for kernel master branch
---
 drivers/mtd/devices/phram.c | 52 +++++++++++++++++++++++++++++----------------
 1 file changed, 34 insertions(+), 18 deletions(-)

diff --git a/drivers/mtd/devices/phram.c b/drivers/mtd/devices/phram.c
index 087b5e86d1bf..1729b94b2abf 100644
--- a/drivers/mtd/devices/phram.c
+++ b/drivers/mtd/devices/phram.c
@@ -6,14 +6,14 @@
  * Usage:
  *
  * one commend line parameter per device, each in the form:
- *   phram=<name>,<start>,<len>
+ *   phram=<name>,<start>,<len>[,<erasesize>]
  * <name> may be up to 63 characters.
- * <start> and <len> can be octal, decimal or hexadecimal.  If followed
+ * <start>, <len>, and <erasesize> can be octal, decimal or hexadecimal.  If followed
  * by "ki", "Mi" or "Gi", the numbers will be interpreted as kilo, mega or
- * gigabytes.
+ * gigabytes. <erasesize> is optional and defaults to PAGE_SIZE.
  *
  * Example:
- *	phram=swap,64Mi,128Mi phram=test,900Mi,1Mi
+ *	phram=swap,64Mi,128Mi phram=test,900Mi,1Mi,64Ki
  */
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
@@ -26,6 +26,7 @@
 #include <linux/moduleparam.h>
 #include <linux/slab.h>
 #include <linux/mtd/mtd.h>
+#include <asm/div64.h>
 
 struct phram_mtd_list {
 	struct mtd_info mtd;
@@ -88,7 +89,7 @@ static void unregister_devices(void)
 	}
 }
 
-static int register_device(char *name, phys_addr_t start, size_t len)
+static int register_device(char *name, phys_addr_t start, size_t len, uint32_t erasesize)
 {
 	struct phram_mtd_list *new;
 	int ret = -ENOMEM;
@@ -115,7 +116,7 @@ static int register_device(char *name, phys_addr_t start, size_t len)
 	new->mtd._write = phram_write;
 	new->mtd.owner = THIS_MODULE;
 	new->mtd.type = MTD_RAM;
-	new->mtd.erasesize = PAGE_SIZE;
+	new->mtd.erasesize = erasesize;
 	new->mtd.writesize = 1;
 
 	ret = -EAGAIN;
@@ -204,22 +205,23 @@ static inline void kill_final_newline(char *str)
 static int phram_init_called;
 /*
  * This shall contain the module parameter if any. It is of the form:
- * - phram=<device>,<address>,<size> for module case
- * - phram.phram=<device>,<address>,<size> for built-in case
- * We leave 64 bytes for the device name, 20 for the address and 20 for the
- * size.
- * Example: phram.phram=rootfs,0xa0000000,512Mi
+ * - phram=<device>,<address>,<size>[,<erasesize>] for module case
+ * - phram.phram=<device>,<address>,<size>[,<erasesize>] for built-in case
+ * We leave 64 bytes for the device name, 20 for the address , 20 for the
+ * size and 20 for the erasesize.
+ * Example: phram.phram=rootfs,0xa0000000,512Mi,65536
  */
-static char phram_paramline[64 + 20 + 20];
+static char phram_paramline[64 + 20 + 20 + 20];
 #endif
 
 static int phram_setup(const char *val)
 {
-	char buf[64 + 20 + 20], *str = buf;
-	char *token[3];
+	char buf[64 + 20 + 20 + 20], *str = buf;
+	char *token[4];
 	char *name;
 	uint64_t start;
 	uint64_t len;
+	uint64_t erasesize = PAGE_SIZE;
 	int i, ret;
 
 	if (strnlen(val, sizeof(buf)) >= sizeof(buf))
@@ -228,7 +230,7 @@ static int phram_setup(const char *val)
 	strcpy(str, val);
 	kill_final_newline(str);
 
-	for (i = 0; i < 3; i++)
+	for (i = 0; i < 4; i++)
 		token[i] = strsep(&str, ",");
 
 	if (str)
@@ -253,11 +255,25 @@ static int phram_setup(const char *val)
 		goto error;
 	}
 
-	ret = register_device(name, start, len);
+	if (token[3]) {
+		ret = parse_num64(&erasesize, token[3]);
+		if (ret) {
+			parse_err("illegal erasesize\n");
+			goto error;
+		}
+	}
+
+	if (len == 0 || erasesize == 0 || erasesize > len
+	    || erasesize > UINT_MAX || do_div(len, (uint32_t)erasesize) != 0) {
+		parse_err("illegal erasesize or len\n");
+		goto error;
+	}
+
+	ret = register_device(name, start, len, (uint32_t)erasesize);
 	if (ret)
 		goto error;
 
-	pr_info("%s device: %#llx at %#llx\n", name, len, start);
+	pr_info("%s device: %#llx at %#llx for erasesize %#llx\n", name, len, start, erasesize);
 	return 0;
 
 error:
@@ -298,7 +314,7 @@ static int phram_param_call(const char *val, const struct kernel_param *kp)
 }
 
 module_param_call(phram, phram_param_call, NULL, NULL, 0200);
-MODULE_PARM_DESC(phram, "Memory region to map. \"phram=<name>,<start>,<length>\"");
+MODULE_PARM_DESC(phram, "Memory region to map. \"phram=<name>,<start>,<length>[,<erasesize>]\"");
 
 
 static int __init init_phram(void)
-- 
2.12.3


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* ping: [PATCH v2] phram: Allow the user to set the erase page size.
  2020-11-25  7:11 [PATCH v2] phram: Allow the user to set the erase page size Guohua Zhong
@ 2020-12-04  8:08 ` Guohua Zhong
  2020-12-04  8:52 ` Richard Weinberger
  1 sibling, 0 replies; 5+ messages in thread
From: Guohua Zhong @ 2020-12-04  8:08 UTC (permalink / raw)
  To: zhongguohua1
  Cc: joern, linux-kernel, linux-mtd, miquel.raynal, nixiaoming,
	patrick, richard, vigneshr, wangle6, young.liuyang

ping

On 11/25/20, Guohua Zhong <zhongguohua1@huawei.com> wrote:
> Permit the user to specify the erase page size as a parameter.
> This solves two problems:
>
> - phram can access images made by mkfs.jffs2.  mkfs.jffs2 won't
> create images with erase sizes less than 8KiB; many architectures
> define PAGE_SIZE as 4KiB.
>
> - Allows more effective use of small capacity devices.  JFFS2
> needs somewhere between 2 and 5 empty pages for garbage collection;
> and for an NVRAM part with only 32KiB of space, a smaller erase page
> allows much better utilization in applications where garbage collection
> is important.
>
> Signed-off-by: Patrick O'Grady <patrick@baymotion.com>
> Reviewed-by: Joern Engel <joern@logfs.org>
> Link: https://lore.kernel.org/lkml/CAJ7m5OqYv_=JB9NhHsqBsa8YU0DFRoP7C+W10PY22wonAGJK=A@mail.gmail.com/
> [Guohua Zhong: fix token array index out of bounds and update patch for kernel master branch]
> Signed-off-by: Guohua Zhong <zhongguohua1@huawei.com>
> Reported-by: kernel test robot <lkp@intel.com>
>
> -------
> v2:
> fix build error which is reported by kernel test robot
>
> v1: https://lore.kernel.org/lkml/20201124061053.10812-1-zhongguohua1@huawei.com/
> 1.fix token array index out of bounds
> 2.update patch for kernel master branch
> ---
>  drivers/mtd/devices/phram.c | 52 +++++++++++++++++++++++++++++----------------
>  1 file changed, 34 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/mtd/devices/phram.c b/drivers/mtd/devices/phram.c
> index 087b5e86d1bf..1729b94b2abf 100644
> --- a/drivers/mtd/devices/phram.c
> +++ b/drivers/mtd/devices/phram.c
> @@ -6,14 +6,14 @@
>   * Usage:
>   *
>   * one commend line parameter per device, each in the form:
>  - *   phram=<name>,<start>,<len>
> + *   phram=<name>,<start>,<len>[,<erasesize>]
>   * <name> may be up to 63 characters.
> - * <start> and <len> can be octal, decimal or hexadecimal.  If followed
> + * <start>, <len>, and <erasesize> can be octal, decimal or hexadecimal.  If followed
>   * by "ki", "Mi" or "Gi", the numbers will be interpreted as kilo, mega or
> - * gigabytes.
> + * gigabytes. <erasesize> is optional and defaults to PAGE_SIZE.
>   *
>   * Example:
> - *	phram=swap,64Mi,128Mi phram=test,900Mi,1Mi
> + *	phram=swap,64Mi,128Mi phram=test,900Mi,1Mi,64Ki
>   */
>  
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> @@ -26,6 +26,7 @@
>  #include <linux/moduleparam.h>
>  #include <linux/slab.h>
>  #include <linux/mtd/mtd.h>
> +#include <asm/div64.h>
>  
>  struct phram_mtd_list {
>  struct mtd_info mtd;
> @@ -88,7 +89,7 @@ static void unregister_devices(void)
>  	}
>  }
>  
> -static int register_device(char *name, phys_addr_t start, size_t len)
> +static int register_device(char *name, phys_addr_t start, size_t len, uint32_t erasesize)
>  {
>  	struct phram_mtd_list *new;
>  	int ret = -ENOMEM;
> @@ -115,7 +116,7 @@ static int register_device(char *name, phys_addr_t start, size_t len)
>  	new->mtd._write = phram_write;
>  	new->mtd.owner = THIS_MODULE;
>  	new->mtd.type = MTD_RAM;
> -	new->mtd.erasesize = PAGE_SIZE;
> +	new->mtd.erasesize = erasesize;
>  	new->mtd.writesize = 1;
>  
>  	ret = -EAGAIN;
> @@ -204,22 +205,23 @@ static inline void kill_final_newline(char *str)
>  static int phram_init_called;
>  /*
>   * This shall contain the module parameter if any. It is of the form:
> - * - phram=<device>,<address>,<size> for module case
> - * - phram.phram=<device>,<address>,<size> for built-in case
> - * We leave 64 bytes for the device name, 20 for the address and 20 for the
> - * size.
> - * Example: phram.phram=rootfs,0xa0000000,512Mi
> + * - phram=<device>,<address>,<size>[,<erasesize>] for module case
> + * - phram.phram=<device>,<address>,<size>[,<erasesize>] for built-in case
> + * We leave 64 bytes for the device name, 20 for the address , 20 for the
> + * size and 20 for the erasesize.
> + * Example: phram.phram=rootfs,0xa0000000,512Mi,65536
>   */
> -static char phram_paramline[64 + 20 + 20];
> +static char phram_paramline[64 + 20 + 20 + 20];
>  #endif
>  
>  static int phram_setup(const char *val)
>  {
> -	char buf[64 + 20 + 20], *str = buf;
> -	char *token[3];
> +	char buf[64 + 20 + 20 + 20], *str = buf;
> +	char *token[4];
>  	char *name;
>  	uint64_t start;
>  	uint64_t len;
> +	uint64_t erasesize = PAGE_SIZE;
>  	int i, ret;
>  
>  	if (strnlen(val, sizeof(buf)) >= sizeof(buf))
> @@ -228,7 +230,7 @@ static int phram_setup(const char *val)
>  	strcpy(str, val);
>  	kill_final_newline(str);
>  
> -	for (i = 0; i < 3; i++)
> +	for (i = 0; i < 4; i++)
>  		token[i] = strsep(&str, ",");
>  
>  	if (str)
> @@ -253,11 +255,25 @@ static int phram_setup(const char *val)
>  		goto error;
>  	}
>  
> -	ret = register_device(name, start, len);
> +	if (token[3]) {
> +		ret = parse_num64(&erasesize, token[3]);
> +		if (ret) {
> +			parse_err("illegal erasesize\n");
> +			goto error;
> +		}
> +	}
> +
> +	if (len == 0 || erasesize == 0 || erasesize > len
> +	    || erasesize > UINT_MAX || do_div(len, (uint32_t)erasesize) != 0) {
> +		parse_err("illegal erasesize or len\n");
> +		goto error;
> +	}
> +
> +	ret = register_device(name, start, len, (uint32_t)erasesize);
>  	if (ret)
>  		goto error;
>  
> -	pr_info("%s device: %#llx at %#llx\n", name, len, start);
> +	pr_info("%s device: %#llx at %#llx for erasesize %#llx\n", name, len, start, erasesize);
>  	return 0;
>  
>  error:
> @@ -298,7 +314,7 @@ static int phram_param_call(const char *val, const struct kernel_param *kp)
>  }
>  
>  module_param_call(phram, phram_param_call, NULL, NULL, 0200);
> -MODULE_PARM_DESC(phram, "Memory region to map. \"phram=<name>,<start>,<length>\"");
> +MODULE_PARM_DESC(phram, "Memory region to map. \"phram=<name>,<start>,<length>[,<erasesize>]\"");
>  
>  
>  static int __init init_phram(void)
> -- 
> 2.12.3



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] phram: Allow the user to set the erase page size.
  2020-11-25  7:11 [PATCH v2] phram: Allow the user to set the erase page size Guohua Zhong
  2020-12-04  8:08 ` ping: " Guohua Zhong
@ 2020-12-04  8:52 ` Richard Weinberger
  2020-12-07  7:07   ` Guohua Zhong
  1 sibling, 1 reply; 5+ messages in thread
From: Richard Weinberger @ 2020-12-04  8:52 UTC (permalink / raw)
  To: Guohua Zhong
  Cc: patrick, Joern Engel, Miquel Raynal, Richard Weinberger,
	Vignesh Raghavendra, linux-mtd, LKML, Xiaoming Ni, wangle6,
	young.liuyang

On Wed, Nov 25, 2020 at 8:14 AM Guohua Zhong <zhongguohua1@huawei.com> wrote:
>
> Permit the user to specify the erase page size as a parameter.
> This solves two problems:
>
> - phram can access images made by mkfs.jffs2.  mkfs.jffs2 won't
> create images with erase sizes less than 8KiB; many architectures
> define PAGE_SIZE as 4KiB.
>
> - Allows more effective use of small capacity devices.  JFFS2
> needs somewhere between 2 and 5 empty pages for garbage collection;
> and for an NVRAM part with only 32KiB of space, a smaller erase page
> allows much better utilization in applications where garbage collection
> is important.
>
> Signed-off-by: Patrick O'Grady <patrick@baymotion.com>
> Reviewed-by: Joern Engel <joern@logfs.org>
> Link: https://lore.kernel.org/lkml/CAJ7m5OqYv_=JB9NhHsqBsa8YU0DFRoP7C+W10PY22wonAGJK=A@mail.gmail.com/
> [Guohua Zhong: fix token array index out of bounds and update patch for kernel master branch]
> Signed-off-by: Guohua Zhong <zhongguohua1@huawei.com>
> Reported-by: kernel test robot <lkp@intel.com>

Looks good to me, except the authorship.
If I understand correctly, you took this old patch and resend it.
Please make sure that the "From:"-Line contains the original author.
You can fix this up using git commit --amend --author=.
The git format-patch will create a correct patch.

-- 
Thanks,
//richard

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: Re: [PATCH v2] phram: Allow the user to set the erase page size.
  2020-12-04  8:52 ` Richard Weinberger
@ 2020-12-07  7:07   ` Guohua Zhong
  2020-12-07  7:56     ` Miquel Raynal
  0 siblings, 1 reply; 5+ messages in thread
From: Guohua Zhong @ 2020-12-07  7:07 UTC (permalink / raw)
  To: richard.weinberger
  Cc: joern, linux-kernel, linux-mtd, miquel.raynal, nixiaoming,
	patrick, richard, vigneshr, wangle6, young.liuyang, zhongguohua1

On Mon, Dec 7, 2020 at 14:56 AM Guohua Zhong <zhongguohua1@huawei.com> wrote:
>
>> Permit the user to specify the erase page size as a parameter.
>> This solves two problems:
>
>> - phram can access images made by mkfs.jffs2.  mkfs.jffs2 won't
>> create images with erase sizes less than 8KiB; many architectures
>> define PAGE_SIZE as 4KiB.
>
>> - Allows more effective use of small capacity devices.  JFFS2
>> needs somewhere between 2 and 5 empty pages for garbage collection;
>> and for an NVRAM part with only 32KiB of space, a smaller erase page
>> allows much better utilization in applications where garbage collection
>> is important.
>
>> Signed-off-by: Patrick O'Grady <patrick@baymotion.com>
>> Reviewed-by: Joern Engel <joern@logfs.org>
>> Link: https://lore.kernel.org/lkml/CAJ7m5OqYv_=JB9NhHsqBsa8YU0DFRoP7C+W10PY22wonAGJK=A@mail.gmail.com/
>> [Guohua Zhong: fix token array index out of bounds and update patch for kernel master branch]
>> Signed-off-by: Guohua Zhong <zhongguohua1@huawei.com>
>> Reported-by: kernel test robot <lkp@intel.com>
>
> Looks good to me, except the authorship.
> If I understand correctly, you took this old patch and resend it.
> Please make sure that the "From:"-Line contains the original author.
> You can fix this up using git commit --amend --author=.
> The git format-patch will create a correct patch.

Sorry, I am not clear this rule before. But I found the same issue independently. It looks good 
after changging the erase size for phram driver. Then when I try to send the patch, I found that 
Patrick O'Grady has already send a patch which has not been merged as the link below
https://lore.kernel.org/lkml/CAJ7m5OqYv_=JB9NhHsqBsa8YU0DFRoP7C+W10PY22wonAGJK=A@mail.gmail.com/

So I resend a patch with some change and fix for mainline kernel with the old patch link of Patrick O'Grady.

If I need to change the authorship, I will resend this patch for V3 with authorship of Patrick O'Grady.

-- 
Thanks,
//guohua

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] phram: Allow the user to set the erase page size.
  2020-12-07  7:07   ` Guohua Zhong
@ 2020-12-07  7:56     ` Miquel Raynal
  0 siblings, 0 replies; 5+ messages in thread
From: Miquel Raynal @ 2020-12-07  7:56 UTC (permalink / raw)
  To: Guohua Zhong
  Cc: richard.weinberger, joern, linux-kernel, linux-mtd, nixiaoming,
	patrick, richard, vigneshr, wangle6, young.liuyang

Hi Guohua,

Guohua Zhong <zhongguohua1@huawei.com> wrote on Mon, 7 Dec 2020
15:07:15 +0800:

> On Mon, Dec 7, 2020 at 14:56 AM Guohua Zhong <zhongguohua1@huawei.com> wrote:
> >  
> >> Permit the user to specify the erase page size as a parameter.
> >> This solves two problems:  
> >  
> >> - phram can access images made by mkfs.jffs2.  mkfs.jffs2 won't
> >> create images with erase sizes less than 8KiB; many architectures
> >> define PAGE_SIZE as 4KiB.  
> >  
> >> - Allows more effective use of small capacity devices.  JFFS2
> >> needs somewhere between 2 and 5 empty pages for garbage collection;
> >> and for an NVRAM part with only 32KiB of space, a smaller erase page
> >> allows much better utilization in applications where garbage collection
> >> is important.  
> >  
> >> Signed-off-by: Patrick O'Grady <patrick@baymotion.com>
> >> Reviewed-by: Joern Engel <joern@logfs.org>
> >> Link: https://lore.kernel.org/lkml/CAJ7m5OqYv_=JB9NhHsqBsa8YU0DFRoP7C+W10PY22wonAGJK=A@mail.gmail.com/
> >> [Guohua Zhong: fix token array index out of bounds and update patch for kernel master branch]
> >> Signed-off-by: Guohua Zhong <zhongguohua1@huawei.com>
> >> Reported-by: kernel test robot <lkp@intel.com>  
> >
> > Looks good to me, except the authorship.
> > If I understand correctly, you took this old patch and resend it.
> > Please make sure that the "From:"-Line contains the original author.
> > You can fix this up using git commit --amend --author=.
> > The git format-patch will create a correct patch.  
> 
> Sorry, I am not clear this rule before. But I found the same issue independently. It looks good 
> after changging the erase size for phram driver. Then when I try to send the patch, I found that 
> Patrick O'Grady has already send a patch which has not been merged as the link below
> https://lore.kernel.org/lkml/CAJ7m5OqYv_=JB9NhHsqBsa8YU0DFRoP7C+W10PY22wonAGJK=A@mail.gmail.com/
> 
> So I resend a patch with some change and fix for mainline kernel with the old patch link of Patrick O'Grady.
> 
> If I need to change the authorship, I will resend this patch for V3 with authorship of Patrick O'Grady.

Yes please. Also please drop the reference to kernel test robot as you
are not fixing an issue in the kernel that the robot reported (perhaps
you had a warning from the robot about the patch itself, this is
called a review and we usually don't get credited for that, at least
not with a misleading reported-by tag).

Thanks,
Miquèl

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-12-07  7:57 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-25  7:11 [PATCH v2] phram: Allow the user to set the erase page size Guohua Zhong
2020-12-04  8:08 ` ping: " Guohua Zhong
2020-12-04  8:52 ` Richard Weinberger
2020-12-07  7:07   ` Guohua Zhong
2020-12-07  7:56     ` Miquel Raynal

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).