Re: [PATCH] virtio-mmio: Devices parameter parsing

Re: [PATCH] virtio-mmio: Devices parameter parsing

[Rusty Russell]

On Tue, 15 Nov 2011 13:53:05 +0000, Pawel Moll <pawel.moll@arm.com> wrote:
> +static char *virtio_mmio_cmdline_devices;
> +module_param_named(devices, virtio_mmio_cmdline_devices, charp, 0);

This is the wrong way to do this.

Don't put things in a charp and process it later.  It's lazy.  You
should write parsers for it and call it straight from module_param.

And if you do it that way, multiple devices are simply multiple
arguments.

Like so:

static int virtio_mmio_set(const char *val, const struct kernel_param *kp)
{
        if (!virtio_mmio_cmdline_parent_initialized) {
        	err = device_register(&virtio_mmio_cmdline_parent);
        	if (err)
        		return err;
                virtio_mmio_cmdline_parent_initialized = true;
        }
        
        ... parse here, return -errno on fail, 0 on success.
}

static struct kernel_param_ops param_ops_virtio_mmio = {
        .set = virtio_mmio_set,
        .get = virtio_mmio_get,
};

module_param_cb(device, &param_ops_virtio_mmio, NULL, 0400);

Initialization and error handling is now done for you...

Cheers,
Rusty.

Re: [PATCH] virtio-mmio: Devices parameter parsing

[Pawel Moll]

On Wed, 2011-11-16 at 00:42 +0000, Rusty Russell wrote:
> On Tue, 15 Nov 2011 13:53:05 +0000, Pawel Moll <pawel.moll@arm.com> wrote:
> > +static char *virtio_mmio_cmdline_devices;
> > +module_param_named(devices, virtio_mmio_cmdline_devices, charp, 0);
> 
> This is the wrong way to do this.
> 
> Don't put things in a charp and process it later.  It's lazy.  

Definitely not lazy - string parsing is very absorbing, really! ;-)

> You
> should write parsers for it and call it straight from module_param.
> 
> And if you do it that way, multiple devices are simply multiple
> arguments.
>
> module_param_cb(device, &param_ops_virtio_mmio, NULL, 0400);

Hm. Honestly, first time I hear someone suggesting using the param_cb
variant... It doesn't seem to be too popular ;-)

$ git grep ^module_param\(.\*charp.\*\)\; | wc -l
159
$ git grep ^module_param_cb\(.\*\)\; | wc -l
7

But anyway, I tried to do use your suggestion (see below), even if I'm
not convinced it's winning anything. But, in order to use the strsep()
and kstrtoull() I need a non-const version of the string. And as the
slab is not available at the time, I can't simply do kstrdup(), I'd have
to abuse the "const char *val" params_ops.set's argument...
Interestingly charp operations have the same problem:

int param_set_charp(const char *val, const struct kernel_param *kp)
<...>
        /* This is a hack.  We can't kmalloc in early boot, and we
         * don't need to; this mangled commandline is preserved. */
        if (slab_is_available()) {

Also, regarding the fact that one parameter define more than one
"entity" - this is how mtd partitions are defined (all similarities
intended ;-), see "drivers/mtd/cmdlinepart.c". And I quite like this
syntax...

There's one more thing I realize I missed. The platform devices are
registered starting from id 0 (so as "virtio-mmio.0"). Now, if you
happened to have a statically defined virtio-mmio with the same id,
there would be a clash. So I wanted to add a "first_id" parameter, but
with the _cb parameter I can't guarantee ordering (I mean, to have the
"first_id" available _before_ first device is being instantiated). So
I'd have to cache the devices and then create them in one go. Sounds
like the charp parameter for me :-)

So, unless you have really strong feelings about it, I'd stick to the
single-string version, I'll just add the "first_id" parameter tomorrow.

Cheers!

Paweł

8<---------------------------------------

/* Devices list parameter */

#if defined(CONFIG_VIRTIO_MMIO_CMDLINE_DEVICES)

static struct device virtio_mmio_cmdline_parent = {
	.init_name = "virtio-mmio-cmdline",
};

static int virtio_mmio_cmdline_parent_registered;
static int virtio_mmio_cmdline_id;

static int virtio_mmio_register_cmdline_device(const char *device,
		const struct kernel_param *kp)
{
	int err;
	struct resource resources[] = {
		{
			.flags = IORESOURCE_IRQ,
		}, {
			.flags = IORESOURCE_MEM,
		}
	};
	char *__token, *token, *size, *base;
	unsigned long long val;

	token = __token = kstrdup(device, GFP_KERNEL);
	printk("token=%s\n", token);

	/* Split memory and IRQ resources */
	pr_info("base=%s, token=%s\n", base, token);
	if (base == token || !token || !*token)
		goto error;

	/* Get IRQ */
	if (kstrtoull(token, 0, &val) != 0)
		goto error;
	resources[0].start = val;
	resources[0].end = val;

	/* Split base address and size */
	size = strsep(&base, "@");
	if (size == base || !base || !*base)
		pr_err("No base in '%s'!\n", device);

	/* Get base address */
	if (kstrtoull(base, 0, &val) != 0)
		pr_err("Wrong base in '%s'!\n", device);
	resources[1].start = val;
	resources[1].end = val;

	/* Get size */
	resources[1].end += memparse(size, &token) - 1;
	if (size == token || *token)
		goto error;

	kfree(__token);

	pr_info("Registering device virtio-mmio.%d at 0x%x-0x%x, IRQ %u.\n",
			virtio_mmio_cmdline_id, resources[1].start,
			resources[1].end, resources[0].start);

	if (!virtio_mmio_cmdline_parent_registered) {
		err = device_register(&virtio_mmio_cmdline_parent);
		if (err) {
			pr_err("Failed to register parent device!\n");
			return err;
		}
		virtio_mmio_cmdline_parent_registered = 1;
	}

	return platform_device_register_resndata(&virtio_mmio_cmdline_parent,
			"virtio-mmio", virtio_mmio_cmdline_id++,
			resources, ARRAY_SIZE(resources), NULL, 0) != NULL;

error:
	kfree(__token);
	return -EINVAL;
}

static struct kernel_param_ops virtio_mmio_cmdline_param_ops = {
	.set = virtio_mmio_register_cmdline_device,
};

module_param_cb(device, &virtio_mmio_cmdline_param_ops, NULL, 0);



_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH v2] virtio-mmio: Devices parameter parsing

[Pawel Moll]

This patch adds an option to instantiate guest virtio-mmio devices
basing on a kernel command line (or module) parameter, for example:

	virtio_mmio.devices=0x100@0x100b0000:48,1K@0x1001e000:74

Signed-off-by: Pawel Moll <pawel.moll@arm.com>
---
 drivers/virtio/Kconfig       |   31 +++++++
 drivers/virtio/virtio_mmio.c |  181 +++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 211 insertions(+), 1 deletions(-)

Hi Rusty,

This version adds the "first_id" parameter I mentioned yesterday,
but still using single charp parameter instead of the _cb version.
And unless you are really (_really_) against it, I'd rather see
this variant.

Cheers!

Paweł

diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
index 816ed08..00f2c82 100644
--- a/drivers/virtio/Kconfig
+++ b/drivers/virtio/Kconfig
@@ -46,4 +46,35 @@ config VIRTIO_BALLOON
 
  	 If unsure, say N.
 
+config VIRTIO_MMIO_CMDLINE_DEVICES
+	bool "Memory mapped virtio devices parameter parsing"
+	depends on VIRTIO_MMIO
+	---help---
+	 Allow virtio-mmio devices instantiation via the kernel command line
+	 or module parameter. Be aware that using incorrect parameters (base
+	 address in particular) can crash your system - you have been warned.
+
+	 The format for the parameter is as follows:
+
+		[virtio_mmio.]devices=<device>[<delim><device>]
+
+	 where:
+		<device>   := <size>@<baseaddr>:<irq>
+		<delim>    := ',' or ';'
+		<size>     := size (can use standard suffixes like K or M)
+		<baseaddr> := physical base address
+		<irq>      := interrupt number (as passed to request_irq())
+
+	 Example kernel command line parameter:
+
+		virtio_mmio.devices=0x100@0x100b0000:48,1K@0x1001e000:74
+
+	 This will register platform devices virtio_mmio.<id>, where <id>
+	 values are consecutive integer numbers starting from 0 by default.
+	 The first id value can be changed with "first_id" parameter:
+
+	 	[virtio_mmio.]first_id=<id>
+
+	 If unsure, say 'N'.
+
 endmenu
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index acc5e43..05b39c1 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -6,6 +6,55 @@
  * This module allows virtio devices to be used over a virtual, memory mapped
  * platform device.
  *
+ * The guest device(s) may be instantiated in one of three equivalent ways:
+ *
+ * 1. Static platform device in board's code, eg.:
+ *
+ *	static struct platform_device v2m_virtio_device = {
+ *		.name = "virtio-mmio",
+ *		.id = -1,
+ *		.num_resources = 2,
+ *		.resource = (struct resource []) {
+ *			{
+ *				.start = 0x1001e000,
+ *				.end = 0x1001e0ff,
+ *				.flags = IORESOURCE_MEM,
+ *			}, {
+ *				.start = 42 + 32,
+ *				.end = 42 + 32,
+ *				.flags = IORESOURCE_IRQ,
+ *			},
+ *		}
+ *	};
+ *
+ * 2. Device Tree node, eg.:
+ *
+ *		virtio_block@1e000 {
+ *			compatible = "virtio,mmio";
+ *			reg = <0x1e000 0x100>;
+ *			interrupts = <42>;
+ *		}
+ *
+ * 3. Kernel module (or command line) parameter:
+ *
+ *		[virtio_mmio.]devices=<device>[<delim><device>]
+ *    where:
+ *		<device>   := <size>@<baseaddr>:<irq>
+ *		<delim>    := ',' or ';'
+ *		<size>     := size (can use standard suffixes like K or M)
+ *		<baseaddr> := physical base address
+ *		<irq>      := interrupt number (as passed to request_irq())
+ *    eg.:
+ *		virtio_mmio.devices=0x100@0x100b0000:48,1K@0x1001e000:74
+ *
+ *    This will register platform devices virtio_mmio.<id>, where <id>
+ *    values are consecutive integer numbers starting from 0 by default.
+ *    The first id value can be changed with "first_id" parameter:
+ *
+ *		[virtio_mmio.]first_id=<id>
+ *
+ *
+ *
  * Registers layout (all 32-bit wide):
  *
  * offset d. name             description
@@ -42,6 +91,8 @@
  * See the COPYING file in the top-level directory.
  */
 
+#define pr_fmt(fmt) "virtio-mmio: " fmt
+
 #include <linux/highmem.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
@@ -443,6 +494,130 @@ static int __devexit virtio_mmio_remove(struct platform_device *pdev)
 
 
 
+/* Devices list parameter */
+
+#if defined(CONFIG_VIRTIO_MMIO_CMDLINE_DEVICES)
+
+static char *virtio_mmio_cmdline_devices;
+module_param_named(devices, virtio_mmio_cmdline_devices, charp, 0);
+
+static int virtio_mmio_cmdline_id;
+module_param_named(first_id, virtio_mmio_cmdline_id, int, 0);
+
+static struct device virtio_mmio_cmdline_parent = {
+	.init_name = "virtio-mmio-cmdline",
+};
+
+static int virtio_mmio_register_cmdline_devices(void)
+{
+	int err;
+	char *device;
+	char *token = NULL;
+
+	err = device_register(&virtio_mmio_cmdline_parent);
+	if (err) {
+		pr_err("Failed to register %s!\n",
+				virtio_mmio_cmdline_parent.init_name);
+		return err;
+	}
+
+	/* Split colon-or-semicolon-separated devices */
+	while ((device = strsep(&virtio_mmio_cmdline_devices, ",;")) != NULL) {
+		struct resource resources[] = {
+			{
+				.flags = IORESOURCE_IRQ,
+			}, {
+				.flags = IORESOURCE_MEM,
+			}
+		};
+		char *size, *base;
+		unsigned long long val;
+
+		if (!*device)
+			continue;
+
+		kfree(token);
+		token = kstrdup(device, GFP_KERNEL);
+
+		/* Split memory and IRQ resources */
+		base = strsep(&token, ":");
+		if (base == token || !token || !*token) {
+			pr_err("No IRQ in '%s'!\n", device);
+			continue;
+		}
+
+		/* Get IRQ */
+		if (kstrtoull(token, 0, &val) != 0) {
+			pr_err("Wrong IRQ in '%s'!\n", device);
+			continue;
+		}
+		resources[0].start = val;
+		resources[0].end = val;
+
+		/* Split base address and size */
+		size = strsep(&base, "@");
+		if (size == base || !base || !*base) {
+			pr_err("No base in '%s'!\n", device);
+			continue;
+		}
+
+		/* Get base address */
+		if (kstrtoull(base, 0, &val) != 0) {
+			pr_err("Wrong base in '%s'!\n", device);
+			continue;
+		}
+		resources[1].start = val;
+		resources[1].end = val;
+
+		/* Get size */
+		resources[1].end += memparse(size, &token) - 1;
+		if (size == token || *token) {
+			pr_err("Wrong size in '%s'!\n", device);
+			continue;
+		}
+
+		pr_info("Registering device %d at 0x%x-0x%x, IRQ %u.\n",
+				virtio_mmio_cmdline_id, resources[1].start,
+				resources[1].end, resources[0].start);
+
+		platform_device_register_resndata(&virtio_mmio_cmdline_parent,
+				"virtio-mmio", virtio_mmio_cmdline_id++,
+				resources, ARRAY_SIZE(resources), NULL, 0);
+	}
+
+	kfree(token);
+
+	return 0;
+}
+
+static int virtio_mmio_unregister_cmdline_device(struct device *dev,
+		void *data)
+{
+	platform_device_unregister(to_platform_device(dev));
+
+	return 0;
+}
+
+static void virtio_mmio_unregister_cmdline_devices(void)
+{
+	device_for_each_child(&virtio_mmio_cmdline_parent, NULL,
+			virtio_mmio_unregister_cmdline_device);
+	device_unregister(&virtio_mmio_cmdline_parent);
+}
+
+#else
+
+static int virtio_mmio_register_cmdline_devices(void)
+{
+	return 0;
+}
+
+static void virtio_mmio_unregister_cmdline_devices(void)
+{
+}
+
+#endif
+
 /* Platform driver */
 
 static struct of_device_id virtio_mmio_match[] = {
@@ -463,11 +638,15 @@ static struct platform_driver virtio_mmio_driver = {
 
 static int __init virtio_mmio_init(void)
 {
-	return platform_driver_register(&virtio_mmio_driver);
+	int err = virtio_mmio_register_cmdline_devices();
+
+	return err ? err : platform_driver_register(&virtio_mmio_driver);
 }
 
 static void __exit virtio_mmio_exit(void)
 {
+	virtio_mmio_unregister_cmdline_devices();
+
 	platform_driver_unregister(&virtio_mmio_driver);
 }
 
-- 
1.6.3.3

Re: [PATCH] virtio-mmio: Devices parameter parsing

[Rusty Russell]

On Wed, 16 Nov 2011 18:13:42 +0000, Pawel Moll <pawel.moll@arm.com> wrote:
> On Wed, 2011-11-16 at 00:42 +0000, Rusty Russell wrote:
> > On Tue, 15 Nov 2011 13:53:05 +0000, Pawel Moll <pawel.moll@arm.com> wrote:
> > > +static char *virtio_mmio_cmdline_devices;
> > > +module_param_named(devices, virtio_mmio_cmdline_devices, charp, 0);
> > 
> > This is the wrong way to do this.
> > 
> > Don't put things in a charp and process it later.  It's lazy.  
> 
> Definitely not lazy - string parsing is very absorbing, really! ;-)
> 
> > You
> > should write parsers for it and call it straight from module_param.
> > 
> > And if you do it that way, multiple devices are simply multiple
> > arguments.
> >
> > module_param_cb(device, &param_ops_virtio_mmio, NULL, 0400);
> 
> Hm. Honestly, first time I hear someone suggesting using the param_cb
> variant... It doesn't seem to be too popular ;-)

No it's not; I didn't bother when I converted things across, and it
shows.  But we expect virtio hackers to be smarter than average :)

> But anyway, I tried to do use your suggestion (see below), even if I'm
> not convinced it's winning anything. But, in order to use the strsep()
> and kstrtoull() I need a non-const version of the string. And as the
> slab is not available at the time, I can't simply do kstrdup(), I'd have
> to abuse the "const char *val" params_ops.set's argument...
> Interestingly charp operations have the same problem:
> 
> int param_set_charp(const char *val, const struct kernel_param *kp)
> <...>
>         /* This is a hack.  We can't kmalloc in early boot, and we
>          * don't need to; this mangled commandline is preserved. */
>         if (slab_is_available()) {
> 
> Also, regarding the fact that one parameter define more than one
> "entity" - this is how mtd partitions are defined (all similarities
> intended ;-), see "drivers/mtd/cmdlinepart.c". And I quite like this
> syntax...

Yes, that's the traditional method.  I don't really hate it, but it
seems unnecessary and it's less useful when reporting parse errors.

> There's one more thing I realize I missed. The platform devices are
> registered starting from id 0 (so as "virtio-mmio.0"). Now, if you
> happened to have a statically defined virtio-mmio with the same id,
> there would be a clash. So I wanted to add a "first_id" parameter, but
> with the _cb parameter I can't guarantee ordering (I mean, to have the
> "first_id" available _before_ first device is being instantiated). So
> I'd have to cache the devices and then create them in one go. Sounds
> like the charp parameter for me :-)

Well, tell them to get the cmdline order right, or allow an explicit
device id in the commandline.

Since I hope we're going to be coding together more often, I've written
this how I would have done it (based loosely on your version) to
compare.

Main changes other than the obvious:
1) Documentation in kernel-parameters.txt
2) Doesn't leak mem on error paths.
3) Handles error from platform_device_register_resndata().
4) Uses shorter names for static functions/variables.
5) Allows (read) access to kernel parameters via sysfs.
5) Completely untested.

See what you think...
Rusty.

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -110,6 +110,7 @@ parameter is applicable:
 	USB	USB support is enabled.
 	USBHID	USB Human Interface Device support is enabled.
 	V4L	Video For Linux support is enabled.
+	VMMIO	CONFIG_VIRTIO_MMIO_CMDLINE_DEVICES is enabled.
 	VGA	The VGA console has been enabled.
 	VT	Virtual terminal support is enabled.
 	WDT	Watchdog support is enabled.
@@ -2720,6 +2721,12 @@ bytes respectively. Such letter suffixes
 	video=		[FB] Frame buffer configuration
 			See Documentation/fb/modedb.txt.
 
+	virtio_mmio.device=<size>[KMG]@<baseaddr>:<irq>
+			Can be used multiple times for multiple devices.
+			Example would be:
+				virtio_mmio.device=0x100@0x100b0000:48
+
+
 	vga=		[BOOT,X86-32] Select a particular video mode
 			See Documentation/x86/boot.txt and
 			Documentation/svga.txt.
diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
--- a/drivers/virtio/Kconfig
+++ b/drivers/virtio/Kconfig
@@ -46,4 +46,15 @@ config VIRTIO_BALLOON
 
  	 If unsure, say N.
 
+config VIRTIO_MMIO_CMDLINE_DEVICES
+	bool "Memory mapped virtio devices parameter parsing"
+	depends on VIRTIO_MMIO
+	---help---
+	 Allow virtio-mmio devices instantiation via the kernel command line
+	 or module parameters. Be aware that using incorrect parameters (base
+	 address in particular) can crash your system - you have been warned.
+	 See Documentation/kernel-parameters.txt for details.
+
+	 If unsure, say 'N'.
+
 endmenu
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -6,6 +6,45 @@
  * This module allows virtio devices to be used over a virtual, memory mapped
  * platform device.
  *
+ * The guest device(s) may be instantiated in one of three equivalent ways:
+ *
+ * 1. Static platform device in board's code, eg.:
+ *
+ *	static struct platform_device v2m_virtio_device = {
+ *		.name = "virtio-mmio",
+ *		.id = -1,
+ *		.num_resources = 2,
+ *		.resource = (struct resource []) {
+ *			{
+ *				.start = 0x1001e000,
+ *				.end = 0x1001e0ff,
+ *				.flags = IORESOURCE_MEM,
+ *			}, {
+ *				.start = 42 + 32,
+ *				.end = 42 + 32,
+ *				.flags = IORESOURCE_IRQ,
+ *			},
+ *		}
+ *	};
+ *
+ * 2. Device Tree node, eg.:
+ *
+ *		virtio_block@1e000 {
+ *			compatible = "virtio,mmio";
+ *			reg = <0x1e000 0x100>;
+ *			interrupts = <42>;
+ *		}
+ *
+ * 3. Kernel module (or command line) parameter (can be used more than once!)
+ *		[virtio_mmio.]device=<size>@<baseaddr>:<irq>
+ *    where:
+ *		<size>     := size (can use standard suffixes like K or M)
+ *		<baseaddr> := physical base address
+ *		<irq>      := interrupt number (as passed to request_irq())
+ *    eg.:
+ *		virtio_mmio.device=0x100@0x100b0000:48 virtio_mmio.device=1K@0x1001e000:74
+ *
+ *
  * Registers layout (all 32-bit wide):
  *
  * offset d. name             description
@@ -42,6 +81,8 @@
  * See the COPYING file in the top-level directory.
  */
 
+#define pr_fmt(fmt) "virtio-mmio: " fmt
+
 #include <linux/highmem.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
@@ -443,6 +484,119 @@ static int __devexit virtio_mmio_remove(
 
 
 
+/* Devices list parameter */
+
+#if defined(CONFIG_VIRTIO_MMIO_CMDLINE_DEVICES)
+static struct device cmdline_parent;
+static bool cmdline_parent_registered;
+static int cmdline_id __initdata;
+
+/* <size>@<baseaddr>:<irq> */
+static int set_cmdline_device(const char *device, const struct kernel_param *kp)
+{
+	int err;
+	struct resource resources[2];
+	unsigned long long val;
+	char *delim, *p;
+	struct platform_device *pdev;
+
+	delim = strchr(device, '@');
+	if (!delim)
+		return -EINVAL;
+
+	/* kstrtoull is strict, so we have to temporarily truncate. */
+	*delim = '\0';
+	err = kstrtoull(device, 0, &val);
+	*delim = '@';
+	if (err)
+		return err;
+
+	resources[0].flags = IORESOURCE_MEM;
+	resources[0].start = val;
+	resources[0].end = val + memparse(delim + 1, &p) - 1;
+
+	if (*p != ':')
+		return -EINVAL;
+
+	err = kstrtoull(p+1, 0, &val);
+	if (err)
+		return err;
+
+	resources[1].flags = IORESOURCE_IRQ;
+	resources[1].start = resources[1].end = val;
+
+	pr_info("Registering device virtio-mmio.%d at 0x%lx-0x%lx, IRQ %u.\n",
+		cmdline_id,
+		(long)resources[0].start, (long)resources[0].end,
+		(int)resources[1].start);
+
+	if (!cmdline_parent_registered) {
+		cmdline_parent.init_name = "virtio-mmio-cmdline";
+		err = device_register(&cmdline_parent);
+		if (err) {
+			pr_err("Failed to register parent device (%u)!\n", err);
+			return err;
+		}
+		cmdline_parent_registered = true;
+	}
+
+	pdev = platform_device_register_resndata(&cmdline_parent,
+						 "virtio-mmio",
+						 cmdline_id,
+						 resources,
+						 ARRAY_SIZE(resources),
+						 NULL, 0);
+	if (IS_ERR(pdev))
+		return PTR_ERR(pdev);
+	cmdline_id++;
+	return 0;
+}
+
+static int get_dev(struct device *dev, void *_buf)
+{
+	char *buf = _buf;
+	unsigned int len = strlen(buf);
+	struct platform_device *pdev = to_platform_device(dev);
+
+	snprintf(buf + len, PAGE_SIZE - len, "%llu@%llu:%llu\n",
+		 pdev->resource[0].end - pdev->resource[0].start + 1ULL,
+		 (unsigned long long)pdev->resource[0].start,
+		 (unsigned long long)pdev->resource[1].start);
+	return 0;
+}
+
+static int get_cmdline_device(char *buffer, const struct kernel_param *kp)
+{
+	buffer[0] = '\0';
+	device_for_each_child(&cmdline_parent, buffer, get_dev);
+	return strlen(buffer) + 1;
+}
+
+static struct kernel_param_ops cmdline_param_ops = {
+	.set = set_cmdline_device,
+	.get = get_cmdline_device,
+};
+
+module_param_cb(device, &cmdline_param_ops, NULL, 0400);
+
+static int unregister_cmdline_device(struct device *dev, void *data)
+{
+	platform_device_unregister(to_platform_device(dev));
+	return 0;
+}
+
+static void unregister_cmdline_devices(void)
+{
+	device_for_each_child(&cmdline_parent, NULL, unregister_cmdline_device);
+	if (cmdline_parent_registered)
+		device_unregister(&cmdline_parent);
+}
+#else /* ! CONFIG_VIRTIO_MMIO_CMDLINE_DEVICES */
+static void unregister_cmdline_devices(void)
+{
+}
+#endif
+
 /* Platform driver */
 
 static struct of_device_id virtio_mmio_match[] = {
@@ -468,6 +622,7 @@ static int __init virtio_mmio_init(void)
 
 static void __exit virtio_mmio_exit(void)
 {
+	unregister_cmdline_devices();
 	platform_driver_unregister(&virtio_mmio_driver);
 }

Re: [PATCH] virtio-mmio: Devices parameter parsing

[Pawel Moll]

On Mon, 2011-11-21 at 03:32 +0000, Rusty Russell wrote:
> No it's not; I didn't bother when I converted things across, and it
> shows.  

546970bc (Rusty Russell   2010-08-11 23:04:20 -0600 133) #define module_param_cb

Now I understand... :-)

> But we expect virtio hackers to be smarter than average :)

Hope I'll stand up to the challenge ;-)

> > there would be a clash. So I wanted to add a "first_id" parameter, but
<...>
> Well, tell them to get the cmdline order right, or allow an explicit
> device id in the commandline.

Yeah, I've came up with the same idea last night:

	<size>[KMG]@<base>:<irq>[:<id>]

<id>, if specified, sets the id for the current device and a base for
the next one.

> Since I hope we're going to be coding together more often, I've written
> this how I would have done it (based loosely on your version) to
> compare.

Thanks, your efforts are truly appreciated!

> Main changes other than the obvious:
> 1) Documentation in kernel-parameters.txt

I was considering this previously but for some reason I thought
kernel-parameters.txt wasn't a place for module parameters at all. Now I
had another look and see I was wrong.

> 2) Doesn't leak mem on error paths.

Em, I don't think my code was leaking memory in any error case? (no
offence meant or taken ;-)

> 3) Handles error from platform_device_register_resndata().

As my code was ignoring wrong descriptions (all the continues) I ignored
this result as well (the implementation complains on its own anyway),
but I get your point.

> 4) Uses shorter names for static functions/variables.

Ah, I get the hint :-) I'm trying to keep the naming convention in
static symbols as well, as it makes cscope/ctags/grep usage easier...
I'll just use the abbreviated "vm_" prefixes then.

> See what you think...

Funnily enough when I proposed some string parser few years ago (totally
different story) I was flamed for using strchr() instead of strsep() ;-)
But ok, I don't mind getting back to basics.

> +static int set_cmdline_device(const char *device, const struct kernel_param *kp)
[...]
> +       delim = strchr(device, '@');
[...]
> +	*delim = '\0';

Ah. I forgot that strchr() takes const char * but returns "non-const"
char *... Cheating, that's what it is ;-), but will work. Probably what
we really want is something like
	kstrtoull_delim(device, 0, &val, "@\0")
I'll have a look and may try to propose something of that sort, but
that's another story. Maybe I should just use simple_strtol() for the
time being?

I'll post v3 tonight or tomorrow.

Cheers!

Paweł


_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] virtio-mmio: Devices parameter parsing

[Pawel Moll]

On Mon, 2011-11-21 at 14:44 +0000, Pawel Moll wrote:
> I'll post v3 tonight or tomorrow.

Ok, it won't be v3 then...

I tried again, using your suggestions - see below (just the crucial bit,
however I have the kernel-parameters.txt bits ready as well). Parsing
works like charm, however when it gets to device_register() and
platform_device_register_resndata() I hit the same problem as
previously. Both are using slab allocator...

device_register()->device_add()->device_private_init()->kzalloc()

and

platform_device_register_resndata()->platform_device_register_full()->
platform_device_alloc()->kzalloc().

Essentially it seems that the parameter callbacks are just executed
waaay to early to do more than just set few integers here and there...

Any ideas?

Cheers!

Paweł

8<-------------------------------------------------------------------

@@ -443,6 +489,145 @@ static int __devexit virtio_mmio_remove(struct platform_device *pdev)
 
 
 
+/* Devices list parameter */
+
+#if defined(CONFIG_VIRTIO_MMIO_CMDLINE_DEVICES)
+
+static struct device vm_cmdline_parent = {
+	.init_name = "virtio-mmio-cmdline",
+};
+
+static int vm_cmdline_parent_registered;
+static int vm_cmdline_id;
+
+static int vm_cmdline_set(const char *str,
+		const struct kernel_param *kp)
+{
+	int err;
+	struct resource resources[2];
+	unsigned long long val;
+	char *delim;
+	struct platform_device *pdev;
+
+	/* Size */
+	resources[0].flags = IORESOURCE_MEM;
+	resources[0].end = memparse(str, &delim) - 1;
+	if (*delim != '@')
+		return -EINVAL;
+	str = delim + 1;
+
+	/* Base address */
+	delim = strchr(str, ':');
+	if (!delim)
+		return -EINVAL;
+	/* kstrtoull is strict, so we have to temporarily truncate */
+	*delim = '\0';
+	err = kstrtoull(str, 0, &val);
+	*delim = ':';
+	if (err)
+		return err;
+	resources[0].start = val;
+	resources[0].end += val;
+	str = delim + 1;
+	
+	/* Interrupt */
+	delim = strchr(str, ':');
+	if (delim) /* Optional device id delimiter */
+		*delim = '\0';
+	err = kstrtoull(str, 0, &val);
+	if (delim)
+		*delim = ':';
+	resources[1].flags = IORESOURCE_IRQ;
+	resources[1].start = resources[1].end = val;
+	str = delim + 1;
+
+	/* Optional device id */
+	if (delim) {
+		err = kstrtoull(str, 0, &val);
+		if (err)
+			return err;
+		vm_cmdline_id = val;
+	}
+
+	if (!vm_cmdline_parent_registered) {
+		err = device_register(&vm_cmdline_parent);
+		if (err) {
+			pr_err("Failed to register parent device!\n");
+			return err;
+		}
+		vm_cmdline_parent_registered = 1;
+	}
+
+       pr_info("Registering device virtio-mmio.%d at 0x%llx-0x%llx, IRQ %d.\n",
+		       vm_cmdline_id++,
+		       (unsigned long long)resources[0].start,
+		       (unsigned long long)resources[0].end,
+		       (int)resources[1].start);
+
+	pdev = platform_device_register_resndata(&vm_cmdline_parent,
+			"virtio-mmio", vm_cmdline_id++,
+			resources, ARRAY_SIZE(resources), NULL, 0);
+	if (IS_ERR(pdev))
+		return PTR_ERR(pdev);
+
+	return 0;
+}
+
+static int vm_cmdline_get_device(struct device *dev, void *data)
+{
+	char *buffer = data;
+	unsigned int len = strlen(buffer);
+	struct platform_device *pdev = to_platform_device(dev);
+
+	snprintf(buffer + len, PAGE_SIZE - len, "0x%llx@0x%llx:%llu:%d\n",
+			pdev->resource[0].end - pdev->resource[0].start + 1ULL,
+			(unsigned long long)pdev->resource[0].start,
+			(unsigned long long)pdev->resource[1].start,
+			pdev->id);
+	return 0;
+}
+
+static int vm_cmdline_get(char *buffer, const struct kernel_param *kp)
+{
+	buffer[0] = '\0';
+	device_for_each_child(&vm_cmdline_parent, buffer,
+			vm_cmdline_get_device);
+	return strlen(buffer) + 1;
+}
+
+static struct kernel_param_ops vm_cmdline_param_ops = {
+	.set = vm_cmdline_set,
+	.get = vm_cmdline_get,
+};
+
+module_param_cb(device, &vm_cmdline_param_ops, NULL, S_IRUSR);
+
+static int vm_unregister_cmdline_device(struct device *dev,
+		void *data)
+{
+	platform_device_unregister(to_platform_device(dev));
+
+	return 0;
+}
+
+static void vm_unregister_cmdline_devices(void)
+{
+	if (vm_cmdline_parent_registered) {
+		device_for_each_child(&vm_cmdline_parent, NULL,
+				vm_unregister_cmdline_device);
+		device_unregister(&vm_cmdline_parent);
+		vm_cmdline_parent_registered = 0;
+	}
+}
+
+#else
+
+static void virtio_mmio_unregister_cmdline_devices(void)
+{
+}
+
+#endif
+
 /* Platform driver */
 
 static struct of_device_id virtio_mmio_match[] = {
@@ -469,6 +654,7 @@ static int __init virtio_mmio_init(void)
 static void __exit virtio_mmio_exit(void)
 {
 	platform_driver_unregister(&virtio_mmio_driver);
+	vm_unregister_cmdline_devices();
 }
 
 module_init(virtio_mmio_init);



_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] virtio-mmio: Devices parameter parsing

[Rusty Russell]

On Mon, 21 Nov 2011 17:56:50 +0000, Pawel Moll <pawel.moll@arm.com> wrote:
> On Mon, 2011-11-21 at 14:44 +0000, Pawel Moll wrote:
> > I'll post v3 tonight or tomorrow.
> 
> Ok, it won't be v3 then...
> 
> I tried again, using your suggestions - see below (just the crucial bit,
> however I have the kernel-parameters.txt bits ready as well). Parsing
> works like charm, however when it gets to device_register() and
> platform_device_register_resndata() I hit the same problem as
> previously. Both are using slab allocator...
> 
> device_register()->device_add()->device_private_init()->kzalloc()
> 
> and
> 
> platform_device_register_resndata()->platform_device_register_full()->
> platform_device_alloc()->kzalloc().
> 
> Essentially it seems that the parameter callbacks are just executed
> waaay to early to do more than just set few integers here and there...
> 
> Any ideas?

Rewrite the kernel and every arch so allocators work as soon as we hit
start_kernel() and we can get rid of hundreds of ugly workarounds?

Perhaps not today.  So, we will need a linked list of devices and
resources.  Yeah, that means allocating, which means YA
slab_is_available()/alloc_bootmem() hack.

Think of yourself as a pioneer...

Thanks :)
Rusty.

Re: [PATCH] virtio-mmio: Devices parameter parsing

[Pawel Moll]

On Tue, 2011-11-22 at 00:53 +0000, Rusty Russell wrote:
> > Any ideas?
> Perhaps not today.  So, we will need a linked list of devices and
> resources.  Yeah, that means allocating, which means YA
> slab_is_available()/alloc_bootmem() hack.

Hm. It doesn't sound like a good deal, really... Loads of tricks to get
data I need quite late in the boot process...

> Think of yourself as a pioneer...

I had a vague idea of "late parameters" last night, which would be
parsed once whole machinery is up and running. Will look around and try
to propose something.

On Tue, 2011-11-22 at 00:44 +0000, Rusty Russell wrote: 
> Or would it be simpler to enhance sscanf() with some weird format option
> for suffixing?  I haven't looked for similar cases, but I'd suspect a
> big win in general.
> 
> This would be neater than anything else we've got:
>         if (sscanf(device, "%llu@%llu[KMG]:%u", ...) != 3
>             && sscanf(device, "%llu@%llu[KMG]:%u:%u", ...) != 4)
>                 return -EINVAL;

sscanf was a good hint! Thanks, why haven't I thought of it myself? ;-)

That's what I came up with:

static int vm_cmdline_set(const char *device,
                const struct kernel_param *kp)
{
        struct resource resources[2] = {};
        char *str;
        long long int base;
        int processed, consumed = 0;
        struct platform_device *pdev;
        
        resources[0].flags = IORESOURCE_MEM;
        resources[1].flags = IORESOURCE_IRQ;
        
        resources[0].end = memparse(device, &str) - 1;
        
        processed = sscanf(str, "@%lli:%u%n:%d%n", 
                        &base, &resources[1].start, &consumed,
                        &vm_cmdline_id, &consumed);
                        
        if (processed < 2 || processed > 3 || str[consumed])
                return -EINVAL;
                
        resources[0].start = base;
        resources[0].end += base;
        resources[1].end = resources[1].start;

The only bit missing from sscanf() would be some sort of "%m" format,
which behaved like memparse and also processed unsigned number with "0
base" (hard to believe but the only "universal" - as in octal, dec and
hex - format is %i, which is signed). But still, looks quite neat
already :-)

I'll try to have a look at the "late parameters" idea tomorrow. Any
early warnings?

Cheers!

Pawel

Re: [PATCH] virtio-mmio: Devices parameter parsing

[Rusty Russell]

On Wed, 23 Nov 2011 18:08:52 +0000, Pawel Moll <pawel.moll@arm.com> wrote:
> On Tue, 2011-11-22 at 00:44 +0000, Rusty Russell wrote: 
> > Or would it be simpler to enhance sscanf() with some weird format option
> > for suffixing?  I haven't looked for similar cases, but I'd suspect a
> > big win in general.
> > 
> > This would be neater than anything else we've got:
> >         if (sscanf(device, "%llu@%llu[KMG]:%u", ...) != 3
> >             && sscanf(device, "%llu@%llu[KMG]:%u:%u", ...) != 4)
> >                 return -EINVAL;
> 
> sscanf was a good hint! Thanks, why haven't I thought of it myself? ;-)
> 
> That's what I came up with:
> 
> static int vm_cmdline_set(const char *device,
>                 const struct kernel_param *kp)
> {
>         struct resource resources[2] = {};
>         char *str;
>         long long int base;
>         int processed, consumed = 0;
>         struct platform_device *pdev;
>         
>         resources[0].flags = IORESOURCE_MEM;
>         resources[1].flags = IORESOURCE_IRQ;
>         
>         resources[0].end = memparse(device, &str) - 1;
>         
>         processed = sscanf(str, "@%lli:%u%n:%d%n", 
>                         &base, &resources[1].start, &consumed,
>                         &vm_cmdline_id, &consumed);
>                         
>         if (processed < 2 || processed > 3 || str[consumed])
>                 return -EINVAL;
>                 
>         resources[0].start = base;
>         resources[0].end += base;
>         resources[1].end = resources[1].start;
> 
> The only bit missing from sscanf() would be some sort of "%m" format,
> which behaved like memparse and also processed unsigned number with "0
> base" (hard to believe but the only "universal" - as in octal, dec and
> hex - format is %i, which is signed). But still, looks quite neat
> already :-)

Yeah, something like %pK for kernel pointers in printk; you need a
suffix so that gcc won't get upset.  The "[KMG]" suffix hack was my
poor suggestion...

> I'll try to have a look at the "late parameters" idea tomorrow. Any
> early warnings?

Off the top of my head, this makes me think of the way initcalls are
ordered.  We could put a parameter parsing initcall at the start of each
initcall level in include/asm-generic/vmlinux.lds.h's INITCALLS macro.

Then we steal four bits from struct kernel_param's flags to indicate the
level of the initcall (-1 == existing ones, otherwise N == before level
N initcalls).

Good luck!
Rusty.

Re: [PATCH] virtio-mmio: Devices parameter parsing

[Pawel Moll]

On Mon, 2011-11-28 at 00:31 +0000, Rusty Russell wrote:
> Off the top of my head, this makes me think of the way initcalls are
> ordered.  We could put a parameter parsing initcall at the start of each
> initcall level in include/asm-generic/vmlinux.lds.h's INITCALLS macro.
> 
> Then we steal four bits from struct kernel_param's flags to indicate the
> level of the initcall (-1 == existing ones, otherwise N == before level
> N initcalls).

Yes, this was my initial idea as well. The only problem I faced is the
fact that there is no "between levels"... It's easy to add parameters
parsing _at_ any particular level, but hard to do this _after_ level A
and _before_ level B. The initcalls section simply contains all the
calls, ordered by the level - the only "separated" level is the pre-SMP
early one. And order within one level is determined by the link order,
so I can't guarantee parsing the parameters as the first call of a level
(nor as the last call of the previous level).

I've came up with a simple prototype (below, only relevant fragments),
doing exactly what I want at the "late" level (maybe that's all we
actually need? ;-) Of course adding all other levels is possible,
probably some clever "generalization" will be useful. And of course the
level would be simply a number in the flags, but what to do with "_sync"
levels (like "arch_initcall_sync", which is "3s" not just "3").

If it makes any sense I'll carry on, any feedback will be useful.

Cheers!

Paweł

---
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 7317dc2..22e6196 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -443,6 +489,122 @@ static int __devexit virtio_mmio_remove(struct platform_device *pdev)
 
 
 
+/* Devices list parameter */
+
+#if defined(CONFIG_VIRTIO_MMIO_CMDLINE_DEVICES)
+
+static struct device vm_cmdline_parent = {
+	.init_name = "virtio-mmio-cmdline",
+};
+
+static int vm_cmdline_parent_registered;
+static int vm_cmdline_id;
+
+static int vm_cmdline_set(const char *device,
+		const struct kernel_param *kp)
+{
+	int err;
+	struct resource resources[2] = {};
+	char *str;
+	long long int base;
+	int processed, consumed = 0;
+	struct platform_device *pdev;
+
+	resources[0].flags = IORESOURCE_MEM;
+	resources[1].flags = IORESOURCE_IRQ;
+
+	resources[0].end = memparse(device, &str) - 1;
+
+	processed = sscanf(str, "@%lli:%u%n:%d%n",
+			&base, &resources[1].start, &consumed,
+			&vm_cmdline_id, &consumed);
+
+	if (processed < 2 || processed > 3 || str[consumed])
+		return -EINVAL;
+
+	resources[0].start = base;
+	resources[0].end += base;
+	resources[1].end = resources[1].start;
+
+	if (!vm_cmdline_parent_registered) {
+		err = device_register(&vm_cmdline_parent);
+		if (err) {
+			pr_err("Failed to register parent device!\n");
+			return err;
+		}
+		vm_cmdline_parent_registered = 1;
+	}
+
+	pr_info("Registering device virtio-mmio.%d at 0x%llx-0x%llx, IRQ %d.\n",
+		       vm_cmdline_id++,
+		       (unsigned long long)resources[0].start,
+		       (unsigned long long)resources[0].end,
+		       (int)resources[1].start);
+
+	pdev = platform_device_register_resndata(&vm_cmdline_parent,
+			"virtio-mmio", vm_cmdline_id++,
+			resources, ARRAY_SIZE(resources), NULL, 0);
+	if (IS_ERR(pdev))
+		return PTR_ERR(pdev);
+
+	return 0;
+}
+
+static int vm_cmdline_get_device(struct device *dev, void *data)
+{
+	char *buffer = data;
+	unsigned int len = strlen(buffer);
+	struct platform_device *pdev = to_platform_device(dev);
+
+	snprintf(buffer + len, PAGE_SIZE - len, "0x%llx@0x%llx:%llu:%d\n",
+			pdev->resource[0].end - pdev->resource[0].start + 1ULL,
+			(unsigned long long)pdev->resource[0].start,
+			(unsigned long long)pdev->resource[1].start,
+			pdev->id);
+	return 0;
+}
+
+static int vm_cmdline_get(char *buffer, const struct kernel_param *kp)
+{
+	buffer[0] = '\0';
+	device_for_each_child(&vm_cmdline_parent, buffer,
+			vm_cmdline_get_device);
+	return strlen(buffer) + 1;
+}
+
+static struct kernel_param_ops vm_cmdline_param_ops = {
+	.set = vm_cmdline_set,
+	.get = vm_cmdline_get,
+};
+
+module_param_cb_late(device, &vm_cmdline_param_ops, NULL, S_IRUSR);
+
+static int vm_unregister_cmdline_device(struct device *dev,
+		void *data)
+{
+	platform_device_unregister(to_platform_device(dev));
+
+	return 0;
+}
+
+static void vm_unregister_cmdline_devices(void)
+{
+	if (vm_cmdline_parent_registered) {
+		device_for_each_child(&vm_cmdline_parent, NULL,
+				vm_unregister_cmdline_device);
+		device_unregister(&vm_cmdline_parent);
+		vm_cmdline_parent_registered = 0;
+	}
+}
+
+#else
+
+static void virtio_mmio_unregister_cmdline_devices(void)
+{
+}
+
+#endif
+
 /* Platform driver */
 
 static struct of_device_id virtio_mmio_match[] = {
@@ -469,6 +631,7 @@ static int __init virtio_mmio_init(void)
 static void __exit virtio_mmio_exit(void)
 {
 	platform_driver_unregister(&virtio_mmio_driver);
+	vm_unregister_cmdline_devices();
 }
 
 module_init(virtio_mmio_init);
diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
index 7939f63..fccf0cb 100644
--- a/include/linux/moduleparam.h
+++ b/include/linux/moduleparam.h
@@ -48,7 +48,8 @@ struct kernel_param_ops {
 };
 
 /* Flag bits for kernel_param.flags */
-#define KPARAM_ISBOOL		2
+#define KPARAM_ISBOOL		(1 << 1)
+#define KPARAM_LATE		(1 << 2)
 
 struct kernel_param {
 	const char *name;
@@ -132,7 +133,13 @@ struct kparam_array
  */
 #define module_param_cb(name, ops, arg, perm)				      \
 	__module_param_call(MODULE_PARAM_PREFIX,			      \
-			    name, ops, arg, __same_type((arg), bool *), perm)
+			    name, ops, arg, 				      \
+			    __same_type((arg), bool *), 0, perm)
+
+#define module_param_cb_late(name, ops, arg, perm)			      \
+	__module_param_call(MODULE_PARAM_PREFIX,			      \
+			    name, ops, arg,				      \
+			    __same_type((arg), bool *), 1, perm)
 
 /* On alpha, ia64 and ppc64 relocations to global data cannot go into
    read-only sections (which is part of respective UNIX ABI on these
@@ -146,7 +153,7 @@ struct kparam_array
 
 /* This is the fundamental function for registering boot/module
    parameters. */
-#define __module_param_call(prefix, name, ops, arg, isbool, perm)	\
+#define __module_param_call(prefix, name, ops, arg, isbool, late, perm)	\
 	/* Default value instead of permissions? */			\
 	static int __param_perm_check_##name __attribute__((unused)) =	\
 	BUILD_BUG_ON_ZERO((perm) < 0 || (perm) > 0777 || ((perm) & 2))	\
@@ -155,7 +162,8 @@ struct kparam_array
 	static struct kernel_param __moduleparam_const __param_##name	\
 	__used								\
     __attribute__ ((unused,__section__ ("__param"),aligned(sizeof(void *)))) \
-	= { __param_str_##name, ops, perm, isbool ? KPARAM_ISBOOL : 0,	\
+	= { __param_str_##name, ops, perm, 				\
+	    (isbool ? KPARAM_ISBOOL : 0) | (late ? KPARAM_LATE : 0),	\
 	    { arg } }
 
 /* Obsolete - use module_param_cb() */
@@ -165,6 +173,7 @@ struct kparam_array
 	__module_param_call(MODULE_PARAM_PREFIX,			\
 			    name, &__param_ops_##name, arg,		\
 			    __same_type(arg, bool *),			\
+			    0,					\
 			    (perm) + sizeof(__check_old_set_param(set))*0)
 
 /* We don't get oldget: it's often a new-style param_get_uint, etc. */
@@ -246,7 +255,7 @@ static inline void __kernel_param_unlock(void)
 #define core_param(name, var, type, perm)				\
 	param_check_##type(name, &(var));				\
 	__module_param_call("", name, &param_ops_##type,		\
-			    &var, __same_type(var, bool), perm)
+			    &var, __same_type(var, bool), 0, perm)
 #endif /* !MODULE */
 
 /**
@@ -264,7 +273,7 @@ static inline void __kernel_param_unlock(void)
 		= { len, string };					\
 	__module_param_call(MODULE_PARAM_PREFIX, name,			\
 			    &param_ops_string,				\
-			    .str = &__param_string_##name, 0, perm);	\
+			    .str = &__param_string_##name, 0, 0, perm);	\
 	__MODULE_PARM_TYPE(name, "string")
 
 /**
@@ -292,6 +301,8 @@ extern int parse_args(const char *name,
 		      char *args,
 		      const struct kernel_param *params,
 		      unsigned num,
+		      u16 flags_mask,
+		      u16 flags,
 		      int (*unknown)(char *param, char *val));
 
 /* Called by module remove. */
@@ -402,7 +413,7 @@ extern int param_get_invbool(char *buffer, const struct kernel_param *kp);
 	__module_param_call(MODULE_PARAM_PREFIX, name,			\
 			    &param_array_ops,				\
 			    .arr = &__param_arr_##name,			\
-			    __same_type(array[0], bool), perm);		\
+			    __same_type(array[0], bool), 0, perm);	\
 	__MODULE_PARM_TYPE(name, "array of " #type)
 
 extern struct kernel_param_ops param_array_ops;
diff --git a/init/main.c b/init/main.c
index 217ed23..ce89a53 100644
--- a/init/main.c
+++ b/init/main.c
@@ -407,7 +407,7 @@ static int __init do_early_param(char *param, char *val)
 
 void __init parse_early_options(char *cmdline)
 {
-	parse_args("early options", cmdline, NULL, 0, do_early_param);
+	parse_args("early options", cmdline, NULL, 0, 0, 0, do_early_param);
 }
 
 /* Arch code calls this early on, or if not, just before other parsing. */
@@ -511,7 +511,7 @@ asmlinkage void __init start_kernel(void)
 	parse_early_param();
 	parse_args("Booting kernel", static_command_line, __start___param,
 		   __stop___param - __start___param,
-		   &unknown_bootoption);
+		   KPARAM_LATE, 0, &unknown_bootoption);
 
 	jump_label_init();
 
@@ -737,6 +737,22 @@ static void __init do_basic_setup(void)
 	do_initcalls();
 }
 
+static int __init ignore_unknown_bootoption(char *param, char *val)
+{
+	return 0;
+}
+
+static int __init parse_late_args(void)
+{
+	extern const struct kernel_param __start___param[], __stop___param[];
+
+	strcpy(static_command_line, saved_command_line);
+	parse_args("Late parameters", static_command_line, __start___param,
+		   __stop___param - __start___param,
+		   KPARAM_LATE, KPARAM_LATE, ignore_unknown_bootoption);
+}
+late_initcall(parse_late_args);
+
 static void __init do_pre_smp_initcalls(void)
 {
 	initcall_t *fn;
diff --git a/kernel/module.c b/kernel/module.c
index 178333c..66d3e75 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2893,7 +2893,8 @@ static struct module *load_module(void __user *umod,
 	mutex_unlock(&module_mutex);
 
 	/* Module is ready to execute: parsing args may do that. */
-	err = parse_args(mod->name, mod->args, mod->kp, mod->num_kp, NULL);
+	err = parse_args(mod->name, mod->args, mod->kp, mod->num_kp,
+			 0, 0, NULL);
 	if (err < 0)
 		goto unlink;
 
diff --git a/kernel/params.c b/kernel/params.c
index 65aae11..560345c 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -94,6 +94,8 @@ static int parse_one(char *param,
 		     char *val,
 		     const struct kernel_param *params,
 		     unsigned num_params,
+		     int flags_mask,
+		     int flags,
 		     int (*handle_unknown)(char *param, char *val))
 {
 	unsigned int i;
@@ -102,6 +104,8 @@ static int parse_one(char *param,
 	/* Find parameter */
 	for (i = 0; i < num_params; i++) {
 		if (parameq(param, params[i].name)) {
+			if ((params[i].flags & flags_mask) != flags)
+				return 0;
 			/* No one handled NULL, so do it here. */
 			if (!val && params[i].ops->set != param_set_bool)
 				return -EINVAL;
@@ -180,6 +184,8 @@ int parse_args(const char *name,
 	       char *args,
 	       const struct kernel_param *params,
 	       unsigned num,
+	       u16 flags_mask,
+	       u16 flags,
 	       int (*unknown)(char *param, char *val))
 {
 	char *param, *val;
@@ -195,7 +201,8 @@ int parse_args(const char *name,
 
 		args = next_arg(args, &param, &val);
 		irq_was_disabled = irqs_disabled();
-		ret = parse_one(param, val, params, num, unknown);
+		ret = parse_one(param, val, params, num,
+			        flags_mask, flags, unknown);
 		if (irq_was_disabled && !irqs_disabled()) {
 			printk(KERN_WARNING "parse_args(): option '%s' enabled "
 					"irq's!\n", param);



_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] virtio-mmio: Devices parameter parsing

[Rusty Russell]

On Tue, 29 Nov 2011 17:36:30 +0000, Pawel Moll <pawel.moll@arm.com> wrote:
> On Mon, 2011-11-28 at 00:31 +0000, Rusty Russell wrote:
> > Off the top of my head, this makes me think of the way initcalls are
> > ordered.  We could put a parameter parsing initcall at the start of each
> > initcall level in include/asm-generic/vmlinux.lds.h's INITCALLS macro.
> > 
> > Then we steal four bits from struct kernel_param's flags to indicate the
> > level of the initcall (-1 == existing ones, otherwise N == before level
> > N initcalls).
> 
> Yes, this was my initial idea as well. The only problem I faced is the
> fact that there is no "between levels"... It's easy to add parameters
> parsing _at_ any particular level, but hard to do this _after_ level A
> and _before_ level B. The initcalls section simply contains all the
> calls, ordered by the level - the only "separated" level is the pre-SMP
> early one. And order within one level is determined by the link order,
> so I can't guarantee parsing the parameters as the first call of a level
> (nor as the last call of the previous level).

Yeah, that's why I suggested changing the linker script.
>  /* This is the fundamental function for registering boot/module
>     parameters. */
> -#define __module_param_call(prefix, name, ops, arg, isbool, perm)	\
> +#define __module_param_call(prefix, name, ops, arg, isbool, late, perm)	\
>  	/* Default value instead of permissions? */			\
>  	static int __param_perm_check_##name __attribute__((unused)) =	\
>  	BUILD_BUG_ON_ZERO((perm) < 0 || (perm) > 0777 || ((perm) & 2))	\

Might as well change isbool to "flags", since we have to fix callers
anyway.

> diff --git a/init/main.c b/init/main.c
> index 217ed23..ce89a53 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -407,7 +407,7 @@ static int __init do_early_param(char *param, char *val)
>  
>  void __init parse_early_options(char *cmdline)
>  {
> -	parse_args("early options", cmdline, NULL, 0, do_early_param);
> +	parse_args("early options", cmdline, NULL, 0, 0, 0, do_early_param);

It'd be nice to replace the early param stuff too, but that's probably a
separate patch.  As is getting rid of the old __setup() calls everywhere
;)

But so far, it looks good!

Thanks,
Rusty.

Re: [PATCH] virtio-mmio: Devices parameter parsing

[Pawel Moll]

Hello,

Sorry for the delay - last weeks were a bit hectic for me (and nothing
will change till Friday when I'm off for holiday! :-)

On Thu, 2011-12-01 at 02:06 +0000, Rusty Russell wrote:
> > Yes, this was my initial idea as well. The only problem I faced is the
> > fact that there is no "between levels"... It's easy to add parameters
> > parsing _at_ any particular level, but hard to do this _after_ level A
> > and _before_ level B. The initcalls section simply contains all the
> > calls, ordered by the level - the only "separated" level is the pre-SMP
> > early one. And order within one level is determined by the link order,
> > so I can't guarantee parsing the parameters as the first call of a level
> > (nor as the last call of the previous level).
> 
> Yeah, that's why I suggested changing the linker script.

Sounded a bit scary, but turned out to be much less intrusive that I
expected... I'll post the proposal in a second.

> >  /* This is the fundamental function for registering boot/module
> >     parameters. */
> > -#define __module_param_call(prefix, name, ops, arg, isbool, perm)	\
> > +#define __module_param_call(prefix, name, ops, arg, isbool, late, perm)	\
> >  	/* Default value instead of permissions? */			\
> >  	static int __param_perm_check_##name __attribute__((unused)) =	\
> >  	BUILD_BUG_ON_ZERO((perm) < 0 || (perm) > 0777 || ((perm) & 2))	\
> 
> Might as well change isbool to "flags", since we have to fix callers
> anyway.

Sure thing, done.

> > diff --git a/init/main.c b/init/main.c
> > index 217ed23..ce89a53 100644
> > --- a/init/main.c
> > +++ b/init/main.c
> > @@ -407,7 +407,7 @@ static int __init do_early_param(char *param, char *val)
> >  
> >  void __init parse_early_options(char *cmdline)
> >  {
> > -	parse_args("early options", cmdline, NULL, 0, do_early_param);
> > +	parse_args("early options", cmdline, NULL, 0, 0, 0, do_early_param);
> 
> It'd be nice to replace the early param stuff too, but that's probably a
> separate patch.  As is getting rid of the old __setup() calls everywhere
> ;)

I promise to look into it next year ;-)

> But so far, it looks good!

Cool, have a look at the patches following this mail then. I hope they
make some sense, however it's unlikely I'll get them ready for 3.3
(unless they already are? ;-) because of my holidays. Maybe at least
they could be linux-next-ed to see what did I break?

Cheers!

Paweł


_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH 1/2] params: <level>_initcall-like kernel parameters

[Pawel Moll]

This patch adds a set of macros that can be used to declare
kernel parameters to be parsed _before_ initcalls at a chosen
level are executed. Such parameters are marked using existing
"flags" field of the "kernel_param" structure.

Linker macro collating init calls had to be modified in order
to add additional symbols between levels that are later used
by the init code to split the calls into blocks.

Signed-off-by: Pawel Moll <pawel.moll@arm.com>
---
 include/asm-generic/vmlinux.lds.h |   35 ++++++++------------
 include/linux/moduleparam.h       |   59 ++++++++++++++++++++++++++++----
 init/main.c                       |   66 +++++++++++++++++++++++++++++++++---
 kernel/module.c                   |    3 +-
 kernel/params.c                   |    9 ++++-
 5 files changed, 135 insertions(+), 37 deletions(-)

diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index b5e2e4c..c4ad0b1 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -615,30 +615,23 @@
 		*(.init.setup)						\
 		VMLINUX_SYMBOL(__setup_end) = .;
 
-#define INITCALLS							\
-	*(.initcallearly.init)						\
-	VMLINUX_SYMBOL(__early_initcall_end) = .;			\
-  	*(.initcall0.init)						\
-  	*(.initcall0s.init)						\
-  	*(.initcall1.init)						\
-  	*(.initcall1s.init)						\
-  	*(.initcall2.init)						\
-  	*(.initcall2s.init)						\
-  	*(.initcall3.init)						\
-  	*(.initcall3s.init)						\
-  	*(.initcall4.init)						\
-  	*(.initcall4s.init)						\
-  	*(.initcall5.init)						\
-  	*(.initcall5s.init)						\
-	*(.initcallrootfs.init)						\
-  	*(.initcall6.init)						\
-  	*(.initcall6s.init)						\
-  	*(.initcall7.init)						\
-  	*(.initcall7s.init)
+#define INIT_CALLS_LEVEL(level)						\
+		VMLINUX_SYMBOL(__initcall##level##_start) = .;		\
+		*(.initcall##level##.init)				\
+		*(.initcall##level##s.init)				\
 
 #define INIT_CALLS							\
 		VMLINUX_SYMBOL(__initcall_start) = .;			\
-		INITCALLS						\
+		*(.initcallearly.init)					\
+		INIT_CALLS_LEVEL(0)					\
+		INIT_CALLS_LEVEL(1)					\
+		INIT_CALLS_LEVEL(2)					\
+		INIT_CALLS_LEVEL(3)					\
+		INIT_CALLS_LEVEL(4)					\
+		INIT_CALLS_LEVEL(5)					\
+		INIT_CALLS_LEVEL(rootfs)				\
+		INIT_CALLS_LEVEL(6)					\
+		INIT_CALLS_LEVEL(7)					\
 		VMLINUX_SYMBOL(__initcall_end) = .;
 
 #define CON_INITCALL							\
diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
index 7939f63..7f4c9b5 100644
--- a/include/linux/moduleparam.h
+++ b/include/linux/moduleparam.h
@@ -48,7 +48,14 @@ struct kernel_param_ops {
 };
 
 /* Flag bits for kernel_param.flags */
-#define KPARAM_ISBOOL		2
+#define KPARAM_ISBOOL		(1 << 1)
+#define KPARAM_ISLEVEL		(1 << 2)
+#define KPARAM_LEVEL_SHIFT	3
+#define KPARAM_LEVEL_MASK	(7 << KPARAM_LEVEL_SHIFT)
+
+#define __kparam_isbool(arg)	(__same_type((arg), bool) ? KPARAM_ISBOOL : 0)
+#define __kparam_isboolp(arg)	(__same_type((arg), bool *) ? KPARAM_ISBOOL : 0)
+#define __kparam_level(level)	(KPARAM_ISLEVEL | (level) << KPARAM_LEVEL_SHIFT)
 
 struct kernel_param {
 	const char *name;
@@ -132,7 +139,42 @@ struct kparam_array
  */
 #define module_param_cb(name, ops, arg, perm)				      \
 	__module_param_call(MODULE_PARAM_PREFIX,			      \
-			    name, ops, arg, __same_type((arg), bool *), perm)
+			    name, ops, arg, __kparam_isboolp(arg), perm)
+
+/**
+ * <level>_param_cb - general callback for a module/cmdline parameter
+ *                    to be evaluated before certain initcall level
+ * @name: a valid C identifier which is the parameter name.
+ * @ops: the set & get operations for this parameter.
+ * @perm: visibility in sysfs.
+ *
+ * The ops can have NULL set or get functions.
+ */
+#define __level_param_cb(level, name, ops, arg, perm)			      \
+	__module_param_call(MODULE_PARAM_PREFIX,			      \
+			    name, ops, arg,				      \
+			    __kparam_isboolp(arg) | __kparam_level(level), perm)
+
+#define core_param_cb(name, ops, arg, perm)				      \
+	__level_param_cb(1, name, ops, arg, perm)
+
+#define postcore_param_cb(name, ops, arg, perm)				      \
+	__level_param_cb(2, name, ops, arg, perm)
+
+#define arch_param_cb(name, ops, arg, perm)				      \
+	__level_param_cb(3, name, ops, arg, perm)
+
+#define subsys_param_cb(name, ops, arg, perm)				      \
+	__level_param_cb(4, name, ops, arg, perm)
+
+#define fs_param_cb(name, ops, arg, perm)				      \
+	__level_param_cb(5, name, ops, arg, perm)
+
+#define device_param_cb(name, ops, arg, perm)				      \
+	__level_param_cb(6, name, ops, arg, perm)
+
+#define late_param_cb(name, ops, arg, perm)				      \
+	__level_param_cb(7, name, ops, arg, perm)
 
 /* On alpha, ia64 and ppc64 relocations to global data cannot go into
    read-only sections (which is part of respective UNIX ABI on these
@@ -146,7 +188,7 @@ struct kparam_array
 
 /* This is the fundamental function for registering boot/module
    parameters. */
-#define __module_param_call(prefix, name, ops, arg, isbool, perm)	\
+#define __module_param_call(prefix, name, ops, arg, flags, perm)	\
 	/* Default value instead of permissions? */			\
 	static int __param_perm_check_##name __attribute__((unused)) =	\
 	BUILD_BUG_ON_ZERO((perm) < 0 || (perm) > 0777 || ((perm) & 2))	\
@@ -155,8 +197,7 @@ struct kparam_array
 	static struct kernel_param __moduleparam_const __param_##name	\
 	__used								\
     __attribute__ ((unused,__section__ ("__param"),aligned(sizeof(void *)))) \
-	= { __param_str_##name, ops, perm, isbool ? KPARAM_ISBOOL : 0,	\
-	    { arg } }
+	= { __param_str_##name, ops, perm, flags, { arg } }
 
 /* Obsolete - use module_param_cb() */
 #define module_param_call(name, set, get, arg, perm)			\
@@ -164,7 +205,7 @@ struct kparam_array
 		 { (void *)set, (void *)get };				\
 	__module_param_call(MODULE_PARAM_PREFIX,			\
 			    name, &__param_ops_##name, arg,		\
-			    __same_type(arg, bool *),			\
+			    __kparam_isboolp(arg),			\
 			    (perm) + sizeof(__check_old_set_param(set))*0)
 
 /* We don't get oldget: it's often a new-style param_get_uint, etc. */
@@ -246,7 +287,7 @@ static inline void __kernel_param_unlock(void)
 #define core_param(name, var, type, perm)				\
 	param_check_##type(name, &(var));				\
 	__module_param_call("", name, &param_ops_##type,		\
-			    &var, __same_type(var, bool), perm)
+			    &var, __kparam_isbool(var), perm)
 #endif /* !MODULE */
 
 /**
@@ -292,6 +333,8 @@ extern int parse_args(const char *name,
 		      char *args,
 		      const struct kernel_param *params,
 		      unsigned num,
+		      u16 flags_mask,
+		      u16 flags,
 		      int (*unknown)(char *param, char *val));
 
 /* Called by module remove. */
@@ -402,7 +445,7 @@ extern int param_get_invbool(char *buffer, const struct kernel_param *kp);
 	__module_param_call(MODULE_PARAM_PREFIX, name,			\
 			    &param_array_ops,				\
 			    .arr = &__param_arr_##name,			\
-			    __same_type(array[0], bool), perm);		\
+			    __kparam_isbool(array[0]), perm);		\
 	__MODULE_PARM_TYPE(name, "array of " #type)
 
 extern struct kernel_param_ops param_array_ops;
diff --git a/init/main.c b/init/main.c
index 217ed23..a7838c8 100644
--- a/init/main.c
+++ b/init/main.c
@@ -407,7 +407,7 @@ static int __init do_early_param(char *param, char *val)
 
 void __init parse_early_options(char *cmdline)
 {
-	parse_args("early options", cmdline, NULL, 0, do_early_param);
+	parse_args("early options", cmdline, NULL, 0, 0, 0, do_early_param);
 }
 
 /* Arch code calls this early on, or if not, just before other parsing. */
@@ -511,7 +511,7 @@ asmlinkage void __init start_kernel(void)
 	parse_early_param();
 	parse_args("Booting kernel", static_command_line, __start___param,
 		   __stop___param - __start___param,
-		   &unknown_bootoption);
+		   KPARAM_ISLEVEL, 0, &unknown_bootoption);
 
 	jump_label_init();
 
@@ -708,16 +708,70 @@ int __init_or_module do_one_initcall(initcall_t fn)
 }
 
 
-extern initcall_t __initcall_start[], __initcall_end[], __early_initcall_end[];
+extern initcall_t __initcall_start[];
+extern initcall_t __initcall0_start[];
+extern initcall_t __initcall1_start[];
+extern initcall_t __initcall2_start[];
+extern initcall_t __initcall3_start[];
+extern initcall_t __initcall4_start[];
+extern initcall_t __initcall5_start[];
+extern initcall_t __initcall6_start[];
+extern initcall_t __initcall7_start[];
+extern initcall_t __initcall_end[];
+
+static initcall_t *initcall_levels[] __initdata = {
+	__initcall0_start,
+	__initcall1_start,
+	__initcall2_start,
+	__initcall3_start,
+	__initcall4_start,
+	__initcall5_start,
+	__initcall6_start,
+	__initcall7_start,
+	__initcall_end,
+};
+
+static char *initcall_level_names[] __initdata = {
+	"core parameters",
+	"postcore parameters",
+	"arch parameters",
+	"subsys parameters",
+	"fs parameters",
+	"device parameters",
+	"late parameters",
+};
+
+static int __init ignore_unknown_bootoption(char *param, char *val)
+{
+	return 0;
+}
 
-static void __init do_initcalls(void)
+static void __init do_initcall_level(int level)
 {
+	extern const struct kernel_param __start___param[], __stop___param[];
 	initcall_t *fn;
 
-	for (fn = __early_initcall_end; fn < __initcall_end; fn++)
+	strcpy(static_command_line, saved_command_line);
+	parse_args(initcall_level_names[level],
+			static_command_line, __start___param,
+			__stop___param - __start___param,
+			KPARAM_ISLEVEL | KPARAM_LEVEL_MASK,
+			__kparam_level(level),
+			ignore_unknown_bootoption);
+
+	for (fn = initcall_levels[level]; fn < initcall_levels[level + 1];
+			fn++)
 		do_one_initcall(*fn);
 }
 
+static void __init do_initcalls(void)
+{
+	int level;
+
+	for (level = 0; level < ARRAY_SIZE(initcall_levels) - 1; level++)
+		do_initcall_level(level);
+}
+
 /*
  * Ok, the machine is now initialized. None of the devices
  * have been touched yet, but the CPU subsystem is up and
@@ -741,7 +795,7 @@ static void __init do_pre_smp_initcalls(void)
 {
 	initcall_t *fn;
 
-	for (fn = __initcall_start; fn < __early_initcall_end; fn++)
+	for (fn = __initcall_start; fn < __initcall0_start; fn++)
 		do_one_initcall(*fn);
 }
 
diff --git a/kernel/module.c b/kernel/module.c
index 178333c..66d3e75 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2893,7 +2893,8 @@ static struct module *load_module(void __user *umod,
 	mutex_unlock(&module_mutex);
 
 	/* Module is ready to execute: parsing args may do that. */
-	err = parse_args(mod->name, mod->args, mod->kp, mod->num_kp, NULL);
+	err = parse_args(mod->name, mod->args, mod->kp, mod->num_kp,
+			 0, 0, NULL);
 	if (err < 0)
 		goto unlink;
 
diff --git a/kernel/params.c b/kernel/params.c
index 65aae11..ddde6a7 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -94,6 +94,8 @@ static int parse_one(char *param,
 		     char *val,
 		     const struct kernel_param *params,
 		     unsigned num_params,
+		     int flags_mask,
+		     int flags,
 		     int (*handle_unknown)(char *param, char *val))
 {
 	unsigned int i;
@@ -102,6 +104,8 @@ static int parse_one(char *param,
 	/* Find parameter */
 	for (i = 0; i < num_params; i++) {
 		if (parameq(param, params[i].name)) {
+			if ((params[i].flags & flags_mask) != flags)
+				return 0;
 			/* No one handled NULL, so do it here. */
 			if (!val && params[i].ops->set != param_set_bool)
 				return -EINVAL;
@@ -180,6 +184,8 @@ int parse_args(const char *name,
 	       char *args,
 	       const struct kernel_param *params,
 	       unsigned num,
+	       u16 flags_mask,
+	       u16 flags,
 	       int (*unknown)(char *param, char *val))
 {
 	char *param, *val;
@@ -195,7 +201,8 @@ int parse_args(const char *name,
 
 		args = next_arg(args, &param, &val);
 		irq_was_disabled = irqs_disabled();
-		ret = parse_one(param, val, params, num, unknown);
+		ret = parse_one(param, val, params, num,
+				flags_mask, flags, unknown);
 		if (irq_was_disabled && !irqs_disabled()) {
 			printk(KERN_WARNING "parse_args(): option '%s' enabled "
 					"irq's!\n", param);
-- 
1.7.5.4

[PATCH 2/2] virtio-mmio: Devices parameter parsing

[Pawel Moll]

This patch adds an option to instantiate guest virtio-mmio devices
basing on a kernel command line (or module) parameter, for example:

	virtio_mmio.devices=0x100@0x100b0000:48

Signed-off-by: Pawel Moll <pawel.moll@arm.com>
---
 Documentation/kernel-parameters.txt |   17 ++++
 drivers/virtio/Kconfig              |   11 +++
 drivers/virtio/virtio_mmio.c        |  163 +++++++++++++++++++++++++++++++++++
 3 files changed, 191 insertions(+), 0 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index a0c5c5f..c155d13 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -110,6 +110,7 @@ parameter is applicable:
 	USB	USB support is enabled.
 	USBHID	USB Human Interface Device support is enabled.
 	V4L	Video For Linux support is enabled.
+	VMMIO   Driver for memory mapped virtio devices is enabled.
 	VGA	The VGA console has been enabled.
 	VT	Virtual terminal support is enabled.
 	WDT	Watchdog support is enabled.
@@ -2720,6 +2721,22 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 	video=		[FB] Frame buffer configuration
 			See Documentation/fb/modedb.txt.
 
+	virtio_mmio.device=
+			[VMMIO] Memory mapped virtio (platform) device.
+
+				<size>@<baseaddr>:<irq>[:<id>]
+			where:
+				<size>     := size (can use standard suffixes
+						like K, M and G)
+				<baseaddr> := physical base address
+				<irq>      := interrupt number (as passed to
+						request_irq())
+				<id>       := (optional) platform device id
+			example:
+				virtio_mmio.device=1K@0x100b0000:48:7
+
+			Can be used multiple times for multiple devices.
+
 	vga=		[BOOT,X86-32] Select a particular video mode
 			See Documentation/x86/boot.txt and
 			Documentation/svga.txt.
diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
index 1a61939..f38b17a 100644
--- a/drivers/virtio/Kconfig
+++ b/drivers/virtio/Kconfig
@@ -46,4 +46,15 @@ config VIRTIO_BALLOON
 
  	 If unsure, say N.
 
+config VIRTIO_MMIO_CMDLINE_DEVICES
+	bool "Memory mapped virtio devices parameter parsing"
+	depends on VIRTIO_MMIO
+	---help---
+	 Allow virtio-mmio devices instantiation via the kernel command line
+	 or module parameters. Be aware that using incorrect parameters (base
+	 address in particular) can crash your system - you have been warned.
+	 See Documentation/kernel-parameters.txt for details.
+
+	 If unsure, say 'N'.
+
 endmenu
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 7317dc2..eab0007 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -6,6 +6,50 @@
  * This module allows virtio devices to be used over a virtual, memory mapped
  * platform device.
  *
+ * The guest device(s) may be instantiated in one of three equivalent ways:
+ *
+ * 1. Static platform device in board's code, eg.:
+ *
+ *	static struct platform_device v2m_virtio_device = {
+ *		.name = "virtio-mmio",
+ *		.id = -1,
+ *		.num_resources = 2,
+ *		.resource = (struct resource []) {
+ *			{
+ *				.start = 0x1001e000,
+ *				.end = 0x1001e0ff,
+ *				.flags = IORESOURCE_MEM,
+ *			}, {
+ *				.start = 42 + 32,
+ *				.end = 42 + 32,
+ *				.flags = IORESOURCE_IRQ,
+ *			},
+ *		}
+ *	};
+ *
+ * 2. Device Tree node, eg.:
+ *
+ *		virtio_block@1e000 {
+ *			compatible = "virtio,mmio";
+ *			reg = <0x1e000 0x100>;
+ *			interrupts = <42>;
+ *		}
+ *
+ * 3. Kernel module (or command line) parameter. Can be used more than once -
+ *    one device will be created for each one. Syntax:
+ *
+ *		[virtio_mmio.]device=<size>@<baseaddr>:<irq>[:<id>]
+ *    where:
+ *		<size>     := size (can use standard suffixes like K, M or G)
+ *		<baseaddr> := physical base address
+ *		<irq>      := interrupt number (as passed to request_irq())
+ *		<id>       := (optional) platform device id
+ *    eg.:
+ *		virtio_mmio.device=0x100@0x100b0000:48 \
+ *				virtio_mmio.device=1K@0x1001e000:74
+ *
+ *
+ *
  * Registers layout (all 32-bit wide):
  *
  * offset d. name             description
@@ -42,6 +86,8 @@
  * See the COPYING file in the top-level directory.
  */
 
+#define pr_fmt(fmt) "virtio-mmio: " fmt
+
 #include <linux/highmem.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
@@ -443,6 +489,122 @@ static int __devexit virtio_mmio_remove(struct platform_device *pdev)
 
 
 
+/* Devices list parameter */
+
+#if defined(CONFIG_VIRTIO_MMIO_CMDLINE_DEVICES)
+
+static struct device vm_cmdline_parent = {
+	.init_name = "virtio-mmio-cmdline",
+};
+
+static int vm_cmdline_parent_registered;
+static int vm_cmdline_id;
+
+static int vm_cmdline_set(const char *device,
+		const struct kernel_param *kp)
+{
+	int err;
+	struct resource resources[2] = {};
+	char *str;
+	long long int base;
+	int processed, consumed = 0;
+	struct platform_device *pdev;
+
+	resources[0].flags = IORESOURCE_MEM;
+	resources[1].flags = IORESOURCE_IRQ;
+
+	resources[0].end = memparse(device, &str) - 1;
+
+	processed = sscanf(str, "@%lli:%u%n:%d%n",
+			&base, &resources[1].start, &consumed,
+			&vm_cmdline_id, &consumed);
+
+	if (processed < 2 || processed > 3 || str[consumed])
+		return -EINVAL;
+
+	resources[0].start = base;
+	resources[0].end += base;
+	resources[1].end = resources[1].start;
+
+	if (!vm_cmdline_parent_registered) {
+		err = device_register(&vm_cmdline_parent);
+		if (err) {
+			pr_err("Failed to register parent device!\n");
+			return err;
+		}
+		vm_cmdline_parent_registered = 1;
+	}
+
+	pr_info("Registering device virtio-mmio.%d at 0x%llx-0x%llx, IRQ %d.\n",
+		       vm_cmdline_id++,
+		       (unsigned long long)resources[0].start,
+		       (unsigned long long)resources[0].end,
+		       (int)resources[1].start);
+
+	pdev = platform_device_register_resndata(&vm_cmdline_parent,
+			"virtio-mmio", vm_cmdline_id++,
+			resources, ARRAY_SIZE(resources), NULL, 0);
+	if (IS_ERR(pdev))
+		return PTR_ERR(pdev);
+
+	return 0;
+}
+
+static int vm_cmdline_get_device(struct device *dev, void *data)
+{
+	char *buffer = data;
+	unsigned int len = strlen(buffer);
+	struct platform_device *pdev = to_platform_device(dev);
+
+	snprintf(buffer + len, PAGE_SIZE - len, "0x%llx@0x%llx:%llu:%d\n",
+			pdev->resource[0].end - pdev->resource[0].start + 1ULL,
+			(unsigned long long)pdev->resource[0].start,
+			(unsigned long long)pdev->resource[1].start,
+			pdev->id);
+	return 0;
+}
+
+static int vm_cmdline_get(char *buffer, const struct kernel_param *kp)
+{
+	buffer[0] = '\0';
+	device_for_each_child(&vm_cmdline_parent, buffer,
+			vm_cmdline_get_device);
+	return strlen(buffer) + 1;
+}
+
+static struct kernel_param_ops vm_cmdline_param_ops = {
+	.set = vm_cmdline_set,
+	.get = vm_cmdline_get,
+};
+
+device_param_cb(device, &vm_cmdline_param_ops, NULL, S_IRUSR);
+
+static int vm_unregister_cmdline_device(struct device *dev,
+		void *data)
+{
+	platform_device_unregister(to_platform_device(dev));
+
+	return 0;
+}
+
+static void vm_unregister_cmdline_devices(void)
+{
+	if (vm_cmdline_parent_registered) {
+		device_for_each_child(&vm_cmdline_parent, NULL,
+				vm_unregister_cmdline_device);
+		device_unregister(&vm_cmdline_parent);
+		vm_cmdline_parent_registered = 0;
+	}
+}
+
+#else
+
+static void vm_unregister_cmdline_devices(void)
+{
+}
+
+#endif
+
 /* Platform driver */
 
 static struct of_device_id virtio_mmio_match[] = {
@@ -469,6 +631,7 @@ static int __init virtio_mmio_init(void)
 static void __exit virtio_mmio_exit(void)
 {
 	platform_driver_unregister(&virtio_mmio_driver);
+	vm_unregister_cmdline_devices();
 }
 
 module_init(virtio_mmio_init);
-- 
1.7.5.4

Re: [PATCH 2/2] virtio-mmio: Devices parameter parsing

[Sasha Levin]

Ping? Was this patch forgotten?

On Mon, Dec 12, 2011 at 6:57 PM, Pawel Moll <pawel.moll@arm.com> wrote:
> This patch adds an option to instantiate guest virtio-mmio devices
> basing on a kernel command line (or module) parameter, for example:
>
>        virtio_mmio.devices=0x100@0x100b0000:48
>
> Signed-off-by: Pawel Moll <pawel.moll@arm.com>
> ---
>  Documentation/kernel-parameters.txt |   17 ++++
>  drivers/virtio/Kconfig              |   11 +++
>  drivers/virtio/virtio_mmio.c        |  163 +++++++++++++++++++++++++++++++++++
>  3 files changed, 191 insertions(+), 0 deletions(-)
>
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index a0c5c5f..c155d13 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -110,6 +110,7 @@ parameter is applicable:
>        USB     USB support is enabled.
>        USBHID  USB Human Interface Device support is enabled.
>        V4L     Video For Linux support is enabled.
> +       VMMIO   Driver for memory mapped virtio devices is enabled.
>        VGA     The VGA console has been enabled.
>        VT      Virtual terminal support is enabled.
>        WDT     Watchdog support is enabled.
> @@ -2720,6 +2721,22 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>        video=          [FB] Frame buffer configuration
>                        See Documentation/fb/modedb.txt.
>
> +       virtio_mmio.device=
> +                       [VMMIO] Memory mapped virtio (platform) device.
> +
> +                               <size>@<baseaddr>:<irq>[:<id>]
> +                       where:
> +                               <size>     := size (can use standard suffixes
> +                                               like K, M and G)
> +                               <baseaddr> := physical base address
> +                               <irq>      := interrupt number (as passed to
> +                                               request_irq())
> +                               <id>       := (optional) platform device id
> +                       example:
> +                               virtio_mmio.device=1K@0x100b0000:48:7
> +
> +                       Can be used multiple times for multiple devices.
> +
>        vga=            [BOOT,X86-32] Select a particular video mode
>                        See Documentation/x86/boot.txt and
>                        Documentation/svga.txt.
> diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> index 1a61939..f38b17a 100644
> --- a/drivers/virtio/Kconfig
> +++ b/drivers/virtio/Kconfig
> @@ -46,4 +46,15 @@ config VIRTIO_BALLOON
>
>         If unsure, say N.
>
> +config VIRTIO_MMIO_CMDLINE_DEVICES
> +       bool "Memory mapped virtio devices parameter parsing"
> +       depends on VIRTIO_MMIO
> +       ---help---
> +        Allow virtio-mmio devices instantiation via the kernel command line
> +        or module parameters. Be aware that using incorrect parameters (base
> +        address in particular) can crash your system - you have been warned.
> +        See Documentation/kernel-parameters.txt for details.
> +
> +        If unsure, say 'N'.
> +
>  endmenu
> diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
> index 7317dc2..eab0007 100644
> --- a/drivers/virtio/virtio_mmio.c
> +++ b/drivers/virtio/virtio_mmio.c
> @@ -6,6 +6,50 @@
>  * This module allows virtio devices to be used over a virtual, memory mapped
>  * platform device.
>  *
> + * The guest device(s) may be instantiated in one of three equivalent ways:
> + *
> + * 1. Static platform device in board's code, eg.:
> + *
> + *     static struct platform_device v2m_virtio_device = {
> + *             .name = "virtio-mmio",
> + *             .id = -1,
> + *             .num_resources = 2,
> + *             .resource = (struct resource []) {
> + *                     {
> + *                             .start = 0x1001e000,
> + *                             .end = 0x1001e0ff,
> + *                             .flags = IORESOURCE_MEM,
> + *                     }, {
> + *                             .start = 42 + 32,
> + *                             .end = 42 + 32,
> + *                             .flags = IORESOURCE_IRQ,
> + *                     },
> + *             }
> + *     };
> + *
> + * 2. Device Tree node, eg.:
> + *
> + *             virtio_block@1e000 {
> + *                     compatible = "virtio,mmio";
> + *                     reg = <0x1e000 0x100>;
> + *                     interrupts = <42>;
> + *             }
> + *
> + * 3. Kernel module (or command line) parameter. Can be used more than once -
> + *    one device will be created for each one. Syntax:
> + *
> + *             [virtio_mmio.]device=<size>@<baseaddr>:<irq>[:<id>]
> + *    where:
> + *             <size>     := size (can use standard suffixes like K, M or G)
> + *             <baseaddr> := physical base address
> + *             <irq>      := interrupt number (as passed to request_irq())
> + *             <id>       := (optional) platform device id
> + *    eg.:
> + *             virtio_mmio.device=0x100@0x100b0000:48 \
> + *                             virtio_mmio.device=1K@0x1001e000:74
> + *
> + *
> + *
>  * Registers layout (all 32-bit wide):
>  *
>  * offset d. name             description
> @@ -42,6 +86,8 @@
>  * See the COPYING file in the top-level directory.
>  */
>
> +#define pr_fmt(fmt) "virtio-mmio: " fmt
> +
>  #include <linux/highmem.h>
>  #include <linux/interrupt.h>
>  #include <linux/io.h>
> @@ -443,6 +489,122 @@ static int __devexit virtio_mmio_remove(struct platform_device *pdev)
>
>
>
> +/* Devices list parameter */
> +
> +#if defined(CONFIG_VIRTIO_MMIO_CMDLINE_DEVICES)
> +
> +static struct device vm_cmdline_parent = {
> +       .init_name = "virtio-mmio-cmdline",
> +};
> +
> +static int vm_cmdline_parent_registered;
> +static int vm_cmdline_id;
> +
> +static int vm_cmdline_set(const char *device,
> +               const struct kernel_param *kp)
> +{
> +       int err;
> +       struct resource resources[2] = {};
> +       char *str;
> +       long long int base;
> +       int processed, consumed = 0;
> +       struct platform_device *pdev;
> +
> +       resources[0].flags = IORESOURCE_MEM;
> +       resources[1].flags = IORESOURCE_IRQ;
> +
> +       resources[0].end = memparse(device, &str) - 1;
> +
> +       processed = sscanf(str, "@%lli:%u%n:%d%n",
> +                       &base, &resources[1].start, &consumed,
> +                       &vm_cmdline_id, &consumed);
> +
> +       if (processed < 2 || processed > 3 || str[consumed])
> +               return -EINVAL;
> +
> +       resources[0].start = base;
> +       resources[0].end += base;
> +       resources[1].end = resources[1].start;
> +
> +       if (!vm_cmdline_parent_registered) {
> +               err = device_register(&vm_cmdline_parent);
> +               if (err) {
> +                       pr_err("Failed to register parent device!\n");
> +                       return err;
> +               }
> +               vm_cmdline_parent_registered = 1;
> +       }
> +
> +       pr_info("Registering device virtio-mmio.%d at 0x%llx-0x%llx, IRQ %d.\n",
> +                      vm_cmdline_id++,
> +                      (unsigned long long)resources[0].start,
> +                      (unsigned long long)resources[0].end,
> +                      (int)resources[1].start);
> +
> +       pdev = platform_device_register_resndata(&vm_cmdline_parent,
> +                       "virtio-mmio", vm_cmdline_id++,
> +                       resources, ARRAY_SIZE(resources), NULL, 0);
> +       if (IS_ERR(pdev))
> +               return PTR_ERR(pdev);
> +
> +       return 0;
> +}
> +
> +static int vm_cmdline_get_device(struct device *dev, void *data)
> +{
> +       char *buffer = data;
> +       unsigned int len = strlen(buffer);
> +       struct platform_device *pdev = to_platform_device(dev);
> +
> +       snprintf(buffer + len, PAGE_SIZE - len, "0x%llx@0x%llx:%llu:%d\n",
> +                       pdev->resource[0].end - pdev->resource[0].start + 1ULL,
> +                       (unsigned long long)pdev->resource[0].start,
> +                       (unsigned long long)pdev->resource[1].start,
> +                       pdev->id);
> +       return 0;
> +}
> +
> +static int vm_cmdline_get(char *buffer, const struct kernel_param *kp)
> +{
> +       buffer[0] = '\0';
> +       device_for_each_child(&vm_cmdline_parent, buffer,
> +                       vm_cmdline_get_device);
> +       return strlen(buffer) + 1;
> +}
> +
> +static struct kernel_param_ops vm_cmdline_param_ops = {
> +       .set = vm_cmdline_set,
> +       .get = vm_cmdline_get,
> +};
> +
> +device_param_cb(device, &vm_cmdline_param_ops, NULL, S_IRUSR);
> +
> +static int vm_unregister_cmdline_device(struct device *dev,
> +               void *data)
> +{
> +       platform_device_unregister(to_platform_device(dev));
> +
> +       return 0;
> +}
> +
> +static void vm_unregister_cmdline_devices(void)
> +{
> +       if (vm_cmdline_parent_registered) {
> +               device_for_each_child(&vm_cmdline_parent, NULL,
> +                               vm_unregister_cmdline_device);
> +               device_unregister(&vm_cmdline_parent);
> +               vm_cmdline_parent_registered = 0;
> +       }
> +}
> +
> +#else
> +
> +static void vm_unregister_cmdline_devices(void)
> +{
> +}
> +
> +#endif
> +
>  /* Platform driver */
>
>  static struct of_device_id virtio_mmio_match[] = {
> @@ -469,6 +631,7 @@ static int __init virtio_mmio_init(void)
>  static void __exit virtio_mmio_exit(void)
>  {
>        platform_driver_unregister(&virtio_mmio_driver);
> +       vm_unregister_cmdline_devices();
>  }
>
>  module_init(virtio_mmio_init);
> --
> 1.7.5.4
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/2] virtio-mmio: Devices parameter parsing

[Pawel Moll]

> On Mon, Dec 12, 2011 at 6:57 PM, Pawel Moll <pawel.moll@arm.com> wrote:
> > This patch adds an option to instantiate guest virtio-mmio devices
> > basing on a kernel command line (or module) parameter, for example:
> >
> >        virtio_mmio.devices=0x100@0x100b0000:48
> >
> > Signed-off-by: Pawel Moll <pawel.moll@arm.com>

On Mon, 2012-04-09 at 17:32 +0100, Sasha Levin wrote:
> Ping? Was this patch forgotten?

Damn, it has been forgetten indeed. It took a while to get the required
"<level>-param" patch got merged and then it slipped through cracks...

I'll re-test it against v3.4-rc and make sure it gets queued for 3.5
(3.4 is unlikely, especially now when Rusty is honeymooning ;-)

Thanks for the reminder and sorry about the delay!

Paweł


_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/2] params: <level>_initcall-like kernel parameters

[Rusty Russell]

On Mon, 12 Dec 2011 17:57:06 +0000, Pawel Moll <pawel.moll@arm.com> wrote:
> This patch adds a set of macros that can be used to declare
> kernel parameters to be parsed _before_ initcalls at a chosen
> level are executed. Such parameters are marked using existing
> "flags" field of the "kernel_param" structure.
> 
> Linker macro collating init calls had to be modified in order
> to add additional symbols between levels that are later used
> by the init code to split the calls into blocks.

This patch wasn't quite what I was thinking, but I've realized that
we can't put the params in the .init section, your approach is probably
the best one.

Note that I've just created a series which gets rid of that silly ISBOOL
thing, so you can use the whole field for "level".  Then I set the level
to -1 for the normal calls; I want to use -2 for the early calls, but
that's not done yet...

I'll rework and rebase your patch like that now.

Thanks!
Rusty.

Re: [PATCH 1/2] params: <level>_initcall-like kernel parameters

[Pawel Moll]

Morning,

On Thu, 2011-12-15 at 03:51 +0000, Rusty Russell wrote:
> On Mon, 12 Dec 2011 17:57:06 +0000, Pawel Moll <pawel.moll@arm.com> wrote:
> > This patch adds a set of macros that can be used to declare
> > kernel parameters to be parsed _before_ initcalls at a chosen
> > level are executed. Such parameters are marked using existing
> > "flags" field of the "kernel_param" structure.
> > 
> > Linker macro collating init calls had to be modified in order
> > to add additional symbols between levels that are later used
> > by the init code to split the calls into blocks.
> 
> This patch wasn't quite what I was thinking, but I've realized that
> we can't put the params in the .init section, your approach is probably
> the best one.

The only way I could think of to put the parameters passing code in
between levels was adding new linker sections, and that sounded like an
overkill...

> Note that I've just created a series which gets rid of that silly ISBOOL
> thing, so you can use the whole field for "level".  Then I set the level
> to -1 for the normal calls; I want to use -2 for the early calls, but
> that's not done yet...
> 
> I'll rework and rebase your patch like that now.

Cool, it's all yours, especially now that I'm the last day at work this
year so won't be able to contribute much in the following weeks... Could
I just ask you to remember about the virtio_mmio parameters patch if you
get somewhere with this? I'll be most grateful!

Cheers!

Paweł


_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] virtio-mmio: Devices parameter parsing

[Rusty Russell]

On Mon, 21 Nov 2011 14:44:48 +0000, Pawel Moll <pawel.moll@arm.com> wrote:
> On Mon, 2011-11-21 at 03:32 +0000, Rusty Russell wrote:
> > 2) Doesn't leak mem on error paths.
> 
> Em, I don't think my code was leaking memory in any error case? (no
> offence meant or taken ;-)

You're right, I'm wrong.  In your original version, your innovative use
of free() then strdup() worked as intended.

> > 3) Handles error from platform_device_register_resndata().
> 
> As my code was ignoring wrong descriptions (all the continues) I ignored
> this result as well (the implementation complains on its own anyway),
> but I get your point.

I was referring to this:

	  return platform_device_register_resndata(&virtio_mmio_cmdline_parent,
			  "virtio-mmio", virtio_mmio_cmdline_id++,
			  resources, ARRAY_SIZE(resources), NULL, 0) != NULL;

The set function returns 0 or a -ve errno, not true/false.

> > 4) Uses shorter names for static functions/variables.
> 
> Ah, I get the hint :-) I'm trying to keep the naming convention in
> static symbols as well, as it makes cscope/ctags/grep usage easier...
> I'll just use the abbreviated "vm_" prefixes then.
> 
> > See what you think...
> 
> Funnily enough when I proposed some string parser few years ago (totally
> different story) I was flamed for using strchr() instead of strsep() ;-)
> But ok, I don't mind getting back to basics.
> 
> > +static int set_cmdline_device(const char *device, const struct kernel_param *kp)
> [...]
> > +       delim = strchr(device, '@');
> [...]
> > +	*delim = '\0';
> 
> Ah. I forgot that strchr() takes const char * but returns "non-const"
> char *... Cheating, that's what it is ;-), but will work. Probably what
> we really want is something like
> 	kstrtoull_delim(device, 0, &val, "@\0")
> I'll have a look and may try to propose something of that sort, but
> that's another story. Maybe I should just use simple_strtol() for the
> time being?

Or would it be simpler to enhance sscanf() with some weird format option
for suffixing?  I haven't looked for similar cases, but I'd suspect a
big win in general.

This would be neater than anything else we've got:
        if (sscanf(device, "%llu@%llu[KMG]:%u", ...) != 3
            && sscanf(device, "%llu@%llu[KMG]:%u:%u", ...) != 4)
                return -EINVAL;

Cheers,
Rusty.