| Message ID | 20241008-uvc-readless-v1-2-042ac4581f44@chromium.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: uvcvideo: Support partial control reads and minor changes | expand |
Hi Ricardo, On Tue, Oct 08, 2024 at 07:06:15AM +0000, Ricardo Ribalda wrote: > Move the query control error logic to its own function. > There is no functional change introduced by this patch. > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > drivers/media/usb/uvc/uvc_video.c | 45 ++++++++++++++++++++++----------------- > 1 file changed, 26 insertions(+), 19 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c > index 853dfb7b5f7b..a57272a2c9e1 100644 > --- a/drivers/media/usb/uvc/uvc_video.c > +++ b/drivers/media/usb/uvc/uvc_video.c > @@ -67,30 +67,12 @@ static const char *uvc_query_name(u8 query) > } > } > > -int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, > - u8 intfnum, u8 cs, void *data, u16 size) > +static int uvc_query_ctrl_error(struct uvc_device *dev, u8 intfnum, void *data) > { > int ret; > u8 error; > u8 tmp; > > - ret = __uvc_query_ctrl(dev, query, unit, intfnum, cs, data, size, > - UVC_CTRL_CONTROL_TIMEOUT); > - if (likely(ret == size)) > - return 0; > - > - if (ret > 0 && ret < size) { > - memset(data + ret, 0, size - ret); > - return 0; > - } > - > - if (ret != -EPIPE) { > - dev_err(&dev->udev->dev, > - "Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n", > - uvc_query_name(query), cs, unit, ret, size); > - return ret ? ret : -EPIPE; > - } > - > /* Reuse data[0] to request the error code. */ > tmp = *(u8 *)data; > > @@ -135,6 +117,31 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, > return -EPIPE; > } > > +int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, > + u8 intfnum, u8 cs, void *data, u16 size) > +{ > + int ret; > + > + ret = __uvc_query_ctrl(dev, query, unit, intfnum, cs, data, size, > + UVC_CTRL_CONTROL_TIMEOUT); > + if (likely(ret == size)) > + return 0; > + > + if (ret == -EPIPE) > + return uvc_query_ctrl_error(dev, intfnum, data); > + > + dev_err(&dev->udev->dev, > + "Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n", > + uvc_query_name(query), cs, unit, ret, size); This message should probably be printed after the check below. I'd actually move the below check before the ret == -EPIPE check as it's a successful case (and changing the condition to <= would make the ret == size check redundant). > + > + if (ret > 0 && ret < size) { > + memset(data + ret, 0, size - ret); > + return 0; > + } > + > + return ret ? ret : -EPIPE; > +} > + > static const struct usb_device_id elgato_cam_link_4k = { > USB_DEVICE(0x0fd9, 0x0066) > }; >
Hi Sakari! On Tue, 8 Oct 2024 at 20:01, Sakari Ailus <sakari.ailus@iki.fi> wrote: > > Hi Ricardo, > > On Tue, Oct 08, 2024 at 07:06:15AM +0000, Ricardo Ribalda wrote: > > Move the query control error logic to its own function. > > There is no functional change introduced by this patch. > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > --- > > drivers/media/usb/uvc/uvc_video.c | 45 ++++++++++++++++++++++----------------- > > 1 file changed, 26 insertions(+), 19 deletions(-) > > > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c > > index 853dfb7b5f7b..a57272a2c9e1 100644 > > --- a/drivers/media/usb/uvc/uvc_video.c > > +++ b/drivers/media/usb/uvc/uvc_video.c > > @@ -67,30 +67,12 @@ static const char *uvc_query_name(u8 query) > > } > > } > > > > -int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, > > - u8 intfnum, u8 cs, void *data, u16 size) > > +static int uvc_query_ctrl_error(struct uvc_device *dev, u8 intfnum, void *data) > > { > > int ret; > > u8 error; > > u8 tmp; > > > > - ret = __uvc_query_ctrl(dev, query, unit, intfnum, cs, data, size, > > - UVC_CTRL_CONTROL_TIMEOUT); > > - if (likely(ret == size)) > > - return 0; > > - > > - if (ret > 0 && ret < size) { > > - memset(data + ret, 0, size - ret); > > - return 0; > > - } > > - > > - if (ret != -EPIPE) { > > - dev_err(&dev->udev->dev, > > - "Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n", > > - uvc_query_name(query), cs, unit, ret, size); > > - return ret ? ret : -EPIPE; > > - } > > - > > /* Reuse data[0] to request the error code. */ > > tmp = *(u8 *)data; > > > > @@ -135,6 +117,31 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, > > return -EPIPE; > > } > > > > +int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, > > + u8 intfnum, u8 cs, void *data, u16 size) > > +{ > > + int ret; > > + > > + ret = __uvc_query_ctrl(dev, query, unit, intfnum, cs, data, size, > > + UVC_CTRL_CONTROL_TIMEOUT); > > + if (likely(ret == size)) > > + return 0; > > + > > + if (ret == -EPIPE) > > + return uvc_query_ctrl_error(dev, intfnum, data); > > + > > + dev_err(&dev->udev->dev, > > + "Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n", > > + uvc_query_name(query), cs, unit, ret, size); > > This message should probably be printed after the check below. If the device is returning less bytes, the hardware is not behaving according to spec and it is good information, specially if you are bringing up a new device. I could make it a dev_warn() (or even uvc_debug) if ret <size. WDYT? > > I'd actually move the below check before the ret == -EPIPE check as it's a > successful case (and changing the condition to <= would make the ret == > size check redundant). something like this? if (ret > 0) { if (ret != size) { print_error(); memcpy(); } return 0; } > > > + > > + if (ret > 0 && ret < size) { > > + memset(data + ret, 0, size - ret); > > + return 0; > > + } > > + > > + return ret ? ret : -EPIPE; > > +} > > + > > static const struct usb_device_id elgato_cam_link_4k = { > > USB_DEVICE(0x0fd9, 0x0066) > > }; > > > > -- > Kind regards, > > Sakari Ailus
Hi Ricardo, On Tue, Oct 08, 2024 at 09:22:25PM +0800, Ricardo Ribalda wrote: > Hi Sakari! > > On Tue, 8 Oct 2024 at 20:01, Sakari Ailus <sakari.ailus@iki.fi> wrote: > > > > Hi Ricardo, > > > > On Tue, Oct 08, 2024 at 07:06:15AM +0000, Ricardo Ribalda wrote: > > > Move the query control error logic to its own function. > > > There is no functional change introduced by this patch. > > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > > --- > > > drivers/media/usb/uvc/uvc_video.c | 45 ++++++++++++++++++++++----------------- > > > 1 file changed, 26 insertions(+), 19 deletions(-) > > > > > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c > > > index 853dfb7b5f7b..a57272a2c9e1 100644 > > > --- a/drivers/media/usb/uvc/uvc_video.c > > > +++ b/drivers/media/usb/uvc/uvc_video.c > > > @@ -67,30 +67,12 @@ static const char *uvc_query_name(u8 query) > > > } > > > } > > > > > > -int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, > > > - u8 intfnum, u8 cs, void *data, u16 size) > > > +static int uvc_query_ctrl_error(struct uvc_device *dev, u8 intfnum, void *data) > > > { > > > int ret; > > > u8 error; > > > u8 tmp; > > > > > > - ret = __uvc_query_ctrl(dev, query, unit, intfnum, cs, data, size, > > > - UVC_CTRL_CONTROL_TIMEOUT); > > > - if (likely(ret == size)) > > > - return 0; > > > - > > > - if (ret > 0 && ret < size) { > > > - memset(data + ret, 0, size - ret); > > > - return 0; > > > - } > > > - > > > - if (ret != -EPIPE) { > > > - dev_err(&dev->udev->dev, > > > - "Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n", > > > - uvc_query_name(query), cs, unit, ret, size); > > > - return ret ? ret : -EPIPE; > > > - } > > > - > > > /* Reuse data[0] to request the error code. */ > > > tmp = *(u8 *)data; > > > > > > @@ -135,6 +117,31 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, > > > return -EPIPE; > > > } > > > > > > +int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, > > > + u8 intfnum, u8 cs, void *data, u16 size) > > > +{ > > > + int ret; > > > + > > > + ret = __uvc_query_ctrl(dev, query, unit, intfnum, cs, data, size, > > > + UVC_CTRL_CONTROL_TIMEOUT); > > > + if (likely(ret == size)) > > > + return 0; > > > + > > > + if (ret == -EPIPE) > > > + return uvc_query_ctrl_error(dev, intfnum, data); > > > + > > > + dev_err(&dev->udev->dev, > > > + "Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n", > > > + uvc_query_name(query), cs, unit, ret, size); > > > > This message should probably be printed after the check below. > > If the device is returning less bytes, the hardware is not behaving > according to spec and it is good information, specially if you are > bringing up a new device. > I could make it a dev_warn() (or even uvc_debug) if ret <size. WDYT? What I also came to think whether this is worth an explicit quirk flag. There could well be devices that have other bugs that would still fall under the same check. Either way, there should not be a message every single time this workaround is applied. Isn't uvc_query_ctrl() also used in some IOCTLs outside probe? > > > > > > I'd actually move the below check before the ret == -EPIPE check as it's a > > successful case (and changing the condition to <= would make the ret == > > size check redundant). > > something like this? > > if (ret > 0) { > if (ret != size) { > print_error(); > memcpy(); > } > return 0; > } Well, if you think it's reasonable to keep the error message (should be a warning in that case IMO), then the original code makes sense. I wonder what Laurent thinks. > > > > > > + > > > + if (ret > 0 && ret < size) { > > > + memset(data + ret, 0, size - ret); > > > + return 0; > > > + } > > > + > > > + return ret ? ret : -EPIPE; > > > +} > > > + > > > static const struct usb_device_id elgato_cam_link_4k = { > > > USB_DEVICE(0x0fd9, 0x0066) > > > }; > > > > >
On Tue, 8 Oct 2024 at 21:31, Sakari Ailus <sakari.ailus@iki.fi> wrote: > > Hi Ricardo, > > On Tue, Oct 08, 2024 at 09:22:25PM +0800, Ricardo Ribalda wrote: > > Hi Sakari! > > > > On Tue, 8 Oct 2024 at 20:01, Sakari Ailus <sakari.ailus@iki.fi> wrote: > > > > > > Hi Ricardo, > > > > > > On Tue, Oct 08, 2024 at 07:06:15AM +0000, Ricardo Ribalda wrote: > > > > Move the query control error logic to its own function. > > > > There is no functional change introduced by this patch. > > > > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > > > --- > > > > drivers/media/usb/uvc/uvc_video.c | 45 ++++++++++++++++++++++----------------- > > > > 1 file changed, 26 insertions(+), 19 deletions(-) > > > > > > > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c > > > > index 853dfb7b5f7b..a57272a2c9e1 100644 > > > > --- a/drivers/media/usb/uvc/uvc_video.c > > > > +++ b/drivers/media/usb/uvc/uvc_video.c > > > > @@ -67,30 +67,12 @@ static const char *uvc_query_name(u8 query) > > > > } > > > > } > > > > > > > > -int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, > > > > - u8 intfnum, u8 cs, void *data, u16 size) > > > > +static int uvc_query_ctrl_error(struct uvc_device *dev, u8 intfnum, void *data) > > > > { > > > > int ret; > > > > u8 error; > > > > u8 tmp; > > > > > > > > - ret = __uvc_query_ctrl(dev, query, unit, intfnum, cs, data, size, > > > > - UVC_CTRL_CONTROL_TIMEOUT); > > > > - if (likely(ret == size)) > > > > - return 0; > > > > - > > > > - if (ret > 0 && ret < size) { > > > > - memset(data + ret, 0, size - ret); > > > > - return 0; > > > > - } > > > > - > > > > - if (ret != -EPIPE) { > > > > - dev_err(&dev->udev->dev, > > > > - "Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n", > > > > - uvc_query_name(query), cs, unit, ret, size); > > > > - return ret ? ret : -EPIPE; > > > > - } > > > > - > > > > /* Reuse data[0] to request the error code. */ > > > > tmp = *(u8 *)data; > > > > > > > > @@ -135,6 +117,31 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, > > > > return -EPIPE; > > > > } > > > > > > > > +int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, > > > > + u8 intfnum, u8 cs, void *data, u16 size) > > > > +{ > > > > + int ret; > > > > + > > > > + ret = __uvc_query_ctrl(dev, query, unit, intfnum, cs, data, size, > > > > + UVC_CTRL_CONTROL_TIMEOUT); > > > > + if (likely(ret == size)) > > > > + return 0; > > > > + > > > > + if (ret == -EPIPE) > > > > + return uvc_query_ctrl_error(dev, intfnum, data); > > > > + > > > > + dev_err(&dev->udev->dev, > > > > + "Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n", > > > > + uvc_query_name(query), cs, unit, ret, size); > > > > > > This message should probably be printed after the check below. > > > > If the device is returning less bytes, the hardware is not behaving > > according to spec and it is good information, specially if you are > > bringing up a new device. > > I could make it a dev_warn() (or even uvc_debug) if ret <size. WDYT? > > What I also came to think whether this is worth an explicit quirk flag. > There could well be devices that have other bugs that would still fall > under the same check. Before https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/media/usb/uvc/uvc_video.c?id=a763b9fb58be869e252a7d33acb0a6390b01c801 we were accepting short reads and showing the error. We are returning to the old behaviour, plus the memset. A quirk will be difficult to maintain. If we can find a heuristics that works I would vote for that. > > Either way, there should not be a message every single time this workaround > is applied. Isn't uvc_query_ctrl() also used in some IOCTLs outside probe? I have only seen this behaviour for GET_DEF, and that value is cached. Meaning that the warning will only be shown once per control. > > > > > > > > > > > I'd actually move the below check before the ret == -EPIPE check as it's a > > > successful case (and changing the condition to <= would make the ret == > > > size check redundant). > > > > something like this? > > > > if (ret > 0) { > > if (ret != size) { > > print_error(); > > memcpy(); > > } > > return 0; > > } > > Well, if you think it's reasonable to keep the error message (should be a > warning in that case IMO), then the original code makes sense. > > I wonder what Laurent thinks. > > > > > > > > > > + > > > > + if (ret > 0 && ret < size) { > > > > + memset(data + ret, 0, size - ret); > > > > + return 0; > > > > + } > > > > + > > > > + return ret ? ret : -EPIPE; > > > > +} > > > > + > > > > static const struct usb_device_id elgato_cam_link_4k = { > > > > USB_DEVICE(0x0fd9, 0x0066) > > > > }; > > > > > > > > > -- > Kind regards, > > Sakari Ailus -- Ricardo Ribalda
Hi Ricardo, On Tue, Oct 08, 2024 at 09:38:58PM +0800, Ricardo Ribalda wrote: > On Tue, 8 Oct 2024 at 21:31, Sakari Ailus <sakari.ailus@iki.fi> wrote: > > > > Hi Ricardo, > > > > On Tue, Oct 08, 2024 at 09:22:25PM +0800, Ricardo Ribalda wrote: > > > Hi Sakari! > > > > > > On Tue, 8 Oct 2024 at 20:01, Sakari Ailus <sakari.ailus@iki.fi> wrote: > > > > > > > > Hi Ricardo, > > > > > > > > On Tue, Oct 08, 2024 at 07:06:15AM +0000, Ricardo Ribalda wrote: > > > > > Move the query control error logic to its own function. > > > > > There is no functional change introduced by this patch. > > > > > > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > > > > --- > > > > > drivers/media/usb/uvc/uvc_video.c | 45 ++++++++++++++++++++++----------------- > > > > > 1 file changed, 26 insertions(+), 19 deletions(-) > > > > > > > > > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c > > > > > index 853dfb7b5f7b..a57272a2c9e1 100644 > > > > > --- a/drivers/media/usb/uvc/uvc_video.c > > > > > +++ b/drivers/media/usb/uvc/uvc_video.c > > > > > @@ -67,30 +67,12 @@ static const char *uvc_query_name(u8 query) > > > > > } > > > > > } > > > > > > > > > > -int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, > > > > > - u8 intfnum, u8 cs, void *data, u16 size) > > > > > +static int uvc_query_ctrl_error(struct uvc_device *dev, u8 intfnum, void *data) > > > > > { > > > > > int ret; > > > > > u8 error; > > > > > u8 tmp; > > > > > > > > > > - ret = __uvc_query_ctrl(dev, query, unit, intfnum, cs, data, size, > > > > > - UVC_CTRL_CONTROL_TIMEOUT); > > > > > - if (likely(ret == size)) > > > > > - return 0; > > > > > - > > > > > - if (ret > 0 && ret < size) { > > > > > - memset(data + ret, 0, size - ret); > > > > > - return 0; > > > > > - } > > > > > - > > > > > - if (ret != -EPIPE) { > > > > > - dev_err(&dev->udev->dev, > > > > > - "Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n", > > > > > - uvc_query_name(query), cs, unit, ret, size); > > > > > - return ret ? ret : -EPIPE; > > > > > - } > > > > > - > > > > > /* Reuse data[0] to request the error code. */ > > > > > tmp = *(u8 *)data; > > > > > > > > > > @@ -135,6 +117,31 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, > > > > > return -EPIPE; > > > > > } > > > > > > > > > > +int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, > > > > > + u8 intfnum, u8 cs, void *data, u16 size) > > > > > +{ > > > > > + int ret; > > > > > + > > > > > + ret = __uvc_query_ctrl(dev, query, unit, intfnum, cs, data, size, > > > > > + UVC_CTRL_CONTROL_TIMEOUT); > > > > > + if (likely(ret == size)) > > > > > + return 0; > > > > > + > > > > > + if (ret == -EPIPE) > > > > > + return uvc_query_ctrl_error(dev, intfnum, data); > > > > > + > > > > > + dev_err(&dev->udev->dev, > > > > > + "Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n", > > > > > + uvc_query_name(query), cs, unit, ret, size); > > > > > > > > This message should probably be printed after the check below. > > > > > > If the device is returning less bytes, the hardware is not behaving > > > according to spec and it is good information, specially if you are > > > bringing up a new device. > > > I could make it a dev_warn() (or even uvc_debug) if ret <size. WDYT? > > > > What I also came to think whether this is worth an explicit quirk flag. > > There could well be devices that have other bugs that would still fall > > under the same check. > > Before https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/media/usb/uvc/uvc_video.c?id=a763b9fb58be869e252a7d33acb0a6390b01c801 > we were accepting short reads and showing the error. > We are returning to the old behaviour, plus the memset. > > A quirk will be difficult to maintain. If we can find a heuristics > that works I would vote for that. Seems reasonable. > > > > > > Either way, there should not be a message every single time this workaround > > is applied. Isn't uvc_query_ctrl() also used in some IOCTLs outside probe? > > I have only seen this behaviour for GET_DEF, and that value is cached. > Meaning that the warning will only be shown once per control. Could you add a comment with this information, also mentioning where this has been noticed? Perhaps we should wait for Laurent's comments still.
diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c index 853dfb7b5f7b..a57272a2c9e1 100644 --- a/drivers/media/usb/uvc/uvc_video.c +++ b/drivers/media/usb/uvc/uvc_video.c @@ -67,30 +67,12 @@ static const char *uvc_query_name(u8 query) } } -int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, - u8 intfnum, u8 cs, void *data, u16 size) +static int uvc_query_ctrl_error(struct uvc_device *dev, u8 intfnum, void *data) { int ret; u8 error; u8 tmp; - ret = __uvc_query_ctrl(dev, query, unit, intfnum, cs, data, size, - UVC_CTRL_CONTROL_TIMEOUT); - if (likely(ret == size)) - return 0; - - if (ret > 0 && ret < size) { - memset(data + ret, 0, size - ret); - return 0; - } - - if (ret != -EPIPE) { - dev_err(&dev->udev->dev, - "Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n", - uvc_query_name(query), cs, unit, ret, size); - return ret ? ret : -EPIPE; - } - /* Reuse data[0] to request the error code. */ tmp = *(u8 *)data; @@ -135,6 +117,31 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, return -EPIPE; } +int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, + u8 intfnum, u8 cs, void *data, u16 size) +{ + int ret; + + ret = __uvc_query_ctrl(dev, query, unit, intfnum, cs, data, size, + UVC_CTRL_CONTROL_TIMEOUT); + if (likely(ret == size)) + return 0; + + if (ret == -EPIPE) + return uvc_query_ctrl_error(dev, intfnum, data); + + dev_err(&dev->udev->dev, + "Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n", + uvc_query_name(query), cs, unit, ret, size); + + if (ret > 0 && ret < size) { + memset(data + ret, 0, size - ret); + return 0; + } + + return ret ? ret : -EPIPE; +} + static const struct usb_device_id elgato_cam_link_4k = { USB_DEVICE(0x0fd9, 0x0066) };
Move the query control error logic to its own function. There is no functional change introduced by this patch. Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> --- drivers/media/usb/uvc/uvc_video.c | 45 ++++++++++++++++++++++----------------- 1 file changed, 26 insertions(+), 19 deletions(-)