Date:	Wed, 10 Oct 2007 17:59:42 -0700 (PDT)
From:	David Miller <davem@...emloft.net>
To:	eliezert@...adcom.com
Cc:	netdev@...r.kernel.org, jeff@...zik.org, mchan@...adcom.com
Subject: Re: [PATCH][BNX2X] round three

From: "Eliezer Tamir" <eliezert@...adcom.com>
Date: Wed, 10 Oct 2007 21:52:30 +0200

> Eliezer Tamir wrote:
> > The full patch is available at:
> > Net_sys_anon@ftp://ftp1.broadcom.com/0001-BNX2X-0.40.10a-net-2.6.24.patch
> 
> Just when I thought I have beaten the line beast.
> (or maybe it's just too much work and not enough sleep.)
> 
> the right links are of course:
> ftp://Net_sys_anon@...1.broadcom.com/0001-BNX2X-0.40.10a-net-2.6.24.patch
> 
> and
> ftp://Net_sys_anon@...1.broadcom.com/0001-BNX2X-0.40.10a-net-2.6.24.patch.gz

I was going to add this to the tree for 2.6.24 but there is simply
too much super-ugly stuff in this driver for me to do so.

bnx2x_hsi.h is a complete mess, it seems to be a concatenation of
several internal header files automatically generated by some
program.  It also defines datastructures for which no tabbing
or coding style has been applied, like this

+struct drv_fw_mb_t
+{
+u32 drv_mb_header;

This is why we hate machine generated register lists and similar
things. They are awful to read by human beings, the very people who
might need to read this to work on helping to fix a bug.

I always type every register offset and bit definition in by hand and
make them properly tabbed and look pleasant to human beings who might
have to stare at it.

+#ifndef __ETH_CONSTANTS_H_
+#define __ETH_CONSTANTS_H_
...
+#ifndef __MICROCODE_CONSTANTS_H_
+#define __MICROCODE_CONSTANTS_H_

All in one header file...  Barf...

+static const struct arb_line read_arb_data[NUM_RD_Q][MAX_RD_ORD + 1] = {
+	{{ 8 , 64 , 25}, { 16 , 64 , 25}, { 32 , 64 , 25}, { 64 , 64 , 41}},
+	{{ 4 , 8 , 4},   { 4 , 8 , 4},    { 4 , 8 , 4},    { 4 , 8 , 4}},
+	{{ 4 , 3 , 3},   { 4 , 3 , 3},    { 4 , 3 , 3},    { 4 , 3 , 3}},
+	{{ 8 , 3 , 6},   { 16 , 3 , 11},  { 16 , 3 , 11},  { 16 , 3 , 11}},
+	{{ 8 , 64 , 25}, { 16 , 64 , 25}, { 32 , 64 , 25}, { 64 , 64 , 41}},

Magic constants, what do they mean?

+static const u32 EVST_TSEM_FAST_MEMORY_COMMON_MEMORY_INIT_ASIC_7[] = {
+0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,
+0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,
 ...
+0x0, 0x0, 0x0, 0x1};

All zeros with a trailing one, remove this table and just program this
sequence by hand.  This table serves nothing but to waste unswappable
kernel memory.  I don't care if they are dependent upon firmware
changes.

There are tons of tables like this, fix them.

+static const u32 EVST_TSEM_FAST_MEMORY_PORT0_MEMORY_INIT_ASIC_27[] = {
+0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,
+0xffffffff, 0xffffffff};

What do these magic constant masks mean?

+static const u32 EVST_CDU_L1TT_COMMON_MEMORY_INIT_HARDWARE[] = {
+0xfffffff3, 0x314fffff, 0xc30c30c, 0xc30c30c3, 0xcf3cf300, 0xf3cf3cf3, 0xcf3c,

Magic constants, what are they?

+static const u32 EVST_CDU_MATT_COMMON_MEMORY_INIT_HARDWARE[] = {
+0xa0000, 0x700a0, 0x28110, 0xb8138, 0x201f0, 0x10210, 0xf0220, 0x10310,
+0x80000, 0x80080, 0x28100, 0xb8128, 0x201e0, 0x10200, 0x70210, 0x20280,

Similarly.

+INIT_REG_WR(TSDM, TSDM_REGISTERS_ENABLE_IN1, 0X7FFFFFF, COMMON, INIT_HARDWARE);
+INIT_REG_WR(TSDM, TSDM_REGISTERS_ENABLE_IN2, 0X3F, COMMON, INIT_HARDWARE);
+INIT_REG_WR(TSDM, TSDM_REGISTERS_ENABLE_OUT1, 0X7FFFFFF, COMMON, INIT_HARDWARE);
+INIT_REG_WR(TSDM, TSDM_REGISTERS_ENABLE_OUT2, 0XF, COMMON, INIT_HARDWARE);

Magic undocumented constants.

+static const u32 EVST_TCM_XX_DESCR_TABLE_COMMON_MEMORY_INIT_HARDWARE[] = {
+0x10000, 0x204c0, 0x30980, 0x40e40, 0x51300, 0x617c0, 0x71c80, 0x82140,
+0x92600, 0xa2ac0, 0xb2f80, 0xc3440, 0xd3900, 0xe3dc0, 0xf4280, 0x104740,
+0x114c00, 0x1250c0, 0x135580, 0x145a40, 0x155f00, 0x1663c0, 0x176880, 0x186d40,
+0x197200, 0x1a76c0, 0x1b7b80, 0x1c8040, 0x1d8500, 0x1e89c0, 0x1f8e80,
+0x209340};

More of same.

Look, there is zero way I'm adding a driver that's written like this
to the tree.

We've set high standards for these sorts of issues in previous
Broadcom network chip drivers and we're not about to start regressing
in this area.

I don't care if it's easier for you guys to run some program which
autogenerates tables than to code it up cleanly into C function
statements which purposefully program each element of the hardware.

This huge header file full of register programming magic goes way
beyond the limits of "reasonable" and has to go.

If that big concatenated header file shows up in the next submission
I'm not even going to review it, so please don't waste my time in
this way.

Thanks.
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists