[PATCH] link errors with internal calls to devexit functions

[PATCH] link errors with internal calls to devexit functions

[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
--- linux-2.4.17.orig/drivers/media/video/bttv-driver.c Sat Dec 22 13:39:39 2001
+++ linux-2.4.17/drivers/media/video/bttv-driver.c      Sat Dec 22 13:46:02 2001
@@ -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
--- linux-2.4.17.orig/drivers/usb/usb-uhci.c    Sat Dec 22 13:39:39 2001
+++ linux-2.4.17/drivers/usb/usb-uhci.c Sat Dec 22 13:46:38 2001
@@ -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;
        }

-- 
Jason Thomas

Re: [PATCH] link errors with internal calls to devexit functions

[Keith Owens]

On Sat, 22 Dec 2001 13:57:25 +1100, 
Jason Thomas <jason@topic.com.au> wrote:
>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
>--- linux-2.4.17.orig/drivers/media/video/bttv-driver.c Sat Dec 22 13:39:39 2001
>+++ linux-2.4.17/drivers/media/video/bttv-driver.c      Sat Dec 22 13:46:02 2001
>@@ -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
>--- linux-2.4.17.orig/drivers/usb/usb-uhci.c    Sat Dec 22 13:39:39 2001
>+++ linux-2.4.17/drivers/usb/usb-uhci.c Sat Dec 22 13:46:38 2001
>@@ -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));

Re: [PATCH] link errors with internal calls to devexit functions

[Andrew Morton]

Keith Owens wrote:
> 
>         __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.

Re: [PATCH] link errors with internal calls to devexit functions

[Kai Germaschewski]

On Sat, 22 Dec 2001, Keith Owens wrote:

> >diff -ur linux-2.4.17.orig/drivers/media/video/bttv-driver.c linux-2.4.17/drivers/media/video/bttv-driver.c
> >--- linux-2.4.17.orig/drivers/media/video/bttv-driver.c Sat Dec 22 13:39:39 2001
> >+++ linux-2.4.17/drivers/media/video/bttv-driver.c      Sat Dec 22 13:46:02 2001
> >@@ -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.
> 
> 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

Re: [PATCH] link errors with internal calls to devexit functions

[Keith Owens]

On Sat, 22 Dec 2001 10:04:19 +0100 (CET), 
Kai Germaschewski <kai@tp1.ruhr-uni-bochum.de> wrote:
>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.

Re: [PATCH] link errors with internal calls to devexit functions

[Legacy Fishtank]

On Sat, Dec 22, 2001 at 01:57:25PM +1100, Jason Thomas wrote:
> 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
> --- linux-2.4.17.orig/drivers/media/video/bttv-driver.c Sat Dec 22 13:39:39 2001
> +++ linux-2.4.17/drivers/media/video/bttv-driver.c      Sat Dec 22 13:46:02 2001
> @@ -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

Re: [PATCH] link errors with internal calls to devexit functions

[Legacy Fishtank]

On Sat, Dec 22, 2001 at 04:05:43PM +1100, Keith Owens wrote:
> >--- linux-2.4.17.orig/drivers/media/video/bttv-driver.c Sat Dec 22 13:39:39 2001
> >+++ linux-2.4.17/drivers/media/video/bttv-driver.c      Sat Dec 22 13:46:02 2001
> >@@ -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


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

[PATCH] link errors with internal calls to devexit functions

[Jason Thomas]

Marcelo, Heres my second attempt at a patch to fix these compile time
issues can you check it over and possibly include it in your next
release.

diff -ur linux-2.4.18-pre2.orig/drivers/media/video/bttv-driver.c linux-2.4.18-pre2/drivers/media/video/bttv-driver.c
--- linux-2.4.18-pre2.orig/drivers/media/video/bttv-driver.c	Sat Dec 22 13:39:39 2001
+++ linux-2.4.18-pre2/drivers/media/video/bttv-driver.c	Wed Jan  9 13:25:18 2002
@@ -2820,11 +2820,10 @@
  *	Scan for a Bt848 card, request the irq and map the io memory 
  */
 
-static void __devexit bttv_remove(struct pci_dev *pci_dev)
+static void bttv_remove_card(struct bttv *btv)
 {
         u8 command;
         int j;
-        struct bttv *btv = pci_get_drvdata(pci_dev);
 
 	if (bttv_verbose)
 		printk("bttv%d: unloading\n",btv->nr);
@@ -2890,10 +2889,18 @@
         btv->shutdown=1;
         wake_up(&btv->gpioq);
 
-	pci_set_drvdata(pci_dev, NULL);
         return;
 }
 
+static void __devexit bttv_remove(struct pci_dev *pci_dev)
+{
+        struct bttv *btv = pci_get_drvdata(pci_dev);
+		
+	if (btv) {
+		bttv_remove_card(btv);
+		pci_set_drvdata(pci_dev, NULL);
+	}
+}
 
 static int __devinit bttv_probe(struct pci_dev *dev, const struct pci_device_id *pci_id)
 {
@@ -2992,7 +2999,7 @@
 	pci_set_drvdata(dev,btv);
 
 	if(init_bt848(btv) < 0) {
-		bttv_remove(dev);
+		bttv_remove_card(btv);
 		return -EIO;
 	}
 	bttv_num++;
diff -ur linux-2.4.18-pre2.orig/drivers/usb/usb-uhci.c linux-2.4.18-pre2/drivers/usb/usb-uhci.c
--- linux-2.4.18-pre2.orig/drivers/usb/usb-uhci.c	Sat Dec 22 13:39:39 2001
+++ linux-2.4.18-pre2/drivers/usb/usb-uhci.c	Wed Jan  9 14:11:19 2002
@@ -2845,10 +2845,9 @@
 	s->running = 1;
 }
 
-_static void __devexit
-uhci_pci_remove (struct pci_dev *dev)
+_static void
+uhci_pci_remove_card (uhci_t *s)
 {
-	uhci_t *s = pci_get_drvdata(dev);
 	struct usb_device *root_hub = s->bus->root_hub;
 
 	s->running = 0;		    // Don't allow submit_urb
@@ -2868,7 +2867,17 @@
 	free_irq (s->irq, s);
 	usb_free_bus (s->bus);
 	cleanup_skel (s);
-	kfree (s);
+}
+
+_static void __devexit
+uhci_pci_remove (struct pci_dev *dev)
+{
+	uhci_t *s = pci_get_drvdata(dev);
+	
+	if (s) {
+		uhci_pci_remove_card(s);
+		kfree (s);
+	}
 }
 
 _static int __init uhci_start_usb (uhci_t *s)
@@ -3001,7 +3010,8 @@
 	s->irq = irq;
 
 	if(uhci_start_usb (s) < 0) {
-		uhci_pci_remove(dev);
+		uhci_pci_remove_card(s);
+		kfree(s);
 		return -1;
 	}