[PATCH RFC 1/1] iommu/of: Deconfigure iommu on driver detach

[PATCH RFC 1/1] iommu/of: Deconfigure iommu on driver detach

[Vivek Gautam]

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.gautam-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.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?

 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;
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH RFC 1/1] iommu/of: Deconfigure iommu on driver detach

[Robin Murphy]

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.gautam-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.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).

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;
> 

Re: [PATCH RFC 1/1] iommu/of: Deconfigure iommu on driver detach

[Vivek Gautam]

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.gautam@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