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 <[email protected]>
Reviewed-by: Joern Engel <[email protected]>
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 <[email protected]>
Reported-by: kernel test robot <[email protected]>
-------
v2:
fix build error which is reported by kernel test robot
v1: https://lore.kernel.org/lkml/[email protected]/
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
ping
On 11/25/20, Guohua Zhong <[email protected]> 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 <[email protected]>
> Reviewed-by: Joern Engel <[email protected]>
> 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 <[email protected]>
> Reported-by: kernel test robot <[email protected]>
>
> -------
> v2:
> fix build error which is reported by kernel test robot
>
> v1: https://lore.kernel.org/lkml/[email protected]/
> 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
On Wed, Nov 25, 2020 at 8:14 AM Guohua Zhong <[email protected]> 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 <[email protected]>
> Reviewed-by: Joern Engel <[email protected]>
> 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 <[email protected]>
> Reported-by: kernel test robot <[email protected]>
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
On Mon, Dec 7, 2020 at 14:56 AM Guohua Zhong <[email protected]> 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 <[email protected]>
>> Reviewed-by: Joern Engel <[email protected]>
>> 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 <[email protected]>
>> Reported-by: kernel test robot <[email protected]>
>
> 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
Hi Guohua,
Guohua Zhong <[email protected]> wrote on Mon, 7 Dec 2020
15:07:15 +0800:
> On Mon, Dec 7, 2020 at 14:56 AM Guohua Zhong <[email protected]> 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 <[email protected]>
> >> Reviewed-by: Joern Engel <[email protected]>
> >> 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 <[email protected]>
> >> Reported-by: kernel test robot <[email protected]>
> >
> > 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