Make phram work with kernel cmdline args when built-in

Make phram work with kernel cmdline args when built-in

[Fache, Herve]

[-- Attachment #1.1: Type: text/plain, Size: 547 bytes --]

Hi guys,

Please find attached a patch that make it possible to use phram built-in
with a kernel command line argument.
This is basically the same patch that was applied in 2006 by Villa Herva to
block2mtd, which saves the argument for later, when kmalloc is available.
This allows us to put kernel and rootfs into physical RAM and boot without
any further copy, hence very fast.

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

[-- Attachment #1.2: Type: text/html, Size: 696 bytes --]

[-- Attachment #2: 0001-phram-make-kernel-boot-command-line-arguments-work.patch --]
[-- Type: application/octet-stream, Size: 3245 bytes --]

From 71f011fa326c494629b9d2fd4f9fdad8dd215d6d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Herv=C3=A9=20Fache?= <h-fache@ti.com>
Date: Tue, 4 Oct 2011 18:35:46 +0200
Subject: [PATCH] phram: make kernel boot command line arguments work
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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.

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 |   44 ++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/devices/phram.c b/drivers/mtd/devices/phram.c
index 23423bd..6b839f0 100644
--- a/drivers/mtd/devices/phram.c
+++ b/drivers/mtd/devices/phram.c
@@ -33,6 +33,10 @@ struct phram_mtd_list {
 
 static LIST_HEAD(phram_list);
 
+#ifndef MODULE
+static int phram_init_called;
+static __initdata char phram_paramline[64+12+12];
+#endif
 
 static int phram_erase(struct mtd_info *mtd, struct erase_info *instr)
 {
@@ -134,7 +138,7 @@ static int register_device(char *name, unsigned long start, unsigned long len)
 	ret = -EIO;
 	new->mtd.priv = ioremap(start, len);
 	if (!new->mtd.priv) {
-		pr_err("ioremap failed\n");
+		pr_err("ioremap failed for %ld@0x%lx\n", len, start);
 		goto out1;
 	}
 
@@ -233,7 +237,7 @@ 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_setup_late(const char *val)
 {
 	char buf[64+12+12], *str = buf;
 	char *token[3];
@@ -282,13 +286,47 @@ static int phram_setup(const char *val, struct kernel_param *kp)
 	return ret;
 }
 
+static int phram_setup(const char *val, struct kernel_param *kp)
+{
+#ifdef MODULE
+	return phram_setup_late(val);
+#else
+	/* 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_setup_late(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_setup_late(), called by init_phram(). */
+
+	strlcpy(phram_paramline, val, sizeof(phram_paramline));
+
+	return 0;
+#endif
+}
+
 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;
+
+#ifndef MODULE
+	if (strlen(phram_paramline))
+		ret = phram_setup_late(phram_paramline);
+	phram_init_called = 1;
+#endif
+
+	return ret;
 }
 
 static void __exit cleanup_phram(void)
-- 
1.7.0.4

Re: Make phram work with kernel cmdline args when built-in

[Jörn Engel]

On Tue, 11 October 2011 11:46:08 +0200, Fache, Herve wrote:
> 
> Please find attached a patch that make it possible to use phram built-in
> with a kernel command line argument.
> This is basically the same patch that was applied in 2006 by Villa Herva to
> block2mtd, which saves the argument for later, when kmalloc is available.
> This allows us to put kernel and rootfs into physical RAM and boot without
> any further copy, hence very fast.

Looks good in principle.

> 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.
> 
> 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 |   44 ++++++++++++++++++++++++++++++++++++++++--
>  1 files changed, 41 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/devices/phram.c b/drivers/mtd/devices/phram.c
> index 23423bd..6b839f0 100644
> --- a/drivers/mtd/devices/phram.c
> +++ b/drivers/mtd/devices/phram.c
> @@ -33,6 +33,10 @@ struct phram_mtd_list {
>  
>  static LIST_HEAD(phram_list);
>  
> +#ifndef MODULE
> +static int phram_init_called;
> +static __initdata char phram_paramline[64+12+12];
> +#endif

What I slightly dislike about this patch is the tiny differences
between Ville Herva's and this one.  In block2mtd those same lines are
added in a different place.  While I don't care much where to put
them, I see some value in having duplicated code stand out as
duplicated code and not have subtle variations all over the place.  So
my preference would be to either change this patch or alternatively
move the code in block2mtd as well.

>  static int phram_erase(struct mtd_info *mtd, struct erase_info *instr)
>  {
> @@ -134,7 +138,7 @@ static int register_device(char *name, unsigned long start, unsigned long len)
>  	ret = -EIO;
>  	new->mtd.priv = ioremap(start, len);
>  	if (!new->mtd.priv) {
> -		pr_err("ioremap failed\n");
> +		pr_err("ioremap failed for %ld@0x%lx\n", len, start);
>  		goto out1;
>  	}
>  
> @@ -233,7 +237,7 @@ 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_setup_late(const char *val)
>  {
>  	char buf[64+12+12], *str = buf;
>  	char *token[3];

Same here.  phram_setup_late may be a better name, but it is
inconsistent with block2mtd_setup2.

So while I have no preference for either of the two variants, I would
like to keep the two drivers as similar as possible.  Apart from that
minor thing, nice work!

Jörn

-- 
I don't understand it. Nobody does.
-- Richard P. Feynman

Re: Make phram work with kernel cmdline args when built-in

[Fache, Herve]

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

Hi again Jörn,

Please find attached a revised version of the patch.

On Wed, Oct 12, 2011 at 18:48, Jörn Engel <joern@logfs.org> wrote:
> Looks good in principle.

Thanks :-)

> What I slightly dislike about this patch is the tiny differences
> between Ville Herva's and this one.  In block2mtd those same lines are
> added in a different place.  While I don't care much where to put
> them, I see some value in having duplicated code stand out as
> duplicated code and not have subtle variations all over the place.  So
> my preference would be to either change this patch or alternatively
> move the code in block2mtd as well.

Thanks for your politeness. I have now moved things in the same place
as the original patch for consistency.

> Same here.  phram_setup_late may be a better name, but it is
> inconsistent with block2mtd_setup2.

Indeed, I have now renamed the function phram_setup2 in my patch.

> So while I have no preference for either of the two variants, I would
> like to keep the two drivers as similar as possible.  Apart from that
> minor thing, nice work!

I have updated my patch rather than block2mtd, so we minimize the changes.

Thanks a lot for your help.
Hervé

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

[-- Attachment #2: 0001-phram-make-kernel-boot-command-line-arguments-work.patch --]
[-- Type: text/x-diff, Size: 2790 bytes --]

From a42930c635d00afa9dab80476eae964213694455 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Herv=C3=A9=20Fache?= <h-fache@ti.com>
Date: Tue, 4 Oct 2011 18:35:46 +0200
Subject: [PATCH] phram: make kernel boot command line arguments work
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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.

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 |   43 +++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 41 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/devices/phram.c b/drivers/mtd/devices/phram.c
index 23423bd..93c7ebf 100644
--- a/drivers/mtd/devices/phram.c
+++ b/drivers/mtd/devices/phram.c
@@ -233,7 +233,12 @@ static inline void kill_final_newline(char *str)
 	return 1;		\
 } while (0)
 
-static int phram_setup(const char *val, struct kernel_param *kp)
+#ifndef MODULE
+static int phram_init_called;
+static __initdata char phram_paramline[64+12+12];
+#endif
+
+static int phram_setup2(const char *val)
 {
 	char buf[64+12+12], *str = buf;
 	char *token[3];
@@ -282,13 +287,47 @@ static int phram_setup(const char *val, struct kernel_param *kp)
 	return ret;
 }
 
+static int phram_setup(const char *val, struct kernel_param *kp)
+{
+#ifdef MODULE
+	return phram_setup2(val);
+#else
+	/* 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;
+#endif
+}
+
 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;
+
+#ifndef MODULE
+	if (strlen(phram_paramline))
+		ret = phram_setup2(phram_paramline);
+	phram_init_called = 1;
+#endif
+
+	return ret;
 }
 
 static void __exit cleanup_phram(void)
-- 
1.7.0.4

Re: Make phram work with kernel cmdline args when built-in

[Artem Bityutskiy]

On Thu, 2011-10-13 at 15:30 +0200, Fache, Herve wrote:
> Hi again Jörn,
> 
> Please find attached a revised version of the patch.

Please, send patches inline. Also, try to make sure you do not
have many of these #ifdef MODULE sections.

-- 
Best Regards,
Artem Bityutskiy

Re: Make phram work with kernel cmdline args when built-in

[Fache, Herve]

Hi Artem,

2011/10/14 Artem Bityutskiy <dedekind1@gmail.com>
> Please, send patches inline. Also, try to make sure you do not
> have many of these #ifdef MODULE sections.

I am sorry but AIUI my company's e-mail system does not seem to allow
me to set patches the proper way. Is there any chance that you can
accept it inline for this time? Or can I have an account on
infradead.org, in which case I'll just push a git tree?

Also, I have tried to make my patch look like the other patch to
block2mtd, as asked by Jörn, which explains the number of #ifdef
MODULE sections...

Regards,
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: Make phram work with kernel cmdline args when built-in

[Artem Bityutskiy]

On Fri, 2011-10-14 at 15:44 +0200, Fache, Herve wrote:
> Hi Artem,
> 
> 2011/10/14 Artem Bityutskiy <dedekind1@gmail.com>
> > Please, send patches inline. Also, try to make sure you do not
> > have many of these #ifdef MODULE sections.
> 
> I am sorry but AIUI my company's e-mail system does not seem to allow
> me to set patches the proper way. Is there any chance that you can
> accept it inline for this time? Or can I have an account on
> infradead.org, in which case I'll just push a git tree?

Not sure about infradead.org, you should ask David Woodhouse, not me. 
Just create gmail.com account and use ssh tunneling via your home
machine or some other host, or use your corporate proxy if it allows
smtp/imap traffic. Note, infradead.org account is not easier to used
than gmail.com.

-- 
Best Regards,
Artem Bityutskiy

Re: Make phram work with kernel cmdline args when built-in

[Jörn Engel]

On Fri, 14 October 2011 15:42:09 +0200, Fache, Herve wrote:
> 2011/10/14 Artem Bityutskiy <dedekind1@gmail.com>
> 
> > Please, send patches inline. Also, try to make sure you do not
> > have many of these #ifdef MODULE sections.
> 
> Also, I have tried to make my patch look like the other patch to block2mtd,
> as asked by Jörn, which explains the number of #ifdef MODULE sections...

If there were a way to get rid of the #ifdefs, I would certainly be in
favor of that.  While I am happy with the functional improvements, I
would be even happier if the code looked less scarred.

I wonder how everyone else does it.  Those two driver surely can't be
the only ones that depend on module parameters for initialization.

Jörn

-- 
Fools ignore complexity.  Pragmatists suffer it.
Some can avoid it.  Geniuses remove it.
-- Perlis's Programming Proverb #58, SIGPLAN Notices, Sept.  1982

Re: Make phram work with kernel cmdline args when built-in

[Fache, Herve]

On Fri, Oct 14, 2011 at 16:38, Jörn Engel <joern@logfs.org> wrote:
> If there were a way to get rid of the #ifdefs, I would certainly be in
> favor of that.  While I am happy with the functional improvements, I
> would be even happier if the code looked less scarred.
>
> I wonder how everyone else does it.  Those two driver surely can't be
> the only ones that depend on module parameters for initialization.

Looks like it's just a matter of properly using 'phram_init_called'.
Shall I submit a new version of the patch that does that?

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: Make phram work with kernel cmdline args when built-in

[Jörn Engel]

On Fri, 14 October 2011 16:48:45 +0200, Fache, Herve wrote:
> 
> Looks like it's just a matter of properly using 'phram_init_called'.
> Shall I submit a new version of the patch that does that?

Since my brain cannot quite translate that into code yet: yes please.

Jörn

-- 
Unless something dramatically changes, by 2015 we'll be largely
wondering what all the fuss surrounding Linux was really about.
-- Rob Enderle