Page MenuHomeFreeBSD

Intel I225 Driver
AcceptedPublic

Authored by grehan on Mon, Jun 7, 8:40 AM.

Details

Summary

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.

Test Plan

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

Repository
rS FreeBSD src repository - subversion
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 39940
Build 36829: arc lint + arc unit

Event Timeline

grehan requested review of this revision.Mon, Jun 7, 8:41 AM
bcr added a subscriber: bcr.

Manpage looks good. Thanks for yet another driver!

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.

erj added inline comments.
sys/amd64/conf/GENERIC
251

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
639

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
639

Eh, just ignore this. It's fine.

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
251

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).

grehan marked 2 inline comments as done.
  • Fix up copyrights based on review feedback.
  • Consistent capitalization of I225
danfe added inline comments.
sys/amd64/conf/GENERIC
251

Hm, I've though "capitalization for consistency" refers to Ethernet, which should start with capital letter (also below).

sys/amd64/conf/NOTES
292

Like here.

315

And here.

sys/i386/conf/GENERIC
223

And here.

sys/i386/conf/NOTES
500

Ditto.

grehan added inline comments.
sys/amd64/conf/GENERIC
251

All locations updated.

grehan marked an inline comment as done.
  • "ethernet" -> "Ethernet"

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?

This revision is now accepted and ready to land.Tue, Jun 15, 3:13 PM
afedorov added inline comments.
sys/dev/igc/if_igc.c
1010

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

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
1010

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
1010

This code was directly derived from igb.

Well, no offense, but this does not quite answer Alexander's question. :-)

If 256375 results in it being removed from igb, then it can be removed from here as well.

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?

scottl added inline comments.
sys/dev/igc/if_igc.c
1010

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
1010

"yes I'm being rude and offensive, but please pretend that I'm a nice person and allow me to abuse you".

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.

I think you're arguing for the sake of arguing

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.

and pretending to be an expert in an area that you have minimal experience in.

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
2

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.)

grehan added inline comments.
share/man/man4/igc.4
2

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.

grehan marked an inline comment as done.
  • Use correct SPDX identifier and move to after copyrights.
This revision now requires review to proceed.Wed, Jun 16, 10:39 PM
kbowling added inline comments.
sys/dev/igc/if_igc.c
1010

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.

This revision is now accepted and ready to land.Thu, Jun 17, 12:33 AM