This should say "Link", right?

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?

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

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

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.

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?

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?

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.

This is intentional. The X550 relies on mac type for speed detection, while the E610 driver can determine speeds using layer flags.

I will fix that.

Done.

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.

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.

As above - I'm not sure what do you mean. I don't see anything like that on main.

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.

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

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.

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.