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
Vivek
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.
Robin.
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)
{
arch_teardown_dma_ops(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);
+
#else
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
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
