Re: [PATCH] [MTD] block2mtd.c: Make kernel boot command line arguments work

Jörn Engel
2006-07-06T14:14:44Z

On Thu, 6 July 2006 11:24:44 +0300, Ville Herva wrote:
>
> Trying to pass kernel command line arguments to block2mtd at boot-time does
> not work currently. block2mtd_setup() is called so early that kmalloc()
> fails nevermind being able to do open_bdev_excl() (which requires rootfs to
> be mounted. This patch only saves the option string at the early boot stage,
> and parses them later when block2mtd_init() is called. If open_bdev_excl()
> fails, open_by_devnum(name_to_dev_t()) is tried instead, which makes it
> possible to initialize the driver before rootfs has been mounted. Also gets
> rid of the superfluous parse_name() that only checks if name is longer than
> 80 chars and copies it to a string that is not kfreed.
>
> With this patch, I can boot statically compiled block2mtd, and mount jffs2
> as rootfs (without modules or initrd), with lilo config like this:
>
> root=/dev/mtdblock0
> append="rootfstype=jffs2 block2mtd.block2mtd=/dev/hdc2,65536"
>
> (Note that rootfstype=jffs2 is required, since the kernel only tries
> filesystems without "nodev" attribute by default, and jffs is "nodev").

Looks fairly good as a basis. A couple of details still remain...

> @@ -426,14 +420,20 @@ static inline void kill_final_newline(ch
> return 0; \
> } while (0)
>
> -static int block2mtd_setup(const char *val, struct kernel_param *kp)
> +static int block2mtd_init_called = 0;
> +static char block2mtd_paramline[80 + 12]; /* 80 for device, 12 for erase size */
> +
> +
> +static int block2mtd_setup2(void)
> {
> - char buf[80+12]; /* 80 for device, 12 for erase size */
> + char* val = (char*)block2mtd_paramline;

The * usually belongs to the var, not the type. Also, I believe we
don't need the cast here.
char *val = block2mtd_paramline;
> +
> + char buf[80 + 12], *str = buf; /* 80 for device, 12 for erase size */
> char *str = buf;

Now we have two variables called "str".

> if (token[1]) {
> - ret = parse_num(&erase_size, token[1]);
> + int ret = parse_num(&erase_size, token[1]);

Maybe the variable declaration can stay at the top of the function,
just to reduce the amount of churn.

> +static int block2mtd_setup(const char *val, struct kernel_param *kp)
> +{
> + /* Only save the parameters here. We must parse them later:
> + if the param passed from kernel boot command line,
> + block2mtd_setup() is called so early that it is not
> + possible to resolve the device (even kmalloc() fails).
> + Deter that work to block2mtd_setup2(). */
> +
> + strlcpy(block2mtd_paramline, val, sizeof(block2mtd_paramline));
> +
> + /* If more parameters are later passed in via
> + /sys/module/block2mtd/parameters/block2mtd
> + and block2mtd_init() has already been called,
> + we can parse the argument now. */
> +
> + if (block2mtd_init_called)
> + return block2mtd_setup2();

Took me a couple of minutes to understand what you're doing here.
Makes sense. But passing the parameter to block2mtd_setup2() through
a global variable instead of stack allows races when two people add
device through the /sys/... file. We could add spinlocks around this
function and block2mtd_init() to solve that. Or we could decide it's
not worth the effort.

If you can address the first two comments, I'm happy.

Jörn

--
Do not stop an army on its way home.
-- Sun Tzu