All of lore.kernel.org
 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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.