e1000: enable "other" interrupt on 82574

[kbowling]

Summary

Enable the "other" interrupt on 82574 to receive link status notifications

PR: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=211219

Test Plan

@franco_opnsense.org do you have the ability to test this patch on FreeBSD 12+ with the inbox em driver?

[franco_opnsense.org]

@kbowling for us https://cgit.freebsd.org/src/commit/?id=0aa7d3ff9ea resolved the issue back then and has not come back. Is this meant to replace it or to be applied on top? I can test it on the specific hardware, but unsure if this will be inconclusive for the mentioned reason

[franco_opnsense.org]

On second thought: maybe I'm mixing up hardware. I'm not sure, but in either case 12.x does seem not exhibit this issue judging from the complete lack of user reports.

[markj]

In D29943#671895, @franco_opnsense.org wrote:

@kbowling for us https://cgit.freebsd.org/src/commit/?id=0aa7d3ff9ea resolved the issue back then and has not come back. Is this meant to replace it or to be applied on top? I can test it on the specific hardware, but unsure if this will be inconclusive for the mentioned reason

I have trouble understanding why that diff is sufficient. We end up setting a bit for "other" in EIAC but not IMS, so as far as I understand, the interrupt is still not enabled.

Before rS286831, we set "other" in both EAIC and IMS, but that revision changed em_enable_intr() for some unexplained reason that looks unrelated to the rest of the diff. Maybe it's some debug patch that snuck in. https://cgit.freebsd.org/src/commit/?id=0aa7d3ff9ea reverts only part of that hunk. The intel-em-kmod driver sets "other" in both registers, as the in-tree FreeBSD driver did originally. The diff in this review will, I believe, bring us back to the pre-rS286831 behaviour for the 82574, so it looks ok to me, but it's hard to judge whether this is necessary given an apparent lack of problem reports.

sys/dev/e1000/if_em.c

2095

E1000_IMS_OTHER?

[kbowling]

sys/dev/e1000/if_em.c

2095

Thanks. I will do some reading tonight and see if EM_MSIX_MASK can go away altogether.

[kbowling]

In D29943#671899, @franco_opnsense.org wrote:

On second thought: maybe I'm mixing up hardware. I'm not sure, but in either case 12.x does seem not exhibit this issue judging from the complete lack of user reports.

Franco, yes I think if you carefully read the code and the comments from erj (in the PR) and markj here you will come agreement. I suspect some of your users are using the intel-em-kmod when they encounter issues like this. I would be delighted if we can get them back to the FreeBSD driver, especially with small fixes such as this,.

[kbowling]

sys/dev/e1000/if_em.c

1517

@marius I'm not really sure why this would be necessary, under what circumstance will the admin task not re-arm link changes? It seems like this might have been because we didn't set the adapter's ims up as this diff will now do.

1534

em-class devices don't have EIMS

1880

This is functionally equivalent i.e.

#include <stdio.h>
#define EM_MSIX_LINK           0x01000000 /* For 82574 use */
int main () {
    unsigned int x = 0;
    x |= 1 << 24;
    if (x == EM_MSIX_LINK)
        printf("%08x == %08x\n", x, EM_MSIX_LINK);
    return 0;
}

01000000 == 01000000

3475

This is functionally equivalent i.e.

#include <stdio.h>
#define EM_MSIX_MASK 0x01F00000 /* For 82574 use */
int main () {
    unsigned int x = 0;
    x |= 1 << 20; // RxQ0
    x |= 1 << 21; // RxQ1
    x |= 1 << 22; // TxQ0
    x |= 1 << 23; // TxQ1
    x |= 1 << 24; // Other
    if (x == EM_MSIX_MASK)
        printf("%08x == %08x\n", x, EM_MSIX_MASK);
    return 0;
}

01f00000 == 01f00000

[kbowling]

sys/dev/e1000/if_em.c

1517

@marius I added this back because I'm not sure under what conditions it will fire. RXSEQ above doesn't seem to be a defined bit on 82574, is that related to the issue this works around?

[kbowling]

Apologies for the excessive churn, this is ready for discussion.

[kbowling]

sys/dev/e1000/if_em.c

1878

@markj @marius for my education, do we defer this re-arm because re-arming in em_msix_link may be too soon in the case of the soft interrupt there?

[markj]

I think the change is ok, I would just suggest simplifying em_msix_link() per my comment there. I tested the diff with an I210 (igb) and didn't notice any obvious regressions there.

sys/dev/e1000/if_em.c

1533–1534

I think this is dead code, the only MSIX-capable devices are igb and 82574, and those are handled in the two cases above.

1878

So one difference between the iflib'ed em and others (pre-iflib em and intel's em) is that em_msix_link() runs as an interrupt filter rather than a threaded interrupt handler. That being the case, I don't understand why em_msix_link() re-arms the LSC at all.

3475

I agree.

[kbowling]

sys/dev/e1000/if_em.c

3487

@marius had changed this to the mac type citing rS206437, but I don't agree. iflib differentiates between msi and msix so we can rely on this test. For another, there is no need to do these writes when 82574 is INTx because the datasheet clearly states EIAC will init zeroed out.

[markj]

Re-tested with igb.

sys/dev/e1000/if_em.c

1879

The indentation looks off here.