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
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
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;
> }
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;
>> }
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
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
> -----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?
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
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
> -----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
> -----Original Message-----
> From: Pandey, Radhey Shyam
> Sent: Monday, January 29, 2024 3:38 PM
> To: Thinh Nguyen <[email protected]>
> Cc: Dan Scally <[email protected]>; Kuen-Han Tsai
> <[email protected]>; [email protected];
> [email protected]; [email protected]; linux-
> [email protected]; Simek, Michal <[email protected]>; Mehta,
> Piyush <[email protected]>; Paladugu, Siva Durga Prasad
> <[email protected]>; Sayyed, Mubin
> <[email protected]>
> 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.
Thinh: Unfortunately, i am not able to replicate failure behaviour and
validate the below fix. Tested webcam gadget taking stream from vivid
and then frame capture on host using yavta.
@Kuen-Han Tsai: Do you have failure environment to replicate the hang?
>
> >
> > 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
Hi Radhey Shyam,
> Thinh: Unfortunately, i am not able to replicate failure behaviour and
> validate the below fix. Tested webcam gadget taking stream from vivid
> and then frame capture on host using yavta.
>
> @Kuen-Han Tsai: Do you have failure environment to replicate the hang?
>
Unfortunately, I don't have a failing environment to replicate the
hang. The reporter didn't provide the steps to reproduce the issue,
and all I have are kernel panic stack traces.
Regards,
Kuen-Han