Hi Robin,

On 4/3/2018 4:27 PM, Robin Murphy wrote:
On 28/03/18 11:25, Vivek Gautam wrote:
As part of dma_deconfigure, lets deconfigure the iommu too
on driver detach, so that we clear the iommu domain and
related group.

Signed-off-by: Vivek Gautam <vivek.gau...@codeaurora.org>

As a part of dma_deconfigure, shouldn't we deconfigure the iommu
as well? This should reverse all that we do in of_iommu_configure.
So that we call the .remove_device ops for iommu and eventually
clear all the iommu domain, related group infrastructure.
I am seeing that the loadable modules get the same iommu configurations
after re-loading them, i.e. iommu domain, and iova_domain configurations,
as we didn't cleared them.
So should we be clearing these configurations, and therefore do a
of_iommu_deconfigure() or sort?

Nope. Unbinding a driver does not cause the device to stop existing, nor remove it from its parent bus, which is what .remove_device represents. Device grouping is based on the underlying hardware topology, which isn't going to change at runtime, so there's little point in the software state pretending otherwise. If a module is leaking DMA mappings in the default domain when unloaded, that's a bug in that module (and certainly not specific to the IOMMU layer).

Thanks for your review, and really sorry for the delayed response.
I can follow what you said. So the device will only be removed (by .remove_device) when there's a BUS_NOTIFY_REMOVED_DEVICE notifier call.
We can drop this patch.

Best regards

For reference, the only reason we touch .add_device in of_iommu_configure() is because iommu_bus_init() is not quite reliable enough for multiple IOMMU instances serving the platform bus (or similar). It's an ugly workaround which should go away if and when we manage to get rid of the notion of per-bus ops in the API.


  drivers/iommu/of_iommu.c | 11 +++++++++++
  drivers/of/device.c      |  1 +
  include/linux/of_iommu.h |  5 +++++
  3 files changed, 17 insertions(+)

diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index 5c36a8b7656a..1a23e6204ade 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -160,6 +160,17 @@ static int of_pci_iommu_init(struct pci_dev *pdev, u16 alias, void *data)
      return err;
  +void of_iommu_deconfigure(struct device *dev)
+    const struct iommu_ops *ops = NULL;
+    if (dev->iommu_fwspec && dev->iommu_fwspec->ops)
+        ops = dev->iommu_fwspec->ops;
+    if (ops && ops->remove_device && dev->iommu_group)
+        ops->remove_device(dev);
  const struct iommu_ops *of_iommu_configure(struct device *dev,
                         struct device_node *master_np)
diff --git a/drivers/of/device.c b/drivers/of/device.c
index 064c818105bd..e13cb7914dbe 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -177,6 +177,7 @@ EXPORT_SYMBOL_GPL(of_dma_configure);
  void of_dma_deconfigure(struct device *dev)
+    of_iommu_deconfigure(dev);
    int of_device_register(struct platform_device *pdev)
diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
index 4fa654e4b5a9..3d4c22e95c0c 100644
--- a/include/linux/of_iommu.h
+++ b/include/linux/of_iommu.h
@@ -15,6 +15,8 @@ extern int of_get_dma_window(struct device_node *dn, const char *prefix,
  extern const struct iommu_ops *of_iommu_configure(struct device *dev,
                      struct device_node *master_np);
  +extern void of_iommu_deconfigure(struct device *dev);
    static inline int of_get_dma_window(struct device_node *dn, const char *prefix, @@ -30,6 +32,9 @@ static inline const struct iommu_ops *of_iommu_configure(struct device *dev,
      return NULL;
  +static inline void of_iommu_deconfigure(struct device *dev)
+{ }
  #endif    /* CONFIG_OF_IOMMU */
    extern struct of_device_id __iommu_of_table;

iommu mailing list

Reply via email to