Assorted fixes and cleanup for em(4)

[marius]

Summary

• Remove the redundant device disabled hint handling; ever since r241119 that's performed globally by device_attach(9).
• As for the EM-class of devices, em(4) supports multiple queues and MSI-X respectively only with 82574 devices. However, since the conversion to iflib(4), em(4) relies on the interrupt type fallback mechanism, i. e. MSI-X -> MSI -> INTx, of iflib(4) to figure out the interrupt type to use for the EM-class (as well as the IGB-class) of MACs. Moreover, despite the datasheet for 82583V not mentioning any support of MSI-X, there actually are 82583V devices out there that report a varying number of MSI-X messages as supported. The interrupt type fallback of iflib(4) is causing two failure modes depending on the actual number of MSI-X messages supported for such instances of 82583V:
  1. With only one MSI-X message supported, none is left for the RX/TX queues as that one message gets assigned to the admin interrupt. Worse, later on - which will be addressed with a separate fix - iflib(4) interprets that one messages as MSI or INTx to be set up, but fails to actually do so as it has previously called pci_alloc_msix(9). [1, 2]
  2. With more message supported, their distribution is okay but then em_if_msix_intr_assign() doesn't work for 82583V, with the interface being left in a non-working state, too. [3]<br> Thus, let em_if_attach_pre() indicate to iflib(4) to try MSI-X with 82574 only, and at most MSI for the remainder of EM-class devices.<br> While at it, remove "try_second_bar" as it's polarity inverted and not actually needed.
• Remove code from em_if_timer() that effectively is a NOP since the conversion to iflib(4) ("trigger" is no longer read).<br> While at it, let the comment for em_if_timer() reflect reality after said conversion.
• Implement an ifdi_watchdog_reset method which only updates the em(4) "watchdog_events" counter but doesn't perform any reset, so that the em(4) "watchdog_timeouts" SYSCTL (iflib(4) doesn't provide a counterpart) reflects reality and these timeouts add to IFCOUNTER_OERRORS again after the iflib(4) conversion.
• Remove the "mbuf_defrag_fail" and "tx_dma_fail" SYSCTLS; since the iflib(4) conversion, associated counters are disconnected, but iflib(4) provides "mbuf_defrag_failed" and "tx_map_failed" respectively as equivalents.
• Move the description preceding lem_smartspeed() to the correct spot before em_reset() and bring back appropriate comments for {igb,em}_initialize_rss_mapping() and lem_smartspeed() lost in the iflib(4) conversion.
• Adapt some other function descriptions and INIT_DEBUGOUT() use to match reality after the iflib(4) conversion.
• Put the debug message of em_enable_vectors_82574() (missed in r343578) under bootverbose, too.

PR: 219428 [1], 235246 [2], 235147 [3]

[erj]

sys/dev/e1000/if_em.c

837 ↗

(On Diff #53657)

This is good!

It's bizarre that 82583 reports MSI-X support.

1832 ↗

(On Diff #53657)

I think the actual reset happens in _task_fn_admin(), where it sees the "do_reset" flag and calls iflib_if_init_locked() and that's supposed to fix whatever triggered the watchdog.

I don't think the details of this comment matter, but saying the hardware is reset in em_if_update_admin_status() doesn't look right; that only seems to handle statistics and link status.

4542 ↗

(On Diff #53657)

It's less clear what the number means when you replace "Current cap" with the generic-sounding NVM word name.

But OTOH, this value is only interesting if you're debugging, in which case you'd want the NVM word name directly, so I'm ok with this change.

[marius]

Improved em_if_timer() and em_if_watchdog_reset() descriptions, brought back the right one for lem_smartspeed() from prior to the iflib(4) conversion.

[marius]

sys/dev/e1000/if_em.c

1832 ↗

(On Diff #53657)

Well, the comment doesn't say "resetting" but "patting". What I mean are things like the LAA reset for 82571 (which well, actually is a kind of reset) and the smartspeed handling for the LEM-class. Prior to the iflib(4) conversion, these latter were initiated by the {l,}em_local_timer() watchdog routines. Thus, my first thought when looking at the current if_em.c was that these tasks had accidentally been dropped as they are neither performed by the ifdi_timer or the ifdi_watchdog_reset method. Thus, the intent of this comment was kind of to say "look closer in em_if_update_admin_status() for non-link and non-statistical parts".

So while it seems like you missed those controller-specific parts of em_if_update_admin_status() at a quick glance, too, I now think that in fact it's better to hint at these in the description of em_if_timer(). I still think that em_if_watchdog_reset() should bear a comment indicating that no further action in case of a reset is required so it doesn't look like code is missing there.

[marius]

Bring back additional function descriptions lost, adjust some others and INIT_DEBUGOUT() invocations to match reality after the iflib(4) conversion.