| Message ID | 20170714201424.23592-1-philipp.zabel@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Philipp, Thank you for the patch. On Friday 14 Jul 2017 22:14:22 Philipp Zabel wrote: > Some USB webcam controllers have extension unit controls that report > different lengths via GET_LEN, depending on internal state. If I ever need to hire a hardware designer, I'll make sure to reject any candidate who thinks that creativity is an asset :-( If the size changes, could the flags change as well ? Should you issue a GET_INFO too ? > Add a flag to mark these controls as variable length and issue GET_LEN > before GET/SET_CUR transfers to verify the current length. What happens if the internal state changes between the GET_LEN and the GET/SET_CUR ? > Signed-off-by: Philipp Zabel <philipp.zabel@gmail.com> > --- > drivers/media/usb/uvc/uvc_ctrl.c | 26 +++++++++++++++++++++++++- > include/uapi/linux/uvcvideo.h | 2 ++ > 2 files changed, 27 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c > b/drivers/media/usb/uvc/uvc_ctrl.c index c2ee6e39fd0c..ce69e2c6937d 100644 > --- a/drivers/media/usb/uvc/uvc_ctrl.c > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > @@ -1597,7 +1597,7 @@ static void uvc_ctrl_fixup_xu_info(struct uvc_device > *dev, struct usb_device_id id; > u8 entity; > u8 selector; > - u8 flags; > + u16 flags; > }; > > static const struct uvc_ctrl_fixup fixups[] = { > @@ -1799,6 +1799,30 @@ int uvc_xu_ctrl_query(struct uvc_video_chain *chain, > goto done; > } > > + if ((ctrl->info.flags & UVC_CTRL_FLAG_VARIABLE_LEN) && reqflags) { > + data = kmalloc(2, GFP_KERNEL); > + /* Check if the control length has changed */ > + ret = uvc_query_ctrl(chain->dev, UVC_GET_LEN, xqry->unit, > + chain->dev->intfnum, xqry->selector, data, > + 2); > + size = le16_to_cpup((__le16 *)data); > + kfree(data); Now data is not NULL. > + if (ret < 0) { > + uvc_trace(UVC_TRACE_CONTROL, > + "GET_LEN failed on control %pUl/%u (%d).\n", > + entity->extension.guidExtensionCode, > + xqry->selector, ret); > + goto done; And the kfree(data) at the done label will cause a double free. > + } > + if (ctrl->info.size != size) { > + uvc_trace(UVC_TRACE_CONTROL, > + "XU control %pUl/%u queried: len %u -> %u\n", > + entity->extension.guidExtensionCode, > + xqry->selector, ctrl->info.size, size); > + ctrl->info.size = size; > + } > + } How about moving this code (or part of it at least) to a function that could be shared with uvc_ctrl_fill_xu_info() ? > if (size != xqry->size) { > ret = -ENOBUFS; > goto done; > diff --git a/include/uapi/linux/uvcvideo.h b/include/uapi/linux/uvcvideo.h > index 3b081862b9e8..0f0d63e79045 100644 > --- a/include/uapi/linux/uvcvideo.h > +++ b/include/uapi/linux/uvcvideo.h > @@ -27,6 +27,8 @@ > #define UVC_CTRL_FLAG_RESTORE (1 << 6) > /* Control can be updated by the camera. */ > #define UVC_CTRL_FLAG_AUTO_UPDATE (1 << 7) > +/* Control can change LEN */ > +#define UVC_CTRL_FLAG_VARIABLE_LEN (1 << 8) > > #define UVC_CTRL_FLAG_GET_RANGE \ > (UVC_CTRL_FLAG_GET_CUR | UVC_CTRL_FLAG_GET_MIN | \
Hi Laurent, Am Samstag, den 15.07.2017, 12:49 +0300 schrieb Laurent Pinchart: > Hi Philipp, > > Thank you for the patch. Thank you for the review. > On Friday 14 Jul 2017 22:14:22 Philipp Zabel wrote: > > Some USB webcam controllers have extension unit controls that report > > different lengths via GET_LEN, depending on internal state. > > If I ever need to hire a hardware designer, I'll make sure to reject any > candidate who thinks that creativity is an asset :-( :) > If the size changes, could the flags change as well ? Should you issue a > GET_INFO too ? The size-changing eSP770U control always reports the same INFO. Could this be added as a separate flag in the future if needed, when the next webcam controller designer feels creative? > > Add a flag to mark these controls as variable length and issue GET_LEN > > before GET/SET_CUR transfers to verify the current length. > > What happens if the internal state changes between the GET_LEN and the > GET/SET_CUR ? At least for the Oculus Sensor, as far as I can tell, the length only changes in reaction to a SET_CUR on another control: Control 11 is written to set up an SPI transfer, apparently. That 16- byte control contains a length field in byte 9. The length written to that field becomes the LEN of control 12, which can then be used to read or write data. > > Signed-off-by: Philipp Zabel <philipp.zabel@gmail.com> > > --- > > drivers/media/usb/uvc/uvc_ctrl.c | 26 +++++++++++++++++++++++++- > > include/uapi/linux/uvcvideo.h | 2 ++ > > 2 files changed, 27 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c > > b/drivers/media/usb/uvc/uvc_ctrl.c index c2ee6e39fd0c..ce69e2c6937d 100644 > > --- a/drivers/media/usb/uvc/uvc_ctrl.c > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > > @@ -1597,7 +1597,7 @@ static void uvc_ctrl_fixup_xu_info(struct uvc_device > > *dev, struct usb_device_id id; > > > > u8 entity; > > > > u8 selector; > > > > - u8 flags; > > > > + u16 flags; > > > > }; > > > > > > static const struct uvc_ctrl_fixup fixups[] = { > > @@ -1799,6 +1799,30 @@ int uvc_xu_ctrl_query(struct uvc_video_chain *chain, > > > > goto done; > > > > } > > > > > > + if ((ctrl->info.flags & UVC_CTRL_FLAG_VARIABLE_LEN) && reqflags) { > > > > + data = kmalloc(2, GFP_KERNEL); > > > > + /* Check if the control length has changed */ > > > > + ret = uvc_query_ctrl(chain->dev, UVC_GET_LEN, xqry->unit, > > + chain->dev->intfnum, xqry->selector, > > data, > > > > + 2); > > > > + size = le16_to_cpup((__le16 *)data); > > + kfree(data); > > Now data is not NULL. > > > > > + if (ret < 0) { > > > > + uvc_trace(UVC_TRACE_CONTROL, > > > > + "GET_LEN failed on control %pUl/%u (%d).\n", > > > > + entity->extension.guidExtensionCode, > > > > + xqry->selector, ret); > > + goto done; > > And the kfree(data) at the done label will cause a double free. Thanks, will fix that. > > + } > > > > + if (ctrl->info.size != size) { > > > > + uvc_trace(UVC_TRACE_CONTROL, > > + "XU control %pUl/%u queried: len %u -> > > %u\n", > > > > + entity->extension.guidExtensionCode, > > > > + xqry->selector, ctrl->info.size, size); > > > > + ctrl->info.size = size; > > > > + } > > + } > > How about moving this code (or part of it at least) to a function that could > be shared with uvc_ctrl_fill_xu_info() ? I'll try. This will come at the cost of a bit more boilerplate. regards Philipp
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index c2ee6e39fd0c..ce69e2c6937d 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -1597,7 +1597,7 @@ static void uvc_ctrl_fixup_xu_info(struct uvc_device *dev, struct usb_device_id id; u8 entity; u8 selector; - u8 flags; + u16 flags; }; static const struct uvc_ctrl_fixup fixups[] = { @@ -1799,6 +1799,30 @@ int uvc_xu_ctrl_query(struct uvc_video_chain *chain, goto done; } + if ((ctrl->info.flags & UVC_CTRL_FLAG_VARIABLE_LEN) && reqflags) { + data = kmalloc(2, GFP_KERNEL); + /* Check if the control length has changed */ + ret = uvc_query_ctrl(chain->dev, UVC_GET_LEN, xqry->unit, + chain->dev->intfnum, xqry->selector, data, + 2); + size = le16_to_cpup((__le16 *)data); + kfree(data); + if (ret < 0) { + uvc_trace(UVC_TRACE_CONTROL, + "GET_LEN failed on control %pUl/%u (%d).\n", + entity->extension.guidExtensionCode, + xqry->selector, ret); + goto done; + } + if (ctrl->info.size != size) { + uvc_trace(UVC_TRACE_CONTROL, + "XU control %pUl/%u queried: len %u -> %u\n", + entity->extension.guidExtensionCode, + xqry->selector, ctrl->info.size, size); + ctrl->info.size = size; + } + } + if (size != xqry->size) { ret = -ENOBUFS; goto done; diff --git a/include/uapi/linux/uvcvideo.h b/include/uapi/linux/uvcvideo.h index 3b081862b9e8..0f0d63e79045 100644 --- a/include/uapi/linux/uvcvideo.h +++ b/include/uapi/linux/uvcvideo.h @@ -27,6 +27,8 @@ #define UVC_CTRL_FLAG_RESTORE (1 << 6) /* Control can be updated by the camera. */ #define UVC_CTRL_FLAG_AUTO_UPDATE (1 << 7) +/* Control can change LEN */ +#define UVC_CTRL_FLAG_VARIABLE_LEN (1 << 8) #define UVC_CTRL_FLAG_GET_RANGE \ (UVC_CTRL_FLAG_GET_CUR | UVC_CTRL_FLAG_GET_MIN | \
Some USB webcam controllers have extension unit controls that report different lengths via GET_LEN, depending on internal state. Add a flag to mark these controls as variable length and issue GET_LEN before GET/SET_CUR transfers to verify the current length. Signed-off-by: Philipp Zabel <philipp.zabel@gmail.com> --- drivers/media/usb/uvc/uvc_ctrl.c | 26 +++++++++++++++++++++++++- include/uapi/linux/uvcvideo.h | 2 ++ 2 files changed, 27 insertions(+), 1 deletion(-)