Re: [PATCH v11 3/7] iommu: Add verisilicon IOMMU driver

[Will Deacon <will@kernel.org>]

Hi Benjamin,

Thanks for posting a v11.

On Wed, Jan 07, 2026 at 11:09:53AM +0100, Benjamin Gaignard wrote:
> The Verisilicon IOMMU hardware block can be found in combination
> with Verisilicon hardware video codecs (encoders or decoders) on
> different SoCs.
> Enable it will allow us to use non contiguous memory allocators
> for Verisilicon video codecs.
> If both decoder and this iommu driver are compiled has modules
> there is undefined symboles issues so this iommu driver could
> only be compiled has built-in.
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> ---
> changes in version 11:
> - Fix dependency issue when decoder driver is build as module.
> 
>  drivers/iommu/Kconfig     |  11 +
>  drivers/iommu/Makefile    |   1 +
>  drivers/iommu/vsi-iommu.c | 808 ++++++++++++++++++++++++++++++++++++++
>  include/linux/vsi-iommu.h |  21 +
>  4 files changed, 841 insertions(+)
>  create mode 100644 drivers/iommu/vsi-iommu.c
>  create mode 100644 include/linux/vsi-iommu.h

Based on your reply to v9:

https://lore.kernel.org/all/0eff8b1a-c45f-47b1-a871-59f4a0101f0f@collabora.com/

I took another look at this to see whether it had changed significantly
from v6 when compared to the rockchip driver. Sadly, they still look
very similar to me and I continue to suspect that the hardware is a
derivative. I really don't understand why having a shared implementation
of the default domain ops is difficult or controversial. Have you tried
to write it?

However, given that nobody from the Rockchip side has contributed to the
discussion and you claim that this is a distinct piece of IP, I don't
want to block the merging of the driver by leaving the conversation
hanging.

There is still one thing I don't understand (which, amusingly, the
rockchip driver doesn't seem to suffer from):

> +static void vsi_iommu_flush_tlb_all(struct iommu_domain *domain)
> +{
> +	struct vsi_iommu_domain *vsi_domain = to_vsi_domain(domain);
> +	struct list_head *pos;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&vsi_domain->lock, flags);
> +
> +	list_for_each(pos, &vsi_domain->iommus) {
> +		struct vsi_iommu *iommu;
> +		int ret;
> +
> +		iommu = list_entry(pos, struct vsi_iommu, node);
> +		ret = pm_runtime_resume_and_get(iommu->dev);
> +		if (ret < 0)
> +			continue;
> +
> +		spin_lock(&iommu->lock);
> +
> +		writel(VSI_MMU_BIT_FLUSH, iommu->regs + VSI_MMU_FLUSH_BASE);
> +		writel(0, iommu->regs + VSI_MMU_FLUSH_BASE);
> +
> +		spin_unlock(&iommu->lock);
> +		pm_runtime_put_autosuspend(iommu->dev);
> +	}
> +
> +	spin_unlock_irqrestore(&vsi_domain->lock, flags);
> +}

[...]

> +static const struct iommu_ops vsi_iommu_ops = {
> +	.identity_domain = &vsi_identity_domain,
> +	.release_domain = &vsi_identity_domain,
> +	.domain_alloc_paging = vsi_iommu_domain_alloc_paging,
> +	.of_xlate = vsi_iommu_of_xlate,
> +	.probe_device = vsi_iommu_probe_device,
> +	.release_device = vsi_iommu_release_device,
> +	.device_group = generic_single_device_group,
> +	.owner = THIS_MODULE,
> +	.default_domain_ops = &(const struct iommu_domain_ops) {
> +		.attach_dev		= vsi_iommu_attach_device,
> +		.map_pages		= vsi_iommu_map,
> +		.unmap_pages		= vsi_iommu_unmap,
> +		.flush_iotlb_all	= vsi_iommu_flush_tlb_all,

This has no callers and so your unmap routine appears to be broken.

Will