[2.6] IBM PowerPC Virtual Ethernet Driver

Here's a patch that adds the inter-partition Virtual Ethernet driver for newer IBM iSeries and pSeries systems:

The patch applies against 2.6.3-rc3...

Jeff, can you formally add this driver to 2.6?... The differences between this driver and the 2.4 driver that you accepted are fairly trivial (i.e. workqueues instead of tasklets)... The architectural additions that I was waiting for have been applied to the mainline tree...

Comments and suggestions are always welcome...

Santiago A. Leon
Power Linux Development
IBM Linux Technology Center

diff -Nru a/drivers/net/Kconfig b/drivers/net/Kconfig
@@ -2162,6 +2162,17 @@
 	  under Linux, say Y here (you must also remember to enable the driver
 	  for your HIPPI card below). Most people will say N here.
 
+config IBMVETH
+	tristate "IBM LAN Virtual Ethernet support"
+	depends on NETDEVICES && NET_ETHERNET && PPC_PSERIES
+	---help---
+	  This driver supports virtual ethernet adapters on newer IBM iSeries
+	  and pSeries systems.
+
+	  To compile this driver as a module, choose M here and read
+	  <file:Documentation/networking/net-modules.txt>. The module will
+	  be called ibmveth.
+
 config ROADRUNNER
 	tristate "Essential RoadRunner HIPPI PCI adapter support (EXPERIMENTAL)"
 	depends on HIPPI && PCI

diff -Nru a/drivers/net/Makefile b/drivers/net/Makefile
@@ -175,6 +175,7 @@
 obj-$(CONFIG_DL2K) += dl2k.o
 obj-$(CONFIG_R8169) += r8169.o
 obj-$(CONFIG_AMD8111_ETH) += amd8111e.o
+obj-$(CONFIG_IBMVETH) += ibmveth.o
 
 obj-$(CONFIG_ARM) += arm/
 obj-$(CONFIG_NET_FC) += fc/

Let me put my though police hat for a short while...

BITFIELDS ARE EVIL !!!

Especially when mapping things like HW registers... I know the p/iSeries code is full of them, I'd strongly recommend getting rid of them.

The compiler is perfectly free, afaik, to re-order them, you have no guarantee of your actual layout, and it's not portable (I know nothing but PPC uses those drivers _right_ now, but still...)

So please, don't add more ;)

Ben.

That can't be right surely ? That would make them utterly useless afaics. I've not seen this happen in practice either with the 2 x86 cpufreq drivers I wrote that both use bitfields extensively.

Dave

It _is_ right and they are utterly useless. Original rationale was, indeed, "describe the layout of hardware registers" but it had gone to hell may years ago. Any assumptions regarding their allocation are non-portable.

Well, to be fair, most compilers still aim to make them useful.

There's a difference between "the standard doesn't guarantee anything" and "the implementation makes no sense".

(Sadly, a lot of compiler people do seem to look to standards more than actual users for guides to do things, but at the same time I do believe that gcc has useful semantics for bitfields and hardware accesses. You just have to know what the implementation-specific rules are)

Linus

Well... I still think it's asking for trouble in the long term to rely on them .... It's definitely broken for drivers of devices that can be used on different archs (PCI cards for example) since the layout of the bitfields is different at least with gcc between big and little endian machines. But I've also been bitten by alignement issues within the bitfield in the past (not with gcc though) and other funny things like that...

So overall, I just recommend to get rid of them.

Ben.

I mostly agree.

You _can_ get bitfields correct, but it takes some care. And btw, bit endianness within a word does not necessarily match the byte endianness, although they are usually connected.

See for example <linux/tcp.h> for what you need to do to make bitfields work right across architectures.

In general, yes. You can make bitfields be more readable, but you need to be careful. And we all know how careful most driver writers tend to be.

Linus

True. However, most of the bitfield uses I've seen would become much cleaner from rewrite to explicit mask-and-shift stuff.

Not it is not. C standard (ISO/IEC 9899:1999 (E)), section 6.7.2.1.

10 ... If enough space remains, a bit-field that immediately follows
another bit-field in a structure shall be packed into adjacent bits of the same unit. ...

13 Within a structure object, the non-bit-field members and the units
in which bit-fields reside have addresses that increase in the order in which they are declared. ...

There is no scope for a compiler to reorder the members or the bit fields of a structure.

Problem is portability between different architectures, especially little endian versus big endian. The 3c59x driver used to use a bitfield to represent hardware registers and had some lovely ifdeffery to make it work on ppc hardware.

It's safe enough in arch-specific code but one may as well use the same idioms as are used in generic code.

The actual bit ordering is implementation-defined, though, so in effect the bitfield setup _does_ depend on the compiler and architecture details.

Linus

He he...

Thanks for the comments and discussion... I changed the bitfields to masks and shifts and I'm attaching the full patch (not just the bitfield changes)...

Jeff, If everything else looks ok, can you add it to 2.6?... I'll send you a patch that fixes the bitfields for the 2.4 version soon...

Santiago A. Leon
Power Linux Development
IBM Linux Technology Center

diff -Nru a/drivers/net/Kconfig b/drivers/net/Kconfig
@@ -2162,6 +2162,17 @@
 	  under Linux, say Y here (you must also remember to enable the driver
 	  for your HIPPI card below). Most people will say N here.
 
+config IBMVETH
+	tristate "IBM LAN Virtual Ethernet support"
+	depends on NETDEVICES && NET_ETHERNET && PPC_PSERIES
+	---help---
+	  This driver supports virtual ethernet adapters on newer IBM iSeries
+	  and pSeries systems.
+
+	  To compile this driver as a module, choose M here and read
+	  <file:Documentation/networking/net-modules.txt>. The module will
+	  be called ibmveth.
+
 config ROADRUNNER
 	tristate "Essential RoadRunner HIPPI PCI adapter support (EXPERIMENTAL)"
 	depends on HIPPI && PCI

diff -Nru a/drivers/net/Makefile b/drivers/net/Makefile
@@ -175,6 +175,7 @@
 obj-$(CONFIG_DL2K) += dl2k.o
 obj-$(CONFIG_R8169) += r8169.o
 obj-$(CONFIG_AMD8111_ETH) += amd8111e.o
+obj-$(CONFIG_IBMVETH) += ibmveth.o
 
 obj-$(CONFIG_ARM) += arm/
 obj-$(CONFIG_NET_FC) += fc/

Hi!

Could we get

..less asciiart:

/*
*
*/

instead of

/**************
* *
**************/

and shorter identifiers so it fits on, say 100 columns?

Pavel

When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

applied