| Message ID | 20210930102717.15720-6-m.grzeschik@pengutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: gadget: uvc: smaller fixes for stability | expand |
Hi Michael, On Thu, Sep 30, 2021 at 12:27:15PM +0200, Michael Grzeschik wrote: > If the streaming endpoint is not enabled, the worker has nothing to do. > In the case it still got enqueued, this patch ensueres that it will bail > out without handling any data. > > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > --- > drivers/usb/gadget/function/uvc_video.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c > index ccee35177411d..152647495fa61 100644 > --- a/drivers/usb/gadget/function/uvc_video.c > +++ b/drivers/usb/gadget/function/uvc_video.c > @@ -335,12 +335,12 @@ 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 usb_request *req; > + struct usb_request *req = NULL; > struct uvc_buffer *buf; > unsigned long flags; > int ret; > > - while (1) { > + while (video->ep->enabled) { > /* Retrieve the first available USB request, protected by the > * request lock. > */ > @@ -390,6 +390,9 @@ static void uvcg_video_pump(struct work_struct *work) > video->req_int_count++; > } > > + if (!req) > + return; > + > spin_lock_irqsave(&video->req_lock, flags); > list_add_tail(&req->list, &video->req_free); > spin_unlock_irqrestore(&video->req_lock, flags); > -- > 2.30.2 >
Hi Michael, Thank you for the patch. On Thu, Sep 30, 2021 at 12:27:15PM +0200, Michael Grzeschik wrote: > If the streaming endpoint is not enabled, the worker has nothing to do. > In the case it still got enqueued, this patch ensueres that it will bail s/it still got enqueued/buffers are still queued/ s/ensueres/ensures/ > out without handling any data. Does this happen when uvc_function_set_alt() calls usb_ep_disable() ? The current implementation will cause usb_ep_queue() (called from uvcg_video_ep_queue(), from uvcg_video_pump()) to return an error in that case, which will result in uvcg_queue_cancel() being called in uvcg_video_pump(). With this patch, I believe this will still work fine as userspace is notified of the stream off event and calls VIDIOC_STREAMOFF, which in turn calls uvcg_video_enable(0) from uvc_v4l2_streamoff() (or uvcg_video_enable(0) gets called from uvc_v4l2_release() in the worst case if the application closes the device). Could you confirm that your understanding matches this analysis ? If so, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> > --- > drivers/usb/gadget/function/uvc_video.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c > index ccee35177411d..152647495fa61 100644 > --- a/drivers/usb/gadget/function/uvc_video.c > +++ b/drivers/usb/gadget/function/uvc_video.c > @@ -335,12 +335,12 @@ 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 usb_request *req; > + struct usb_request *req = NULL; > struct uvc_buffer *buf; > unsigned long flags; > int ret; > > - while (1) { > + while (video->ep->enabled) { > /* Retrieve the first available USB request, protected by the > * request lock. > */ > @@ -390,6 +390,9 @@ static void uvcg_video_pump(struct work_struct *work) > video->req_int_count++; > } > > + if (!req) > + return; > + > spin_lock_irqsave(&video->req_lock, flags); > list_add_tail(&req->list, &video->req_free); > spin_unlock_irqrestore(&video->req_lock, flags);
On Tue, Oct 05, 2021 at 01:29:25AM +0300, Laurent Pinchart wrote: >Hi Michael, > >Thank you for the patch. > >On Thu, Sep 30, 2021 at 12:27:15PM +0200, Michael Grzeschik wrote: >> If the streaming endpoint is not enabled, the worker has nothing to do. >> In the case it still got enqueued, this patch ensueres that it will bail > >s/it still got enqueued/buffers are still queued/ >s/ensueres/ensures/ Thanks, will fix this and your other comments on the patches in v3. >> out without handling any data. > >Does this happen when uvc_function_set_alt() calls usb_ep_disable() ? >The current implementation will cause usb_ep_queue() (called from >uvcg_video_ep_queue(), from uvcg_video_pump()) to return an error in >that case, which will result in uvcg_queue_cancel() being called in >uvcg_video_pump(). With this patch, I believe this will still work fine >as userspace is notified of the stream off event and calls >VIDIOC_STREAMOFF, which in turn calls uvcg_video_enable(0) from >uvc_v4l2_streamoff() (or uvcg_video_enable(0) gets called from >uvc_v4l2_release() in the worst case if the application closes the >device). Could you confirm that your understanding matches this analysis >? If so, Yes! I can confirm your analysis correct. > >Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> >> --- >> drivers/usb/gadget/function/uvc_video.c | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c >> index ccee35177411d..152647495fa61 100644 >> --- a/drivers/usb/gadget/function/uvc_video.c >> +++ b/drivers/usb/gadget/function/uvc_video.c >> @@ -335,12 +335,12 @@ 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 usb_request *req; >> + struct usb_request *req = NULL; >> struct uvc_buffer *buf; >> unsigned long flags; >> int ret; >> >> - while (1) { >> + while (video->ep->enabled) { >> /* Retrieve the first available USB request, protected by the >> * request lock. >> */ >> @@ -390,6 +390,9 @@ static void uvcg_video_pump(struct work_struct *work) >> video->req_int_count++; >> } >> >> + if (!req) >> + return; >> + >> spin_lock_irqsave(&video->req_lock, flags); >> list_add_tail(&req->list, &video->req_free); >> spin_unlock_irqrestore(&video->req_lock, flags); > >-- >Regards, > >Laurent Pinchart >
diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c index ccee35177411d..152647495fa61 100644 --- a/drivers/usb/gadget/function/uvc_video.c +++ b/drivers/usb/gadget/function/uvc_video.c @@ -335,12 +335,12 @@ 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 usb_request *req; + struct usb_request *req = NULL; struct uvc_buffer *buf; unsigned long flags; int ret; - while (1) { + while (video->ep->enabled) { /* Retrieve the first available USB request, protected by the * request lock. */ @@ -390,6 +390,9 @@ static void uvcg_video_pump(struct work_struct *work) video->req_int_count++; } + if (!req) + return; + spin_lock_irqsave(&video->req_lock, flags); list_add_tail(&req->list, &video->req_free); spin_unlock_irqrestore(&video->req_lock, flags);
If the streaming endpoint is not enabled, the worker has nothing to do. In the case it still got enqueued, this patch ensueres that it will bail out without handling any data. Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> --- drivers/usb/gadget/function/uvc_video.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)