Hi,
Christoph Hellwig hch@infradead.org writes:
On Wed, Feb 05, 2020 at 01:03:51PM -0800, John Stultz wrote:
On Thu, Jan 23, 2020 at 9:46 AM Felipe Balbi balbi@kernel.org wrote:
I'm pretty sure this should be solved at the DMA API level, just want to confirm.
I have sent you the tracepoints long time ago. Also my analysis of the problem (BTW, I don't think the tracepoints helped much). It's basically a logic problem in function dwc3_gadget_ep_reclaim_trb_sg().
AFAICT, this is caused by DMA API merging pages together when map an sglist for DMA. While doing that, it does *not* move the SG_END flag which sg_is_last() checks.
I consider that an overlook on the DMA API, wouldn't you? Why should DMA API users care if pages were merged or not while mapping the sglist? We have for_each_sg() and sg_is_last() for a reason.
From an initial look, I agree this is pretty confusing. dma_map_sg()
can coalesce entries in the sg list, modifying the sg entires themselves, however, in doing so it doesn't modify the number of entries in the sglist (nor the end state bit). That's pretty subtle!
dma_map_sg only coalesces the dma address. The page, offset and len members are immutable.
ok
The problem is really the design of the scatterlist structure - it combines immutable input parameters (page, offset, len) and output parameters (dma_addr, dma_len) in one data structure, and then needs different accessors depending on which information you care about. The end marker only works for the "CPU" view.
right
The right fix is top stop using struct scatterlist, but that is going to be larger and painful change. At least for block layer stuff I plan to incrementally do that, though.
I don't think that would be necessary though.
So I'm not sure that sg_is_last() is really valid for iterating on mapped sg lists.
Should it be? Probably (at least with my unfamiliar eyes), but sg_is_last() has been around for almost as long coexisting with this behavioral quirk, so I'm also not sure this is the best hill for the dwc3 driver to die on. :)
No, it shoudn't. dma_map_sg returns the number of mapped segments, and the callers need to remember that.
We _do_ remember that:
unsigned int remaining = req->request.num_mapped_sgs - req->num_queued_sgs;
for_each_sg(sg, s, remaining, i) { unsigned int length = req->request.length; unsigned int maxp = usb_endpoint_maxp(dep->endpoint.desc); unsigned int rem = length % maxp; unsigned chain = true;
if (sg_is_last(s)) chain = false;
if (rem && usb_endpoint_dir_out(dep->endpoint.desc) && !chain) {
that req->request.num_mapped_sgs is the returned value. So you're saying we should test for i == num_mapped_sgs, instead of using sg_is_last(). Is that it?
Fair enough. Just out of curiosity, then, when *should* we use sg_is_last()?
cheers