[PATCH] kill symbol_get & friends

[PATCH] kill symbol_get & friends

[Christoph Hellwig]

Rusty introduced symbol_get as a replacement for inter_module_get, but
it doesn't really solved the underlying problem.

Which is that just about every subsystem assumes that the implicit
reference a module get's as soon as it's exported symbols are used
can't go away unexpectly, e.g. any

	foo_register()

will always be followed by an

	foo_unregister()

before the module goes away, and thus foo_register can just allocate
data structures which will be freed by foo_unregister.

Now with symbol_get we do

	int (*bar_foo)(struct bar_foo *) = symbol_get(fump_bar_foo);
	if (bar_foo)
		bar_foo(...);
	symbol_put(bar_foo);

for the register call, but the module exporting fump_bar_foo can
go away before we do the same game for the unregister function.

I think we should simply disallow these weak references in any form, as
the obvious way to use them is incorrect and Linux has done very well
so far in trading a little bit of memory (requiring the module to be
a hard depency) for such complexity.  And usually the requirement can
be turned off hard anyway (e.g. CONFIG_I2C for the one and only current
user of symbol_get).

And as mentioned we only have one single users of this mechanism in the
tree which doesn't speak for it's general usefullness (well, and another
one of the previous version, inter_module_*)


--- 1.66/drivers/video/Kconfig	2004-10-28 09:39:53 +02:00
+++ edited/drivers/video/Kconfig	2005-01-12 21:09:13 +01:00
@@ -798,8 +798,6 @@
 config FB_SAVAGE
 	tristate "S3 Savage support"
 	depends on FB && PCI && EXPERIMENTAL
-	select I2C_ALGOBIT if FB_SAVAGE_I2C
-	select I2C if FB_SAVAGE_I2C
 	select FB_MODE_HELPERS
 	help
 	  This driver supports notebooks and computers with S3 Savage PCI/AGP
@@ -811,9 +809,10 @@
 	  will be called savagefb.
 
 config FB_SAVAGE_I2C
-       tristate "Enable DDC2 Support"
-       depends on FB_SAVAGE
-       help
+	tristate "Enable DDC2 Support"
+	select I2C_ALGOBIT if FB_SAVAGE_I2C
+	select I2C if FB_SAVAGE_I2C
+	help
 	  This enables I2C support for S3 Savage Chipsets.  This is used
 	  only for getting EDID information from the attached display
 	  allowing for robust video mode handling and switching.
--- 1.4/drivers/video/savage/savagefb-i2c.c	2005-01-05 03:48:33 +01:00
+++ edited/drivers/video/savage/savagefb-i2c.c	2005-01-12 21:08:33 +01:00
@@ -17,6 +17,7 @@
 #include <linux/delay.h>
 #include <linux/pci.h>
 #include <linux/fb.h>
+#include <linux/i2c.h>
 
 #include <asm/io.h>
 #include "savagefb.h"
@@ -141,10 +142,9 @@
 static int savage_setup_i2c_bus(struct savagefb_i2c_chan *chan,
 				const char *name)
 {
-	int (*add_bus)(struct i2c_adapter *) = symbol_get(i2c_bit_add_bus);
 	int rc = 0;
 
-	if (add_bus && chan->par) {
+	if (chan->par) {
 		strcpy(chan->adapter.name, name);
 		chan->adapter.owner		= THIS_MODULE;
 		chan->adapter.id		= I2C_ALGO_SAVAGE;
@@ -162,7 +162,7 @@
 		chan->algo.setscl(chan, 1);
 		udelay(20);
 
-		rc = add_bus(&chan->adapter);
+		rc = i2c_bit_add_bus(&chan->adapter);
 
 		if (rc == 0)
 			dev_dbg(&chan->par->pcidev->dev,
@@ -170,8 +170,6 @@
 		else
 			dev_warn(&chan->par->pcidev->dev,
 				 "Failed to register I2C bus %s.\n", name);
-
-		symbol_put(i2c_bit_add_bus);
 	} else
 		chan->par = NULL;
 
@@ -212,14 +210,9 @@
 void savagefb_delete_i2c_busses(struct fb_info *info)
 {
 	struct savagefb_par *par = (struct savagefb_par *)info->par;
-	int (*del_bus)(struct i2c_adapter *) =
-		symbol_get(i2c_bit_del_bus);
-
-	if (del_bus && par->chan.par) {
-		del_bus(&par->chan.adapter);
-		symbol_put(i2c_bit_del_bus);
-	}
 
+	if (par->chan.par)
+		i2c_bit_del_bus(&par->chan.adapter);
 	par->chan.par = NULL;
 }
 EXPORT_SYMBOL(savagefb_delete_i2c_busses);
@@ -227,8 +220,6 @@
 static u8 *savage_do_probe_i2c_edid(struct savagefb_i2c_chan *chan)
 {
 	u8 start = 0x0;
-	int (*transfer)(struct i2c_adapter *, struct i2c_msg *, int) =
-		symbol_get(i2c_transfer);
 	struct i2c_msg msgs[] = {
 		{
 			.addr	= SAVAGE_DDC,
@@ -242,21 +233,19 @@
 	};
 	u8 *buf = NULL;
 
-	if (transfer && chan->par) {
+	if (chan->par) {
 		buf = kmalloc(EDID_LENGTH, GFP_KERNEL);
 
 		if (buf) {
 			msgs[1].buf = buf;
 
-			if (transfer(&chan->adapter, msgs, 2) != 2) {
+			if (i2c_transfer(&chan->adapter, msgs, 2) != 2) {
 				dev_dbg(&chan->par->pcidev->dev,
 					"Unable to read EDID block.\n");
 				kfree(buf);
 				buf = NULL;
 			}
 		}
-
-		symbol_put(i2c_transfer);
 	}
 
 	return buf;
--- 1.92/include/linux/module.h	2005-01-10 20:28:15 +01:00
+++ edited/include/linux/module.h	2005-01-12 21:07:45 +01:00
@@ -169,11 +169,6 @@
 
 #ifdef CONFIG_MODULES
 
-/* Get/put a kernel symbol (calls must be symmetric) */
-void *__symbol_get(const char *symbol);
-void *__symbol_get_gpl(const char *symbol);
-#define symbol_get(x) ((typeof(&x))(__symbol_get(MODULE_SYMBOL_PREFIX #x)))
-
 #ifndef __GENKSYMS__
 #ifdef CONFIG_MODVERSIONS
 /* Mark the CRC weak since genksyms apparently decides not to
@@ -350,9 +345,6 @@
 
 #ifdef CONFIG_MODULE_UNLOAD
 unsigned int module_refcount(struct module *mod);
-void __symbol_put(const char *symbol);
-#define symbol_put(x) __symbol_put(MODULE_SYMBOL_PREFIX #x)
-void symbol_put_addr(void *addr);
 
 /* Sometimes we know we already have a refcount, and it's easier not
    to handle the error case (which only happens with rmmod --wait). */
@@ -403,9 +395,6 @@
 static inline void __module_get(struct module *module)
 {
 }
-#define symbol_put(x) do { } while(0)
-#define symbol_put_addr(p) do { } while(0)
-
 #endif /* CONFIG_MODULE_UNLOAD */
 
 /* This is a #define so the string doesn't get put in every .o file */
@@ -466,11 +455,6 @@
 	return NULL;
 }
 
-/* Get/put a kernel symbol (calls should be symmetric) */
-#define symbol_get(x) ({ extern typeof(x) x __attribute__((weak)); &(x); })
-#define symbol_put(x) do { } while(0)
-#define symbol_put_addr(x) do { } while(0)
-
 static inline void __module_get(struct module *module)
 {
 }
@@ -545,8 +529,6 @@
 
 #endif /* CONFIG_MODULES */
 
-#define symbol_request(x) try_then_request_module(symbol_get(x), "symbol:" #x)
-
 /* BELOW HERE ALL THESE ARE OBSOLETE AND WILL VANISH */
 
 struct obsolete_modparm {
@@ -567,7 +549,6 @@
 
 #define __MODULE_STRING(x) __stringify(x)
 
-/* Use symbol_get and symbol_put instead.  You'll thank me. */
 #define HAVE_INTER_MODULE
 extern void __deprecated inter_module_register(const char *,
 		struct module *, const void *);
===== kernel/module.c 1.132 vs edited =====
--- 1.132/kernel/module.c	2005-01-12 01:42:57 +01:00
+++ edited/kernel/module.c	2005-01-12 21:09:51 +01:00
@@ -624,33 +624,6 @@
 		seq_printf(m, "-");
 }
 
-void __symbol_put(const char *symbol)
-{
-	struct module *owner;
-	unsigned long flags;
-	const unsigned long *crc;
-
-	spin_lock_irqsave(&modlist_lock, flags);
-	if (!__find_symbol(symbol, &owner, &crc, 1))
-		BUG();
-	module_put(owner);
-	spin_unlock_irqrestore(&modlist_lock, flags);
-}
-EXPORT_SYMBOL(__symbol_put);
-
-void symbol_put_addr(void *addr)
-{
-	unsigned long flags;
-
-	spin_lock_irqsave(&modlist_lock, flags);
-	if (!kernel_text_address((unsigned long)addr))
-		BUG();
-
-	module_put(module_text_address((unsigned long)addr));
-	spin_unlock_irqrestore(&modlist_lock, flags);
-}
-EXPORT_SYMBOL_GPL(symbol_put_addr);
-
 static ssize_t show_refcnt(struct module_attribute *mattr,
 			   struct module *mod, char *buffer)
 {
@@ -1098,22 +1071,6 @@
 	/* Finally, free the core (containing the module structure) */
 	module_free(mod, mod->module_core);
 }
-
-void *__symbol_get(const char *symbol)
-{
-	struct module *owner;
-	unsigned long value, flags;
-	const unsigned long *crc;
-
-	spin_lock_irqsave(&modlist_lock, flags);
-	value = __find_symbol(symbol, &owner, &crc, 1);
-	if (value && !strong_try_module_get(owner))
-		value = 0;
-	spin_unlock_irqrestore(&modlist_lock, flags);
-
-	return (void *)value;
-}
-EXPORT_SYMBOL_GPL(__symbol_get);
 
 /* Change all symbols so that sh_value encodes the pointer directly. */
 static int simplify_symbols(Elf_Shdr *sechdrs,

Re: [PATCH] kill symbol_get & friends

[Rusty Russell]

On Wed, 2005-01-12 at 21:31 +0100, Christoph Hellwig wrote:
> Rusty introduced symbol_get as a replacement for inter_module_get, but
> it doesn't really solved the underlying problem.

Sorry, Christoph, I must be particularly obtuse today.

If you don't hold a reference, then yes, the module can go away.  This
hasn't been a huge problem for users in the past.

The lack of users is because, firstly, dynamic dependencies are less
common than static ones, and secondly because the remaining inter-module
users (AGP and mtd) have not been converted.  Patches have been sent
several times, but maintainers are distracted, it seems.  I *will* run
out of patience and push those patches which take away intermodule.c one
day (hint, hint!).

For optional module dependencies, weak symbols can be used, but there
seems to be a desire for genuine dynamic dependencies.  If you can get
rid of those, I'll apply your patch in a second!

Cheers,
Rusty.
-- 
A bad analogy is like a leaky screwdriver -- Richard Braakman

Re: [PATCH] kill symbol_get & friends

[Dave Airlie]

> 
> Sorry, Christoph, I must be particularly obtuse today.
> 
> If you don't hold a reference, then yes, the module can go away.  This
> hasn't been a huge problem for users in the past.
> 
> The lack of users is because, firstly, dynamic dependencies are less
> common than static ones, and secondly because the remaining inter-module
> users (AGP and mtd) have not been converted.  Patches have been sent
> several times, but maintainers are distracted, it seems.  I *will* run
> out of patience and push those patches which take away intermodule.c one
> day (hint, hint!).

well the DRM doesn't use the AGP anymore so it should be safe to nuke
(does i810 framebuffer use it??), Christoph didn't like converting the
DRM to use module_get so I just went straight to the agp backend..
it's not perfect but it'll work in nearly all situations..

> 
> For optional module dependencies, weak symbols can be used, but there
> seems to be a desire for genuine dynamic dependencies.  If you can get
> rid of those, I'll apply your patch in a second!

what weak symbol support? can I actually use gcc weak symbols and have
it all work?
what happens if the module goes away? 

Dave.

Re: [PATCH] kill symbol_get & friends

[Rusty Russell]

On Thu, 2005-01-13 at 11:59 +1100, Dave Airlie wrote:
> > For optional module dependencies, weak symbols can be used, but there
> > seems to be a desire for genuine dynamic dependencies.  If you can get
> > rid of those, I'll apply your patch in a second!
> 
> what weak symbol support? can I actually use gcc weak symbols and have
> it all work?
> what happens if the module goes away? 

1) See kernel/module.c:1156 ("/* OK if weak. */").

2) Weak undefined symbols should "just work".  Overriding of weak
defined symbols is not implemented: noone has asked.

3) Like any statically-resolved symbol, this module will hold a
reference to the module exporting the symbol.  The only special thing
about weak symbols is that we don't fail to load if they are unresolved
(and the address will be NULL in this case).

Hope that clarifies,
Rusty.
-- 
A bad analogy is like a leaky screwdriver -- Richard Braakman

Re: [PATCH] kill symbol_get & friends

[Dave Jones]

On Thu, Jan 13, 2005 at 11:19:33AM +1100, Rusty Russell wrote:
 
 > The lack of users is because, firstly, dynamic dependencies are less
 > common than static ones, and secondly because the remaining inter-module
 > users (AGP and mtd) have not been converted.  Patches have been sent
 > several times, but maintainers are distracted, it seems.

Patch for AGP is going to go in real soon. I wanted to get the
other agp stuff currently in Andrews tree out of the way first
as its quite large, and I feel Mikes pain in having to rediff
that stuff constantly due to more trivial changes taking place.

 > I *will* run out of patience and push those patches which take away
 > intermodule.c one day (hint, hint!).

I must not respond to Rusty's taunts..

8-)
		Dave

Re: [PATCH] kill symbol_get & friends

[David Woodhouse]

On Thu, 2005-01-13 at 11:59 +1100, Dave Airlie wrote:
> what weak symbol support? can I actually use gcc weak symbols and have
> it all work?

Weak undefined symbols used to work, certainly. The MTD code used them
for a while, before kaos imposed the inter_module_crap on it despite my
objections.

> what happens if the module goes away? 

If another module is using a weak symbol, IIRC the module providing the
symbol _can't_ go away. I could be wrong -- I didn't use that
functionality. I was using get_symbol() and I added put_symbol() so I
could explicitly refcount it. 

-- 
dwmw2

Re: [PATCH] kill symbol_get & friends

[David Woodhouse]

On Thu, 2005-01-13 at 11:19 +1100, Rusty Russell wrote:
> The lack of users is because, firstly, dynamic dependencies are less
> common than static ones, and secondly because the remaining inter-module
> users (AGP and mtd) have not been converted.  Patches have been sent
> several times, but maintainers are distracted, it seems.  I *will* run
> out of patience and push those patches which take away intermodule.c one
> day (hint, hint!).

I'd be more than happy to see the back of the intermodule crap. I don't
recall seeing patches to remove it again that I'm happy with -- although
you're right, I'm easily distracted.

I'd actually like to go through the whole CFI chip probe and command set
selection crap, understand it all again, and make myself happy with it
rather than just hacking it up further. It's too convoluted, and some of
the reasons for that are no longer relevant.

(Please don't use my broken SPF-afflicted email address, btw. That
address should no longer be present in the kernel sources or
MAINTAINERS.)

-- 
dwmw2

Re: [PATCH] kill symbol_get & friends

[Christoph Hellwig]

On Thu, Jan 13, 2005 at 11:19:33AM +1100, Rusty Russell wrote:
> If you don't hold a reference, then yes, the module can go away.  This
> hasn't been a huge problem for users in the past.

There's a single users, and it has these problems.

> The lack of users is because, firstly, dynamic dependencies are less
> common than static ones, and secondly because the remaining inter-module
> users (AGP and mtd) have not been converted.

AGP doesn't use dynamic symbols anymore, only mtd is gone.  And I'd
rather see it not switching to symbol_get.

Re: [PATCH] kill symbol_get & friends

[Rusty Russell]

On Thu, 2005-01-13 at 18:05 +0100, Christoph Hellwig wrote:
> On Thu, Jan 13, 2005 at 11:19:33AM +1100, Rusty Russell wrote:
> > If you don't hold a reference, then yes, the module can go away.  This
> > hasn't been a huge problem for users in the past.
> 
> There's a single users, and it has these problems.

It is an excellent candidate for weak symbols though.  If you want the
symbols to stay around, of course you have to keep a reference to them.
This code seems silly to me.

> > The lack of users is because, firstly, dynamic dependencies are less
> > common than static ones, and secondly because the remaining inter-module
> > users (AGP and mtd) have not been converted.
> 
> AGP doesn't use dynamic symbols anymore, only mtd is gone.  And I'd
> rather see it not switching to symbol_get.

If it really wants dynamic symbol lookup, that's damn well what's going
to happen.  intermodule must die.  If David doesn't want that feature
any more, then sure, remove it.

Rusty.
-- 
A bad analogy is like a leaky screwdriver -- Richard Braakman

Re: [PATCH] kill symbol_get & friends

[Richard Purdie]

Rusty Russell:
> If it really wants dynamic symbol lookup, that's damn well what's going
> to happen.  intermodule must die.  If David doesn't want that feature
> any more, then sure, remove it.

I can see one scenario where symbol_get would appear to be useful. Say you 
have two modules A and B. Both can run independently of the other. If and 
only if both are loaded at the same time they need to exchange data.

Without symbol_get, you can only have hard dependencies between the modules 
and hence you would be forced into loading both modules even if you only 
want one of them.

I came across this function when trying to solve this exact problem. If the 
function is going to be removed, what is the alternative? Apologies if I've 
missed something obvious...

Richard 

Re: [PATCH] kill symbol_get & friends

[Rusty Russell]

On Sun, 2005-01-16 at 20:46 +0000, Richard Purdie wrote:
> Without symbol_get, you can only have hard dependencies between the modules 
> and hence you would be forced into loading both modules even if you only 
> want one of them.
> 
> I came across this function when trying to solve this exact problem. If the 
> function is going to be removed, what is the alternative? Apologies if I've 
> missed something obvious...

The workaround is to put some registration wedge in the core code (see
net/core/netfilter.c:808 for an example).

But this is exactly what symbol_get is for.  However, if noone needs it,
we can remove it, as keeping infrastructure around because "someone
might need it" is not usually a good idea.

Rusty.
-- 
A bad analogy is like a leaky screwdriver -- Richard Braakman