Skip to contentSkip to conversation

link errors with internal calls to devexit functions

PATCH In review Jason Thomas 5 participants 7 messages Dec 22, 2001 – Dec 25, 2001

Subsystems (2)
Media (V4L/DVB) USB
lore
Share & tools
lore

Jason Thomas

author
#0 lore raw

PATCH 1/1 link errors with internal calls to devexit functions

please CC me I'm not on the list.

This patch against 2.4.17 fixes internal calls to devexit functions (which is bypasses the devexit_p wrapper) in drivers/media/video/bttv-driver.c and drivers/usb/usb-uhci.c, they are the only two I found.

drivers/media/video/bttv-driver.c Sat Dec 22 13:46:02 2001 renamed from drivers/media/video/bttv-driver.c Sat Dec 22 13:39:39 2001 +2 -0
diff -ur linux-2.4.17.orig/drivers/media/video/bttv-driver.c linux-2.4.17/drivers/media/video/bttv-driver.c
@@ -2992,7 +2992,9 @@
        pci_set_drvdata(dev,btv);
 
        if(init_bt848(btv) < 0) {
+#if defined(MODULE) || defined(CONFIG_HOTPLUG)
                bttv_remove(dev);
+#endif
                return -EIO;
        }
        bttv_num++;
drivers/usb/usb-uhci.c Sat Dec 22 13:46:38 2001 renamed from drivers/usb/usb-uhci.c Sat Dec 22 13:39:39 2001 +2 -0
diff -ur linux-2.4.17.orig/drivers/usb/usb-uhci.c linux-2.4.17/drivers/usb/usb-uhci.c
@@ -3001,7 +3001,9 @@
        s->irq = irq;
 
        if(uhci_start_usb (s) < 0) {
+#if defined(MODULE) || defined(CONFIG_HOTPLUG)
                uhci_pci_remove(dev);
+#endif
                return -1;
        }
Signature · 1 line
Jason Thomas
#1 lore raw
Quoted 23 lines · Jason Thomas

please CC me I'm not on the list.

This patch against 2.4.17 fixes internal calls to devexit functions (which is bypasses the devexit_p wrapper) in drivers/media/video/bttv-driver.c and drivers/usb/usb-uhci.c, they are the only two I found.

diff -ur linux-2.4.17.orig/drivers/media/video/bttv-driver.c linux-2.4.17/drivers/media/video/bttv-driver.c

drivers/media/video/bttv-driver.c Sat Dec 22 13:46:02 2001 renamed from drivers/media/video/bttv-driver.c Sat Dec 22 13:39:39 2001 +2 -0
@@ -2992,7 +2992,9 @@
       pci_set_drvdata(dev,btv);
 
       if(init_bt848(btv) < 0) {
+#if defined(MODULE) || defined(CONFIG_HOTPLUG)
               bttv_remove(dev);
+#endif
               return -EIO;
       }
       bttv_num++;

diff -ur linux-2.4.17.orig/drivers/usb/usb-uhci.c linux-2.4.17/drivers/usb/usb-uhci.c

drivers/usb/usb-uhci.c Sat Dec 22 13:46:38 2001 line 3008

Quoted 8 lines
drivers/usb/usb-uhci.c Sat Dec 22 13:46:38 2001 renamed from drivers/usb/usb-uhci.c Sat Dec 22 13:39:39 2001 +2 -0
@@ -3001,7 +3001,9 @@
       s->irq = irq;
 
       if(uhci_start_usb (s) < 0) {
+#if defined(MODULE) || defined(CONFIG_HOTPLUG)
               uhci_pci_remove(dev);
+#endif
               return -1;
       }

I don't like #if defined(MODULE) || defined(CONFIG_HOTPLUG) in open code. If the rules for what gets discarded change (again) then those ifdefs will be out of sync. That is why __devexit_p() is a wrapper, it is defined once and only has to be changed once when the rules change.

Define a __devexit_call wrapper in include/linux/init.h at the same place that __devexit_p is defined and use the wrapper around the calls. Untested.

#if defined(MODULE) || defined(CONFIG_HOTPLUG)
#define __devexit_p(x) x
#define __devexit_call(x) x
#else
#define __devexit_call(x) do { } while (0)
#define __devexit_p(x) NULL
#endif

	__devexit_call(bttv_remove(dev));
	__devexit_call(uhci_pci_remove(dev));
#2 lore raw
Quoted 19 lines · Keith Owens

drivers/media/video/bttv-driver.c Sat Dec 22 13:46:02 2001 line 3000

Quoted 13 lines

diff -ur linux-2.4.17.orig/drivers/media/video/bttv-driver.c linux-2.4.17/drivers/media/video/bttv-driver.c

drivers/media/video/bttv-driver.c Sat Dec 22 13:46:02 2001 renamed from drivers/media/video/bttv-driver.c Sat Dec 22 13:39:39 2001 +2 -0
@@ -2992,7 +2992,9 @@
       pci_set_drvdata(dev,btv);
 
       if(init_bt848(btv) < 0) {
+#if defined(MODULE) || defined(CONFIG_HOTPLUG)
               bttv_remove(dev);
+#endif
               return -EIO;
       }
       bttv_num++;

I don't like #if defined(MODULE) || defined(CONFIG_HOTPLUG) in open code. If the rules for what gets discarded change (again) then those ifdefs will be out of sync. That is why __devexit_p() is a wrapper, it is defined once and only has to be changed once when the rules change.

Quoted 3 lines

Define a __devexit_call wrapper in include/linux/init.h at the same place that __devexit_p is defined and use the wrapper around the calls. Untested.

I'd rather think that the patch (and the original code) is broken, as it seems we call an __devexit function from an init function. So the correct fix is to remove the __devexit attribute from the offending functions.

--Kai

#3 lore raw
Quoted 4 lines · Kai Germaschewski

I'd rather think that the patch (and the original code) is broken, as it seems we call an __devexit function from an init function. So the correct fix is to remove the __devexit attribute from the offending functions.

Andrew Morton said the same thing and I agree. The code is broken, no amount of fancy wrappers will fix that. This is the perfect example of why the new binutils are good, they are catching broken code.

#4 lore raw
In reply to Jason Thomas
Quoted 3 lines · Keith Owens
        __devexit_call(bttv_remove(dev));
        __devexit_call(uhci_pci_remove(dev));

This may break the drivers. They both call the remove function on the init path, when something failed.

I believe the correct fix here is to simply remove __devinit altogether from the definition of both those functions.

#5 lore raw
In reply to Jason Thomas
Quoted 10 lines · Jason Thomas

please CC me I'm not on the list.

This patch against 2.4.17 fixes internal calls to devexit functions (which is bypasses the devexit_p wrapper) in drivers/media/video/bttv-driver.c and drivers/usb/usb-uhci.c, they are the only two I found.

diff -ur linux-2.4.17.orig/drivers/media/video/bttv-driver.c linux-2.4.17/drivers/media/video/bttv-driver.c

drivers/media/video/bttv-driver.c Sat Dec 22 13:46:02 2001 line 2997

Quoted 6 lines
drivers/media/video/bttv-driver.c Sat Dec 22 13:46:02 2001 renamed from drivers/media/video/bttv-driver.c Sat Dec 22 13:39:39 2001 +2 -0
@@ -2992,7 +2992,9 @@
        pci_set_drvdata(dev,btv);
 
        if(init_bt848(btv) < 0) {
+#if defined(MODULE) || defined(CONFIG_HOTPLUG)
                bttv_remove(dev);
+#endif

These changes are incorrect... if a remove function is called during the init phase it should never have been marked __devexit in the first place.

Jeff

#6 lore raw
In reply to Jason Thomas
Quoted 11 lines · Keith Owens

drivers/media/video/bttv-driver.c Sat Dec 22 13:46:02 2001 line 2997

Quoted 9 lines
drivers/media/video/bttv-driver.c Sat Dec 22 13:46:02 2001 renamed from drivers/media/video/bttv-driver.c Sat Dec 22 13:39:39 2001 +2 -0
@@ -2992,7 +2992,9 @@
       pci_set_drvdata(dev,btv);
 
       if(init_bt848(btv) < 0) {
+#if defined(MODULE) || defined(CONFIG_HOTPLUG)
               bttv_remove(dev);
+#endif

I don't like #if defined(MODULE) || defined(CONFIG_HOTPLUG) in open code.

1000.0% agreed

Quoted 2 lines
	__devexit_call(bttv_remove(dev));
	__devexit_call(uhci_pci_remove(dev));

ug... Just use plain logic: if a function is called from non-devexit code, it should not be marked devexit. That's the bug, we don't need new API crud.

Jeff