From owner-svn-src-all@FreeBSD.ORG Fri Mar 18 19:16:12 2011 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 086CB106566C; Fri, 18 Mar 2011 19:16:12 +0000 (UTC) (envelope-from juli@clockworksquid.com) Received: from mail-yx0-f182.google.com (mail-yx0-f182.google.com [209.85.213.182]) by mx1.freebsd.org (Postfix) with ESMTP id 8B1D18FC18; Fri, 18 Mar 2011 19:16:11 +0000 (UTC) Received: by yxl31 with SMTP id 31so1951685yxl.13 for ; Fri, 18 Mar 2011 12:16:10 -0700 (PDT) Received: by 10.151.117.5 with SMTP id u5mr1580167ybm.135.1300475770089; Fri, 18 Mar 2011 12:16:10 -0700 (PDT) MIME-Version: 1.0 Sender: juli@clockworksquid.com Received: by 10.150.197.12 with HTTP; Fri, 18 Mar 2011 12:15:50 -0700 (PDT) In-Reply-To: <201103181854.p2IIs0G1077313@svn.freebsd.org> References: <201103181854.p2IIs0G1077313@svn.freebsd.org> From: Juli Mallett Date: Fri, 18 Mar 2011 12:15:50 -0700 X-Google-Sender-Auth: a6BPr7jzak-RDO_Hvgk6E5tbKPc Message-ID: To: Jack F Vogel Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r219753 - head/sys/dev/e1000 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 18 Mar 2011 19:16:12 -0000 On Fri, Mar 18, 2011 at 11:54, Jack F Vogel wrote: > Author: jfv > Date: Fri Mar 18 18:54:00 2011 > New Revision: 219753 > URL: http://svn.freebsd.org/changeset/base/219753 > > Log: > =A0This delta updates the em driver to version 7.2.2 which has > =A0been undergoing test for some weeks. This improves the RX > =A0mbuf handling to avoid system hang due to depletion. Thanks > =A0to all those who have been testing the code, and to Beezar > =A0Liu for the design changes. I understand that the fundamental unit coming out of Intel is an atomic driver version, but it really, really, really would be nice to not fix functional and stylistic changes at such a fundamental level and with such a widely-used driver as these. I understand that you have enough work already, but if it's at all possible to do these updates in a couple of passes, I would really appreciate it, and I know there are other people who have to mine the significant deltas from these updates who would appreciate it, too. You could start with stylistic changes and then do a couple of functional changes and then update the driver version to reflect the Intel finished product, say. It's good that it's been undergoing a long period of test, but it would have been nice if the commit message had included more details on the functional changes. Some specific comments: It seems like the update to e1000_82575 (and other chip-specific bits) is totally independent of the other style changes and the depletion fix and that it would have been easy to do that separately. > - =A0 =A0 =A0 if (nvm->word_size =3D=3D (1 << 15)) { > + =A0 =A0 =A0 if (nvm->word_size =3D=3D (1 << 15)) > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0nvm->page_size =3D 128; > - =A0 =A0 =A0 } > - Great that this is moving towards KNF. Might be good to take a few minutes and fix all such statements now rather than doing them in pieces later in time. > -#define E1000_DMACR_DMACTHR_MASK =A0 =A0 =A0 =A00x00FF0000 /* DMA Coales= cing Receive > +#define E1000_DMACR_DMACTHR_MASK =A0 =A0 =A0 =A00x00FF0000 /* DMA Coales= cing Rx > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 * Threshold */ All of these changes could have easily been done separately since they are entirely stylistic. > +/* Energy efficient ethernet - default to OFF */ > +static int eee_setting =3D 0; > +TUNABLE_INT("hw.em.eee_setting", &eee_setting); Would have been useful to see some mention of this in the commit message. > + =A0 =A0 =A0 struct e1000_hw *hw; > =A0 =A0 =A0 =A0int =A0 =A0 =A0 =A0 =A0 =A0 error =3D 0; > > =A0 =A0 =A0 =A0INIT_DEBUGOUT("em_attach: begin"); > > =A0 =A0 =A0 =A0adapter =3D device_get_softc(dev); > =A0 =A0 =A0 =A0adapter->dev =3D adapter->osdep.dev =3D dev; > + =A0 =A0 =A0 hw =3D &adapter->hw; [...] > - =A0 =A0 =A0 if ((adapter->hw.mac.type =3D=3D e1000_ich8lan) || > - =A0 =A0 =A0 =A0 =A0 (adapter->hw.mac.type =3D=3D e1000_ich9lan) || > - =A0 =A0 =A0 =A0 =A0 (adapter->hw.mac.type =3D=3D e1000_ich10lan) || > - =A0 =A0 =A0 =A0 =A0 (adapter->hw.mac.type =3D=3D e1000_pchlan) || > - =A0 =A0 =A0 =A0 =A0 (adapter->hw.mac.type =3D=3D e1000_pch2lan)) { > + =A0 =A0 =A0 if ((hw->mac.type =3D=3D e1000_ich8lan) || > + =A0 =A0 =A0 =A0 =A0 (hw->mac.type =3D=3D e1000_ich9lan) || > + =A0 =A0 =A0 =A0 =A0 (hw->mac.type =3D=3D e1000_ich10lan) || > + =A0 =A0 =A0 =A0 =A0 (hw->mac.type =3D=3D e1000_pchlan) || > + =A0 =A0 =A0 =A0 =A0 (hw->mac.type =3D=3D e1000_pch2lan)) { Like the brace changes elsewhere, this is not the first time this stylistic improvement has been made in the e1000 code base. Perhaps it would be good to replace all uses of adapter->hw. with hw-> and add the appropriate hw variables now. It seems insignificant, but I assure you it is a source of conflicts for those of us who have to maintain local changes to e1000. That's alright as a one time cost, and it would seem worthwhile to fix this universally since that's the direction this driver and the Linux driver seem to be going in. > - =A0 =A0 =A0 bool =A0 =A0 =A0 =A0 =A0 =A0more; > > > =A0 =A0 =A0 =A0if (ifp->if_drv_flags & IFF_DRV_RUNNING) { > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 more =3D em_rxeof(rxr, adapter->rx_process_= limit, NULL); > - > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 bool more =3D em_rxeof(rxr, adapter->rx_pro= cess_limit, NULL); This seems like a style regression, at least by FreeBSD standards. > - =A0 =A0 =A0 for (int i =3D 0; i < j; ++i) { > + =A0 =A0 =A0 for (int i =3D 0, n =3D 0; i < q; ++i) { Why is n being initialized to 0 when it will be set immediately below? Even if it were necessary, putting it in the for statement is only an obfuscatory eye-sore, doubly so since it has nothing to do with the loop construct as far as I can tell. > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0rxr =3D &adapter->rx_rings[i]; > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 for (int n =3D 0; n < adapter->num_rx_desc;= n++) { > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 n =3D rxr->next_to_check; ^^^ Here n is set. > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 while(n !=3D rxr->next_to_refresh) { ^^^ Here n is used in a while missing a very small amount of whitespace. > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 while(i !=3D rxr->next_to_refresh) { ^^^ Similar. > - =A0Copyright (c) 2001-2011, Intel Corporation > + =A0Copyright (c) 2001-2010, Intel Corporation This seems like a step backwards. Through time. > +/* > +** DMA Coalescing, only for i350 - default to off, > +** this feature is for power savings > +*/ > +static int igb_dma_coalesce =3D FALSE; > +TUNABLE_INT("hw.igb.dma_coalesce", &igb_dma_coalesce); Again, would've loved some mention of this in the commit message! > *** DIFF OUTPUT TRUNCATED AT 1000 LINES *** Another compelling reason to split up stylistic and functional changes. Thanks, Juli.