Re: [PATCH 2/3] usb: dwc3: gadget: Fix early exit in set/clear ep halt

[Felipe Balbi <balbi@kernel.org>]

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


Hi,

Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>>> Felipe Balbi <balbi@kernel.org> writes:
>>>>> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>>>>> This patch fixes a commit that causes a hang from device waiting for
>>>>>> data with the wrong sequence number. The commit ffb80fc672c3 ("usb:
>>>>>> dwc3: gadget: skip Set/Clear Halt when invalid") adds a check to return
>>>>>> early depending on DWC3_EP_STALL is set or not, prevent sending the ep
>>>>>> halt command to HW endpoint to do CLEAR_FEATURE(ENDPOINT_HALT) request.
>>>>>> This was to workaround the issue for macOS where the device hangs from
>>>>>> sending DWC3 clear stall command.
>>>>>>
>>>>>> In USB 3.1 spec, 9.4.5, CLEAR_FEATURE(ENDPOINT_HALT) request always
>>>>>> results in the data sequence being reinitialized to zero regardless
>>>>>> whether the endpoint has been halted or not. Some device class depends
>>>>>> on this feature for its protocol. For instance, in mass storage class,
>>>>>> there is MSC reset protocol that does CLEAR_FEATURE(ENDPOINT_HALT) on
>>>>>> bulk endpoints. This protocol reinitializes the data sequence and
>>>>>> ensures that whatever pending data requested from previous CBW will be
>>>>>> reset. Otherwise this will cause a hang as the device can wait for the
>>>>>> data with the wrong sequence number from the previous CBW. We found this
>>>>>> failure in USB CV: MSC Error Recovery Test with f_mass_storage.
>>>>>>
>>>>>> This patch fixes this issue by checking to see whether the set/halt ep
>>>>>> call is a protocol call before early exit to make sure that set/clear
>>>>>> halt endpoint command can go through if it is a device class protocol.
>>>>>>
>>>>>> Fixes: ffb80fc672c3 ("usb: dwc3: gadget: skip Set/Clear Halt when invalid")
>>>>>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
>>>>>
>>>>> this will regress the macOS case I wrote that commit for. We need to
>>>>
>>>> no wait, it won't regress macOS, but we're still left with the problem
>>>> of host and peripheral being able to get DataToggle/SeqN out of sync.
>>>>
>>>
>>> This patch is for the regression we have. Can you provide more
>>> information for the macOS? I'm not sure if this is the case for macOS,
>> 
>> I need to find a way to reproduce it again first. When I first
>> reproduced it was with dwc3 running adb and connecting it to a macOS
>> machine.
>> 
>>> but maybe there is still pending transfer when it tries to send the
>>> request? (There shouldn't be any before issuing ClearStall command). Do
>> 
>> this could be, I don't remember if I checked this or not :-)
>> 
>> Really, the best way here, IMHO, would be to re-verify what's going on
>> with macOS and revert my orignal patch since it's, rather clearly,
>> wrong.
>> 
>
> Sure. Are you going to make a revert patch or I am?

Well, after we really know what's going on with macOS and have a better
fix, then who makes the revert is less important as long as problems get
sorted out :-) Either way is fine for me.

-- 
balbi

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