From owner-svn-src-all@FreeBSD.ORG Fri Mar 18 19:37:48 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 1A0EA106566B; Fri, 18 Mar 2011 19:37:48 +0000 (UTC) (envelope-from jfvogel@gmail.com) Received: from mail-vw0-f54.google.com (mail-vw0-f54.google.com [209.85.212.54]) by mx1.freebsd.org (Postfix) with ESMTP id 455388FC13; Fri, 18 Mar 2011 19:37:47 +0000 (UTC) Received: by vws18 with SMTP id 18so4465177vws.13 for ; Fri, 18 Mar 2011 12:37:46 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=FU3yx/eApMC3d5nmIERVw4Abtfx9aEDnCVmaUufEvo8=; b=KfrX29nSaUpJ1f6TvxKSBpHgdSB1JLIx8e1lVsyw+t1eAVxhBx9T4xAh/VncPmt0X5 euU35n/IFDuvQE6OUyg7GOqngunZ5b8EYRMjbZAHNr6fbqMbsOxTSFlyP/iQFugGs1EB d+6l4BFy5VAVj8P42aogSO6S4GpjmZ+vkxiic= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; b=niHPbtZkY5t/GYhIgZ9SUHq782ZHHzivsCXyrZe342yL1KKSMkpVLGXNpHF6C7PReT VX0EBsvOcUQuAGj/cZnDOaQ9CAl1XMZoPyy20tcpG3lJZMQQxvNgQ1QTLp5ozrEtKHRl dTueoRZ2uGScm1kBQTSZiyXdbaNTaeaTcsdnQ= MIME-Version: 1.0 Received: by 10.52.88.136 with SMTP id bg8mr2094499vdb.78.1300477066612; Fri, 18 Mar 2011 12:37:46 -0700 (PDT) Received: by 10.52.159.165 with HTTP; Fri, 18 Mar 2011 12:37:46 -0700 (PDT) In-Reply-To: References: <201103181854.p2IIs0G1077313@svn.freebsd.org> Date: Fri, 18 Mar 2011 12:37:46 -0700 Message-ID: From: Jack Vogel To: Juli Mallett Content-Type: text/plain; charset=ISO-8859-1 X-Content-Filtered-By: Mailman/MimeDel 2.1.5 Cc: Jack F Vogel , 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:37:48 -0000 The problem is that the shared code is delivered to me as a component. I generally do try to filter it a little, but if I start diverging from the internal component files, then I just introduce a never-ending task for myself to do that. It also is tested in house as a complete component, so every time I would change something that introduces risks. To both you and Yongari... its nice to compare how things are done with Linux, but what you need to understand is that there are, oh, about 20 engineers with varying tasks that handle Linux. For FreeBSD there is just me, so the only way I can accomplish as much is to creatively leverage the work of others, or to consolidate tasks in some ways. I know there are plenty of things to criticize, I apologize when I don't get it quite the way you'd like, or I make work for others, I'll try to take the feedback into account, I'm trying to do my best though. Regards, Jack On Fri, Mar 18, 2011 at 12:15 PM, Juli Mallett wrote: > 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: > > This delta updates the em driver to version 7.2.2 which has > > been undergoing test for some weeks. This improves the RX > > mbuf handling to avoid system hang due to depletion. Thanks > > to all those who have been testing the code, and to Beezar > > Liu 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. > > > - if (nvm->word_size == (1 << 15)) { > > + if (nvm->word_size == (1 << 15)) > > nvm->page_size = 128; > > - } > > - > > 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 0x00FF0000 /* DMA Coalescing > Receive > > +#define E1000_DMACR_DMACTHR_MASK 0x00FF0000 /* DMA Coalescing Rx > > * 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 = 0; > > +TUNABLE_INT("hw.em.eee_setting", &eee_setting); > > Would have been useful to see some mention of this in the commit message. > > > + struct e1000_hw *hw; > > int error = 0; > > > > INIT_DEBUGOUT("em_attach: begin"); > > > > adapter = device_get_softc(dev); > > adapter->dev = adapter->osdep.dev = dev; > > + hw = &adapter->hw; > > [...] > > > - if ((adapter->hw.mac.type == e1000_ich8lan) || > > - (adapter->hw.mac.type == e1000_ich9lan) || > > - (adapter->hw.mac.type == e1000_ich10lan) || > > - (adapter->hw.mac.type == e1000_pchlan) || > > - (adapter->hw.mac.type == e1000_pch2lan)) { > > + if ((hw->mac.type == e1000_ich8lan) || > > + (hw->mac.type == e1000_ich9lan) || > > + (hw->mac.type == e1000_ich10lan) || > > + (hw->mac.type == e1000_pchlan) || > > + (hw->mac.type == 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. > > > - bool more; > > > > > > if (ifp->if_drv_flags & IFF_DRV_RUNNING) { > > - more = em_rxeof(rxr, adapter->rx_process_limit, NULL); > > - > > + bool more = em_rxeof(rxr, adapter->rx_process_limit, > NULL); > > This seems like a style regression, at least by FreeBSD standards. > > > - for (int i = 0; i < j; ++i) { > > + for (int i = 0, n = 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. > > > rxr = &adapter->rx_rings[i]; > > - for (int n = 0; n < adapter->num_rx_desc; n++) { > > + n = rxr->next_to_check; > > ^^^ Here n is set. > > > + while(n != rxr->next_to_refresh) { > > ^^^ Here n is used in a while missing a very small amount of whitespace. > > > + while(i != rxr->next_to_refresh) { > > ^^^ Similar. > > > > - Copyright (c) 2001-2011, Intel Corporation > > + Copyright (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 = 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. >