[PATCH] xen/pci: fix phantom error path in assign_device()

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

Currently if an iommu_call() for a phantom function fails, there is no
indication of the failure. Propagate (but don't return) the error code
from the most recently failed iommu_call() and emit a warning. While
here, add a comment to clarify that the loop keeps iterating even when
failure is encountered.

Fixes: cd7dedad8209 ("passthrough: simplify locking and logging")
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>
---
Unlike assign_device(), deassign_device() breaks out of the phantom loop
when a failure is encountered and returns the error code. I'm curious
why assign_device() and deassign_device() behave differently? It looks
like it's been that way since
4e9950dc1bd2 ("IOMMU: add phantom function support"). I was initially
inclined to break out of the loop and return the error code in
assign_device(), but adhering to the principle of Chesterton's fence,
I'd first like to understand why this is not currently being done.

I'm aware that I could have avoided introducing a tmp local variable by
using the conditional operator with omitted middle operand:

    rc_nonfatal = iommu_call(...) ?: rc_nonfatal;

However, I explicitly chose not to do this to avoid relying on a GNU
extension in yet another place.
---
 xen/drivers/passthrough/pci.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 28ed8ea8172a..71072efceb7a 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -1407,7 +1407,7 @@ static int assign_device(struct domain *d, u16 seg, u8 
bus, u8 devfn, u32 flag)
 {
     const struct domain_iommu *hd = dom_iommu(d);
     struct pci_dev *pdev;
-    int rc = 0;
+    int rc = 0, rc_nonfatal = 0;
 
     if ( !is_iommu_enabled(d) )
         return 0;
@@ -1443,21 +1443,28 @@ static int assign_device(struct domain *d, u16 seg, u8 
bus, u8 devfn, u32 flag)
                           pci_to_dev(pdev), flag)) )
         goto done;
 
-    for ( ; pdev->phantom_stride; rc = 0 )
+    while ( pdev->phantom_stride )
     {
+        int tmp;
+
         devfn += pdev->phantom_stride;
         if ( PCI_SLOT(devfn) != PCI_SLOT(pdev->devfn) )
+        {
+            devfn -= pdev->phantom_stride; /* Adjust for printing */
             break;
+        }
-        rc = iommu_call(hd->platform_ops, assign_device, d, devfn,
-                        pci_to_dev(pdev), flag);
+        tmp = iommu_call(hd->platform_ops, assign_device, d, devfn,
+                         pci_to_dev(pdev), flag);
+        rc_nonfatal = tmp ? tmp : rc_nonfatal;
+        /* Keep iterating even if the iommu call failed. */
     }
 
  done:
-    if ( rc )
+    if ( rc || rc_nonfatal )
         printk(XENLOG_G_WARNING "%pd: assign (%pp) failed (%d)\n",
-               d, &PCI_SBDF(seg, bus, devfn), rc);
+               d, &PCI_SBDF(seg, bus, devfn), rc ? rc : rc_nonfatal);
     /* The device is assigned to dom_io so mark it as quarantined */
-    else if ( d == dom_io )
+    if ( !rc && d == dom_io )
         pdev->quarantine = true;
 
     return rc;

base-commit: 1403131596fa77663708f6baa0fee8bf7b95eb5a
-- 
2.43.0