On Wed, 2011-11-16 at 00:42 +0000, Rusty Russell wrote:
> On Tue, 15 Nov 2011 13:53:05 +0000, Pawel Moll <[email protected]> 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, ¶m_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
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
