Driver for the Intel I225 2.5G ethernet controller.
This driver is based on code originally written by Intel and
converted to use the iflib interface by Netgate.
SPDX abbreviated copyrights are used, making this review dependent
on D29226.
Details
Details
- Reviewers
kbowling scottl loos erj - Group Reviewers
Intel Networking manpages - Commits
- rG9ffb4c0adab4: igc(4): Introduce new driver for the Intel I225 Ethernet controller.
rGd7388d33b4dd: MFC 517904de5cca: igc(4): Introduce new driver for the Intel I225 Ethernet…
rG517904de5cca: igc(4): Introduce new driver for the Intel I225 Ethernet controller.
This has been in testing on Netgate h/w for a few months now, with
various combinations of tx/rx checksum, IPv4/IPv6, TSO, LRO, VLANs etc.
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 39758 Build 36647: arc lint + arc unit
Event Timeline
Comment Actions
I have a draft standard on licenses.
The SPDX-License-Identifier lines all look good from that draft perspective. All the licenses are in the acceptable list.
In new code, I've typically seen the copyright lines before rather than after the SPDX-License-Identifier line, if you care about consistency. The tools work both ways, but my draft currently recommends putting the copyright lines before.
Even absent my new draft, there's already a few files in the tree with this construct, so these should be fine going in prior to it being finalized.
| sys/dev/igc/if_igc.h | ||
|---|---|---|
| 6 | You likely don't want all rights reserved here. If you are modifying Nicole's code, then place this line at the end of the 2016 copyright line. | |
| sys/amd64/conf/GENERIC | ||
|---|---|---|
| 252 | You should capitalize this for consistency. | |
| sys/dev/igc/igc_i225.c | ||
| 8 | Why does this file have a different method of including $FreeBSD$? | |
| sys/modules/Makefile | ||
| 640 | It might not be a great idea to have this compile on 32-bit systems because we basically stopped using 32-bit OSes internally; so it's possible if you're taking updates from Intel somewhere th | |
| sys/modules/Makefile | ||
|---|---|---|
| 640 | Eh, just ignore this. It's fine. | |
Comment Actions
Even absent my new draft, there's already a few files in the tree with this construct, so these should be fine going in prior to it being finalized.
Fixed.
| sys/amd64/conf/GENERIC | ||
|---|---|---|
| 252 | Done, and updated all NOTES/GENERIC confs. | |
| sys/dev/igc/if_igc.h | ||
| 6 | Fixed. | |
| sys/dev/igc/igc_i225.c | ||
| 8 | Not sure what you mean here: all C files have this; headers just the $FreeBSD$ as per style(9). | |
| sys/amd64/conf/GENERIC | ||
|---|---|---|
| 252 | All locations updated. | |
Comment Actions
I don't see any glaring problems with it that should block it from going upstream. Are you going to announce this addition on the mailing lists?
| sys/dev/igc/if_igc.c | ||
|---|---|---|
| 1011 | Can someone explain why this call is needed? Please see the discussion in PR: 256375 For example, ixgbe does not call iflib_admin_intr_deferred (ctx): https://github.com/freebsd/freebsd-src/blob/main/sys/dev/ixgbe/if_ix.c#L2199 | |
Comment Actions
I don't see any glaring problems with it that should block it from going upstream. Are you going to announce this addition on the mailing lists?
I just had a patch submitted by Make Karels so will pull that into the diff.
Once that is Ok's - yes, will announce.
| sys/dev/igc/if_igc.c | ||
|---|---|---|
| 1011 | This code was directly derived from igb. If 256375 results in it being removed from igb, then it can be removed from here as well. | |
| sys/dev/igc/if_igc.c | ||
|---|---|---|
| 1011 |
Well, no offense, but this does not quite answer Alexander's question. :-)
Given this chance (new driver), shouldn't we try to understand the story behind this line now, during the due review process, rather than keep adding copies of dubious code to the tree in a faint and naïve hope that one day it would all get fixed? | |
| sys/dev/igc/if_igc.c | ||
|---|---|---|
| 1011 | In colloquial english, "...no offense, but..." means, "yes I'm being rude and offensive, but please pretend that I'm a nice person and allow me to abuse you". I think you're arguing for the sake of arguing, and pretending to be an expert in an area that you have minimal experience in. | |
| sys/dev/igc/if_igc.c | ||
|---|---|---|
| 1011 |
I've spend about ten minutes thinking of how to phrase "okay, but does someone actually know and can explain why this call is needed?" nicely and even put a smiley face to indicate the friendliness of my question. It is you, Scott, who took offense when it was not implied or indented, insulted me, and derailed the discussion away from technical grounds. Now that is rude and abusive.
Your thinking is wrong. If you would've been less inconsiderate, you would have noticed that PR 256375 was filed by me as this bug affects my daily FreeBSD usage, so my interest does not come out of blue.
I'm only pretending that I didn't notice this disgusting personal attack, and won't report on your misconduct. | |
| share/man/man4/igc.4 | ||
|---|---|---|
| 3 | SPDX does not have a BSD-3-Clause-FreeBSD listed. BSD-2-Clause OK? | |
| sys/dev/igc/igc_i225.c | ||
| 8 | I suspect the question was why __FSDID("$FreeBSD"); is used in some places vs $FreeBSD$. In any case we should really remove $FreeBSD$ from style(9) in fact; it still has some value in stable branches built from svn but likely makes sense to omit it from new files. (It doesn't really matter if this change is committed with or without it - i.e., discussion of updating style(9) shouldn't have any impact on getting this committed.) | |
| share/man/man4/igc.4 | ||
|---|---|---|
| 3 | Done. Will stick to BSD-3-Clause since the man page was derived from the em/ixgbe man pages covered by a 3-clause copyright. | |
| sys/dev/igc/if_igc.c | ||
|---|---|---|
| 1011 | A number of things are being conflated here, I agree this shouldn't block igc(4). To answer the question it may be related to handling !msi-x cases as a quick code inspection. ixgbe is rarely run w/o msi-x, while a lot of e1000 parts only are. The microcode is also a lot more primitive on e1000, there is a full second delay in checking the phy in the e1000 API which is probably why you are looking at this. @danfe @afedorov here's how you can progress this: remove the line, test with msi-x and without msi-x (loader tunable hw.em.msix=0). Use your original PR to post results and findings. If progress is made it will be absorbed into igc(4) before or after the merge depending on timing. | |