| Message ID | 20180918103532.2961-1-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] usb: gadget: uvc: Factor out video USB request queueing | expand |
On Tue, Sep 18, 2018 at 01:35:30PM +0300, Laurent Pinchart wrote: > USB requests for video data are queued from two different locations in > the driver, with the same code block occurring twice. Factor it out to a > function. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> For the whole series: Looks good to me. Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> Tested-by: Paul Elder <paul.elder@ideasonboard.com> > --- > drivers/usb/gadget/function/uvc_video.c | 30 ++++++++++++++++++++---------- > 1 file changed, 20 insertions(+), 10 deletions(-) > > diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c > index d3567b90343a..a95c8e2364ed 100644 > --- a/drivers/usb/gadget/function/uvc_video.c > +++ b/drivers/usb/gadget/function/uvc_video.c > @@ -125,6 +125,19 @@ uvc_video_encode_isoc(struct usb_request *req, struct uvc_video *video, > * Request handling > */ > > +static int uvcg_video_ep_queue(struct uvc_video *video, struct usb_request *req) > +{ > + int ret; > + > + ret = usb_ep_queue(video->ep, req, GFP_ATOMIC); > + if (ret < 0) { > + printk(KERN_INFO "Failed to queue request (%d).\n", ret); > + usb_ep_set_halt(video->ep); > + } > + > + return ret; > +} > + > /* > * I somehow feel that synchronisation won't be easy to achieve here. We have > * three events that control USB requests submission: > @@ -189,14 +202,13 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req) > > video->encode(req, video, buf); > > - if ((ret = usb_ep_queue(ep, req, GFP_ATOMIC)) < 0) { > - printk(KERN_INFO "Failed to queue request (%d).\n", ret); > - usb_ep_set_halt(ep); > - spin_unlock_irqrestore(&video->queue.irqlock, flags); > + ret = uvcg_video_ep_queue(video, req); > + spin_unlock_irqrestore(&video->queue.irqlock, flags); > + > + if (ret < 0) { > uvcg_queue_cancel(queue, 0); > goto requeue; > } > - spin_unlock_irqrestore(&video->queue.irqlock, flags); > > return; > > @@ -316,15 +328,13 @@ int uvcg_video_pump(struct uvc_video *video) > video->encode(req, video, buf); > > /* Queue the USB request */ > - ret = usb_ep_queue(video->ep, req, GFP_ATOMIC); > + ret = uvcg_video_ep_queue(video, req); > + spin_unlock_irqrestore(&queue->irqlock, flags); > + > if (ret < 0) { > - printk(KERN_INFO "Failed to queue request (%d)\n", ret); > - usb_ep_set_halt(video->ep); > - spin_unlock_irqrestore(&queue->irqlock, flags); > uvcg_queue_cancel(queue, 0); > break; > } > - spin_unlock_irqrestore(&queue->irqlock, flags); > } > > spin_lock_irqsave(&video->req_lock, flags); > -- > Regards, > > Laurent Pinchart >
Hi Laurent, On 18/09/18 11:35, Laurent Pinchart wrote: > USB requests for video data are queued from two different locations in > the driver, with the same code block occurring twice. Factor it out to a > function. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Looks good, and simplifies locking. Win win. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > drivers/usb/gadget/function/uvc_video.c | 30 ++++++++++++++++++++---------- > 1 file changed, 20 insertions(+), 10 deletions(-) > > diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c > index d3567b90343a..a95c8e2364ed 100644 > --- a/drivers/usb/gadget/function/uvc_video.c > +++ b/drivers/usb/gadget/function/uvc_video.c > @@ -125,6 +125,19 @@ uvc_video_encode_isoc(struct usb_request *req, struct uvc_video *video, > * Request handling > */ > > +static int uvcg_video_ep_queue(struct uvc_video *video, struct usb_request *req) > +{ > + int ret; > + > + ret = usb_ep_queue(video->ep, req, GFP_ATOMIC); > + if (ret < 0) { > + printk(KERN_INFO "Failed to queue request (%d).\n", ret); > + usb_ep_set_halt(video->ep); > + } > + > + return ret; > +} > + > /* > * I somehow feel that synchronisation won't be easy to achieve here. We have > * three events that control USB requests submission: > @@ -189,14 +202,13 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req) > > video->encode(req, video, buf); > > - if ((ret = usb_ep_queue(ep, req, GFP_ATOMIC)) < 0) { > - printk(KERN_INFO "Failed to queue request (%d).\n", ret); > - usb_ep_set_halt(ep); > - spin_unlock_irqrestore(&video->queue.irqlock, flags); > + ret = uvcg_video_ep_queue(video, req); > + spin_unlock_irqrestore(&video->queue.irqlock, flags); > + > + if (ret < 0) { > uvcg_queue_cancel(queue, 0); > goto requeue; > } > - spin_unlock_irqrestore(&video->queue.irqlock, flags); > > return; > > @@ -316,15 +328,13 @@ int uvcg_video_pump(struct uvc_video *video) > video->encode(req, video, buf); > > /* Queue the USB request */ > - ret = usb_ep_queue(video->ep, req, GFP_ATOMIC); > + ret = uvcg_video_ep_queue(video, req); > + spin_unlock_irqrestore(&queue->irqlock, flags); > + > if (ret < 0) { > - printk(KERN_INFO "Failed to queue request (%d)\n", ret); > - usb_ep_set_halt(video->ep); > - spin_unlock_irqrestore(&queue->irqlock, flags); > uvcg_queue_cancel(queue, 0); > break; > } > - spin_unlock_irqrestore(&queue->irqlock, flags); > } > > spin_lock_irqsave(&video->req_lock, flags); >
diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c index d3567b90343a..a95c8e2364ed 100644 --- a/drivers/usb/gadget/function/uvc_video.c +++ b/drivers/usb/gadget/function/uvc_video.c @@ -125,6 +125,19 @@ uvc_video_encode_isoc(struct usb_request *req, struct uvc_video *video, * Request handling */ +static int uvcg_video_ep_queue(struct uvc_video *video, struct usb_request *req) +{ + int ret; + + ret = usb_ep_queue(video->ep, req, GFP_ATOMIC); + if (ret < 0) { + printk(KERN_INFO "Failed to queue request (%d).\n", ret); + usb_ep_set_halt(video->ep); + } + + return ret; +} + /* * I somehow feel that synchronisation won't be easy to achieve here. We have * three events that control USB requests submission: @@ -189,14 +202,13 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req) video->encode(req, video, buf); - if ((ret = usb_ep_queue(ep, req, GFP_ATOMIC)) < 0) { - printk(KERN_INFO "Failed to queue request (%d).\n", ret); - usb_ep_set_halt(ep); - spin_unlock_irqrestore(&video->queue.irqlock, flags); + ret = uvcg_video_ep_queue(video, req); + spin_unlock_irqrestore(&video->queue.irqlock, flags); + + if (ret < 0) { uvcg_queue_cancel(queue, 0); goto requeue; } - spin_unlock_irqrestore(&video->queue.irqlock, flags); return; @@ -316,15 +328,13 @@ int uvcg_video_pump(struct uvc_video *video) video->encode(req, video, buf); /* Queue the USB request */ - ret = usb_ep_queue(video->ep, req, GFP_ATOMIC); + ret = uvcg_video_ep_queue(video, req); + spin_unlock_irqrestore(&queue->irqlock, flags); + if (ret < 0) { - printk(KERN_INFO "Failed to queue request (%d)\n", ret); - usb_ep_set_halt(video->ep); - spin_unlock_irqrestore(&queue->irqlock, flags); uvcg_queue_cancel(queue, 0); break; } - spin_unlock_irqrestore(&queue->irqlock, flags); } spin_lock_irqsave(&video->req_lock, flags);
USB requests for video data are queued from two different locations in the driver, with the same code block occurring twice. Factor it out to a function. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- drivers/usb/gadget/function/uvc_video.c | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-)