| Message ID | 1318866038-11781-1-git-send-email-h-fache@ti.com |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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?
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.
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 :-)
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
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é
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?
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.
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
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é
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
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)
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.
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)