netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jeff Garzik <jeff@garzik.org>
To: Arjan van de Ven <arjan@linux.intel.com>
Cc: netdev@vger.kernel.org, "Kok, Auke" <auke-jan.h.kok@intel.com>,
	John Ronciak <john.ronciak@intel.com>,
	Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Subject: Re: [patch 03/14] e1000: omit stats for broken counter in 82543
Date: Fri, 15 Dec 2006 09:50:23 -0500	[thread overview]
Message-ID: <4582B62F.9020705@garzik.org> (raw)
In-Reply-To: <4582B21F.8090907@linux.intel.com>

Arjan van de Ven wrote:
> Jeff Garzik wrote:
>> Needs to use an "i have broken stats" feature flag, rather than adding 
>> yet another mac_type test into the code.  This testing of MAC type 
>> rather than feature flags is a major e1000 problem, and it bloats the 
>> driver quite a bit.  Intel has been told for /months/ this is a 
>> problem, yet I still see patches like this.
>>
> it is "nice" that you say this, and Intel is working on a "flags" based 
> driver. However that is, as you state yourself here, a major invasive 
> change, and thus not suitable for 2.6.20 inclusion. Yet these fixes are 
> important bugfixes; I don't think it's fair to hold these hostage..

Completely false.  I /never/ said it was a major invasive change.

The following is obviously /not/ an invasive change, but rather a simple 
incremental approach:

1) Define "unsigned long flags" in your adapter struct
(only has to be done once)

2) For the management patch (patch #3?), define a flag 
E1000_FLG_I_HAVE_MGMT.

3) Set this flag everywhere patch #3 does a mac_type test.  Appears to 
be 2-4 locations TOUCHED BY THE PATCH ANYWAY.

4) Watch the patch get applied.

Shall I do this for Intel, since this has been explained multiple times 
without success?  This is not an invasive approach, it only touches what 
code you were touching anyway.


A WORD OF WARNING:  I am also highly suspicious of impending driver 
rewrites.  Such massive events inevitably break 'git bisect'.  A far 
better approach is the Al Viro equivalent-transformation approach, which 
does not break 'git bisect' and can be easily verified by a human.

	Jeff




  reply	other threads:[~2006-12-15 14:50 UTC|newest]

Thread overview: 41+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2006-12-15  9:28 [patch 00/14] E1000 bugfix series for 2.6.20 Arjan van de Ven
2006-12-15  9:29 ` [patch 01/14] e1000: The user-supplied itr setting needs the lower 2 bits masked off Arjan van de Ven
2006-12-15  9:30 ` [patch 02/14] e1000: dynamic itr: take TSO and jumbo into account Arjan van de Ven
2006-12-15  9:31 ` [patch 03/14] e1000: omit stats for broken counter in 82543 Arjan van de Ven
2006-12-15 14:30   ` Jeff Garzik
2006-12-15 14:33     ` Arjan van de Ven
2006-12-15 14:50       ` Jeff Garzik [this message]
2006-12-15 15:37   ` Jeff Garzik
2006-12-15  9:32 ` [patch 04/14] e1000: consolidate managability enabling/disabling Arjan van de Ven
2006-12-15 14:32   ` Jeff Garzik
2006-12-15 15:57   ` Jeff Garzik
2006-12-15  9:33 ` [patch 05/14] e1000: Fix Wake-on-Lan with forced gigabit speed Arjan van de Ven
2006-12-15 14:33   ` Jeff Garzik
2006-12-15 16:00   ` Jeff Garzik
2006-12-15  9:34 ` [patch 06/14] e1000: disable TSO on the 82544 with slab debugging Arjan van de Ven
2006-12-15 14:33   ` Jeff Garzik
2006-12-16  1:04     ` Herbert Xu
2006-12-16 20:18       ` Jesse Brandeburg
2006-12-26 21:28       ` Jeff Garzik
2006-12-15  9:35 ` [patch 07/14] e1000: workaround for the ESB2 NIC RX unit issue Arjan van de Ven
2006-12-15 14:33   ` Jeff Garzik
2006-12-15  9:36 ` [patch 08/14] e1000: fix to set the new max frame size before resetting the adapter Arjan van de Ven
2006-12-15  9:37 ` [patch 09/14] e1000: fix ethtool reported bus type for older adapters Arjan van de Ven
2006-12-15 14:34   ` Jeff Garzik
2006-12-15  9:38 ` [patch 10/14] e1000: narrow down the scope of the tipg timer tweak Arjan van de Ven
2006-12-15 14:34   ` Jeff Garzik
2006-12-15  9:39 ` [patch 11/14] e1000: Fix PBA allocation calculations Arjan van de Ven
2006-12-15  9:40 ` [patch 12/14] e1000: Make the copybreak value a module parameter Arjan van de Ven
2006-12-15 14:35   ` Jeff Garzik
2006-12-15 16:09   ` Jeff Garzik
2006-12-15  9:41 ` [patch 13/14] e1000: 3 new driver stats for managability testing Arjan van de Ven
2006-12-15 14:35   ` Jeff Garzik
2006-12-15 16:17   ` Jeff Garzik
2006-12-15  9:42 ` [patch 14/14] e1000: No-delay link detection at interface up Arjan van de Ven
2006-12-15 14:36   ` Jeff Garzik
2006-12-15 14:37     ` Arjan van de Ven
2006-12-15 14:50       ` Jeff Garzik
2006-12-15 14:02 ` [patch 00/14] E1000 bugfix series for 2.6.20 Jeff Garzik
2006-12-15 14:04   ` Arjan van de Ven
2006-12-15 14:07     ` Jeff Garzik
2006-12-15 14:08       ` Arjan van de Ven

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4582B62F.9020705@garzik.org \
    --to=jeff@garzik.org \
    --cc=arjan@linux.intel.com \
    --cc=auke-jan.h.kok@intel.com \
    --cc=jeffrey.t.kirsher@intel.com \
    --cc=john.ronciak@intel.com \
    --cc=netdev@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).