From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Roger Quadros <rogerq@ti.com>
Cc: balbi@kernel.org, b-liu@ti.com, nsekhar@ti.com,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] uvc-gadget: Fix Set Interface (alternate setting) response behaviour
Date: Mon, 06 Mar 2017 22:50:14 +0200
Message-ID: <3028836.5DEGZBmlnn@avalon>
User-Agent: KMail/4.14.10 (Linux/4.9.6-gentoo-r1; KDE/4.14.29; x86_64; ; )
In-Reply-To: <1488539835-11851-1-git-send-email-rogerq@ti.com>
References: <1488539835-11851-1-git-send-email-rogerq@ti.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2197
Lines: 64

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.

> 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 <rogerq@ti.com>
> ---
>  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);

-- 
Regards,

Laurent Pinchart