Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752512AbdCGIli (ORCPT ); Tue, 7 Mar 2017 03:41:38 -0500 Received: from lelnx194.ext.ti.com ([198.47.27.80]:30381 "EHLO lelnx194.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753099AbdCGIl2 (ORCPT ); Tue, 7 Mar 2017 03:41:28 -0500 Subject: Re: [PATCH] uvc-gadget: Fix Set Interface (alternate setting) response behaviour To: Laurent Pinchart References: <1488539835-11851-1-git-send-email-rogerq@ti.com> <3028836.5DEGZBmlnn@avalon> CC: , , , , From: Roger Quadros Message-ID: <4c33f742-cdb7-92b2-9596-efe99443f89c@ti.com> Date: Tue, 7 Mar 2017 10:41:06 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 MIME-Version: 1.0 In-Reply-To: <3028836.5DEGZBmlnn@avalon> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2374 Lines: 70 Laurent, On 06/03/17 22:50, Laurent Pinchart wrote: > Hi Roger, > > Thank you for the patch. > > On Friday 03 Mar 2017 13:17:15 Roger Quadros wrote: >> On alternate setting change, webcam gadget sends us a UVC_EVENT_STREAMON >> or UVC_EVENT_STREAMOFF event. It expects delayed status response on >> STREAMON event only but doesn't expect us to send that response over USB. >> It sends the delayed response when we issue the VIDIOC_STREAMON ioctl. >> >> So we must not send UVCIOC_SEND_RESPONSE ioctl in these cases that too >> with invalid response length. > > The commit message only explains why we should not call UVCIOC_SEND_RESPONSE > in response to a STREAMON event, but not why we shouldn't either in response > to a STREAMOFF event. The patch is correct changing both, but I propose > wording the above two paragraphs as follows. > > "uvc-gadget: Do not send Set Interface (alternate setting) response twice > > On alternate setting change, the webcam gadget sends us a UVC_EVENT_STREAMON > or UVC_EVENT_STREAMOFF event. In the first case, the driver will issue a > delayed status response automatically when we call the VIDIOC_STREAMON ioctl. > In the second case, the driver sends the status response immediately. We must > thus not send the status response manually with UVCIOC_SEND_RESPONSE in any of > those cases." > > If you're fine with that I'll change the message when applying, there's no > need to resend the patch. I'm fine with your suggested commit message. Thanks. > >> Without this, the ISO streaming doesn't work if host application >> (e.g. luvcview) is closed and restarted. >> On dwc3 gadget controller it was resulting in Buffer Expiry error on >> the ISO endpoint. >> >> Signed-off-by: Roger Quadros >> --- >> uvc-gadget.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/uvc-gadget.c b/uvc-gadget.c >> index 9ef315c..4d59ab8 100644 >> --- a/uvc-gadget.c >> +++ b/uvc-gadget.c >> @@ -597,12 +597,12 @@ uvc_events_process(struct uvc_device *dev) >> case UVC_EVENT_STREAMON: >> uvc_video_reqbufs(dev, 4); >> uvc_video_stream(dev, 1); >> - break; >> + return; >> >> case UVC_EVENT_STREAMOFF: >> uvc_video_stream(dev, 0); >> uvc_video_reqbufs(dev, 0); >> - break; >> + return; >> } >> >> ioctl(dev->fd, UVCIOC_SEND_RESPONSE, &resp); > -- cheers, -roger