[PATCH 0/4] scatterlist: sg_table from virtual pointer

[PATCH 0/4] scatterlist: sg_table from virtual pointer

[Boris Brezillon]

Hello,

This series has been extracted from another series [1] adding support
for DMA operations in a NAND driver.

The reason I decided to post those patches separately is because they
are touching core stuff, and I'd like to have feedback on these specific
aspects.

The idea is to provide a generic function creating an sg_table from
a virtual pointer and a length. This operation is complicated by
the different memory regions exposed in kernel space. For example,
you have the lowmem region, which guarantees that buffers are
physically contiguous, while the vmalloc region does not.

sg_alloc_table_from_buf() detects in which memory region your buffer
reside, and takes the appropriate precautions when creating the
sg_table. This function also takes an extract parameter, allowing
one to specify extra constraints, like the maximum DMA segment size,
the required and the preferred alignment.

Patch 1 and 2 are implementing sg_alloc_table_from_buf() (patch 1
is needed to properly detect buffers residing in the highmem/kmap
area).

Patch 3 is making use of sg_alloc_table_from_buf() in the spi_map_buf()
function (hopefully, other subsystems/drivers will be able to easily
switch to this function too).

Patch 4 is implementing what I really need: generic functions
to map/unmap a virtual buffer passed through mtd->_read/_write().

I'm not exactly a DMA or MM experts, so that would be great to have
feedbacks on this approach. That's why I added so many people in Cc
even if they're not directly impacted by those patches. Let me know if
you want me to drop/add people from/to the recipient list.

Thanks.

Best Regards,

Boris

[1]http://www.spinics.net/lists/arm-kernel/msg493552.html

Boris Brezillon (4):
  mm: add is_highmem_addr() helper
  scatterlist: add sg_alloc_table_from_buf() helper
  spi: use sg_alloc_table_from_buf()
  mtd: provide helper to prepare buffers for DMA operations

 drivers/mtd/mtdcore.c       |  66 ++++++++++++++++
 drivers/spi/spi.c           |  45 ++---------
 include/linux/highmem.h     |  13 ++++
 include/linux/mtd/mtd.h     |  25 ++++++
 include/linux/scatterlist.h |  24 ++++++
 lib/scatterlist.c           | 183 ++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 316 insertions(+), 40 deletions(-)

-- 
2.5.0

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 1/4] mm: add is_highmem_addr() helper

[Boris Brezillon]

Add an helper to check if a virtual address is in the highmem region.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 include/linux/highmem.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/include/linux/highmem.h b/include/linux/highmem.h
index bb3f329..13dff37 100644
--- a/include/linux/highmem.h
+++ b/include/linux/highmem.h
@@ -41,6 +41,14 @@ void kmap_flush_unused(void);
 
 struct page *kmap_to_page(void *addr);
 
+static inline bool is_highmem_addr(const void *x)
+{
+	unsigned long vaddr = (unsigned long)x;
+
+	return vaddr >=  PKMAP_BASE &&
+	       vaddr < ((PKMAP_BASE + LAST_PKMAP) * PAGE_SIZE);
+}
+
 #else /* CONFIG_HIGHMEM */
 
 static inline unsigned int nr_free_highpages(void) { return 0; }
@@ -50,6 +58,11 @@ static inline struct page *kmap_to_page(void *addr)
 	return virt_to_page(addr);
 }
 
+static inline bool is_highmem_addr(const void *x)
+{
+	return false;
+}
+
 #define totalhigh_pages 0UL
 
 #ifndef ARCH_HAS_KMAP
-- 
2.5.0

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/4] mm: add is_highmem_addr() helper

[Vignesh R]

Hi,

On 03/31/2016 05:59 PM, Boris Brezillon wrote:
> Add an helper to check if a virtual address is in the highmem region.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
>  include/linux/highmem.h | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/include/linux/highmem.h b/include/linux/highmem.h
> index bb3f329..13dff37 100644
> --- a/include/linux/highmem.h
> +++ b/include/linux/highmem.h
> @@ -41,6 +41,14 @@ void kmap_flush_unused(void);
>  
>  struct page *kmap_to_page(void *addr);
>  
> +static inline bool is_highmem_addr(const void *x)
> +{
> +	unsigned long vaddr = (unsigned long)x;
> +
> +	return vaddr >=  PKMAP_BASE &&
> +	       vaddr < ((PKMAP_BASE + LAST_PKMAP) * PAGE_SIZE);


Shouldn't this be:
		vaddr < (PKMAP_BASE + (LAST_PKMAP * PAGE_SIZE)) ?

-- 
Regards
Vignesh

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/4] mm: add is_highmem_addr() helper

[Boris Brezillon]

On Mon, 4 Apr 2016 13:44:11 +0530
Vignesh R <vigneshr-l0cyMroinI0@public.gmane.org> wrote:

> Hi,
> 
> On 03/31/2016 05:59 PM, Boris Brezillon wrote:
> > Add an helper to check if a virtual address is in the highmem region.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > ---
> >  include/linux/highmem.h | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/include/linux/highmem.h b/include/linux/highmem.h
> > index bb3f329..13dff37 100644
> > --- a/include/linux/highmem.h
> > +++ b/include/linux/highmem.h
> > @@ -41,6 +41,14 @@ void kmap_flush_unused(void);
> >  
> >  struct page *kmap_to_page(void *addr);
> >  
> > +static inline bool is_highmem_addr(const void *x)
> > +{
> > +	unsigned long vaddr = (unsigned long)x;
> > +
> > +	return vaddr >=  PKMAP_BASE &&
> > +	       vaddr < ((PKMAP_BASE + LAST_PKMAP) * PAGE_SIZE);
> 
> 
> Shouldn't this be:
> 		vaddr < (PKMAP_BASE + (LAST_PKMAP * PAGE_SIZE)) ?

Oops, yes indeed.

Anyway, given Russell's feedback I don't think I'm gonna follow up on
this series.

Sorry.

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/4] scatterlist: add sg_alloc_table_from_buf() helper

[Boris Brezillon]

sg_alloc_table_from_buf() provides an easy solution to create an sg_table
from a virtual address pointer. This function takes care of dealing with
vmallocated buffers, buffer alignment, or DMA engine limitations (maximum
DMA transfer size).

Signed-off-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 include/linux/scatterlist.h |  24 ++++++
 lib/scatterlist.c           | 183 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 207 insertions(+)

diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
index 556ec1e..18d1091 100644
--- a/include/linux/scatterlist.h
+++ b/include/linux/scatterlist.h
@@ -41,6 +41,27 @@ struct sg_table {
 	unsigned int orig_nents;	/* original size of list */
 };
 
+/**
+ * struct sg_constraints - SG constraints structure
+ *
+ * @max_segment_size: maximum segment length. Each SG entry has to be smaller
+ *		      than this value. Zero means no constraint.
+ * @required_alignment: minimum alignment. Is used for both size and pointer
+ *			alignment. If this constraint is not met, the function
+ *			should return -EINVAL.
+ * @preferred_alignment: preferred alignment. Mainly used to optimize
+ *			 throughput when the DMA engine performs better when
+ *			 doing aligned accesses.
+ *
+ * This structure is here to help sg_alloc_table_from_buf() create the optimal
+ * SG list based on DMA engine constraints.
+ */
+struct sg_constraints {
+	size_t max_segment_size;
+	size_t required_alignment;
+	size_t preferred_alignment;
+};
+
 /*
  * Notes on SG table design.
  *
@@ -265,6 +286,9 @@ int sg_alloc_table_from_pages(struct sg_table *sgt,
 	struct page **pages, unsigned int n_pages,
 	unsigned long offset, unsigned long size,
 	gfp_t gfp_mask);
+int sg_alloc_table_from_buf(struct sg_table *sgt, const void *buf, size_t len,
+			    const struct sg_constraints *constraints,
+			    gfp_t gfp_mask);
 
 size_t sg_copy_buffer(struct scatterlist *sgl, unsigned int nents, void *buf,
 		      size_t buflen, off_t skip, bool to_buffer);
diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index 004fc70..9c9746e 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -433,6 +433,189 @@ int sg_alloc_table_from_pages(struct sg_table *sgt,
 }
 EXPORT_SYMBOL(sg_alloc_table_from_pages);
 
+static size_t sg_buf_chunk_len(const void *buf, size_t len,
+			       const struct sg_constraints *cons)
+{
+	size_t chunk_len = len;
+
+	if (cons->max_segment_size)
+		chunk_len = min_t(size_t, chunk_len, cons->max_segment_size);
+
+	if (is_vmalloc_addr(buf)) {
+		unsigned long offset_in_page = offset_in_page(buf);
+		size_t contig_len = PAGE_SIZE - offset_in_page;
+		unsigned long phys = vmalloc_to_pfn(buf) - offset_in_page;
+		const void *contig_ptr = buf + contig_len;
+
+		/*
+		 * Vmalloced buffer might be composed of several physically
+		 * contiguous pages. Avoid extra scattergather entries in
+		 * this case.
+		 */
+		while (contig_len < chunk_len) {
+			if (phys + PAGE_SIZE != vmalloc_to_pfn(contig_ptr))
+				break;
+
+			contig_len += PAGE_SIZE;
+			contig_ptr += PAGE_SIZE;
+			phys += PAGE_SIZE;
+		}
+
+		chunk_len = min_t(size_t, chunk_len, contig_len);
+	}
+
+	if (!IS_ALIGNED((unsigned long)buf, cons->preferred_alignment)) {
+		const void *aligned_buf = PTR_ALIGN(buf,
+						    cons->preferred_alignment);
+		size_t unaligned_len = (unsigned long)(aligned_buf - buf);
+
+		chunk_len = min_t(size_t, chunk_len, unaligned_len);
+	} else if (chunk_len > cons->preferred_alignment) {
+		chunk_len &= ~(cons->preferred_alignment - 1);
+	}
+
+	return chunk_len;
+}
+
+#define sg_for_each_chunk_in_buf(buf, len, chunk_len, constraints)	\
+	for (chunk_len = sg_buf_chunk_len(buf, len, constraints);	\
+	     len;							\
+	     len -= chunk_len, buf += chunk_len,			\
+	     chunk_len = sg_buf_chunk_len(buf, len, constraints))
+
+static int sg_check_constraints(struct sg_constraints *cons,
+				const void *buf, size_t len)
+{
+	/*
+	 * We only accept buffers coming from the lowmem, vmalloc and
+	 * highmem regions.
+	 */
+	if (!virt_addr_valid(buf) && !is_vmalloc_addr(buf) &&
+	    !is_highmem_addr(buf))
+		return -EINVAL;
+
+	if (!cons->required_alignment)
+		cons->required_alignment = 1;
+
+	if (!cons->preferred_alignment)
+		cons->preferred_alignment = cons->required_alignment;
+
+	/* Test if buf and len are properly aligned. */
+	if (!IS_ALIGNED((unsigned long)buf, cons->required_alignment) ||
+	    !IS_ALIGNED(len, cons->required_alignment))
+		return -EINVAL;
+
+	/*
+	 * if the buffer has been vmallocated or kmapped and required_alignment
+	 * is more than PAGE_SIZE we cannot guarantee it.
+	 */
+	if (!virt_addr_valid(buf) && cons->required_alignment > PAGE_SIZE)
+		return -EINVAL;
+
+	/*
+	 * max_segment_size has to be aligned to required_alignment to
+	 * guarantee that all buffer chunks are aligned correctly.
+	 */
+	if (!IS_ALIGNED(cons->max_segment_size, cons->required_alignment))
+		return -EINVAL;
+
+	/*
+	 * preferred_alignment has to be aligned to required_alignment
+	 * to avoid misalignment of buffer chunks.
+	 */
+	if (!IS_ALIGNED(cons->preferred_alignment, cons->required_alignment))
+		return -EINVAL;
+
+	return 0;
+}
+
+/**
+ * sg_alloc_table_from_buf - create an SG table from a buffer
+ *
+ * @sgt: SG table
+ * @buf: buffer you want to create this SG table from
+ * @len: length of buf
+ * @constraints: optional constraints to take into account when creating
+ *		 the SG table. Can be NULL if no specific constraints are
+ *		 required.
+ * @gfp_mask: type of allocation to use when creating the table
+ *
+ * This function creates an SG table from a buffer, its length and some
+ * SG constraints.
+ *
+ * Note: This function supports buffers coming from the lowmem, vmalloc or
+ * highmem region.
+ */
+int sg_alloc_table_from_buf(struct sg_table *sgt, const void *buf, size_t len,
+			    const struct sg_constraints *constraints,
+			    gfp_t gfp_mask)
+{
+	struct sg_constraints cons = { };
+	size_t remaining, chunk_len;
+	const void *sg_buf;
+	int i, ret;
+
+	if (constraints)
+		cons = *constraints;
+
+	ret = sg_check_constraints(&cons, buf, len);
+	if (ret)
+		return ret;
+
+	sg_buf = buf;
+	remaining = len;
+	i = 0;
+	sg_for_each_chunk_in_buf(sg_buf, remaining, chunk_len, &cons)
+		i++;
+
+	ret = sg_alloc_table(sgt, i, gfp_mask);
+	if (ret)
+		return ret;
+
+	sg_buf = buf;
+	remaining = len;
+	i = 0;
+	sg_for_each_chunk_in_buf(sg_buf, remaining, chunk_len, &cons) {
+		if (virt_addr_valid(buf)) {
+			/*
+			 * Buffer is in lowmem, we can safely call
+			 * sg_set_buf().
+			 */
+			sg_set_buf(&sgt->sgl[i], sg_buf, chunk_len);
+		} else {
+			struct page *vm_page;
+
+			/*
+			 * Buffer has been obtained with vmalloc() or kmap().
+			 * In this case we have to extract the page information
+			 * and use sg_set_page().
+			 */
+			if (is_vmalloc_addr(sg_buf))
+				vm_page = vmalloc_to_page(sg_buf);
+			else
+				vm_page = kmap_to_page((void *)sg_buf);
+
+			if (!vm_page) {
+				ret = -ENOMEM;
+				goto err_free_table;
+			}
+
+			sg_set_page(&sgt->sgl[i], vm_page, chunk_len,
+				    offset_in_page(sg_buf));
+		}
+
+		i++;
+	}
+
+	return 0;
+
+err_free_table:
+	sg_free_table(sgt);
+
+	return ret;
+}
+EXPORT_SYMBOL(sg_alloc_table_from_buf);
+
 void __sg_page_iter_start(struct sg_page_iter *piter,
 			  struct scatterlist *sglist, unsigned int nents,
 			  unsigned long pgoffset)
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] scatterlist: add sg_alloc_table_from_buf() helper

[Russell King - ARM Linux]

On Thu, Mar 31, 2016 at 02:29:42PM +0200, Boris Brezillon wrote:
> sg_alloc_table_from_buf() provides an easy solution to create an sg_table
> from a virtual address pointer. This function takes care of dealing with
> vmallocated buffers, buffer alignment, or DMA engine limitations (maximum
> DMA transfer size).

Please note that the DMA API does not take account of coherency of memory
regions other than non-high/lowmem - there are specific extensions to
deal with this.

What this means is that having an API that takes any virtual address
pointer, converts it to a scatterlist which is then DMA mapped, is
unsafe.

It'll be okay for PIPT and non-aliasing VIPT cache architectures, but
for other cache architectures this will hide this problem and make
review harder.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 2/4] scatterlist: add sg_alloc_table_from_buf() helper

[Boris Brezillon]

Hi Russell,

On Thu, 31 Mar 2016 15:14:13 +0100
Russell King - ARM Linux <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org> wrote:

> On Thu, Mar 31, 2016 at 02:29:42PM +0200, Boris Brezillon wrote:
> > sg_alloc_table_from_buf() provides an easy solution to create an sg_table
> > from a virtual address pointer. This function takes care of dealing with
> > vmallocated buffers, buffer alignment, or DMA engine limitations (maximum
> > DMA transfer size).
> 
> Please note that the DMA API does not take account of coherency of memory
> regions other than non-high/lowmem - there are specific extensions to
> deal with this.

Ok, you said 'non-high/lowmem', this means vmalloced and kmapped buffers
already fall in this case, right?

Could you tell me more about those specific extensions?

> 
> What this means is that having an API that takes any virtual address
> pointer, converts it to a scatterlist which is then DMA mapped, is
> unsafe.

Which means some implementations already get this wrong (see
spi_map_buf(), and I'm pretty sure it's not the only one).

> 
> It'll be okay for PIPT and non-aliasing VIPT cache architectures, but
> for other cache architectures this will hide this problem and make
> review harder.
> 

Ok, you lost me. I'll have to do my homework and try to understand what
this means :).

Thanks for your valuable inputs.

Best Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] scatterlist: add sg_alloc_table_from_buf() helper

[Russell King - ARM Linux]

On Thu, Mar 31, 2016 at 04:45:57PM +0200, Boris Brezillon wrote:
> Hi Russell,
> 
> On Thu, 31 Mar 2016 15:14:13 +0100
> Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> 
> > On Thu, Mar 31, 2016 at 02:29:42PM +0200, Boris Brezillon wrote:
> > > sg_alloc_table_from_buf() provides an easy solution to create an sg_table
> > > from a virtual address pointer. This function takes care of dealing with
> > > vmallocated buffers, buffer alignment, or DMA engine limitations (maximum
> > > DMA transfer size).
> > 
> > Please note that the DMA API does not take account of coherency of memory
> > regions other than non-high/lowmem - there are specific extensions to
> > deal with this.
> 
> Ok, you said 'non-high/lowmem', this means vmalloced and kmapped buffers
> already fall in this case, right?
> 
> Could you tell me more about those specific extensions?

I was slightly confused - the extensions I was thinking of are those
listed at the bottom of Documentation/cachetlb.txt, which have nothing
to do with DMA.

However, it's probably worth reading Documentation/DMA-API-HOWTO.txt
to read up on what kinds of memory are considered to be DMA-able in
the kernel.

> > What this means is that having an API that takes any virtual address
> > pointer, converts it to a scatterlist which is then DMA mapped, is
> > unsafe.
> 
> Which means some implementations already get this wrong (see
> spi_map_buf(), and I'm pretty sure it's not the only one).

Quite possible, but that is driver stuff, and driver stuff gets things
wrong all the time. :)

> > It'll be okay for PIPT and non-aliasing VIPT cache architectures, but
> > for other cache architectures this will hide this problem and make
> > review harder.
> > 
> 
> Ok, you lost me. I'll have to do my homework and try to understand what
> this means :).

P = physical address
V = virtual address
I = indexed
T = tag

The tag is held in each cache line.  When a location is looked up in the
cache, an index is used to locate a set of cache lines and the tag is
compared to check which cache line in the set is the correct one (or
whether the address even exists in the cache.)

How the index and tag are derived varies between cache architectures.

PIPT = indexed by physical address, tagged with physical address.  Never
aliases with itself in the presence of multiple virtual mappings.

VIPT = indexed by virtual address, tagged with physical address.  If the
bits from the virtual address do not overlap the MMU page size, it is
also alias free, otherwise aliases can exist, but can be eliminated by
"cache colouring" - ensuring that a physical address is always mapped
with the same overlapping bits.

VIVT = indexed by virtual address, tagged with virtual address.  The
worst kind of cache, since every different mapping of the same physical
address is guaranteed by design to alias with other mappings.

There is little cache colouring between different kernel mappings (eg,
between lowmem and vmalloc space.)

What this means is that, while the DMA API takes care of DMA aliases
in the lowmem mappings, an alias-prone VIPT cache will remain incoherent
with DMA if it is remapped into vmalloc space, and the mapping happens
to have a different cache colour.  In other words, this is a data
corruption issue.

Hence, taking a range of vmalloc() addresses, converting them into a
scatterlist, then using the DMA API on the scatterlist _only_ guarantees
that the lowmem (and kmap'd highmem mappings) are coherent with DMA.
There is no way for the DMA API to know that other mappings exist, and
obviously flushing every possible cache line just because a mapping might
exist multiplies the expense of the DMA API: not only in terms of time
spent running through all the possibilities, which doubles for every
aliasing bit of VIPT, but also TLB pressure since you'd have to create
a mapping for each alias and tear it back down.

VIVT is even worse, since there is no other virtual mapping which is
coherent, would need to be known, and each mapping would need to be
individually flushed.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 3/4] spi: use sg_alloc_table_from_buf()

[Boris Brezillon]

Replace custom implementation of sg_alloc_table_from_buf() by a call to
sg_alloc_table_from_buf().

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/spi/spi.c | 45 +++++----------------------------------------
 1 file changed, 5 insertions(+), 40 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index de2f2f9..eed461d 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -705,49 +705,14 @@ static int spi_map_buf(struct spi_master *master, struct device *dev,
 		       struct sg_table *sgt, void *buf, size_t len,
 		       enum dma_data_direction dir)
 {
-	const bool vmalloced_buf = is_vmalloc_addr(buf);
-	unsigned int max_seg_size = dma_get_max_seg_size(dev);
-	int desc_len;
-	int sgs;
-	struct page *vm_page;
-	void *sg_buf;
-	size_t min;
-	int i, ret;
-
-	if (vmalloced_buf) {
-		desc_len = min_t(int, max_seg_size, PAGE_SIZE);
-		sgs = DIV_ROUND_UP(len + offset_in_page(buf), desc_len);
-	} else {
-		desc_len = min_t(int, max_seg_size, master->max_dma_len);
-		sgs = DIV_ROUND_UP(len, desc_len);
-	}
+	struct sg_constraints constraints = { };
+	int ret;
 
-	ret = sg_alloc_table(sgt, sgs, GFP_KERNEL);
-	if (ret != 0)
+	constraints.max_segment_size = dma_get_max_seg_size(dev);
+	ret = sg_alloc_table_from_buf(sgt, buf, len, &constraints, GFP_KERNEL);
+	if (ret)
 		return ret;
 
-	for (i = 0; i < sgs; i++) {
-
-		if (vmalloced_buf) {
-			min = min_t(size_t,
-				    len, desc_len - offset_in_page(buf));
-			vm_page = vmalloc_to_page(buf);
-			if (!vm_page) {
-				sg_free_table(sgt);
-				return -ENOMEM;
-			}
-			sg_set_page(&sgt->sgl[i], vm_page,
-				    min, offset_in_page(buf));
-		} else {
-			min = min_t(size_t, len, desc_len);
-			sg_buf = buf;
-			sg_set_buf(&sgt->sgl[i], sg_buf, min);
-		}
-
-		buf += min;
-		len -= min;
-	}
-
 	ret = dma_map_sg(dev, sgt->sgl, sgt->nents, dir);
 	if (!ret)
 		ret = -ENOMEM;
-- 
2.5.0

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 3/4] spi: use sg_alloc_table_from_buf()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 240 bytes --]

On Thu, Mar 31, 2016 at 02:29:43PM +0200, Boris Brezillon wrote:
> Replace custom implementation of sg_alloc_table_from_buf() by a call to
> sg_alloc_table_from_buf().

Acked-by: Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH 4/4] mtd: provide helper to prepare buffers for DMA operations

[Boris Brezillon]

Some NAND controller drivers are making use of DMA to transfer data from
the controller to the buffer passed by the MTD user.
Provide a generic mtd_map/unmap_buf() implementation to avoid open coded
(and sometime erroneous) implementations.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/mtd/mtdcore.c   | 66 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mtd/mtd.h | 25 +++++++++++++++++++
 2 files changed, 91 insertions(+)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 3096251..4c20f33 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -1253,6 +1253,72 @@ void *mtd_kmalloc_up_to(const struct mtd_info *mtd, size_t *size)
 }
 EXPORT_SYMBOL_GPL(mtd_kmalloc_up_to);
 
+#ifdef CONFIG_HAS_DMA
+/**
+ * mtd_map_buf - create an SG table and prepare it for DMA operations
+ *
+ * @mtd: mtd device description object pointer
+ * @dev: device handling the DMA operation
+ * @buf: buf used to create the SG table
+ * @len: length of buf
+ * @constraints: optional constraints to take into account when creating
+ *		 the SG table. Can be NULL if no specific constraints
+ *		 are required.
+ * @dir: direction of the DMA operation
+ *
+ * This function should be used when an MTD driver wants to do DMA operations
+ * on a buffer passed by the MTD layer. This functions takes care of
+ * vmallocated buffer constraints, and return and sg_table that you can safely
+ * use.
+ */
+int mtd_map_buf(struct mtd_info *mtd, struct device *dev,
+		struct sg_table *sgt, const void *buf, size_t len,
+		const struct sg_constraints *constraints,
+		enum dma_data_direction dir)
+{
+	int ret;
+
+	ret = sg_alloc_table_from_buf(sgt, buf, len, constraints, GFP_KERNEL);
+	if (ret)
+		return ret;
+
+	ret = dma_map_sg(dev, sgt->sgl, sgt->nents, dir);
+	if (!ret)
+		ret = -ENOMEM;
+
+	if (ret < 0) {
+		sg_free_table(sgt);
+		return ret;
+	}
+
+	sgt->nents = ret;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(mtd_map_buf);
+
+/**
+ * mtd_unmap_buf - unmap an SG table and release its resources
+ *
+ * @mtd: mtd device description object pointer
+ * @dev: device handling the DMA operation
+ * @sgt: SG table
+ * @dir: direction of the DMA operation
+ *
+ * This function unmaps a previously mapped SG table and release SG table
+ * resources. Should be called when your DMA operation is done.
+ */
+void mtd_unmap_buf(struct mtd_info *mtd, struct device *dev,
+		   struct sg_table *sgt, enum dma_data_direction dir)
+{
+	if (sgt->orig_nents) {
+		dma_unmap_sg(dev, sgt->sgl, sgt->orig_nents, dir);
+		sg_free_table(sgt);
+	}
+}
+EXPORT_SYMBOL_GPL(mtd_unmap_buf);
+#endif /* !CONFIG_HAS_DMA */
+
 #ifdef CONFIG_PROC_FS
 
 /*====================================================================*/
diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index 7712721..15cff85 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -24,6 +24,7 @@
 #include <linux/uio.h>
 #include <linux/notifier.h>
 #include <linux/device.h>
+#include <linux/dma-mapping.h>
 
 #include <mtd/mtd-abi.h>
 
@@ -410,6 +411,30 @@ extern void register_mtd_user (struct mtd_notifier *new);
 extern int unregister_mtd_user (struct mtd_notifier *old);
 void *mtd_kmalloc_up_to(const struct mtd_info *mtd, size_t *size);
 
+#ifdef CONFIG_HAS_DMA
+int mtd_map_buf(struct mtd_info *mtd, struct device *dev,
+		struct sg_table *sgt, const void *buf, size_t len,
+		const struct sg_constraints *constraints,
+		enum dma_data_direction dir);
+void mtd_unmap_buf(struct mtd_info *mtd, struct device *dev,
+		   struct sg_table *sgt, enum dma_data_direction dir);
+#else
+static inline int mtd_map_buf(struct mtd_info *mtd, struct device *dev,
+			      struct sg_table *sgt, const void *buf,
+			      size_t len,
+			      const struct sg_constraints *constraints
+			      enum dma_data_direction dir)
+{
+	return -ENOTSUPP;
+}
+
+static void mtd_unmap_buf(struct mtd_info *mtd, struct device *dev,
+			  struct sg_table *sgt, enum dma_data_direction dir)
+{
+	return -ENOTSUPP;
+}
+#endif
+
 void mtd_erase_callback(struct erase_info *instr);
 
 static inline int mtd_is_bitflip(int err) {
-- 
2.5.0

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 4/4] mtd: provide helper to prepare buffers for DMA operations

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2848 bytes --]

Hi Boris,

[auto build test ERROR on spi/for-next]
[also build test ERROR on v4.6-rc1 next-20160331]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Boris-Brezillon/scatterlist-sg_table-from-virtual-pointer/20160331-203118
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi for-next
config: m32r-m32104ut_defconfig (attached as .config)
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=m32r 

All error/warnings (new ones prefixed by >>):

   In file included from include/linux/mtd/super.h:17:0,
                    from fs/romfs/storage.c:13:
>> include/linux/mtd/mtd.h:426:10: error: expected ';', ',' or ')' before 'enum'
             enum dma_data_direction dir)
             ^
   include/linux/mtd/mtd.h: In function 'mtd_unmap_buf':
>> include/linux/mtd/mtd.h:434:2: warning: 'return' with a value, in function returning void
     return -ENOTSUPP;
     ^
   fs/romfs/storage.c: At top level:
   include/linux/mtd/mtd.h:431:13: warning: 'mtd_unmap_buf' defined but not used [-Wunused-function]
    static void mtd_unmap_buf(struct mtd_info *mtd, struct device *dev,
                ^
--
   In file included from include/linux/mtd/super.h:17:0,
                    from fs/romfs/super.c:72:
>> include/linux/mtd/mtd.h:426:10: error: expected ';', ',' or ')' before 'enum'
             enum dma_data_direction dir)
             ^
   include/linux/mtd/mtd.h: In function 'mtd_unmap_buf':
>> include/linux/mtd/mtd.h:434:2: warning: 'return' with a value, in function returning void
     return -ENOTSUPP;
     ^
   fs/romfs/super.c: At top level:
   include/linux/mtd/mtd.h:431:13: warning: 'mtd_unmap_buf' defined but not used [-Wunused-function]
    static void mtd_unmap_buf(struct mtd_info *mtd, struct device *dev,
                ^

vim +426 include/linux/mtd/mtd.h

   420			   struct sg_table *sgt, enum dma_data_direction dir);
   421	#else
   422	static inline int mtd_map_buf(struct mtd_info *mtd, struct device *dev,
   423				      struct sg_table *sgt, const void *buf,
   424				      size_t len,
   425				      const struct sg_constraints *constraints
 > 426				      enum dma_data_direction dir)
   427	{
   428		return -ENOTSUPP;
   429	}
   430	
   431	static void mtd_unmap_buf(struct mtd_info *mtd, struct device *dev,
   432				  struct sg_table *sgt, enum dma_data_direction dir)
   433	{
 > 434		return -ENOTSUPP;
   435	}
   436	#endif
   437	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 10416 bytes --]