s2255drv: fix spinlock issue
PATCH 1/1 s2255drv: fix spinlock issue
qlock spinlock controls access to buf_list and sequence. qlock spinlock should not be locked during a copy to video buffers, an operation that may sleep.
Signed-off-by Dean Anderson <[email protected]>
--- drivers/media/usb/s2255/s2255drv.c | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-)
drivers/media/usb/s2255/s2255drv.c +11 -12
@@ -558,27 +558,31 @@ static void s2255_fwchunk_complete(struct urb *urb) } -static int s2255_got_frame (struct s2255_vc *vc,int jpgsize) +static void s2255_got_frame (struct s2255_vc *vc,int jpgsize) {struct s2255_buffer *buf;struct s2255_dev *dev =to_s2255_dev (vc->vdev.v4l2_dev);unsigned long flags =0 ; -int rc =0 ; +spin_lock_irqsave (&vc->qlock, flags);if (list_empty (&vc->buf_list)) {dprintk (dev,1 ,"No active queue to serve\n" ); - rc =-1 ; - goto unlock; +spin_unlock_irqrestore (&vc->qlock, flags); +return ; } buf =list_entry (vc->buf_list.next, struct s2255_buffer, list);list_del (&buf->list);v4l2_get_timestamp (&buf->vb.v4l2_buf.timestamp); + buf->vb.v4l2_buf.field = vc->field; + buf->vb.v4l2_buf.sequence = vc->frame_count; +spin_unlock_irqrestore (&vc->qlock, flags); +s2255_fillbuff (vc, buf, jpgsize); +/* tell v4l buffer was filled */ +vb2_buffer_done (&buf->vb,VB2_BUF_STATE_DONE );dprintk (dev,2 ,"%s: [buf] [%p]\n" , __func__, buf); -unlock: -spin_unlock_irqrestore (&vc->qlock, flags); -return rc; +return ; }static const struct s2255_fmt *format_by_fourcc (int fourcc)
@@ -649,11 +653,6 @@ static void s2255_fillbuff(struct s2255_vc *vc, }dprintk (dev,2 ,"s2255fill at : Buffer 0x%08lx size= %d\n" , (unsigned long )vbuf, pos); -/* tell v4l buffer was filled */ - buf->vb.v4l2_buf.field = vc->field; - buf->vb.v4l2_buf.sequence = vc->frame_count; -v4l2_get_timestamp (&buf->vb.v4l2_buf.timestamp); -vb2_buffer_done (&buf->vb,VB2_BUF_STATE_DONE ); }
Signature · 1 line
1.9.1
This patch is not so urgent, but should still be considered. Please disregard the second comment below, which is not correct.
The original code, however, holds the spinlock for a long time. From the structure of videobuf2, we could just hold qlock during buf_list and sequence accesses.
Quoted 68 lines · Dean Anderson
qlock spinlock controls access to
buf_listand sequence. qlock spinlock should not be locked during a copy to video buffers, an operation that may sleep.Signed-off-by Dean Anderson <[email protected]>--- drivers/media/usb/s2255/s2255drv.c | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-)drivers/media/usb/s2255/s2255drv.c +11
-7@@ -558,27 +558,31 @@ static void s2255_fwchunk_complete(struct urb *urb) } -static int s2255_got_frame (struct s2255_vc *vc,int jpgsize) +static void s2255_got_frame (struct s2255_vc *vc,int jpgsize) {struct s2255_buffer *buf;struct s2255_dev *dev =to_s2255_dev (vc->vdev.v4l2_dev);unsigned long flags =0 ; -int rc =0 ; +spin_lock_irqsave (&vc->qlock, flags);if (list_empty (&vc->buf_list)) {dprintk (dev,1 ,"No active queue to serve\n" ); - rc =-1 ; - goto unlock; +spin_unlock_irqrestore (&vc->qlock, flags); +return ; } buf =list_entry (vc->buf_list.next, struct s2255_buffer, list);list_del (&buf->list);v4l2_get_timestamp (&buf->vb.v4l2_buf.timestamp); + buf->vb.v4l2_buf.field = vc->field; + buf->vb.v4l2_buf.sequence = vc->frame_count; +spin_unlock_irqrestore (&vc->qlock, flags); +s2255_fillbuff (vc, buf, jpgsize); +/* tell v4l buffer was filled */ +vb2_buffer_done (&buf->vb,VB2_BUF_STATE_DONE );dprintk (dev,2 ,"%s: [buf] [%p]\n" , __func__, buf); -unlock: -spin_unlock_irqrestore (&vc->qlock, flags); -return rc; +return ; }static const struct
s2255_fmt*format_by_fourcc(int fourcc)drivers/media/usb/s2255/s2255drv.c +0
-6@@ -649,11 +653,6 @@ static void s2255_fillbuff(struct s2255_vc *vc, }dprintk (dev,2 ,"s2255fill at : Buffer 0x%08lx size= %d\n" , (unsigned long )vbuf, pos); -/* tell v4l buffer was filled */ - buf->vb.v4l2_buf.field = vc->field; - buf->vb.v4l2_buf.sequence = vc->frame_count; -v4l2_get_timestamp (&buf->vb.v4l2_buf.timestamp); -vb2_buffer_done (&buf->vb,VB2_BUF_STATE_DONE ); } --1.9.1