Re: [PATCH v3 1/6] ARM/dma-mapping: Add arm_iommu_release_device()

[Robin Murphy <robin.murphy@arm.com>]

On 2023-03-06 02:57, Lu Baolu wrote:
> It is like arm_iommu_detach_device() except it handles the special case
> of being called under an iommu driver's release operation. In this case
> the driver must have already detached the device from any attached
> domain before calling this function.
> 
> Replace arm_iommu_detach_device() with arm_iommu_release_device() in the
> release path of the ipmmu-vmsa driver.
> 
> The bonus is that it also removes a obstacle of arm_iommu_detach_device()
> re-entering the iommu core during release_device. With this removed, the
> iommu core code could be simplified a lot.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>   arch/arm/include/asm/dma-iommu.h |  1 +
>   arch/arm/mm/dma-mapping.c        | 25 +++++++++++++++++++++++++
>   drivers/iommu/ipmmu-vmsa.c       | 15 +++++++++++++--
>   3 files changed, 39 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/include/asm/dma-iommu.h b/arch/arm/include/asm/dma-iommu.h
> index fe9ef6f79e9c..ea7198a17861 100644
> --- a/arch/arm/include/asm/dma-iommu.h
> +++ b/arch/arm/include/asm/dma-iommu.h
> @@ -31,6 +31,7 @@ void arm_iommu_release_mapping(struct dma_iommu_mapping *mapping);
>   int arm_iommu_attach_device(struct device *dev,
>   					struct dma_iommu_mapping *mapping);
>   void arm_iommu_detach_device(struct device *dev);
> +void arm_iommu_release_device(struct device *dev);
>   
>   #endif /* __KERNEL__ */
>   #endif
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index 8bc01071474a..96fa27f4a164 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -1682,6 +1682,31 @@ int arm_iommu_attach_device(struct device *dev,
>   }
>   EXPORT_SYMBOL_GPL(arm_iommu_attach_device);
>   
> +/**
> + * arm_iommu_release_device
> + * @dev: valid struct device pointer
> + *
> + * This is like arm_iommu_detach_device() except it handles the special
> + * case of being called under an iommu driver's release operation. In this
> + * case the driver must have already detached the device from any attached
> + * domain before calling this function.
> + */
> +void arm_iommu_release_device(struct device *dev)
> +{
> +	struct dma_iommu_mapping *mapping;
> +
> +	mapping = to_dma_iommu_mapping(dev);
> +	if (!mapping) {
> +		dev_warn(dev, "Not attached\n");
> +		return;
> +	}
> +
> +	kref_put(&mapping->kref, release_iommu_mapping);
> +	to_dma_iommu_mapping(dev) = NULL;
> +	set_dma_ops(dev, NULL);
> +}
> +EXPORT_SYMBOL_GPL(arm_iommu_release_device);
> +
>   /**
>    * arm_iommu_detach_device
>    * @dev: valid struct device pointer
> diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
> index bdf1a4e5eae0..de9c74cf61a4 100644
> --- a/drivers/iommu/ipmmu-vmsa.c
> +++ b/drivers/iommu/ipmmu-vmsa.c
> @@ -30,7 +30,7 @@
>   #define arm_iommu_create_mapping(...)	NULL
>   #define arm_iommu_attach_device(...)	-ENODEV
>   #define arm_iommu_release_mapping(...)	do {} while (0)
> -#define arm_iommu_detach_device(...)	do {} while (0)
> +#define arm_iommu_release_device(...)	do {} while (0)
>   #endif
>   
>   #define IPMMU_CTX_MAX		16U
> @@ -820,7 +820,18 @@ static void ipmmu_probe_finalize(struct device *dev)
>   
>   static void ipmmu_release_device(struct device *dev)
>   {
> -	arm_iommu_detach_device(dev);
> +	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> +	struct ipmmu_vmsa_device *mmu = to_ipmmu(dev);
> +	unsigned int i;
> +
> +	for (i = 0; i < fwspec->num_ids; ++i) {
> +		unsigned int utlb = fwspec->ids[i];
> +
> +		ipmmu_imuctr_write(mmu, utlb, 0);
> +		mmu->utlb_ctx[utlb] = IPMMU_CTX_INVALID;
> +	}
> +
> +	arm_iommu_release_device(dev);

Just call the existing arm_iommu_release_mapping(). Look at where the 
BUS_NOTIFY_REMOVED_DEVICE point is in device_del(); this device is not 
coming back. Zeroing out pointers and testing for a condition which 
cannot be true by construction is simply a waste of time and code.

Thanks,
Robin.

>   }
>   
>   static struct iommu_group *ipmmu_find_group(struct device *dev)