[2.5 patch] let COMX depend on PROC_FS

[2.5 patch] let COMX depend on PROC_FS

[Adrian Bunk]

>From drivers/net/wan/comx.c:

<--  snip  -->

...
#ifndef CONFIG_PROC_FS
#error For now, COMX really needs the /proc filesystem
#endif
...

<--  snip  -->


The following patch add a dependency to Kconfig to avoid compile errors 
with CONFIG_COMX and !CONFIG_PROC_FS:


--- linux-2.5.70-mm6/drivers/net/wan/Kconfig.old	2003-06-08 15:54:41.000000000 +0200
+++ linux-2.5.70-mm6/drivers/net/wan/Kconfig	2003-06-08 15:55:14.000000000 +0200
@@ -62,7 +62,7 @@
 #
 config COMX
 	tristate "MultiGate (COMX) synchronous serial boards support"
-	depends on WAN && (ISA || PCI)
+	depends on WAN && (ISA || PCI) && PROC_FS
 	---help---
 	  Say Y if you want to use any board from the MultiGate (COMX) family.
 	  These boards are synchronous serial adapters for the PC,



cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [2.5 patch] let COMX depend on PROC_FS

[Christoph Hellwig]

On Sun, Jun 08, 2003 at 04:40:38PM +0200, Adrian Bunk wrote:
> >From drivers/net/wan/comx.c:
> 
> <--  snip  -->
> 
> ...
> #ifndef CONFIG_PROC_FS
> #error For now, COMX really needs the /proc filesystem
> #endif
> ...
> 
> <--  snip  -->
> 
> 
> The following patch add a dependency to Kconfig to avoid compile errors 
> with CONFIG_COMX and !CONFIG_PROC_FS:

Actually it still doesn't link with this because the procfs code in it
is utter crap and relies on a symbol proc_get_inode that isn't exported
since 2.3.  As no one cared for this driver over years I'd suggest just
removing it.

Re: [2.5 patch] let COMX depend on PROC_FS

[Adrian Bunk]

On Sun, Jun 08, 2003 at 05:49:08PM +0100, Christoph Hellwig wrote:
> On Sun, Jun 08, 2003 at 04:40:38PM +0200, Adrian Bunk wrote:
> > >From drivers/net/wan/comx.c:
> > 
> > <--  snip  -->
> > 
> > ...
> > #ifndef CONFIG_PROC_FS
> > #error For now, COMX really needs the /proc filesystem
> > #endif
> > ...
> > 
> > <--  snip  -->
> > 
> > 
> > The following patch add a dependency to Kconfig to avoid compile errors 
> > with CONFIG_COMX and !CONFIG_PROC_FS:
> 
> Actually it still doesn't link with this because the procfs code in it
> is utter crap and relies on a symbol proc_get_inode that isn't exported
> since 2.3.  As no one cared for this driver over years I'd suggest just
> removing it.

The proc_get_inode link problem only affects the modular build of 
comx.c .

The static build works fine in both 2.4 and 2.5.

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [2.5 patch] let COMX depend on PROC_FS

[Christoph Hellwig]

On Sun, Jun 08, 2003 at 06:56:08PM +0200, Adrian Bunk wrote:
> The proc_get_inode link problem only affects the modular build of 
> comx.c .

But it's still broken :)  This just shows no one actually tested
it with actual hardware.

Re: [2.5 patch] let COMX depend on PROC_FS

[Pásztor Szilárd]

Christoph Hellwig:
> > The proc_get_inode link problem only affects the modular build of 
> > comx.c .
> 
> But it's still broken :)  This just shows no one actually tested
> it with actual hardware.

I'm the current "maintainter" of the comx drivers (seriously lacked time up
to now), so it's me to flame if you have some spare fuel. And forgive me for
having forgot to update the maintainer line in comx.c (comx-* are fine). :)

The drivers are used by some hundreds of cards today but we tell users to
get the small kernelpatch from www.itc.hu and the patch, among other things,
exports proc_get_inode. There was a process to integrate the patch into the
mainstream kernel last year but, due to lack of time on my part, it was
suspended. I hope to be able to pick the line up again and clean things up.

s.
          ------------------------------------------------------------
          |  Programmers don't die, they just GOSUB without RETURN.  |
          ------------------------------------------------------------

Re: [2.5 patch] let COMX depend on PROC_FS

[Christoph Hellwig]

On Tue, Jun 10, 2003 at 01:55:22PM +0200, Pásztor Szilárd wrote:
> The drivers are used by some hundreds of cards today but we tell users to
> get the small kernelpatch from www.itc.hu and the patch, among other things,
> exports proc_get_inode. There was a process to integrate the patch into the
> mainstream kernel last year but, due to lack of time on my part, it was
> suspended. I hope to be able to pick the line up again and clean things up.

So what about fixing it instead?  The usage of proc_get_inode is broken
and so is the whole profs mess in the comx driver.  If you want to keep
the API you need to add a ramfs-style filesystem instead of abusing
procfs.

Re: [2.5 patch] let COMX depend on PROC_FS

[Pásztor Szilárd]

Christoph Hellwig:
> So what about fixing it instead?  The usage of proc_get_inode is broken
> and so is the whole profs mess in the comx driver.  If you want to keep
> the API you need to add a ramfs-style filesystem instead of abusing
> procfs.

Is the case the same with the SCSI drivers, IDE drivers, network core,
filesystems and everything that creates directories and file entries in
procfs?

              ---------------------------------------------------
              |  Widows '95 - The Micro$oft Solution Preventer  |
              ---------------------------------------------------

Re: [2.5 patch] let COMX depend on PROC_FS

[Christoph Hellwig]

On Tue, Jun 10, 2003 at 03:51:09PM +0200, Pásztor Szilárd wrote:
> Is the case the same with the SCSI drivers, IDE drivers, network core,
> filesystems and everything that creates directories and file entries in
> procfs?

No.  The problem with comx is that unlike other driver it doesn't
not use the published procfs API but instead tries to implemented
half of an own filesystem abusing procfs infrastructure.

Re: [2.5 patch] let COMX depend on PROC_FS

[Pásztor Szilárd]

Christoph Hellwig:
> > Is the case the same with the SCSI drivers, IDE drivers, network core,
> > filesystems and everything that creates directories and file entries in
> > procfs?
> 
> No.  The problem with comx is that unlike other driver it doesn't
> not use the published procfs API but instead tries to implemented
> half of an own filesystem abusing procfs infrastructure.

It'll get updated and fixed.

                 ----------------------------------------------
                 |  If you can't learn to do something well,  |
                 |      learn to enjoy doing it poorly.       |
                 ----------------------------------------------

Re: [2.5 patch] let COMX depend on PROC_FS

[viro]

On Tue, Jun 10, 2003 at 02:26:14PM +0100, Christoph Hellwig wrote:
> On Tue, Jun 10, 2003 at 01:55:22PM +0200, Pásztor Szilárd wrote:
> > The drivers are used by some hundreds of cards today but we tell users to
> > get the small kernelpatch from www.itc.hu and the patch, among other things,
> > exports proc_get_inode. There was a process to integrate the patch into the
> > mainstream kernel last year but, due to lack of time on my part, it was
> > suspended. I hope to be able to pick the line up again and clean things up.
> 
> So what about fixing it instead?  The usage of proc_get_inode is broken
> and so is the whole profs mess in the comx driver.  If you want to keep
> the API you need to add a ramfs-style filesystem instead of abusing
> procfs.

"broken" is a very polite way to describe that driver.  Starting with the
idea of mkdir in virtual filesystem (procfs or otherwise) creating and
populating a diretory (unmodifiable, BTW) and rmdir - removing it, even
though it's non-empty (and can't be emptied, due to above).

Guys, that's _sick_.  And that's aside of the shitload of races all over
that code (no locking whatsoever).  And kmalloc(..., GFP_KERNEL) with
interrupts disabled.  And shutting the hardware down before unregistering
netdev (yes, you check that it's down; nothing guarantees that it will
stay down while you do ->hw_exit() and friends).  And so on, and so on...

IOW, driver needs a serious rewrite, starting with its API.