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)
Fri, Nov 15, 11:21 PM
Unknown Object (File)
Sat, Nov 9, 4:32 AM
Unknown Object (File)
Fri, Nov 8, 7:00 PM
Unknown Object (File)
Fri, Nov 8, 7:42 AM
Unknown Object (File)
Thu, Nov 7, 2:17 AM
Unknown Object (File)
Wed, Nov 6, 5:43 PM
Unknown Object (File)
Tue, Nov 5, 9:04 PM
Unknown Object (File)
Tue, Nov 5, 2:15 PM

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 8108
Build 8312: 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
169

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

174

unneeded braces once error style is fixed

422

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.

462

Style - cases should be not be indented.

462

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

545

boolean test on int return?

619

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

707

space

738

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

807

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

397

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

426–427

no need for spacing

590–601

Style needs wraping as it was.

890

Braced returns

1226

Style needs wraping as it was.

1564–1565

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

1564–1565

As above this should be only a single indent more.

1908–1909

Long line needs wrapping

3024

Long line needs wrapping

sys/dev/ixgbe/if_sriov.c
241

Left in debug

300

Unneeded blank line

700

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.