Hi Jason,
On 21.03.2023 17:42, Jason Gunthorpe wrote:
> On Tue, Mar 21, 2023 at 04:43:42PM +0100, Marek Szyprowski wrote:
>> On 21.03.2023 15:41, Jason Gunthorpe wrote:
>>> On Thu, Mar 16, 2023 at 12:25:14AM +0100, Marek Szyprowski wrote:
>>>> There are some subtle differences between release_device() and
>>>> set_platform_dma_ops() callbacks, so separate those two callbacks. Device
>>>> links should be removed only in release_device(), because they were
>>>> created in probe_device() on purpose and they are needed for proper
>>>> Exynos IOMMU driver operation. While fixing this, remove the conditional
>>>> code as it is not really needed.
>>>>
>>>> Reported-by: Jason Gunthorpe <jgg@xxxxxxxx>
>>>> Fixes: 189d496b48b1 ("iommu/exynos: Add missing set_platform_dma_ops callback")
>>>> Signed-off-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
>>>> ---
>>>> v2:
>>>> - keep set_platform_dma_ops only on ARM 32bit
>>>>
>>>> Some more background why set_platform_dma_ops is needed on ARM 32bit is
>>>> available here:
>>>> https://lore.kernel.org/all/9a12fcac-c347-5d81-acef-1124c50d0c37@xxxxxxx/
>>>> ---
>>>> drivers/iommu/exynos-iommu.c | 17 ++++++++++-------
>>>> 1 file changed, 10 insertions(+), 7 deletions(-)
>>> It seems OK, but do you know what state the device is left in after
>>> exynos_iommu_detach_device ? Ie is it blocking or identity?
>> identity
> Can you do this cleanup like this instead?
Frankly speaking I would like to have a minimal fix (like my $subject
patch) applied to v6.3-rcX ASAP and then apply this identity domain
support on top of than for -next (6.4). I've checked and your proposed
patch works fine in my case, so You can send it as formal patch.
Joerg: is that okay for you?
> Jason
>
> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
> index 483aaaeb6daeac..2c2b5cba191459 100644
> --- a/drivers/iommu/exynos-iommu.c
> +++ b/drivers/iommu/exynos-iommu.c
> @@ -24,6 +24,7 @@
>
> typedef u32 sysmmu_iova_t;
> typedef u32 sysmmu_pte_t;
> +static struct iommu_domain exynos_identity_domain;
>
> /* We do not consider super section mapping (16MB) */
> #define SECT_ORDER 20
> @@ -900,6 +901,9 @@ static struct iommu_domain *exynos_iommu_domain_alloc(unsigned type)
> dma_addr_t handle;
> int i;
>
> + if (type == IOMMU_DOMAIN_IDENTITY)
> + return &exynos_identity_domain;
> +
> /* Check if correct PTE offsets are initialized */
> BUG_ON(PG_ENT_SHIFT < 0 || !dma_dev);
>
> @@ -988,17 +992,22 @@ static void exynos_iommu_domain_free(struct iommu_domain *iommu_domain)
> kfree(domain);
> }
>
> -static void exynos_iommu_detach_device(struct iommu_domain *iommu_domain,
> - struct device *dev)
> +static int exynos_iommu_identity_attach(struct iommu_domain *identity_domain,
> + struct device *dev)
> {
> - struct exynos_iommu_domain *domain = to_exynos_domain(iommu_domain);
> struct exynos_iommu_owner *owner = dev_iommu_priv_get(dev);
> - phys_addr_t pagetable = virt_to_phys(domain->pgtable);
> + struct exynos_iommu_domain *domain;
> + phys_addr_t pagetable;
> struct sysmmu_drvdata *data, *next;
> unsigned long flags;
>
> - if (!has_sysmmu(dev) || owner->domain != iommu_domain)
> - return;
> + if (!owner)
> + return -ENODEV;
> + if (owner->domain == identity_domain)
> + return 0;
> +
> + domain = to_exynos_domain(owner->domain);
> + pagetable = virt_to_phys(domain->pgtable);
>
> mutex_lock(&owner->rpm_lock);
>
> @@ -1017,15 +1026,32 @@ static void exynos_iommu_detach_device(struct iommu_domain *iommu_domain,
> list_del_init(&data->domain_node);
> spin_unlock(&data->lock);
> }
> - owner->domain = NULL;
> + owner->domain = identity_domain;
> spin_unlock_irqrestore(&domain->lock, flags);
>
> mutex_unlock(&owner->rpm_lock);
>
> dev_dbg(dev, "%s: Detached IOMMU with pgtable %pa\n", __func__,
> &pagetable);
> + return 0;
> }
>
> +static struct iommu_domain_ops exynos_identity_ops = {
> + .attach_dev = exynos_iommu_identity_attach,
> +};
> +
> +static struct iommu_domain exynos_identity_domain = {
> + .type = IOMMU_DOMAIN_IDENTITY,
> + .ops = &exynos_identity_ops,
> +};
> +
> +#ifdef CONFIG_ARM
> +static void exynos_iommu_set_platform_dma(struct device *dev)
> +{
> + WARN_ON(exynos_iommu_identity_attach(&exynos_identity_domain, dev));
> +}
> +#endif
> +
> static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain,
> struct device *dev)
> {
> @@ -1034,12 +1060,11 @@ static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain,
> struct sysmmu_drvdata *data;
> phys_addr_t pagetable = virt_to_phys(domain->pgtable);
> unsigned long flags;
> + int err;
>
> - if (!has_sysmmu(dev))
> - return -ENODEV;
> -
> - if (owner->domain)
> - exynos_iommu_detach_device(owner->domain, dev);
> + err = exynos_iommu_identity_attach(&exynos_identity_domain, dev);
> + if (err)
> + return err;
>
> mutex_lock(&owner->rpm_lock);
>
> @@ -1420,18 +1445,7 @@ static void exynos_iommu_release_device(struct device *dev)
> struct exynos_iommu_owner *owner = dev_iommu_priv_get(dev);
> struct sysmmu_drvdata *data;
>
> - if (owner->domain) {
> - struct iommu_group *group = iommu_group_get(dev);
> -
> - if (group) {
> -#ifndef CONFIG_ARM
> - WARN_ON(owner->domain !=
> - iommu_group_default_domain(group));
> -#endif
> - exynos_iommu_detach_device(owner->domain, dev);
> - iommu_group_put(group);
> - }
> - }
> + WARN_ON(exynos_iommu_identity_attach(&exynos_identity_domain, dev));
>
> list_for_each_entry(data, &owner->controllers, owner_node)
> device_link_del(data->link);
> @@ -1462,6 +1476,7 @@ static int exynos_iommu_of_xlate(struct device *dev,
>
> INIT_LIST_HEAD(&owner->controllers);
> mutex_init(&owner->rpm_lock);
> + owner->domain = &exynos_identity_domain;
> dev_iommu_priv_set(dev, owner);
> }
>
> @@ -1479,7 +1494,7 @@ static const struct iommu_ops exynos_iommu_ops = {
> .domain_alloc = exynos_iommu_domain_alloc,
> .device_group = generic_device_group,
> #ifdef CONFIG_ARM
> - .set_platform_dma_ops = exynos_iommu_release_device,
> + .set_platform_dma_ops = exynos_iommu_set_platform_dma,
> #endif
> .probe_device = exynos_iommu_probe_device,
> .release_device = exynos_iommu_release_device,
>
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
