On Fri, Jan 04, 2019 at 12:22:10PM -0600, Shiraz Saleem wrote:
> On Fri, Jan 04, 2019 at 09:16:13AM -0700, Jason Gunthorpe wrote:
> > On Fri, Jan 04, 2019 at 08:22:13AM +0100, Christoph Hellwig wrote:
> > > On Fri, Jan 04, 2019 at 08:18:52AM +0100, Christoph Hellwig wrote:
> > > > > CH: Does sg_page_iter_dma_address() even make any sense??
> > > >
> > > > No, it doesn't and should never have been edited. It's one of these
> > > > typical scatterlist abuses in the media code :(
> > >
> > > Actually I take this back. Due to the sg_page_iter structure iterating
> > > over segments first and then pages inside the segment it actually
> > > is ok. It just looks very awkward.
> >
> > I'm looking at __sg_page_iter_next which does this:
> >
> > static int sg_page_count(struct scatterlist *sg)
> > {
> > return PAGE_ALIGN(sg->offset + sg->length) >> PAGE_SHIFT;
> > }
> >
> > bool __sg_page_iter_next(struct sg_page_iter *piter)
> > {
> > [..]
> > while (piter->sg_pgoffset >= sg_page_count(piter->sg)) {
> > piter->sg_pgoffset -= sg_page_count(piter->sg);
> >
> > Which isn't going to yield the right math if
> > sg->length != sg_dma_len(sg)
> >
> > Surely we need a version of sg_page_iter_next() that uses sg_dma_len
> > to compute page_count if the caller intents to call
> > sg_page_iter_dma_address() ?
>
> /*
> * sg page iterator
> *
> * Iterates over sg entries page-by-page. On each successful iteration,
> * you can call sg_page_iter_page(@piter) and sg_page_iter_dma_address(@piter)
> * to get the current page and its dma address. @piter->sg will point to the
> * sg holding this page and @piter->sg_pgoffset to the page's page offset
> * within the sg. The iteration will stop either when a maximum number of sg
> * entries was reached or a terminating sg (sg_last(sg) == true) was reached.
> */
> struct sg_page_iter {
> struct scatterlist *sg; /* sg holding the page */
> unsigned int sg_pgoffset; /* page offset within the sg */
> [..]
>
> If the intent of the page iterator was to grab the cur page and dma address,
> then shouldnt sg_page_count just be updated to use sg_dma_len(sg)?
The page iterator is used widely in places that have nothing to do
with DMA, it needs to keep working as is: iterating over the CPU
struct pages, not the DMA physical addresses.
This is what I was thinking of doing..
>From cb4a9f568e9bffe1f6baf3d70e6fb9cfff21525f Mon Sep 17 00:00:00 2001
From: Jason Gunthorpe <jgg@xxxxxxxxxxxx>
Date: Fri, 4 Jan 2019 11:40:21 -0700
Subject: [PATCH] lib/scatterlist: Provide a DMA page iterator
Commit 2db76d7c3c6d ("lib/scatterlist: sg_page_iter: support sg lists w/o
backing pages") introduced the sg_page_iter_dma_address() function without
providing a way to use it in the general case. If the sg_dma_len is not
equal to the dma_length callers cannot safely use the
for_each_sg_page/sg_page_iter_dma_address combination.
Resolve this API mistake by providing a for_each_sg_dma_page() iterator
that uses the right length so sg_page_iter_dma_address() works as expected
in all cases.
Users cannot mix DMA and non-DMA accessors within the same iteration.
Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxxxx>
---
drivers/media/pci/intel/ipu3/ipu3-cio2.c | 2 +-
include/linux/scatterlist.h | 37 ++++++++++++++++++++----
lib/scatterlist.c | 24 +++++++++++++++
3 files changed, 56 insertions(+), 7 deletions(-)
diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
index 447baaebca4486..a6f9bdfe9e46ed 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
@@ -873,7 +873,7 @@ static int cio2_vb2_buf_init(struct vb2_buffer *vb)
b->offset = sg->sgl->offset;
i = j = 0;
- for_each_sg_page(sg->sgl, &sg_iter, sg->nents, 0) {
+ for_each_sg_dma_page(sg->sgl, &sg_iter, sg->nents, 0) {
if (!pages--)
break;
b->lop[i][j] = sg_page_iter_dma_address(&sg_iter) >> PAGE_SHIFT;
diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
index 093aa57120b0cf..8b64e59752ce6b 100644
--- a/include/linux/scatterlist.h
+++ b/include/linux/scatterlist.h
@@ -339,12 +339,15 @@ int sg_alloc_table_chained(struct sg_table *table, int nents,
/*
* sg page iterator
*
- * Iterates over sg entries page-by-page. On each successful iteration,
- * you can call sg_page_iter_page(@piter) and sg_page_iter_dma_address(@piter)
- * to get the current page and its dma address. @piter->sg will point to the
- * sg holding this page and @piter->sg_pgoffset to the page's page offset
- * within the sg. The iteration will stop either when a maximum number of sg
- * entries was reached or a terminating sg (sg_last(sg) == true) was reached.
+ * Iterates over sg entries page-by-page. When for_each_sg_page() is used
+ * each successful iteration can call sg_page_iter_page(@piter) to get the
+ * current page, while when using for_each_sg_dma_page() iterations can call
+ * sg_page_iter_dma_address(@piter) to get the current dma address
+ *
+ * @piter->sg will point to the sg holding this page and @piter->sg_pgoffset
+ * to the page's page offset within the sg. The iteration will stop either
+ * when a maximum number of sg entries was reached or a terminating sg
+ * (sg_last(sg) == true) was reached.
*/
struct sg_page_iter {
struct scatterlist *sg; /* sg holding the page */
@@ -357,6 +360,7 @@ struct sg_page_iter {
};
bool __sg_page_iter_next(struct sg_page_iter *piter);
+bool __sg_page_iter_dma_next(struct sg_page_iter *piter);
void __sg_page_iter_start(struct sg_page_iter *piter,
struct scatterlist *sglist, unsigned int nents,
unsigned long pgoffset);
@@ -385,11 +389,32 @@ static inline dma_addr_t sg_page_iter_dma_address(struct sg_page_iter *piter)
* @piter: page iterator to hold current page, sg, sg_pgoffset
* @nents: maximum number of sg entries to iterate over
* @pgoffset: starting page offset
+ *
+ * Callers may use sg_page_iter_page() to get each page pointer.
*/
#define for_each_sg_page(sglist, piter, nents, pgoffset) \
for (__sg_page_iter_start((piter), (sglist), (nents), (pgoffset)); \
__sg_page_iter_next(piter);)
+/**
+ * for_each_sg_dma_page - iterate over the pages of the given sg list
+ * @sglist: sglist to iterate over
+ * @piter: page iterator to hold current page, sg, sg_pgoffset
+ * @dma_nents: maximum number of sg entries to iterate over, this is the value
+ * returned from dma_map_sg
+ * @pgoffset: starting page offset
+ *
+ * Callers may use sg_page_iter_dma_address() to get each page's DMA address.
+ */
+#ifdef CONFIG_NEED_SG_DMA_LENGTH
+#define for_each_sg_dma_page(sglist, piter, dma_nents, pgoffset) \
+ for (__sg_page_iter_start(piter, sglist, dma_nents, pgoffset); \
+ __sg_page_iter_dma_next(piter);)
+#else
+#define for_each_sg_dma_page(sglist, piter, dma_nents, pgoffset) \
+ for_each_sg_page(sglist, piter, dma_nents, pgoffest)
+#endif
+
/*
* Mapping sg iterator
*
diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index 7c6096a7170486..fc6e224869ad65 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -625,6 +625,30 @@ bool __sg_page_iter_next(struct sg_page_iter *piter)
}
EXPORT_SYMBOL(__sg_page_iter_next);
+static int sg_dma_page_count(struct scatterlist *sg)
+{
+ return PAGE_ALIGN(sg->offset + sg_dma_len(sg)) >> PAGE_SHIFT;
+}
+
+bool __sg_page_iter_dma_next(struct sg_page_iter *piter)
+{
+ if (!piter->__nents || !piter->sg)
+ return false;
+
+ piter->sg_pgoffset += piter->__pg_advance;
+ piter->__pg_advance = 1;
+
+ while (piter->sg_pgoffset >= sg_dma_page_count(piter->sg)) {
+ piter->sg_pgoffset -= sg_dma_page_count(piter->sg);
+ piter->sg = sg_next(piter->sg);
+ if (!--piter->__nents || !piter->sg)
+ return false;
+ }
+
+ return true;
+}
+EXPORT_SYMBOL(__sg_page_iter_next);
+
/**
* sg_miter_start - start mapping iteration over a sg list
* @miter: sg mapping iter to be started
--
2.20.1
