[PATCH 0/2] ARM SMMU fixes

[PATCH 0/2] ARM SMMU fixes

[Laurent Pinchart]

Hello Will,

I've studied your arm-smmu driver as a base to write a Renesas IOMMU driver
and found two small issues. Here are patches to fix them. Please bear with me
if my understanding was incorrect and the patches wrong :-)

Laurent Pinchart (2):
  iommu/arm-smmu: Replace list walk with platform driver data
  iommu/arm-smmu: Return 0 on unmap failure

 drivers/iommu/arm-smmu.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

-- 
Regards,

Laurent Pinchart

[PATCH 1/2] iommu/arm-smmu: Replace list walk with platform driver data

[Laurent Pinchart]

Instead of walking the list of registered SMMU devices at remove time to
locate the device being removed, set platform driver data at probe time
to point to the SMMU and retrieve the pointer at remove time.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/iommu/arm-smmu.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 1d9ab39..c33a310 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1912,6 +1912,8 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 	list_add(&smmu->list, &arm_smmu_devices);
 	spin_unlock(&arm_smmu_devices_lock);
 
+	platform_set_drvdata(pdev, smmu);
+
 	arm_smmu_device_reset(smmu);
 	return 0;
 
@@ -1937,22 +1939,13 @@ static int arm_smmu_device_remove(struct platform_device *pdev)
 {
 	int i;
 	struct device *dev = &pdev->dev;
-	struct arm_smmu_device *curr, *smmu = NULL;
+	struct arm_smmu_device *smmu = platform_get_drvdata(pdev);
 	struct rb_node *node;
 
 	spin_lock(&arm_smmu_devices_lock);
-	list_for_each_entry(curr, &arm_smmu_devices, list) {
-		if (curr->dev == dev) {
-			smmu = curr;
-			list_del(&smmu->list);
-			break;
-		}
-	}
+	list_del(&smmu->list);
 	spin_unlock(&arm_smmu_devices_lock);
 
-	if (!smmu)
-		return -ENODEV;
-
 	if (smmu->parent_of_node)
 		of_node_put(smmu->parent_of_node);
 
-- 
1.8.3.2

[PATCH 2/2] iommu/arm-smmu: Return 0 on unmap failure

[Laurent Pinchart]

The IOMMU core expects the unmap operation to return the number of bytes
that have been unmapped or 0 on failure, a negative return value being
treated like a number of bytes.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/iommu/arm-smmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index c33a310..e7528f0 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1464,7 +1464,7 @@ static size_t arm_smmu_unmap(struct iommu_domain *domain, unsigned long iova,
 
 	ret = arm_smmu_handle_mapping(smmu_domain, iova, 0, size, 0);
 	arm_smmu_tlb_inv_context(&smmu_domain->root_cfg);
-	return ret ? ret : size;
+	return ret ? 0 : size;
 }
 
 static phys_addr_t arm_smmu_iova_to_phys(struct iommu_domain *domain,
-- 
1.8.3.2

[PATCH 1/2] iommu/arm-smmu: Replace list walk with platform driver data

[Will Deacon]

Hi Laurent,

On Fri, Feb 28, 2014 at 03:37:09PM +0000, Laurent Pinchart wrote:
> Instead of walking the list of registered SMMU devices at remove time to
> locate the device being removed, set platform driver data at probe time
> to point to the SMMU and retrieve the pointer at remove time.

What does this gain us other than a slight code shrinkage? If we could get
rid of the arm_smmu_devices list, then I'd be more excited by this change!

Will

[PATCH 2/2] iommu/arm-smmu: Return 0 on unmap failure

[Will Deacon]

On Fri, Feb 28, 2014 at 03:37:10PM +0000, Laurent Pinchart wrote:
> The IOMMU core expects the unmap operation to return the number of bytes
> that have been unmapped or 0 on failure, a negative return value being
> treated like a number of bytes.

Makes sense, thanks. It's a pity we can't propagate the error code.

Will

> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  drivers/iommu/arm-smmu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index c33a310..e7528f0 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -1464,7 +1464,7 @@ static size_t arm_smmu_unmap(struct iommu_domain *domain, unsigned long iova,
>  
>  	ret = arm_smmu_handle_mapping(smmu_domain, iova, 0, size, 0);
>  	arm_smmu_tlb_inv_context(&smmu_domain->root_cfg);
> -	return ret ? ret : size;
> +	return ret ? 0 : size;
>  }
>  
>  static phys_addr_t arm_smmu_iova_to_phys(struct iommu_domain *domain,
> -- 
> 1.8.3.2
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

[PATCH 1/2] iommu/arm-smmu: Replace list walk with platform driver data

[Laurent Pinchart]

Hi Will,

On Friday 28 February 2014 16:38:37 Will Deacon wrote:
> Hi Laurent,
> 
> On Fri, Feb 28, 2014 at 03:37:09PM +0000, Laurent Pinchart wrote:
> > Instead of walking the list of registered SMMU devices at remove time to
> > locate the device being removed, set platform driver data at probe time
> > to point to the SMMU and retrieve the pointer at remove time.
> 
> What does this gain us other than a slight code shrinkage?

Just a slight code shrinkage. That's better than nothing, isn't it ? :-)

> If we could get rid of the arm_smmu_devices list, then I'd be more excited
> by this change!

Associating devices with IOMMUs is something that should be performed by the 
IOMMU core. I might give this a try, but I'll be busy with other tasks for the 
next couple of weeks.

-- 
Regards,

Laurent Pinchart

[PATCH 0/2] ARM SMMU fixes

[Laurent Pinchart]

Hi Will,

On Friday 28 February 2014 16:37:08 Laurent Pinchart wrote:
> Hello Will,
> 
> I've studied your arm-smmu driver as a base to write a Renesas IOMMU driver
> and found two small issues. Here are patches to fix them. Please bear with
> me if my understanding was incorrect and the patches wrong :-)
> 
> Laurent Pinchart (2):
>   iommu/arm-smmu: Replace list walk with platform driver data
>   iommu/arm-smmu: Return 0 on unmap failure
> 
>  drivers/iommu/arm-smmu.c | 17 +++++------------
>  1 file changed, 5 insertions(+), 12 deletions(-)

Do you plan to take these patches (or at least patch 2/2) in your tree ? I can 
send a pull request to Joerg if you give me your acked-by.

-- 
Regards,

Laurent Pinchart

[PATCH 0/2] ARM SMMU fixes

[Laurent Pinchart]

I've obviously forgotten that Will was away for a month. CC'ing Marc Zyngier.

On Thursday 03 April 2014 01:52:55 Laurent Pinchart wrote:
> On Friday 28 February 2014 16:37:08 Laurent Pinchart wrote:
> > Hello Will,
> > 
> > I've studied your arm-smmu driver as a base to write a Renesas IOMMU
> > driver and found two small issues. Here are patches to fix them. Please
> > bear with me if my understanding was incorrect and the patches wrong :-)
> > 
> > Laurent Pinchart (2):
> >   iommu/arm-smmu: Replace list walk with platform driver data
> >   iommu/arm-smmu: Return 0 on unmap failure
> >  
> >  drivers/iommu/arm-smmu.c | 17 +++++------------
> >  1 file changed, 5 insertions(+), 12 deletions(-)
> 
> Do you plan to take these patches (or at least patch 2/2) in your tree ? I
> can send a pull request to Joerg if you give me your acked-by.

Marc, would you like to handle this, or would you prefer to wait until Will 
comes back ?

-- 
Regards,

Laurent Pinchart

[PATCH 0/2] ARM SMMU fixes

[Marc Zyngier]

On 08/04/14 14:41, Laurent Pinchart wrote:
> I've obviously forgotten that Will was away for a month. CC'ing Marc Zyngier.
> 
> On Thursday 03 April 2014 01:52:55 Laurent Pinchart wrote:
>> On Friday 28 February 2014 16:37:08 Laurent Pinchart wrote:
>>> Hello Will,
>>>
>>> I've studied your arm-smmu driver as a base to write a Renesas IOMMU
>>> driver and found two small issues. Here are patches to fix them. Please
>>> bear with me if my understanding was incorrect and the patches wrong :-)
>>>
>>> Laurent Pinchart (2):
>>>   iommu/arm-smmu: Replace list walk with platform driver data
>>>   iommu/arm-smmu: Return 0 on unmap failure
>>>  
>>>  drivers/iommu/arm-smmu.c | 17 +++++------------
>>>  1 file changed, 5 insertions(+), 12 deletions(-)
>>
>> Do you plan to take these patches (or at least patch 2/2) in your tree ? I
>> can send a pull request to Joerg if you give me your acked-by.
> 
> Marc, would you like to handle this, or would you prefer to wait until Will 
> comes back ?

Hi Laurent,

Yup, I'll have a look and stash them in a temp tree. Given that Will
will be back in about a week, he will have the final say.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 0/2] ARM SMMU fixes

[Will Deacon]

On Tue, Apr 08, 2014 at 02:57:43PM +0100, Marc Zyngier wrote:
> On 08/04/14 14:41, Laurent Pinchart wrote:
> > I've obviously forgotten that Will was away for a month. CC'ing Marc Zyngier.
> > 
> > On Thursday 03 April 2014 01:52:55 Laurent Pinchart wrote:
> >> On Friday 28 February 2014 16:37:08 Laurent Pinchart wrote:
> >>> Hello Will,
> >>>
> >>> I've studied your arm-smmu driver as a base to write a Renesas IOMMU
> >>> driver and found two small issues. Here are patches to fix them. Please
> >>> bear with me if my understanding was incorrect and the patches wrong :-)
> >>>
> >>> Laurent Pinchart (2):
> >>>   iommu/arm-smmu: Replace list walk with platform driver data
> >>>   iommu/arm-smmu: Return 0 on unmap failure
> >>>  
> >>>  drivers/iommu/arm-smmu.c | 17 +++++------------
> >>>  1 file changed, 5 insertions(+), 12 deletions(-)
> >>
> >> Do you plan to take these patches (or at least patch 2/2) in your tree ? I
> >> can send a pull request to Joerg if you give me your acked-by.
> > 
> > Marc, would you like to handle this, or would you prefer to wait until Will 
> > comes back ?
> 
> Hi Laurent,
> 
> Yup, I'll have a look and stash them in a temp tree. Given that Will
> will be back in about a week, he will have the final say.

I've already got the fix queued ("Return 0 on unmap failure") and plan to
send it to Joerg this week. I think the other patch doesn't really add
anything to the driver :)

Will

[PATCH 0/2] ARM SMMU fixes

[Laurent Pinchart]

Hi Will,

On Monday 14 April 2014 17:58:58 Will Deacon wrote:
> On Tue, Apr 08, 2014 at 02:57:43PM +0100, Marc Zyngier wrote:
> > On 08/04/14 14:41, Laurent Pinchart wrote:
> > > I've obviously forgotten that Will was away for a month. CC'ing Marc
> > > Zyngier.
> > >
> > > On Thursday 03 April 2014 01:52:55 Laurent Pinchart wrote:
> > >> On Friday 28 February 2014 16:37:08 Laurent Pinchart wrote:
> > >>> Hello Will,
> > >>> 
> > >>> I've studied your arm-smmu driver as a base to write a Renesas IOMMU
> > >>> driver and found two small issues. Here are patches to fix them.
> > >>> Please bear with me if my understanding was incorrect and the patches
> > >>> wrong :-)
> > >>> 
> > >>> Laurent Pinchart (2):
> > >>>   iommu/arm-smmu: Replace list walk with platform driver data
> > >>>   iommu/arm-smmu: Return 0 on unmap failure
> > >>>  
> > >>>  drivers/iommu/arm-smmu.c | 17 +++++------------
> > >>>  1 file changed, 5 insertions(+), 12 deletions(-)
> > >> 
> > >> Do you plan to take these patches (or at least patch 2/2) in your tree
> > >> ? I can send a pull request to Joerg if you give me your acked-by.
> > > 
> > > Marc, would you like to handle this, or would you prefer to wait until
> > > Will comes back ?
> > 
> > Hi Laurent,
> > 
> > Yup, I'll have a look and stash them in a temp tree. Given that Will
> > will be back in about a week, he will have the final say.
> 
> I've already got the fix queued ("Return 0 on unmap failure") and plan to
> send it to Joerg this week.

Thank you.

> I think the other patch doesn't really add anything to the driver :)

Fair enough, it's your driver, so the decision is yours :-)

On a different but related topic, I've written an ipmmu-vmsa.c driver for a 
Renesas IOMMU. The IP core has custom registers but uses VMSA-compatible page 
tables. What would you think about sharing the page table management code 
between the two drivers ? The biggest difference between the two 
implementations is that I've hardcoded the long descriptor format, while you 
have reused more system MMU macros that make the arm-smmu driver use 2 or 3 
levels of page tables depending on whether LPAE is disabled or enabled.

-- 
Regards,

Laurent Pinchart

[PATCH 0/2] ARM SMMU fixes

[Will Deacon]

On Tue, Apr 15, 2014 at 04:55:07PM +0100, Laurent Pinchart wrote:
> Hi Will,

Hi Laurent,

> On a different but related topic, I've written an ipmmu-vmsa.c driver for a 
> Renesas IOMMU. The IP core has custom registers but uses VMSA-compatible page 
> tables. What would you think about sharing the page table management code 
> between the two drivers ? The biggest difference between the two 
> implementations is that I've hardcoded the long descriptor format, while you 
> have reused more system MMU macros that make the arm-smmu driver use 2 or 3 
> levels of page tables depending on whether LPAE is disabled or enabled.

Actually, my driver also only supports the long-descriptor format, so the
code should be shareable. Things I support that you might have issues with
are:

 - Stage-1 and Stage-2 formats (although long-descriptor only)
 - 4k and 64k pages (the latter for arm64 only)
 - Contiguous pte hints

Do you have support for section mappings in your driver? That is something
I'd be keen to add for the ARM SMMU, but it really complicates the code.

Will

[PATCH 0/2] ARM SMMU fixes

[Laurent Pinchart]

Hi Will,

On Wednesday 16 April 2014 15:22:06 Will Deacon wrote:
> On Tue, Apr 15, 2014 at 04:55:07PM +0100, Laurent Pinchart wrote:
> > Hi Will,
> 
> Hi Laurent,
> 
> > On a different but related topic, I've written an ipmmu-vmsa.c driver for
> > a Renesas IOMMU. The IP core has custom registers but uses VMSA-compatible
> > page tables. What would you think about sharing the page table management
> > code between the two drivers ? The biggest difference between the two
> > implementations is that I've hardcoded the long descriptor format, while
> > you have reused more system MMU macros that make the arm-smmu driver use
> > 2 or 3 levels of page tables depending on whether LPAE is disabled or
> > enabled.
>
> Actually, my driver also only supports the long-descriptor format, so the
> code should be shareable. Things I support that you might have issues with
> are:
> 
>  - Stage-1 and Stage-2 formats (although long-descriptor only)
>  - 4k and 64k pages (the latter for arm64 only)
>  - Contiguous pte hints
> 
> Do you have support for section mappings in your driver? That is something
> I'd be keen to add for the ARM SMMU, but it really complicates the code.

I have pending patches for that, I still need to clean them up. I'll try to 
post them in the next few days and I'll CC you. Discussing the implementation 
will be easier with the code, so I propose resuming this thread in response to 
the patches.

-- 
Regards,

Laurent Pinchart