[PATCH] usb: gadget: uvc: reserve frame interval headroom for the zero length backlog

[PATCH] usb: gadget: uvc: reserve frame interval headroom for the zero length backlog

[Daehwan Jung]

uvc_video_prep_requests() spreads a frame over every service interval of
the negotiated frame interval:

	nreq = DIV_ROUND_UP(video->interval, interval_duration);

and uvc_buffer_prepare() then splits every buffer into exactly
reqs_per_frame requests - only the request size varies, never the count.
So transmitting one frame always occupies nreq service intervals, 100% of
the frame interval, with nothing left over.

That budget cannot be met. While no video buffer is available,
uvcg_video_hw_submit() deliberately keeps a backlog of zero length
requests queued to the endpoint:

	if (list_empty(&video->req_free) ||
	    (atomic_read(&video->queued) > UVCG_REQ_MAX_ZERO_COUNT)) {

Up to UVCG_REQ_MAX_ZERO_COUNT + 1 of them are in flight whenever the
encoder is late, each occupying a service interval that carries no
payload, and the ring is FIFO so they all drain before the next frame's
payload moves. The frame period is therefore reqs_per_frame plus that
backlog, always longer than the frame interval the host asked for. The
UVCG_REQ_MAX_ZERO_COUNT compensation already in this function only sizes
the request pool; it buys the backlog no time.

At 60fps over SuperSpeed with bInterval 1 the backlog is 33 of the 133
intervals a frame interval has - 4.125ms, a quarter of the budget. The
waste is a fixed count, so it hurts in inverse proportion to the frame
interval: 24.8% at 60fps down to 6.2% at 15fps.

Reserve it. Measured on a SuperSpeed gadget (isoc IN, wMaxPacketSize
1024, bMaxBurst 2, bInterval 1) running the Android DeviceAsWebcam
service, as reported by the CTS Verifier "Camera - Webcam" test, next to
the rate the microframe accounting predicts:

  target   predicted  measured  |  predicted  measured
            (before)  (before)  |   (after)    (after)
    60        47.9       46     |    59.7        56
    30        26.7       26     |    30.0        28
    24        21.8       22     |    24.5        22
    15        14.1       13     |    16.0        13

Before, the prediction matches the measurement at every rate - this
function was the limit. After, 60 and 30fps move up with the budget,
while 24 and 15fps do not move at all even though their ceiling now
exceeds the target. At those two rates the remaining gap is on the buffer
supply side, not here.

req_size grows in proportion, 376 -> 495 bytes for the frames measured
here. That costs no isochronous bandwidth: it stays far inside what the
endpoint descriptor already reserves, so the clamp below is not reached
and the host does not have to re-reserve anything.

Fixes: 48dbe731171e ("usb: gadget: uvc: set req_size and n_requests based on the frame interval")
Signed-off-by: Daehwan Jung <dh10.jung@samsung.com>
Assisted-by: Claude:claude-opus-5
---
 drivers/usb/gadget/function/uvc.h       | 10 ++++++++++
 drivers/usb/gadget/function/uvc_video.c | 15 ++++++++++++++-
 2 files changed, 24 insertions(+), 1 deletion(-)

--- a/drivers/usb/gadget/function/uvc.h
+++ b/drivers/usb/gadget/function/uvc.h
@@ -74,6 +74,15 @@
 #define UVCG_REQ_MAX_INT_COUNT			16
 #define UVCG_REQ_MAX_ZERO_COUNT			(2 * UVCG_REQ_MAX_INT_COUNT)
 
+/*
+ * Service intervals of a frame interval that must not carry payload: they
+ * absorb the up to UVCG_REQ_MAX_ZERO_COUNT + 1 zero length requests that
+ * uvcg_video_hw_submit() keeps queued while it waits for a video buffer, or
+ * 1/UVCG_REQ_HEADROOM_FRACT of the frame interval, whichever is larger.
+ */
+#define UVCG_REQ_HEADROOM_COUNT			(UVCG_REQ_MAX_ZERO_COUNT + 1)
+#define UVCG_REQ_HEADROOM_FRACT			8
+
 #define UVCG_STREAMING_MIN_BUFFERS		2
 
 /* ------------------------------------------------------------------------
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -501,7 +501,7 @@
 	struct usb_composite_dev *cdev = uvc->func.config->cdev;
 	unsigned int interval_duration;
 	unsigned int max_req_size, req_size, header_size;
-	unsigned int nreq;
+	unsigned int nreq, headroom;
 
 	max_req_size = video->max_req_size;
 
@@ -521,6 +521,20 @@
 
 	nreq = DIV_ROUND_UP(video->interval, interval_duration);
 
+	/*
+	 * Not all of those intervals can carry payload: the zero length
+	 * requests uvcg_video_hw_submit() queues while it waits for a buffer
+	 * have to drain first, and uvc_buffer_prepare() splits a buffer into
+	 * exactly reqs_per_frame requests regardless. Reserve them, unless
+	 * that would claim more than half of the frame interval - a frame
+	 * interval that short cannot absorb the backlog either way, and
+	 * nreq - headroom would underflow.
+	 */
+	headroom = max_t(unsigned int, nreq / UVCG_REQ_HEADROOM_FRACT,
+			 UVCG_REQ_HEADROOM_COUNT);
+	if (nreq > 2 * headroom)
+		nreq -= headroom;
+
 	header_size = nreq * UVCG_REQUEST_HEADER_LEN;
 
 	req_size = DIV_ROUND_UP(video->imagesize + header_size, nreq);
-- 
2.43.0

Re: [PATCH] usb: gadget: uvc: reserve frame interval headroom for the zero length backlog

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] usb: gadget: uvc: Divide by zero in uvc_video_prep_requests()
- [High] usb: gadget: uvc: NULL pointer dereference if sg_alloc_table fails
--

commit 405aa67c114665f2574cee699b4f2eabe9b5de64
Author: Daehwan Jung <dh10.jung@samsung.com>

usb: gadget: uvc: reserve frame interval headroom for the zero length backlog

This commit modifies uvc_video_prep_requests() to reserve frame interval
headroom for zero length backlog requests. It achieves this by subtracting
headroom from nreq to ensure the processing budget can be met.

> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
> index 9ba09118bb74a..90882c88ddc6a 100644
> --- a/drivers/usb/gadget/function/uvc_video.c
> +++ b/drivers/usb/gadget/function/uvc_video.c
> @@ -521,6 +521,20 @@ uvc_video_prep_requests(struct uvc_video *video)
>  
>  	nreq = DIV_ROUND_UP(video->interval, interval_duration);
>  
> +	/*
> +	 * Not all of those intervals can carry payload: the zero length
> +	 * requests uvcg_video_hw_submit() queues while it waits for a buffer
> +	 * have to drain first, and uvc_buffer_prepare() splits a buffer into
> +	 * exactly reqs_per_frame requests regardless. Reserve them, unless
> +	 * that would claim more than half of the frame interval - a frame
> +	 * interval that short cannot absorb the backlog either way, and
> +	 * nreq - headroom would underflow.
> +	 */
> +	headroom = max_t(unsigned int, nreq / UVCG_REQ_HEADROOM_FRACT,
> +			 UVCG_REQ_HEADROOM_COUNT);
> +	if (nreq > 2 * headroom)
> +		nreq -= headroom;
> +
>  	header_size = nreq * UVCG_REQUEST_HEADER_LEN;
>  
>  	req_size = DIV_ROUND_UP(video->imagesize + header_size, nreq);

[Severity: High]
Does this code trigger a divide by zero if video->interval is 0?

Userspace can set a timeperframe numerator of 0 via VIDIOC_S_PARM, which
uvc_v4l2_s_parm() converts without bounds checking:

drivers/usb/gadget/function/uvc_v4l2.c:uvc_v4l2_s_parm() {
    ...
    video->interval = v4l2_fraction_to_interval(timeperframe.numerator,
                                                timeperframe.denominator);
    ...
}

If video->interval is 0, nreq will evaluate to 0 in uvc_video_prep_requests().
Since 0 > 2 * headroom evaluates to false, nreq remains 0. The subsequent
DIV_ROUND_UP() calculation will then attempt to divide by zero, causing a
kernel panic.

[Severity: High]
Can uvc_video_encode_isoc_sg() oops if sg_alloc_table() fails?

In uvc_video_alloc_requests(), the return value of sg_alloc_table() is
unchecked:

drivers/usb/gadget/function/uvc_video.c:uvc_video_alloc_requests() {
    ...
    sg_alloc_table(&ureq->sgt,
                   DIV_ROUND_UP(video->req_size - UVCG_REQUEST_HEADER_LEN,
                                PAGE_SIZE) + 2, GFP_KERNEL);
    ...
}

If the allocation fails, ureq->sgt.sgl remains NULL and nents is 0, but the
request is incorrectly considered valid and placed on the free list.

When queuing video buffers with video->queue.use_sg active, this request
will be used by uvc_video_encode_isoc_sg():

drivers/usb/gadget/function/uvc_video.c:uvc_video_encode_isoc_sg() {
    ...
    sg = ureq->sgt.sgl;
    sg_init_table(sg, ureq->sgt.nents);
    ...
}

This calls sg_init_table(NULL, 0), which eventually invokes
sg_mark_end(&sgl[nents - 1]). With nents = 0, this results in an
out-of-bounds dereference.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012034018.3193146-1-dh10.jung@samsung.com?part=1