From: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
To: Wesley Cheng <wcheng@codeaurora.org>,
Thinh Nguyen <Thinh.Nguyen@synopsys.com>,
"balbi@kernel.org" <balbi@kernel.org>,
"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>
Cc: "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"jackp@codeaurora.org" <jackp@codeaurora.org>
Subject: Re: [PATCH v2] usb: dwc3: gadget: Avoid canceling current request for queuing error
Date: Tue, 4 May 2021 03:12:00 +0000 [thread overview]
Message-ID: <e12fc396-76e6-9506-31c8-cfdee3fb7577@synopsys.com> (raw)
In-Reply-To: <513e6c16-9586-c78e-881b-08e0a73c50a8@codeaurora.org>
Hi Wesley,
Wesley Cheng wrote:
>
>
> On 5/3/2021 7:20 PM, Thinh Nguyen wrote:
>> Hi,
>>
>> Wesley Cheng wrote:
>>> If an error is received when issuing a start or update transfer
>>> command, the error handler will stop all active requests (including
>>> the current USB request), and call dwc3_gadget_giveback() to notify
>>> function drivers of the requests which have been stopped. Avoid
>>> having to cancel the current request which is trying to be queued, as
>>> the function driver will handle the EP queue error accordingly.
>>> Simply unmap the request as it was done before, and allow previously
>>> started transfers to be cleaned up.
>>>
>
> Hi Thinh,
>
>>
>> It looks like you're still letting dwc3 stopping and cancelling all the
>> active requests instead letting the function driver doing the dequeue.
>>
>
> Yeah, main issue isn't due to the function driver doing dequeue, but
> having cleanup (ie USB request free) if there is an error during
> usb_ep_queue().
>
> The function driver in question at the moment is the f_fs driver in AIO
> mode. When async IO is enabled in the FFS driver, every time it queues
> a packet, it will allocate a io_data struct beforehand. If the
> usb_ep_queue() fails it will free this io_data memory. Problem is that,
> since the DWC3 gadget calls the completion with -ECONNRESET, the FFS
> driver will also schedule a work item (within io_data struct) to handle
> the completion. So you end up with a flow like below
>
> allocate io_data (ffs)
> --> usb_ep_queue()
> --> __dwc3_gadget_kick_transfer()
> --> dwc3_send_gadget_ep_cmd(EINVAL)
> --> dwc3_gadget_ep_cleanup_cancelled_requests()
> --> dwc3_gadget_giveback(ECONNRESET)
> ffs completion callback
> queue work item within io_data
> --> usb_ep_queue returns EINVAL
> ffs frees io_data
> ...
>
> work scheduled
> --> NULL pointer/memory fault as io_data is freed
sounds like a race issue.
>
>> BTW, what kinds of command and error do you see in your setup and for
>> what type endpoint? I'm thinking of letting the function driver to
>> dequeue the requests instead of letting dwc3 automatically
>> ending/cancelling the queued requests. However, it's a bit tricky to do
>> that if the error is -ETIMEDOUT since we're not sure if the controller
>> had already cached the TRBs.
>>
>
> Happens on bulk EPs so far, but I think it wouldn't matter as long as
> its over the FFS interface. (and using async IO transfers)
Do you know which command and error code? It's strange if
UPDATE_TRANSFER command failed.
>
>> This seems to add more complexity and I don't have a good solution to
>> it. Since you're already cancelling all the active request anyway, what
>> do you think of always letting dwc3_gadget_ep_queue() to go through with
>> success, but report failure through request completion?
>>
>
> We do have something similar as well downstream (returning success
> always on dwc3_gadget_ep_queue()) and its been working for us also.
> Problem is we don't test the ISOC path much, so this is the only type of
> EP that might come into question...
>
It should be similiar with isoc. I can't think of a potential issue yet.
> Coming up with a way to address the concerns you brought up was a bit
> difficult as there were scenarios we needed to consider. next_request()
> doesn't always have to be the request being queued (even if ep queue
> triggered it). There was no easy way to determine if kick transfer was
> due to ep queue, but even if there was, we'd need to remember the
> previous point as well.
>
Yeah, there are a few pitfalls. I don't have a good solution to it if we
want to return failure immediately and let the function driver handle
the dequeue (if it wants to).
Thanks,
Thinh
next prev parent reply other threads:[~2021-05-04 3:12 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-04 1:21 [PATCH v2] usb: dwc3: gadget: Avoid canceling current request for queuing error Wesley Cheng
2021-05-04 2:20 ` Thinh Nguyen
2021-05-04 2:45 ` Wesley Cheng
2021-05-04 3:12 ` Thinh Nguyen [this message]
2021-05-04 3:27 ` Wesley Cheng
2021-05-04 5:22 ` Thinh Nguyen
2021-05-04 8:24 ` Wesley Cheng
2021-05-05 1:50 ` Thinh Nguyen
2021-05-05 3:37 ` Wesley Cheng
2021-05-05 12:59 ` Felipe Balbi
2021-05-05 12:57 ` Felipe Balbi
2021-05-05 15:19 ` Alan Stern
2021-05-05 18:01 ` Wesley Cheng
2021-05-05 18:31 ` Thinh Nguyen
2021-05-05 18:46 ` Alan Stern
2021-05-06 9:04 ` Felipe Balbi
2021-05-06 18:06 ` Thinh Nguyen
2021-05-05 17:59 ` Wesley Cheng
2021-05-06 9:00 ` Felipe Balbi
2021-05-05 19:06 ` Thinh Nguyen
2021-05-05 12:50 ` Felipe Balbi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e12fc396-76e6-9506-31c8-cfdee3fb7577@synopsys.com \
--to=thinh.nguyen@synopsys.com \
--cc=balbi@kernel.org \
--cc=gregkh@linuxfoundation.org \
--cc=jackp@codeaurora.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=wcheng@codeaurora.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).