RE: [RFC][PATCH 0/2] Avoiding DWC3 transfer stalls/hangs when using adb over f_fs

From: Felipe Balbi <balbi@kernel.org>
To: "Yang\, Fei" <fei.yang@intel.com>,
	Andrzej Pietrasiewicz <andrzej.p@collabora.com>,
	John Stultz <john.stultz@linaro.org>,
	lkml <linux-kernel@vger.kernel.org>
Cc: Felipe Balbi <felipe.balbi@linux.intel.com>,
	Thinh Nguyen <thinhn@synopsys.com>,
	Tejas Joglekar <tejas.joglekar@synopsys.com>,
	Jack Pham <jackp@codeaurora.org>, Todd Kjos <tkjos@google.com>,
	Greg KH <gregkh@linuxfoundation.org>,
	Linux USB List <linux-usb@vger.kernel.org>,
	stable <stable@vger.kernel.org>
Subject: RE: [RFC][PATCH 0/2] Avoiding DWC3 transfer stalls/hangs when using adb over f_fs
Date: Thu, 23 Jan 2020 19:46:51 +0200	[thread overview]
Message-ID: <87lfpy3w1g.fsf@kernel.org> (raw)
In-Reply-To: <02E7334B1630744CBDC55DA8586225837F9EE335@ORSMSX102.amr.corp.intel.com>

[-- Attachment #1: Type: text/plain, Size: 1993 bytes --]

Hi,

"Yang, Fei" <fei.yang@intel.com> writes:
>>>>> Hey all,
>>>>>    I wanted to send these out for comment and thoughts.
>>>>> 
>>>>> Since ~4.20, when the functionfs gadget enabled scatter-gather 
>>>>> support, we have seen problems with adb connections stalling and 
>>>>> stopping to function on hardware with dwc3 usb controllers.
>>>>> Specifically, HiKey960, Dragonboard 845c, and Pixel3 devices.
>>>>
>>>> Any chance this:
>>>> 
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git/commit/
>>>> ?h=testing/next&id=f63333e8e4fd63d8d8ae83b89d2c38cf21d64801
>>> This is a different issue. I have tried initializing num_sgs when debugging this adb stall problem, but it didn't help.
>>
>> So multiple folks have run through this problem, but not *one* has tracepoints collected from the issue? C'mon guys. Can someone, please, collect tracepoints so we can figure out what's actually going on?
>>
>> 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.

> I can try dig into my old emails and resend, but that is a bit hard to find.

Don't bother, I'm still not convinced we should fix at the driver level
when sg_is_last() should be working here, unless we should iterate over
num_sgs instead of num_mapped_sgs, though I don't think that's the case
since in that case we would have to chain buffers of size zero.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]