[Bug 216456] iflib error checking for MSIX

[bugzilla-noreply at freebsd.org]

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

            Bug ID: 216456
           Summary: iflib error checking for MSIX
           Product: Base System
           Version: CURRENT
          Hardware: Any
                OS: Any
            Status: New
          Severity: Affects Some People
          Priority: ---
         Component: kern
          Assignee: freebsd-net at FreeBSD.org
          Reporter: bz at FreeBSD.org
                CC: sbruno at FreeBSD.org

Structurally this seems entirely backwards that drivers do supply a
isc_msix_bar value without checking if MSIX is available.  Writing code based
on assumptions is not stable but iflib should guard against this.

Add error checking to the pci_find_cap(, PCIY_MSIX,) call that is returns
success and a good value.  Only then try to use it and set the MSIX_ENABLE bit.

With the current em(4) driver we have observed failures in this case in a
specific environment when pci_find_cap() would not return the assumed value,
which meant we ended up writing to PCI register 2 (PCI_DEVICE_ID) which is
read-only.

It seems that a lot more safeguarding and perhaps better structuring between
drivers and iflib is needed to avoid these kinds of errors.

This patch just adds the safeguard in a single place in iflib, em(4) should
ideally be improved to not signal an msix bar value anymore if MSIX is not
avail.


Index: head-r312664.svn/sys/net/iflib.c
===================================================================
--- head-r312664.svn/sys/net/iflib.c    (revision 312664)
+++ head-r312664.svn/sys/net/iflib.c    (working copy)
@@ -3710,6 +3710,10 @@ iflib_device_register(device_t dev, void *sc, if_s
        if (sctx->isc_flags & IFLIB_SKIP_MSIX) {
                msix = scctx->isc_vectors;
        } else if (scctx->isc_msix_bar != 0)
+               /*
+                * The simple fact that isc_msix_bar is not 0 does not mean we
+                * we have a good value there that is known to work.
+                */
                msix = iflib_msix_init(ctx);
        else {
                scctx->isc_vectors = 1;
@@ -4754,15 +4758,21 @@ iflib_msix_init(if_ctx_t ctx)
                uint16_t pci_cmd_word;
                int msix_ctrl, rid;

-               rid = 0;
                pci_cmd_word = pci_read_config(dev, PCIR_COMMAND, 2);
                pci_cmd_word |= PCIM_CMD_BUSMASTEREN;
                pci_write_config(dev, PCIR_COMMAND, pci_cmd_word, 2);
-               pci_find_cap(dev, PCIY_MSIX, &rid);
-               rid += PCIR_MSIX_CTRL;
-               msix_ctrl = pci_read_config(dev, rid, 2);
-               msix_ctrl |= PCIM_MSIXCTRL_MSIX_ENABLE;
-               pci_write_config(dev, rid, msix_ctrl, 2);
+
+               rid = 0;
+               if (pci_find_cap(dev, PCIY_MSIX, &rid) == 0 && rid != 0) {
+                       rid += PCIR_MSIX_CTRL;
+                       msix_ctrl = pci_read_config(dev, rid, 2);
+                       msix_ctrl |= PCIM_MSIXCTRL_MSIX_ENABLE;
+                       pci_write_config(dev, rid, msix_ctrl, 2);
+               } else {
+                       device_printf(dev, "PCIY_MSIX capability not found; "
+                           "or rid %d == 0.\n", rid);
+                       goto msi;
+               }
        }

        /*

-- 
You are receiving this mail because:
You are the assignee for the bug.