kill symbol_get & friends

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_*)

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

@@ -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;

@@ -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 =====

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

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

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

Dave.

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

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.

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

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 must not respond to Rusty's taunts..

8-)
Dave

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

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

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

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.

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

Rusty Russell:

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

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