[PATCH] iommu/rockchip: Add missing set_platform_dma_ops callback

[PATCH] iommu/rockchip: Add missing set_platform_dma_ops callback

[Steven Price]

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(+)

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,
-- 
2.34.1

Re: [PATCH] iommu/rockchip: Add missing set_platform_dma_ops callback

[Jason Gunthorpe]

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

Re: [PATCH] iommu/rockchip: Add missing set_platform_dma_ops callback

[Steven Price]

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

Re: [PATCH] iommu/rockchip: Add missing set_platform_dma_ops callback

[Jason Gunthorpe]

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

Re: [PATCH] iommu/rockchip: Add missing set_platform_dma_ops callback

[Steven Price]

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) {

Re: [PATCH] iommu/rockchip: Add missing set_platform_dma_ops callback

[Jason Gunthorpe]

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

Re: [PATCH] iommu/rockchip: Add missing set_platform_dma_ops callback

[Steven Price]

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

Re: [PATCH] iommu/rockchip: Add missing set_platform_dma_ops callback

[Jason Gunthorpe]

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

Re: [PATCH] iommu/rockchip: Add missing set_platform_dma_ops callback

[Steven Price]

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