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

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.

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.

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

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.