Message ID | 20230217112129.18879-1-m.szyprowski@samsung.com (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | iommu/exynos: Fix set_platform_dma_ops() callback | expand |
On 2023/2/17 19:21, 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(). While fixing this, remove the conditional code > as it is not really needed. > > Reported-by: Jason Gunthorpe<jgg@ziepe.ca> > Fixes: 189d496b48b1 ("iommu/exynos: Add missing set_platform_dma_ops callback") The exynos iommu driver actually supports default domain. Why need to add the set_platform_dma_ops? 897 static struct iommu_domain *exynos_iommu_domain_alloc(unsigned type) 898 { 899 struct exynos_iommu_domain *domain; 900 dma_addr_t handle; 901 int i; 902 903 /* Check if correct PTE offsets are initialized */ 904 BUG_ON(PG_ENT_SHIFT < 0 || !dma_dev); 905 906 if (type != IOMMU_DOMAIN_DMA && type != IOMMU_DOMAIN_UNMANAGED) 907 return NULL; It actually doesn't support identity default domain. But the iommu core allows this and roll back to DMA domain instead. 1686 static int iommu_group_alloc_default_domain(struct iommu_group *group, 1687 struct device *dev, 1688 unsigned int type) 1689 { 1690 struct iommu_domain *dom; 1691 1692 dom = __iommu_domain_alloc(dev, type); 1693 if (!dom && type != IOMMU_DOMAIN_DMA) { 1694 dom = __iommu_domain_alloc(dev, IOMMU_DOMAIN_DMA); 1695 if (dom) 1696 pr_warn("Failed to allocate default IOMMU domain of type %u for group %s - Falling back to IOMMU_DOMAIN_D MA", 1697 type, group->name); 1698 } I have a feeling, set_platform_dma_ops was mistakenly added to fix other problems instead of missing a callback. Best regards, baolu
On Fri, Feb 17, 2023 at 12:21:29PM +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(). While fixing this, remove the conditional code > as it is not really needed. > > Reported-by: Jason Gunthorpe <jgg@ziepe.ca> > Fixes: 189d496b48b1 ("iommu/exynos: Add missing set_platform_dma_ops callback") > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- > drivers/iommu/exynos-iommu.c | 19 ++++++++++--------- > 1 file changed, 10 insertions(+), 9 deletions(-) > > diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c > index 483aaaeb6dae..4141625542ef 100644 > --- a/drivers/iommu/exynos-iommu.c > +++ b/drivers/iommu/exynos-iommu.c > @@ -1415,23 +1415,26 @@ static struct iommu_device *exynos_iommu_probe_device(struct device *dev) > return &data->iommu; > } > > -static void exynos_iommu_release_device(struct device *dev) > +static void exynos_iommu_set_platform_dma(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); > } > } See, this is my confusion from the other email, all this does is "detach_device" but it really should restore the arm_iommu's mapping->domain. > @@ -1478,11 +1481,9 @@ static int exynos_iommu_of_xlate(struct device *dev, > 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, > -#endif > .probe_device = exynos_iommu_probe_device, > .release_device = exynos_iommu_release_device, > + .set_platform_dma_ops = exynos_iommu_set_platform_dma, The ifdef is still needed, when using default domains this op should not be set at all. Jason
diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c index 483aaaeb6dae..4141625542ef 100644 --- a/drivers/iommu/exynos-iommu.c +++ b/drivers/iommu/exynos-iommu.c @@ -1415,23 +1415,26 @@ static struct iommu_device *exynos_iommu_probe_device(struct device *dev) return &data->iommu; } -static void exynos_iommu_release_device(struct device *dev) +static void exynos_iommu_set_platform_dma(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); } } +} + +static void exynos_iommu_release_device(struct device *dev) +{ + struct exynos_iommu_owner *owner = dev_iommu_priv_get(dev); + struct sysmmu_drvdata *data; + + exynos_iommu_set_platform_dma(dev); list_for_each_entry(data, &owner->controllers, owner_node) device_link_del(data->link); @@ -1478,11 +1481,9 @@ static int exynos_iommu_of_xlate(struct device *dev, 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, -#endif .probe_device = exynos_iommu_probe_device, .release_device = exynos_iommu_release_device, + .set_platform_dma_ops = exynos_iommu_set_platform_dma, .pgsize_bitmap = SECT_SIZE | LPAGE_SIZE | SPAGE_SIZE, .of_xlate = exynos_iommu_of_xlate, .default_domain_ops = &(const struct iommu_domain_ops) {
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(). While fixing this, remove the conditional code as it is not really needed. Reported-by: Jason Gunthorpe <jgg@ziepe.ca> Fixes: 189d496b48b1 ("iommu/exynos: Add missing set_platform_dma_ops callback") Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> --- drivers/iommu/exynos-iommu.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)