From: Kay Sievers <[email protected]>
Subject: module: sysfs - add 'uevent' file to allow built-in coldplug
Userspace wants to manage module parameters with udev rules.
This currently only works for loaded modules, but not for
built-in ones.
To allow access to the built-in modules we need to
re-trigger all module load events that happened before any
userspace was running. We already do the same thing for all
devices, subsystems(buses) and drivers.
This adds the currently missing /sys/module/<name>/uevent files
to all module entries.
Signed-off-by: Kay Sievers <[email protected]>
---
include/linux/module.h | 24 +++++++++++++-----------
kernel/module.c | 31 ++++++++++++++++++++++++-------
kernel/params.c | 12 +++++++-----
3 files changed, 44 insertions(+), 23 deletions(-)
diff --git a/include/linux/module.h b/include/linux/module.h
index d9ca2d5..ac6975c 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -48,10 +48,18 @@ struct modversion_info
struct module;
+struct module_kobject
+{
+ struct kobject kobj;
+ struct module *mod;
+ struct kobject *drivers_dir;
+ struct module_param_attrs *mp;
+};
+
struct module_attribute {
- struct attribute attr;
- ssize_t (*show)(struct module_attribute *, struct module *, char *);
- ssize_t (*store)(struct module_attribute *, struct module *,
+ struct attribute attr;
+ ssize_t (*show)(struct module_attribute *, struct module_kobject *, char *);
+ ssize_t (*store)(struct module_attribute *, struct module_kobject *,
const char *, size_t count);
void (*setup)(struct module *, const char *);
int (*test)(struct module *);
@@ -65,15 +73,9 @@ struct module_version_attribute {
} __attribute__ ((__aligned__(sizeof(void *))));
extern ssize_t __modver_version_show(struct module_attribute *,
- struct module *, char *);
+ struct module_kobject *, char *);
-struct module_kobject
-{
- struct kobject kobj;
- struct module *mod;
- struct kobject *drivers_dir;
- struct module_param_attrs *mp;
-};
+extern struct module_attribute module_uevent;
/* These are either module local, or the kernel's dummy ones. */
extern int init_module(void);
diff --git a/kernel/module.c b/kernel/module.c
index 795bdc7..2f0d562 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -545,9 +545,9 @@ static void setup_modinfo_##field(struct module *mod, const char *s) \
mod->field = kstrdup(s, GFP_KERNEL); \
} \
static ssize_t show_modinfo_##field(struct module_attribute *mattr, \
- struct module *mod, char *buffer) \
+ struct module_kobject *mk, char *buffer) \
{ \
- return sprintf(buffer, "%s\n", mod->field); \
+ return sprintf(buffer, "%s\n", mk->mod->field); \
} \
static int modinfo_##field##_exists(struct module *mod) \
{ \
@@ -902,9 +902,9 @@ void symbol_put_addr(void *addr)
EXPORT_SYMBOL_GPL(symbol_put_addr);
static ssize_t show_refcnt(struct module_attribute *mattr,
- struct module *mod, char *buffer)
+ struct module_kobject *mk, char *buffer)
{
- return sprintf(buffer, "%u\n", module_refcount(mod));
+ return sprintf(buffer, "%u\n", module_refcount(mk->mod));
}
static struct module_attribute refcnt = {
@@ -952,11 +952,11 @@ static inline int module_unload_init(struct module *mod)
#endif /* CONFIG_MODULE_UNLOAD */
static ssize_t show_initstate(struct module_attribute *mattr,
- struct module *mod, char *buffer)
+ struct module_kobject *mk, char *buffer)
{
const char *state = "unknown";
- switch (mod->state) {
+ switch (mk->mod->state) {
case MODULE_STATE_LIVE:
state = "live";
break;
@@ -975,10 +975,27 @@ static struct module_attribute initstate = {
.show = show_initstate,
};
+static ssize_t store_uevent(struct module_attribute *mattr,
+ struct module_kobject *mk,
+ const char *buffer, size_t count)
+{
+ enum kobject_action action;
+
+ if (kobject_action_type(buffer, count, &action) == 0)
+ kobject_uevent(&mk->kobj, action);
+ return count;
+}
+
+struct module_attribute module_uevent = {
+ .attr = { .name = "uevent", .mode = 0200 },
+ .store = store_uevent,
+};
+
static struct module_attribute *modinfo_attrs[] = {
&modinfo_version,
&modinfo_srcversion,
&initstate,
+ &module_uevent,
#ifdef CONFIG_MODULE_UNLOAD
&refcnt,
#endif
@@ -1187,7 +1204,7 @@ struct module_sect_attrs
};
static ssize_t module_sect_show(struct module_attribute *mattr,
- struct module *mod, char *buf)
+ struct module_kobject *mk, char *buf)
{
struct module_sect_attr *sattr =
container_of(mattr, struct module_sect_attr, mattr);
diff --git a/kernel/params.c b/kernel/params.c
index ed72e13..d2f02ca 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -511,7 +511,7 @@ struct module_param_attrs
#define to_param_attr(n) container_of(n, struct param_attribute, mattr)
static ssize_t param_attr_show(struct module_attribute *mattr,
- struct module *mod, char *buf)
+ struct module_kobject *mk, char *buf)
{
int count;
struct param_attribute *attribute = to_param_attr(mattr);
@@ -531,7 +531,7 @@ static ssize_t param_attr_show(struct module_attribute *mattr,
/* sysfs always hands a nul-terminated string in buf. We rely on that. */
static ssize_t param_attr_store(struct module_attribute *mattr,
- struct module *owner,
+ struct module_kobject *km,
const char *buf, size_t len)
{
int err;
@@ -730,6 +730,8 @@ static struct module_kobject * __init locate_module_kobject(const char *name)
mk->kobj.kset = module_kset;
err = kobject_init_and_add(&mk->kobj, &module_ktype, NULL,
"%s", name);
+ if (!err)
+ err = sysfs_create_file(&mk->kobj, &module_uevent.attr);
if (err) {
kobject_put(&mk->kobj);
printk(KERN_ERR
@@ -807,7 +809,7 @@ static void __init param_sysfs_builtin(void)
}
ssize_t __modver_version_show(struct module_attribute *mattr,
- struct module *mod, char *buf)
+ struct module_kobject *mk, char *buf)
{
struct module_version_attribute *vattr =
container_of(mattr, struct module_version_attribute, mattr);
@@ -852,7 +854,7 @@ static ssize_t module_attr_show(struct kobject *kobj,
if (!attribute->show)
return -EIO;
- ret = attribute->show(attribute, mk->mod, buf);
+ ret = attribute->show(attribute, mk, buf);
return ret;
}
@@ -871,7 +873,7 @@ static ssize_t module_attr_store(struct kobject *kobj,
if (!attribute->store)
return -EIO;
- ret = attribute->store(attribute, mk->mod, buf, len);
+ ret = attribute->store(attribute, mk, buf, len);
return ret;
}
On Sun, 19 Jun 2011 00:00:29 +0200, Kay Sievers <[email protected]> wrote:
> From: Kay Sievers <[email protected]>
> Subject: module: sysfs - add 'uevent' file to allow built-in coldplug
>
> Userspace wants to manage module parameters with udev rules.
> This currently only works for loaded modules, but not for
> built-in ones.
I'm confused. What does "manage" mean here?
Thanks,
Rusty.
On Mon, Jun 20, 2011 at 01:23, Rusty Russell <[email protected]> wrote:
> On Sun, 19 Jun 2011 00:00:29 +0200, Kay Sievers <[email protected]> wrote:
>> From: Kay Sievers <[email protected]>
>> Subject: module: sysfs - add 'uevent' file to allow built-in coldplug
>>
>> Userspace wants to manage module parameters with udev rules.
>> This currently only works for loaded modules, but not for
>> built-in ones.
>
> I'm confused. What does "manage" mean here?
Hook system management into module-load events, which might include
changing module parameters in /sys/module/*/parameters/*, or at
bootup/coldplug run it for built-in modules.
We do the same to set properties for buses, drivers or devices when they appear.
Kay
On Mon, 20 Jun 2011 13:20:00 +0200, Kay Sievers <[email protected]> wrote:
> On Mon, Jun 20, 2011 at 01:23, Rusty Russell <[email protected]> wrote:
> > On Sun, 19 Jun 2011 00:00:29 +0200, Kay Sievers <[email protected]> wrote:
> >> From: Kay Sievers <[email protected]>
> >> Subject: module: sysfs - add 'uevent' file to allow built-in coldplug
> >>
> >> Userspace wants to manage module parameters with udev rules.
> >> This currently only works for loaded modules, but not for
> >> built-in ones.
> >
> > I'm confused. What does "manage" mean here?
>
> Hook system management into module-load events, which might include
> changing module parameters in /sys/module/*/parameters/*, or at
> bootup/coldplug run it for built-in modules.
>
> We do the same to set properties for buses, drivers or devices when they appear.
Sorry, that's another vague answer :(
udev already knows about module load, and the parameters are created at
that point; they are not *changed*.
What, *exactly* will this enable? Don't use the word "hook" or
"management": both vague terms which assume I know what you're talking
about. I don't.
Show me exactly what udev will now be able to do that it can't do now,
and how it will be used. Preferably with examples. Assume (correctly)
that I have only a vague idea what udev does.
Thanks,
Rusty.
On Tue, Jun 21, 2011 at 03:53, Rusty Russell <[email protected]> wrote:
> On Mon, 20 Jun 2011 13:20:00 +0200, Kay Sievers <[email protected]> wrote:
>> On Mon, Jun 20, 2011 at 01:23, Rusty Russell <[email protected]> wrote:
>> > On Sun, 19 Jun 2011 00:00:29 +0200, Kay Sievers <[email protected]> wrote:
>> >> From: Kay Sievers <[email protected]>
>> >> Subject: module: sysfs - add 'uevent' file to allow built-in coldplug
>> >>
>> >> Userspace wants to manage module parameters with udev rules.
>> >> This currently only works for loaded modules, but not for
>> >> built-in ones.
>> >
>> > I'm confused. What does "manage" mean here?
>>
>> Hook system management into module-load events, which might include
>> changing module parameters in /sys/module/*/parameters/*, or at
>> bootup/coldplug run it for built-in modules.
>>
>> We do the same to set properties for buses, drivers or devices when they appear.
>
> Sorry, that's another vague answer :(
>
> udev already knows about module load
Not for built-ins.
> and the parameters are created at
> that point; they are not *changed*.
The can be changed, and sometimes they need.
> What, *exactly* will this enable? Don't use the word "hook" or
> "management": both vague terms which assume I know what you're talking
> about. I don't.
Just used these words because it's absolutely generic infrastructure
we use already everywhere in /sys. :) Just run a: find /sys -name
uevent
> Show me exactly what udev will now be able to do that it can't do now,
> and how it will be used. Preferably with examples. Assume (correctly)
> that I have only a vague idea what udev does.
Set the polling interval of a always-built-in module parameter:
SUBSYSTEM=="module", KERNEL=="block",
ATTR{parameters/events_dfl_poll_msecs}="2000"
http://git.kernel.org/?p=linux/hotplug/udev.git;a=commitdiff;h=c5a41da94916815ac14d950a0547bffc4411f7af
Kay
On Wed, 22 Jun 2011 00:47:55 +0200, Kay Sievers <[email protected]> wrote:
> On Tue, Jun 21, 2011 at 03:53, Rusty Russell <[email protected]> wrote:
> > Sorry, that's another vague answer :(
> >
> > udev already knows about module load
>
> Not for built-ins.
OK, I re-read your commit message, and down the bottom it does say what
it *does*:
> This adds the currently missing /sys/module/<name>/uevent files
> to all module entries.
I apologize for skimming, but this should be the *title* of the patch!
Then I saw your patch hit params.c and thought you were adding a uevent
file to /sys/module/<name>/parameters/. I was even more confused when
you replied:
> Hook system management into module-load events, which might include
> changing module parameters in /sys/module/*/parameters/*...
Because loading a module might *create* module parameters, but it won't
*change* them. If we want to have events for change, we need much
more...
Now we've got that sorted, is there a reason why you changed all the
signatures rather than just using mod->mkobj in store_uevent()?
Thanks,
Rusty.
On Wed, Jun 22, 2011 at 04:00, Rusty Russell <[email protected]> wrote:
> On Wed, 22 Jun 2011 00:47:55 +0200, Kay Sievers <[email protected]> wrote:
>> On Tue, Jun 21, 2011 at 03:53, Rusty Russell <[email protected]> wrote:
>> > Sorry, that's another vague answer :(
>> >
>> > udev already knows about module load
>>
>> Not for built-ins.
>
> OK, I re-read your commit message, and down the bottom it does say what
> it *does*:
>
>> This adds the currently missing /sys/module/<name>/uevent files
>> to all module entries.
>
> I apologize for skimming,
Absolutely no problem. Asking such questions can not be wrong.
> but this should be the *title* of the patch!
Right, it's hard sometimes from 'inside' to make good titles that make
titles that are properly understood 'outside'. If you have any better
idea, please just change it.
> Then I saw your patch hit params.c and thought you were adding a uevent
> file to /sys/module/<name>/parameters/. I was even more confused when
> you replied:
>
>> Hook system management into module-load events, which might include
>> changing module parameters in /sys/module/*/parameters/*...
>
> Because loading a module might *create* module parameters, but it won't
> *change* them. If we want to have events for change, we need much
> more...
>
> Now we've got that sorted, is there a reason why you changed all the
> signatures rather than just using mod->mkobj in store_uevent()?
Because we should be able to use the same 'struct module_attribute'
for built-in modules and for loaded modules at the same time. The
current 'struct module_attribute' has 'struct module' references, but
'struct module' will never exist for built-in modules.
'Struct module_kobject' has nice back-pointer to 'struct module', so
this was the simplest to do, and looks still fine, I thought.
Kay
On Wed, 22 Jun 2011 12:17:49 +0200, Kay Sievers <[email protected]> wrote:
> On Wed, Jun 22, 2011 at 04:00, Rusty Russell <[email protected]> wrote:
> > I apologize for skimming,
>
> Absolutely no problem. Asking such questions can not be wrong.
Sure, but as we know nothing is as abrupt as a hacker who has just
encountered their own ignorance :)
> > but this should be the *title* of the patch!
>
> Right, it's hard sometimes from 'inside' to make good titles that make
> titles that are properly understood 'outside'. If you have any better
> idea, please just change it.
Good observation; for me writing documentation serves this purpose. I
renamed half the functions in the iptables code after I'd written the
user docs...
> > Now we've got that sorted, is there a reason why you changed all the
> > signatures rather than just using mod->mkobj in store_uevent()?
>
> Because we should be able to use the same 'struct module_attribute'
> for built-in modules and for loaded modules at the same time. The
> current 'struct module_attribute' has 'struct module' references, but
> 'struct module' will never exist for built-in modules.
>
> 'Struct module_kobject' has nice back-pointer to 'struct module', so
> this was the simplest to do, and looks still fine, I thought.
Yes, it's weird. The only reason it currently works is because we don't
use the mod parameter in param_attr_show and param_attr_store; it's NULL
for built-in modules.
I'd prefer that patch first, I think: it's a sensible cleanup.
Thanks,
Rusty.
On Thu, Jun 23, 2011 at 02:27, Rusty Russell <[email protected]> wrote:
> On Wed, 22 Jun 2011 12:17:49 +0200, Kay Sievers <[email protected]> wrote:
>> On Wed, Jun 22, 2011 at 04:00, Rusty Russell <[email protected]> wrote:
>> > Now we've got that sorted, is there a reason why you changed all the
>> > signatures rather than just using mod->mkobj in store_uevent()?
>>
>> Because we should be able to use the same 'struct module_attribute'
>> for built-in modules and for loaded modules at the same time. The
>> current 'struct module_attribute' has 'struct module' references, but
>> 'struct module' will never exist for built-in modules.
>>
>> 'Struct module_kobject' has nice back-pointer to 'struct module', so
>> this was the simplest to do, and looks still fine, I thought.
>
> Yes, it's weird. The only reason it currently works is because we don't
> use the mod parameter in param_attr_show and param_attr_store; it's NULL
> for built-in modules.
>
> I'd prefer that patch first, I think: it's a sensible cleanup.
You want the patch split up in two? You want to remove the mod
parameter somehow?
Thanks,
Kay
On Thu, Jun 23, 2011 at 13:24, Kay Sievers <[email protected]> wrote:
> On Thu, Jun 23, 2011 at 02:27, Rusty Russell <[email protected]> wrote:
>> On Wed, 22 Jun 2011 12:17:49 +0200, Kay Sievers <[email protected]> wrote:
>>> On Wed, Jun 22, 2011 at 04:00, Rusty Russell <[email protected]> wrote:
>
>>> > Now we've got that sorted, is there a reason why you changed all the
>>> > signatures rather than just using mod->mkobj in store_uevent()?
>>>
>>> Because we should be able to use the same 'struct module_attribute'
>>> for built-in modules and for loaded modules at the same time. The
>>> current 'struct module_attribute' has 'struct module' references, but
>>> 'struct module' will never exist for built-in modules.
>>>
>>> 'Struct module_kobject' has nice back-pointer to 'struct module', so
>>> this was the simplest to do, and looks still fine, I thought.
>>
>> Yes, it's weird. The only reason it currently works is because we don't
>> use the mod parameter in param_attr_show and param_attr_store; it's NULL
>> for built-in modules.
>>
>> I'd prefer that patch first, I think: it's a sensible cleanup.
>
> You want the patch split up in two? You want to remove the mod
> parameter somehow?
Can we get these 20 lines of code sorted out please? :)
Kay