Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 18 Mar 2011 12:37:46 -0700
From:      Jack Vogel <jfvogel@gmail.com>
To:        Juli Mallett <jmallett@freebsd.org>
Cc:        Jack F Vogel <jfv@freebsd.org>, svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org
Subject:   Re: svn commit: r219753 - head/sys/dev/e1000
Message-ID:  <AANLkTi=3Zm69PW26tqqAHpT3%2BKwzCOga3hhOuU0wwheG@mail.gmail.com>
In-Reply-To: <AANLkTi=hx4mby-U3UM3ELcXEibM_gHp4F_GiG84qCTKA@mail.gmail.com>
References:  <201103181854.p2IIs0G1077313@svn.freebsd.org> <AANLkTi=hx4mby-U3UM3ELcXEibM_gHp4F_GiG84qCTKA@mail.gmail.com>

next in thread | previous in thread | raw e-mail | index | archive | help
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 <jmallett@freebsd.org> wrote:

> On Fri, Mar 18, 2011 at 11:54, Jack F Vogel <jfv@freebsd.org> 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.
>



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?AANLkTi=3Zm69PW26tqqAHpT3%2BKwzCOga3hhOuU0wwheG>