Differential D19108

Assorted fixes and cleanup for em(4)
ClosedPublic
Actions

Authored by marius on Feb 7 2019, 3:30 PM.

Details

Reviewers

shurd
sbruno

Group Reviewers

Intel Networking

Commits

rS344098: MFC: r343934
rS343934: - Remove the redundant device disabled hint handling; ever since

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]

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 22383

Event Timeline

marius created this revision.Feb 7 2019, 3:30 PM

Herald added a reviewer: sbruno. · View Herald TranscriptFeb 7 2019, 3:30 PM

Herald added a subscriber: imp. · View Herald Transcript

Harbormaster completed remote builds in B22379: Diff 53657.Feb 7 2019, 3:30 PM

marius edited the summary of this revision. (Show Details)Feb 7 2019, 3:33 PM

marius edited the summary of this revision. (Show Details)

erj added a subscriber: erj.Feb 7 2019, 8:05 PM

erj added inline comments.

sys/dev/e1000/if_em.c
837	This is good! It's bizarre that 82583 reports MSI-X support.
1833	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.
4540	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.

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.

Harbormaster completed remote builds in B22383: Diff 53673.Feb 7 2019, 10:13 PM

marius added inline comments.Feb 7 2019, 10:14 PM

sys/dev/e1000/if_em.c
1833	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.