2023-11-02 07:12:36

by Mehta, Piyush

[permalink] [raw]
Subject: [PATCH] usb: gadget: uvc_video: unlock before submitting a request to ep

There could be chances where the usb_ep_queue() could fail and trigger
complete() handler with error status. In this case, if usb_ep_queue()
is called with lock held and the triggered complete() handler is waiting
for the same lock to be cleared could result in a deadlock situation and
could result in system hang. To aviod this scenerio, call usb_ep_queue()
with lock removed. This patch does the same.

Signed-off-by: Piyush Mehta <[email protected]>
---
drivers/usb/gadget/function/uvc_video.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
index 91af3b1ef0d4..0a5d9ac145e7 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -460,11 +460,12 @@ static void uvcg_video_pump(struct work_struct *work)
req->no_interrupt = 1;
}

- /* Queue the USB request */
- ret = uvcg_video_ep_queue(video, req);
spin_unlock_irqrestore(&queue->irqlock, flags);

+ /* Queue the USB request */
+ ret = uvcg_video_ep_queue(video, req);
if (ret < 0) {
+ usb_ep_set_halt(video->ep);
uvcg_queue_cancel(queue, 0);
break;
}
--
2.25.1


2023-11-02 09:06:44

by Sergey Shtylyov

[permalink] [raw]
Subject: Re: [PATCH] usb: gadget: uvc_video: unlock before submitting a request to ep

Hello!

On 11/2/23 10:11 AM, Piyush Mehta wrote:

> There could be chances where the usb_ep_queue() could fail and trigger
> complete() handler with error status. In this case, if usb_ep_queue()
> is called with lock held and the triggered complete() handler is waiting
> for the same lock to be cleared could result in a deadlock situation and
> could result in system hang. To aviod this scenerio, call usb_ep_queue()

Scenario. :-)

> with lock removed. This patch does the same.

The last sentence is hardly needed.

> Signed-off-by: Piyush Mehta <[email protected]>
> ---
> drivers/usb/gadget/function/uvc_video.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
> index 91af3b1ef0d4..0a5d9ac145e7 100644
> --- a/drivers/usb/gadget/function/uvc_video.c
> +++ b/drivers/usb/gadget/function/uvc_video.c
> @@ -460,11 +460,12 @@ static void uvcg_video_pump(struct work_struct *work)
> req->no_interrupt = 1;
> }
>
> - /* Queue the USB request */
> - ret = uvcg_video_ep_queue(video, req);
> spin_unlock_irqrestore(&queue->irqlock, flags);
>
> + /* Queue the USB request */
> + ret = uvcg_video_ep_queue(video, req);
> if (ret < 0) {
> + usb_ep_set_halt(video->ep);

Hm, you don't say anything about this change in the patch
description...

[...]

MBR, Sergey

2023-11-02 12:01:36

by Dan Scally

[permalink] [raw]
Subject: Re: [PATCH] usb: gadget: uvc_video: unlock before submitting a request to ep

Hi Piyush - thanks for the patch

On 02/11/2023 07:11, Piyush Mehta wrote:
> There could be chances where the usb_ep_queue() could fail and trigger
> complete() handler with error status. In this case, if usb_ep_queue()
> is called with lock held and the triggered complete() handler is waiting
> for the same lock to be cleared could result in a deadlock situation and
> could result in system hang. To aviod this scenerio, call usb_ep_queue()
> with lock removed. This patch does the same.


s/aviod/avoid. s/scenerio/scenario/

>
> Signed-off-by: Piyush Mehta <[email protected]>
> ---
> drivers/usb/gadget/function/uvc_video.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
> index 91af3b1ef0d4..0a5d9ac145e7 100644
> --- a/drivers/usb/gadget/function/uvc_video.c
> +++ b/drivers/usb/gadget/function/uvc_video.c
> @@ -460,11 +460,12 @@ static void uvcg_video_pump(struct work_struct *work)
> req->no_interrupt = 1;
> }
>
> - /* Queue the USB request */
> - ret = uvcg_video_ep_queue(video, req);
> spin_unlock_irqrestore(&queue->irqlock, flags);
>
> + /* Queue the USB request */
> + ret = uvcg_video_ep_queue(video, req);
> if (ret < 0) {
> + usb_ep_set_halt(video->ep);


This change isn't mentioned, and shouldn't be necessary - uvcg_video_ep_queue() will already call
usb_ep_set_halt() if it's in the error path.

> uvcg_queue_cancel(queue, 0);
> break;
> }

2023-11-16 09:28:50

by Dan Scally

[permalink] [raw]
Subject: Re: [PATCH] usb: gadget: uvc_video: unlock before submitting a request to ep

CC Thinh - sorry to bother you, just want to make sure we fix this in the right place.

On 08/11/2023 11:48, Kuen-Han Tsai wrote:
> On 02/11/2023 07:11, Piyush Mehta wrote:
>> There could be chances where the usb_ep_queue() could fail and trigger
>> complete() handler with error status. In this case, if usb_ep_queue()
>> is called with lock held and the triggered complete() handler is waiting
>> for the same lock to be cleared could result in a deadlock situation and
>> could result in system hang. To aviod this scenerio, call usb_ep_queue()
>> with lock removed. This patch does the same.
> I would like to provide more background information on this problem.
>
> We met a deadlock issue on Android devices and the followings are stack traces.
>
> [35845.978435][T18021] Core - Debugging Information for Hardlockup core(8) - locked CPUs mask (0x100)
> [35845.978442][T18021] Call trace:
> [*][T18021] queued_spin_lock_slowpath+0x84/0x388
> [35845.978451][T18021] uvc_video_complete+0x180/0x24c
> [35845.978458][T18021] usb_gadget_giveback_request+0x38/0x14c
> [35845.978464][T18021] dwc3_gadget_giveback+0xe4/0x218
> [35845.978469][T18021] dwc3_gadget_ep_cleanup_cancelled_requests+0xc8/0x108
> [35845.978474][T18021] __dwc3_gadget_kick_transfer+0x34c/0x368
> [35845.978479][T18021] __dwc3_gadget_start_isoc+0x13c/0x3b8
> [35845.978483][T18021] dwc3_gadget_ep_queue+0x150/0x2f0
> [35845.978488][T18021] usb_ep_queue+0x58/0x16c
> [35845.978493][T18021] uvcg_video_pump+0x22c/0x518


I note in the kerneldoc comment for usb_ep_queue() that calling .complete() from within itself is
specifically disallowed [1]:

    Note that @req's ->complete() callback must never be called from

    within usb_ep_queue() as that can create deadlock situations.


And it looks like that's what's happening here - is this something that needs addressing in the dwc3
driver?


Thanks

Dan


[1] https://elixir.bootlin.com/linux/v6.7-rc1/source/drivers/usb/gadget/udc/core.c#L275

>
> As mentioned by Piyush, the uvcg_video_pump function acquires a spinlock before submitting the USB
> request to the endpoint, which will be processed by the dwc3 controller in our case.
>
> However, a deadlock can occur when the dwc3 controller fails to kick the transfer and decides to
> cancel and clean up all requests. At this point, the dwc3 driver calls the giveback function asking
> the corresponding driver to handle the cancellation. The uvcg_queue_cancel function then acquires
> the same spinlock to cancel the request, which results in a double acquirement and a deadlock.
>
>> Signed-off-by: Piyush Mehta <[email protected]>
>> ---
>> drivers/usb/gadget/function/uvc_video.c | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
>> index 91af3b1ef0d4..0a5d9ac145e7 100644
>> --- a/drivers/usb/gadget/function/uvc_video.c
>> +++ b/drivers/usb/gadget/function/uvc_video.c
>> @@ -460,11 +460,12 @@ static void uvcg_video_pump(struct work_struct *work)
>> req->no_interrupt = 1;
>> }
>>
>> - /* Queue the USB request */
>> - ret = uvcg_video_ep_queue(video, req);
>> spin_unlock_irqrestore(&queue->irqlock, flags);
>>
>> + /* Queue the USB request */
>> + ret = uvcg_video_ep_queue(video, req);
>> if (ret < 0) {
>> + usb_ep_set_halt(video->ep);
>> uvcg_queue_cancel(queue, 0);
>> break;
>> }

2023-11-17 01:47:07

by Thinh Nguyen

[permalink] [raw]
Subject: Re: [PATCH] usb: gadget: uvc_video: unlock before submitting a request to ep

Hi,

On Thu, Nov 16, 2023, Dan Scally wrote:
> CC Thinh - sorry to bother you, just want to make sure we fix this in the right place.
>
> On 08/11/2023 11:48, Kuen-Han Tsai wrote:
> > On 02/11/2023 07:11, Piyush Mehta wrote:
> > > There could be chances where the usb_ep_queue() could fail and trigger
> > > complete() handler with error status. In this case, if usb_ep_queue()
> > > is called with lock held and the triggered complete() handler is waiting
> > > for the same lock to be cleared could result in a deadlock situation and
> > > could result in system hang. To aviod this scenerio, call usb_ep_queue()
> > > with lock removed. This patch does the same.
> > I would like to provide more background information on this problem.
> >
> > We met a deadlock issue on Android devices and the followings are stack traces.
> >
> > [35845.978435][T18021] Core - Debugging Information for Hardlockup core(8) - locked CPUs mask (0x100)
> > [35845.978442][T18021] Call trace:
> > [*][T18021] queued_spin_lock_slowpath+0x84/0x388
> > [35845.978451][T18021] uvc_video_complete+0x180/0x24c
> > [35845.978458][T18021] usb_gadget_giveback_request+0x38/0x14c
> > [35845.978464][T18021] dwc3_gadget_giveback+0xe4/0x218
> > [35845.978469][T18021] dwc3_gadget_ep_cleanup_cancelled_requests+0xc8/0x108
> > [35845.978474][T18021] __dwc3_gadget_kick_transfer+0x34c/0x368
> > [35845.978479][T18021] __dwc3_gadget_start_isoc+0x13c/0x3b8
> > [35845.978483][T18021] dwc3_gadget_ep_queue+0x150/0x2f0
> > [35845.978488][T18021] usb_ep_queue+0x58/0x16c
> > [35845.978493][T18021] uvcg_video_pump+0x22c/0x518
>
>
> I note in the kerneldoc comment for usb_ep_queue() that calling .complete()
> from within itself is specifically disallowed [1]:
>
>     Note that @req's ->complete() callback must never be called from
>
>     within usb_ep_queue() as that can create deadlock situations.
>
>
> And it looks like that's what's happening here - is this something that
> needs addressing in the dwc3 driver?
>

Looks like it. The issue is in dwc3. It should only affect isoc request
queuing.

Can we try with this patch:

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 858fe4c299b7..37e08eed49d9 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1684,12 +1684,15 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep)
dwc3_gadget_move_cancelled_request(req, DWC3_REQUEST_STATUS_DEQUEUED);

/* If ep isn't started, then there's no end transfer pending */
- if (!(dep->flags & DWC3_EP_END_TRANSFER_PENDING))
+ if (!(dep->flags & DWC3_EP_PENDING_REQUEST) &&
+ !(dep->flags & DWC3_EP_END_TRANSFER_PENDING))
dwc3_gadget_ep_cleanup_cancelled_requests(dep);

return ret;
}

+ dep->flags &= ~DWC3_EP_PENDING_REQUEST;
+
if (dep->stream_capable && req->request.is_last &&
!DWC3_MST_CAPABLE(&dep->dwc->hwparams))
dep->flags |= DWC3_EP_WAIT_TRANSFER_COMPLETE;

---

Thanks,
Thinh

2023-11-17 03:29:01

by Thinh Nguyen

[permalink] [raw]
Subject: Re: [PATCH] usb: gadget: uvc_video: unlock before submitting a request to ep

On Fri, Nov 17, 2023, Thinh Nguyen wrote:
> Hi,
>
> On Thu, Nov 16, 2023, Dan Scally wrote:
> > CC Thinh - sorry to bother you, just want to make sure we fix this in the right place.
> >
> > On 08/11/2023 11:48, Kuen-Han Tsai wrote:
> > > On 02/11/2023 07:11, Piyush Mehta wrote:
> > > > There could be chances where the usb_ep_queue() could fail and trigger
> > > > complete() handler with error status. In this case, if usb_ep_queue()
> > > > is called with lock held and the triggered complete() handler is waiting
> > > > for the same lock to be cleared could result in a deadlock situation and
> > > > could result in system hang. To aviod this scenerio, call usb_ep_queue()
> > > > with lock removed. This patch does the same.
> > > I would like to provide more background information on this problem.
> > >
> > > We met a deadlock issue on Android devices and the followings are stack traces.
> > >
> > > [35845.978435][T18021] Core - Debugging Information for Hardlockup core(8) - locked CPUs mask (0x100)
> > > [35845.978442][T18021] Call trace:
> > > [*][T18021] queued_spin_lock_slowpath+0x84/0x388
> > > [35845.978451][T18021] uvc_video_complete+0x180/0x24c
> > > [35845.978458][T18021] usb_gadget_giveback_request+0x38/0x14c
> > > [35845.978464][T18021] dwc3_gadget_giveback+0xe4/0x218
> > > [35845.978469][T18021] dwc3_gadget_ep_cleanup_cancelled_requests+0xc8/0x108
> > > [35845.978474][T18021] __dwc3_gadget_kick_transfer+0x34c/0x368
> > > [35845.978479][T18021] __dwc3_gadget_start_isoc+0x13c/0x3b8
> > > [35845.978483][T18021] dwc3_gadget_ep_queue+0x150/0x2f0
> > > [35845.978488][T18021] usb_ep_queue+0x58/0x16c
> > > [35845.978493][T18021] uvcg_video_pump+0x22c/0x518
> >
> >
> > I note in the kerneldoc comment for usb_ep_queue() that calling .complete()
> > from within itself is specifically disallowed [1]:
> >
> >     Note that @req's ->complete() callback must never be called from
> >
> >     within usb_ep_queue() as that can create deadlock situations.
> >
> >
> > And it looks like that's what's happening here - is this something that
> > needs addressing in the dwc3 driver?
> >
>
> Looks like it. The issue is in dwc3. It should only affect isoc request
> queuing.
>
> Can we try with this patch:
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 858fe4c299b7..37e08eed49d9 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1684,12 +1684,15 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep)
> dwc3_gadget_move_cancelled_request(req, DWC3_REQUEST_STATUS_DEQUEUED);
>
> /* If ep isn't started, then there's no end transfer pending */
> - if (!(dep->flags & DWC3_EP_END_TRANSFER_PENDING))
> + if (!(dep->flags & DWC3_EP_PENDING_REQUEST) &&
> + !(dep->flags & DWC3_EP_END_TRANSFER_PENDING))
> dwc3_gadget_ep_cleanup_cancelled_requests(dep);
>
> return ret;
> }
>
> + dep->flags &= ~DWC3_EP_PENDING_REQUEST;
> +
> if (dep->stream_capable && req->request.is_last &&
> !DWC3_MST_CAPABLE(&dep->dwc->hwparams))
> dep->flags |= DWC3_EP_WAIT_TRANSFER_COMPLETE;
>
> ---
>

Actually, please ignore the above, that's not correct. I'll send out a
proper patch later.

Thanks,
Thinh

2024-01-10 09:15:20

by Pandey, Radhey Shyam

[permalink] [raw]
Subject: RE: [PATCH] usb: gadget: uvc_video: unlock before submitting a request to ep

> -----Original Message-----
> From: Thinh Nguyen <[email protected]>
> Sent: Friday, November 17, 2023 8:59 AM
> To: Dan Scally <[email protected]>
> Cc: Kuen-Han Tsai <[email protected]>; [email protected];
> [email protected]; [email protected]; linux-
> [email protected]; Simek, Michal <[email protected]>; Mehta,
> Piyush <[email protected]>; Pandey, Radhey Shyam
> <[email protected]>; Paladugu, Siva Durga Prasad
> <[email protected]>
> Subject: Re: [PATCH] usb: gadget: uvc_video: unlock before submitting a
> request to ep
>
> On Fri, Nov 17, 2023, Thinh Nguyen wrote:
> > Hi,
> >
> > On Thu, Nov 16, 2023, Dan Scally wrote:
> > > CC Thinh - sorry to bother you, just want to make sure we fix this in the
> right place.
> > >
> > > On 08/11/2023 11:48, Kuen-Han Tsai wrote:
> > > > On 02/11/2023 07:11, Piyush Mehta wrote:
> > > > > There could be chances where the usb_ep_queue() could fail and
> > > > > trigger
> > > > > complete() handler with error status. In this case, if
> > > > > usb_ep_queue() is called with lock held and the triggered
> > > > > complete() handler is waiting for the same lock to be cleared
> > > > > could result in a deadlock situation and could result in system
> > > > > hang. To aviod this scenerio, call usb_ep_queue() with lock removed.
> This patch does the same.
> > > > I would like to provide more background information on this problem.
> > > >
> > > > We met a deadlock issue on Android devices and the followings are
> stack traces.
> > > >
> > > > [35845.978435][T18021] Core - Debugging Information for Hardlockup
> > > > core(8) - locked CPUs mask (0x100) [35845.978442][T18021] Call trace:
> > > > [*][T18021] queued_spin_lock_slowpath+0x84/0x388
> > > > [35845.978451][T18021] uvc_video_complete+0x180/0x24c
> > > > [35845.978458][T18021] usb_gadget_giveback_request+0x38/0x14c
> > > > [35845.978464][T18021] dwc3_gadget_giveback+0xe4/0x218
> > > > [35845.978469][T18021]
> > > > dwc3_gadget_ep_cleanup_cancelled_requests+0xc8/0x108
> > > > [35845.978474][T18021] __dwc3_gadget_kick_transfer+0x34c/0x368
> > > > [35845.978479][T18021] __dwc3_gadget_start_isoc+0x13c/0x3b8
> > > > [35845.978483][T18021] dwc3_gadget_ep_queue+0x150/0x2f0
> > > > [35845.978488][T18021] usb_ep_queue+0x58/0x16c
> > > > [35845.978493][T18021] uvcg_video_pump+0x22c/0x518
> > >
> > >
> > > I note in the kerneldoc comment for usb_ep_queue() that calling
> > > .complete() from within itself is specifically disallowed [1]:
> > >
> > >     Note that @req's ->complete() callback must never be called from
> > >
> > >     within usb_ep_queue() as that can create deadlock situations.
> > >
> > >
> > > And it looks like that's what's happening here - is this something
> > > that needs addressing in the dwc3 driver?
> > >
> >
> > Looks like it. The issue is in dwc3. It should only affect isoc
> > request queuing.
> >
> > Can we try with this patch:
> >
> > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> > index 858fe4c299b7..37e08eed49d9 100644
> > --- a/drivers/usb/dwc3/gadget.c
> > +++ b/drivers/usb/dwc3/gadget.c
> > @@ -1684,12 +1684,15 @@ static int __dwc3_gadget_kick_transfer(struct
> dwc3_ep *dep)
> > dwc3_gadget_move_cancelled_request(req,
> > DWC3_REQUEST_STATUS_DEQUEUED);
> >
> > /* If ep isn't started, then there's no end transfer pending */
> > - if (!(dep->flags & DWC3_EP_END_TRANSFER_PENDING))
> > + if (!(dep->flags & DWC3_EP_PENDING_REQUEST) &&
> > + !(dep->flags & DWC3_EP_END_TRANSFER_PENDING))
> > dwc3_gadget_ep_cleanup_cancelled_requests(dep);
> >
> > return ret;
> > }
> >
> > + dep->flags &= ~DWC3_EP_PENDING_REQUEST;
> > +
> > if (dep->stream_capable && req->request.is_last &&
> > !DWC3_MST_CAPABLE(&dep->dwc->hwparams))
> > dep->flags |= DWC3_EP_WAIT_TRANSFER_COMPLETE;
> >
> > ---
> >
>
> Actually, please ignore the above, that's not correct. I'll send out a proper
> patch later.

Thanks, Thinh. I came across this thread and wanted to check if you
have some fix ready?

2024-01-11 01:22:01

by Thinh Nguyen

[permalink] [raw]
Subject: Re: [PATCH] usb: gadget: uvc_video: unlock before submitting a request to ep

On Wed, Jan 10, 2024, Pandey, Radhey Shyam wrote:
> > -----Original Message-----
> > From: Thinh Nguyen <[email protected]>
> > Sent: Friday, November 17, 2023 8:59 AM
> > To: Dan Scally <[email protected]>
> > Cc: Kuen-Han Tsai <[email protected]>; [email protected];
> > [email protected]; [email protected]; linux-
> > [email protected]; Simek, Michal <[email protected]>; Mehta,
> > Piyush <[email protected]>; Pandey, Radhey Shyam
> > <[email protected]>; Paladugu, Siva Durga Prasad
> > <[email protected]>
> > Subject: Re: [PATCH] usb: gadget: uvc_video: unlock before submitting a
> > request to ep
> >
> > On Fri, Nov 17, 2023, Thinh Nguyen wrote:
> > > Hi,
> > >
>
> Thanks, Thinh. I came across this thread and wanted to check if you
> have some fix ready?

Hi Pandey,

Sorry, I just recently came back from a break. It's on my TODO list. I
expect to have the fix patch after 6.8-rc1 release.

Thanks,
Thinh

2024-01-19 02:16:06

by Thinh Nguyen

[permalink] [raw]
Subject: Re: [PATCH] usb: gadget: uvc_video: unlock before submitting a request to ep

Hi,

On Wed, Jan 10, 2024, Pandey, Radhey Shyam wrote:
>
> Thanks, Thinh. I came across this thread and wanted to check if you
> have some fix ready?

Would you mind test this change to see if it fixes the issue:

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 8d1881adcd80..f40c7b9105cc 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1808,6 +1808,7 @@ static int dwc3_prepare_trbs(struct dwc3_ep *dep)
return ret;
}

+static void dwc3_gadget_ep_skip_trbs(struct dwc3_ep *dep, struct dwc3_request *req);
static void dwc3_gadget_ep_cleanup_cancelled_requests(struct dwc3_ep *dep);
static void dwc3_gadget_restore_requests(struct dwc3_ep *dep);

@@ -1874,9 +1875,27 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep)
list_for_each_entry_safe(req, tmp, &dep->started_list, list)
dwc3_gadget_move_cancelled_request(req, DWC3_REQUEST_STATUS_DEQUEUED);

- /* If ep isn't started, then there's no end transfer pending */
- if (!(dep->flags & DWC3_EP_END_TRANSFER_PENDING))
- dwc3_gadget_ep_cleanup_cancelled_requests(dep);
+ if ((dep->flags & DWC3_EP_DELAY_START) ||
+ (dep->flags & DWC3_EP_WAIT_TRANSFER_COMPLETE) ||
+ (dep->flags & DWC3_EP_TRANSFER_STARTED)) {
+ /* If ep isn't started, then there's no end transfer pending */
+ if (!(dep->flags & DWC3_EP_END_TRANSFER_PENDING))
+ dwc3_gadget_ep_cleanup_cancelled_requests(dep);
+ } else {
+ /*
+ * To be in this condition means usb_ep_queue() isn't
+ * completed yet. Since the controller hasn't owned the
+ * requests yet, don't give back the requests on failed
+ * usb_ep_queue. Simply remove them from the endpoint's
+ * list.
+ */
+ while (!list_empty(&dep->cancelled_list)) {
+ req = next_request(&dep->cancelled_list);
+ dwc3_gadget_ep_skip_trbs(dep, req);
+ dwc3_gadget_del_and_unmap_request(dep, req, -EINPROGRESS);
+ req->status = DWC3_REQUEST_STATUS_COMPLETED;
+ }
+ }

return ret;
}

BTW, what was the error code returned from usb_ep_queue()? Is it
-ETIMEDOUT?

Thanks,
Thinh

2024-01-29 10:08:25

by Pandey, Radhey Shyam

[permalink] [raw]
Subject: RE: [PATCH] usb: gadget: uvc_video: unlock before submitting a request to ep

> -----Original Message-----
> From: Thinh Nguyen <[email protected]>
> Sent: Friday, January 19, 2024 7:45 AM
> To: Pandey, Radhey Shyam <[email protected]>
> Cc: Thinh Nguyen <[email protected]>; Dan Scally
> <[email protected]>; Kuen-Han Tsai <[email protected]>;
> [email protected]; [email protected]; linux-
> [email protected]; [email protected]; Simek, Michal
> <[email protected]>; Mehta, Piyush <[email protected]>;
> Paladugu, Siva Durga Prasad <[email protected]>
> Subject: Re: [PATCH] usb: gadget: uvc_video: unlock before submitting a
> request to ep
>
> Hi,
>
> On Wed, Jan 10, 2024, Pandey, Radhey Shyam wrote:
> >
> > Thanks, Thinh. I came across this thread and wanted to check if you
> > have some fix ready?

I have added Mubin to this thread as he is working on validating below fix.

>
> Would you mind test this change to see if it fixes the issue:
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index
> 8d1881adcd80..f40c7b9105cc 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1808,6 +1808,7 @@ static int dwc3_prepare_trbs(struct dwc3_ep *dep)
> return ret;
> }
>
> +static void dwc3_gadget_ep_skip_trbs(struct dwc3_ep *dep, struct
> +dwc3_request *req);
> static void dwc3_gadget_ep_cleanup_cancelled_requests(struct dwc3_ep
> *dep); static void dwc3_gadget_restore_requests(struct dwc3_ep *dep);
>
> @@ -1874,9 +1875,27 @@ static int __dwc3_gadget_kick_transfer(struct
> dwc3_ep *dep)
> list_for_each_entry_safe(req, tmp, &dep->started_list, list)
> dwc3_gadget_move_cancelled_request(req,
> DWC3_REQUEST_STATUS_DEQUEUED);
>
> - /* If ep isn't started, then there's no end transfer pending */
> - if (!(dep->flags & DWC3_EP_END_TRANSFER_PENDING))
> - dwc3_gadget_ep_cleanup_cancelled_requests(dep);
> + if ((dep->flags & DWC3_EP_DELAY_START) ||
> + (dep->flags & DWC3_EP_WAIT_TRANSFER_COMPLETE) ||
> + (dep->flags & DWC3_EP_TRANSFER_STARTED)) {
> + /* If ep isn't started, then there's no end transfer
> pending */
> + if (!(dep->flags &
> DWC3_EP_END_TRANSFER_PENDING))
> +
> dwc3_gadget_ep_cleanup_cancelled_requests(dep);
> + } else {
> + /*
> + * To be in this condition means usb_ep_queue() isn't
> + * completed yet. Since the controller hasn't owned
> the
> + * requests yet, don't give back the requests on failed
> + * usb_ep_queue. Simply remove them from the
> endpoint's
> + * list.
> + */
> + while (!list_empty(&dep->cancelled_list)) {
> + req = next_request(&dep->cancelled_list);
> + dwc3_gadget_ep_skip_trbs(dep, req);
> + dwc3_gadget_del_and_unmap_request(dep,
> req, -EINPROGRESS);
> + req->status =
> DWC3_REQUEST_STATUS_COMPLETED;
> + }
> + }
>
> return ret;
> }
>
> BTW, what was the error code returned from usb_ep_queue()? Is it -
> ETIMEDOUT?
>
> Thanks,
> Thinh