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)?
> I assume it works for media because they have arranged things so that
> sg_dma_len == sg->length, but in the general case with dma_map_sg that
> can't be assumed.
>
