Date:   Mon, 3 May 2021 20:27:53 -0700
From:   Wesley Cheng <wcheng@...eaurora.org>
To:     Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
        "balbi@...nel.org" <balbi@...nel.org>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>
Cc:     "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "jackp@...eaurora.org" <jackp@...eaurora.org>
Subject: Re: [PATCH v2] usb: dwc3: gadget: Avoid canceling current request for
 queuing error



On 5/3/2021 8:12 PM, Thinh Nguyen wrote:
> 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

Hi Thinh,

> 
> sounds like a race issue.
>

It'll always happen if usb_ep_queue() fails with an error. Sorry for not
clarifying, but the "..." represents executing in a different context
:). Anything above the "..." is in the same context.
>>
>>> 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.
> 

Sorry for missing that part of the question.  It is a no xfer resource
error on a start transfer command.  So far this happens on low system
memory test cases, so there may be some sequences that were missed,
which led to this particular command error.

Thanks
Wesley Cheng

>>
>>> 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
> 

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Powered by blists - more mailing lists