| Message ID | 20230315232514.1046589-1-m.szyprowski@samsung.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] iommu/exynos: Fix set_platform_dma_ops() callback | expand |
On Wed, 15 Mar 2023 at 18:25, Marek Szyprowski <m.szyprowski@samsung.com> 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@ziepe.ca> > Fixes: 189d496b48b1 ("iommu/exynos: Add missing set_platform_dma_ops callback") > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org> > 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@arm.com/ > --- > drivers/iommu/exynos-iommu.c | 17 ++++++++++------- > 1 file changed, 10 insertions(+), 7 deletions(-) > > diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c > index 483aaaeb6dae..1abd187c6075 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); > @@ -1479,7 +1482,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, > -- > 2.34.1 >
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@ziepe.ca> > Fixes: 189d496b48b1 ("iommu/exynos: Add missing set_platform_dma_ops callback") > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- > 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@arm.com/ > --- > 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? Jason
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@ziepe.ca> > >> Fixes: 189d496b48b1 ("iommu/exynos: Add missing set_platform_dma_ops callback") > >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > >> --- > >> 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@arm.com/ > >> --- > >> 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? 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,
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@ziepe.ca> >>>> Fixes: 189d496b48b1 ("iommu/exynos: Add missing set_platform_dma_ops callback") >>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >>>> --- >>>> 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@arm.com/ >>>> --- >>>> 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
On Fri, Mar 24, 2023 at 09:59:27PM +0100, Marek Szyprowski wrote: > 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@ziepe.ca> > >>>> Fixes: 189d496b48b1 ("iommu/exynos: Add missing set_platform_dma_ops callback") > >>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > >>>> --- > >>>> 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@arm.com/ > >>>> --- > >>>> 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. I thought this was a cosmetic fix, do you have an actual bug here? If so we should split it as you say, but you should describe the actual bug in the commit message. Jason
On Fri, Mar 24, 2023 at 09:59:27PM +0100, Marek Szyprowski wrote: > 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? Agreed, a minimal fix for 6.3 would be great. Regards, Joerg
On 28.03.2023 15:11, Joerg Roedel wrote: > On Fri, Mar 24, 2023 at 09:59:27PM +0100, Marek Szyprowski wrote: >> 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? > Agreed, a minimal fix for 6.3 would be great. Then this v2 is the minimal fix. Best regards
On Thu, Mar 16, 2023 at 12:25:14AM +0100, Marek Szyprowski wrote: > drivers/iommu/exynos-iommu.c | 17 ++++++++++------- > 1 file changed, 10 insertions(+), 7 deletions(-) Applied to iommu/fixes.
diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c index 483aaaeb6dae..1abd187c6075 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); @@ -1479,7 +1482,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,
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@ziepe.ca> Fixes: 189d496b48b1 ("iommu/exynos: Add missing set_platform_dma_ops callback") Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> --- 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@arm.com/ --- drivers/iommu/exynos-iommu.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)