Re: [PATCH 4/9] ARM: dma-mapping: NULLify dev->archdata.mapping pointer on detach

Re: [PATCH 4/9] ARM: dma-mapping: NULLify dev->archdata.mapping pointer on detach

[Hiroshi Doyu]

Hi Will,

Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org> wrote @ Mon, 10 Jun 2013 20:34:40 +0200:

> The current code only clobbers a local variable, so the device is left
> with a stale mapping pointer.

True. This's my bad. Thanks.

Re: [PATCH 4/9] ARM: dma-mapping: NULLify dev->archdata.mapping pointer on detach

[Will Deacon]

On Tue, Jun 11, 2013 at 06:34:55AM +0100, Hiroshi Doyu wrote:
> Hi Will,
> 
> Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org> wrote @ Mon, 10 Jun 2013 20:34:40 +0200:
> 
> > The current code only clobbers a local variable, so the device is left
> > with a stale mapping pointer.
> 
> True. This's my bad. Thanks.

That's alright, it's easy to fix. Mind if I add your ack please?

Will

Re: [PATCH 4/9] ARM: dma-mapping: NULLify dev->archdata.mapping pointer on detach

[Hiroshi Doyu]

On Tue, 11 Jun 2013 10:50:15 +0200
Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org> wrote:

> On Tue, Jun 11, 2013 at 06:34:55AM +0100, Hiroshi Doyu wrote:
> > Hi Will,
> > 
> > Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org> wrote @ Mon, 10 Jun 2013 20:34:40 +0200:
> > 
> > > The current code only clobbers a local variable, so the device is left
> > > with a stale mapping pointer.
> > 
> > True. This's my bad. Thanks.
> 
> That's alright, it's easy to fix. Mind if I add your ack please?

Feel free to add: Acked-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Re: [PATCH 4/9] ARM: dma-mapping: NULLify dev->archdata.mapping pointer on detach

[Marek Szyprowski]

Hello,

On 6/11/2013 11:39 AM, Hiroshi Doyu wrote:
> On Tue, 11 Jun 2013 10:50:15 +0200
> Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org> wrote:
>
> > On Tue, Jun 11, 2013 at 06:34:55AM +0100, Hiroshi Doyu wrote:
> > > Hi Will,
> > >
> > > Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org> wrote @ Mon, 10 Jun 2013 20:34:40 +0200:
> > >
> > > > The current code only clobbers a local variable, so the device is left
> > > > with a stale mapping pointer.
> > >
> > > True. This's my bad. Thanks.
> >
> > That's alright, it's easy to fix. Mind if I add your ack please?
>
> Feel free to add: Acked-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Thanks for spotting the bug. I've taked the patch to my dma-mapping tree.

Best regards
-- 
Marek Szyprowski
Samsung R&D Institute Poland

Re: [PATCH 3/9] ARM: dma-mapping: convert DMA direction into IOMMU protection attributes

[Hiroshi Doyu]

Hi Will,

On Mon, 10 Jun 2013 20:34:39 +0200
Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org> wrote:
...
> @@ -1636,13 +1636,27 @@ static dma_addr_t arm_coherent_iommu_map_page(struct device *dev, struct page *p
>  {
>  	struct dma_iommu_mapping *mapping = dev->archdata.mapping;
>  	dma_addr_t dma_addr;
> -	int ret, len = PAGE_ALIGN(size + offset);
> +	int ret, prot, len = PAGE_ALIGN(size + offset);
>  
>  	dma_addr = __alloc_iova(mapping, len);
>  	if (dma_addr == DMA_ERROR_CODE)
>  		return dma_addr;
>  
> -	ret = iommu_map(mapping->domain, dma_addr, page_to_phys(page), len, 0);
> +	switch (dir) {
> +	case DMA_BIDIRECTIONAL:
> +		prot = IOMMU_READ | IOMMU_WRITE;
> +		break;
> +	case DMA_TO_DEVICE:
> +		prot = IOMMU_READ;
> +		break;
> +	case DMA_FROM_DEVICE:
> +		prot = IOMMU_WRITE;
> +		break;
> +	default:
> +		prot = 0;
> +	}
> +
> +	ret = iommu_map(mapping->domain, dma_addr, page_to_phys(page), len, prot);

Do we need similar changes for map_sg case as well? They still passes '0' as prot.

Re: [PATCH 3/9] ARM: dma-mapping: convert DMA direction into IOMMU protection attributes

[Will Deacon]

On Tue, Jun 25, 2013 at 11:12:15AM +0100, Hiroshi Doyu wrote:
> Hi Will,

Hi Hiroshi,

> On Mon, 10 Jun 2013 20:34:39 +0200
> Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org> wrote:
> ...
> > @@ -1636,13 +1636,27 @@ static dma_addr_t arm_coherent_iommu_map_page(struct device *dev, struct page *p
> >  {
> >  	struct dma_iommu_mapping *mapping = dev->archdata.mapping;
> >  	dma_addr_t dma_addr;
> > -	int ret, len = PAGE_ALIGN(size + offset);
> > +	int ret, prot, len = PAGE_ALIGN(size + offset);
> >  
> >  	dma_addr = __alloc_iova(mapping, len);
> >  	if (dma_addr == DMA_ERROR_CODE)
> >  		return dma_addr;
> >  
> > -	ret = iommu_map(mapping->domain, dma_addr, page_to_phys(page), len, 0);
> > +	switch (dir) {
> > +	case DMA_BIDIRECTIONAL:
> > +		prot = IOMMU_READ | IOMMU_WRITE;
> > +		break;
> > +	case DMA_TO_DEVICE:
> > +		prot = IOMMU_READ;
> > +		break;
> > +	case DMA_FROM_DEVICE:
> > +		prot = IOMMU_WRITE;
> > +		break;
> > +	default:
> > +		prot = 0;
> > +	}
> > +
> > +	ret = iommu_map(mapping->domain, dma_addr, page_to_phys(page), len, prot);
> 
> Do we need similar changes for map_sg case as well? They still passes '0' as prot.

Yes, we could use the same trick there (probably worth moving the logic into
a helper function for translating dma_data_direction into IOMMU_* values).

There are also iommu_map calls when allocating DMA buffers, but I think 0 is
the right thing to pass there (i.e. no permission until pages have been
explicitly mapped). Although, to be honest, I don't see why we need to map
the buffer at all when we allocate it.

Will

Re: [PATCH 3/9] ARM: dma-mapping: convert DMA direction into IOMMU protection attributes

[Hiroshi Doyu]

Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org> wrote @ Tue, 25 Jun 2013 13:37:14 +0200:
...
> > Do we need similar changes for map_sg case as well? They still passes '0' as prot.
> 
> Yes, we could use the same trick there (probably worth moving the logic into
> a helper function for translating dma_data_direction into IOMMU_* values).
> 
> There are also iommu_map calls when allocating DMA buffers, but I think 0 is
> the right thing to pass there (i.e. no permission until pages have been
> explicitly mapped). Although, to be honest, I don't see why we need to map
> the buffer at all when we allocate it.

Yes, I thought too. I have a patch for that as below. If you like,
I'll rebase and send for merge with the one which changes
dma-mapping.c.

From 699e6bd4fef86383d197775486b47bcbdc594f4a Mon Sep 17 00:00:00 2001
From: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Date: Tue, 25 Jun 2013 13:43:29 +0300
Subject: [PATCH 1/2] iommu/core: convert DMA direction into IOMMU protection
 attributes

Introduce a new function to convert DMA direction into IOMMU
protection attributes.

Signed-off-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 include/linux/iommu.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 00af00f..ce3be78 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -21,11 +21,26 @@
 
 #include <linux/errno.h>
 #include <linux/types.h>
+#include <linux/dma-direction.h>
 
 #define IOMMU_READ	(1)
 #define IOMMU_WRITE	(2)
 #define IOMMU_CACHE	(4) /* DMA cache coherency */
 
+static inline int to_iommu_prot(enum dma_data_direction dir)
+{
+	switch (dir) {
+	case DMA_BIDIRECTIONAL:
+		return IOMMU_READ | IOMMU_WRITE;
+	case DMA_TO_DEVICE:
+		return IOMMU_READ;
+	case DMA_FROM_DEVICE:
+		return IOMMU_WRITE;
+	default:
+		return 0;
+	}
+}
+
 struct iommu_ops;
 struct iommu_group;
 struct bus_type;
-- 
1.8.1.5

Re: [PATCH 3/9] ARM: dma-mapping: convert DMA direction into IOMMU protection attributes

[Will Deacon]

On Tue, Jun 25, 2013 at 12:52:26PM +0100, Hiroshi Doyu wrote:
> Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org> wrote @ Tue, 25 Jun 2013 13:37:14 +0200:
> ...
> > > Do we need similar changes for map_sg case as well? They still passes '0' as prot.
> > 
> > Yes, we could use the same trick there (probably worth moving the logic into
> > a helper function for translating dma_data_direction into IOMMU_* values).
> > 
> > There are also iommu_map calls when allocating DMA buffers, but I think 0 is
> > the right thing to pass there (i.e. no permission until pages have been
> > explicitly mapped). Although, to be honest, I don't see why we need to map
> > the buffer at all when we allocate it.
> 
> Yes, I thought too. I have a patch for that as below. If you like,
> I'll rebase and send for merge with the one which changes
> dma-mapping.c.

Yes, please send the series and I'll take a look. Marek's already picked up
my original patch, so it's better if you can base against a stable branch
from him.

Will