On 03/10/17 23:16, David Woodhouse wrote: > On Tue, 2017-10-03 at 19:05 +0100, Robin Murphy wrote: >> >> Now, there are indeed plenty of drivers and subsystems which do work on >> lists of explicitly single pages - anything doing some variant of >> "addr = kmap_atomic(sg_page(sg)) + sg->offset;" is easy to spot - but I >> don't think DMA API implementations are in a position to make any kind >> of assumption; nearly all of them just shut up and handle sg->length >> bytes from sg_phys(sg) without questioning the caller, and I reckon >> that's exactly what they should be doing. > > So what's the point in sg->page in the first place? If even the > *offset* can be greater than page size, it isn't even the *first* page > (as you called it). Why aren't we just using a physical address, > instead of an arbitrary page and an offset from that? To nitpick, "the first of one or more contiguous pages" does not imply "the first page of the buffer" - the buffer just lies *somewhere* within that run of pages. Historically, it looks like page+offset first came about in the 2.5 era to support highmem[1] - note that scatterlist users still only really care about virtual and DMA address, not physical. Sure, it wouldn't be entirely impossible to use phys_addr_t instead, but at this point it would be a massively invasive change, and would make implementations of one DMA API callback a tiny bit simpler at the cost of pushing rather more complexity out to hundreds of other users, some of whom might not even call dma_map_sg(). The fact is, regardless of how much sense it does or doesn't make, a fair amount of scatterlist-mangling code exists that is capable of generating silly offsets, and it's so simple to make sure the DMA API implementations don't care that I'd rather just keep on top of that than go off on a crusade in attempt to wipe out the grey areas. > Can we have *negative* sg->offset too? :) unsigned int offset; I'm gonna say no ;) Robin. [1]:https://git.kernel.org/pub/scm/linux/kernel/git/history/history.git/tree/include/asm-i386/scatterlist.h?h=v2.5.0
