On Tue, May 13, 2008 at 09:17:04AM +0200, Jean Delvare wrote:

Hi Jan,

Le mercredi 07 mai 2008, Jan Beulich a écrit :

...

Parts of the first patch (the actual network driver) are in 2.6.26-rc1, but this one handles more hardware, and without the resource drivers it's not useful for the special virtualization purposes that FATE#303479 is about. Jan

I took a quick look at the network driver patch, and for the little part I can comment on, this is horrible. The driver includes:

* A complete reimplementation of an i2c stack, including software bit-banging (so basically duplicating all of the i2c-core and i2c-algo-bit kernel drivers): 470 lines of code. * A complete reimplementation of the lm87 hardware monitoring driver: 350 lines of code. * Custom code to handle PCA9539 and PCF8575 GPIO chips, while we now have generic drivers for these under drivers/gpio. * Custom code to handle a MAX6647 thermal sensor chip. We don't support this one yes but it really should be a reusable driver under drivers/hwmon rather than custom code hidden deep under drivers/net/sfc.

That's a lot of code duplication and it shows how little the driver author cared about making use of everything the kernel already has support for. And this is only for the part I am familiar with... if the rest of the code is of the same nature, there's a lot of work before this driver will fit reasonably in the kernel. Well, I guess this explains the 24000 lines of code this driver weights. I don't even want to think of the size of the binary module :(

Thanks for reviewing it. In further comments on a different list, Jan has withdrawn the wish to have this code included in 11.0 due to lack of upstream acceptance and review.

Apparently part of this crap managed to make it into the kernel already. I see the i2c stack reimplementation is there. Let me go shout at whoever let this happen.

Good luck, I bet the network driver maintainers didn't even realize it was there :( thanks, greg k-h -- To unsubscribe, e-mail: opensuse-kernel+unsubscribe@opensuse.org For additional commands, e-mail: opensuse-kernel+help@opensuse.org

On Fri, May 16, 2008 at 03:08:40PM +0100, Jan Beulich wrote:

...

Apparently part of this crap managed to make it into the kernel already. I see the i2c stack reimplementation is there. Let me go shout at whoever let this happen.

This is what I just got in response to forwarding your comments to the Solarflare folks:

* I just thought I'd mention that I chatted to some of the developers here * and we do plan to change our in-tree Linux driver to use the * Linux-provided i2c code, and this will come in a subsequent patch. * Andrew Morton was happy for our current driver to be included (in the * interests of getting support in early) on the understanding that this * change is made subsequently. Hopefully the same rationale could be used * at Novell.

I have no objection to taking what is upstream right now, but I do object to taking the portion that is not upstream. So if you wish to redo your patch to be representative of the code that is currently in the 2.6.26-rc2 kernel, I'd be glad to re-review it. thanks, greg k-h -- To unsubscribe, e-mail: opensuse-kernel+unsubscribe@opensuse.org For additional commands, e-mail: opensuse-kernel+help@opensuse.org