Hi,
Christoph Hellwig <hch@xxxxxxxxxxxxx> 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@xxxxxxxxxx> 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
Attachment:
signature.asc
Description: PGP signature
