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

-- 
balbi