[PATCH net-next 00/11] tg3: Bugfixes and 5719 support

[PATCH net-next 00/11] tg3: Bugfixes and 5719 support

[Matt Carlson]

This patchset adds some bugfixes and adds 5719 device support.

Re: [PATCH net-next 00/11] tg3: Bugfixes and 5719 support

[David Miller]

From: "Matt Carlson" <mcarlson@broadcom.com>
Date: Sat, 5 Jun 2010 20:24:29 -0700

> This patchset adds some bugfixes and adds 5719 device support.

All applied to net-next-2.6 but there are two things I'm very disappointed
with in this series:

1) Naming register bits things like "TX_MBUF_FIX" isn't descriptive,
   and I suspect the actual bit name used in your programming manuals
   is very different and would be more helpful to someone reading the
   code and trying to understand exactly what that bit does.

   How does it change the chips internal MBUF handling behavior?  Does
   it insert a delay in accesses or state changes?  Does it change
   the MBUF arbitration?  What the heck does that bit do exactly?

2) Removing register definitions is something we really shouldn't do.
   Just because you're not using the register any more in the driver,
   doesn't mean you should remove it's definition from tg3.h

   What if some other developer wants to play with that register and
   use it for some other purpose or experiment?

You really have to handle situations like #1 and #2 better especially
since you guys do not public post the full PDF hardware programming
manuals of your chips online for other developers to use.

I wouldn't, therefore, impose these rules on Intel and their drivers
because they do public the programming manuals for their networking
chips.

Re: [PATCH net-next 00/11] tg3: Bugfixes and 5719 support

[Matt Carlson]

On Sun, Jun 06, 2010 at 06:00:29PM -0700, David Miller wrote:
> From: "Matt Carlson" <mcarlson@broadcom.com>
> Date: Sat, 5 Jun 2010 20:24:29 -0700
> 
> > This patchset adds some bugfixes and adds 5719 device support.
> 
> All applied to net-next-2.6 but there are two things I'm very disappointed
> with in this series:
> 
> 1) Naming register bits things like "TX_MBUF_FIX" isn't descriptive,
>    and I suspect the actual bit name used in your programming manuals
>    is very different and would be more helpful to someone reading the
>    code and trying to understand exactly what that bit does.

Actually, the programming manual describes this register just as
tersely.

>    How does it change the chips internal MBUF handling behavior?  Does
>    it insert a delay in accesses or state changes?  Does it change
>    the MBUF arbitration?  What the heck does that bit do exactly?

The programming manual says this bit prevents a tx state machine lockup
due to tx mbuf corruption.  The conditions under which the tx mbufs get
corrupted is complicated, but the net effect of this bit is that the
RDMA engine acts a little more conservatively.

> 2) Removing register definitions is something we really shouldn't do.
>    Just because you're not using the register any more in the driver,
>    doesn't mean you should remove it's definition from tg3.h
> 
>    What if some other developer wants to play with that register and
>    use it for some other purpose or experiment?

The problem is that register definitions can change from asic rev to
asic rev.  Someone interested in playing with their hardware would have
to know more about their chip before it could even be considered safe to
use these bits.  Rather than have a bit do something other than appears
advertised, it would be safer to just remove it from view.

The patch that removed the register definition exists precisely because
that register definition was changing.  The motivating reason for the
patch was to make the code cleaner and more maintainable, but anyone
attempting to use that definition on a non-5717 device would be using it
incorrectly.

> You really have to handle situations like #1 and #2 better especially
> since you guys do not public post the full PDF hardware programming
> manuals of your chips online for other developers to use.
> 
> I wouldn't, therefore, impose these rules on Intel and their drivers
> because they do public the programming manuals for their networking
> chips.

Understood.

Re: [PATCH net-next 00/11] tg3: Bugfixes and 5719 support

[David Miller]

From: "Matt Carlson" <mcarlson@broadcom.com>
Date: Sun, 6 Jun 2010 20:59:34 -0700

> On Sun, Jun 06, 2010 at 06:00:29PM -0700, David Miller wrote:
> The programming manual says this bit prevents a tx state machine lockup
> due to tx mbuf corruption.  The conditions under which the tx mbufs get
> corrupted is complicated, but the net effect of this bit is that the
> RDMA engine acts a little more conservatively.

That last phrase is a good description and could have been used to
better name the register bit macro in question.  You're basically
confirming that you named the register bit too tersely.

> The problem is that register definitions can change from asic rev to
> asic rev.

Frankly, this kind of justification really ticks me off.

How the heck does that make a difference?  Describe which chip revs
the register bit is valid for in a comment for crying out loud.

Document your hardware properly, not selectively or where you see
it fit to do so.

It's bad enough that you guys don't publish your hardware specs.
As a consequence as much knowledge as possible must go into the
driver sources.  Any piece of information you take away is bad.

There are tons of chip register bits in this driver already which are
only valid on certain hardware revs.  In fact, there are many.  Yet
they are all still there in tg3.h whether they are used by the driver
or not.  In fact, if I recall correctly, the DMA burst size controls
are pretty much different on every single rev of the chip.

Documenting where for which chips a register bit is valid is
pervasively done elsewhere, and is nothing new.  Your driver and your
hardware is not special.

Re: [PATCH net-next 00/11] tg3: Bugfixes and 5719 support

[Matt Carlson]

On Mon, Jun 07, 2010 at 12:28:05AM -0700, David Miller wrote:
> From: "Matt Carlson" <mcarlson@broadcom.com>
> Date: Sun, 6 Jun 2010 20:59:34 -0700
> 
> > On Sun, Jun 06, 2010 at 06:00:29PM -0700, David Miller wrote:
> > The programming manual says this bit prevents a tx state machine lockup
> > due to tx mbuf corruption.  The conditions under which the tx mbufs get
> > corrupted is complicated, but the net effect of this bit is that the
> > RDMA engine acts a little more conservatively.
> 
> That last phrase is a good description and could have been used to
> better name the register bit macro in question.  You're basically
> confirming that you named the register bit too tersely.
> 
> > The problem is that register definitions can change from asic rev to
> > asic rev.
> 
> Frankly, this kind of justification really ticks me off.
> 
> How the heck does that make a difference?  Describe which chip revs
> the register bit is valid for in a comment for crying out loud.
> 
> Document your hardware properly, not selectively or where you see
> it fit to do so.
> 
> It's bad enough that you guys don't publish your hardware specs.
> As a consequence as much knowledge as possible must go into the
> driver sources.  Any piece of information you take away is bad.
> 
> There are tons of chip register bits in this driver already which are
> only valid on certain hardware revs.  In fact, there are many.  Yet
> they are all still there in tg3.h whether they are used by the driver
> or not.  In fact, if I recall correctly, the DMA burst size controls
> are pretty much different on every single rev of the chip.
> 
> Documenting where for which chips a register bit is valid is
> pervasively done elsewhere, and is nothing new.  Your driver and your
> hardware is not special.

Maybe we don't have to take this path.

Our hardware specs are available.  You can find them at:

http://www.broadcom.com/support/ethernet_nic/open_source.php?source=top

The specs for the latest asic revs won't be there yet, but I'm sure
they'll get there.

In light of this, do you still feel we need to take this stance with
the tg3 driver?

Re: [PATCH net-next 00/11] tg3: Bugfixes and 5719 support

[David Miller]

From: "Matt Carlson" <mcarlson@broadcom.com>
Date: Mon, 7 Jun 2010 11:40:38 -0700

> Our hardware specs are available.  You can find them at:
> 
> http://www.broadcom.com/support/ethernet_nic/open_source.php?source=top
> 
> The specs for the latest asic revs won't be there yet, but I'm sure
> they'll get there.
> 
> In light of this, do you still feel we need to take this stance with
> the tg3 driver?

Nope, not with those documents available, that changes everything.

Thanks!