[PATCHv7,4/6] arm: dma-mapping: add {map, unmap}_resource for iommu ops

Commit Message

Niklas Söderlund June 1, 2016, 3:22 p.m. UTC

Add methods to map/unmap device resources addresses for dma_map_ops that
are IOMMU aware. This is needed to map a device MMIO register from a
physical address.

Signed-off-by: Niklas Söderlund <[e-mail]>
Reviewed-by: Laurent Pinchart <[e-mail]>
---
 arch/arm/mm/dma-mapping.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

Comments

Russell King (Oracle) June 1, 2016, 4:16 p.m. UTC | #1

On Wed, Jun 01, 2016 at 05:22:27PM +0200, Niklas Söderlund wrote:
> +static dma_addr_t arm_iommu_map_resource(struct device *dev,
> +		phys_addr_t phys_addr, size_t size,
> +		enum dma_data_direction dir, struct dma_attrs *attrs)
> +{
> +	struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);
> +	dma_addr_t dma_addr;
> +	int ret, prot;
> +	phys_addr_t addr = phys_addr & PAGE_MASK;
> +	int offset = phys_addr & ~PAGE_MASK;
> +	int len = PAGE_ALIGN(size + offset);

Shouldn't both of these be unsigned - preferably size_t for len?

> +
> +	dma_addr = __alloc_iova(mapping, size);

Is this really correct?  What if size = 4095 and offset = 10?  Do we
really only need one IOVA page for such a mapping (I count two pages.)
Shouldn't this be "len" ?

> +	if (dma_addr == DMA_ERROR_CODE)
> +		return dma_addr;
> +
> +	prot = __dma_direction_to_prot(dir) | IOMMU_MMIO;
> +
> +	ret = iommu_map(mapping->domain, dma_addr, addr, len, prot);
> +	if (ret < 0)
> +		goto fail;
> +
> +	return dma_addr + offset;
> +fail:
> +	__free_iova(mapping, dma_addr, size);

Shouldn't this be "len" as well?

> +	return DMA_ERROR_CODE;
> +}
> +
> +/**
> + * arm_iommu_unmap_resource - unmap a device DMA resource
> + * @dev: valid struct device pointer
> + * @dma_handle: DMA address to resource
> + * @size: size of resource to map
> + * @dir: DMA transfer direction
> + */
> +static void arm_iommu_unmap_resource(struct device *dev, dma_addr_t dma_handle,
> +		size_t size, enum dma_data_direction dir,
> +		struct dma_attrs *attrs)
> +{
> +	struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);
> +	dma_addr_t iova = dma_handle & PAGE_MASK;
> +	int offset = dma_handle & ~PAGE_MASK;
> +	int len = PAGE_ALIGN(size + offset);

unsigned/size_t again.

> +
> +	if (!iova)
> +		return;
> +
> +	iommu_unmap(mapping->domain, iova, len);
> +	__free_iova(mapping, iova, len);

Here, you free "len" bytes of iova, which is different from above.

> +}
> +
>  static void arm_iommu_sync_single_for_cpu(struct device *dev,
>  		dma_addr_t handle, size_t size, enum dma_data_direction dir)
>  {
> @@ -1994,6 +2051,9 @@ struct dma_map_ops iommu_ops = {
>  	.unmap_sg		= arm_iommu_unmap_sg,
>  	.sync_sg_for_cpu	= arm_iommu_sync_sg_for_cpu,
>  	.sync_sg_for_device	= arm_iommu_sync_sg_for_device,
> +
> +	.map_resource		= arm_iommu_map_resource,
> +	.unmap_resource		= arm_iommu_unmap_resource,
>  };
>  
>  struct dma_map_ops iommu_coherent_ops = {
> @@ -2007,6 +2067,9 @@ struct dma_map_ops iommu_coherent_ops = {
>  
>  	.map_sg		= arm_coherent_iommu_map_sg,
>  	.unmap_sg	= arm_coherent_iommu_unmap_sg,
> +
> +	.map_resource	= arm_iommu_map_resource,
> +	.unmap_resource	= arm_iommu_unmap_resource,
>  };
>  
>  /**
> -- 
> 2.8.2
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Niklas Söderlund June 2, 2016, 12:50 p.m. UTC | #2

Hi Russell,

Thanks for your feedback.

On 2016-06-01 17:16:06 +0100, Russell King - ARM Linux wrote:
> On Wed, Jun 01, 2016 at 05:22:27PM +0200, Niklas Söderlund wrote:
> > +static dma_addr_t arm_iommu_map_resource(struct device *dev,
> > +		phys_addr_t phys_addr, size_t size,
> > +		enum dma_data_direction dir, struct dma_attrs *attrs)
> > +{
> > +	struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);
> > +	dma_addr_t dma_addr;
> > +	int ret, prot;
> > +	phys_addr_t addr = phys_addr & PAGE_MASK;
> > +	int offset = phys_addr & ~PAGE_MASK;
> > +	int len = PAGE_ALIGN(size + offset);
> 
> Shouldn't both of these be unsigned - preferably size_t for len?

I have looked at arm_coherent_iommu_map_page() when writing this where 
len is int. But I do agree that it should probably be size_t and offset 
should be unsigned. Will fix this.

> 
> > +
> > +	dma_addr = __alloc_iova(mapping, size);
> 
> Is this really correct?  What if size = 4095 and offset = 10?  Do we
> really only need one IOVA page for such a mapping (I count two pages.)
> Shouldn't this be "len" ?

Wops, you are correct it should be len not size.

> 
> > +	if (dma_addr == DMA_ERROR_CODE)
> > +		return dma_addr;
> > +
> > +	prot = __dma_direction_to_prot(dir) | IOMMU_MMIO;
> > +
> > +	ret = iommu_map(mapping->domain, dma_addr, addr, len, prot);
> > +	if (ret < 0)
> > +		goto fail;
> > +
> > +	return dma_addr + offset;
> > +fail:
> > +	__free_iova(mapping, dma_addr, size);
> 
> Shouldn't this be "len" as well?

Yes.

> 
> > +	return DMA_ERROR_CODE;
> > +}
> > +
> > +/**
> > + * arm_iommu_unmap_resource - unmap a device DMA resource
> > + * @dev: valid struct device pointer
> > + * @dma_handle: DMA address to resource
> > + * @size: size of resource to map
> > + * @dir: DMA transfer direction
> > + */
> > +static void arm_iommu_unmap_resource(struct device *dev, dma_addr_t dma_handle,
> > +		size_t size, enum dma_data_direction dir,
> > +		struct dma_attrs *attrs)
> > +{
> > +	struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);
> > +	dma_addr_t iova = dma_handle & PAGE_MASK;
> > +	int offset = dma_handle & ~PAGE_MASK;
> > +	int len = PAGE_ALIGN(size + offset);
> 
> unsigned/size_t again.

Will fix.

> 
> > +
> > +	if (!iova)
> > +		return;
> > +
> > +	iommu_unmap(mapping->domain, iova, len);
> > +	__free_iova(mapping, iova, len);
> 
> Here, you free "len" bytes of iova, which is different from above.

Yes you are correct. By using len instead of size in 
arm_iommu_map_resource() the sizes do match.

> 
> > +}
> > +
> >  static void arm_iommu_sync_single_for_cpu(struct device *dev,
> >  		dma_addr_t handle, size_t size, enum dma_data_direction dir)
> >  {
> > @@ -1994,6 +2051,9 @@ struct dma_map_ops iommu_ops = {
> >  	.unmap_sg		= arm_iommu_unmap_sg,
> >  	.sync_sg_for_cpu	= arm_iommu_sync_sg_for_cpu,
> >  	.sync_sg_for_device	= arm_iommu_sync_sg_for_device,
> > +
> > +	.map_resource		= arm_iommu_map_resource,
> > +	.unmap_resource		= arm_iommu_unmap_resource,
> >  };
> >  
> >  struct dma_map_ops iommu_coherent_ops = {
> > @@ -2007,6 +2067,9 @@ struct dma_map_ops iommu_coherent_ops = {
> >  
> >  	.map_sg		= arm_coherent_iommu_map_sg,
> >  	.unmap_sg	= arm_coherent_iommu_unmap_sg,
> > +
> > +	.map_resource	= arm_iommu_map_resource,
> > +	.unmap_resource	= arm_iommu_unmap_resource,
> >  };
> >  
> >  /**
> > -- 
> > 2.8.2
> > 
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > [e-mail]
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> -- 
> RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
> according to speedtest.net.

Patch

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index ff7ed56..8f12ec8 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -1951,6 +1951,63 @@  static void arm_iommu_unmap_page(struct device *dev, dma_addr_t handle,
 	__free_iova(mapping, iova, len);
 }
 
+/**
+ * arm_iommu_map_resource - map a device resource for DMA
+ * @dev: valid struct device pointer
+ * @phys_addr: physical address of resource
+ * @size: size of resource to map
+ * @dir: DMA transfer direction
+ */
+static dma_addr_t arm_iommu_map_resource(struct device *dev,
+		phys_addr_t phys_addr, size_t size,
+		enum dma_data_direction dir, struct dma_attrs *attrs)
+{
+	struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);
+	dma_addr_t dma_addr;
+	int ret, prot;
+	phys_addr_t addr = phys_addr & PAGE_MASK;
+	int offset = phys_addr & ~PAGE_MASK;
+	int len = PAGE_ALIGN(size + offset);
+
+	dma_addr = __alloc_iova(mapping, size);
+	if (dma_addr == DMA_ERROR_CODE)
+		return dma_addr;
+
+	prot = __dma_direction_to_prot(dir) | IOMMU_MMIO;
+
+	ret = iommu_map(mapping->domain, dma_addr, addr, len, prot);
+	if (ret < 0)
+		goto fail;
+
+	return dma_addr + offset;
+fail:
+	__free_iova(mapping, dma_addr, size);
+	return DMA_ERROR_CODE;
+}
+
+/**
+ * arm_iommu_unmap_resource - unmap a device DMA resource
+ * @dev: valid struct device pointer
+ * @dma_handle: DMA address to resource
+ * @size: size of resource to map
+ * @dir: DMA transfer direction
+ */
+static void arm_iommu_unmap_resource(struct device *dev, dma_addr_t dma_handle,
+		size_t size, enum dma_data_direction dir,
+		struct dma_attrs *attrs)
+{
+	struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);
+	dma_addr_t iova = dma_handle & PAGE_MASK;
+	int offset = dma_handle & ~PAGE_MASK;
+	int len = PAGE_ALIGN(size + offset);
+
+	if (!iova)
+		return;
+
+	iommu_unmap(mapping->domain, iova, len);
+	__free_iova(mapping, iova, len);
+}
+
 static void arm_iommu_sync_single_for_cpu(struct device *dev,
 		dma_addr_t handle, size_t size, enum dma_data_direction dir)
 {
@@ -1994,6 +2051,9 @@  struct dma_map_ops iommu_ops = {
 	.unmap_sg		= arm_iommu_unmap_sg,
 	.sync_sg_for_cpu	= arm_iommu_sync_sg_for_cpu,
 	.sync_sg_for_device	= arm_iommu_sync_sg_for_device,
+
+	.map_resource		= arm_iommu_map_resource,
+	.unmap_resource		= arm_iommu_unmap_resource,
 };
 
 struct dma_map_ops iommu_coherent_ops = {
@@ -2007,6 +2067,9 @@  struct dma_map_ops iommu_coherent_ops = {
 
 	.map_sg		= arm_coherent_iommu_map_sg,
 	.unmap_sg	= arm_coherent_iommu_unmap_sg,
+
+	.map_resource	= arm_iommu_map_resource,
+	.unmap_resource	= arm_iommu_unmap_resource,
 };
 
 /**