Subject: Re: [PATCH] powerpc/powernv: Fix IOMMU group lost

From: Wei Yang <[email-masked]>
Date: Tue, 5 Aug 2014 18:00:36 +0800

I didn't manage to test this one PHB3, since some network issue, I can't
access the machine in Austin.

Will reply after I test this on PHB3.

On Tue, Aug 05, 2014 at 06:27:38PM +1000, Gavin Shan wrote:
>When we take full hotplug to recover from EEH errors, PCI buses
>could be involved. For the case, the child devices of involved
>PCI buses can't be attached to IOMMU group properly, which is
>caused by commit 3f28c5a ("powerpc/powernv: Reduce multi-hit of
>iommu_add_device()").
>
>When adding the PCI devices of the newly created PCI buses to
>the system, the IOMMU group is expected to be added in (C).
>(A) fails to bind the IOMMU group because bus->is_added is
>false. (B) fails because the device doesn't have binding IOMMU
>table yet. bus->is_added is set to true at end of (C) and
>pdev->is_added is set to true at (D).
>
>   pcibios_add_pci_devices()
>      pci_scan_bridge()
>         pci_scan_child_bus()
>            pci_scan_slot()
>               pci_scan_single_device()
>                  pci_scan_device()
>                  pci_device_add()
>                     pcibios_add_device()           A: Ignore
>                     device_add()                   B: Ignore
>                  pcibios_fixup_bus()
>                     pcibios_setup_bus_devices()
>                        pcibios_setup_device()      C: Hit
>      pcibios_finish_adding_to_bus()
>         pci_bus_add_devices()
>            pci_bus_add_device()                    D: Add device
>
>If the parent PCI bus isn't involved in hotplug, the IOMMU
>group is expected to be bound in (A).
>
>The patch fixes the issue by reverting commit 3f28c5a and remove
>WARN_ON() in iommu_add_device() to allow calling the function
>even the specified device already has associated IOMMU group.
>
>Cc: <[email-masked]>  # 3.16+
>Reported-by: Thadeu Lima de Souza Cascardo <[email-masked]>
>Signed-off-by: Gavin Shan <[email-masked]>
>Tested-by: Wei Yang <[email-masked]>
>---
> arch/powerpc/kernel/iommu.c               | 30 +++++++++++++-----------------
> arch/powerpc/platforms/powernv/pci-ioda.c |  2 +-
> 2 files changed, 14 insertions(+), 18 deletions(-)
>
>diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
>index 88e3ec6..4663c10 100644
>--- a/arch/powerpc/kernel/iommu.c
>+++ b/arch/powerpc/kernel/iommu.c
>@@ -1120,37 +1120,33 @@ EXPORT_SYMBOL_GPL(iommu_release_ownership);
> int iommu_add_device(struct device *dev)
> {
> 	struct iommu_table *tbl;
>-	int ret = 0;
>
>-	if (WARN_ON(dev->iommu_group)) {
>-		pr_warn("iommu_tce: device %s is already in iommu group %d, skipping\n",
>-				dev_name(dev),
>-				iommu_group_id(dev->iommu_group));
>+	if (dev->iommu_group) {
>+		pr_debug("%s: Skipping device %s with iommu group %d\n",
>+			 __func__, dev_name(dev),
>+			 iommu_group_id(dev->iommu_group));
> 		return -EBUSY;
> 	}
>
> 	tbl = get_iommu_table_base(dev);
> 	if (!tbl || !tbl->it_group) {
>-		pr_debug("iommu_tce: skipping device %s with no tbl\n",
>-				dev_name(dev));
>+		pr_debug("%s: Skipping device %s with no tbl\n",
>+			 __func__, dev_name(dev));
> 		return 0;
> 	}
>
>-	pr_debug("iommu_tce: adding %s to iommu group %d\n",
>-			dev_name(dev), iommu_group_id(tbl->it_group));
>+	pr_debug("%s: Adding %s to iommu group %d\n",
>+		 __func__, dev_name(dev),
>+		 iommu_group_id(tbl->it_group));
>
> 	if (PAGE_SIZE < IOMMU_PAGE_SIZE(tbl)) {
>-		pr_err("iommu_tce: unsupported iommu page size.");
>-		pr_err("%s has not been added\n", dev_name(dev));
>+		pr_err("%s: Invalid IOMMU page size %lx (%lx) on %s\n",
>+		       __func__, IOMMU_PAGE_SIZE(tbl),
>+		       PAGE_SIZE, dev_name(dev));
> 		return -EINVAL;
> 	}
>
>-	ret = iommu_group_add_device(tbl->it_group, dev);
>-	if (ret < 0)
>-		pr_err("iommu_tce: %s has not been added, ret=%d\n",
>-				dev_name(dev), ret);
>-
>-	return ret;
>+	return iommu_group_add_device(tbl->it_group, dev);
> }
> EXPORT_SYMBOL_GPL(iommu_add_device);
>
>diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
>index de19ede..86290eb 100644
>--- a/arch/powerpc/platforms/powernv/pci-ioda.c
>+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
>@@ -462,7 +462,7 @@ static void pnv_pci_ioda_dma_dev_setup(struct pnv_phb *phb, struct pci_dev *pdev
>
> 	pe = &phb->ioda.pe_array[pdn->pe_number];
> 	WARN_ON(get_dma_ops(&pdev->dev) != &dma_iommu_ops);
>-	set_iommu_table_base(&pdev->dev, &pe->tce32_table);
>+	set_iommu_table_base_and_group(&pdev->dev, &pe->tce32_table);
> }
>
> static int pnv_pci_ioda_dma_set_mask(struct pnv_phb *phb,
>-- 
>1.8.3.2

-- 
Richard Yang
Help you, Help me