Skip to contentSkip to conversation

SET_MODULE_OWNER?

DISCUSSION Rusty Russell 4 participants 6 messages Apr 9, 2003 1 message not in the archive

Subsystems (6)
Networking Network drivers USB Char and misc drivers +2 more subsystems
lore
Share & tools
lore

Rusty Russell

author
#1 lore raw
In reply to a message not in the archive

In message <[email protected]> you write:

Quoted 8 lines
Quoted 3 lines · Rusty Russell

Unlike that, substituting dev->owner = THIS_MODULE; has no backwards compatibility loss, and it removes a confusing and pointless macro which *never* had a point.

Substituting dev->owner=THIS_MODULE has _obvious_ backwards compat loss, because 'owner' member did not exist in struct net_device.

Oh, so SET_MODULE_OWNER is a struct net_device only thing? Certain authors (myself included) obviously don't know that.

Quoted 3 lines

If you had bothered to even do a trivial grep, you would have seen the use to which SET_MODULE_OWNER is being put. Christoph's try* changes are annoying but work-around-able. Removal of SET_MODULE_OWNER is not.

If *you* had bothered to do a grep, you would have seen non-netdevice uses to which it is really being put, as it's managed to thoroughly confuse coders.

APM, isdn, USB, hell, you even fooled the USAGI guys!

Seriously, adding an owner arg to init_etherdev and friends, or creating a set of SET_NET_DEVICE_OWNER etc macros would have been defensible for backwards compatibility.

Rusty.
PS. I didn't fix up ISDN. Kai, want me to do that?
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Name: Move SET_MODULE_OWNER to netdevice.h
Author: Rusty Russell
Status: Completely Untested

D: Jeff points out that SET_MODULE_OWNER is really only for struct
D: net_device, so move it from module.h to netdevice.h and fix up
D: areas which were confused about it (renaming would be nice, but
D: too invasive).

arch/i386/kernel/apm.c +1 -1
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.5.67/arch/i386/kernel/apm.c working-2.5.67-set_owner/arch/i386/kernel/apm.c
@@ -2038,7 +2038,7 @@ static int __init apm_init(void)
 
 	apm_proc = create_proc_info_entry("apm", 0, NULL, apm_get_info);
 	if (apm_proc)
-		SET_MODULE_OWNER(apm_proc);
+		apm_proc->owner = THIS_MODULE;
 
 	kernel_thread(apm, NULL, CLONE_FS | CLONE_FILES | CLONE_SIGHAND | SIGCHLD);
drivers/char/i8k.c +1 -1
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.5.67/drivers/char/i8k.c working-2.5.67-set_owner/drivers/char/i8k.c
@@ -757,7 +757,7 @@ int __init i8k_init(void)
 	return -ENOENT;
     }
     proc_i8k->proc_fops = &i8k_fops;
-    SET_MODULE_OWNER(proc_i8k);
+    proc_i8k->owner = THIS_MODULE;
 
     printk(KERN_INFO
 	   "Dell laptop SMM driver v%s Massimo Dal Zotto ([email protected])\n",
drivers/usb/media/pwc-if.c +1 -1
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.5.67/drivers/usb/media/pwc-if.c working-2.5.67-set_owner/drivers/usb/media/pwc-if.c
@@ -1805,7 +1805,7 @@ static int usb_pwc_probe(struct usb_inte
 	}
 	memcpy(vdev, &pwc_template, sizeof(pwc_template));
 	strcpy(vdev->name, name);
-	SET_MODULE_OWNER(vdev);
+	vdev->owner = THIS_MODULE;
 	pdev->vdev = vdev;
 	vdev->priv = pdev;
include/linux/module.h +0 -1
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.5.67/include/linux/module.h working-2.5.67-set_owner/include/linux/module.h
@@ -408,7 +408,6 @@ __attribute__((section(".gnu.linkonce.th
 #endif /* MODULE */
 
 #define symbol_request(x) try_then_request_module(symbol_get(x), "symbol:" #x)
-#define SET_MODULE_OWNER(dev) ((dev)->owner = THIS_MODULE)
 
 /* BELOW HERE ALL THESE ARE OBSOLETE AND WILL VANISH */
include/linux/netdevice.h +5 -0
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.5.67/include/linux/netdevice.h working-2.5.67-set_owner/include/linux/netdevice.h
@@ -835,6 +835,11 @@ extern int		netdev_fastroute_obstacles;
 extern void		dev_clear_fastroute(struct net_device *dev);
 #endif
 
+/* For use with struct netdevice, which didn't have an owner field in
+   2.2 and before: this provides backwards compatibility if you care.
+   Don't use on other structs (unless you know the owner field was
+   added at the same time as struct netdevice's was). */
+#define SET_MODULE_OWNER(dev) ((dev)->owner = THIS_MODULE)
 
 #endif /* __KERNEL__ */
net/ipv4/ah.c +1 -1
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.5.67/net/ipv4/ah.c working-2.5.67-set_owner/net/ipv4/ah.c
@@ -320,6 +320,7 @@ static void ah_destroy(struct xfrm_state
 static struct xfrm_type ah_type =
 {
 	.description	= "AH4",
+	.owner		= THIS_MODULE,
 	.proto	     	= IPPROTO_AH,
 	.init_state	= ah_init_state,
 	.destructor	= ah_destroy,

@@ -335,7 +336,6 @@ static struct inet_protocol ah4_protocol
 
 static int __init ah4_init(void)
 {
-	SET_MODULE_OWNER(&ah_type);
 	if (xfrm_register_type(&ah_type, AF_INET) < 0) {
 		printk(KERN_INFO "ip ah init: can't add xfrm type\n");
 		return -EAGAIN;
net/ipv4/esp.c +1 -0
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.5.67/net/ipv4/esp.c working-2.5.67-set_owner/net/ipv4/esp.c
@@ -552,6 +552,7 @@ error:
 static struct xfrm_type esp_type =
 {
 	.description	= "ESP4",
+	.owner		= THIS_MODULE,
 	.proto	     	= IPPROTO_ESP,
 	.init_state	= esp_init_state,
 	.destructor	= esp_destroy,
net/ipv6/ah6.c +1 -2
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.5.67/net/ipv6/ah6.c working-2.5.67-set_owner/net/ipv6/ah6.c
@@ -318,6 +318,7 @@ static void ah6_destroy(struct xfrm_stat
 static struct xfrm_type ah6_type =
 {
 	.description	= "AH6",
+	.owner		= THIS_MODULE,
 	.proto	     	= IPPROTO_AH,
 	.init_state	= ah6_init_state,
 	.destructor	= ah6_destroy,

@@ -333,8 +334,6 @@ static struct inet6_protocol ah6_protoco
 
 int __init ah6_init(void)
 {
-	SET_MODULE_OWNER(&ah6_type);
-
 	if (xfrm_register_type(&ah6_type, AF_INET6) < 0) {
 		printk(KERN_INFO "ipv6 ah init: can't add xfrm type\n");
 		return -EAGAIN;
net/ipv6/esp6.c +1 -1
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.5.67/net/ipv6/esp6.c working-2.5.67-set_owner/net/ipv6/esp6.c
@@ -488,6 +488,7 @@ error:
 static struct xfrm_type esp6_type =
 {
 	.description	= "ESP6",
+	.owner	     	= THIS_MODULE,
 	.proto	     	= IPPROTO_ESP,
 	.init_state	= esp6_init_state,
 	.destructor	= esp6_destroy,

@@ -504,7 +505,6 @@ static struct inet6_protocol esp6_protoc
 
 int __init esp6_init(void)
 {
-	SET_MODULE_OWNER(&esp6_type);
 	if (xfrm_register_type(&esp6_type, AF_INET6) < 0) {
 		printk(KERN_INFO "ipv6 esp init: can't add xfrm type\n");
 		return -EAGAIN;
#3 lore raw
In reply to Rusty Russell
Quoted 25 lines · Rusty Russell

In message <[email protected]> you write:

Quoted 8 lines
Quoted 3 lines · Rusty Russell

Unlike that, substituting dev->owner = THIS_MODULE; has no backwards compatibility loss, and it removes a confusing and pointless macro which *never* had a point.

Substituting dev->owner=THIS_MODULE has _obvious_ backwards compat loss, because 'owner' member did not exist in struct net_device.

Oh, so SET_MODULE_OWNER is a struct net_device only thing? Certain authors (myself included) obviously don't know that.

Quoted 1 line

If you had bothered to even do a trivial grep, you would have seen the use to which SET_MODULE_OWNER is being put. Christoph's try* changes are annoying but work-around-able. Removal of SET_MODULE_OWNER is not.

If *you* had bothered to do a grep, you would have seen non-netdevice uses to which it is really being put, as it's managed to thoroughly confuse coders.

APM, isdn, USB, hell, you even fooled the USAGI guys!

Quoted 8 lines

Seriously, adding an owner arg to init_etherdev and friends, or creating a set of SET_NET_DEVICE_OWNER etc macros would have been defensible for backwards compatibility.

D: Jeff points out that SET_MODULE_OWNER is really only for struct
D: net_device, so move it from module.h to netdevice.h and fix up
D: areas which were confused about it (renaming would be nice, but
D: too invasive).

Rusty, this is amazingly disingenuous.

Never did I say SET_MODULE_OWNER was only for struct net_device.

I even pointed out specifically that it IS NOT tied to a specific structure:

Quoted 3 lines

no, SET_MODULE_OWNER is quite intentionally independent of the struct. It only requires a consisnent naming in the source, between structures that may use the macro.

That's a feature.

Why don't you just let the maintainers apply the driver "cleanups" if they wish, or do not wish, like DaveM did. Only when that is accomplished is it reasonable to consider moving SET_MODULE_OWNER -- and only then if other people do not need it's obvious utility.

I would personally rather the macro lived on as a part of the module API. Think of it as part of maintaining friendly hardware vendor relations, perhaps?

Jeff

author
#4 lore raw

In message <[email protected]> you write:

Quoted 4 lines

Why don't you just let the maintainers apply the driver "cleanups" if they wish, or do not wish, like DaveM did. Only when that is accomplished is it reasonable to consider moving SET_MODULE_OWNER -- and only then if other people do not need it's obvious utility.

The please define when it should and should not be used, so everyone knows.

Currently it seems to be:

/* This macro should be used on structures which had the owner field
added between 2.2 and 2.4, and not others. */

Is that correct?
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

#5 lore raw
Quoted 7 lines · Rusty Russell

In message <[email protected]> you write:

Quoted 1 line

Why don't you just let the maintainers apply the driver "cleanups" if they wish, or do not wish, like DaveM did. Only when that is accomplished is it reasonable to consider moving SET_MODULE_OWNER -- and only then if other people do not need it's obvious utility.

The please define when it should and should not be used, so everyone knows.

Use with structures that have an owner field, if you care about cross-version kernel source compatibility.

Quoted 6 lines

Currently it seems to be:

/* This macro should be used on structures which had the owner field
added between 2.2 and 2.4, and not others. */

Is that correct?

No. SET_MODULE_OWNER is useful regardless of kernel version, not just the restrictive set you define here. Different vendors may implement SET_MODULE_OWNER with a different range of kernel versions, if they so choose. It's not restricted at all to when struct net_device gained an 'owner' field.

Maybe think of it this way: a source code hook whose implementation is free to change, as long as it functionally produces the desired result.
The in-kernel definition of the macro is only one of N implementations.

Jeff

#6 lore raw
In reply to Rusty Russell
Quoted 1 line · Rusty Russell

PS. I didn't fix up ISDN. Kai, want me to do that?

I can do it, no problem, I have a batch of other ISDN changes pending, anyway. It'll probably be a week or so, though.

--Kai