We have been seeing two main bugs when stopping stream:
1. attempting to queue usb_requests on a disabled usb endpoint, and
2. use-after-free problems for inflight requests
Avichal Rakesh (2):
usb: gadget: uvc: prevent use of disabled endpoint
usb: gadget: uvc: prevent de-allocating inflight usb_requests
drivers/usb/gadget/function/f_uvc.c | 11 ++++----
drivers/usb/gadget/function/f_uvc.h | 2 +-
drivers/usb/gadget/function/uvc.h | 5 +++-
drivers/usb/gadget/function/uvc_v4l2.c | 21 ++++++++++++---
drivers/usb/gadget/function/uvc_video.c | 34 +++++++++++++++++++++++--
5 files changed, 60 insertions(+), 13 deletions(-)
--
2.42.0.283.g2d96d420d3-goog
Currently the set_alt callback immediately disables the endpoint and queues
the v4l2 streamoff event. However, as the streamoff event is processed
asynchronously, it is possible that the video_pump thread attempts to queue
requests to an already disabled endpoint.
This change moves disabling usb endpoint to the end of streamoff event
callback. As the endpoint's state can no longer be used, video_pump is
now guarded by uvc->state as well. To be consistent with the actual
streaming state, uvc->state is now toggled between CONNECTED and STREAMING
from the v4l2 event callback only.
Link: https://lore.kernel.org/[email protected]/
Link: https://lore.kernel.org/[email protected]/
Signed-off-by: Avichal Rakesh <[email protected]>
---
drivers/usb/gadget/function/f_uvc.c | 11 +++++------
drivers/usb/gadget/function/f_uvc.h | 2 +-
drivers/usb/gadget/function/uvc.h | 2 +-
drivers/usb/gadget/function/uvc_v4l2.c | 21 ++++++++++++++++++---
drivers/usb/gadget/function/uvc_video.c | 3 ++-
5 files changed, 27 insertions(+), 12 deletions(-)
diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
index faa398109431..75c9f9a3f884 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -263,10 +263,13 @@ uvc_function_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
return 0;
}
-void uvc_function_setup_continue(struct uvc_device *uvc)
+void uvc_function_setup_continue(struct uvc_device *uvc, int disable_ep)
{
struct usb_composite_dev *cdev = uvc->func.config->cdev;
+ if (disable_ep && uvc->video.ep) {
+ usb_ep_disable(uvc->video.ep);
+ }
usb_composite_setup_continue(cdev);
}
@@ -337,15 +340,11 @@ uvc_function_set_alt(struct usb_function *f, unsigned interface, unsigned alt)
if (uvc->state != UVC_STATE_STREAMING)
return 0;
- if (uvc->video.ep)
- usb_ep_disable(uvc->video.ep);
-
memset(&v4l2_event, 0, sizeof(v4l2_event));
v4l2_event.type = UVC_EVENT_STREAMOFF;
v4l2_event_queue(&uvc->vdev, &v4l2_event);
- uvc->state = UVC_STATE_CONNECTED;
- return 0;
+ return USB_GADGET_DELAYED_STATUS;
case 1:
if (uvc->state != UVC_STATE_CONNECTED)
diff --git a/drivers/usb/gadget/function/f_uvc.h b/drivers/usb/gadget/function/f_uvc.h
index 1db972d4beeb..e7f9f13f14dc 100644
--- a/drivers/usb/gadget/function/f_uvc.h
+++ b/drivers/usb/gadget/function/f_uvc.h
@@ -11,7 +11,7 @@
struct uvc_device;
-void uvc_function_setup_continue(struct uvc_device *uvc);
+void uvc_function_setup_continue(struct uvc_device *uvc, int disale_ep);
void uvc_function_connect(struct uvc_device *uvc);
diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
index 6751de8b63ad..989bc6b4e93d 100644
--- a/drivers/usb/gadget/function/uvc.h
+++ b/drivers/usb/gadget/function/uvc.h
@@ -177,7 +177,7 @@ struct uvc_file_handle {
* Functions
*/
-extern void uvc_function_setup_continue(struct uvc_device *uvc);
+extern void uvc_function_setup_continue(struct uvc_device *uvc, int disable_ep);
extern void uvc_function_connect(struct uvc_device *uvc);
extern void uvc_function_disconnect(struct uvc_device *uvc);
diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
index 3f0a9795c0d4..3d3469883ed0 100644
--- a/drivers/usb/gadget/function/uvc_v4l2.c
+++ b/drivers/usb/gadget/function/uvc_v4l2.c
@@ -451,7 +451,7 @@ uvc_v4l2_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
* Complete the alternate setting selection setup phase now that
* userspace is ready to provide video frames.
*/
- uvc_function_setup_continue(uvc);
+ uvc_function_setup_continue(uvc, 0);
uvc->state = UVC_STATE_STREAMING;
return 0;
@@ -463,11 +463,19 @@ uvc_v4l2_streamoff(struct file *file, void *fh, enum v4l2_buf_type type)
struct video_device *vdev = video_devdata(file);
struct uvc_device *uvc = video_get_drvdata(vdev);
struct uvc_video *video = &uvc->video;
+ int ret = 0;
if (type != video->queue.queue.type)
return -EINVAL;
- return uvcg_video_enable(video, 0);
+ uvc->state = UVC_STATE_CONNECTED;
+ ret = uvcg_video_enable(video, 0);
+ if (ret < 0) {
+ return ret;
+ }
+
+ uvc_function_setup_continue(uvc, 1);
+ return 0;
}
static int
@@ -500,6 +508,14 @@ uvc_v4l2_subscribe_event(struct v4l2_fh *fh,
static void uvc_v4l2_disable(struct uvc_device *uvc)
{
uvc_function_disconnect(uvc);
+ if (uvc->state == UVC_STATE_STREAMING) {
+ /*
+ * Drop uvc->state to CONNECTED if it was streaming before.
+ * This ensures that the usb_requests are no longer queued
+ * to the controller.
+ */
+ uvc->state = UVC_STATE_CONNECTED;
+ }
uvcg_video_enable(&uvc->video, 0);
uvcg_free_buffers(&uvc->video.queue);
uvc->func_connected = false;
@@ -647,4 +663,3 @@ const struct v4l2_file_operations uvc_v4l2_fops = {
.get_unmapped_area = uvcg_v4l2_get_unmapped_area,
#endif
};
-
diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
index 91af3b1ef0d4..70ff88854539 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -382,6 +382,7 @@ static void uvcg_video_pump(struct work_struct *work)
{
struct uvc_video *video = container_of(work, struct uvc_video, pump);
struct uvc_video_queue *queue = &video->queue;
+ struct uvc_device *uvc = video->uvc;
/* video->max_payload_size is only set when using bulk transfer */
bool is_bulk = video->max_payload_size;
struct usb_request *req = NULL;
@@ -390,7 +391,7 @@ static void uvcg_video_pump(struct work_struct *work)
bool buf_done;
int ret;
- while (video->ep->enabled) {
+ while (uvc->state == UVC_STATE_STREAMING && video->ep->enabled) {
/*
* Retrieve the first available USB request, protected by the
* request lock.
--
2.42.0.283.g2d96d420d3-goog
Currently, when stopping the stream, uvcg_video_enable immediately
deallocates the usb_requests after calling usb_ep_dequeue. However,
usb_ep_dequeue is asynchronous and it is possible that it deallocates an
inflight request. The gadget drivers should wait until the complete
callbacks before assuming ownership of the request.
This patch adds a simple request counting mechanism to track how many
requests are currently owned by the driver. Now when stopping the stream,
uvcg_video_enable waits for all the complete callbacks to come through
before deallocating the usb_requests.
Signed-off-by: Avichal Rakesh <[email protected]>
---
drivers/usb/gadget/function/uvc.h | 3 +++
drivers/usb/gadget/function/uvc_video.c | 31 ++++++++++++++++++++++++-
2 files changed, 33 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
index 989bc6b4e93d..e40e702a7074 100644
--- a/drivers/usb/gadget/function/uvc.h
+++ b/drivers/usb/gadget/function/uvc.h
@@ -104,8 +104,11 @@ struct uvc_video {
unsigned int req_size;
struct uvc_request *ureq;
struct list_head req_free;
+ unsigned int req_free_count; /* number of requests in req_free */
spinlock_t req_lock;
+ wait_queue_head_t req_free_queue;
+
unsigned int req_int_count;
void (*encode) (struct usb_request *req, struct uvc_video *video,
diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
index 70ff88854539..3ea7d52df80d 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -284,10 +284,18 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
spin_lock_irqsave(&video->req_lock, flags);
list_add_tail(&req->list, &video->req_free);
+ video->req_free_count++;
spin_unlock_irqrestore(&video->req_lock, flags);
- if (uvc->state == UVC_STATE_STREAMING)
+ if (uvc->state == UVC_STATE_STREAMING) {
queue_work(video->async_wq, &video->pump);
+ } else if (video->req_free_count == video->req_size) {
+ /*
+ * Wake up thread waiting for all requests to be returned to
+ * the gadget driver.
+ */
+ wake_up_interruptible(&video->req_free_queue);
+ }
}
static int
@@ -316,6 +324,7 @@ uvc_video_free_requests(struct uvc_video *video)
INIT_LIST_HEAD(&video->req_free);
video->req_size = 0;
+ video->req_free_count = 0;
return 0;
}
@@ -360,6 +369,7 @@ uvc_video_alloc_requests(struct uvc_video *video)
}
video->req_size = req_size;
+ video->req_free_count = req_size; /* all requests are currently free */
return 0;
@@ -404,6 +414,7 @@ static void uvcg_video_pump(struct work_struct *work)
req = list_first_entry(&video->req_free, struct usb_request,
list);
list_del(&req->list);
+ video->req_free_count--;
spin_unlock_irqrestore(&video->req_lock, flags);
/*
@@ -480,6 +491,7 @@ static void uvcg_video_pump(struct work_struct *work)
spin_lock_irqsave(&video->req_lock, flags);
list_add_tail(&req->list, &video->req_free);
+ video->req_free_count++;
spin_unlock_irqrestore(&video->req_lock, flags);
return;
}
@@ -506,6 +518,22 @@ int uvcg_video_enable(struct uvc_video *video, int enable)
if (video->ureq && video->ureq[i].req)
usb_ep_dequeue(video->ep, video->ureq[i].req);
+ /*
+ * Wait 500ms for the usb_requests to be given back to the
+ * gadget driver. This ensures that we don't accidentally
+ * reference de-allocated usb_requests in the complete callback.
+ */
+ if (video->req_free_count != video->req_size) {
+ uvcg_info(&video->uvc->func,
+ "Waiting 500ms for usb_request complete callbacks.\n");
+ ret = wait_event_interruptible_timeout(
+ video->req_free_queue,
+ video->req_free_count == video->req_size,
+ msecs_to_jiffies(500));
+ uvcg_info(&video->uvc->func,
+ "Done waiting for complete callbacks: %d\n", ret);
+ }
+
uvc_video_free_requests(video);
uvcg_queue_enable(&video->queue, 0);
return 0;
@@ -538,6 +566,7 @@ int uvcg_video_init(struct uvc_video *video, struct uvc_device *uvc)
{
INIT_LIST_HEAD(&video->req_free);
spin_lock_init(&video->req_lock);
+ init_waitqueue_head(&video->req_free_queue);
INIT_WORK(&video->pump, uvcg_video_pump);
/* Allocate a work queue for asynchronous video pump handler. */
--
2.42.0.283.g2d96d420d3-goog
On Mon, Sep 11, 2023 at 9:19 PM Avichal Rakesh <[email protected]> wrote:
>
> We have been seeing two main bugs when stopping stream:
> 1. attempting to queue usb_requests on a disabled usb endpoint, and
> 2. use-after-free problems for inflight requests
>
> Avichal Rakesh (2):
> usb: gadget: uvc: prevent use of disabled endpoint
> usb: gadget: uvc: prevent de-allocating inflight usb_requests
>
> drivers/usb/gadget/function/f_uvc.c | 11 ++++----
> drivers/usb/gadget/function/f_uvc.h | 2 +-
> drivers/usb/gadget/function/uvc.h | 5 +++-
> drivers/usb/gadget/function/uvc_v4l2.c | 21 ++++++++++++---
> drivers/usb/gadget/function/uvc_video.c | 34 +++++++++++++++++++++++--
> 5 files changed, 60 insertions(+), 13 deletions(-)
>
Bumping this thread up. Laurent, Dan, and Michael could you take a look?
Thank you!
- Avi.
Hi Avichal
On Thu, Sep 14, 2023 at 04:05:36PM -0700, Avichal Rakesh wrote:
>On Mon, Sep 11, 2023 at 9:19 PM Avichal Rakesh <[email protected]> wrote:
>>
>> We have been seeing two main bugs when stopping stream:
>> 1. attempting to queue usb_requests on a disabled usb endpoint, and
>> 2. use-after-free problems for inflight requests
>>
>> Avichal Rakesh (2):
>> usb: gadget: uvc: prevent use of disabled endpoint
>> usb: gadget: uvc: prevent de-allocating inflight usb_requests
>>
>> drivers/usb/gadget/function/f_uvc.c | 11 ++++----
>> drivers/usb/gadget/function/f_uvc.h | 2 +-
>> drivers/usb/gadget/function/uvc.h | 5 +++-
>> drivers/usb/gadget/function/uvc_v4l2.c | 21 ++++++++++++---
>> drivers/usb/gadget/function/uvc_video.c | 34 +++++++++++++++++++++++--
>> 5 files changed, 60 insertions(+), 13 deletions(-)
>>
>
>Bumping this thread up. Laurent, Dan, and Michael could you take a look?
I tested the patches against my setup and it did not help.
In fact I saw two different issues when calling the streamoff event.
One issue was a stalled pipeline after the streamoff from the host came in.
The streaming application did not handle any events anymore.
The second issue was when the streamoff event is triggered sometimes the
following trace is shown, even with your patches applied.
[ 104.202689] Unable to handle kernel paging request at virtual address 005bf43a692a5fd5
[ 104.235122] Mem abort info:
[ 104.238257] ESR = 0x0000000096000004
[ 104.242449] EC = 0x25: DABT (current EL), IL = 32 bits
[ 104.248391] SET = 0, FnV = 0
[ 104.251803] EA = 0, S1PTW = 0
[ 104.255313] FSC = 0x04: level 0 translation fault
[ 104.260765] Data abort info:
[ 104.263982] ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000
[ 104.270114] CM = 0, WnR = 0, TnD = 0, TagAccess = 0
[ 104.275760] GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
[ 104.281698] [005bf43a692a5fd5] address between user and kernel address ranges
[ 104.290042] Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
[ 104.297060] Dumping ftrace buffer:
[ 104.300869] (ftrace buffer empty)
[ 104.304862] Modules linked in: st1232 hantro_vpu v4l2_vp9 v4l2_h264 uio_pdrv_genirq fuse [last unloaded: rockchip_vpu(C)]
[ 104.312080] panfrost fde60000.gpu: Panfrost Dump: BO has no sgt, cannot dump
[ 104.317137] CPU: 0 PID: 465 Comm: irq/46-dwc3 Tainted: G C 6.5.0-20230831-2+ #5
[ 104.317144] Hardware name: WolfVision PF5 (DT)
[ 104.317148] pstate: 604000c9 (nZCv daIF +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[ 104.317154] pc : __list_del_entry_valid+0x48/0xe8
[ 104.352728] lr : dwc3_gadget_giveback+0x3c/0x1b0
[ 104.357893] sp : ffffffc08381bc60
[ 104.361593] x29: ffffffc08381bc60 x28: ffffff80047d4000 x27: ffffff80047de440
[ 104.369576] x26: 0000000000000000 x25: ffffffc08135b2d0 x24: ffffffc08381bd00
[ 104.377559] x23: 00000000ffffff98 x22: ffffff8004204880 x21: ffffff80047d4000
[ 104.385541] x20: ffffff800718dea0 x19: ffffff800718dea0 x18: 0000000000000000
[ 104.393523] x17: 7461747320687469 x16: 7720646574656c70 x15: 6d6f632074736575
[ 104.401504] x14: 716572205356203a x13: 2e3430312d207375 x12: 7461747320687469
[ 104.409486] x11: ffffffc0815c98f0 x10: 0000000000000000 x9 : ffffffc0808f4fa0
[ 104.417468] x8 : ffffffc082415000 x7 : ffffffc0808f4e2c x6 : ffffffc0823d0928
[ 104.425450] x5 : 0000000000000282 x4 : 0000000000000201 x3 : d85bf43a692a5fcd
[ 104.433431] x2 : ffffff80047d4048 x1 : ffffff800718dea0 x0 : dead000000000122
[ 104.441413] Call trace:
[ 104.444142] __list_del_entry_valid+0x48/0xe8
[ 104.449013] dwc3_gadget_giveback+0x3c/0x1b0
[ 104.453786] dwc3_gadget_ep_cleanup_cancelled_requests+0xe0/0x170
[ 104.460599] dwc3_process_event_buf+0x2a8/0xbb0
[ 104.465662] dwc3_thread_interrupt+0x4c/0x90
[ 104.470435] irq_thread_fn+0x34/0xb8
[ 104.474431] irq_thread+0x1a0/0x290
[ 104.478327] kthread+0x10c/0x120
[ 104.481933] ret_from_fork+0x10/0x20
The error path triggering these list errors are usually in the
dwc3 driver handling the cancelled or completed list.
Regards,
Michael
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On 9/15/23 16:16, Michael Grzeschik wrote:
> Hi Avichal
>
> On Thu, Sep 14, 2023 at 04:05:36PM -0700, Avichal Rakesh wrote:
>> On Mon, Sep 11, 2023 at 9:19 PM Avichal Rakesh <[email protected]> wrote:
>>>
>>> We have been seeing two main bugs when stopping stream:
>>> 1. attempting to queue usb_requests on a disabled usb endpoint, and
>>> 2. use-after-free problems for inflight requests
>>>
>>> Avichal Rakesh (2):
>>> usb: gadget: uvc: prevent use of disabled endpoint
>>> usb: gadget: uvc: prevent de-allocating inflight usb_requests
>>>
>>> drivers/usb/gadget/function/f_uvc.c | 11 ++++----
>>> drivers/usb/gadget/function/f_uvc.h | 2 +-
>>> drivers/usb/gadget/function/uvc.h | 5 +++-
>>> drivers/usb/gadget/function/uvc_v4l2.c | 21 ++++++++++++---
>>> drivers/usb/gadget/function/uvc_video.c | 34 +++++++++++++++++++++++--
>>> 5 files changed, 60 insertions(+), 13 deletions(-)
>>>
>>
>> Bumping this thread up. Laurent, Dan, and Michael could you take a look?
>
> I tested the patches against my setup and it did not help.
Thank you for testing the patch, I really appreciate your help in testing the
patches!
>
> In fact I saw two different issues when calling the streamoff event.
>
> One issue was a stalled pipeline after the streamoff from the host came in.
> The streaming application did not handle any events anymore.
This sounds like uvc_function_setup_continue was never called, so no more control
events were queued for the userspace to handle. This is a little bit of a shot in
the dark: if you are not using the Laurent's uvc-gadget
(https://git.ideasonboard.org/uvc-gadget.git) on the gadget, could you check that
your userspace application (on the gadget side) calls VIDIOC_STREAMOFF when
handling UVC_EVENT_STREAMOFF?
This is similar to how it should called VIDIOC_STREAMON when handling
UVC_EVENT_STREAMON. Before my patch, I think UVC gadget driver is functionally fine
with userspace application not calling VIDIOC_STREAMOFF. However, my patch prevents
the host from making any more control requests until the gadget's userspace
application calls VIDIOC_STREAMOFF, which looks like a stalled control ep.
>
> The second issue was when the streamoff event is triggered sometimes the
> following trace is shown, even with your patches applied.
>
>
> [ 104.202689] Unable to handle kernel paging request at virtual address 005bf43a692a5fd5
> [ 104.235122] Mem abort info:
> [ 104.238257] ESR = 0x0000000096000004
> [ 104.242449] EC = 0x25: DABT (current EL), IL = 32 bits
> [ 104.248391] SET = 0, FnV = 0
> [ 104.251803] EA = 0, S1PTW = 0
> [ 104.255313] FSC = 0x04: level 0 translation fault
> [ 104.260765] Data abort info:
> [ 104.263982] ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000
> [ 104.270114] CM = 0, WnR = 0, TnD = 0, TagAccess = 0
> [ 104.275760] GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
> [ 104.281698] [005bf43a692a5fd5] address between user and kernel address ranges
> [ 104.290042] Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
> [ 104.297060] Dumping ftrace buffer:
> [ 104.300869] (ftrace buffer empty)
> [ 104.304862] Modules linked in: st1232 hantro_vpu v4l2_vp9 v4l2_h264 uio_pdrv_genirq fuse [last unloaded: rockchip_vpu(C)]
> [ 104.312080] panfrost fde60000.gpu: Panfrost Dump: BO has no sgt, cannot dump
> [ 104.317137] CPU: 0 PID: 465 Comm: irq/46-dwc3 Tainted: G C 6.5.0-20230831-2+ #5
> [ 104.317144] Hardware name: WolfVision PF5 (DT)
> [ 104.317148] pstate: 604000c9 (nZCv daIF +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [ 104.317154] pc : __list_del_entry_valid+0x48/0xe8
> [ 104.352728] lr : dwc3_gadget_giveback+0x3c/0x1b0
> [ 104.357893] sp : ffffffc08381bc60
> [ 104.361593] x29: ffffffc08381bc60 x28: ffffff80047d4000 x27: ffffff80047de440
> [ 104.369576] x26: 0000000000000000 x25: ffffffc08135b2d0 x24: ffffffc08381bd00
> [ 104.377559] x23: 00000000ffffff98 x22: ffffff8004204880 x21: ffffff80047d4000
> [ 104.385541] x20: ffffff800718dea0 x19: ffffff800718dea0 x18: 0000000000000000
> [ 104.393523] x17: 7461747320687469 x16: 7720646574656c70 x15: 6d6f632074736575
> [ 104.401504] x14: 716572205356203a x13: 2e3430312d207375 x12: 7461747320687469
> [ 104.409486] x11: ffffffc0815c98f0 x10: 0000000000000000 x9 : ffffffc0808f4fa0
> [ 104.417468] x8 : ffffffc082415000 x7 : ffffffc0808f4e2c x6 : ffffffc0823d0928
> [ 104.425450] x5 : 0000000000000282 x4 : 0000000000000201 x3 : d85bf43a692a5fcd
> [ 104.433431] x2 : ffffff80047d4048 x1 : ffffff800718dea0 x0 : dead000000000122
> [ 104.441413] Call trace:
> [ 104.444142] __list_del_entry_valid+0x48/0xe8
> [ 104.449013] dwc3_gadget_giveback+0x3c/0x1b0
> [ 104.453786] dwc3_gadget_ep_cleanup_cancelled_requests+0xe0/0x170
> [ 104.460599] dwc3_process_event_buf+0x2a8/0xbb0
> [ 104.465662] dwc3_thread_interrupt+0x4c/0x90
> [ 104.470435] irq_thread_fn+0x34/0xb8
> [ 104.474431] irq_thread+0x1a0/0x290
> [ 104.478327] kthread+0x10c/0x120
> [ 104.481933] ret_from_fork+0x10/0x20
>
> The error path triggering these list errors are usually in the
> dwc3 driver handling the cancelled or completed list.
It looks like we're still freeing un-returned requests :(. If you still have
the setup can you pull the uvc logs to see if waiting for requests to be returned timed
out? I wonder if dwc3's interrupt handler is being scheduled too late. 500ms seemed
like a reasonable time out to me, but this seems to prove otherwise.
Thank you!
- Avi.
On 9/15/23 18:35, Avichal Rakesh wrote:
>
>
> On 9/15/23 16:16, Michael Grzeschik wrote:
>> Hi Avichal
>>
>> On Thu, Sep 14, 2023 at 04:05:36PM -0700, Avichal Rakesh wrote:
>>> On Mon, Sep 11, 2023 at 9:19 PM Avichal Rakesh <[email protected]> wrote:
>>>>
>>>> We have been seeing two main bugs when stopping stream:
>>>> 1. attempting to queue usb_requests on a disabled usb endpoint, and
>>>> 2. use-after-free problems for inflight requests
>>>>
>>>> Avichal Rakesh (2):
>>>> usb: gadget: uvc: prevent use of disabled endpoint
>>>> usb: gadget: uvc: prevent de-allocating inflight usb_requests
>>>>
>>>> drivers/usb/gadget/function/f_uvc.c | 11 ++++----
>>>> drivers/usb/gadget/function/f_uvc.h | 2 +-
>>>> drivers/usb/gadget/function/uvc.h | 5 +++-
>>>> drivers/usb/gadget/function/uvc_v4l2.c | 21 ++++++++++++---
>>>> drivers/usb/gadget/function/uvc_video.c | 34 +++++++++++++++++++++++--
>>>> 5 files changed, 60 insertions(+), 13 deletions(-)
>>>>
>>>
>>> Bumping this thread up. Laurent, Dan, and Michael could you take a look?
>>
>> I tested the patches against my setup and it did not help.
>
> Thank you for testing the patch, I really appreciate your help in testing the
> patches!
>
>>
>> In fact I saw two different issues when calling the streamoff event.
>>
>> One issue was a stalled pipeline after the streamoff from the host came in.
>> The streaming application did not handle any events anymore.
>
> This sounds like uvc_function_setup_continue was never called, so no more control
> events were queued for the userspace to handle. This is a little bit of a shot in
> the dark: if you are not using the Laurent's uvc-gadget
> (https://git.ideasonboard.org/uvc-gadget.git) on the gadget, could you check that
> your userspace application (on the gadget side) calls VIDIOC_STREAMOFF when
> handling UVC_EVENT_STREAMOFF?
>
> This is similar to how it should called VIDIOC_STREAMON when handling
> UVC_EVENT_STREAMON. Before my patch, I think UVC gadget driver is functionally fine
> with userspace application not calling VIDIOC_STREAMOFF. However, my patch prevents
> the host from making any more control requests until the gadget's userspace
> application calls VIDIOC_STREAMOFF, which looks like a stalled control ep.
>
>>
>> The second issue was when the streamoff event is triggered sometimes the
>> following trace is shown, even with your patches applied.
>>
>>
>> [ 104.202689] Unable to handle kernel paging request at virtual address 005bf43a692a5fd5
>> [ 104.235122] Mem abort info:
>> [ 104.238257] ESR = 0x0000000096000004
>> [ 104.242449] EC = 0x25: DABT (current EL), IL = 32 bits
>> [ 104.248391] SET = 0, FnV = 0
>> [ 104.251803] EA = 0, S1PTW = 0
>> [ 104.255313] FSC = 0x04: level 0 translation fault
>> [ 104.260765] Data abort info:
>> [ 104.263982] ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000
>> [ 104.270114] CM = 0, WnR = 0, TnD = 0, TagAccess = 0
>> [ 104.275760] GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
>> [ 104.281698] [005bf43a692a5fd5] address between user and kernel address ranges
>> [ 104.290042] Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
>> [ 104.297060] Dumping ftrace buffer:
>> [ 104.300869] (ftrace buffer empty)
>> [ 104.304862] Modules linked in: st1232 hantro_vpu v4l2_vp9 v4l2_h264 uio_pdrv_genirq fuse [last unloaded: rockchip_vpu(C)]
>> [ 104.312080] panfrost fde60000.gpu: Panfrost Dump: BO has no sgt, cannot dump
>> [ 104.317137] CPU: 0 PID: 465 Comm: irq/46-dwc3 Tainted: G C 6.5.0-20230831-2+ #5
>> [ 104.317144] Hardware name: WolfVision PF5 (DT)
>> [ 104.317148] pstate: 604000c9 (nZCv daIF +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>> [ 104.317154] pc : __list_del_entry_valid+0x48/0xe8
>> [ 104.352728] lr : dwc3_gadget_giveback+0x3c/0x1b0
>> [ 104.357893] sp : ffffffc08381bc60
>> [ 104.361593] x29: ffffffc08381bc60 x28: ffffff80047d4000 x27: ffffff80047de440
>> [ 104.369576] x26: 0000000000000000 x25: ffffffc08135b2d0 x24: ffffffc08381bd00
>> [ 104.377559] x23: 00000000ffffff98 x22: ffffff8004204880 x21: ffffff80047d4000
>> [ 104.385541] x20: ffffff800718dea0 x19: ffffff800718dea0 x18: 0000000000000000
>> [ 104.393523] x17: 7461747320687469 x16: 7720646574656c70 x15: 6d6f632074736575
>> [ 104.401504] x14: 716572205356203a x13: 2e3430312d207375 x12: 7461747320687469
>> [ 104.409486] x11: ffffffc0815c98f0 x10: 0000000000000000 x9 : ffffffc0808f4fa0
>> [ 104.417468] x8 : ffffffc082415000 x7 : ffffffc0808f4e2c x6 : ffffffc0823d0928
>> [ 104.425450] x5 : 0000000000000282 x4 : 0000000000000201 x3 : d85bf43a692a5fcd
>> [ 104.433431] x2 : ffffff80047d4048 x1 : ffffff800718dea0 x0 : dead000000000122
>> [ 104.441413] Call trace:
>> [ 104.444142] __list_del_entry_valid+0x48/0xe8
>> [ 104.449013] dwc3_gadget_giveback+0x3c/0x1b0
>> [ 104.453786] dwc3_gadget_ep_cleanup_cancelled_requests+0xe0/0x170
>> [ 104.460599] dwc3_process_event_buf+0x2a8/0xbb0
>> [ 104.465662] dwc3_thread_interrupt+0x4c/0x90
>> [ 104.470435] irq_thread_fn+0x34/0xb8
>> [ 104.474431] irq_thread+0x1a0/0x290
>> [ 104.478327] kthread+0x10c/0x120
>> [ 104.481933] ret_from_fork+0x10/0x20
>>
>> The error path triggering these list errors are usually in the
>> dwc3 driver handling the cancelled or completed list.
>
> It looks like we're still freeing un-returned requests :(. If you still have
> the setup can you pull the uvc logs to see if waiting for requests to be returned timed
> out? I wonder if dwc3's interrupt handler is being scheduled too late. 500ms seemed
> like a reasonable time out to me, but this seems to prove otherwise.
>
Hey Michael, were you able to look into the comments from the previous
email?
Avi.
Currently, when stopping the stream, uvcg_video_enable immediately
deallocates the usb_requests after calling usb_ep_dequeue. However,
usb_ep_dequeue is asynchronous and it is possible that it a request
that is still in use by the controller. This can lead to some hard
to reproduce crashes with double frees and general memory
inconsistencies.
This patch sets up some stronger guarantees around when a request should
be deallocated. To that extent, this patch does the following:
1. When stream is stopped only the currently owned uvc_requests are freed
and all in-flight uvc_requests are marked 'abandoned'
2. uvc_video_complete callback is made responsible for freeing up the
abandoned requests. No uvc specific logic is triggered when handling
abandoned requests.
This ensures that the ownership of uvc_request (and its corresponding
usb_request) is never ambiguous, and uvc_requests are always freed
regardless of when the requests are returned to the gadget driver.
Other changes in the patch are required to decouple the allocated
uvc_requests from uvc_video that allocated them.
Link: https://lore.kernel.org/[email protected]
Suggested-by: Michael Grzeschik <[email protected]>
Signed-off-by: Avichal Rakesh <[email protected]>
---
v1 -> v2: Switched from a timed wait to free on complete model based
on discussions with Michael.
drivers/usb/gadget/function/uvc.h | 4 +-
drivers/usb/gadget/function/uvc_video.c | 182 +++++++++++++++++-------
2 files changed, 133 insertions(+), 53 deletions(-)
diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
index 989bc6b4e93d..e69cfb7cced1 100644
--- a/drivers/usb/gadget/function/uvc.h
+++ b/drivers/usb/gadget/function/uvc.h
@@ -81,6 +81,8 @@ struct uvc_request {
struct sg_table sgt;
u8 header[UVCG_REQUEST_HEADER_LEN];
struct uvc_buffer *last_buf;
+ struct list_head list;
+ bool is_abandoned;
};
struct uvc_video {
@@ -102,7 +104,7 @@ struct uvc_video {
/* Requests */
unsigned int req_size;
- struct uvc_request *ureq;
+ struct list_head ureqs; /* all uvc_requests allocated by uvc_video */
struct list_head req_free;
spinlock_t req_lock;
diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
index 70ff88854539..e999bf5bd458 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -227,6 +227,23 @@ uvc_video_encode_isoc(struct usb_request *req, struct uvc_video *video,
* Request handling
*/
+static void uvc_video_free_request(struct uvc_request *ureq, struct usb_ep *ep)
+{
+ sg_free_table(&ureq->sgt);
+ if (ureq->req && ep) {
+ usb_ep_free_request(ep, ureq->req);
+ ureq->req = NULL;
+ }
+
+ kfree(ureq->req_buffer);
+ ureq->req_buffer = NULL;
+
+ if (!list_empty(&ureq->list))
+ list_del(&ureq->list);
+
+ kfree(ureq);
+}
+
static int uvcg_video_ep_queue(struct uvc_video *video, struct usb_request *req)
{
int ret;
@@ -254,7 +271,21 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
struct uvc_video *video = ureq->video;
struct uvc_video_queue *queue = &video->queue;
struct uvc_device *uvc = video->uvc;
+ struct uvc_buffer *last_buf;
unsigned long flags;
+ bool is_abandoned;
+
+ spin_lock_irqsave(&video->req_lock, flags);
+ is_abandoned = ureq->is_abandoned;
+ last_buf = ureq->last_buf;
+ ureq->last_buf = NULL;
+ spin_unlock_irqrestore(&video->req_lock, flags);
+
+ if (is_abandoned) {
+ uvcg_dbg(&video->uvc->func, "Freeing abandoned request\n");
+ uvc_video_free_request(ureq, ep);
+ return;
+ }
switch (req->status) {
case 0:
@@ -277,15 +308,27 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
uvcg_queue_cancel(queue, 0);
}
- if (ureq->last_buf) {
- uvcg_complete_buffer(&video->queue, ureq->last_buf);
- ureq->last_buf = NULL;
+ if (last_buf) {
+ spin_lock_irqsave(&video->queue.irqlock, flags);
+ uvcg_complete_buffer(&video->queue, last_buf);
+ spin_unlock_irqrestore(&video->queue.irqlock, flags);
}
+ /*
+ * request might have been abandoned while being processed.
+ * do a last minute check before queueing the request back.
+ */
spin_lock_irqsave(&video->req_lock, flags);
- list_add_tail(&req->list, &video->req_free);
+ is_abandoned = ureq->is_abandoned;
+ if (!is_abandoned)
+ list_add_tail(&req->list, &video->req_free);
spin_unlock_irqrestore(&video->req_lock, flags);
+ if (is_abandoned) {
+ uvc_video_free_request(ureq, ep);
+ return;
+ }
+
if (uvc->state == UVC_STATE_STREAMING)
queue_work(video->async_wq, &video->pump);
}
@@ -293,25 +336,10 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
static int
uvc_video_free_requests(struct uvc_video *video)
{
- unsigned int i;
-
- if (video->ureq) {
- for (i = 0; i < video->uvc_num_requests; ++i) {
- sg_free_table(&video->ureq[i].sgt);
+ struct uvc_request *ureq, *temp;
- if (video->ureq[i].req) {
- usb_ep_free_request(video->ep, video->ureq[i].req);
- video->ureq[i].req = NULL;
- }
-
- if (video->ureq[i].req_buffer) {
- kfree(video->ureq[i].req_buffer);
- video->ureq[i].req_buffer = NULL;
- }
- }
-
- kfree(video->ureq);
- video->ureq = NULL;
+ list_for_each_entry_safe(ureq, temp, &video->ureqs, list) {
+ uvc_video_free_request(ureq, video->ep);
}
INIT_LIST_HEAD(&video->req_free);
@@ -322,6 +350,7 @@ uvc_video_free_requests(struct uvc_video *video)
static int
uvc_video_alloc_requests(struct uvc_video *video)
{
+ struct uvc_request *ureq;
unsigned int req_size;
unsigned int i;
int ret = -ENOMEM;
@@ -332,35 +361,36 @@ uvc_video_alloc_requests(struct uvc_video *video)
* max_t(unsigned int, video->ep->maxburst, 1)
* (video->ep->mult);
- video->ureq = kcalloc(video->uvc_num_requests, sizeof(struct uvc_request), GFP_KERNEL);
- if (video->ureq == NULL)
- return -ENOMEM;
-
- for (i = 0; i < video->uvc_num_requests; ++i) {
- video->ureq[i].req_buffer = kmalloc(req_size, GFP_KERNEL);
- if (video->ureq[i].req_buffer == NULL)
+ INIT_LIST_HEAD(&video->ureqs);
+ for (i = 0; i < video->uvc_num_requests; i++) {
+ ureq = kzalloc(sizeof(struct uvc_request), GFP_KERNEL);
+ if (ureq == NULL)
goto error;
+ INIT_LIST_HEAD(&ureq->list);
+ list_add_tail(&ureq->list, &video->ureqs);
+ }
- video->ureq[i].req = usb_ep_alloc_request(video->ep, GFP_KERNEL);
- if (video->ureq[i].req == NULL)
+ list_for_each_entry(ureq, &video->ureqs, list) {
+ ureq->req_buffer = kmalloc(req_size, GFP_KERNEL);
+ if (ureq->req_buffer == NULL)
goto error;
-
- video->ureq[i].req->buf = video->ureq[i].req_buffer;
- video->ureq[i].req->length = 0;
- video->ureq[i].req->complete = uvc_video_complete;
- video->ureq[i].req->context = &video->ureq[i];
- video->ureq[i].video = video;
- video->ureq[i].last_buf = NULL;
-
- list_add_tail(&video->ureq[i].req->list, &video->req_free);
+ ureq->req = usb_ep_alloc_request(video->ep, GFP_KERNEL);
+ if (ureq->req == NULL)
+ goto error;
+ ureq->req->buf = ureq->req_buffer;
+ ureq->req->length = 0;
+ ureq->req->complete = uvc_video_complete;
+ ureq->req->context = ureq;
+ ureq->video = video;
+ ureq->last_buf = NULL;
+ list_add_tail(&ureq->req->list, &video->req_free);
/* req_size/PAGE_SIZE + 1 for overruns and + 1 for header */
- sg_alloc_table(&video->ureq[i].sgt,
+ sg_alloc_table(&ureq->sgt,
DIV_ROUND_UP(req_size - UVCG_REQUEST_HEADER_LEN,
PAGE_SIZE) + 2, GFP_KERNEL);
}
video->req_size = req_size;
-
return 0;
error:
@@ -484,12 +514,68 @@ static void uvcg_video_pump(struct work_struct *work)
return;
}
+/*
+ * Disable video stream. This ensures that any inflight usb requests are marked
+ * for clean up and video buffers are dropped up before returning.
+ */
+static void uvcg_video_disable(struct uvc_video *video)
+{
+ struct uvc_buffer *buf, *tmp_buf;
+ struct uvc_request *ureq, *temp;
+ struct list_head buf_list; /* track in-flight video buffers */
+ struct usb_request *req;
+ unsigned long flags;
+
+ cancel_work_sync(&video->pump);
+ uvcg_queue_cancel(&video->queue, 0);
+
+ INIT_LIST_HEAD(&buf_list);
+ spin_lock_irqsave(&video->req_lock, flags);
+ /* abandon all usb requests */
+ list_for_each_entry_safe(ureq, temp, &video->ureqs, list) {
+ ureq->is_abandoned = true;
+ if (ureq->last_buf) {
+ list_add(&ureq->last_buf->queue, &buf_list);
+ ureq->last_buf = NULL;
+ }
+ list_del_init(&ureq->list);
+ if (ureq->req)
+ usb_ep_dequeue(video->ep, ureq->req);
+ }
+ /*
+ * re-add uvc_requests currently owned by the gadget to
+ * video->ureqs to be deallocated
+ */
+ list_for_each_entry(req, &video->req_free, list) {
+ ureq = req->context;
+ list_add_tail(&ureq->list, &video->ureqs);
+ }
+ spin_unlock_irqrestore(&video->req_lock, flags);
+
+ /*
+ * drop abandoned uvc_buffers, as the completion handler
+ * no longer will
+ */
+ if (!list_empty(&buf_list)) {
+ spin_lock_irqsave(&video->queue.irqlock, flags);
+ list_for_each_entry_safe(buf, tmp_buf,
+ &buf_list, queue) {
+ video->queue.flags |= UVC_QUEUE_DROP_INCOMPLETE;
+ uvcg_complete_buffer(&video->queue, buf);
+ list_del(&buf->queue);
+ }
+ spin_unlock_irqrestore(&video->queue.irqlock, flags);
+ }
+
+ uvc_video_free_requests(video);
+ uvcg_queue_enable(&video->queue, 0);
+}
+
/*
* Enable or disable the video stream.
*/
int uvcg_video_enable(struct uvc_video *video, int enable)
{
- unsigned int i;
int ret;
if (video->ep == NULL) {
@@ -499,15 +585,7 @@ int uvcg_video_enable(struct uvc_video *video, int enable)
}
if (!enable) {
- cancel_work_sync(&video->pump);
- uvcg_queue_cancel(&video->queue, 0);
-
- for (i = 0; i < video->uvc_num_requests; ++i)
- if (video->ureq && video->ureq[i].req)
- usb_ep_dequeue(video->ep, video->ureq[i].req);
-
- uvc_video_free_requests(video);
- uvcg_queue_enable(&video->queue, 0);
+ uvcg_video_disable(video);
return 0;
}
--
2.42.0.459.ge4e396fd5e-goog
On 9/19/23 11:24, Avichal Rakesh wrote:
>
>
> On 9/15/23 18:35, Avichal Rakesh wrote:
>>
>>
>> On 9/15/23 16:16, Michael Grzeschik wrote:
>>> Hi Avichal
>>>
>>> On Thu, Sep 14, 2023 at 04:05:36PM -0700, Avichal Rakesh wrote:
>>>> On Mon, Sep 11, 2023 at 9:19 PM Avichal Rakesh <[email protected]> wrote:
>>>>>
>>>>> We have been seeing two main bugs when stopping stream:
>>>>> 1. attempting to queue usb_requests on a disabled usb endpoint, and
>>>>> 2. use-after-free problems for inflight requests
>>>>>
>>>
>>> The error path triggering these list errors are usually in the
>>> dwc3 driver handling the cancelled or completed list.
>>
>> It looks like we're still freeing un-returned requests :(. If you still have
>> the setup can you pull the uvc logs to see if waiting for requests to be returned timed
>> out? I wonder if dwc3's interrupt handler is being scheduled too late. 500ms seemed
>> like a reasonable time out to me, but this seems to prove otherwise.
>>
>
>
> Hey Michael, were you able to look into the comments from the previous
> email?
>
Bumping the thread up. Laurent, Dan, and Michael: the patches are ready to
review/test. Please take a look when possible.
Thank you!
Avi.
Currently, when stopping the stream, uvcg_video_enable immediately
deallocates the usb_requests after calling usb_ep_dequeue. However,
usb_ep_dequeue is asynchronous and it is possible that it a request
that is still in use by the controller. This can lead to some hard
to reproduce crashes with double frees and general memory
inconsistencies.
This patch sets up some stronger guarantees around when a request should
be deallocated. To that extent, this patch does the following:
1. When stream is stopped only the currently owned uvc_requests are freed
and all in-flight uvc_requests are marked 'abandoned'
2. uvc_video_complete callback is made responsible for freeing up the
abandoned requests. No uvc specific logic is triggered when handling
abandoned requests.
This ensures that the ownership of uvc_request (and its corresponding
usb_request) is never ambiguous, and uvc_requests are always freed
regardless of when the requests are returned to the gadget driver.
Other changes in the patch are required to decouple the allocated
uvc_requests from uvc_video that allocated them.
Link: https://lore.kernel.org/[email protected]
Suggested-by: Michael Grzeschik <[email protected]>
Signed-off-by: Avichal Rakesh <[email protected]>
---
v1 -> v2: Switched from a timed wait to free on complete model based
on discussions with Michael.
v3 -> v3: Initialized ureqs at init to prevent accessing invalid
memory when no stream is created.
drivers/usb/gadget/function/uvc.h | 4 +-
drivers/usb/gadget/function/uvc_video.c | 183 +++++++++++++++++-------
2 files changed, 134 insertions(+), 53 deletions(-)
diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
index 989bc6b4e93d..e69cfb7cced1 100644
--- a/drivers/usb/gadget/function/uvc.h
+++ b/drivers/usb/gadget/function/uvc.h
@@ -81,6 +81,8 @@ struct uvc_request {
struct sg_table sgt;
u8 header[UVCG_REQUEST_HEADER_LEN];
struct uvc_buffer *last_buf;
+ struct list_head list;
+ bool is_abandoned;
};
struct uvc_video {
@@ -102,7 +104,7 @@ struct uvc_video {
/* Requests */
unsigned int req_size;
- struct uvc_request *ureq;
+ struct list_head ureqs; /* all uvc_requests allocated by uvc_video */
struct list_head req_free;
spinlock_t req_lock;
diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
index 70ff88854539..aa5ef63e5540 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -227,6 +227,23 @@ uvc_video_encode_isoc(struct usb_request *req, struct uvc_video *video,
* Request handling
*/
+static void uvc_video_free_request(struct uvc_request *ureq, struct usb_ep *ep)
+{
+ sg_free_table(&ureq->sgt);
+ if (ureq->req && ep) {
+ usb_ep_free_request(ep, ureq->req);
+ ureq->req = NULL;
+ }
+
+ kfree(ureq->req_buffer);
+ ureq->req_buffer = NULL;
+
+ if (!list_empty(&ureq->list))
+ list_del(&ureq->list);
+
+ kfree(ureq);
+}
+
static int uvcg_video_ep_queue(struct uvc_video *video, struct usb_request *req)
{
int ret;
@@ -254,7 +271,21 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
struct uvc_video *video = ureq->video;
struct uvc_video_queue *queue = &video->queue;
struct uvc_device *uvc = video->uvc;
+ struct uvc_buffer *last_buf;
unsigned long flags;
+ bool is_abandoned;
+
+ spin_lock_irqsave(&video->req_lock, flags);
+ is_abandoned = ureq->is_abandoned;
+ last_buf = ureq->last_buf;
+ ureq->last_buf = NULL;
+ spin_unlock_irqrestore(&video->req_lock, flags);
+
+ if (is_abandoned) {
+ uvcg_dbg(&video->uvc->func, "Freeing abandoned request\n");
+ uvc_video_free_request(ureq, ep);
+ return;
+ }
switch (req->status) {
case 0:
@@ -277,15 +308,27 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
uvcg_queue_cancel(queue, 0);
}
- if (ureq->last_buf) {
- uvcg_complete_buffer(&video->queue, ureq->last_buf);
- ureq->last_buf = NULL;
+ if (last_buf) {
+ spin_lock_irqsave(&video->queue.irqlock, flags);
+ uvcg_complete_buffer(&video->queue, last_buf);
+ spin_unlock_irqrestore(&video->queue.irqlock, flags);
}
+ /*
+ * request might have been abandoned while being processed.
+ * do a last minute check before queueing the request back.
+ */
spin_lock_irqsave(&video->req_lock, flags);
- list_add_tail(&req->list, &video->req_free);
+ is_abandoned = ureq->is_abandoned;
+ if (!is_abandoned)
+ list_add_tail(&req->list, &video->req_free);
spin_unlock_irqrestore(&video->req_lock, flags);
+ if (is_abandoned) {
+ uvc_video_free_request(ureq, ep);
+ return;
+ }
+
if (uvc->state == UVC_STATE_STREAMING)
queue_work(video->async_wq, &video->pump);
}
@@ -293,25 +336,10 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
static int
uvc_video_free_requests(struct uvc_video *video)
{
- unsigned int i;
-
- if (video->ureq) {
- for (i = 0; i < video->uvc_num_requests; ++i) {
- sg_free_table(&video->ureq[i].sgt);
+ struct uvc_request *ureq, *temp;
- if (video->ureq[i].req) {
- usb_ep_free_request(video->ep, video->ureq[i].req);
- video->ureq[i].req = NULL;
- }
-
- if (video->ureq[i].req_buffer) {
- kfree(video->ureq[i].req_buffer);
- video->ureq[i].req_buffer = NULL;
- }
- }
-
- kfree(video->ureq);
- video->ureq = NULL;
+ list_for_each_entry_safe(ureq, temp, &video->ureqs, list) {
+ uvc_video_free_request(ureq, video->ep);
}
INIT_LIST_HEAD(&video->req_free);
@@ -322,6 +350,7 @@ uvc_video_free_requests(struct uvc_video *video)
static int
uvc_video_alloc_requests(struct uvc_video *video)
{
+ struct uvc_request *ureq;
unsigned int req_size;
unsigned int i;
int ret = -ENOMEM;
@@ -332,35 +361,36 @@ uvc_video_alloc_requests(struct uvc_video *video)
* max_t(unsigned int, video->ep->maxburst, 1)
* (video->ep->mult);
- video->ureq = kcalloc(video->uvc_num_requests, sizeof(struct uvc_request), GFP_KERNEL);
- if (video->ureq == NULL)
- return -ENOMEM;
-
- for (i = 0; i < video->uvc_num_requests; ++i) {
- video->ureq[i].req_buffer = kmalloc(req_size, GFP_KERNEL);
- if (video->ureq[i].req_buffer == NULL)
+ INIT_LIST_HEAD(&video->ureqs);
+ for (i = 0; i < video->uvc_num_requests; i++) {
+ ureq = kzalloc(sizeof(struct uvc_request), GFP_KERNEL);
+ if (ureq == NULL)
goto error;
+ INIT_LIST_HEAD(&ureq->list);
+ list_add_tail(&ureq->list, &video->ureqs);
+ }
- video->ureq[i].req = usb_ep_alloc_request(video->ep, GFP_KERNEL);
- if (video->ureq[i].req == NULL)
+ list_for_each_entry(ureq, &video->ureqs, list) {
+ ureq->req_buffer = kmalloc(req_size, GFP_KERNEL);
+ if (ureq->req_buffer == NULL)
goto error;
-
- video->ureq[i].req->buf = video->ureq[i].req_buffer;
- video->ureq[i].req->length = 0;
- video->ureq[i].req->complete = uvc_video_complete;
- video->ureq[i].req->context = &video->ureq[i];
- video->ureq[i].video = video;
- video->ureq[i].last_buf = NULL;
-
- list_add_tail(&video->ureq[i].req->list, &video->req_free);
+ ureq->req = usb_ep_alloc_request(video->ep, GFP_KERNEL);
+ if (ureq->req == NULL)
+ goto error;
+ ureq->req->buf = ureq->req_buffer;
+ ureq->req->length = 0;
+ ureq->req->complete = uvc_video_complete;
+ ureq->req->context = ureq;
+ ureq->video = video;
+ ureq->last_buf = NULL;
+ list_add_tail(&ureq->req->list, &video->req_free);
/* req_size/PAGE_SIZE + 1 for overruns and + 1 for header */
- sg_alloc_table(&video->ureq[i].sgt,
+ sg_alloc_table(&ureq->sgt,
DIV_ROUND_UP(req_size - UVCG_REQUEST_HEADER_LEN,
PAGE_SIZE) + 2, GFP_KERNEL);
}
video->req_size = req_size;
-
return 0;
error:
@@ -484,12 +514,68 @@ static void uvcg_video_pump(struct work_struct *work)
return;
}
+/*
+ * Disable video stream. This ensures that any inflight usb requests are marked
+ * for clean up and video buffers are dropped up before returning.
+ */
+static void uvcg_video_disable(struct uvc_video *video)
+{
+ struct uvc_buffer *buf, *tmp_buf;
+ struct uvc_request *ureq, *temp;
+ struct list_head buf_list; /* track in-flight video buffers */
+ struct usb_request *req;
+ unsigned long flags;
+
+ cancel_work_sync(&video->pump);
+ uvcg_queue_cancel(&video->queue, 0);
+
+ INIT_LIST_HEAD(&buf_list);
+ spin_lock_irqsave(&video->req_lock, flags);
+ /* abandon all usb requests */
+ list_for_each_entry_safe(ureq, temp, &video->ureqs, list) {
+ ureq->is_abandoned = true;
+ if (ureq->last_buf) {
+ list_add(&ureq->last_buf->queue, &buf_list);
+ ureq->last_buf = NULL;
+ }
+ list_del_init(&ureq->list);
+ if (ureq->req)
+ usb_ep_dequeue(video->ep, ureq->req);
+ }
+ /*
+ * re-add uvc_requests currently owned by the gadget to
+ * video->ureqs to be deallocated
+ */
+ list_for_each_entry(req, &video->req_free, list) {
+ ureq = req->context;
+ list_add_tail(&ureq->list, &video->ureqs);
+ }
+ spin_unlock_irqrestore(&video->req_lock, flags);
+
+ /*
+ * drop abandoned uvc_buffers, as the completion handler
+ * no longer will
+ */
+ if (!list_empty(&buf_list)) {
+ spin_lock_irqsave(&video->queue.irqlock, flags);
+ list_for_each_entry_safe(buf, tmp_buf,
+ &buf_list, queue) {
+ video->queue.flags |= UVC_QUEUE_DROP_INCOMPLETE;
+ uvcg_complete_buffer(&video->queue, buf);
+ list_del(&buf->queue);
+ }
+ spin_unlock_irqrestore(&video->queue.irqlock, flags);
+ }
+
+ uvc_video_free_requests(video);
+ uvcg_queue_enable(&video->queue, 0);
+}
+
/*
* Enable or disable the video stream.
*/
int uvcg_video_enable(struct uvc_video *video, int enable)
{
- unsigned int i;
int ret;
if (video->ep == NULL) {
@@ -499,15 +585,7 @@ int uvcg_video_enable(struct uvc_video *video, int enable)
}
if (!enable) {
- cancel_work_sync(&video->pump);
- uvcg_queue_cancel(&video->queue, 0);
-
- for (i = 0; i < video->uvc_num_requests; ++i)
- if (video->ureq && video->ureq[i].req)
- usb_ep_dequeue(video->ep, video->ureq[i].req);
-
- uvc_video_free_requests(video);
- uvcg_queue_enable(&video->queue, 0);
+ uvcg_video_disable(video);
return 0;
}
@@ -536,6 +614,7 @@ int uvcg_video_enable(struct uvc_video *video, int enable)
*/
int uvcg_video_init(struct uvc_video *video, struct uvc_device *uvc)
{
+ INIT_LIST_HEAD(&video->ureqs);
INIT_LIST_HEAD(&video->req_free);
spin_lock_init(&video->req_lock);
INIT_WORK(&video->pump, uvcg_video_pump);
--
2.42.0.582.g8ccd20d70d-goog