svn commit: r219753 - head/sys/dev/e1000

Juli Mallett jmallett at FreeBSD.org
Fri Mar 18 19:16:12 UTC 2011


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


More information about the svn-src-all mailing list