Page MenuHomeFreeBSD
Differential D50067

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

Authored by Yogesh.Bhosale_intel.com on Apr 29 2025, 10:44 AM.
Tags
None
Referenced Files
F170577058: D50067.id155128.diff
Sat, Sep 5, 11:54 AM
Unknown Object (File)
Wed, Sep 2, 12:53 PM
Unknown Object (File)
Tue, Sep 1, 8:40 PM
Unknown Object (File)
Fri, Aug 28, 1:42 PM
Unknown Object (File)
Fri, Aug 28, 1:42 PM
Unknown Object (File)
Fri, Aug 28, 1:42 PM
Unknown Object (File)
Fri, Aug 28, 1:41 PM
Unknown Object (File)
Fri, Aug 28, 1:40 PM
View All Files
Subscribers
ae
erj
imp
kbowling
kgalazka
markj

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 Passed
Unit
No Test Coverage
Build Status
Buildable 64458
Build 61342: arc lint + arc unit

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
kgalazka added reviewers: erj, kbowling.May 13 2025, 6:34 PM
kgalazka added subscribers: kbowling, erj, kgalazka.

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

erj added a comment.May 14 2025, 7:58 PM

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
806

Any 25G Base-T parts on the horizon for Linkville? :p

Or, are those only coming in maybe a future follow-on...

kbowling added a comment.May 14 2025, 8:28 PM

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.

kgalazka added a comment.May 16 2025, 7:59 PM
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.

kbowling requested changes to this revision.May 16 2025, 8:25 PM

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.

Yogesh.Bhosale_intel.com updated this revision to Diff 156040.May 26 2025, 9:39 AM

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
Yogesh.Bhosale_intel.com updated this revision to Diff 156044.May 26 2025, 10:28 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.

Yogesh.Bhosale_intel.com updated this revision to Diff 156080.May 27 2025, 8:30 AM
Yogesh.Bhosale_intel.com marked 3 inline comments as done.

Fixing a typo

Harbormaster completed remote builds in B64458: Diff 156080.May 27 2025, 8:30 AM
kbowling added a comment.May 30 2025, 12:28 PM

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.

Yogesh.Bhosale_intel.com updated this revision to Diff 156873.Jun 12 2025, 9:58 AM

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
806

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.

Yogesh.Bhosale_intel.com updated this revision to Diff 158210.Jul 9 2025, 8:42 AM

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
kgalazka added a comment.Jul 31 2025, 9:35 PM

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

kbowling accepted this revision.Aug 2 2025, 7:58 AM

@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
erj accepted this revision.Aug 4 2025, 2:43 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.

5153

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
kgalazka added a comment.Aug 19 2025, 6:41 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?

kbowling added a comment.Aug 19 2025, 6:48 PM
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.

erj added a comment.Aug 19 2025, 7:21 PM

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.

kgalazka closed this revision.Aug 20 2025, 7:13 PM

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

Revision Contents
Changeset List

PathSize
sys/
conf/
2 lines
dev/
ixgbe/
224 lines
6 lines
11 lines
1 line
16 lines
25 lines
224 lines
5586 lines
33 lines
26 lines
69 lines
2289 lines
3 lines
modules/
ix/
2 lines
ixv/
2 lines

Diff 156080

View Options

sys/conf/files

Loading...
View Options

sys/dev/ixgbe/if_ix.c

Loading...
View Options

sys/dev/ixgbe/if_ixv.c

Loading...
View Options

sys/dev/ixgbe/ixgbe.h

Loading...
View Options

sys/dev/ixgbe/ixgbe_api.h

Loading...
View Options

sys/dev/ixgbe/ixgbe_api.c

Loading...
View Options

sys/dev/ixgbe/ixgbe_common.c

Loading...
View Options

sys/dev/ixgbe/ixgbe_e610.h

Loading...
View Options

sys/dev/ixgbe/ixgbe_e610.c

Loading...
View Options

sys/dev/ixgbe/ixgbe_osdep.h

Loading...
View Options

sys/dev/ixgbe/ixgbe_osdep.c

Loading...
View Options

sys/dev/ixgbe/ixgbe_type.h

Loading...
View Options

sys/dev/ixgbe/ixgbe_type_e610.h

Loading...
View Options

sys/dev/ixgbe/ixgbe_vf.c

Loading...
View Options

sys/modules/ix/Makefile

Loading...
View Options

sys/modules/ixv/Makefile

Loading...