Details

Reviewers

erj
kbowling
gowtham.kumar.ks_intel.com

Group Reviewers

Restricted Owners Package

(Owns No Changed Paths)

Commits

rGa728b96686e6: ix/ixv: Add support for new Intel Ethernet E610 family devices
rGdea5f973d0c8: ix/ixv: Add support for new Intel Ethernet E610 family devices

Summary

This is part 1 of the support for the new Intel Ethernet E610 family of devices.

Introduce new PCI device IDs:
• 57AE: Intel(R) E610 (Backplane)
• 57AF: Intel(R) E610 (SFP)
• 57B0: Intel(R) E610 (10 GbE)
• 57B1: Intel(R) E610 (2.5 GbE)
• 57B2: Intel(R) E610 (SGMII)

Key updates for E610 family:
• Firmware manages Link and PHY
• Implement new CSR-based Admin Command Interface (ACI) for SW-FW interaction
• Tested exclusively for x64 operating systems on E610-XT2/XT4 (10G) and E610-IT4 (2.5G)
• Enable link speeds above 1G: 2.5G, 5G and 10G
• NVM Recovery Mode and Rollback support

Signed-off-by: Yogesh Bhosale yogesh.bhosale@intel.com
Co-developed-by: Krzysztof Galazka krzysztof.galazka@intel.com

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

Yogesh.Bhosale_intel.com created this revision.Apr 29 2025, 10:44 AM

Owners added 1 blocking reviewer(s): Restricted Owners Package.Apr 29 2025, 10:44 AM

Herald added subscribers: ae, imp. · View Herald TranscriptApr 29 2025, 10:44 AM

Yogesh.Bhosale_intel.com requested review of this revision.Apr 29 2025, 10:44 AM

kgalazka mentioned this in D50128: ixgbe: add MDIO bus support.May 6 2025, 8:21 PM

Yogesh.Bhosale_intel.com updated this revision to Diff 155128.May 8 2025, 9:27 AM

@erj, @kbowling: We're still awaiting for feedback from validation team, but would you mind taking a look?

In the description:

Support exclusively for x64 operating systems; no 32-bit OS support

Doesn't the in-kernel ixgbe driver still build for i386? Does this just mean no "official" support when it's running on non amd64 kernels?

Enable link speeds above 1G: 2.5G, 5G and 10G

These are already enabled -- do you mean just beyond the initial 1G E610 part? It's a little confusing to add this when support for those link speeds already exists.

Otherwise,
I think it mostly looks good -- but what's going to be included in part 2?

sys/dev/ixgbe/if_ix.c
1589	This should say "Link", right?
3054	It feels like you could combine this call with the one below for ixgbe_shutdown_aci() to reduce the number of if-statements here; `mta` isn't needed in these E610-only cleanup calls?
sys/dev/ixgbe/ixgbe_type_e610.h
805	Any 25G Base-T parts on the horizon for Linkville? :p Or, are those only coming in maybe a future follow-on...

To add to what @erj said, multi-arch build is a requirement the same as on other OS projects. We have to provide some advance notice to drop things and this should be MFC'd back to 13 and 14. See build(7), 'make universe' or 'make kernels' is enough. I don't see anything off hand that is problematic, and there is no expectation on Intel for validation for odd platforms. But it has to build everywhere it currently does.

In D50067#1149007, @erj wrote:

In the description:

Support exclusively for x64 operating systems; no 32-bit OS support

Doesn't the in-kernel ixgbe driver still build for i386? Does this just mean no "official" support when it's running on non amd64 kernels?

It does build on i386 and other architectures. The intention was only to make it clear that it was tested only on x86. I should have notice that the choice of words is not very fortunate. We'll fix that.

To be sure I run make kernels as suggested by Kevin and there was only one error on arm.TEGRA124, but unrelated to ixgbe:

src/sys/dev/drm2/ttm/ttm_bo_vm.c:238:2: error: call to undeclared function 'pctrie_iter_reset'; ISO C99 and later do not support implicit function declarations [-Werror,-Wimplicit-function-declaration]
  238 |         pctrie_iter_reset(&pages);

Enable link speeds above 1G: 2.5G, 5G and 10G

These are already enabled -- do you mean just beyond the initial 1G E610 part? It's a little confusing to add this when support for those link speeds already exists.

Yeah, that also needs better wording.

Otherwise,
I think it mostly looks good -- but what's going to be included in part 2?

In part 2 we will add support for debug dump and in part 3 FW logging. I thought it may be easier to review that way.

Please reupload with context (git diff -U9999). It also seems to not be based on FreeBSD main.

sys/dev/ixgbe/if_ix.c
1414	I'm a little confused by this, is this diff against git main? @erj already pointed out we have support for this.
2441	this seems unnecessary if you rebase on main
sys/dev/ixgbe/ixgbe.h
200	this should not be necessary
sys/dev/ixgbe/ixgbe_vf.c
659	I wonder if this should all just be folded into the top level case, and the mac type and NON_STD chacks dropped.. isn't link speed just informational for a VF?

This revision now requires changes to proceed.May 16 2025, 8:25 PM

kbowling added inline comments.May 16 2025, 8:54 PM

sys/dev/ixgbe/ixgbe_osdep.h
203	is the aci accessed by multiple threads? the iflib admin task, media status, init, etc is protected internally by the CTX_LOCK I wonder if you might be able to change ixgbe_acquire_lock to #ifdef INVARIANTS sx_assert(iflib_ctx_lock_get(((struct e1000_osdep *)hw->back)->ctx), SX_XLOCKED) #endif and make the other lock functions no-op and see what a WITNESS kernel says.
240	It seems this is main usage of this beyond the small HW buffer limits is to get the shadow ROM in a contiguous buffer which the checksum is then computed incrementally in word size chunks anyways. It could go away with a small refactoring to just move the 4k reads to the right place?
246	it seems like this is used for 4k IXGBE_ACI_MAX_BUFFER_SIZE operations, is there any reason not to do that on the stack?

kbowling added inline comments.May 17 2025, 12:18 AM

sys/dev/ixgbe/ixgbe_osdep.h
246	Otherwise since the use is all serialized, malloc one or more buffers you need on init and access them as needed. I don't think it makes sense to expose the runtime path to malloc and the associated error handling.

Combining if's as suggested by Eric and fixing a kernel panic issue during unload

Harbormaster completed remote builds in B64435: Diff 156040.May 26 2025, 9:39 AM

Combining if's as suggested by Eric and fixing a kernel panic issue during unload

Also fixing a broken update of the patch

Harbormaster completed remote builds in B64437: Diff 156044.May 26 2025, 10:28 AM

Yogesh.Bhosale_intel.com edited the summary of this revision. (Show Details)May 27 2025, 8:14 AM

Yogesh.Bhosale_intel.com marked 3 inline comments as done.May 27 2025, 8:28 AM

Yogesh.Bhosale_intel.com added inline comments.

sys/dev/ixgbe/if_ix.c
1414	This is intentional. The X550 relies on mac type for speed detection, while the E610 driver can determine speeds using layer flags.
1589	I will fix that.
3054	Done.
sys/dev/ixgbe/ixgbe_osdep.h
203	We plan to add debug dump and firmware logging features that will access this interface using ioctl and sysctl, which are not protected by the iflib context lock. I will submit a patch for the debug dump shortly to clarify the use case and simplify this review.

Fixing a typo

Harbormaster completed remote builds in B64458: Diff 156080.May 27 2025, 8:30 AM

Generally ok with this. I would like to know if the malloc thing can be addressed during init or if that is required by your upcoming changes.

kgalazka added inline comments.May 30 2025, 8:56 PM

sys/dev/ixgbe/if_ix.c
2441	I'm not sure what are you referring to. It was rebased on main. I don't see anything missing here: https://cgit.freebsd.org/src/tree/sys/dev/ixgbe/if_ix.c#n2373.
sys/dev/ixgbe/ixgbe.h
200	As above - I'm not sure what do you mean. I don't see anything like that on main.
sys/dev/ixgbe/ixgbe_osdep.h
246	it seems like this is used for 4k IXGBE_ACI_MAX_BUFFER_SIZE operations, is there any reason not to do that on the stack? I think it was based on ICE code, where those buffers are used for DMA. Anyway it is used by the part of the code shared between OSes and some of them have more explicit requirements regarding stack usage. Otherwise since the use is all serialized, malloc one or more buffers you need on init and access them as needed. Both debug dump and FW logging use ACI interface, and only ixgbe_aci_send_cmd_execute call is serialized with ACI lock. One buffer won't be enough. With multiple buffers we would have to track which are in use. It's not a trivial change I guess. I think it would be useful if we can stay in sync with the out-of-tree version for a while to leverage work of our teammates working on other OSes.

Fix ixv driver loading issue

Harbormaster completed remote builds in B64791: Diff 156873.Jun 12 2025, 9:58 AM

Yogesh.Bhosale_intel.com edited the summary of this revision. (Show Details)Jun 13 2025, 6:51 AM

Yogesh.Bhosale_intel.com retitled this revision from Add support for new Intel Ethernet E610 family devices - Part 1 to ix/ixv: Add support for new Intel Ethernet E610 family devices.Jun 19 2025, 9:43 AM

Yogesh.Bhosale_intel.com edited the summary of this revision. (Show Details)

Yogesh.Bhosale_intel.com added inline comments.Jun 19 2025, 9:47 AM

sys/dev/ixgbe/ixgbe_osdep.h
203	@kbowling: Here is the patch I referred to earlier: https://reviews.freebsd.org/D50934 Please take a look and let me know if you still have any concerns.

kgalazka added inline comments.Jun 26 2025, 9:27 PM

sys/dev/ixgbe/ixgbe_type_e610.h
805	It looks like we missed that 25G PHY types were removed from the final version of a E610 datasheet, so I guess not in a nearest future. We should probably remove them from the code too.
sys/dev/ixgbe/ixgbe_vf.c
659	I'm not sure about the use case but at least on Linux folks like to have accurate information about the link speed, so unless you think it may cause some issues I would prefer to keep it.

Review comment and a fix for firmware event

Harbormaster completed remote builds in B65294: Diff 158210.Jul 9 2025, 8:42 AM

gowtham.kumar.ks_intel.com accepted this revision.Jul 31 2025, 6:39 AM

@erj @kbowling - We've got a green light from our validation team. Could you please take a look at latest updates?

@kgalazka I'm ok with this, go ahead. Please think about budgeting some time for tinderbox, lint, and early user feedback issues that may come up, and by extension when you want to send it in, i.e. a Monday would be good.

This revision is now accepted and ready to land.Aug 2 2025, 7:58 AM

I think it's ok as-is, but there are some future changes to look at.

sys/dev/ixgbe/if_ix.c
1414	I don't remember -- why couldn't the X550T use these physical layer flags, too? I'm looking at ixgbe_x550.c, and it appears someone could've created an `ixgbe_get_supported_physical_layer_X550T` or similar that specified the 2500BASE_T and 5000BASE_T speeds. Though, the X540 version reads an MDIO register for the capabilities; were those new 2500BASE_T and 5000BASE_T speeds not reported there for X550T devices? Regardless, your explanation is good enough for me for E610 -- I just noticed this because @kbowling called it out and I don't think I ever looked into why there's a difference here, other than X550T didn't "officially" support 2.5G and 5G speeds.
5158	No SR-IOV support? Someone is going to complain.
sys/dev/ixgbe/ixgbe_common.c
3703	Isn't this sort of a codename leak? Someone should rename it from LKV -> E610.
sys/dev/ixgbe/ixgbe_vf.c
649	Doesn't this check need to be updated for E610 VFs as well?

kgalazka added a commit: rGdea5f973d0c8: ix/ixv: Add support for new Intel Ethernet E610 family devices.Aug 19 2025, 6:20 PM

Fixing Eric's comment about codename seemed as a trivial change so I tried to do it without engaging Yogesh and updating the review. I'm sorry, it looks like it was a poor idea. Commit wasn't automatically detected by Phabricator and this review is still open. Is there anything else I should do except for adding the commit with 'Edit Related Objects', and promising to not try to cut the corners in the future?

In D50067#1188547, @kgalazka wrote:

Fixing Eric's comment about codename seemed as a trivial change so I tried to do it without engaging Yogesh and updating the review. I'm sorry, it looks like it was a poor idea. Commit wasn't automatically detected by Phabricator and this review is still open. Is there anything else I should do except for adding the commit with 'Edit Related Objects', and promising to not try to cut the corners in the future?

Trivial edits are a normal thing to do when committing, especially if there is implicit approval like this. The phab integration is based solely on the 'Differential Revision:' tag, and it does seem like the commit was picked up. Usually it would automagically update the review and close it so I don't know what is different here. You can simply close the review out in the action drop down, the commit link is enough context back and forth IMO.

I think the automatic close fails if the diff is too big or has too many files? It's happened a lot to me, but I never figured out why. So as @kbowling says, it's not cutting corners, it's just an imperfect integration between git and Phabricator; it's normal to make small edits between the review being approved and pushing it upstream.

Thanks again!

markj added a subscriber: markj.Aug 28 2025, 7:58 PM

markj added inline comments.

sys/dev/ixgbe/ixgbe_e610.c
1412	I see a compiler warning that this function is unused. Is that expected?

Yogesh.Bhosale_intel.com marked 10 inline comments as done.Sep 2 2025, 7:46 AM

kgalazka added inline comments.Sep 5 2025, 4:10 PM

sys/dev/ixgbe/ixgbe_e610.c
1412	Most probably not. I'm sorry for a late response, I've been AFK due to family reasons. I'll take care of that early next week.

Yogesh.Bhosale_intel.com marked 2 inline comments as done.Sep 10 2025, 7:25 AM

Yogesh.Bhosale_intel.com added inline comments.

sys/dev/ixgbe/ixgbe_e610.c
1412	I have raised a review for the fix: https://reviews.freebsd.org/D52467

markj added inline comments.Sep 10 2025, 12:45 PM

sys/dev/ixgbe/ixgbe_e610.c
1412	Perfect, thank you!

kgalazka added a commit: rGa728b96686e6: ix/ixv: Add support for new Intel Ethernet E610 family devices.Oct 17 2025, 12:01 PM

ix/ixv: Add support for new Intel Ethernet E610 family devices
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 155128

sys/conf/files

sys/dev/ixgbe/if_ix.c

sys/dev/ixgbe/if_ixv.c

sys/dev/ixgbe/ixgbe.h

sys/dev/ixgbe/ixgbe_api.h

sys/dev/ixgbe/ixgbe_api.c

sys/dev/ixgbe/ixgbe_common.c

sys/dev/ixgbe/ixgbe_e610.h

sys/dev/ixgbe/ixgbe_e610.c

sys/dev/ixgbe/ixgbe_osdep.h

sys/dev/ixgbe/ixgbe_osdep.c

sys/dev/ixgbe/ixgbe_type.h

sys/dev/ixgbe/ixgbe_type_e610.h

sys/dev/ixgbe/ixgbe_vf.c

sys/modules/ix/Makefile

sys/modules/ixv/Makefile

ix/ixv: Add support for new Intel Ethernet E610 family devicesClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 155128

sys/conf/files

sys/dev/ixgbe/if_ix.c

sys/dev/ixgbe/if_ixv.c

sys/dev/ixgbe/ixgbe.h

sys/dev/ixgbe/ixgbe_api.h

sys/dev/ixgbe/ixgbe_api.c

sys/dev/ixgbe/ixgbe_common.c

sys/dev/ixgbe/ixgbe_e610.h

sys/dev/ixgbe/ixgbe_e610.c

sys/dev/ixgbe/ixgbe_osdep.h

sys/dev/ixgbe/ixgbe_osdep.c

sys/dev/ixgbe/ixgbe_type.h

sys/dev/ixgbe/ixgbe_type_e610.h

sys/dev/ixgbe/ixgbe_vf.c

sys/modules/ix/Makefile

sys/modules/ixv/Makefile

ix/ixv: Add support for new Intel Ethernet E610 family devices
ClosedPublic
Actions

Revision Contents
Changeset List