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