Re: [PATCH 1/2] dmaengine: rcar-dmac: add iommu support for slave transfers

["Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>]

Hi Laurent,

* Laurent Pinchart <laurent.pinchart@ideasonboard.com> [2016-01-14 01:37:37 +0200]:

> Hi Niklas,
>
> Thank you for the patch, and welcome to the hairy details of the DMA mapping
> API :-)

Thanks and thank you for your feedback.

>
> On Monday 11 January 2016 03:17:46 Niklas Söderlund wrote:
> > Enable slave transfers to devices behind IPMMU:s by mapping the slave
> > addresses using the dma-mapping API.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/dma/sh/rcar-dmac.c | 64 ++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 60 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
> > index 7820d07..da94809 100644
> > --- a/drivers/dma/sh/rcar-dmac.c
> > +++ b/drivers/dma/sh/rcar-dmac.c
> > @@ -13,6 +13,7 @@
> >  #include <linux/dma-mapping.h>
> >  #include <linux/dmaengine.h>
> >  #include <linux/interrupt.h>
> > +#include <linux/iommu.h>
> >  #include <linux/list.h>
> >  #include <linux/module.h>
> >  #include <linux/mutex.h>
> > @@ -1101,6 +1102,24 @@ rcar_dmac_prep_dma_cyclic(struct dma_chan *chan,
> > dma_addr_t buf_addr, return desc;
> >  }
> >
> > +static dma_addr_t __rcar_dmac_dma_map(struct dma_chan *chan, phys_addr_t
> > addr,
> > +		size_t size, enum dma_data_direction dir)
> > +{
> > +	struct rcar_dmac_chan *rchan = to_rcar_dmac_chan(chan);
> > +	struct page *page = phys_to_page(addr);
>
> I wonder if that's really safe given that the physical address, not being part
> of RAM, is (unless I'm mistaken) not backed by a struct page.

I agree it's not how I wanted to do it but I could not figure out a way
that do not end up calling ops->map_page in dma-mapping-common.h. I did
also get a note from 'kbuild test robot' that phys_to_page is not
available on all platforms. So in my v2 of this series I currently use:

    page = pfn_to_page(addr >> PAGE_SHIFT);

But I guess it's just as safe as phys_to_page.

>
> > +	size_t offset = addr - page_to_phys(page);
> > +	dma_addr_t map = dma_map_page(chan->device->dev, page, offset, size,
> > +			dir);
>
> You might want to use an _attrs() version of the call to pass the
> DMA_ATTR_NO_KERNEL_MAPPING and DMA_ATTR_SKIP_CPU_SYNC flags. Unfortunately
> there's no dma_map_page_attrs(), maybe it should be added.

I have implemented and are using such a construct for my v2. I also
found the following patch but I could not determine if the idea of of a
dma_map_pag_attrs was accepted or rejected.

PATCH v1 1/2] dma-mapping-common: add dma_map_page_attrs API
https://www.spinics.net/lists/linux-arch/msg32334.html

>
> > +
> > +	if (dma_mapping_error(chan->device->dev, map)) {
> > +		dev_err(chan->device->dev, "chan%u: failed to map %zx@%pap",
> > +				rchan->index, size, &addr);
> > +		return 0;
> > +	}
> > +
> > +	return map;
> > +}
> > +
> >  static int rcar_dmac_device_config(struct dma_chan *chan,
> >  				   struct dma_slave_config *cfg)
> >  {
> > @@ -1110,10 +1129,47 @@ static int rcar_dmac_device_config(struct dma_chan
> > *chan, * We could lock this, but you shouldn't be configuring the
> >  	 * channel, while using it...
> >  	 */
> > -	rchan->src_slave_addr = cfg->src_addr;
> > -	rchan->dst_slave_addr = cfg->dst_addr;
> > -	rchan->src_xfer_size = cfg->src_addr_width;
> > -	rchan->dst_xfer_size = cfg->dst_addr_width;
> > +
> > +	/* If we don't have a iommu domain no idea to trying to use it */
> > +	if (!iommu_get_domain_for_dev(chan->device->dev)) {
> > +		rchan->src_slave_addr = cfg->src_addr;
> > +		rchan->dst_slave_addr = cfg->dst_addr;
> > +		rchan->src_xfer_size = cfg->src_addr_width;
> > +		rchan->dst_xfer_size = cfg->dst_addr_width;
> > +		return 0;
> > +	}
>
> Driver are not supposed to deal with the IOMMU API directly. Would it be an
> issue dropping this check ? The dma_map_page() call should work fine without
> an IOMMU and return a DMA address identical to the physical address. Unless
> the memory is not DMA-ble, in which case bounce buffers would be used, and
> possible a few other corner cases. I'm not sure if we need to care about them.
>

You are correct, this check can be removed. It was needed in a earlier
version. I will wait a few days and see if it becomes clearer if the
mapping should happen in the dmaengine or in the client. And depending
on that outcome I will send out an updated version.


> > +	/* unmap old */
> > +	if (rchan->src_slave_addr) {
> > +		dma_unmap_page(chan->device->dev, rchan->src_slave_addr,
> > +				rchan->src_xfer_size, DMA_FROM_DEVICE);
> > +		rchan->src_slave_addr = 0;
> > +		rchan->src_xfer_size = 0;
> > +	}
> > +
> > +	if (rchan->dst_slave_addr) {
> > +		dma_unmap_page(chan->device->dev, rchan->dst_slave_addr,
> > +				rchan->dst_xfer_size, DMA_TO_DEVICE);
> > +		rchan->dst_slave_addr = 0;
> > +		rchan->dst_xfer_size = 0;
> > +	}
> > +
> > +	/* map new */
> > +	if (cfg->src_addr) {
> > +		rchan->src_slave_addr = __rcar_dmac_dma_map(chan, cfg->src_addr,
> > +				cfg->src_addr_width, DMA_FROM_DEVICE);
> > +		if (!rchan->src_slave_addr)
> > +			return -EIO;
> > +		rchan->src_xfer_size = cfg->src_addr_width;
> > +	}
> > +
> > +	if (cfg->dst_addr) {
> > +		rchan->dst_slave_addr = __rcar_dmac_dma_map(chan, cfg->dst_addr,
> > +				cfg->dst_addr_width, DMA_TO_DEVICE);
> > +		if (!rchan->dst_slave_addr)
> > +			return -EIO;
> > +		rchan->dst_xfer_size = cfg->dst_addr_width;
> > +	}
> >
> >  	return 0;
> >  }
--
// Niklas