Re: [PATCH] module: add modinfo support for all built-in modules

[Dave Young]

On Jan 18, 2008 5:54 PM, rae l <crquan@xxxxxxxxx> wrote:
> On Jan 16, 2008 8:25 PM, Rusty Russell <rusty@xxxxxxxxxxxxxxx> wrote:
> > I'd love to see patches.  module_parm showed it's possible, if messy.
> >
> > Thanks!
> > Rusty.
>
> here's the patch, I added .modinfo section to the vmlinux, to collect
> built-in module information.
>
> I have just define __MODULE_INFO to another meaning while
> CONFIG_MODULES undefined
> (modules compiled built-in), instead of nothing; and so the
> MODULE_LICENSE, MODULE_AUTHOR,
> MODULE_DESCRIPTION's meaning also changed, each macro would define one
> struct kernel_modinfo
> entry in the .modinfo section of vmlinux; and one __initcall converts
> all these information to read-only
> files under /sys/modules/<module-name>/...
>
> but the MODULE_PARM_DESC macro is still different:
> it generates entries with the same tag, that would confuse
> sys_create_group, so I skipped them in the __initcall,
> since the parameters had been in /sys/modules/<>/parameters/(with perm
> non-zero) or didn't appear(with perm 0);
> I think the parameter description might be only useful for external
> module files, not needed in memory(under /sys/module/),
> so a better solution is define MODULE_PARM_DESC to nothing while
> CONFIG_MODULES undefined.
>
> Another possible defect is that it compares two modname with
> (km->modname != modname),
> that depends on a gcc feature: keep same constant string only one copy
> in the image,
> this did work on my test machines, but I'm not sure it's standard or
> not; and if not, I would change it to strcmp.
>
> Apperantly this approach will increase the kernel image size. on a
> moderate system(with 1.8MB bzImage),
> this patch would increase vmlinux 46KB and after compression increase
> bzImage 9.2KB.
> and in the increment of vmlinux, the .modinfo section occupied 5.3KB
> and others are constant strings.
>
> However, the iscsid can now work well when scsi_transport_iscsi module
> built-in without the problem refered in my former email.
>
> please give comments.
>
> From 50831a260b1ad2c8b495854a58408c1fbc75a3fe Mon Sep 17 00:00:00 2001
> From: Denis Cheng <crquan@xxxxxxxxx>
> Date: Fri, 18 Jan 2008 16:37:35 +0800
> Subject: [PATCH] module: add modinfo support for all built-in modules
>
> the current modinfo support is for external modules only, it provided module
> information under /sys/module/<XYZ>/, such as verion, ...;
> now some application(such as iscsid of open-iscsi) has been designed to
> use this module information; but built-in modules don't have modinfo support,
> so these apps would break if modules they depend on are compiled built-in.
>
> this patch add modinfo support for all built-in modules, so now no matter
> whether modules they depends on are built-in or external, modules' information
> could always be accessed from /sys/module/<XYZ>/version, apps won't break.
>
> Signed-off-by: Denis Cheng <crquan@xxxxxxxxx>
> ---
>  include/asm-generic/vmlinux.lds.h |    7 ++
>  include/linux/moduleparam.h       |   18 ++++-
>  kernel/module.c                   |  147 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 170 insertions(+), 2 deletions(-)
>
> diff --git a/include/asm-generic/vmlinux.lds.h
> b/include/asm-generic/vmlinux.lds.h
> index 9f584cc..896f0fe 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -137,6 +137,13 @@
>                 VMLINUX_SYMBOL(__start___param) = .;                    \
>                 *(__param)                                              \
>                 VMLINUX_SYMBOL(__stop___param) = .;                     \
> +       }                                                               \
> +                                                                       \
> +       /* Built-in module information. */                              \
> +       .modinfo : AT(ADDR(.modinfo) - LOAD_OFFSET) {                   \
> +               VMLINUX_SYMBOL(__start___modinfo) = .;                  \
> +               *(.modinfo)                                             \
> +               VMLINUX_SYMBOL(__stop___modinfo) = .;                   \
>                 VMLINUX_SYMBOL(__end_rodata) = .;                       \
>         }                                                               \
>                                                                         \
> diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
> index 13410b2..86ddbd4 100644
> --- a/include/linux/moduleparam.h
> +++ b/include/linux/moduleparam.h
> @@ -13,16 +13,30 @@
>  #define MODULE_PARAM_PREFIX KBUILD_MODNAME "."
>  #endif
>
> -#ifdef MODULE
>  #define ___module_cat(a,b) __mod_ ## a ## b
>  #define __module_cat(a,b) ___module_cat(a,b)
> +
> +#ifdef MODULE
>  #define __MODULE_INFO(tag, name, info)                                   \
>  static const char __module_cat(name,__LINE__)[]                                  \
>    __attribute_used__                                                     \
>    __attribute__((section(".modinfo"),unused)) = __stringify(tag) "=" info
>  #else  /* !MODULE */
> -#define __MODULE_INFO(tag, name, info)
> +struct kernel_modinfo {
> +       char *modname;
> +       char *tag;
> +       char *info;
> +};
> +#define __MODULE_INFO(tag, name, info)                                   \
> +static const struct kernel_modinfo __module_cat(name, __LINE__)          \
> +  __attribute_used__                                                     \
> +  __attribute__((section(".modinfo"), unused)) =                         \
> +  { KBUILD_MODNAME, __stringify(tag), info }
> +
> +extern const struct kernel_modinfo __start___modinfo[], __stop___modinfo[];
> +
>  #endif
> +
>  #define __MODULE_PARM_TYPE(name, _type)                                          \
>    __MODULE_INFO(parmtype, name##type, #name ":" _type)
>
> diff --git a/kernel/module.c b/kernel/module.c
> index c2e3e2e..9828918 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2609,3 +2609,150 @@ void module_update_markers(struct module
> *probe_module, int *refcount)
>         mutex_unlock(&module_mutex);
>  }
>  #endif
> +
> +#ifdef CONFIG_SYSFS
> +struct modinfo_attribute {
> +       struct module_attribute mattr;
> +       const struct kernel_modinfo *modinfo;
> +};
> +
> +struct module_modinfo_attrs {
> +       struct attribute_group grp;
> +       struct modinfo_attribute attrs[0];
> +};
> +
> +#define to_modinfo_attr(n) container_of(n, struct modinfo_attribute, mattr)
> +
> +static ssize_t modinfo_attr_show(struct module_attribute *mattr,
> +               struct module *mod, char *buf)
> +{
> +       const struct kernel_modinfo *km = to_modinfo_attr(mattr)->modinfo;
> +       return snprintf(buf, PAGE_SIZE, "%s\n", km->info);
> +}
> +
> +/* skip the parameters' description in .modinfo */
> +static __init int modinfo_skip(char *name)
> +{
> +       return (strlen(name) == 4 && !strncmp(name, "parm", 4)) ||
> +               (strlen(name) == 8 && !strncmp(name, "parmtype", 8));
> +}
> +
> +static __init void modinfo_sysfs_setup(struct module_kobject *mk,
> +                 const struct kernel_modinfo km_begin[],
> +                 unsigned int num_params)
> +{
> +       struct module_modinfo_attrs *mp;
> +       unsigned int valid_attrs = 0;
> +       unsigned int i, size[2];
> +       struct modinfo_attribute *pattr;
> +       struct attribute **gattr;
> +
> +       for (i = 0; i < num_params; ++i) {
> +               if (!modinfo_skip(km_begin[i].tag))
> +                       valid_attrs++;
> +       }
> +
> +       if (!valid_attrs)
> +               return;
> +
> +       size[0] = ALIGN(sizeof(*mp) +
> +                       valid_attrs * sizeof(mp->attrs[0]),
> +                       sizeof(mp->grp.attrs[0]));
> +       size[1] = (valid_attrs + 1) * sizeof(mp->grp.attrs[0]);
> +
> +       mp = kzalloc(size[0] + size[1], GFP_KERNEL);
> +       if (!mp)
> +               return;
> +
> +       mp->grp.attrs = (void *)mp + size[0];
> +
> +       pattr = &mp->attrs[0];
> +       gattr = &mp->grp.attrs[0];
> +       for (i = 0; i < valid_attrs; i++) {
> +               const struct kernel_modinfo *km = &km_begin[i];
> +               if (!modinfo_skip(km_begin[i].tag)) {
> +                       pattr->modinfo = km;
> +                       pattr->mattr.show = modinfo_attr_show;
> +                       pattr->mattr.attr.name = km->tag;
> +                       pattr->mattr.attr.mode = S_IRUGO;
> +                       *(gattr++) = &(pattr++)->mattr.attr;
> +               }
> +       }
> +
> +       if (sysfs_create_group(&mk->kobj, &mp->grp))
> +               kfree(mp);
> +}
> +
> +static void __init kernel_modinfo_sysfs_setup(const char *modname,
> +               const struct kernel_modinfo km_begin[],
> +               unsigned int num_params)
> +{
> +       struct kobject *mkobj;
> +       struct module_kobject *mk;
> +       int ret;
> +
> +       mkobj = kset_find_obj(&module_subsys, modname);
> +
> +       if (mkobj)
> +               mk = container_of(mkobj, struct module_kobject, kobj);
> +       else {
> +               mk = kzalloc(sizeof(struct module_kobject), GFP_KERNEL);
> +
> +               kobj_set_kset_s(mk, module_subsys);
> +               kobject_set_name(&mk->kobj, modname);
> +               kobject_init(&mk->kobj);
> +               ret = kobject_add(&mk->kobj);
> +               if (ret) {
> +                       printk(KERN_ERR "Module '%s' failed to be added to"
> +                                       " sysfs, error number %d\n",
> +                                       modname, ret);
> +                       printk(KERN_ERR "The system will be unstable now.\n");

Maybe should put mk->kobj for release

> +                       return;
> +               }
> +       }
> +
> +       modinfo_sysfs_setup(mk, km_begin, num_params);
> +
> +       if (mkobj) {
> +               /* kset_find_obj incremental a reference on mkobj */
> +               kobject_put(mkobj);
> +       } else
> +               kobject_uevent(&mk->kobj, KOBJ_ADD);

The if/else could be merged to the above one.

> +}
> +
> +/*
> + * module_info_sysfs_init is for built-in modules
> + *
> + * __initcall("6") is later than subsys_init
> + */
> +static int __init builtin_modinfo_sysfs_init(void)
> +{
> +       const struct kernel_modinfo *km, *km_begin = NULL;
> +       unsigned int count = 0;
> +       char *modname = NULL;
> +
> +       for (km = __start___modinfo; km < __stop___modinfo; km++) {
> +               /* new kbuild_modname? */
> +               if (km->modname != modname) {
> +                       /* add a new kobject for previous kernel_modinfo . */
> +                       if (count)
> +                               kernel_modinfo_sysfs_setup(modname,
> +                                               km_begin,
> +                                               count);
> +
> +                       modname = km->modname;
> +                       count = 0;
> +                       km_begin = km;
> +               }
> +               count++;
> +       }
> +
> +       /* last kernel_modinfo need to be registered as well */
> +       if (count)
> +               kernel_modinfo_sysfs_setup(modname, km_begin, count);
> +
> +       return 0;
> +}
> +__initcall(builtin_modinfo_sysfs_init);
> +
> +#endif
> --
> 1.5.3.5
>
> --
> Denis Cheng
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/