Re: [PATCH v2] usb: dwc3: gadget: Avoid canceling current request for queuing error

[Felipe Balbi <balbi@kernel.org>]

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


Hi,

Wesley Cheng <wcheng@codeaurora.org> writes:
>> If I understood the whole thing correctly, we want everything except the
>> current request (the one that failed START or UPDATE transfer) to go
>> through giveback(). This really tells me that we're not handling error
>> case in kick_transfer and/or prepare_trbs() correctly.
>> 
>
> We don't want the request passed in usb_ep_queue() to be calling
> giveback() IF DONE IN the usb_ep_queue() context only.

right, that's how this should behave.

>> I also don't want to pass another argument to kick_transfer because it
>> should be unnecessary: the current request should *always* be the last
>> one in the list. Therefore we should rely on something like
>> list_last_entry() followed by list_for_each_entry_safe_reverse() to
>> handle this without a special case.
>> 
>> ret = dwc3_send_gadget_ep_cmd();
>> if (ret < 0) {
>> 	current = list_last_entry();
>> 
>> 	unmap(current);
>>         for_each_trb_in(current) {
>>         	clear_HWO(trb);
>>         }
>> 
>> 	list_for_entry_safe_reverse() {
>>         	move_cancelled();
>>         }
>> }
>>
> Nice, thanks for the suggestion and info!  Problem we have is that kick
> transfer is being used elsewhere, for example, during the TRB complete path:
>
> static bool dwc3_gadget_endpoint_trbs_complete(struct dwc3_ep *dep,
> 		const struct dwc3_event_depevt *event, int status)
> {
> ...
> 	else if (dwc3_gadget_ep_should_continue(dep))
> 		if (__dwc3_gadget_kick_transfer(dep) == 0)
> 			no_started_trb = false;
>
> So in these types of calls, we would still want ALL requests to be
> cancelled w/ giveback() called, so that the completion() callbacks can
> cleanup/free those requests accordingly.
>
> If we went and only unmapped the last entry (and removed it from any
> list), then no one would clean it up as it is outside of the
> usb_ep_queue() context, and not within any of the DWC3 lists.

oh, I see what you mean. At the moment we want kick_transfer to behave
in two different manners and that's probably where the bug is
originating from.

It sounds like it's time to split kick_transfer into
kick_queued_transfer() and e.g. continue_pending_transfers()

The thing is that if we continue to sprinkle special cases all over the
place, soon enough it'll be super hard to maintain the driver.

-- 
balbi

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