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

[Vivek Gautam <vivek.gautam-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>]

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