Page MenuHomeFreeBSD

ixgbe(4): Update HEAD to 3.2.12-k
AbandonedPublic

Authored by cramerj_intel.com on Mar 2 2017, 1:33 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Apr 14, 2:18 PM
Unknown Object (File)
Tue, Apr 9, 6:27 PM
Unknown Object (File)
Tue, Apr 9, 7:39 AM
Unknown Object (File)
Fri, Apr 5, 4:12 AM
Unknown Object (File)
Wed, Apr 3, 1:02 AM
Unknown Object (File)
Sun, Mar 31, 4:41 AM
Unknown Object (File)
Sat, Mar 30, 3:53 AM
Unknown Object (File)
Sat, Mar 30, 3:37 AM

Details

Summary

iflib-enabled ixgbe driver update, including:

  1. Support for X550EM devices.
  2. Support for Bypass adapters.
  3. Flow Director code moved to separate files
  4. SR-IOV code moved to separate files

This is dependent on D5213

Test Plan

Compile-tested with minimal touch-testing for the PF driver (none for VF). Respectfully request waiting for Jeff's team to perform a validation pass before committing.

Diff Detail

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 7831
Build 7975: arc lint + arc unit

Event Timeline

cramerj_intel.com retitled this revision from to ixgbe(4): Update HEAD to 3.2.12-k.
cramerj_intel.com updated this object.
cramerj_intel.com edited the test plan for this revision. (Show Details)
cramerj_intel.com set the repository for this revision to rS FreeBSD src repository - subversion.

arc patch fails:

root@s254:/diskless/os/FreeBSD/12.0-CURRENT_2017-02-10_D9851/usr/src # arc patch D9851
Created and checked out branch arcpatch-D9851.
Checking patch head/sys/net/iflib.c...
error: head/sys/net/iflib.c: does not exist in index

Patch Failed!
Usage Exception: Unable to apply patch!

This is against a fresh clone of HEAD:

root@s254:/diskless/os/FreeBSD/12.0-CURRENT_2017-02-10_D9851/usr/src # git log -4
commit c86cbe211d7604a8cb7ff5133adb4d705f3a6de9
Author: emaste <emaste@FreeBSD.org>
Date: Thu Mar 2 02:10:59 2017 +0000

regen src.conf.5 after r313169

Sponsored by:   The FreeBSD Foundation

commit df165f3ffff714308d11ddf1ad4cc0d5aa71ba87
Author: scottl <scottl@FreeBSD.org>
Date: Thu Mar 2 01:39:23 2017 +0000

Expose the sbuf_putbuf() symbol to libsbuf.  There are a few other symbols
that are present but not exposed, like get/set/clear flags, not sure if they
need to be exposed at this point.

Sponsored by:   Netflix

commit 76410f366f4751ed303c8da602d9ffc098dffa9f
Author: jasone <jasone@FreeBSD.org>
Date: Thu Mar 2 01:14:48 2017 +0000

Update jemalloc to 4.5.0.

commit 627a54806e13aef53015685e2860546f9b9ab650
Author: des <des@FreeBSD.org>
Date: Thu Mar 2 00:27:21 2017 +0000

Update to reflect that SHA-1 has now been broken.

Submitted by:   ak
MFC after:      1 week

arc patch --skip-dependencies has multiple failures (see attached log)

This revision now requires changes to proceed.Mar 2 2017, 3:43 AM

This does apply cleanly with D5213 as a dependency.

This revision is now accepted and ready to land.Mar 6 2017, 11:15 PM
smh requested changes to this revision.Mar 7 2017, 5:15 PM
smh edited edge metadata.

Massive amounts of changes in this so impossible to see if everything is good, however a number of style related bits highlighted, some of which are regressions.

sys/dev/ixgbe/if_bypass.c
168

error is no a boolean so this and others should really be if (error != 0)

173

unneeded braces once error style is fixed

421

This seems to always be called after ixgbe_bypass_mutex_clear, so gives the impression its a requirement of the function so might be better to incorporate it in there instead of having all the duplicate code.

461

Style - cases should be not be indented.

461

Looks like there's lots of duplication here with the only real variable being the initial value used for arg.

544

boolean test on int return?

618

its not instantly clear so why your using -EINVAL so possibly worth commenting, more below

706

space

737

I believe the more common style is the | on the preceding line

806

No need for return at the end of a void function

sys/dev/ixgbe/if_ix.c
312

Add a space after the comma

318

Add a space after the comma

391

Its picky yes but do we really need to wrap this early, makes it more difficult to read.

583

no need for spacing

1007

Style needs wraping as it was.

1301

Braced returns

1937

Style needs wraping as it was.

2555

As the style is being corrected anyway this should be only a single indent more

2673

As above this should be only a single indent more.

3568

Long line needs wrapping

4053

Long line needs wrapping

sys/dev/ixgbe/if_sriov.c
240

Left in debug

299

Unneeded blank line

699

braced returns more below too

sys/dev/ixgbe/ixgbe_api.c
1705

!= NULL

sys/dev/ixgbe/ixgbe_common.c
1194

braced returns, more below

This revision now requires changes to proceed.Mar 7 2017, 5:15 PM
In D9851#204961, @smh wrote:

Massive amounts of changes in this so impossible to see if everything is good, however a number of style related bits highlighted, some of which are regressions.

In regard to the long lines not being wrapped, I think Jeb is adhering to the linux kernel coding convention of not wrapping user-visible strings in order to make searching for them easier. I think it's a reasonable convention, so I don't object to it.

Thanks for the feedback. I'll look into these suggestions.

cramerj_intel.com edited edge metadata.
  • Fixed compilation issue when PCIOV is not defined.
  • Fixed issue with setting advertise speed correctly for DNV hardware.
  • Applied some changes based off of smh's style suggestions.

I would like to see this integrated soon, @sbruno has gotten iflib.c leveled up to a good spot and we are running it in production for e1000. @jeffrey.e.pieper_intel.com are you ok with @cramerj_intel.com latest fixes?

In D9851#209276, @kevin.bowling_kev009.com wrote:

I would like to see this integrated soon, @sbruno has gotten iflib.c leveled up to a good spot and we are running it in production for e1000. @jeffrey.e.pieper_intel.com are you ok with @cramerj_intel.com latest fixes?

No, there are several outstanding issues that Jeb is still working on. I believe he will be sending out an email with details shortly. There are also changes needed to D5213.

This revision now requires changes to proceed.Mar 24 2017, 1:19 AM

I think this review supersedes D5213, if there are other diffs they should be added here. Matt approved the change proposed by @krzysztof.galazka_intel.com https://github.com/mattmacy/networking/pull/5

I believe this review is dependent on D5213...at least that is how it was written. It certain doesn't apply without it. This review adds support for Denverton as well as separating the VF driver from the base driver.