[3C509] Fix sysfs leak.

[3C509] Fix sysfs leak.

[davej]

If the driver fails to load, we leave a 3c509 eisa directory
in sysfs.

		Dave

--- linux-2.6.4/drivers/net/3c509.c~	2004-03-15 21:23:55.000000000 +0000
+++ linux-2.6.4/drivers/net/3c509.c	2004-03-15 21:24:30.000000000 +0000
@@ -1657,7 +1655,7 @@
 	}
 
 #ifdef CONFIG_EISA
-	if (eisa_driver_register (&el3_eisa_driver) < 0) {
+	if (eisa_driver_register (&el3_eisa_driver) <= 0) {
 		eisa_driver_unregister (&el3_eisa_driver);
 	}
 #endif

Re: [3C509] Fix sysfs leak.

[Marc Zyngier]

>>>>> "davej" == davej  <davej@redhat.com> writes:

davej>  #ifdef CONFIG_EISA
davej> -	if (eisa_driver_register (&el3_eisa_driver) < 0) {
davej> +	if (eisa_driver_register (&el3_eisa_driver) <= 0) {
davej>  		eisa_driver_unregister (&el3_eisa_driver);
davej>  	}
davej>  #endif

This is bogus. eisa_driver_register returns 0 when it *succeeds*.

	M.
-- 
Places change, faces change. Life is so very strange.

Re: [3C509] Fix sysfs leak.

[Dave Jones]

On Tue, Mar 16, 2004 at 11:56:37AM +0100, Marc Zyngier wrote:
 > >>>>> "davej" == davej  <davej@redhat.com> writes:
 > 
 > davej>  #ifdef CONFIG_EISA
 > davej> -	if (eisa_driver_register (&el3_eisa_driver) < 0) {
 > davej> +	if (eisa_driver_register (&el3_eisa_driver) <= 0) {
 > davej>  		eisa_driver_unregister (&el3_eisa_driver);
 > davej>  	}
 > davej>  #endif
 > 
 > This is bogus. eisa_driver_register returns 0 when it *succeeds*.

Then the probing routine is bogus, it returns 0 when it fails too.

		Dave

Re: [3C509] Fix sysfs leak.

[Marc Zyngier]

>>>>> "Dave" == Dave Jones <davej@redhat.com> writes:

Dave> Then the probing routine is bogus, it returns 0 when it fails too.

Uh ? el3_eisa_probe looks like it properly returns an error...

Or maybe you call a failure not finding a proper device on the bus ?
When the driver registers, the bus may not have been probed yet
(built-in case). So un-registering the driver when it fails to find a
proper device is simply wrong with the current implementation.

In the meantime, 2.6.5-rc1 is broken WRT 3c579.

	M.
-- 
Places change, faces change. Life is so very strange.

Re: [3C509] Fix sysfs leak.

[Dave Jones]

On Tue, Mar 16, 2004 at 03:09:49PM +0100, Marc Zyngier wrote:
 > >>>>> "Dave" == Dave Jones <davej@redhat.com> writes:
 > 
 > Dave> Then the probing routine is bogus, it returns 0 when it fails too.
 > 
 > Uh ? el3_eisa_probe looks like it properly returns an error...
 > 
 > Or maybe you call a failure not finding a proper device on the bus ?

The damned bus doesn't even exist. If this is a case that couldn't be
detected, I'd not be complaining, but this is just nonsense having
a driver claim that its found an EISA device, when there aren't even
any EISA slots on the board.

 > When the driver registers, the bus may not have been probed yet
 > (built-in case). So un-registering the driver when it fails to find a
 > proper device is simply wrong with the current implementation.

This happens long after bus initialisation should have already figured
out that the bus doesn't exist. Even if it was left this late, the
eisa registration code should be doing a 'oh, I've not even checked
if I have a bus yet, I'll do it now' before it starts doing completely
bogus things like checking for devices.

The way I see it, EISA bus support is completely horked right now.

		Dave

Re: [3C509] Fix sysfs leak.

[Marc Zyngier]

>>>>> "Dave" == Dave Jones <davej@redhat.com> writes:

Dave,

Dave> The damned bus doesn't even exist. If this is a case that couldn't be
Dave> detected, I'd not be complaining, but this is just nonsense having
Dave> a driver claim that its found an EISA device, when there aren't even
Dave> any EISA slots on the board.

The driver doesn't claim to have found any device. It just registered
to the EISA framework, which you compiled in for a reason.

Unload the driver and the directory will go.

Dave> This happens long after bus initialisation should have already figured
Dave> out that the bus doesn't exist. Even if it was left this late, the
Dave> eisa registration code should be doing a 'oh, I've not even checked
Dave> if I have a bus yet, I'll do it now' before it starts doing completely
Dave> bogus things like checking for devices.

Sure. When EISA bus is hanging off the PCI bus, which haven't been
probed yet ? When the driver registers, the EISA framework may not
have a f*cking clue about where the EISA bus sits, or if it even
exists.

Dave> The way I see it, EISA bus support is completely horked right now.

Feel free to submit a patch. One that works for x86, Alpha, HPPA and
IP22, with EISA connected to PCI, GSC, GIO or CPU bus. It works nicely
enough for me on these platforms.

Regards,

	M.
-- 
Places change, faces change. Life is so very strange.

Re: [3C509] Fix sysfs leak.

[Dave Jones]

On Tue, Mar 16, 2004 at 04:05:38PM +0100, Marc Zyngier wrote:
 > >>>>> "Dave" == Dave Jones <davej@redhat.com> writes:
 > 
 > Dave,
 > 
 > Dave> The damned bus doesn't even exist. If this is a case that couldn't be
 > Dave> detected, I'd not be complaining, but this is just nonsense having
 > Dave> a driver claim that its found an EISA device, when there aren't even
 > Dave> any EISA slots on the board.
 > 
 > The driver doesn't claim to have found any device. It just registered
 > to the EISA framework, which you compiled in for a reason.

which is valid for a single kernel image that supports boxes both with
and without eisa (ie, vendor kernels).

 > Unload the driver and the directory will go.

no it doesn't, which is the whole purpose of the patch I sent.
try it..

modprobe 3c509
lsmod | grep 3c509     # module didnt stay around
find /sys | grep 3c509 # oh look, it left crap in sysfs


 > Dave> This happens long after bus initialisation should have already figured
 > Dave> out that the bus doesn't exist. Even if it was left this late, the
 > Dave> eisa registration code should be doing a 'oh, I've not even checked
 > Dave> if I have a bus yet, I'll do it now' before it starts doing completely
 > Dave> bogus things like checking for devices.
 > 
 > Sure. When EISA bus is hanging off the PCI bus, which haven't been
 > probed yet ?

We have multiple levels of initcalls for a reason. Whilst they suck in a lot
of ways, they can be used to enforce dependancies like this.

 > When the driver registers, the EISA framework may not
 > have a f*cking clue about where the EISA bus sits, or if it even
 > exists.

Why is this even an issue so late on? Bus probing should have been done as
part of bootup. By the time I get to modprobing device drivers, it should have
been determined already.

Your argument seems to be "probing is hard, so we don't do it", which is
just *wrong*.

		Dave

Re: [3C509] Fix sysfs leak.

[Marc Zyngier]

>>>>> "Dave" == Dave Jones <davej@redhat.com> writes:

Dave> no it doesn't, which is the whole purpose of the patch I sent.
Dave> try it..

Dave> modprobe 3c509
Dave> lsmod | grep 3c509     # module didnt stay around
Dave> find /sys | grep 3c509 # oh look, it left crap in sysfs

Real problem, wrong fix. You just killed 3c509 EISA support
altogether. Could you please test the following patch (against latest
bk) :

===== drivers/net/3c509.c 1.49 vs edited =====
--- 1.49/drivers/net/3c509.c	Mon Mar 15 22:24:30 2004
+++ edited/drivers/net/3c509.c	Tue Mar 16 16:44:24 2004
@@ -1655,14 +1655,14 @@
 	}
 
 #ifdef CONFIG_EISA
-	if (eisa_driver_register (&el3_eisa_driver) <= 0) {
+	if (eisa_driver_register (&el3_eisa_driver) < 0) {
 		eisa_driver_unregister (&el3_eisa_driver);
 	}
 #endif
 #ifdef CONFIG_MCA
 	mca_register_driver(&el3_mca_driver);
 #endif
-	return el3_cards ? 0 : -ENODEV;
+	return 0;
 }
 
 static void __exit el3_cleanup_module(void)

This is not pretty either, but 3c579 probing will work, and in your
case it won't leave a dangling directory in sysfs.

Dave> Why is this even an issue so late on? Bus probing should have
Dave> been done as part of bootup. By the time I get to modprobing
Dave> device drivers, it should have been determined already.

Modprobing is perfectly OK, and indeed everything has been probed at
this stage. But having built-in drivers raises a few different
problems (the driver may be initialized before all busses are probed).

Dave> Your argument seems to be "probing is hard, so we don't do it",
Dave> which is just *wrong*.

If you want to get back to the 2.4 state, where every single EISA
driver reinvents EISA probing, fair enough. I think 2.6 has it mostly
right (at least, it respects the bus hierarchy, which is necessary to
set futile things like dma_mask correctly). Drivers may be broken
(does hp100 rings a bell ?), but the framework looks sane to me.

Again, if you have something better to offer, I'm all ears.

Regards,

	M.
-- 
Places change, faces change. Life is so very strange.

Re: [3C509] Fix sysfs leak.

[Dave Jones]

On Tue, Mar 16, 2004 at 05:05:45PM +0100, Marc Zyngier wrote:
 >  #ifdef CONFIG_MCA
 >  	mca_register_driver(&el3_mca_driver);
 >  #endif
 > -	return el3_cards ? 0 : -ENODEV;
 > +	return 0;
 >  }
 >  
 >  static void __exit el3_cleanup_module(void)
 > 
 > This is not pretty either, but 3c579 probing will work, and in your
 > case it won't leave a dangling directory in sysfs.

Yes, leaving the module around, and cleaning up at rmmod time should
also work.  I'll test it in a while to be sure.

 > Dave> Why is this even an issue so late on? Bus probing should have
 > Dave> been done as part of bootup. By the time I get to modprobing
 > Dave> device drivers, it should have been determined already.
 > 
 > Modprobing is perfectly OK, and indeed everything has been probed at
 > this stage.

Clearly it hadn't, or otherwise modprobing 3c509 would have failed
due to the lack of an eisa bus.

 > But having built-in drivers raises a few different
 > problems (the driver may be initialized before all busses are probed).

There were no built-in drivers in this case.

		Dave

Re: [3C509] Fix sysfs leak.

[Richard B. Johnson]

On Tue, 16 Mar 2004, Dave Jones wrote:

> On Tue, Mar 16, 2004 at 03:09:49PM +0100, Marc Zyngier wrote:
>  > >>>>> "Dave" == Dave Jones <davej@redhat.com> writes:
>  >
>  > Dave> Then the probing routine is bogus, it returns 0 when it fails too.
>  >
>  > Uh ? el3_eisa_probe looks like it properly returns an error...
>  >
>  > Or maybe you call a failure not finding a proper device on the bus ?
>
> The damned bus doesn't even exist. If this is a case that couldn't be
> detected, I'd not be complaining, but this is just nonsense having
> a driver claim that its found an EISA device, when there aren't even
> any EISA slots on the board.

There is no way that any software knows about an EISA bus. It
only knows that there is some device at some port. Since a 3c503
was built to go into an 8-bit EISA slot, if one is found it
is assumed to be in such a slot on the EISA bus!

So, if the device doesn't exist there is a problem with the
detection method for the device, not a detection method for
a bus because the bus can't be detected at all.

Cheers,
Dick Johnson
Penguin : Linux version 2.4.24 on an i686 machine (797.90 BogoMips).
            Note 96.31% of all statistics are fiction.

Re: [3C509] Fix sysfs leak.

[Vojtech Pavlik]

On Tue, Mar 16, 2004 at 10:58:08AM -0500, Richard B. Johnson wrote:
> On Tue, 16 Mar 2004, Dave Jones wrote:
> 
> > On Tue, Mar 16, 2004 at 03:09:49PM +0100, Marc Zyngier wrote:
> >  > >>>>> "Dave" == Dave Jones <davej@redhat.com> writes:
> >  >
> >  > Dave> Then the probing routine is bogus, it returns 0 when it fails too.
> >  >
> >  > Uh ? el3_eisa_probe looks like it properly returns an error...
> >  >
> >  > Or maybe you call a failure not finding a proper device on the bus ?
> >
> > The damned bus doesn't even exist. If this is a case that couldn't be
> > detected, I'd not be complaining, but this is just nonsense having
> > a driver claim that its found an EISA device, when there aren't even
> > any EISA slots on the board.
> 
> There is no way that any software knows about an EISA bus. It
> only knows that there is some device at some port. Since a 3c503
> was built to go into an 8-bit EISA slot, if one is found it
> is assumed to be in such a slot on the EISA bus!
> 
> So, if the device doesn't exist there is a problem with the
> detection method for the device, not a detection method for
> a bus because the bus can't be detected at all.

3c503 in EISA? 8-bit EISA? I think you mean ISA ...

EISA has slot information available, as far as I know, and device
identifiers and ... and is a 32-bit bus.

-- 
Vojtech Pavlik
SuSE Labs, SuSE CR

Re: [3C509] Fix sysfs leak.

[Jeff Garzik]

Dave Jones wrote:
> On Tue, Mar 16, 2004 at 11:56:37AM +0100, Marc Zyngier wrote:
>  > >>>>> "davej" == davej  <davej@redhat.com> writes:
>  > 
>  > davej>  #ifdef CONFIG_EISA
>  > davej> -	if (eisa_driver_register (&el3_eisa_driver) < 0) {
>  > davej> +	if (eisa_driver_register (&el3_eisa_driver) <= 0) {
>  > davej>  		eisa_driver_unregister (&el3_eisa_driver);
>  > davej>  	}
>  > davej>  #endif
>  > 
>  > This is bogus. eisa_driver_register returns 0 when it *succeeds*.
> 
> Then the probing routine is bogus, it returns 0 when it fails too.

No, for the hotplug case the API allows registration to succeed, then 
probing calls the ->init_one function later.

	Jeff

Re: [3C509] Fix sysfs leak.

[Dave Jones]

On Tue, Mar 16, 2004 at 11:06:10AM -0500, Jeff Garzik wrote:
 > Dave Jones wrote:
 > >On Tue, Mar 16, 2004 at 11:56:37AM +0100, Marc Zyngier wrote:
 > > > >>>>> "davej" == davej  <davej@redhat.com> writes:
 > > > 
 > > > davej>  #ifdef CONFIG_EISA
 > > > davej> -	if (eisa_driver_register (&el3_eisa_driver) < 0) {
 > > > davej> +	if (eisa_driver_register (&el3_eisa_driver) <= 0) {
 > > > davej>  		eisa_driver_unregister (&el3_eisa_driver);
 > > > davej>  	}
 > > > davej>  #endif
 > > > 
 > > > This is bogus. eisa_driver_register returns 0 when it *succeeds*.
 > >
 > >Then the probing routine is bogus, it returns 0 when it fails too.
 > 
 > No, for the hotplug case the API allows registration to succeed, then 
 > probing calls the ->init_one function later.

Not when the module buggers off afterwards it doesn't.
->init_one points to lala land at that point.

		Dave