Re: [PATCH v7 2/2] uvcvideo: handle control pipe protocol STALLs

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Guennadi,

Thank you for the patch.

On Friday, 23 March 2018 11:24:01 EEST Laurent Pinchart wrote:
> From: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> 
> When a command ends up in a STALL on the control pipe, use the Request
> Error Code control to provide a more precise error information to the
> user.

This is the kind of change that I believe is completely right, but that 
nonetheless worries me. All the years I've spent working with UVC webcams 
taught me that lots of cameras have buggy firmware, and that any change in how 
the host driver issues requests can have dire consequences for users. This is 
especially true when the host driver issues a request that was never issued 
before.

The UVC specification also doesn't clearly tell whether the request error code 
control is mandatory for a device to implement. My interpretation of the 
specification is that it is, but it would have been nice for the specification 
to be explicit on this topic. Have you encountered devices that don't 
implement this control ?

This being said, I don't claim that would be a reason not to use the request 
error code control as proposed by this patch, but I would feel less worried if 
I knew whether the Windows driver used that control as well. If it does 
there's a high chance that cameras will implement it correctly, while if it 
doesn't we will most certainly hit bugs with several cameras. I was thus 
wondering if in the course of your UVC developments you would have happened to 
find out whether this control is used by Windows.

I would also like to know a bit more about the purpose of this patch. Logging 
the error code is certainly useful for diagnosis purpose. Have you also found 
it useful to report different errors back to userspace ? If so, could you 
explain your use cases ?

> Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com>
> ---
>  drivers/media/usb/uvc/uvc_video.c | 59 ++++++++++++++++++++++++++++++++----
>  1 file changed, 53 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_video.c
> b/drivers/media/usb/uvc/uvc_video.c index aa0082fe5833..eb9e04a59427 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -34,15 +34,59 @@ static int __uvc_query_ctrl(struct uvc_device *dev, u8
> query, u8 unit, u8 intfnum, u8 cs, void *data, u16 size,
>  			int timeout)
>  {
> -	u8 type = USB_TYPE_CLASS | USB_RECIP_INTERFACE;
> +	u8 type = USB_TYPE_CLASS | USB_RECIP_INTERFACE, tmp, error;

Would you mind declaring one variable per line to match the style of the rest 
of the driver ?

>  	unsigned int pipe;
> +	int ret;
> 
>  	pipe = (query & 0x80) ? usb_rcvctrlpipe(dev->udev, 0)
>  			      : usb_sndctrlpipe(dev->udev, 0);
> 
>  	type |= (query & 0x80) ? USB_DIR_IN : USB_DIR_OUT;
> 
> -	return usb_control_msg(dev->udev, pipe, query, type, cs << 8,
> +	ret = usb_control_msg(dev->udev, pipe, query, type, cs << 8,
>  			unit << 8 | intfnum, data, size, timeout);
> +

Nitpicking, you can remove the blank line here.

> +	if (ret != -EPIPE)
> +		return ret;
> +
> +	tmp = *(u8 *)data;
> +
> +	pipe = usb_rcvctrlpipe(dev->udev, 0);
> +	type = USB_TYPE_CLASS | USB_RECIP_INTERFACE | USB_DIR_IN;
> +	ret = usb_control_msg(dev->udev, pipe, UVC_GET_CUR, type,
> +			      UVC_VC_REQUEST_ERROR_CODE_CONTROL << 8,
> +			      unit << 8 | intfnum, data, 1, timeout);

Unless I'm mistaken the wIndex value should be "Zero and Interface" according 
to both the UVC 1.1 and UVC 1.5 specifications. This should thus be

	ret = usb_control_msg(dev->udev, pipe, UVC_GET_CUR, type,
			      UVC_VC_REQUEST_ERROR_CODE_CONTROL << 8,
			      intfnum, data, 1, timeout);

UVC_VC_REQUEST_ERROR_CODE_CONTROL is only applicable to requests to the 
control interface, not to the streaming interfaces, while __uvc_query_ctrl() 
is used for both. I think the code should thus be moved to uvc_query_ctrl(). 
This would allow calling __uvc_query_ctrl() for both the original request and 
the error code control instead of calling usb_control_msg() manually.

> +	error = *(u8 *)data;
> +	*(u8 *)data = tmp;
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	if (!ret)

I wonder if this check should be if (ret != 1) as it would be an error if the 
device returned more than 1 byte. I suppose this can't happen when calling 
usb_control_msg() with the size set to 1, but I'd find the check easier to 
understand.

> +		return -EINVAL;

Should we return -EPIPE instead ? if ret != 1 we have failed reading the error 
code, so reporting the STALL to the caller is the best we could do in my 
opinion. -EINVAL would mean that this function was called with invalid 
parameters, and we don't know that for sure.

> +	uvc_trace(UVC_TRACE_CONTROL, "Control error %u\n", error);
> +
> +	switch (error) {
> +	case 0:
> +		/* Cannot happen - we received a STALL */
> +		return -EPIPE;
> +	case 1: /* Not ready */
> +		return -EAGAIN;
> +	case 2: /* Wrong state */
> +		return -EILSEQ;
> +	case 3: /* Power */
> +		return -EREMOTE;
> +	case 4: /* Out of range */
> +		return -ERANGE;
> +	case 5: /* Invalid unit */
> +	case 6: /* Invalid control */
> +	case 7: /* Invalid Request */
> +	case 8: /* Invalid value within range */

I agree that these four should return -EINVAL (I'd do it explicitly here).

> +	default: /* reserved or unknown */

But here we don't know what went wrong, so I think returning -EPIPE would be 
better, as we don't have more information than that the device stalled the 
original control request.

> +		break;
> +	}
> +
> +	return -EINVAL;
>  }
> 
>  static const char *uvc_query_name(u8 query)
> @@ -80,7 +124,7 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8
> unit, uvc_printk(KERN_ERR, "Failed to query (%s) UVC control %u on "
>  			"unit %u: %d (exp. %u).\n", uvc_query_name(query), cs,
>  			unit, ret, size);
> -		return -EIO;
> +		return ret < 0 ? ret : -EIO;
>  	}
> 
>  	return 0;
> @@ -203,13 +247,15 @@ static int uvc_get_video_ctrl(struct uvc_streaming
> *stream, uvc_warn_once(stream->dev, UVC_WARN_PROBE_DEF, "UVC non "
>  			"compliance - GET_DEF(PROBE) not supported. "
>  			"Enabling workaround.\n");
> -		ret = -EIO;
> +		if (ret >= 0)
> +			ret = -EIO;
>  		goto out;
>  	} else if (ret != size) {
>  		uvc_printk(KERN_ERR, "Failed to query (%u) UVC %s control : "
>  			"%d (exp. %u).\n", query, probe ? "probe" : "commit",
>  			ret, size);
> -		ret = -EIO;
> +		if (ret >= 0)
> +			ret = -EIO;
>  		goto out;
>  	}
> 
> @@ -290,7 +336,8 @@ static int uvc_set_video_ctrl(struct uvc_streaming
> *stream, uvc_printk(KERN_ERR, "Failed to set UVC %s control : "
>  			"%d (exp. %u).\n", probe ? "probe" : "commit",
>  			ret, size);
> -		ret = -EIO;
> +		if (ret >= 0)
> +			ret = -EIO;
>  	}
> 
>  	kfree(data);

If you move the request error code handling to uvc_query_ctrl() you won't need 
these changes.

-- 
Regards,

Laurent Pinchart