On Fri, Jan 04, 2019 at 11:49:09AM -0700, Jason Gunthorpe wrote:
> 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.
This looks reasonable. I wonder if we should use a different
struct type for the dma iter so that we get better type checking?
Also this seems to miss vmwgfx, the other user of the interface.
> +#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
I'd rather not introduce ifdefs here and always have the DMA iter
code.
