| Message ID | 20230324111127.221640-1-steven.price@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] iommu/rockchip: Add missing set_platform_dma_ops callback | expand |
On 2023/3/24 19:11, Steven Price wrote: > Similar to exynos, we need a set_platform_dma_ops() callback for proper > operation on ARM 32 bit after recent changes in the IOMMU framework > (detach ops removal). But also the use of a NULL domain is confusing. > > Rework the code to have a singleton rk_identity_domain which is assigned > to domain when using an identity mapping rather than "detaching". This > makes the code easier to reason about. > > Signed-off-by: Steven Price <steven.price@arm.com> > --- > Changes since v1[1]: > > * Reworked the code to avoid a NULL domain, instead a singleton > rk_identity_domain is used instead. The 'detach' language is no > longer used. > > [1] https://lore.kernel.org/r/20230315164152.333251-1-steven.price%40arm.com > > drivers/iommu/rockchip-iommu.c | 50 ++++++++++++++++++++++++++-------- > 1 file changed, 39 insertions(+), 11 deletions(-) > > diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c > index f30db22ea5d7..437541004994 100644 > --- a/drivers/iommu/rockchip-iommu.c > +++ b/drivers/iommu/rockchip-iommu.c > @@ -124,6 +124,7 @@ struct rk_iommudata { > > static struct device *dma_dev; > static const struct rk_iommu_ops *rk_ops; > +static struct iommu_domain rk_identity_domain; > > static inline void rk_table_flush(struct rk_iommu_domain *dom, dma_addr_t dma, > unsigned int count) > @@ -980,26 +981,27 @@ static int rk_iommu_enable(struct rk_iommu *iommu) > return ret; > } > > -static void rk_iommu_detach_device(struct iommu_domain *domain, > - struct device *dev) > +static int rk_iommu_identity_attach(struct iommu_domain *identity_domain, > + struct device *dev) > { > struct rk_iommu *iommu; > - struct rk_iommu_domain *rk_domain = to_rk_domain(domain); > + struct rk_iommu_domain *rk_domain; > unsigned long flags; > int ret; > > /* Allow 'virtual devices' (eg drm) to detach from domain */ > iommu = rk_iommu_from_dev(dev); > if (!iommu) > - return; > + return -ENODEV; > + > + rk_domain = to_rk_domain(iommu->domain); > > dev_dbg(dev, "Detaching from iommu domain\n"); > > - /* iommu already detached */ > - if (iommu->domain != domain) > - return; > + if (iommu->domain == identity_domain) > + return 0; > > - iommu->domain = NULL; > + iommu->domain = identity_domain; Where did identity_domain come from? Is it rk_identity_domain? Best regards, baolu
On 24/03/2023 13:16, Baolu Lu wrote: > On 2023/3/24 19:11, Steven Price wrote: >> Similar to exynos, we need a set_platform_dma_ops() callback for proper >> operation on ARM 32 bit after recent changes in the IOMMU framework >> (detach ops removal). But also the use of a NULL domain is confusing. >> >> Rework the code to have a singleton rk_identity_domain which is assigned >> to domain when using an identity mapping rather than "detaching". This >> makes the code easier to reason about. >> >> Signed-off-by: Steven Price <steven.price@arm.com> >> --- >> Changes since v1[1]: >> >> * Reworked the code to avoid a NULL domain, instead a singleton >> rk_identity_domain is used instead. The 'detach' language is no >> longer used. >> >> [1] >> https://lore.kernel.org/r/20230315164152.333251-1-steven.price%40arm.com >> >> drivers/iommu/rockchip-iommu.c | 50 ++++++++++++++++++++++++++-------- >> 1 file changed, 39 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/iommu/rockchip-iommu.c >> b/drivers/iommu/rockchip-iommu.c >> index f30db22ea5d7..437541004994 100644 >> --- a/drivers/iommu/rockchip-iommu.c >> +++ b/drivers/iommu/rockchip-iommu.c >> @@ -124,6 +124,7 @@ struct rk_iommudata { >> static struct device *dma_dev; >> static const struct rk_iommu_ops *rk_ops; >> +static struct iommu_domain rk_identity_domain; >> static inline void rk_table_flush(struct rk_iommu_domain *dom, >> dma_addr_t dma, >> unsigned int count) >> @@ -980,26 +981,27 @@ static int rk_iommu_enable(struct rk_iommu *iommu) >> return ret; >> } >> -static void rk_iommu_detach_device(struct iommu_domain *domain, >> - struct device *dev) >> +static int rk_iommu_identity_attach(struct iommu_domain >> *identity_domain, >> + struct device *dev) >> { >> struct rk_iommu *iommu; >> - struct rk_iommu_domain *rk_domain = to_rk_domain(domain); >> + struct rk_iommu_domain *rk_domain; >> unsigned long flags; >> int ret; >> /* Allow 'virtual devices' (eg drm) to detach from domain */ >> iommu = rk_iommu_from_dev(dev); >> if (!iommu) >> - return; >> + return -ENODEV; >> + >> + rk_domain = to_rk_domain(iommu->domain); >> dev_dbg(dev, "Detaching from iommu domain\n"); >> - /* iommu already detached */ >> - if (iommu->domain != domain) >> - return; >> + if (iommu->domain == identity_domain) >> + return 0; >> - iommu->domain = NULL; >> + iommu->domain = identity_domain; > > Where did identity_domain come from? Is it rk_identity_domain? It's a parameter of the function. In the case of the call in rk_iommu_attach_device() then, yes, it's rk_identity_domain. But this function is also the "attach_dev" callback of "rk_identity_ops". I'll admit this is cargo-culted from Jason's example: https://lore.kernel.org/linux-iommu/ZBnef7g7GCxogPNz@ziepe.ca/ Steve > Best regards, > baolu
On 2023/3/24 21:24, Steven Price wrote: > On 24/03/2023 13:16, Baolu Lu wrote: >> On 2023/3/24 19:11, Steven Price wrote: >>> Similar to exynos, we need a set_platform_dma_ops() callback for proper >>> operation on ARM 32 bit after recent changes in the IOMMU framework >>> (detach ops removal). But also the use of a NULL domain is confusing. >>> >>> Rework the code to have a singleton rk_identity_domain which is assigned >>> to domain when using an identity mapping rather than "detaching". This >>> makes the code easier to reason about. >>> >>> Signed-off-by: Steven Price<steven.price@arm.com> >>> --- >>> Changes since v1[1]: >>> >>> * Reworked the code to avoid a NULL domain, instead a singleton >>> rk_identity_domain is used instead. The 'detach' language is no >>> longer used. >>> >>> [1] >>> https://lore.kernel.org/r/20230315164152.333251-1-steven.price%40arm.com >>> >>> drivers/iommu/rockchip-iommu.c | 50 ++++++++++++++++++++++++++-------- >>> 1 file changed, 39 insertions(+), 11 deletions(-) >>> >>> diff --git a/drivers/iommu/rockchip-iommu.c >>> b/drivers/iommu/rockchip-iommu.c >>> index f30db22ea5d7..437541004994 100644 >>> --- a/drivers/iommu/rockchip-iommu.c >>> +++ b/drivers/iommu/rockchip-iommu.c >>> @@ -124,6 +124,7 @@ struct rk_iommudata { >>> static struct device *dma_dev; >>> static const struct rk_iommu_ops *rk_ops; >>> +static struct iommu_domain rk_identity_domain; >>> static inline void rk_table_flush(struct rk_iommu_domain *dom, >>> dma_addr_t dma, >>> unsigned int count) >>> @@ -980,26 +981,27 @@ static int rk_iommu_enable(struct rk_iommu *iommu) >>> return ret; >>> } >>> -static void rk_iommu_detach_device(struct iommu_domain *domain, >>> - struct device *dev) >>> +static int rk_iommu_identity_attach(struct iommu_domain >>> *identity_domain, >>> + struct device *dev) >>> { >>> struct rk_iommu *iommu; >>> - struct rk_iommu_domain *rk_domain = to_rk_domain(domain); >>> + struct rk_iommu_domain *rk_domain; >>> unsigned long flags; >>> int ret; >>> /* Allow 'virtual devices' (eg drm) to detach from domain */ >>> iommu = rk_iommu_from_dev(dev); >>> if (!iommu) >>> - return; >>> + return -ENODEV; >>> + >>> + rk_domain = to_rk_domain(iommu->domain); >>> dev_dbg(dev, "Detaching from iommu domain\n"); >>> - /* iommu already detached */ >>> - if (iommu->domain != domain) >>> - return; >>> + if (iommu->domain == identity_domain) >>> + return 0; >>> - iommu->domain = NULL; >>> + iommu->domain = identity_domain; >> Where did identity_domain come from? Is it rk_identity_domain? > It's a parameter of the function. In the case of the call in > rk_iommu_attach_device() then, yes, it's rk_identity_domain. But this > function is also the "attach_dev" callback of "rk_identity_ops". > > I'll admit this is cargo-culted from Jason's example: > > https://lore.kernel.org/linux-iommu/ZBnef7g7GCxogPNz@ziepe.ca/ Oh! I overlooked that. Thank you for the explanation. Best regards, baolu
On Fri, Mar 24, 2023 at 11:11:27AM +0000, Steven Price wrote: > Similar to exynos, we need a set_platform_dma_ops() callback for proper > operation on ARM 32 bit after recent changes in the IOMMU framework > (detach ops removal). But also the use of a NULL domain is confusing. > > Rework the code to have a singleton rk_identity_domain which is assigned > to domain when using an identity mapping rather than "detaching". This > makes the code easier to reason about. > > Signed-off-by: Steven Price <steven.price@arm.com> > --- > Changes since v1[1]: > > * Reworked the code to avoid a NULL domain, instead a singleton > rk_identity_domain is used instead. The 'detach' language is no > longer used. > > [1] https://lore.kernel.org/r/20230315164152.333251-1-steven.price%40arm.com > > drivers/iommu/rockchip-iommu.c | 50 ++++++++++++++++++++++++++-------- > 1 file changed, 39 insertions(+), 11 deletions(-) > > diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c > index f30db22ea5d7..437541004994 100644 > --- a/drivers/iommu/rockchip-iommu.c > +++ b/drivers/iommu/rockchip-iommu.c [snip] > +static struct iommu_domain rk_identity_domain = { > + .type = IOMMU_DOMAIN_IDENTITY, > + .ops = &rk_identity_ops, > +}; > + > +#ifdef CONFIG_ARM Is this #ifdef needed? I can't see anything ARM-specific about this function or .set_platform_dma_ops. > +static void rk_iommu_set_platform_dma(struct device *dev) > +{ > + WARN_ON(rk_iommu_identity_attach(&rk_identity_domain, dev)); > } > +#endif Not shown in the patch are the pm_runtime hooks. Do they need to change like this? static int __maybe_unused rk_iommu_suspend(struct device *dev) { struct rk_iommu *iommu = dev_get_drvdata(dev); - if (!iommu->domain) + if (iommu->domain == &rk_identity_domain) return 0; rk_iommu_disable(iommu); return 0; } static int __maybe_unused rk_iommu_resume(struct device *dev) { struct rk_iommu *iommu = dev_get_drvdata(dev); - if (!iommu->domain) + if (iommu->domain == &rk_identity_domain) return 0; return rk_iommu_enable(iommu); }
On Mon, Mar 27, 2023 at 03:35:04PM +0100, John Keeping wrote: > On Fri, Mar 24, 2023 at 11:11:27AM +0000, Steven Price wrote: > > Similar to exynos, we need a set_platform_dma_ops() callback for proper > > operation on ARM 32 bit after recent changes in the IOMMU framework > > (detach ops removal). But also the use of a NULL domain is confusing. > > > > Rework the code to have a singleton rk_identity_domain which is assigned > > to domain when using an identity mapping rather than "detaching". This > > makes the code easier to reason about. > > > > Signed-off-by: Steven Price <steven.price@arm.com> > > --- > > Changes since v1[1]: > > > > * Reworked the code to avoid a NULL domain, instead a singleton > > rk_identity_domain is used instead. The 'detach' language is no > > longer used. > > > > [1] https://lore.kernel.org/r/20230315164152.333251-1-steven.price%40arm.com > > > > drivers/iommu/rockchip-iommu.c | 50 ++++++++++++++++++++++++++-------- > > 1 file changed, 39 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c > > index f30db22ea5d7..437541004994 100644 > > --- a/drivers/iommu/rockchip-iommu.c > > +++ b/drivers/iommu/rockchip-iommu.c > [snip] > > +static struct iommu_domain rk_identity_domain = { > > + .type = IOMMU_DOMAIN_IDENTITY, > > + .ops = &rk_identity_ops, > > +}; > > + > > +#ifdef CONFIG_ARM > > Is this #ifdef needed? I can't see anything ARM-specific about this > function or .set_platform_dma_ops. set_platform_dma_ops is never called on ARM64. > Not shown in the patch are the pm_runtime hooks. Do they need to > change like this? Most likely yes Jason
On Fri, Mar 24, 2023 at 11:11:27AM +0000, Steven Price wrote: > Similar to exynos, we need a set_platform_dma_ops() callback for proper > operation on ARM 32 bit after recent changes in the IOMMU framework > (detach ops removal). But also the use of a NULL domain is confusing. > > Rework the code to have a singleton rk_identity_domain which is assigned > to domain when using an identity mapping rather than "detaching". This > makes the code easier to reason about. > > Signed-off-by: Steven Price <steven.price@arm.com> > --- > Changes since v1[1]: > > * Reworked the code to avoid a NULL domain, instead a singleton > rk_identity_domain is used instead. The 'detach' language is no > longer used. > > [1] https://lore.kernel.org/r/20230315164152.333251-1-steven.price%40arm.com > > drivers/iommu/rockchip-iommu.c | 50 ++++++++++++++++++++++++++-------- > 1 file changed, 39 insertions(+), 11 deletions(-) Aside from the pm stuff this looks OK to me Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> Thanks, Jason
On 27/03/2023 16:26, Jason Gunthorpe wrote: > On Mon, Mar 27, 2023 at 03:35:04PM +0100, John Keeping wrote: >> On Fri, Mar 24, 2023 at 11:11:27AM +0000, Steven Price wrote: >>> Similar to exynos, we need a set_platform_dma_ops() callback for proper >>> operation on ARM 32 bit after recent changes in the IOMMU framework >>> (detach ops removal). But also the use of a NULL domain is confusing. >>> >>> Rework the code to have a singleton rk_identity_domain which is assigned >>> to domain when using an identity mapping rather than "detaching". This >>> makes the code easier to reason about. >>> >>> Signed-off-by: Steven Price <steven.price@arm.com> >>> --- >>> Changes since v1[1]: >>> >>> * Reworked the code to avoid a NULL domain, instead a singleton >>> rk_identity_domain is used instead. The 'detach' language is no >>> longer used. >>> >>> [1] https://lore.kernel.org/r/20230315164152.333251-1-steven.price%40arm.com >>> >>> drivers/iommu/rockchip-iommu.c | 50 ++++++++++++++++++++++++++-------- >>> 1 file changed, 39 insertions(+), 11 deletions(-) >>> >>> diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c >>> index f30db22ea5d7..437541004994 100644 >>> --- a/drivers/iommu/rockchip-iommu.c >>> +++ b/drivers/iommu/rockchip-iommu.c >> [snip] >>> +static struct iommu_domain rk_identity_domain = { >>> + .type = IOMMU_DOMAIN_IDENTITY, >>> + .ops = &rk_identity_ops, >>> +}; >>> + >>> +#ifdef CONFIG_ARM >> >> Is this #ifdef needed? I can't see anything ARM-specific about this >> function or .set_platform_dma_ops. > > set_platform_dma_ops is never called on ARM64. I have to admit this was somewhat cargo-culted from the exynos-iommu change. This is only needed for Arm, but I'm not sure if there's any real harm including it for other arches. >> Not shown in the patch are the pm_runtime hooks. Do they need to >> change like this? > > Most likely yes Good spot - I'll send a v3 with this fixed. Thanks, Steve
diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c index f30db22ea5d7..437541004994 100644 --- a/drivers/iommu/rockchip-iommu.c +++ b/drivers/iommu/rockchip-iommu.c @@ -124,6 +124,7 @@ struct rk_iommudata { static struct device *dma_dev; static const struct rk_iommu_ops *rk_ops; +static struct iommu_domain rk_identity_domain; static inline void rk_table_flush(struct rk_iommu_domain *dom, dma_addr_t dma, unsigned int count) @@ -980,26 +981,27 @@ static int rk_iommu_enable(struct rk_iommu *iommu) return ret; } -static void rk_iommu_detach_device(struct iommu_domain *domain, - struct device *dev) +static int rk_iommu_identity_attach(struct iommu_domain *identity_domain, + struct device *dev) { struct rk_iommu *iommu; - struct rk_iommu_domain *rk_domain = to_rk_domain(domain); + struct rk_iommu_domain *rk_domain; unsigned long flags; int ret; /* Allow 'virtual devices' (eg drm) to detach from domain */ iommu = rk_iommu_from_dev(dev); if (!iommu) - return; + return -ENODEV; + + rk_domain = to_rk_domain(iommu->domain); dev_dbg(dev, "Detaching from iommu domain\n"); - /* iommu already detached */ - if (iommu->domain != domain) - return; + if (iommu->domain == identity_domain) + return 0; - iommu->domain = NULL; + iommu->domain = identity_domain; spin_lock_irqsave(&rk_domain->iommus_lock, flags); list_del_init(&iommu->node); @@ -1011,7 +1013,25 @@ static void rk_iommu_detach_device(struct iommu_domain *domain, rk_iommu_disable(iommu); pm_runtime_put(iommu->dev); } + + return 0; +} + +static struct iommu_domain_ops rk_identity_ops = { + .attach_dev = rk_iommu_identity_attach, +}; + +static struct iommu_domain rk_identity_domain = { + .type = IOMMU_DOMAIN_IDENTITY, + .ops = &rk_identity_ops, +}; + +#ifdef CONFIG_ARM +static void rk_iommu_set_platform_dma(struct device *dev) +{ + WARN_ON(rk_iommu_identity_attach(&rk_identity_domain, dev)); } +#endif static int rk_iommu_attach_device(struct iommu_domain *domain, struct device *dev) @@ -1035,8 +1055,9 @@ static int rk_iommu_attach_device(struct iommu_domain *domain, if (iommu->domain == domain) return 0; - if (iommu->domain) - rk_iommu_detach_device(iommu->domain, dev); + ret = rk_iommu_identity_attach(&rk_identity_domain, dev); + if (ret) + return ret; iommu->domain = domain; @@ -1050,7 +1071,7 @@ static int rk_iommu_attach_device(struct iommu_domain *domain, ret = rk_iommu_enable(iommu); if (ret) - rk_iommu_detach_device(iommu->domain, dev); + WARN_ON(rk_iommu_identity_attach(&rk_identity_domain, dev)); pm_runtime_put(iommu->dev); @@ -1061,6 +1082,9 @@ static struct iommu_domain *rk_iommu_domain_alloc(unsigned type) { struct rk_iommu_domain *rk_domain; + if (type == IOMMU_DOMAIN_IDENTITY) + return &rk_identity_domain; + if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA) return NULL; @@ -1176,6 +1200,7 @@ static int rk_iommu_of_xlate(struct device *dev, iommu_dev = of_find_device_by_node(args->np); data->iommu = platform_get_drvdata(iommu_dev); + data->iommu->domain = &rk_identity_domain; dev_iommu_priv_set(dev, data); platform_device_put(iommu_dev); @@ -1188,6 +1213,9 @@ static const struct iommu_ops rk_iommu_ops = { .probe_device = rk_iommu_probe_device, .release_device = rk_iommu_release_device, .device_group = rk_iommu_device_group, +#ifdef CONFIG_ARM + .set_platform_dma_ops = rk_iommu_set_platform_dma, +#endif .pgsize_bitmap = RK_IOMMU_PGSIZE_BITMAP, .of_xlate = rk_iommu_of_xlate, .default_domain_ops = &(const struct iommu_domain_ops) {
Similar to exynos, we need a set_platform_dma_ops() callback for proper operation on ARM 32 bit after recent changes in the IOMMU framework (detach ops removal). But also the use of a NULL domain is confusing. Rework the code to have a singleton rk_identity_domain which is assigned to domain when using an identity mapping rather than "detaching". This makes the code easier to reason about. Signed-off-by: Steven Price <steven.price@arm.com> --- Changes since v1[1]: * Reworked the code to avoid a NULL domain, instead a singleton rk_identity_domain is used instead. The 'detach' language is no longer used. [1] https://lore.kernel.org/r/20230315164152.333251-1-steven.price%40arm.com drivers/iommu/rockchip-iommu.c | 50 ++++++++++++++++++++++++++-------- 1 file changed, 39 insertions(+), 11 deletions(-)