| Message ID | 20230315164152.333251-1-steven.price@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iommu/rockchip: Add missing set_platform_dma_ops callback | expand |
On Wed, Mar 15, 2023 at 04:41:52PM +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). > > Fixes: c1fe9119ee70 ("iommu: Add set_platform_dma_ops callbacks") > Signed-off-by: Steven Price <steven.price@arm.com> > --- > This fixes a splat I was seeing on a Firefly-RK3288, more details here: > https://lore.kernel.org/all/26a5d1b8-40b3-b1e4-bc85-740409c26838@arm.com/ Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> Do you know what state the iommu is left in after rk_iommu_detach_device()? Ie is it blocking DMA or doing identity or something else? Jason
On 21/03/2023 14:38, Jason Gunthorpe wrote: > On Wed, Mar 15, 2023 at 04:41:52PM +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). >> >> Fixes: c1fe9119ee70 ("iommu: Add set_platform_dma_ops callbacks") >> Signed-off-by: Steven Price <steven.price@arm.com> >> --- >> This fixes a splat I was seeing on a Firefly-RK3288, more details here: >> https://lore.kernel.org/all/26a5d1b8-40b3-b1e4-bc85-740409c26838@arm.com/ > > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> Thanks for the review. > Do you know what state the iommu is left in after > rk_iommu_detach_device()? Ie is it blocking DMA or doing identity or > something else? To be honest I really don't know for sure. But from my small understanding of the code: rk_iommu_detach_device() ends up in rk_iommu_disable_paging() which appears to switch to identity mode ("Disable memory translation"). But I don't actually have any familiarity with the hardware block, so I might be missing something. Steve
On Wed, Mar 22, 2023 at 09:02:41AM +0000, Steven Price wrote: > On 21/03/2023 14:38, Jason Gunthorpe wrote: > > On Wed, Mar 15, 2023 at 04:41:52PM +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). > >> > >> Fixes: c1fe9119ee70 ("iommu: Add set_platform_dma_ops callbacks") > >> Signed-off-by: Steven Price <steven.price@arm.com> > >> --- > >> This fixes a splat I was seeing on a Firefly-RK3288, more details here: > >> https://lore.kernel.org/all/26a5d1b8-40b3-b1e4-bc85-740409c26838@arm.com/ > > > > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> > > Thanks for the review. > > > Do you know what state the iommu is left in after > > rk_iommu_detach_device()? Ie is it blocking DMA or doing identity or > > something else? > > To be honest I really don't know for sure. But from my small > understanding of the code: rk_iommu_detach_device() ends up in > rk_iommu_disable_paging() which appears to switch to identity mode > ("Disable memory translation"). Can you consider writing this patch like this instead: https://lore.kernel.org/linux-iommu/ZBnef7g7GCxogPNz@ziepe.ca/ ? I'd much rather we move toward clearly documenting what is going on with the HW and remove this undefined "detach" language. Jason
On 22/03/2023 12:50, Jason Gunthorpe wrote: > On Wed, Mar 22, 2023 at 09:02:41AM +0000, Steven Price wrote: >> On 21/03/2023 14:38, Jason Gunthorpe wrote: >>> On Wed, Mar 15, 2023 at 04:41:52PM +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). >>>> >>>> Fixes: c1fe9119ee70 ("iommu: Add set_platform_dma_ops callbacks") >>>> Signed-off-by: Steven Price <steven.price@arm.com> >>>> --- >>>> This fixes a splat I was seeing on a Firefly-RK3288, more details here: >>>> https://lore.kernel.org/all/26a5d1b8-40b3-b1e4-bc85-740409c26838@arm.com/ >>> >>> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> >> >> Thanks for the review. >> >>> Do you know what state the iommu is left in after >>> rk_iommu_detach_device()? Ie is it blocking DMA or doing identity or >>> something else? >> >> To be honest I really don't know for sure. But from my small >> understanding of the code: rk_iommu_detach_device() ends up in >> rk_iommu_disable_paging() which appears to switch to identity mode >> ("Disable memory translation"). > > Can you consider writing this patch like this instead: > > https://lore.kernel.org/linux-iommu/ZBnef7g7GCxogPNz@ziepe.ca/ > > ? > > I'd much rather we move toward clearly documenting what is going on > with the HW and remove this undefined "detach" language. I'm really not very familiar with the code or hardware, but I had a go at doing the same sort of conversion. The below successfully boots, but it would be good if someone more knowledgable could take a look as I can't say I really understand this code. Steve diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c index f30db22ea5d7..3fd108f04a2a 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,8 +1013,26 @@ 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; @@ -1049,8 +1070,6 @@ static int rk_iommu_attach_device(struct iommu_domain *domain, return 0; ret = rk_iommu_enable(iommu); - if (ret) - rk_iommu_detach_device(iommu->domain, dev); pm_runtime_put(iommu->dev); @@ -1061,6 +1080,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 +1198,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 +1211,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) {
On Wed, Mar 22, 2023 at 03:08:41PM +0000, Steven Price wrote: > @@ -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; > > @@ -1049,8 +1070,6 @@ static int rk_iommu_attach_device(struct iommu_domain *domain, > return 0; > > ret = rk_iommu_enable(iommu); > - if (ret) > - rk_iommu_detach_device(iommu->domain, dev); I think this still needs error handling, it should put it back to the identity domain and return an error code if it fails to attach to the requested domain. It should also initlaize iommu->domain to the identity domain when the iommu struct is allocated. The iommu->domain should never be NULL. identity domain means the IOMMU is turned off which was previously called "detached". Otherwise it looks like I would expect, thanks Jason
On 22/03/2023 15:16, Jason Gunthorpe wrote: > On Wed, Mar 22, 2023 at 03:08:41PM +0000, Steven Price wrote: >> @@ -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; >> >> @@ -1049,8 +1070,6 @@ static int rk_iommu_attach_device(struct iommu_domain *domain, >> return 0; >> >> ret = rk_iommu_enable(iommu); >> - if (ret) >> - rk_iommu_detach_device(iommu->domain, dev); > > I think this still needs error handling, it should put it back to the > identity domain and return an error code if it fails to attach to the > requested domain. What confused me here is that there's already a call to rk_iommu_identity_attach() just above. But I can obviously add a... if (ret) rk_iommu_identity_attach(&rk_identity_domain, dev); ... in here. But I don't know how to handle an error from rk_iommu_identity_attach() at this point. Does it need handling - is a WARN_ON sufficient? > It should also initlaize iommu->domain to the identity domain when the > iommu struct is allocated. The iommu->domain should never be > NULL. identity domain means the IOMMU is turned off which was > previously called "detached". I presume you mean in rk_iommu_probe()? > Otherwise it looks like I would expect, thanks Ok, I'll give it a spin with the above changes and post a v2 of this patch. Thanks, Steve
On Wed, Mar 22, 2023 at 04:04:25PM +0000, Steven Price wrote: > On 22/03/2023 15:16, Jason Gunthorpe wrote: > > On Wed, Mar 22, 2023 at 03:08:41PM +0000, Steven Price wrote: > >> @@ -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; > >> > >> @@ -1049,8 +1070,6 @@ static int rk_iommu_attach_device(struct iommu_domain *domain, > >> return 0; > >> > >> ret = rk_iommu_enable(iommu); > >> - if (ret) > >> - rk_iommu_detach_device(iommu->domain, dev); > > > > I think this still needs error handling, it should put it back to the > > identity domain and return an error code if it fails to attach to the > > requested domain. > > What confused me here is that there's already a call to > rk_iommu_identity_attach() just above. But I can obviously add a... I don't know this driver at all, but to me it looks like this is perhaps undoing a partially failed rk_iommu_enable() since it doesn't seem to enetirely fix itself. Ie it zeros the INT_MASK and DTE_ADDR Maybe it would be better to put that error cleanup direclty into enable and just move the iommu->domain assignment to after enable success. > if (ret) > rk_iommu_identity_attach(&rk_identity_domain, dev); > > ... in here. But I don't know how to handle an error from > rk_iommu_identity_attach() at this point. Does it need handling - is a > WARN_ON sufficient? WARN_ON should be fine, that is kind of hacky, it would be better to organize things so there is an identity attach function that cannot fail, ie pre-assumes all the validation is done alread.y > > > It should also initlaize iommu->domain to the identity domain when the > > iommu struct is allocated. The iommu->domain should never be > > NULL. identity domain means the IOMMU is turned off which was > > previously called "detached". > > I presume you mean in rk_iommu_probe()? It would be best if it was setup at allocation time so in rk_iommu_of_xlate() before dev_iommu_priv_set() Jason
On 22/03/2023 17:36, Jason Gunthorpe wrote: > On Wed, Mar 22, 2023 at 04:04:25PM +0000, Steven Price wrote: >> On 22/03/2023 15:16, Jason Gunthorpe wrote: >>> On Wed, Mar 22, 2023 at 03:08:41PM +0000, Steven Price wrote: >>>> @@ -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; >>>> >>>> @@ -1049,8 +1070,6 @@ static int rk_iommu_attach_device(struct iommu_domain *domain, >>>> return 0; >>>> >>>> ret = rk_iommu_enable(iommu); >>>> - if (ret) >>>> - rk_iommu_detach_device(iommu->domain, dev); >>> >>> I think this still needs error handling, it should put it back to the >>> identity domain and return an error code if it fails to attach to the >>> requested domain. >> >> What confused me here is that there's already a call to >> rk_iommu_identity_attach() just above. But I can obviously add a... > > I don't know this driver at all, but to me it looks like this is > perhaps undoing a partially failed rk_iommu_enable() since it doesn't > seem to enetirely fix itself. Ie it zeros the INT_MASK and DTE_ADDR > > Maybe it would be better to put that error cleanup direclty into > enable and just move the iommu->domain assignment to after enable > success. While I agree this would be better - I don't feel I understand the driver enough to have confidence in doing this. And I don't know how to trigger the error conditions to test this either. >> if (ret) >> rk_iommu_identity_attach(&rk_identity_domain, dev); >> >> ... in here. But I don't know how to handle an error from >> rk_iommu_identity_attach() at this point. Does it need handling - is a >> WARN_ON sufficient? > > WARN_ON should be fine, that is kind of hacky, it would be better to > organize things so there is an identity attach function that cannot > fail, ie pre-assumes all the validation is done alread.y As the code currently stands rk_iommu_identity_attach can fail for exactly one reason: if rk_iommu_from_dev() fails. And since that check is already done in rk_iommu_attach_device() this cannot fail (baring memory corruption etc). So I'll stick to WARN_ON for now. >> >>> It should also initlaize iommu->domain to the identity domain when the >>> iommu struct is allocated. The iommu->domain should never be >>> NULL. identity domain means the IOMMU is turned off which was >>> previously called "detached". >> >> I presume you mean in rk_iommu_probe()? > > It would be best if it was setup at allocation time so in > rk_iommu_of_xlate() before dev_iommu_priv_set() I've already put an assignment in rk_iommu_of_xlate() just before dev_iommu_priv_set(). Steve
diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c index f30db22ea5d7..312a3df19904 100644 --- a/drivers/iommu/rockchip-iommu.c +++ b/drivers/iommu/rockchip-iommu.c @@ -1183,6 +1183,12 @@ static int rk_iommu_of_xlate(struct device *dev, return 0; } +static void rk_iommu_set_platform_dma(struct device *dev) +{ + struct iommu_domain *domain = iommu_get_domain_for_dev(dev); + rk_iommu_detach_device(domain, dev); +} + static const struct iommu_ops rk_iommu_ops = { .domain_alloc = rk_iommu_domain_alloc, .probe_device = rk_iommu_probe_device, @@ -1190,6 +1196,7 @@ static const struct iommu_ops rk_iommu_ops = { .device_group = rk_iommu_device_group, .pgsize_bitmap = RK_IOMMU_PGSIZE_BITMAP, .of_xlate = rk_iommu_of_xlate, + .set_platform_dma_ops = rk_iommu_set_platform_dma, .default_domain_ops = &(const struct iommu_domain_ops) { .attach_dev = rk_iommu_attach_device, .map = rk_iommu_map,
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). Fixes: c1fe9119ee70 ("iommu: Add set_platform_dma_ops callbacks") Signed-off-by: Steven Price <steven.price@arm.com> --- This fixes a splat I was seeing on a Firefly-RK3288, more details here: https://lore.kernel.org/all/26a5d1b8-40b3-b1e4-bc85-740409c26838@arm.com/ drivers/iommu/rockchip-iommu.c | 7 +++++++ 1 file changed, 7 insertions(+)