| Message ID | 525C2147.9060605@nsn.com |
|---|---|
| State | New, archived |
| Headers | show |
Hi Alexander, On Mon, Oct 14, 2013 at 06:52:23PM +0200, Alexander Sverdlin wrote: > mtd: phram: Repair multiple instances support > > Commit b2a2a84d35e0f42ad26e326ec4258f6a8b8eecbe (mtd: phram: dot not crash when > built-in and passing boot param) claims to be "based on Ville Herva's similar > patch to block2mtd" (c4e7fb313771ac03dfdca26d30e8b721731c562b), but it has > missed the crucial point of the original path: all these "if(n)def MODULE". > It has broken the possibility to create several phram instances when phram is > compiled as module. The possibility to add instances via /sys writes to > /sys/module/phram/parameters/phram was also broken with mentioned patch. > Proposed patch takes the idea of original block2mtd patch to its full extent. > Assumtion "This function is always called before 'init_phram()'" was also > incorrect, so removed the comment. This patch effectively reverts also > b11ec57fc6e6d4882ef01a0c09a1dde58f50492e (mtd: phram: fix section mismatch for > phram_setup). > > v2 changes: Fixed error handling in init_phram(). > > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nsn.com> Can we get any testers? Perhaps Hervé can confirm this? > --- > --- linux.orig/drivers/mtd/devices/phram.c > +++ linux/drivers/mtd/devices/phram.c > @@ -205,6 +205,8 @@ static inline void kill_final_newline(ch > return 1; \ > } while (0) > > +#ifndef MODULE > +static int phram_init_called = 0; > /* > * This shall contain the module parameter if any. It is of the form: > * - phram=<device>,<address>,<size> for module case > @@ -213,9 +215,10 @@ static inline void kill_final_newline(ch > * size. > * Example: phram.phram=rootfs,0xa0000000,512Mi > */ > -static __initdata char phram_paramline[64 + 20 + 20]; > +static char phram_paramline[64 + 20 + 20]; > +#endif > > -static int __init phram_setup(const char *val) > +static int phram_setup(const char *val) > { > char buf[64 + 20 + 20], *str = buf; > char *token[3]; > @@ -264,17 +267,36 @@ static int __init phram_setup(const char > return ret; > } > > -static int __init phram_param_call(const char *val, struct kernel_param *kp) > +static int phram_param_call(const char *val, struct kernel_param *kp) > { > +#ifdef MODULE > + return phram_setup(val); > +#else > /* > - * This function is always called before 'init_phram()', whether > - * built-in or module. > + * 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 now. > */ > + > + if (phram_init_called) > + return phram_setup(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_param_call() is > + * called so early that it is not possible to resolve > + * the device (even kmalloc() fails). Defer that work to > + * phram_setup(). > + */ > + > if (strlen(val) >= sizeof(phram_paramline)) > return -ENOSPC; > strcpy(phram_paramline, val); > > return 0; > +#endif > } > > module_param_call(phram, phram_param_call, NULL, NULL, 000); > @@ -283,10 +305,15 @@ MODULE_PARM_DESC(phram, "Memory region t > > static int __init init_phram(void) > { > + int ret = 0; > + > +#ifndef MODULE > if (phram_paramline[0]) > - return phram_setup(phram_paramline); > + ret = phram_setup(phram_paramline); > + phram_init_called = 1; > +#endif > > - return 0; > + return ret; > } > > static void __exit cleanup_phram(void) Brian
Hello Brian, On 07/11/13 08:31, ext Brian Norris wrote: > On Mon, Oct 14, 2013 at 06:52:23PM +0200, Alexander Sverdlin wrote: >> mtd: phram: Repair multiple instances support >> >> Commit b2a2a84d35e0f42ad26e326ec4258f6a8b8eecbe (mtd: phram: dot not crash when >> built-in and passing boot param) claims to be "based on Ville Herva's similar >> patch to block2mtd" (c4e7fb313771ac03dfdca26d30e8b721731c562b), but it has >> missed the crucial point of the original path: all these "if(n)def MODULE". >> It has broken the possibility to create several phram instances when phram is >> compiled as module. The possibility to add instances via /sys writes to >> /sys/module/phram/parameters/phram was also broken with mentioned patch. >> Proposed patch takes the idea of original block2mtd patch to its full extent. >> Assumtion "This function is always called before 'init_phram()'" was also >> incorrect, so removed the comment. This patch effectively reverts also >> b11ec57fc6e6d4882ef01a0c09a1dde58f50492e (mtd: phram: fix section mismatch for >> phram_setup). >> >> v2 changes: Fixed error handling in init_phram(). >> >> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nsn.com> > > Can we get any testers? Perhaps Hervé can confirm this? Have addressed Ville Herva, Ryosuke Saito, Hervé Fache on 07.11.2013, no answer until now. Seems nobody interested in it any more. Any chances we still get this in? >> --- >> --- linux.orig/drivers/mtd/devices/phram.c >> +++ linux/drivers/mtd/devices/phram.c >> @@ -205,6 +205,8 @@ static inline void kill_final_newline(ch >> return 1; \ >> } while (0) >> >> +#ifndef MODULE >> +static int phram_init_called = 0; >> /* >> * This shall contain the module parameter if any. It is of the form: >> * - phram=<device>,<address>,<size> for module case >> @@ -213,9 +215,10 @@ static inline void kill_final_newline(ch >> * size. >> * Example: phram.phram=rootfs,0xa0000000,512Mi >> */ >> -static __initdata char phram_paramline[64 + 20 + 20]; >> +static char phram_paramline[64 + 20 + 20]; >> +#endif >> >> -static int __init phram_setup(const char *val) >> +static int phram_setup(const char *val) >> { >> char buf[64 + 20 + 20], *str = buf; >> char *token[3]; >> @@ -264,17 +267,36 @@ static int __init phram_setup(const char >> return ret; >> } >> >> -static int __init phram_param_call(const char *val, struct kernel_param *kp) >> +static int phram_param_call(const char *val, struct kernel_param *kp) >> { >> +#ifdef MODULE >> + return phram_setup(val); >> +#else >> /* >> - * This function is always called before 'init_phram()', whether >> - * built-in or module. >> + * 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 now. >> */ >> + >> + if (phram_init_called) >> + return phram_setup(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_param_call() is >> + * called so early that it is not possible to resolve >> + * the device (even kmalloc() fails). Defer that work to >> + * phram_setup(). >> + */ >> + >> if (strlen(val) >= sizeof(phram_paramline)) >> return -ENOSPC; >> strcpy(phram_paramline, val); >> >> return 0; >> +#endif >> } >> >> module_param_call(phram, phram_param_call, NULL, NULL, 000); >> @@ -283,10 +305,15 @@ MODULE_PARM_DESC(phram, "Memory region t >> >> static int __init init_phram(void) >> { >> + int ret = 0; >> + >> +#ifndef MODULE >> if (phram_paramline[0]) >> - return phram_setup(phram_paramline); >> + ret = phram_setup(phram_paramline); >> + phram_init_called = 1; >> +#endif >> >> - return 0; >> + return ret; >> } >> >> static void __exit cleanup_phram(void) > > Brian > >
On Tue, Dec 17, 2013 at 08:52:49AM +0100, Alexander Sverdlin wrote: > On 07/11/13 08:31, ext Brian Norris wrote: > > On Mon, Oct 14, 2013 at 06:52:23PM +0200, Alexander Sverdlin wrote: > >> mtd: phram: Repair multiple instances support > >> > >> Commit b2a2a84d35e0f42ad26e326ec4258f6a8b8eecbe (mtd: phram: dot not crash when > >> built-in and passing boot param) claims to be "based on Ville Herva's similar > >> patch to block2mtd" (c4e7fb313771ac03dfdca26d30e8b721731c562b), but it has > >> missed the crucial point of the original path: all these "if(n)def MODULE". > >> It has broken the possibility to create several phram instances when phram is > >> compiled as module. The possibility to add instances via /sys writes to > >> /sys/module/phram/parameters/phram was also broken with mentioned patch. > >> Proposed patch takes the idea of original block2mtd patch to its full extent. > >> Assumtion "This function is always called before 'init_phram()'" was also > >> incorrect, so removed the comment. This patch effectively reverts also > >> b11ec57fc6e6d4882ef01a0c09a1dde58f50492e (mtd: phram: fix section mismatch for > >> phram_setup). > >> > >> v2 changes: Fixed error handling in init_phram(). > >> > >> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nsn.com> > > > > Can we get any testers? Perhaps Hervé can confirm this? > > Have addressed Ville Herva, Ryosuke Saito, Hervé Fache on 07.11.2013, no answer > until now. Seems nobody interested in it any more. Any chances we still get this in? Yeah, looks ok. Pushed to l2-mtd.git/next, for 3.15. Brian
--- linux.orig/drivers/mtd/devices/phram.c +++ linux/drivers/mtd/devices/phram.c @@ -205,6 +205,8 @@ static inline void kill_final_newline(ch return 1; \ } while (0) +#ifndef MODULE +static int phram_init_called = 0; /* * This shall contain the module parameter if any. It is of the form: * - phram=<device>,<address>,<size> for module case @@ -213,9 +215,10 @@ static inline void kill_final_newline(ch * size. * Example: phram.phram=rootfs,0xa0000000,512Mi */ -static __initdata char phram_paramline[64 + 20 + 20]; +static char phram_paramline[64 + 20 + 20]; +#endif -static int __init phram_setup(const char *val) +static int phram_setup(const char *val) { char buf[64 + 20 + 20], *str = buf; char *token[3]; @@ -264,17 +267,36 @@ static int __init phram_setup(const char return ret; } -static int __init phram_param_call(const char *val, struct kernel_param *kp) +static int phram_param_call(const char *val, struct kernel_param *kp) { +#ifdef MODULE + return phram_setup(val); +#else /* - * This function is always called before 'init_phram()', whether - * built-in or module. + * 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 now. */ + + if (phram_init_called) + return phram_setup(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_param_call() is + * called so early that it is not possible to resolve + * the device (even kmalloc() fails). Defer that work to + * phram_setup(). + */ + if (strlen(val) >= sizeof(phram_paramline)) return -ENOSPC; strcpy(phram_paramline, val); return 0; +#endif } module_param_call(phram, phram_param_call, NULL, NULL, 000); @@ -283,10 +305,15 @@ MODULE_PARM_DESC(phram, "Memory region t static int __init init_phram(void) { + int ret = 0; + +#ifndef MODULE if (phram_paramline[0]) - return phram_setup(phram_paramline); + ret = phram_setup(phram_paramline); + phram_init_called = 1; +#endif - return 0; + return ret; } static void __exit cleanup_phram(void)
mtd: phram: Repair multiple instances support Commit b2a2a84d35e0f42ad26e326ec4258f6a8b8eecbe (mtd: phram: dot not crash when built-in and passing boot param) claims to be "based on Ville Herva's similar patch to block2mtd" (c4e7fb313771ac03dfdca26d30e8b721731c562b), but it has missed the crucial point of the original path: all these "if(n)def MODULE". It has broken the possibility to create several phram instances when phram is compiled as module. The possibility to add instances via /sys writes to /sys/module/phram/parameters/phram was also broken with mentioned patch. Proposed patch takes the idea of original block2mtd patch to its full extent. Assumtion "This function is always called before 'init_phram()'" was also incorrect, so removed the comment. This patch effectively reverts also b11ec57fc6e6d4882ef01a0c09a1dde58f50492e (mtd: phram: fix section mismatch for phram_setup). v2 changes: Fixed error handling in init_phram(). Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nsn.com> ---