In D61144#1402380, @kgalazka wrote:

Ouch :( my apologies for this oversight. ixgbe_apply_ldo_link_state is guarded by !sc->ldo_valid so it should be fine.

BTW, what do you think about using feat_cap/feat_en instead of a MAC type? I can send a follow-up patch.

We also have over a year to investigate and fix any issues before the next release. Since there are no known regressions currently, makes sense to me to start baking.

In D61147#1401927, @gallatin wrote:

Looks mostly OK to me.

Once concern is that the mp_ring paths got more expensive, but I'm hoping those are on the way out.

Paging Doctor Zeeb. What happens on your system?

In D61147#1401817, @gallatin wrote:

I think the tx task could be running after iflib_stop(), thus leaving us not quite quiesced. Is there any reason why iflib_stop() does not do a gtaskqueue_drain() for tx tasks?

The latest update addresses the size and userland concerns and look good to me.

In D61084#1401681, @ngie wrote:

Could the bug addressed here have helped contribute to https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=300451 ? I see TCP MTU in the stacktrace running the if_lagg tests, which seems weakly related to this issue.

In D61128#1401318, @olivier wrote:

My FrameWork desktop (AMD RYZEN AI MAX+ 395) is running main-n290446-0974ef03b43f and don’t have this "Log cmd failures", but would you like I test this patch anyway ?

This is trickier because i210/i211 aren't read1clear. It may be worth considering delta tracking and resetting the value on init here?

This is reasonable. You could also track deltas and reset the counter on hw init to eliminate the read-write hole.

Looks good to me shape wise, thanks.

There are about a dozen other driver issues, do you want me to open reviews with possible fixups?

I spot bxe(4) sets if_baudrate to 10 Gb/s even for a 1 Gb/s SFP, qlnx sets it based on PCI ID, oce sets on attach but needs update logic all which should be fixed independently.

Added NET_EPOCH_ASSERT() at entry to bstp_linkstate(). Removed the synchronous wait by moving STP mutex destruction into the bridge’s existing epoch cleanup callback. Draining tasks alone would not cover packet input readers, which can still acquire the mutex before checking whether the port is inactive. The existing callback provides the required grace period.

Is there an estimate time on this validation? I've gotten most of the other drivers implemented.

Reworked this and add media status. Can anyone test?

is this still necessary? it seems like the causal commit was reverted

tests/sys/sys/mbuf_test.c still refs M_HASHTYPE_INNER so it needs to be updated or removed

I think I'm fine with this for main if @gallatin and @np agree, the type safety seems worth the churn.

I'm fine with the general premise aside. This would be tricky to MFC, particularly for external drivers. You can remove the test in this diff, or maybe it could be covered adequately by a handful of static asserts?

In D60987#1398491, @adrian wrote:

this may go back to flow director stuff way back when.

OPAQUE_HASH is that it's a hash that isn't necessarily RSS but has hash properties. Ie it's not some small numerical space of possible values. I think OPAQUE was used by flow director to be able to direct /very specific flows/ in specific ways, rather than how RSS determines how to distribute traffic based on hash.

This looks cleaner but it still feels grafted on, and my earlier wonder remains: perhaps unnecessarily; doing what @gallatin said with the call or disproving that would be convincing to me that you've reasoned through the existing mechanism and found the minimal implementation.

In D60602#1389595, @gallatin wrote:

In D60602#1389590, @kbowling wrote:

Otherwise (eventually) drop that file and use __builtin_prefetch directly (my preference). The 2lines variant would be dead code after simpletx so it can remain as some short term hack.

My initial stupid test didn't catch the fact that __builtin_prefetch doesn't work on x86 because clang skips prefetch if you compile with -mno-sse. So we need the wrapper.
For now, I just want to get prefetching working on iflib for arm64. I'll try to run some tests on nt0 vs nt1, which we'd want to do before using kern_prefetch

It looks like rrs added a sys/kern_prefetch.h... I would say use/move the definitions there if you must maintain wrappers (and drop the amd64 case in it).

Could we first evaluate tuning txq_max_db_deferred() and having simpletx honor the existing policy?

It seems like e1000 has the same prior code (igc is generally copied from it)

I'm about to leave on a business trip so I can't give a proper affirmative review, but one thing jumps out without digging into anything.. the multi-line comment and another control flow make it somewhat unpleasing to me. I would investigate a couple ways of writing this, either build the ring decision up with single statement ifs (and line comments as needed), or use de morgans to move true or false around so there are early exits and only one ring decision (perhaps de-indented.. whatever ends up feeling clean after playing with it.

Looks good to me. There are probably similar elsewhere in the tree.

I thought the idea of d9c55b2e8cd6b79f6926278e10a79f1bcca27a4b was to get rid of most of the ifdef. rss_gethashconfig is a public function, why not just apply what you have in the ifdef RSS section always?

This is sufficient as a fix.

Are you waiting for internal validation? I don't like keeping head in a known broken state this long if their findings can be posted as a followup.

Rebase, use vendored TcgService.h, move new code to libefi

@olivier can you make sure I didn't break Pluton boot

In D59477#1371334, @kib wrote:

I am curious what, in your opinion, drivers should do if they get an error from PCI config space access? We tried to disable the busmaster (or any other enable bit in the config register, see below) and it failed, then what?

This now handles the close-first ordering, but the open-first ordering from my example still appears possible. Once an opener passes the SI_LASTCLOSE check, nothing records its presence until after d_open returns and the vnode lock is reacquired. A last close can still run in that interval. We need protection in both directions checking SI_LASTCLOSE only before d_open does not establish mutual exclusion.

It fixes the failed open race (from the mentioned issue) but I was able to trigger the following with an LLM mock:

Drop D_TRACKCLOSE

Should be fixed by updated D59578

In D59692#1370212, @gallatin wrote:

Why is this needed? All NICs should be using the same rss key these days and that's available via the sysctls in sys/net/rss_config.c

@erj thanks that is cleaner