Re: [PATCH] (4/25) sk98: change #define to typedef

Re: [PATCH] (4/25) sk98: change #define to typedef

[Mirko Lindner]

 > I don't think the driver is complex enough for all that os-common mess.
 > If you look at the BSD sk driver it's about a third of the size of the
 > Linux driver because it doesn't have all this mess.

Note that the kernel BSD sk driver was _not_ written by SysKonnect and 
only supports Genesis and Yukon1; linux supports Genesis (single and 
dual), Yukon1, Yukon Plus, Yukon EC, Yukon FE and Yukon2 (single and 
dual). The BSD sk driver supports substantially less functionality than 
the Linux driver. For example, there is no link failover capability in 
the sk driver; it has no power management; numerous chip changes and 
minor tweaks have not been added, since many problems have become 
apparent through our other supported platforms (some 15 operating 
systems) and have since been fixed in our common modules; sk has no 
wake-on-LAN and so on. The official SysKonnect BSD driver (yk) also uses 
the common modules and is not open source.

However, the real point is that the Linux driver (as indeed _all_ of our 
drivers) shares common code with all our supported platforms and this 
necessarily introduces some overhead. But it isn't excessive, and the 
benefits _far_ outweigh the drawbacks, namely, that we have a very wide 
range of systems sharing common code and thus the probability of finding 
and fixing bugs is very substantially higher than if we wrote a 
dedicated driver for each platform.

For instance the symbol SK_IOC mentioned in your mail:

 > -#define SK_IOC char __iomem *
 > +typedef void __iomem *SK_IOC;

is used in a large number of OS independent driver files (e.g. skvpd.c).
The -for instance- file skvpd.c used by our Linux driver is used
at the same time without any changes by all our other drivers running
on other operating systems (e.g. Solaris, Windows).

This is the reason why we need to redefine this symbol to the Linux
specific implementation (void __iomem).

Mirko

Re: [PATCH] (4/25) sk98: change #define to typedef

[Stephen Hemminger]

On Tue, 16 Nov 2004 15:56:15 +0000
Mirko Lindner <mlindner@syskonnect.de> wrote:

>  > I don't think the driver is complex enough for all that os-common mess.
>  > If you look at the BSD sk driver it's about a third of the size of the
>  > Linux driver because it doesn't have all this mess.
> 
> Note that the kernel BSD sk driver was _not_ written by SysKonnect and 
> only supports Genesis and Yukon1; linux supports Genesis (single and 
> dual), Yukon1, Yukon Plus, Yukon EC, Yukon FE and Yukon2 (single and 
> dual). The BSD sk driver supports substantially less functionality than 
> the Linux driver. For example, there is no link failover capability in 
> the sk driver; it has no power management; numerous chip changes and 
> minor tweaks have not been added, since many problems have become 
> apparent through our other supported platforms (some 15 operating 
> systems) and have since been fixed in our common modules; sk has no 
> wake-on-LAN and so on. The official SysKonnect BSD driver (yk) also uses 
> the common modules and is not open source.
> 
> However, the real point is that the Linux driver (as indeed _all_ of our 
> drivers) shares common code with all our supported platforms and this 
> necessarily introduces some overhead. But it isn't excessive, and the 
> benefits _far_ outweigh the drawbacks, namely, that we have a very wide 
> range of systems sharing common code and thus the probability of finding 
> and fixing bugs is very substantially higher than if we wrote a 
> dedicated driver for each platform.
> 
> For instance the symbol SK_IOC mentioned in your mail:
> 
>  > -#define SK_IOC char __iomem *
>  > +typedef void __iomem *SK_IOC;
> 
> is used in a large number of OS independent driver files (e.g. skvpd.c).
> The -for instance- file skvpd.c used by our Linux driver is used
> at the same time without any changes by all our other drivers running
> on other operating systems (e.g. Solaris, Windows).
> 
> This is the reason why we need to redefine this symbol to the Linux
> specific implementation (void __iomem).

I understand the benefits of a common driver, but there are costs too.
First, it means the community can't really support or fix the common code
only SysKonnect can. Also, there is functionality in the common part that
really isn't necessary:
	* Vendor specific MIB support -- what tool could/does use that?
	* Board specific bridging support
If the regular Linux infrastructure was used for these, then the
driver would get additional kernel bridge filtering, common management and security fixes.

My wish is that the common hardware layer could be used without dragging
in all the MIB and bridging stuff.

Re: [PATCH] (4/25) sk98: change #define to typedef

[Christoph Hellwig]

> Note that the kernel BSD sk driver was _not_ written by SysKonnect and 
> only supports Genesis and Yukon1; linux supports Genesis (single and 
> dual), Yukon1, Yukon Plus, Yukon EC, Yukon FE and Yukon2 (single and 
> dual).

They have Yukon2 support for while - unlike Linux.

> The BSD sk driver supports substantially less functionality than 
> the Linux driver. For example, there is no link failover capability in 
> the sk driver;

Which isn't something that belongs into a driver anyway.  It also
means you common code is obsfucated enough that no one noticed :)

> For instance the symbol SK_IOC mentioned in your mail:
> 
> > -#define SK_IOC char __iomem *
> > +typedef void __iomem *SK_IOC;
> 
> is used in a large number of OS independent driver files (e.g. skvpd.c).
> The -for instance- file skvpd.c used by our Linux driver is used
> at the same time without any changes by all our other drivers running
> on other operating systems (e.g. Solaris, Windows).
> 
> This is the reason why we need to redefine this symbol to the Linux
> specific implementation (void __iomem).

And you still haven't explained why your common code uses such
a broken structure instead of having some semi-opaque private data
passed around everywhere.

Also note that we don't new drivers using such horrible "common code"
anymore, you can be happy you sneaked it in at all a few years ago.

Re: [PATCH] (4/25) sk98: change #define to typedef

[Stephen Hemminger]

On Wed, 17 Nov 2004 15:01:34 +0000
Christoph Hellwig <hch@infradead.org> wrote:

> > Note that the kernel BSD sk driver was _not_ written by SysKonnect and 
> > only supports Genesis and Yukon1; linux supports Genesis (single and 
> > dual), Yukon1, Yukon Plus, Yukon EC, Yukon FE and Yukon2 (single and 
> > dual).
> 
> They have Yukon2 support for while - unlike Linux.
> 
> > The BSD sk driver supports substantially less functionality than 
> > the Linux driver. For example, there is no link failover capability in 
> > the sk driver;

Is it even possible to do link failover with existing Open Source tools?

Also the whole Remote Link Mangement (RLMT) stuff has no security so
if it is used over a hostile network look out.

I intrigued by the idea of  of porting FreeBSD driver to Linux. But this card
is already taking more time than I want to spend.

[PATCH] (4/25) sk98: change #define to typedef

[Stephen Hemminger]

Change #define of to a typedef.

Signed-off-by: Stephen Hemminger <shemminger@osdl.org>

diff -Nru a/drivers/net/sk98lin/h/skdrv1st.h b/drivers/net/sk98lin/h/skdrv1st.h
--- a/drivers/net/sk98lin/h/skdrv1st.h	2004-11-15 11:22:28 -08:00
+++ b/drivers/net/sk98lin/h/skdrv1st.h	2004-11-15 11:22:28 -08:00
@@ -106,7 +106,7 @@
 #define SK_MAX_MACS		2
 #define SK_MAX_NETS		2
 
-#define SK_IOC			char __iomem *
+typedef void __iomem *SK_IOC;
 
 typedef struct s_DrvRlmtMbuf SK_MBUF;
 

Re: [PATCH] (4/25) sk98: change #define to typedef

[Christoph Hellwig]

On Mon, Nov 15, 2004 at 03:22:03PM -0800, Stephen Hemminger wrote:
> Change #define of to a typedef.
> 
> Signed-off-by: Stephen Hemminger <shemminger@osdl.org>
> 
> diff -Nru a/drivers/net/sk98lin/h/skdrv1st.h b/drivers/net/sk98lin/h/skdrv1st.h
> --- a/drivers/net/sk98lin/h/skdrv1st.h	2004-11-15 11:22:28 -08:00
> +++ b/drivers/net/sk98lin/h/skdrv1st.h	2004-11-15 11:22:28 -08:00
> @@ -106,7 +106,7 @@
>  #define SK_MAX_MACS		2
>  #define SK_MAX_NETS		2
>  
> -#define SK_IOC			char __iomem *
> +typedef void __iomem *SK_IOC;

Why do you need the typedef at all?  Just about every driver on the
planet is fine with a single void *__iomem iobase; in the per-device
structure