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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20100607035934.GB11599@mcarlson.broadcom.com>
Date:	Sun, 6 Jun 2010 20:59:34 -0700
From:	"Matt Carlson" <mcarlson@...adcom.com>
To:	"David Miller" <davem@...emloft.net>
cc:	"Matthew Carlson" <mcarlson@...adcom.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"andy@...yhouse.net" <andy@...yhouse.net>
Subject: Re: [PATCH net-next 00/11] tg3: Bugfixes and 5719 support

On Sun, Jun 06, 2010 at 06:00:29PM -0700, David Miller wrote:
> From: "Matt Carlson" <mcarlson@...adcom.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.

--
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