Skip to contentSkip to conversation

block2mtd.c: Make kernel boot command line arguments work

PATCH In review Ville Herva 2 participants 3 messages Jul 6, 2006

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 block2mtd.c: Make kernel boot command line arguments work

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").

From: Ville Herva <[email protected]>

drivers/mtd/devices/block2mtd.c +55 -32
@@ -18,6 +18,7 @@
 #include <linux/mtd/mtd.h>
 #include <linux/buffer_head.h>
 #include <linux/mutex.h>
+#include <linux/mount.h>
 
 #define VERSION "$Revision: 1.30 $"
 
@@ -236,6 +237,8 @@ static int _block2mtd_write(struct block
 	}
 	return 0;
 }
+
+
 static int block2mtd_write(struct mtd_info *mtd, loff_t to, size_t len,
 		size_t *retlen, const u_char *buf)
 {
@@ -300,6 +303,17 @@ static struct block2mtd_dev *add_device(
 	/* Get a handle on the device */
 	bdev = open_bdev_excl(devname, O_RDWR, NULL);
 	if (IS_ERR(bdev)) {
+
+		/* We might not have rootfs mounted at this point. Try 
+		   to resolve the device name by other means. */
+
+		dev_t dev = name_to_dev_t(devname);
+		if (dev != 0) {
+			bdev = open_by_devnum(dev, FMODE_WRITE | FMODE_READ);
+		}
+	}
+
+	if (IS_ERR(bdev)) {
 		ERROR("error: cannot open device %s", devname);
 		goto devinit_err;
 	}
@@ -393,26 +407,6 @@ static int parse_num(size_t *num, const 
 }
 
 
-static int parse_name(char **pname, const char *token, size_t limit)
-{
-	size_t len;
-	char *name;
-
-	len = strlen(token) + 1;
-	if (len > limit)
-		return -ENOSPC;
-
-	name = kmalloc(len, GFP_KERNEL);
-	if (!name)
-		return -ENOMEM;
-
-	strcpy(name, token);
-
-	*pname = name;
-	return 0;
-}
-
-
 static inline void kill_final_newline(char *str)
 {
 	char *newline = strrchr(str, '\n');
@@ -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;
+
+	char buf[80 + 12], *str = buf; /* 80 for device, 12 for erase size */
 	char *str = buf;
 	char *token[2];
 	char *name;
 	size_t erase_size = PAGE_SIZE;
-	int i, ret;
+	int i;
 
 	if (strnlen(val, sizeof(buf)) >= sizeof(buf))
 		parse_err("parameter too long");
@@ -450,16 +450,12 @@ static int block2mtd_setup(const char *v
 	if (!token[0])
 		parse_err("no argument");
 
-	ret = parse_name(&name, token[0], 80);
-	if (ret == -ENOMEM)
-		parse_err("out of memory");
-	if (ret == -ENOSPC)
-		parse_err("name too long");
-	if (ret)
-		return 0;
+	name = token[0];
+	if (strlen(name) + 1 > 80)
+		parse_err("device name too long");
 
 	if (token[1]) {
-		ret = parse_num(&erase_size, token[1]);
+		int ret = parse_num(&erase_size, token[1]);
 		if (ret) {
 			kfree(name);
 			parse_err("illegal erase size");
@@ -472,13 +468,40 @@ static int block2mtd_setup(const char *v
 }
 
 
+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();
+	
+	return 0;
+}
+
+
 module_param_call(block2mtd, block2mtd_setup, NULL, NULL, 0200);
 MODULE_PARM_DESC(block2mtd, "Device to use. \"block2mtd=<dev>[,<erasesize>]\"");
 
 static int __init block2mtd_init(void)
 {
+	int ret = 0;
 	INFO("version " VERSION);
-	return 0;
+
+	ret = block2mtd_setup2();
+	block2mtd_init_called = 1;
+
+	return ret;
 }
 
 
#1 lore raw
Quoted 11 lines · Ville Herva

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.

Quoted 8 lines

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...

drivers/mtd/devices/block2mtd.c static inline void kill_final_newline(ch · line 429

Quoted 13 lines
drivers/mtd/devices/block2mtd.c +6 -2
@@ -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;

drivers/mtd/devices/block2mtd.c static inline void kill_final_newline(ch · line 432

Quoted 3 lines
drivers/mtd/devices/block2mtd.c +2 -0
+
+	char buf[80 + 12], *str = buf; /* 80 for device, 12 for erase size */
 	char *str = buf;

Now we have two variables called "str".

drivers/mtd/devices/block2mtd.c static int block2mtd_setup(const char *v · line 458

Quoted 3 lines
drivers/mtd/devices/block2mtd.c +1 -1
 	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.

Quoted 7 lines
drivers/mtd/devices/block2mtd.c +7 -0
+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(). */

drivers/mtd/devices/block2mtd.c static int block2mtd_setup(const char *v · line 487

Quoted 10 lines
drivers/mtd/devices/block2mtd.c +10 -0
+
+	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

Signature · 2 lines
Do not stop an army on its way home.
-- Sun Tzu
#2 lore raw
Quoted 2 lines · you [Jörn Engel]

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

Great.

Quoted 4 lines

drivers/mtd/devices/block2mtd.c static inline void kill_final_newline(ch · line 429

Quoted 2 lines
drivers/mtd/devices/block2mtd.c +1 -1
-	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.

Ok, I to follow the kernel style, but missed that. You're right.

Quoted 3 lines

Also, I believe we
don't need the cast here.

	char *val = block2mtd_paramline;

Fair enough.

Quoted 4 lines

drivers/mtd/devices/block2mtd.c static inline void kill_final_newline(ch · line 432

Quoted 2 lines
drivers/mtd/devices/block2mtd.c +1 -0
+	char buf[80 + 12], *str = buf; /* 80 for device, 12 for erase size */
 	char *str = buf;

Now we have two variables called "str".

Oops, I'm terribly sorry. There were minor changes between 2.6.17.3 and MTD GIT. I decided to rediff againt MTD GIT, but apparently wasn't careful enough, since I introduced that merging error.

Quoted 2 lines

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

Fair enough.

Quoted 12 lines
Quoted 12 lines
drivers/mtd/devices/block2mtd.c +12 -0
+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
Quoted 8 lines

drivers/mtd/devices/block2mtd.c static int block2mtd_setup(const char *v · line 487

Quoted 5 lines
drivers/mtd/devices/block2mtd.c +5 -0
+	   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.

Well that's not the only way to do it, and arguably not the cleanest way, but it worked for me.

Quoted 5 lines

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.

You're right, I didn't think of that. (I'm a complete kernel newbie, sorry.)

Perhaps we could do:

static int block2mtd_setup(const char *val, struct kernel_param *kp)

{
	if (block2mtd_init_called)
		return block2mtd_setup2(val);

	strlcpy(block2mtd_paramline, val, sizeof(block2mtd_paramline));

instead of

static int block2mtd_setup(const char *val, struct kernel_param *kp)

{
	strlcpy(block2mtd_paramline, val, sizeof(block2mtd_paramline));

	if (block2mtd_init_called)
		return block2mtd_setup2();

and then parse val in block2mtd_setup2() instead of block2mtd_paramline if it is not non-NULL. Avoids the strlcpy(). I'm not sure that's better than spinlocks. I'm not also sure there aren't problems in adding devices in parallel later in the code path (perhaps you looked closer.) And what I feel least the least qualified to comment is if this worth it at all (I'll leave that to you) :).

Another thing I thought a bit about is that the global block2mtd_paramline array is now never released. It's only 92 bytes, but still.

If I'm reading http://www.faqs.org/docs/kernel_2_4/lki-1.html 1.8 "Freeing initialisation data and code" right, block2mtd_paramline could be marked as __initdata, right? It should be freed after module_init(). In module case, it is still compiled in, and not released until the module is unloaded - that of course could be #ifdef'ed, but perhaps the code clutter is not worth it?

Quoted 1 line

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

Sure, I'll redo the patch with those addressed and resend.

I can look at the other two points later, depending on your comments.

-- v --

[email protected]