SET_MODULE_OWNER?

SET_MODULE_OWNER?

[Rusty Russell]

I thought it was completely useless, hence deprecated.

Anyone have any reason to defend it?
Rusty.
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Re: SET_MODULE_OWNER?

[Christoph Hellwig]

On Mon, Apr 07, 2003 at 04:47:28PM +1000, Rusty Russell wrote:
> I thought it was completely useless, hence deprecated.
> 
> Anyone have any reason to defend it?

It's used in the net drivers to set their owner.  One could argue whether
it makes sense to use a macro for it or not, but as most drivers converted
to new-style refcounting use it I don't see any reason to deprecated it.

Re: SET_MODULE_OWNER?

[Jeff Garzik]

Rusty Russell wrote:
> I thought it was completely useless, hence deprecated.
> 
> Anyone have any reason to defend it?


It's used to allow source compatibility with all kernels, old or new.

Thus it is in active use, and should not be removed.

	Jeff

Re: SET_MODULE_OWNER?

[Rusty Russell]

In message <3E91C398.9070400@pobox.com> you write:
> Rusty Russell wrote:
> > I thought it was completely useless, hence deprecated.
> > 
> > Anyone have any reason to defend it?
> 
> 
> It's used to allow source compatibility with all kernels, old or new.
> 
> Thus it is in active use, and should not be removed.

Inside individual drivers, or a set of compat macros, it makes sense.
But as a general module.h primitive it doesn't.

Imagine a structure adds an owner field in 2.5.  This macro doesn't
help you, you need a specific compat macro for that struct.

ie. AFAICT it only buys you 2.2 compatibility, and even then only if
you #define it at the top of your driver.

I still don't understand: please demonstrate a use in existing source.
Rusty.
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Re: SET_MODULE_OWNER?

[Zwane Mwaikambo]

On Tue, 8 Apr 2003, Rusty Russell wrote:

> I still don't understand: please demonstrate a use in existing source.
> Rusty.

I do agree it only does something which we do manually with a lot of the 
other members, however i'm not quite sure wether the compatibility thing 
still holds since a number of drivers have a lot of 2.5 specific stuff 
(e.g. pci-dma api changes). Regardless, here is a typical use in a 
netdriver.

	/* dev and dev->priv zeroed in alloc_etherdev */
	dev = alloc_etherdev (sizeof (*tp));
	if (dev == NULL) {
		printk (KERN_ERR PFX "%s: Unable to alloc new net device\n", pdev->slot_name);
		return -ENOMEM;
	}
	SET_MODULE_OWNER(dev);
	tp = dev->priv;
	
	/* followed by ... */
	dev->foo = __foo;
	dev->bar = __bar;


	Zwane

--
function.linuxpower.ca

Re: SET_MODULE_OWNER?

[Rusty Russell]

In message <Pine.LNX.4.50.0304072212310.21025-100000@montezuma.mastecende.com> 
you write:
> (e.g. pci-dma api changes). Regardless, here is a typical use in a 
> netdriver.
> 
> 	/* dev and dev->priv zeroed in alloc_etherdev */
> 	dev = alloc_etherdev (sizeof (*tp));
> 	if (dev == NULL) {
> 		printk (KERN_ERR PFX "%s: Unable to alloc new net device\n", pd
ev->slot_name);
> 		return -ENOMEM;
> 	}
> 	SET_MODULE_OWNER(dev);
> 	tp = dev->priv;
> 	
> 	/* followed by ... */
> 	dev->foo = __foo;
> 	dev->bar = __bar;

I don't understand how that helps compatibility over:

	dev->owner = THIS_MODULE;

Both will:
1) Work on 2.4 and 2.5
2) Break on 2.2.

Confused,
Rusty.
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Re: SET_MODULE_OWNER?

[Jeff Garzik]

Rusty Russell wrote:
> In message <Pine.LNX.4.50.0304072212310.21025-100000@montezuma.mastecende.com> 
> you write:
> 
>>(e.g. pci-dma api changes). Regardless, here is a typical use in a 
>>netdriver.
>>
>>	/* dev and dev->priv zeroed in alloc_etherdev */
>>	dev = alloc_etherdev (sizeof (*tp));
>>	if (dev == NULL) {
>>		printk (KERN_ERR PFX "%s: Unable to alloc new net device\n", pd
> 
> ev->slot_name);
> 
>>		return -ENOMEM;
>>	}
>>	SET_MODULE_OWNER(dev);
>>	tp = dev->priv;
>>	
>>	/* followed by ... */
>>	dev->foo = __foo;
>>	dev->bar = __bar;
> 
> 
> I don't understand how that helps compatibility over:
> 
> 	dev->owner = THIS_MODULE;
> 
> Both will:
> 1) Work on 2.4 and 2.5
> 2) Break on 2.2.


Wrong.

SET_MODULE_OWNER works in 2.2 _and earlier_... given some compat glue in 
a "#include <kcompat>" macros.

You may take a look at "kcompat" for further examples. 
http://sf.net/projects/gkernel/   I provide an example of how to get a 
net driver from 2.4 running under 2.2, such that the 2.4 driver 
-appears- to be completely free of compatibility glue.

The only sticking points in such glue are when structure members are 
directly exposed in the source code.  That cannot be worked around with 
cpp magic.  This is why SET_MODULE_OWNER exists, this is why 
pci_{get,set}_drvdata() exist, and so on.

	Jeff

Re: SET_MODULE_OWNER?

[Rusty Russell]

In message <3E925292.8060104@pobox.com> you write:
> You may take a look at "kcompat" for further examples. 
> http://sf.net/projects/gkernel/   I provide an example of how to get a 
> net driver from 2.4 running under 2.2, such that the 2.4 driver 
> -appears- to be completely free of compatibility glue.

Interesting.  How is this related to the older linux/compatmac.h?
Have you thought of actually integrating a 2.4<->2.6 version once
2.6.0 is out?

Thanks for the pointer!
Rusty.
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Re: SET_MODULE_OWNER?

[Jeff Garzik]

Rusty Russell wrote:
> In message <3E925292.8060104@pobox.com> you write:
> 
>>You may take a look at "kcompat" for further examples. 
>>http://sf.net/projects/gkernel/   I provide an example of how to get a 
>>net driver from 2.4 running under 2.2, such that the 2.4 driver 
>>-appears- to be completely free of compatibility glue.
> 
> 
> Interesting.  How is this related to the older linux/compatmac.h?


It's not related at all to compatmac.h, though there are no doubt 
duplicate definitions in there.  This is mainly my own code, plus 
tytso's (serial.c), plus Donald Becker's.


> Have you thought of actually integrating a 2.4<->2.6 version once
> 2.6.0 is out?

Oh, I would love to have such items in there...   I take patches, and no 
need to wait for 2.6 either :)

	Jeff

Re: SET_MODULE_OWNER?

[Jeff Garzik]

Rusty Russell wrote:
> In message <3E91C398.9070400@pobox.com> you write:
> 
>>Rusty Russell wrote:
>>
>>>I thought it was completely useless, hence deprecated.
>>>
>>>Anyone have any reason to defend it?
>>
>>
>>It's used to allow source compatibility with all kernels, old or new.
>>
>>Thus it is in active use, and should not be removed.
> 
> 
> Inside individual drivers, or a set of compat macros, it makes sense.
> But as a general module.h primitive it doesn't.
> 
> Imagine a structure adds an owner field in 2.5.  This macro doesn't
> help you, you need a specific compat macro for that struct.

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.


> ie. AFAICT it only buys you 2.2 compatibility, and even then only if
> you #define it at the top of your driver.

no, farther back than that, to infinity and beyond :)  The idea of the 
macro is that on earlier kernels, it is simply a no-op, and module 
refcounting is handled by other means.


> I still don't understand: please demonstrate a use in existing source.

demonstrate?  grep for it.  It's used quite a bit.  Removal of 
SET_MODULE_OWNER looks to me to be pointless churn for negative gain. 
If if you wish to pointedly ignore the old-source compatibility angle, 
it is a nice convenience macro.

	Jeff

Re: SET_MODULE_OWNER?

[Rusty Russell]

In message <3E923390.9010206@pobox.com> you write:
> > ie. AFAICT it only buys you 2.2 compatibility, and even then only if
> > you #define it at the top of your driver.
> 
> no, farther back than that, to infinity and beyond :)  The idea of the 
> macro is that on earlier kernels, it is simply a no-op, and module 
> refcounting is handled by other means.

Crap.  Since hch removed the other module ops, if your module does its
own refcount THAT won't compile in 2.5.

> > I still don't understand: please demonstrate a use in existing source.
> 
> demonstrate?  grep for it.  It's used quite a bit.  Removal of 
> SET_MODULE_OWNER looks to me to be pointless churn for negative gain. 
> If if you wish to pointedly ignore the old-source compatibility angle, 
> it is a nice convenience macro.

This is complete crap.  It's an obfuscation macro, with no backwards
compatibility capabilities as currently implemented.

Christoph went through and substituted try_inc_mod_count to
try_module_get, for no gain, and broke backwards compatibility.

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.

Unless you can come up with a real *reason*, I'll move it back under
"deprecated" and start substituting.

Cheers,
Rusty.
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Re: SET_MODULE_OWNER?

[Christoph Hellwig]

On Tue, Apr 08, 2003 at 01:46:52PM +1000, Rusty Russell wrote:
> Christoph went through and substituted try_inc_mod_count to
> try_module_get, for no gain, and broke backwards compatibility.

You can still implement try_module_get and module_put on older
kernels.  Jeff mostly cares about net drivers anyway that use neither :)

Re: SET_MODULE_OWNER?

[Alan Cox]

On Maw, 2003-04-08 at 04:46, Rusty Russell wrote:
> 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.

Its an abstraction macro.

> Unless you can come up with a real *reason*, I'll move it back under
> "deprecated" and start substituting.

Thats fun, and the rest of us can play submit patches to substitute it
back. 

This is not how to work with people

Re: SET_MODULE_OWNER?

[Jamie Lokier]

Alan Cox wrote:
> > Unless you can come up with a real *reason*, I'll move it back under
> > "deprecated" and start substituting.
> 
> Thats fun, and the rest of us can play submit patches to substitute it
> back. 

If Jeff's drivers are using <kcompat>, can't kcompat provide the macro
for 2.4 and 2.5 kernels in the same way it does for 2.2 kernels?

-- Jamie

Re: SET_MODULE_OWNER?

[Jeff Garzik]

On Tue, Apr 08, 2003 at 03:46:44PM +0100, Jamie Lokier wrote:
> Alan Cox wrote:
> > > Unless you can come up with a real *reason*, I'll move it back under
> > > "deprecated" and start substituting.
> > 
> > Thats fun, and the rest of us can play submit patches to substitute it
> > back. 
> 
> If Jeff's drivers are using <kcompat>, can't kcompat provide the macro
> for 2.4 and 2.5 kernels in the same way it does for 2.2 kernels?

No.  Because Rusty wanted to replace a "func_call()" object with a
direct reference to a structure.  Direct struct member references is the
big issue that we are trying to _avoid_, because they are the single
most painful issue to deal with, WRT source back-compat.  You can ifdef
around a function quite easily, but not a direct struct member use.

To give another concrete example, I was able to take a 2.4 PCI driver
and make it work under 2.2 transparently, with a single exception:  The
"driver_data" member of the new struct pci_dev.  Drivers were directly
referencing that, which was a new addition in 2.4.x (really 2.3.x).  So,
I created the abstraction wrappers pci_[gs]et_drvdata(), which does
nothing but a simple C assignment (or read, for _get_).  The addition of
this wrapper removed the need for nasty ifdefs in the drivers for 2.2
versus 2.4, and make it possible for the kernel source to continue to be
readable, "pretty", and ifdef-free.

	Jeff

Re: SET_MODULE_OWNER?

[Jamie Lokier]

Jeff Garzik wrote:
> No.  Because Rusty wanted to replace a "func_call()" object with a
> direct reference to a structure.  Direct struct member references is the
> big issue that we are trying to _avoid_, because they are the single
> most painful issue to deal with, WRT source back-compat.  You can ifdef
> around a function quite easily, but not a direct struct member use.
> 
> To give another concrete example, I was able to take a 2.4 PCI driver
> and make it work under 2.2 transparently, with a single exception:  The
> "driver_data" member of the new struct pci_dev.  Drivers were directly
> referencing that, which was a new addition in 2.4.x (really 2.3.x).  So,
> I created the abstraction wrappers pci_[gs]et_drvdata(), which does
> nothing but a simple C assignment (or read, for _get_).  The addition of
> this wrapper removed the need for nasty ifdefs in the drivers for 2.2
> versus 2.4, and make it possible for the kernel source to continue to be
> readable, "pretty", and ifdef-free.

I've done similar things myself.  My drivers, including network and
char devices, work on all kernels from 2.0 to 2.4 using similar
kcompat-like macros.  I've not extended any to work with 2.5, though.

If you want to write drivers that work on 2.4 and 2.2 kernels, that's
easy: #include <kcompat.h> or whatever you use, and call
SET_MODULE_OWNER in your drivers.

However, if you insist on using drivers from the 2.4 kernel tree on
2.2 kernels, that's a different matter, and you have a point.

For 3rd party drivers that I've written, I take the first approach and
write code that very nearly conforms to 2.4 style except when it is
not possible for that to be back compatible.  This occurs in certain
file operations (2.0 has different function prototypes), in mapping
device memory to user space (change from vma->vm_offset to ->vm_pgoff,
PCI configuration (lots of changes in 2.1.93 :) and a few other
things.

It is all very well to insist that SET_MODULE_OWNER() remains so you
can take 2.4 drivers and easily compile them for 2.2...  but why is
that the benchmark?  I can't take 2.4 drivers and do that, because I
want to support 2.0 as well, so I bite the bullet and make the
necessary changes for broader compatibility.

So.. back to a point.  Is 2.2 compilability (with the help of kcompat)
one of the goals to aim for in 2.5 drivers generally?  Or is this
specifically meant for the network drivers which you support?

Cheers,
-- Jamie

Re: SET_MODULE_OWNER?

[Jeff Garzik]

On Tue, Apr 08, 2003 at 05:45:01PM +0100, Jamie Lokier wrote:
> It is all very well to insist that SET_MODULE_OWNER() remains so you
> can take 2.4 drivers and easily compile them for 2.2...  but why is
> that the benchmark?  I can't take 2.4 drivers and do that, because I
> want to support 2.0 as well, so I bite the bullet and make the
> necessary changes for broader compatibility.

You can do 2.0 compat with kcompat.  Just needs a couple more compat
macros in kcompat tarball.  I grant you that net drivers are much more
resilient across kernel versions, and are easier to make portable across
the various kernel API changes -- precisely because we've managed to
keep the core interfaces fairly stable, logic- and locking-wise.
SET_MODULE_OWNER is just one more piece of this conscious effort.


> So.. back to a point.  Is 2.2 compilability (with the help of kcompat)
> one of the goals to aim for in 2.5 drivers generally?  Or is this
> specifically meant for the network drivers which you support?

In general, the mainline kernel has two conflicting goals:
* maintain source back-compat as long as it is reasonable
* keep back-compat garbage to a minimum, eliminating it where possible

It really comes down to a maintainer decision, unless there is an
overriding decision to purposefully break source back-compat.

To answer your question specifically, SET_MODULE_OWNER eases source
back-compat in general, but it's main user is network drivers.

	Jeff

Re: SET_MODULE_OWNER?

[Rusty Russell]

In message <3E92515B.6030807@pobox.com> you write:
> Rusty Russell wrote:
> > 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.

> 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).

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
--- linux-2.5.67/arch/i386/kernel/apm.c	2003-04-08 11:13:40.000000000 +1000
+++ working-2.5.67-set_owner/arch/i386/kernel/apm.c	2003-04-08 22:03:56.000000000 +1000
@@ -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);
 
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
--- linux-2.5.67/drivers/char/i8k.c	2003-02-07 19:22:26.000000000 +1100
+++ working-2.5.67-set_owner/drivers/char/i8k.c	2003-04-08 22:04:33.000000000 +1000
@@ -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 (dz@debian.org)\n",
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
--- linux-2.5.67/drivers/usb/media/pwc-if.c	2003-03-18 12:21:38.000000000 +1100
+++ working-2.5.67-set_owner/drivers/usb/media/pwc-if.c	2003-04-08 22:21:30.000000000 +1000
@@ -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;
 
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
--- linux-2.5.67/include/linux/module.h	2003-04-08 11:15:01.000000000 +1000
+++ working-2.5.67-set_owner/include/linux/module.h	2003-04-08 22:13:05.000000000 +1000
@@ -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 */
 
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
--- linux-2.5.67/include/linux/netdevice.h	2003-04-08 11:15:01.000000000 +1000
+++ working-2.5.67-set_owner/include/linux/netdevice.h	2003-04-08 22:16:01.000000000 +1000
@@ -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__ */
 
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
--- linux-2.5.67/net/ipv4/ah.c	2003-04-08 11:15:06.000000000 +1000
+++ working-2.5.67-set_owner/net/ipv4/ah.c	2003-04-08 22:20:05.000000000 +1000
@@ -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;
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
--- linux-2.5.67/net/ipv4/esp.c	2003-04-08 11:15:06.000000000 +1000
+++ working-2.5.67-set_owner/net/ipv4/esp.c	2003-04-08 22:20:18.000000000 +1000
@@ -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,
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
--- linux-2.5.67/net/ipv6/ah6.c	2003-04-08 11:15:09.000000000 +1000
+++ working-2.5.67-set_owner/net/ipv6/ah6.c	2003-04-08 22:20:27.000000000 +1000
@@ -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;
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
--- linux-2.5.67/net/ipv6/esp6.c	2003-04-08 11:15:09.000000000 +1000
+++ working-2.5.67-set_owner/net/ipv6/esp6.c	2003-04-08 22:20:42.000000000 +1000
@@ -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;

Re: SET_MODULE_OWNER?

[David S. Miller]

The IPSEC bits are applied to my tree, thanks.

Re: SET_MODULE_OWNER?

[Jeff Garzik]

Rusty Russell wrote:
> In message <3E92515B.6030807@pobox.com> you write:
> 
>>Rusty Russell wrote:
>>
>>>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.
> 
> 
>>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.

> 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:

> 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

Re: SET_MODULE_OWNER?

[Rusty Russell]

In message <3E937144.9090105@pobox.com> you write:
> 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.

Re: SET_MODULE_OWNER?

[Jeff Garzik]

Rusty Russell wrote:
> In message <3E937144.9090105@pobox.com> you write:
> 
>>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.


> 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

Re: SET_MODULE_OWNER?

[Kai Germaschewski]

On Tue, 8 Apr 2003, Rusty Russell wrote:

> 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

Re: SET_MODULE_OWNER?

[Rusty Russell]

In message <3E93AA3D.4050104@pobox.com> you write:
> 
> looks ok to me

After months of intense negotiation, we have... a comment. 8)

Thanks Jeff.

Linus, please apply.
Rusty.

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.5.67-bk1/include/linux/module.h working-2.5.67-bk1-set-owner/include/linux/module.h
--- linux-2.5.67-bk1/include/linux/module.h	2003-04-08 11:15:01.000000000 +1000
+++ working-2.5.67-bk1-set-owner/include/linux/module.h	2003-04-09 15:15:47.000000000 +1000
@@ -408,6 +408,12 @@ __attribute__((section(".gnu.linkonce.th
 #endif /* MODULE */
 
 #define symbol_request(x) try_then_request_module(symbol_get(x), "symbol:" #x)
+
+/* If you want backwards compat: some structs didn't have owner fields once */
+/* Think of SET_MODULE_OWNER like an IBM mainframe: leave it in a dark 
+   corner for years, don't break it, but don't ever upgrade it either :) 
+   If there is something newer and sexier than the mainframe, it's ok to 
+   use that instead.  The mainframe won't feel lonely. -- Jeff Garzik */
 #define SET_MODULE_OWNER(dev) ((dev)->owner = THIS_MODULE)
 
 /* BELOW HERE ALL THESE ARE OBSOLETE AND WILL VANISH */
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.