2001-12-22 02:57:47

by Jason Thomas

[permalink] [raw]
Subject: [PATCH] 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.

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


2001-12-22 05:06:18

by Keith Owens

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

On Sat, 22 Dec 2001 13:57:25 +1100,
Jason Thomas <[email protected]> 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));

2001-12-22 05:27:52

by Andrew Morton

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

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.

2001-12-22 09:05:41

by Kai Germaschewski

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

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



2001-12-22 09:14:15

by Keith Owens

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

On Sat, 22 Dec 2001 10:04:19 +0100 (CET),
Kai Germaschewski <[email protected]> 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.

2001-12-25 17:57:00

by Jeff Garzik

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

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


2001-12-25 18:00:10

by Jeff Garzik

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

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