Message ID | 1457539401-11515-8-git-send-email-ao2@ao2.it (mailing list archive) |
---|---|
State | Changes Requested, archived |
Delegated to: | Hans de Goede |
Headers |
Received: from mail.tu-berlin.de ([130.149.7.33]) by www.linuxtv.org with esmtp (Exim 4.84) (envelope-from <linux-media-owner@vger.kernel.org>) id 1adgaY-00089r-FM; Wed, 09 Mar 2016 16:03:54 +0000 X-tubIT-Incoming-IP: 209.132.180.67 Received: from vger.kernel.org ([209.132.180.67]) by mail.tu-berlin.de (exim-4.76/mailfrontend-6) with esmtp id 1adgaW-0007yh-4u; Wed, 09 Mar 2016 17:03:54 +0100 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933170AbcCIQDk (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Wed, 9 Mar 2016 11:03:40 -0500 Received: from smtp205.alice.it ([82.57.200.101]:1602 "EHLO smtp205.alice.it" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933140AbcCIQDe (ORCPT <rfc822;linux-media@vger.kernel.org>); Wed, 9 Mar 2016 11:03:34 -0500 Received: from jcn (87.3.192.69) by smtp205.alice.it (8.6.060.28) id 56332F71207C8982; Wed, 9 Mar 2016 17:03:29 +0100 Received: from ao2 by jcn with local (Exim 4.86_2) (envelope-from <ao2@ao2.it>) id 1adga8-00030x-El; Wed, 09 Mar 2016 17:03:28 +0100 From: Antonio Ospite <ao2@ao2.it> To: Linux Media <linux-media@vger.kernel.org> Cc: Hans de Goede <hdegoede@redhat.com>, Hans Verkuil <hverkuil@xs4all.nl>, Antonio Ospite <ao2@ao2.it> Subject: [PATCH 7/7] [media] gspca: fix a v4l2-compliance failure during read() Date: Wed, 9 Mar 2016 17:03:21 +0100 Message-Id: <1457539401-11515-8-git-send-email-ao2@ao2.it> X-Mailer: git-send-email 2.7.0 In-Reply-To: <1457539401-11515-1-git-send-email-ao2@ao2.it> References: <1457539401-11515-1-git-send-email-ao2@ao2.it> X-Face: z*RaLf`X<@C75u6Ig9}{oW$H; 1_\2t5)({*|jhM<pyWR#k60!#=#>/Vb; ]yA5<GWI5`6u&+ ; 6b'@y|8w"wB; 4/e!7wYYrcqdJFY,~%Gk_4]cq$Ei/7<j&N3ah(m`ku?pX.&+~:_/wC~dwn^)MizBG !pE^+iDQQ1yC6^,)YDKkxDd!T>\I~93>J<_`<4)A{':UrE Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-PMX-Version: 6.0.0.2142326, Antispam-Engine: 2.7.2.2107409, Antispam-Data: 2016.3.9.160018 X-PMX-Spam: Gauge=IIIIIIII, Probability=8%, Report=' MULTIPLE_RCPTS 0.1, HTML_00_01 0.05, HTML_00_10 0.05, BODYTEXTP_SIZE_3000_LESS 0, BODY_SIZE_1300_1399 0, BODY_SIZE_2000_LESS 0, BODY_SIZE_5000_LESS 0, BODY_SIZE_7000_LESS 0, NO_URI_HTTPS 0, REFERENCES 0, SINGLE_URI_IN_BODY 0, URI_ENDS_IN_HTML 0, __ANY_URI 0, __CP_URI_IN_BODY 0, __HAS_FROM 0, __HAS_MSGID 0, __HAS_X_MAILER 0, __HAS_X_MAILING_LIST 0, __IN_REP_TO 0, __MIME_TEXT_ONLY 0, __MULTIPLE_RCPTS_CC_X2 0, __REFERENCES 0, __SANE_MSGID 0, __SINGLE_URI_TEXT 0, __TO_MALFORMED_2 0, __URI_IN_BODY 0, __URI_NO_WWW 0, __URI_NS , __URI_WITH_PATH 0' |
Commit Message
Antonio Ospite
March 9, 2016, 4:03 p.m. UTC
v4l2-compliance fails with this message:
fail: v4l2-test-buffers.cpp(512): Expected EBUSY, got 22
test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: FAIL
Looking at the v4l2-compliance code reveals that this failure is about
the read() callback.
In gspca, dev_read() is calling vidioc_dqbuf() which calls
frame_ready_nolock() but the latter returns -EINVAL in a case when
v4l2-compliance expects -EBUSY.
Fix the failure by changing the return value in frame_ready_nolock().
Signed-off-by: Antonio Ospite <ao2@ao2.it>
---
drivers/media/usb/gspca/gspca.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Comments
Hi, On 09-03-16 17:03, Antonio Ospite wrote: > v4l2-compliance fails with this message: > > fail: v4l2-test-buffers.cpp(512): Expected EBUSY, got 22 > test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: FAIL > > Looking at the v4l2-compliance code reveals that this failure is about > the read() callback. > > In gspca, dev_read() is calling vidioc_dqbuf() which calls > frame_ready_nolock() but the latter returns -EINVAL in a case when > v4l2-compliance expects -EBUSY. > > Fix the failure by changing the return value in frame_ready_nolock(). > > Signed-off-by: Antonio Ospite <ao2@ao2.it> > --- > drivers/media/usb/gspca/gspca.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/usb/gspca/gspca.c b/drivers/media/usb/gspca/gspca.c > index 915b6c7..de7e300 100644 > --- a/drivers/media/usb/gspca/gspca.c > +++ b/drivers/media/usb/gspca/gspca.c > @@ -1664,7 +1664,7 @@ static int frame_ready_nolock(struct gspca_dev *gspca_dev, struct file *file, > return -ENODEV; > if (gspca_dev->capt_file != file || gspca_dev->memory != memory || > !gspca_dev->streaming) > - return -EINVAL; > + return -EBUSY; > > /* check if a frame is ready */ > return gspca_dev->fr_o != atomic_read(&gspca_dev->fr_i); I'm not sure that this is the right fix: 1) !gspca_dev->streaming should result in -EINVAL, this is the same as what videobuf2 is doing 2) gspca_dev->memory != memory should result in -EINVAL 3) gspca_dev->capt_file != file means calling dqbuf without having done reqbufs (through the same fd) which certainly seemes like -EINVAL to me. The actual problem is that dev_read() is not catching that mmap is being in use: static ssize_t dev_read(struct file *file, char __user *data, size_t count, loff_t *ppos) { ... if (gspca_dev->memory == GSPCA_MEMORY_NO) { /* first time ? */ ret = read_alloc(gspca_dev, file); if (ret != 0) return ret; } It will skip the read_alloc since gspca_dev->memory is USERPTR or MMAP and then do a dqbuf with memory == GSPCA_MEMORY_READ, triggering the gspca_dev->memory != memory check. There are a couple of issues here: 1) gspca_dev->memory check without holding usb_lock, the taking and releasing of usb_lock should be moved from read_alloc() into dev_read() itself. 2) dev_read() should not assume that reading is ok if gspca_dev->memory == GSPCA_MEMORY_NO, it needs a: if (gspca_dev->memory != GSPCA_MEMORY_NO && gspca_dev->memory != GSPCA_MEMORY_READ) return -EBUSY; (while holding the usb_lock so the above is wrong) 3) If gspca_dev->memory == GSPCA_MEMORY_READ already the stream could have been stopped. so we need to check gspca_dev->streaming (while holding the usb_lock) and do a streamon if it is not set (and then we can remove the streamon from read_alloc()) So TL;DR: dev_read needs some love. Regards, Hans p.s. If you've time to work on v4l2 stuff what gspca really needs is to just have its buffer handling ripped out and be rewritten to use videobuf2. I would certainly love to see a patch for that. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 10 Mar 2016 16:59:45 +0100 Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > On 09-03-16 17:03, Antonio Ospite wrote: > > v4l2-compliance fails with this message: > > > > fail: v4l2-test-buffers.cpp(512): Expected EBUSY, got 22 > > test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: FAIL > > > > Looking at the v4l2-compliance code reveals that this failure is about > > the read() callback. > > > > In gspca, dev_read() is calling vidioc_dqbuf() which calls > > frame_ready_nolock() but the latter returns -EINVAL in a case when > > v4l2-compliance expects -EBUSY. > > > > Fix the failure by changing the return value in frame_ready_nolock(). > > > > Signed-off-by: Antonio Ospite <ao2@ao2.it> > > --- > > drivers/media/usb/gspca/gspca.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/media/usb/gspca/gspca.c b/drivers/media/usb/gspca/gspca.c > > index 915b6c7..de7e300 100644 > > --- a/drivers/media/usb/gspca/gspca.c > > +++ b/drivers/media/usb/gspca/gspca.c > > @@ -1664,7 +1664,7 @@ static int frame_ready_nolock(struct gspca_dev *gspca_dev, struct file *file, > > return -ENODEV; > > if (gspca_dev->capt_file != file || gspca_dev->memory != memory || > > !gspca_dev->streaming) > > - return -EINVAL; > > + return -EBUSY; > > > > /* check if a frame is ready */ > > return gspca_dev->fr_o != atomic_read(&gspca_dev->fr_i); > > I'm not sure that this is the right fix: > > > 1) !gspca_dev->streaming should result in -EINVAL, this is the same as what videobuf2 is doing > 2) gspca_dev->memory != memory should result in -EINVAL > 3) gspca_dev->capt_file != file means calling dqbuf without having done reqbufs (through the same fd) > which certainly seemes like -EINVAL to me. > > The actual problem is that dev_read() is not catching that mmap is being in use: > > static ssize_t dev_read(struct file *file, char __user *data, > size_t count, loff_t *ppos) > { > ... > if (gspca_dev->memory == GSPCA_MEMORY_NO) { /* first time ? */ > ret = read_alloc(gspca_dev, file); > if (ret != 0) > return ret; > } > > It will skip the read_alloc since gspca_dev->memory is USERPTR or MMAP > and then do a dqbuf with memory == GSPCA_MEMORY_READ, triggering the > gspca_dev->memory != memory check. > > There are a couple of issues here: > > 1) gspca_dev->memory check without holding usb_lock, the taking and > releasing of usb_lock should be moved from read_alloc() into dev_read() > itself. > > 2) dev_read() should not assume that reading is ok if > gspca_dev->memory == GSPCA_MEMORY_NO, it needs a: > > if (gspca_dev->memory != GSPCA_MEMORY_NO && > gspca_dev->memory != GSPCA_MEMORY_READ) > return -EBUSY; > > (while holding the usb_lock so the above is wrong) > > 3) If gspca_dev->memory == GSPCA_MEMORY_READ already the > stream could have been stopped. so we need to check > gspca_dev->streaming (while holding the usb_lock) > and do a streamon if it is not set (and then we can > remove the streamon from read_alloc()) > > So TL;DR: dev_read needs some love. > I'll try to take a look at this too later this week. > Regards, > > Hans > > > p.s. > > If you've time to work on v4l2 stuff what gspca really needs > is to just have its buffer handling ripped out and be rewritten > to use videobuf2. I would certainly love to see a patch for that. > It'd be an interesting tasklet but I don't know when I'll be able to do that. Ciao, Antonio
diff --git a/drivers/media/usb/gspca/gspca.c b/drivers/media/usb/gspca/gspca.c index 915b6c7..de7e300 100644 --- a/drivers/media/usb/gspca/gspca.c +++ b/drivers/media/usb/gspca/gspca.c @@ -1664,7 +1664,7 @@ static int frame_ready_nolock(struct gspca_dev *gspca_dev, struct file *file, return -ENODEV; if (gspca_dev->capt_file != file || gspca_dev->memory != memory || !gspca_dev->streaming) - return -EINVAL; + return -EBUSY; /* check if a frame is ready */ return gspca_dev->fr_o != atomic_read(&gspca_dev->fr_i);