* [PATCH] mtd: phram: Allow multiple phram devices on cmd line @ 2014-10-21 18:03 Rob Ward 2014-10-21 18:25 ` Brian Norris 2014-11-09 16:24 ` Rob Ward 0 siblings, 2 replies; 6+ messages in thread From: Rob Ward @ 2014-10-21 18:03 UTC (permalink / raw) To: joern, dwmw2, computersforpeace; +Cc: linux-kernel >From 9ddd010f1e2bf4fdfac5a7627c5de821a2dcd8f5 Mon Sep 17 00:00:00 2001 From: Rob Ward <robert.ward114@googlemail.com> Date: Tue, 21 Oct 2014 17:46:53 +0100 Subject: [PATCH] mtd: phram: Allow multiple phram devices on cmd line Allow the phram module the ability to create multiple phram mtd devices via the kernel command line. Currently the phram module only allows a single mtd device to be created via the kernel command line. This is due to the phram module having to store the values until it is initialised later. This storage is done using a single char[] meaning when the module_param_call is made the previous value is overidden. This change modifies the phram system to use a char[][] allowing multiple devices to be created. The array currently allows up to 64 devices to be created via the kernel command line. If the array is full a message is printed to the console and the module_param_call returns. To test, in all cases an area of memory needs to be reserved via the command line e.g. memmap=10M$114M To test with phram build into the kernel on the command line add the following: phram.phram=alpha,114Mi,1Mi phram.phram=beta,115Mi,1Mi To test phram built as a module insmod with the following arguments: phram=gamma,114Mi,1Mi phram=delta,115Mi,1Mi In both cases two mtd devices should be created. Signed-off-by: Rob Ward <robert.ward114@googlemail.com> --- drivers/mtd/devices/phram.c | 41 ++++++++++++++++++++++++++++++++++++----- 1 file changed, 36 insertions(+), 5 deletions(-) diff --git a/drivers/mtd/devices/phram.c b/drivers/mtd/devices/phram.c index effd9a4..bd409be 100644 --- a/drivers/mtd/devices/phram.c +++ b/drivers/mtd/devices/phram.c @@ -214,7 +214,7 @@ static int phram_init_called; * size. * Example: phram.phram=rootfs,0xa0000000,512Mi */ -static char phram_paramline[64 + 20 + 20]; +static char phram_paramline[64][64 + 20 + 20]; #endif static int phram_setup(const char *val) @@ -271,6 +271,9 @@ static int phram_param_call(const char *val, struct kernel_param *kp) #ifdef MODULE return phram_setup(val); #else + int paramline_it = 0; + int arraysize = 0; + /* * If more parameters are later passed in via * /sys/module/phram/parameters/phram @@ -290,9 +293,27 @@ static int phram_param_call(const char *val, struct kernel_param *kp) * phram_setup(). */ - if (strlen(val) >= sizeof(phram_paramline)) + if (strlen(val) >= sizeof(phram_paramline[0])) return -ENOSPC; - strcpy(phram_paramline, val); + + arraysize = sizeof(phram_paramline)/sizeof(phram_paramline[0]); + + /* + * Check if any space is left in the array. If no space + * is left then print warning and return 0 + */ + + if (phram_paramline[arraysize - 1][0]) { + pr_warn("exceeded limit via cmd_line - %s ignored", val); + return 0; + } + + for (paramline_it = 0; paramline_it <= arraysize; paramline_it++) { + if (!phram_paramline[paramline_it][0]) { + strcpy(phram_paramline[paramline_it], val); + break; + } + } return 0; #endif @@ -307,8 +328,18 @@ static int __init init_phram(void) int ret = 0; #ifndef MODULE - if (phram_paramline[0]) - ret = phram_setup(phram_paramline); + int arraysize = 0; + int paramline_it = 0; + + arraysize = sizeof(phram_paramline)/sizeof(phram_paramline[0]); + + for (paramline_it = 0; paramline_it <= arraysize; paramline_it++) { + if (phram_paramline[paramline_it][0]) { + ret = phram_setup(phram_paramline[paramline_it]); + if (ret) + break; + } + } phram_init_called = 1; #endif -- 2.0.2 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mtd: phram: Allow multiple phram devices on cmd line 2014-10-21 18:03 [PATCH] mtd: phram: Allow multiple phram devices on cmd line Rob Ward @ 2014-10-21 18:25 ` Brian Norris 2014-11-09 16:24 ` Rob Ward 1 sibling, 0 replies; 6+ messages in thread From: Brian Norris @ 2014-10-21 18:25 UTC (permalink / raw) To: Rob Ward; +Cc: joern, dwmw2, linux-kernel, linux-mtd + linux-mtd On Tue, Oct 21, 2014 at 07:03:23PM +0100, Rob Ward wrote: > From 9ddd010f1e2bf4fdfac5a7627c5de821a2dcd8f5 Mon Sep 17 00:00:00 2001 > From: Rob Ward <robert.ward114@googlemail.com> > Date: Tue, 21 Oct 2014 17:46:53 +0100 > Subject: [PATCH] mtd: phram: Allow multiple phram devices on cmd line > > Allow the phram module the ability to create multiple phram mtd > devices via the kernel command line. > > Currently the phram module only allows a single mtd device to be > created via the kernel command line. This is due to the phram > module having to store the values until it is initialised > later. This storage is done using a single char[] meaning when > the module_param_call is made the previous value is overidden. > > This change modifies the phram system to use a char[][] allowing > multiple devices to be created. > > The array currently allows up to 64 devices to be created via the > kernel command line. > > If the array is full a message is printed to the console and the > module_param_call returns. > > To test, in all cases an area of memory needs to be reserved via > the command line e.g. memmap=10M$114M > > To test with phram build into the kernel on the command line add > the following: > > phram.phram=alpha,114Mi,1Mi phram.phram=beta,115Mi,1Mi > > To test phram built as a module insmod with the following arguments: > > phram=gamma,114Mi,1Mi phram=delta,115Mi,1Mi > > In both cases two mtd devices should be created. > > Signed-off-by: Rob Ward <robert.ward114@googlemail.com> > --- > drivers/mtd/devices/phram.c | 41 ++++++++++++++++++++++++++++++++++++----- > 1 file changed, 36 insertions(+), 5 deletions(-) > > diff --git a/drivers/mtd/devices/phram.c b/drivers/mtd/devices/phram.c > index effd9a4..bd409be 100644 > --- a/drivers/mtd/devices/phram.c > +++ b/drivers/mtd/devices/phram.c > @@ -214,7 +214,7 @@ static int phram_init_called; > * size. > * Example: phram.phram=rootfs,0xa0000000,512Mi > */ > -static char phram_paramline[64 + 20 + 20]; > +static char phram_paramline[64][64 + 20 + 20]; > #endif > > static int phram_setup(const char *val) > @@ -271,6 +271,9 @@ static int phram_param_call(const char *val, struct kernel_param *kp) > #ifdef MODULE > return phram_setup(val); > #else > + int paramline_it = 0; > + int arraysize = 0; > + > /* > * If more parameters are later passed in via > * /sys/module/phram/parameters/phram > @@ -290,9 +293,27 @@ static int phram_param_call(const char *val, struct kernel_param *kp) > * phram_setup(). > */ > > - if (strlen(val) >= sizeof(phram_paramline)) > + if (strlen(val) >= sizeof(phram_paramline[0])) > return -ENOSPC; > - strcpy(phram_paramline, val); > + > + arraysize = sizeof(phram_paramline)/sizeof(phram_paramline[0]); > + > + /* > + * Check if any space is left in the array. If no space > + * is left then print warning and return 0 > + */ > + > + if (phram_paramline[arraysize - 1][0]) { > + pr_warn("exceeded limit via cmd_line - %s ignored", val); > + return 0; > + } > + > + for (paramline_it = 0; paramline_it <= arraysize; paramline_it++) { > + if (!phram_paramline[paramline_it][0]) { > + strcpy(phram_paramline[paramline_it], val); > + break; > + } > + } > > return 0; > #endif > @@ -307,8 +328,18 @@ static int __init init_phram(void) > int ret = 0; > > #ifndef MODULE > - if (phram_paramline[0]) > - ret = phram_setup(phram_paramline); > + int arraysize = 0; > + int paramline_it = 0; > + > + arraysize = sizeof(phram_paramline)/sizeof(phram_paramline[0]); > + > + for (paramline_it = 0; paramline_it <= arraysize; paramline_it++) { > + if (phram_paramline[paramline_it][0]) { > + ret = phram_setup(phram_paramline[paramline_it]); > + if (ret) > + break; > + } > + } > phram_init_called = 1; > #endif > > -- > 2.0.2 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] mtd: phram: Allow multiple phram devices on cmd line 2014-10-21 18:03 [PATCH] mtd: phram: Allow multiple phram devices on cmd line Rob Ward 2014-10-21 18:25 ` Brian Norris @ 2014-11-09 16:24 ` Rob Ward 2014-11-26 6:32 ` Brian Norris 2014-12-03 7:27 ` [PATCH V3] " Rob Ward 1 sibling, 2 replies; 6+ messages in thread From: Rob Ward @ 2014-11-09 16:24 UTC (permalink / raw) To: joern, dwmw2, computersforpeace; +Cc: linux-kernel, linux-mtd >From 4e9b8ff3a6731a0ac43eac2e8bdf47101ff20ede Mon Sep 17 00:00:00 2001 From: Rob Ward <robert.ward114@googlemail.com> Date: Tue, 21 Oct 2014 17:46:53 +0100 Subject: [PATCH] mtd: phram: Allow multiple phram devices on cmd line Allow the phram module the ability to create multiple phram mtd devices via the kernel command line. Currently the phram module only allows a single mtd device to be created via the kernel command line. This is due to the phram module having to store the values until it is initialised later. This storage is done using a single char[] meaning when the module_param_call is made the previous value is overidden. This change modifies the phram system to use a char[][] allowing multiple devices to be created. The array size if controlled using the new config option CONFIG_MTD_PHRAM_MAX_CMDLINE_ARGS that allows the maximum number of devices to be specified. Currently this option defaults to a value of 1 leaving the behaviour unchanged. If the array is full a message is printed to the console and the module_param_call returns. To test, in all cases an area of memory needs to be reserved via the command line e.g. memmap=10M$114M To test with phram build into the kernel on the command line add the following: phram.phram=alpha,114Mi,1Mi phram.phram=beta,115Mi,1Mi If CONFIG_MTD_PHRAM_MAX_CMDLINE_ARGS is left as default i.e. 1 then the first device, alpha will be created only. If the value of CONFIG_MTD_PHRAM_MAX_CMDLINE_ARGS is increased to 2 or more then both alpha and beta will be created. To test phram built as a module insmod with the following arguments: phram=gamma,114Mi,1Mi phram=delta,115Mi,1Mi In this case two devices should be created. Signed-off-by: Rob Ward <robert.ward114@googlemail.com> --- drivers/mtd/devices/Kconfig | 12 ++++++++++++ drivers/mtd/devices/phram.c | 45 ++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 52 insertions(+), 5 deletions(-) diff --git a/drivers/mtd/devices/Kconfig b/drivers/mtd/devices/Kconfig index c49d0b1..5fdc80b 100644 --- a/drivers/mtd/devices/Kconfig +++ b/drivers/mtd/devices/Kconfig @@ -136,6 +136,18 @@ config MTD_PHRAM doesn't have access to, memory beyond the mem=xxx limit, nvram, memory on the video card, etc... +config MTD_PHRAM_MAX_CMDLINE_ARGS + int "Max number of devices via Kernel command line" + depends on MTD_PHRAM=y + default 1 + help + Specify the number of phram devices that can be initialised + using the Kernel command line. + + This option is only applicable when phram is built into the + Kernel. When built as a module many devices can be specified + at module insmod. + config MTD_LART tristate "28F160xx flash driver for LART" depends on SA1100_LART diff --git a/drivers/mtd/devices/phram.c b/drivers/mtd/devices/phram.c index effd9a4..223f221 100644 --- a/drivers/mtd/devices/phram.c +++ b/drivers/mtd/devices/phram.c @@ -212,9 +212,13 @@ static int phram_init_called; * - 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. + * + * The maximum number of devices supported is controlled by the + * MTD_PHRAM_MAX_CMDLINE_ARGS config option + * * Example: phram.phram=rootfs,0xa0000000,512Mi */ -static char phram_paramline[64 + 20 + 20]; +static char phram_paramline[CONFIG_MTD_PHRAM_MAX_CMDLINE_ARGS][64 + 20 + 20]; #endif static int phram_setup(const char *val) @@ -271,6 +275,9 @@ static int phram_param_call(const char *val, struct kernel_param *kp) #ifdef MODULE return phram_setup(val); #else + int paramline_it = 0; + int arraysize = 0; + /* * If more parameters are later passed in via * /sys/module/phram/parameters/phram @@ -290,9 +297,27 @@ static int phram_param_call(const char *val, struct kernel_param *kp) * phram_setup(). */ - if (strlen(val) >= sizeof(phram_paramline)) + if (strlen(val) >= sizeof(phram_paramline[0])) return -ENOSPC; - strcpy(phram_paramline, val); + + arraysize = sizeof(phram_paramline)/sizeof(phram_paramline[0]); + + /* + * Check if any space is left in the array. If no space + * is left then print warning and return 0 + */ + + if (phram_paramline[arraysize - 1][0]) { + pr_warn("exceeded limit via cmd_line - %s ignored", val); + return 0; + } + + for (paramline_it = 0; paramline_it <= arraysize; paramline_it++) { + if (!phram_paramline[paramline_it][0]) { + strcpy(phram_paramline[paramline_it], val); + break; + } + } return 0; #endif @@ -307,8 +332,18 @@ static int __init init_phram(void) int ret = 0; #ifndef MODULE - if (phram_paramline[0]) - ret = phram_setup(phram_paramline); + int arraysize = 0; + int paramline_it = 0; + + arraysize = sizeof(phram_paramline)/sizeof(phram_paramline[0]); + + for (paramline_it = 0; paramline_it <= arraysize; paramline_it++) { + if (phram_paramline[paramline_it][0]) { + ret = phram_setup(phram_paramline[paramline_it]); + if (ret) + break; + } + } phram_init_called = 1; #endif -- 2.0.2 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mtd: phram: Allow multiple phram devices on cmd line 2014-11-09 16:24 ` Rob Ward @ 2014-11-26 6:32 ` Brian Norris 2014-11-26 8:54 ` Rob Ward 2014-12-03 7:27 ` [PATCH V3] " Rob Ward 1 sibling, 1 reply; 6+ messages in thread From: Brian Norris @ 2014-11-26 6:32 UTC (permalink / raw) To: Rob Ward; +Cc: joern, dwmw2, linux-kernel, linux-mtd On Sun, Nov 09, 2014 at 04:24:44PM +0000, Rob Ward wrote: > From 4e9b8ff3a6731a0ac43eac2e8bdf47101ff20ede Mon Sep 17 00:00:00 2001 > From: Rob Ward <robert.ward114@googlemail.com> > Date: Tue, 21 Oct 2014 17:46:53 +0100 > Subject: [PATCH] mtd: phram: Allow multiple phram devices on cmd line > > Allow the phram module the ability to create multiple phram mtd > devices via the kernel command line. Is the sysfs module parameter option not sufficient? /sys/module/phram/parameters/phram It looks like it should be reusable for multiple device creations. Notably, we use 000 permissions for module_param_call(), so the parameter doesn't even show up by default AFAICT. I had to hack it to 0600 or similar. > Currently the phram module only allows a single mtd device to be > created via the kernel command line. This is due to the phram > module having to store the values until it is initialised > later. This storage is done using a single char[] meaning when > the module_param_call is made the previous value is overidden. > > This change modifies the phram system to use a char[][] allowing > multiple devices to be created. > > The array size if controlled using the new config option > CONFIG_MTD_PHRAM_MAX_CMDLINE_ARGS that allows the maximum > number of devices to be specified. Currently this option > defaults to a value of 1 leaving the behaviour unchanged. > > If the array is full a message is printed to the console and the > module_param_call returns. > > To test, in all cases an area of memory needs to be reserved via > the command line e.g. memmap=10M$114M > > To test with phram build into the kernel on the command line add > the following: > > phram.phram=alpha,114Mi,1Mi phram.phram=beta,115Mi,1Mi > > If CONFIG_MTD_PHRAM_MAX_CMDLINE_ARGS is left as default i.e. 1 > then the first device, alpha will be created only. If the value of > CONFIG_MTD_PHRAM_MAX_CMDLINE_ARGS is increased to 2 or more then > both alpha and beta will be created. I really don't think we want this to be a Kconfig option. We can dynamically allocate and use a linked list instead, which would be more flexible and avoid making a fixed compile-time choice. > To test phram built as a module insmod with the following arguments: > > phram=gamma,114Mi,1Mi phram=delta,115Mi,1Mi > > In this case two devices should be created. > > Signed-off-by: Rob Ward <robert.ward114@googlemail.com> > --- > drivers/mtd/devices/Kconfig | 12 ++++++++++++ > drivers/mtd/devices/phram.c | 45 ++++++++++++++++++++++++++++++++++++++++----- > 2 files changed, 52 insertions(+), 5 deletions(-) > > diff --git a/drivers/mtd/devices/Kconfig b/drivers/mtd/devices/Kconfig > index c49d0b1..5fdc80b 100644 > --- a/drivers/mtd/devices/Kconfig > +++ b/drivers/mtd/devices/Kconfig > @@ -136,6 +136,18 @@ config MTD_PHRAM > doesn't have access to, memory beyond the mem=xxx limit, nvram, > memory on the video card, etc... > > +config MTD_PHRAM_MAX_CMDLINE_ARGS > + int "Max number of devices via Kernel command line" > + depends on MTD_PHRAM=y > + default 1 > + help > + Specify the number of phram devices that can be initialised > + using the Kernel command line. > + > + This option is only applicable when phram is built into the > + Kernel. When built as a module many devices can be specified > + at module insmod. > + > config MTD_LART > tristate "28F160xx flash driver for LART" > depends on SA1100_LART > diff --git a/drivers/mtd/devices/phram.c b/drivers/mtd/devices/phram.c > index effd9a4..223f221 100644 > --- a/drivers/mtd/devices/phram.c > +++ b/drivers/mtd/devices/phram.c > @@ -212,9 +212,13 @@ static int phram_init_called; > * - 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. > + * > + * The maximum number of devices supported is controlled by the > + * MTD_PHRAM_MAX_CMDLINE_ARGS config option > + * > * Example: phram.phram=rootfs,0xa0000000,512Mi > */ > -static char phram_paramline[64 + 20 + 20]; > +static char phram_paramline[CONFIG_MTD_PHRAM_MAX_CMDLINE_ARGS][64 + 20 + 20]; > #endif > > static int phram_setup(const char *val) > @@ -271,6 +275,9 @@ static int phram_param_call(const char *val, struct kernel_param *kp) > #ifdef MODULE > return phram_setup(val); > #else > + int paramline_it = 0; > + int arraysize = 0; Neither of these need to be initialized here. And can you shorten the long-ish names? We don't need an "iterator" -- it's just and index 'i'. > + > /* > * If more parameters are later passed in via > * /sys/module/phram/parameters/phram > @@ -290,9 +297,27 @@ static int phram_param_call(const char *val, struct kernel_param *kp) > * phram_setup(). > */ > > - if (strlen(val) >= sizeof(phram_paramline)) > + if (strlen(val) >= sizeof(phram_paramline[0])) > return -ENOSPC; > - strcpy(phram_paramline, val); > + > + arraysize = sizeof(phram_paramline)/sizeof(phram_paramline[0]); Coccinelle complains: drivers/mtd/devices/phram.c:303:35-36: WARNING: Use ARRAY_SIZE > + > + /* > + * Check if any space is left in the array. If no space > + * is left then print warning and return 0 > + */ > + > + if (phram_paramline[arraysize - 1][0]) { > + pr_warn("exceeded limit via cmd_line - %s ignored", val); > + return 0; > + } > + > + for (paramline_it = 0; paramline_it <= arraysize; paramline_it++) { Use '<' not '<='. You're overflowing the array. > + if (!phram_paramline[paramline_it][0]) { > + strcpy(phram_paramline[paramline_it], val); > + break; > + } > + } > > return 0; > #endif > @@ -307,8 +332,18 @@ static int __init init_phram(void) > int ret = 0; > > #ifndef MODULE > - if (phram_paramline[0]) > - ret = phram_setup(phram_paramline); > + int arraysize = 0; > + int paramline_it = 0; Same comments as above. Neither of these need to be initialized here, and shorten the name. > + > + arraysize = sizeof(phram_paramline)/sizeof(phram_paramline[0]); drivers/mtd/devices/phram.c:338:35-36: WARNING: Use ARRAY_SIZE > + > + for (paramline_it = 0; paramline_it <= arraysize; paramline_it++) { You're off-by-one; use '<', not '<='. So this line could just be: for (i = 0; i < ARRAY_SIZE(phram_paramline); i++) { (But then, we can avoid this by just allocating and appending to a linked list.) > + if (phram_paramline[paramline_it][0]) { > + ret = phram_setup(phram_paramline[paramline_it]); > + if (ret) > + break; > + } > + } > phram_init_called = 1; > #endif > Brian ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mtd: phram: Allow multiple phram devices on cmd line 2014-11-26 6:32 ` Brian Norris @ 2014-11-26 8:54 ` Rob Ward 0 siblings, 0 replies; 6+ messages in thread From: Rob Ward @ 2014-11-26 8:54 UTC (permalink / raw) To: Brian Norris; +Cc: Rob Ward, joern, dwmw2, linux-kernel, linux-mtd On Tue, 25 Nov 2014, Brian Norris wrote: > On Sun, Nov 09, 2014 at 04:24:44PM +0000, Rob Ward wrote: > > From 4e9b8ff3a6731a0ac43eac2e8bdf47101ff20ede Mon Sep 17 00:00:00 2001 > > From: Rob Ward <robert.ward114@googlemail.com> > > Date: Tue, 21 Oct 2014 17:46:53 +0100 > > Subject: [PATCH] mtd: phram: Allow multiple phram devices on cmd line > > > > Allow the phram module the ability to create multiple phram mtd > > devices via the kernel command line. > > Is the sysfs module parameter option not sufficient? > While using the sysfs would work in most situations there are times when it is less than ideal. An example is where the information regarding the locations of filesystems within RAM is not know by the kernel and needs to be passed from the bootloader. This can be acheived a number of ways but passing via the cmdline and having phram understand multiple options is a nicer and consistent solution. > > /sys/module/phram/parameters/phram > > It looks like it should be reusable for multiple device creations. > > Notably, we use 000 permissions for module_param_call(), so the > parameter doesn't even show up by default AFAICT. I had to hack it to > 0600 or similar. > > > Currently the phram module only allows a single mtd device to be > > created via the kernel command line. This is due to the phram > > module having to store the values until it is initialised > > later. This storage is done using a single char[] meaning when > > the module_param_call is made the previous value is overidden. > > > > This change modifies the phram system to use a char[][] allowing > > multiple devices to be created. > > > > The array size if controlled using the new config option > > CONFIG_MTD_PHRAM_MAX_CMDLINE_ARGS that allows the maximum > > number of devices to be specified. Currently this option > > defaults to a value of 1 leaving the behaviour unchanged. > > > > If the array is full a message is printed to the console and the > > module_param_call returns. > > > > To test, in all cases an area of memory needs to be reserved via > > the command line e.g. memmap=10M$114M > > > > To test with phram build into the kernel on the command line add > > the following: > > > > phram.phram=alpha,114Mi,1Mi phram.phram=beta,115Mi,1Mi > > > > If CONFIG_MTD_PHRAM_MAX_CMDLINE_ARGS is left as default i.e. 1 > > then the first device, alpha will be created only. If the value of > > CONFIG_MTD_PHRAM_MAX_CMDLINE_ARGS is increased to 2 or more then > > both alpha and beta will be created. > > I really don't think we want this to be a Kconfig option. We can > dynamically allocate and use a linked list instead, which would be more > flexible and avoid making a fixed compile-time choice. > My first thought was to implement a linked list, however in the phram code there is a comment: phram_param_call() is called so early that it is not possible to resolve the device (even kmalloc() fails). Defer that work to phram_setup(). To test I modify the current phram code to include a kmalloc: diff --git a/drivers/mtd/devices/phram.c b/drivers/mtd/devices/phram.c index effd9a4..93b3cd9 100644 --- a/drivers/mtd/devices/phram.c +++ b/drivers/mtd/devices/phram.c @@ -271,6 +271,7 @@ static int phram_param_call(const char *val, struct kernel_param *kp) #ifdef MODULE return phram_setup(val); #else + char* fred = kmalloc(100, GFP_KERNEL); /* * If more parameters are later passed in via * /sys/module/phram/parameters/phram then when the kernel is booted(qemu in my case) using the following command: qemu-system-x86_64 -kernel arch/x86/boot/bzImage -append "console=ttyAMA0 console=ttyS0 phram.phram=alpha,114Mi,1Mi memmap=10M\$114M" -serial stdio the the kernel will not boot. If I remove the phram option so it doesn't go through the phram_param_call() and no kmalloc is called then the kernel boots fine. I couldn't think of a nicer solution than using a Kconfig option other than hard coding a value which I don't personally like, I am however happy to change it if there is a better solution I haven't thought of. > > To test phram built as a module insmod with the following arguments: > > > > phram=gamma,114Mi,1Mi phram=delta,115Mi,1Mi > > > > In this case two devices should be created. > > > > Signed-off-by: Rob Ward <robert.ward114@googlemail.com> > > --- > > drivers/mtd/devices/Kconfig | 12 ++++++++++++ > > drivers/mtd/devices/phram.c | 45 ++++++++++++++++++++++++++++++++++++++++----- > > 2 files changed, 52 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/mtd/devices/Kconfig b/drivers/mtd/devices/Kconfig > > index c49d0b1..5fdc80b 100644 > > --- a/drivers/mtd/devices/Kconfig > > +++ b/drivers/mtd/devices/Kconfig > > @@ -136,6 +136,18 @@ config MTD_PHRAM > > doesn't have access to, memory beyond the mem=xxx limit, nvram, > > memory on the video card, etc... > > > > +config MTD_PHRAM_MAX_CMDLINE_ARGS > > + int "Max number of devices via Kernel command line" > > + depends on MTD_PHRAM=y > > + default 1 > > + help > > + Specify the number of phram devices that can be initialised > > + using the Kernel command line. > > + > > + This option is only applicable when phram is built into the > > + Kernel. When built as a module many devices can be specified > > + at module insmod. > > + > > config MTD_LART > > tristate "28F160xx flash driver for LART" > > depends on SA1100_LART > > diff --git a/drivers/mtd/devices/phram.c b/drivers/mtd/devices/phram.c > > index effd9a4..223f221 100644 > > --- a/drivers/mtd/devices/phram.c > > +++ b/drivers/mtd/devices/phram.c > > @@ -212,9 +212,13 @@ static int phram_init_called; > > * - 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. > > + * > > + * The maximum number of devices supported is controlled by the > > + * MTD_PHRAM_MAX_CMDLINE_ARGS config option > > + * > > * Example: phram.phram=rootfs,0xa0000000,512Mi > > */ > > -static char phram_paramline[64 + 20 + 20]; > > +static char phram_paramline[CONFIG_MTD_PHRAM_MAX_CMDLINE_ARGS][64 + 20 + 20]; > > #endif > > > > static int phram_setup(const char *val) > > @@ -271,6 +275,9 @@ static int phram_param_call(const char *val, struct kernel_param *kp) > > #ifdef MODULE > > return phram_setup(val); > > #else > > + int paramline_it = 0; > > + int arraysize = 0; > > Neither of these need to be initialized here. And can you shorten the > long-ish names? We don't need an "iterator" -- it's just and index 'i'. > > > + > > /* > > * If more parameters are later passed in via > > * /sys/module/phram/parameters/phram > > @@ -290,9 +297,27 @@ static int phram_param_call(const char *val, struct kernel_param *kp) > > * phram_setup(). > > */ > > > > - if (strlen(val) >= sizeof(phram_paramline)) > > + if (strlen(val) >= sizeof(phram_paramline[0])) > > return -ENOSPC; > > - strcpy(phram_paramline, val); > > + > > + arraysize = sizeof(phram_paramline)/sizeof(phram_paramline[0]); > > Coccinelle complains: > > drivers/mtd/devices/phram.c:303:35-36: WARNING: Use ARRAY_SIZE > > > + > > + /* > > + * Check if any space is left in the array. If no space > > + * is left then print warning and return 0 > > + */ > > + > > + if (phram_paramline[arraysize - 1][0]) { > > + pr_warn("exceeded limit via cmd_line - %s ignored", val); > > + return 0; > > + } > > + > > + for (paramline_it = 0; paramline_it <= arraysize; paramline_it++) { > > Use '<' not '<='. You're overflowing the array. > > > + if (!phram_paramline[paramline_it][0]) { > > + strcpy(phram_paramline[paramline_it], val); > > + break; > > + } > > + } > > > > return 0; > > #endif > > @@ -307,8 +332,18 @@ static int __init init_phram(void) > > int ret = 0; > > > > #ifndef MODULE > > - if (phram_paramline[0]) > > - ret = phram_setup(phram_paramline); > > + int arraysize = 0; > > + int paramline_it = 0; > > Same comments as above. Neither of these need to be initialized here, > and shorten the name. > > > + > > + arraysize = sizeof(phram_paramline)/sizeof(phram_paramline[0]); > > drivers/mtd/devices/phram.c:338:35-36: WARNING: Use ARRAY_SIZE > > > + > > + for (paramline_it = 0; paramline_it <= arraysize; paramline_it++) { > > You're off-by-one; use '<', not '<='. So this line could just be: > > for (i = 0; i < ARRAY_SIZE(phram_paramline); i++) { > > (But then, we can avoid this by just allocating and appending to a > linked list.) > > > + if (phram_paramline[paramline_it][0]) { > > + ret = phram_setup(phram_paramline[paramline_it]); > > + if (ret) > > + break; > > + } > > + } > > phram_init_called = 1; > > #endif > > > > Brian > Cheers, Rob ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V3] mtd: phram: Allow multiple phram devices on cmd line 2014-11-09 16:24 ` Rob Ward 2014-11-26 6:32 ` Brian Norris @ 2014-12-03 7:27 ` Rob Ward 1 sibling, 0 replies; 6+ messages in thread From: Rob Ward @ 2014-12-03 7:27 UTC (permalink / raw) To: joern, dwmw2, computersforpeace; +Cc: linux-kernel, linux-mtd >From 900dbad52b2cc88b46c5716b1afd22698f6d83aa Mon Sep 17 00:00:00 2001 From: Rob Ward <robert.ward114@googlemail.com> Date: Tue, 21 Oct 2014 17:46:53 +0100 Subject: [PATCH] mtd: phram: Allow multiple phram devices on cmd line Allow the phram module the ability to create multiple phram mtd devices via the kernel command line. Currently the phram module only allows a single mtd device to be created via the kernel command line. This is due to the phram module having to store the values until it is initialised later. This storage is done using a single char[] meaning when the module_param_call is made the previous value is overidden. This change modifies the phram system to use a char[][] allowing multiple devices to be created. The array size if controlled using the new config option CONFIG_MTD_PHRAM_MAX_CMDLINE_ARGS that allows the maximum number of devices to be specified. Currently this option defaults to a value of 1 leaving the behaviour unchanged. If the array is full a message is printed to the console and the module_param_call returns. To test, in all cases an area of memory needs to be reserved via the command line e.g. memmap=10M$114M To test with phram build into the kernel on the command line add the following: phram.phram=alpha,114Mi,1Mi phram.phram=beta,115Mi,1Mi If CONFIG_MTD_PHRAM_MAX_CMDLINE_ARGS is left as default i.e. 1 then the first device, alpha will be created only. If the value of CONFIG_MTD_PHRAM_MAX_CMDLINE_ARGS is increased to 2 or more then both alpha and beta will be created. To test phram built as a module insmod with the following arguments: phram=gamma,114Mi,1Mi phram=delta,115Mi,1Mi In this case two devices should be created. Signed-off-by: Rob Ward <robert.ward114@googlemail.com> --- drivers/mtd/devices/Kconfig | 12 ++++++++++++ drivers/mtd/devices/phram.c | 39 ++++++++++++++++++++++++++++++++++----- 2 files changed, 46 insertions(+), 5 deletions(-) diff --git a/drivers/mtd/devices/Kconfig b/drivers/mtd/devices/Kconfig index c49d0b1..5fdc80b 100644 --- a/drivers/mtd/devices/Kconfig +++ b/drivers/mtd/devices/Kconfig @@ -136,6 +136,18 @@ config MTD_PHRAM doesn't have access to, memory beyond the mem=xxx limit, nvram, memory on the video card, etc... +config MTD_PHRAM_MAX_CMDLINE_ARGS + int "Max number of devices via Kernel command line" + depends on MTD_PHRAM=y + default 1 + help + Specify the number of phram devices that can be initialised + using the Kernel command line. + + This option is only applicable when phram is built into the + Kernel. When built as a module many devices can be specified + at module insmod. + config MTD_LART tristate "28F160xx flash driver for LART" depends on SA1100_LART diff --git a/drivers/mtd/devices/phram.c b/drivers/mtd/devices/phram.c index effd9a4..b42678e 100644 --- a/drivers/mtd/devices/phram.c +++ b/drivers/mtd/devices/phram.c @@ -212,9 +212,13 @@ static int phram_init_called; * - 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. + * + * The maximum number of devices supported is controlled by the + * MTD_PHRAM_MAX_CMDLINE_ARGS config option + * * Example: phram.phram=rootfs,0xa0000000,512Mi */ -static char phram_paramline[64 + 20 + 20]; +static char phram_paramline[CONFIG_MTD_PHRAM_MAX_CMDLINE_ARGS][64 + 20 + 20]; #endif static int phram_setup(const char *val) @@ -271,6 +275,8 @@ static int phram_param_call(const char *val, struct kernel_param *kp) #ifdef MODULE return phram_setup(val); #else + int i; + /* * If more parameters are later passed in via * /sys/module/phram/parameters/phram @@ -290,9 +296,25 @@ static int phram_param_call(const char *val, struct kernel_param *kp) * phram_setup(). */ - if (strlen(val) >= sizeof(phram_paramline)) + if (strlen(val) >= sizeof(phram_paramline[0])) return -ENOSPC; - strcpy(phram_paramline, val); + + /* + * Check if any space is left in the array. If no space + * is left then print warning and return 0 + */ + + if (phram_paramline[ARRAY_SIZE(phram_paramline) - 1][0]) { + pr_warn("exceeded limit via cmd_line - %s ignored", val); + return 0; + } + + for (i = 0; i < ARRAY_SIZE(phram_paramline); i++) { + if (!phram_paramline[i][0]) { + strcpy(phram_paramline[i], val); + break; + } + } return 0; #endif @@ -307,8 +329,15 @@ static int __init init_phram(void) int ret = 0; #ifndef MODULE - if (phram_paramline[0]) - ret = phram_setup(phram_paramline); + int i; + + for (i = 0; i < ARRAY_SIZE(phram_paramline); i++) { + if (phram_paramline[i][0]) { + ret = phram_setup(phram_paramline[i]); + if (ret) + break; + } + } phram_init_called = 1; #endif -- 2.0.2 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-12-03 7:27 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-10-21 18:03 [PATCH] mtd: phram: Allow multiple phram devices on cmd line Rob Ward 2014-10-21 18:25 ` Brian Norris 2014-11-09 16:24 ` Rob Ward 2014-11-26 6:32 ` Brian Norris 2014-11-26 8:54 ` Rob Ward 2014-12-03 7:27 ` [PATCH V3] " Rob Ward
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®