[PATCH] phram: make kernel boot command line arguments work

[PATCH] phram: make kernel boot command line arguments work

[h-fache]

From: Hervé Fache <h-fache@ti.com>

This patch is based on Ville Herva's similar patch to block2mtd.

Trying to pass kernel command line arguments at boot-time did not work, as
phram_setup() was called so early that kmalloc() was not functional.

This patch only saves the option string at the early boot stage, and parses
it later when init_phram() is called. This is determined by the fact that
init was called, or not.

With this patch, I can boot with a statically-compiled phram, and mount a
ext2 root fs from physical RAM, without the need for a initrd.

Signed-off-by: Hervé Fache <h-fache@ti.com>
---
 drivers/mtd/devices/phram.c |   35 +++++++++++++++++++++++++++++++++--
 1 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/devices/phram.c b/drivers/mtd/devices/phram.c
index 23423bd..6d58bf0 100644
--- a/drivers/mtd/devices/phram.c
+++ b/drivers/mtd/devices/phram.c
@@ -233,7 +233,10 @@ static inline void kill_final_newline(char *str)
 	return 1;		\
 } while (0)
 
-static int phram_setup(const char *val, struct kernel_param *kp)
+static int phram_init_called;
+static __initdata char phram_paramline[64+12+12];
+
+static int phram_setup2(const char *val)
 {
 	char buf[64+12+12], *str = buf;
 	char *token[3];
@@ -282,13 +285,41 @@ static int phram_setup(const char *val, struct kernel_param *kp)
 	return ret;
 }
 
+static int phram_setup(const char *val, struct kernel_param *kp)
+{
+	/* If more parameters are later passed in via
+	   /sys/module/phram/parameters/phram
+	   and init_phram() has already been called,
+	   we can parse the argument directly. */
+
+	if (phram_init_called)
+		return phram_setup2(val);
+
+	/* During early boot stage, we only save the parameters
+	   here. We must parse them later: if the param passed
+	   from kernel boot command line, phram_setup() is
+	   called so early that it is not possible to resolve
+	   the device (kmalloc() fails). Defer that work to
+	   phram_setup2(), called by init_phram(). */
+
+	strlcpy(phram_paramline, val, sizeof(phram_paramline));
+
+	return 0;
+}
+
 module_param_call(phram, phram_setup, NULL, NULL, 000);
 MODULE_PARM_DESC(phram, "Memory region to map. \"phram=<name>,<start>,<length>\"");
 
 
 static int __init init_phram(void)
 {
-	return 0;
+	int ret = 0;
+
+	if (strlen(phram_paramline))
+		ret = phram_setup2(phram_paramline);
+	phram_init_called = 1;
+
+	return ret;
 }
 
 static void __exit cleanup_phram(void)
-- 
1.7.5.4

Re: [PATCH] phram: make kernel boot command line arguments work

[Jörn Engel]

On Mon, 17 October 2011 17:40:38 +0200, h-fache@ti.com wrote:
> From: Hervé Fache <h-fache@ti.com>
> 
> This patch is based on Ville Herva's similar patch to block2mtd.
> 
> Trying to pass kernel command line arguments at boot-time did not work, as
> phram_setup() was called so early that kmalloc() was not functional.
> 
> This patch only saves the option string at the early boot stage, and parses
> it later when init_phram() is called. This is determined by the fact that
> init was called, or not.
> 
> With this patch, I can boot with a statically-compiled phram, and mount a
> ext2 root fs from physical RAM, without the need for a initrd.
> 
> Signed-off-by: Hervé Fache <h-fache@ti.com>

I like it.
Acked-by: Joern Engel <joern@logfs.org>

Dedekind, can I leave it to you to merge this?  And Hervé, can I
motivate you to remove the #ifdef MODULE parts from block2mtd as well?
If someone complains about increased binary size, I am willing to look
for ten bytes to remove elsewhere as a trade-off.

Jörn

-- 
Computer system analysis is like child-rearing; you can do grievous damage,
but you cannot ensure success."
-- Tom DeMarco

Re: [PATCH] phram: make kernel boot command line arguments work

[Fache, Herve]

Hi Jörn,

On Tue, Oct 18, 2011 at 02:32, Jörn Engel <joern@logfs.org> wrote:
> I like it.
Thanks :-)

> And Hervé, can I motivate you to remove the #ifdef MODULE parts from block2mtd as well?
Will do, no problem.

Hervé
--
Texas Instruments France SA, 821 Avenue Jack Kilby, 06270 Villeneuve
Loubet. 036 420 040 R.C.S Antibes. Capital de EUR 753.920

Re: [PATCH] phram: make kernel boot command line arguments work

[Artem Bityutskiy]

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

On Mon, 2011-10-17 at 17:40 +0200, h-fache@ti.com wrote:
> From: Hervé Fache <h-fache@ti.com>
> 
> This patch is based on Ville Herva's similar patch to block2mtd.
> 
> Trying to pass kernel command line arguments at boot-time did not work, as
> phram_setup() was called so early that kmalloc() was not functional.
> 
> This patch only saves the option string at the early boot stage, and parses
> it later when init_phram() is called. This is determined by the fact that
> init was called, or not.
> 
> With this patch, I can boot with a statically-compiled phram, and mount a
> ext2 root fs from physical RAM, without the need for a initrd.
> 
> Signed-off-by: Hervé Fache <h-fache@ti.com>

Could you please make checkpatch.pl happy?

-- 
Best Regards,
Artem Bityutskiy

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

Re: [PATCH] phram: make kernel boot command line arguments work

[Artem Bityutskiy]

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

On Mon, 2011-10-17 at 17:40 +0200, h-fache@ti.com wrote:

> -static int phram_setup(const char *val, struct kernel_param *kp)
> +static int phram_init_called;
> +static __initdata char phram_paramline[64+12+12];

snip..
> +static int phram_setup(const char *val, struct kernel_param *kp)
> +{

snip..

> +	strlcpy(phram_paramline, val, sizeof(phram_paramline));

Accessing __initdata from non __init function. Please check that your
patch does not introduce a new section mismatch. There is even a kernel
config option to debug these issues.

-- 
Best Regards,
Artem Bityutskiy

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

Re: [PATCH] phram: make kernel boot command line arguments work

[Artem Bityutskiy]

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

On Tue, 2011-10-18 at 02:32 +0200, Jörn Engel wrote:
> Dedekind, can I leave it to you to merge this?

I am too stupid comparing to Dedekind :-) The nickname is actually
because I liked his theorems back at University days, so my nick name is
after him :-)

But sure, as soon as I get a nice patch, I'll merge it to my l2 tree,
just like I do for every nice MTD patch :-)

-- 
Best Regards,
Artem Bityutskiy

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

Re: [PATCH] phram: make kernel boot command line arguments work

[Fache, Herve]

Hi Artem,

On Thu, Oct 20, 2011 at 18:20, Artem Bityutskiy <dedekind1@gmail.com> wrote:
>
> On Mon, 2011-10-17 at 17:40 +0200, h-fache@ti.com wrote:
>
> > -static int phram_setup(const char *val, struct kernel_param *kp)
> > +static int phram_init_called;
> > +static __initdata char phram_paramline[64+12+12];
>
> snip..
> > +static int phram_setup(const char *val, struct kernel_param *kp)
> > +{
>
> snip..
>
> > +     strlcpy(phram_paramline, val, sizeof(phram_paramline));
>
> Accessing __initdata from non __init function. Please check that your
> patch does not introduce a new section mismatch. There is even a kernel
> config option to debug these issues.

I enabled DEBUG_SECTION_MISMATCH but I don't get errors. Also, this
part of the code will never be reached once init has run. If you think
that it's a risk, then I can remove the __initdata keyword.

Hervé
--
Texas Instruments France SA, 821 Avenue Jack Kilby, 06270 Villeneuve
Loubet. 036 420 040 R.C.S Antibes. Capital de EUR 753.920

Re: [PATCH] phram: make kernel boot command line arguments work

[Fache, Herve]

Hi again,

On Thu, Oct 20, 2011 at 18:16, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> Could you please make checkpatch.pl happy?

Would you mind posting the errors/warnings you get? I did run
checkpatch.pl before submitting and got none...

Hervé
-- 
Texas Instruments France SA, 821 Avenue Jack Kilby, 06270 Villeneuve
Loubet. 036 420 040 R.C.S Antibes. Capital de EUR 753.920

Re: [PATCH] phram: make kernel boot command line arguments work

[Artem Bityutskiy]

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

On Fri, 2011-10-21 at 10:49 +0200, Fache, Herve wrote:
> > Accessing __initdata from non __init function. Please check that your
> > patch does not introduce a new section mismatch. There is even a kernel
> > config option to debug these issues.
> 
> I enabled DEBUG_SECTION_MISMATCH but I don't get errors. Also, this
> part of the code will never be reached once init has run. If you think
> that it's a risk, then I can remove the __initdata keyword.

Did you test this for the case when it is compiled into the kernel as
well, not only as kernel module?

-- 
Best Regards,
Artem Bityutskiy

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

Re: [PATCH] phram: make kernel boot command line arguments work

[Artem Bityutskiy]

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

On Fri, 2011-10-21 at 10:51 +0200, Fache, Herve wrote:
> Hi again,
> 
> On Thu, Oct 20, 2011 at 18:16, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> > Could you please make checkpatch.pl happy?
> 
> Would you mind posting the errors/warnings you get? I did run
> checkpatch.pl before submitting and got none...

Yeah, I thought checkpatch.pl would blame you for improper comments
style. The standard way to write comments is described in
Documentation/CodingStyle.

-- 
Best Regards,
Artem Bityutskiy

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

Re: [PATCH] phram: make kernel boot command line arguments work

[Fache, Herve]

Hi Artem,

On Tue, Oct 25, 2011 at 09:32, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> Did you test this for the case when it is compiled into the kernel as
> well, not only as kernel module?

Yes indeed, both cases have been tested.

Hervé
--
Texas Instruments France SA, 821 Avenue Jack Kilby, 06270 Villeneuve
Loubet. 036 420 040 R.C.S Antibes. Capital de EUR 753.920

Re: [PATCH] phram: make kernel boot command line arguments work

[Fache, Herve]

On Tue, Oct 25, 2011 at 09:37, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> Yeah, I thought checkpatch.pl would blame you for improper comments
> style. The standard way to write comments is described in
> Documentation/CodingStyle.

I have to admit I just re-used the original patch's comments. Do they
need to be changed prior to merging?

Hervé
-- 
Texas Instruments France SA, 821 Avenue Jack Kilby, 06270 Villeneuve
Loubet. 036 420 040 R.C.S Antibes. Capital de EUR 753.920

Re: [PATCH] phram: make kernel boot command line arguments work

[Jörn Engel]

On Tue, 25 October 2011 11:09:02 +0200, Fache, Herve wrote:
> On Tue, Oct 25, 2011 at 09:37, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> > Yeah, I thought checkpatch.pl would blame you for improper comments
> > style. The standard way to write comments is described in
> > Documentation/CodingStyle.
> 
> I have to admit I just re-used the original patch's comments. Do they
> need to be changed prior to merging?

Nah!  Your patch is a clear improvement.  If the worst offence of your
patch is that you moved code around that had an undesired (but still
quite common) comment style, you are in good shape. ;)

Jörn

-- 
Everything should be made as simple as possible, but not simpler.
-- Albert Einstein

Re: [PATCH] phram: make kernel boot command line arguments work

[Artem Bityutskiy]

On Mon, 2011-10-17 at 17:40 +0200, h-fache@ti.com wrote:
> diff --git a/drivers/mtd/devices/phram.c b/drivers/mtd/devices/phram.c
> index 23423bd..6d58bf0 100644
> --- a/drivers/mtd/devices/phram.c
> +++ b/drivers/mtd/devices/phram.c
> @@ -233,7 +233,10 @@ static inline void kill_final_newline(char *str)
>  	return 1;		\
>  } while (0)
>  
> -static int phram_setup(const char *val, struct kernel_param *kp)
> +static int phram_init_called;

You should not need this variable. I think it should work this way:

1. You declare the phram_setup param_call.
2. This function will be called before 'init_phram()' in both cases -
   module and compiled-in.
3. pram_setup should parse the parameters and save them in a temporary
   data structure marked as __initdata. In your case it is
   'phram_paramline'.
4. The 'init_phram()' parses that temporary data structure for real.

> +static __initdata char phram_paramline[64+12+12];
Please, add comment about 64 and 12.

> +
> +static int phram_setup2(const char *val)
>  {
>  	char buf[64+12+12], *str = buf;
>  	char *token[3];
> @@ -282,13 +285,41 @@ static int phram_setup(const char *val, struct kernel_param *kp)
>  	return ret;
>  }
>  
> +static int phram_setup(const char *val, struct kernel_param *kp)
> +{
> +	/* If more parameters are later passed in via
> +	   /sys/module/phram/parameters/phram
> +	   and init_phram() has already been called,
> +	   we can parse the argument directly. */
> +
> +	if (phram_init_called)
> +		return phram_setup2(val);
This if should not be needed - I think this function is called before
'init_phram()' always. So this check should be always false.
> +
> +	/* During early boot stage, we only save the parameters
> +	   here. We must parse them later: if the param passed
> +	   from kernel boot command line, phram_setup() is
> +	   called so early that it is not possible to resolve
> +	   the device (kmalloc() fails). Defer that work to
> +	   phram_setup2(), called by init_phram(). */
> +
> +	strlcpy(phram_paramline, val, sizeof(phram_paramline));

No please, do not silently truncate possibly longer command line. Do
proper strlen here and if it is longer than you array - return an error.

> +
> +	return 0;
> +}
> +
>  module_param_call(phram, phram_setup, NULL, NULL, 000);
>  MODULE_PARM_DESC(phram, "Memory region to map. \"phram=<name>,<start>,<length>\"");
>  
> 
>  static int __init init_phram(void)
>  {
> -	return 0;
> +	int ret = 0;
> +
> +	if (strlen(phram_paramline))
> +		ret = phram_setup2(phram_paramline);

Well, matter of taste, but I think strlen is too much for this, I'd use
if (*phram_paramline) or if (phram_paramline[0]) ...


> +	phram_init_called = 1;
> +
> +	return ret;
>  }
>  
>  static void __exit cleanup_phram(void)

Re: [PATCH] phram: make kernel boot command line arguments work

[Artem Bityutskiy]

On Tue, 2011-10-25 at 11:09 +0200, Fache, Herve wrote:
> On Tue, Oct 25, 2011 at 09:37, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> > Yeah, I thought checkpatch.pl would blame you for improper comments
> > style. The standard way to write comments is described in
> > Documentation/CodingStyle.
> 
> I have to admit I just re-used the original patch's comments. Do they
> need to be changed prior to merging?

No, that's fine, thanks!

Artem.