iflib-enabled ixgbe driver update, including:
- Support for X550EM devices.
- Support for Bypass adapters.
- Flow Director code moved to separate files
- SR-IOV code moved to separate files
This is dependent on D5213
Details
Details
- Reviewers
jeffrey.e.pieper_intel.com scottl gnn sbruno adrian smh - Group Reviewers
Intel Networking
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
Diff Detail
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 8108 Build 8312: arc lint + arc unit
Event Timeline
Comment Actions
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)
Comment Actions
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 | |
Comment Actions
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.
Comment Actions
- 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.
Comment Actions
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?
Comment Actions
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.
Comment Actions
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
Comment Actions
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.