| Message ID | 347cdb4a-19a2-98ce-580f-5aba4abfb2fc@xs4all.nl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | uvcvideo: improve error handling in uvc_query_ctrl() | expand |
Hi Hans Thanks for the patch. I like how uvc is ending :) On Mon, Mar 22, 2021 at 1:09 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: > > - If __uvc_query_ctrl() failed with a non-EPIPE error, then > report that with dev_err. If an error code is obtained, then > report that with dev_dbg. > > - For error 2 (Wrong state) return -EACCES instead of -EILSEQ. > EACCES is a much more appropriate error code. EILSEQ will return > "Invalid or incomplete multibyte or wide character." in strerror(), > which is a *very* confusing message. > > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> > --- > Ricardo, this too can be added to the uvc series. > --- > drivers/media/usb/uvc/uvc_video.c | 44 +++++++++++++++++-------------- > 1 file changed, 24 insertions(+), 20 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c > index b63c073ec30e..3f461bb4eeb9 100644 > --- a/drivers/media/usb/uvc/uvc_video.c > +++ b/drivers/media/usb/uvc/uvc_video.c > @@ -68,7 +68,7 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, > u8 intfnum, u8 cs, void *data, u16 size) > { > int ret; > - u8 error; > + u8 error = 0; > u8 tmp; > > ret = __uvc_query_ctrl(dev, query, unit, intfnum, cs, data, size, > @@ -76,35 +76,39 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, > if (likely(ret == size)) > return 0; > > - dev_dbg(&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); > + ret = ret < 0 ? ret : -EPIPE; > > - if (ret != -EPIPE) > - return ret; > - > - tmp = *(u8 *)data; > + if (ret == -EPIPE) { > + tmp = *(u8 *)data; > > - ret = __uvc_query_ctrl(dev, UVC_GET_CUR, 0, intfnum, > - UVC_VC_REQUEST_ERROR_CODE_CONTROL, data, 1, > - UVC_CTRL_CONTROL_TIMEOUT); > + ret = __uvc_query_ctrl(dev, UVC_GET_CUR, 0, intfnum, > + UVC_VC_REQUEST_ERROR_CODE_CONTROL, data, 1, > + UVC_CTRL_CONTROL_TIMEOUT); > > - error = *(u8 *)data; > - *(u8 *)data = tmp; > + if (ret == 1) > + error = *(u8 *)data; > + *(u8 *)data = tmp; > + if (ret != 1) > + ret = ret < 0 ? ret : -EPIPE; > + } > > - if (ret != 1) > - return ret < 0 ? ret : -EPIPE; > + if (error) > + dev_dbg(&dev->udev->dev, > + "Failed to query (%s) UVC control %u on unit %u: got error %u.\n", > + uvc_query_name(query), cs, unit, error); > + else > + 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 __uvc_query_ctrl and UVC_VC_REQUEST_ERROR_CODE_CONTROL failed, error is 0. And I think that you want to show a dev_err in that case. Maybe we can initialize error to 7 ? > > - uvc_dbg(dev, CONTROL, "Control error %u\n", error); > + if (!error) > + return ret; I think we do not want these two lines (read next comment) > > switch (error) { > - case 0: > - /* Cannot happen - we received a STALL */ > - return -EPIPE; > case 1: /* Not ready */ > return -EBUSY; > case 2: /* Wrong state */ > - return -EILSEQ; > + return -EACCES; > case 3: /* Power */ > return -EREMOTE; > case 4: /* Out of range */ Maybe we want a dev_dbg if the error code is unknown and return ret? > -- > 2.30.0 >
On 23/03/2021 16:25, Ricardo Ribalda Delgado wrote: > Hi Hans > > Thanks for the patch. I like how uvc is ending :) > > On Mon, Mar 22, 2021 at 1:09 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: >> >> - If __uvc_query_ctrl() failed with a non-EPIPE error, then >> report that with dev_err. If an error code is obtained, then >> report that with dev_dbg. >> >> - For error 2 (Wrong state) return -EACCES instead of -EILSEQ. >> EACCES is a much more appropriate error code. EILSEQ will return >> "Invalid or incomplete multibyte or wide character." in strerror(), >> which is a *very* confusing message. >> >> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> >> --- >> Ricardo, this too can be added to the uvc series. >> --- >> drivers/media/usb/uvc/uvc_video.c | 44 +++++++++++++++++-------------- >> 1 file changed, 24 insertions(+), 20 deletions(-) >> >> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c >> index b63c073ec30e..3f461bb4eeb9 100644 >> --- a/drivers/media/usb/uvc/uvc_video.c >> +++ b/drivers/media/usb/uvc/uvc_video.c >> @@ -68,7 +68,7 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, >> u8 intfnum, u8 cs, void *data, u16 size) >> { >> int ret; >> - u8 error; >> + u8 error = 0; >> u8 tmp; >> >> ret = __uvc_query_ctrl(dev, query, unit, intfnum, cs, data, size, >> @@ -76,35 +76,39 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, >> if (likely(ret == size)) >> return 0; >> >> - dev_dbg(&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); >> + ret = ret < 0 ? ret : -EPIPE; >> >> - if (ret != -EPIPE) >> - return ret; >> - >> - tmp = *(u8 *)data; >> + if (ret == -EPIPE) { >> + tmp = *(u8 *)data; >> >> - ret = __uvc_query_ctrl(dev, UVC_GET_CUR, 0, intfnum, >> - UVC_VC_REQUEST_ERROR_CODE_CONTROL, data, 1, >> - UVC_CTRL_CONTROL_TIMEOUT); >> + ret = __uvc_query_ctrl(dev, UVC_GET_CUR, 0, intfnum, >> + UVC_VC_REQUEST_ERROR_CODE_CONTROL, data, 1, >> + UVC_CTRL_CONTROL_TIMEOUT); >> >> - error = *(u8 *)data; >> - *(u8 *)data = tmp; >> + if (ret == 1) >> + error = *(u8 *)data; >> + *(u8 *)data = tmp; >> + if (ret != 1) >> + ret = ret < 0 ? ret : -EPIPE; >> + } >> >> - if (ret != 1) >> - return ret < 0 ? ret : -EPIPE; >> + if (error) >> + dev_dbg(&dev->udev->dev, >> + "Failed to query (%s) UVC control %u on unit %u: got error %u.\n", >> + uvc_query_name(query), cs, unit, error); >> + else >> + 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 __uvc_query_ctrl and UVC_VC_REQUEST_ERROR_CODE_CONTROL failed, > error is 0. And I think that you want to show a dev_err in that case. > Maybe we can initialize error to 7 ? I'm confused, if error == 0, then it does show dev_err. > > >> >> - uvc_dbg(dev, CONTROL, "Control error %u\n", error); >> + if (!error) >> + return ret; > I think we do not want these two lines (read next comment) >> >> switch (error) { >> - case 0: >> - /* Cannot happen - we received a STALL */ >> - return -EPIPE; >> case 1: /* Not ready */ >> return -EBUSY; >> case 2: /* Wrong state */ >> - return -EILSEQ; >> + return -EACCES; >> case 3: /* Power */ >> return -EREMOTE; >> case 4: /* Out of range */ > > Maybe we want a dev_dbg if the error code is unknown and return ret? Make sense. Regards, Hans
hi Hans On Thu, Mar 25, 2021 at 1:18 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: > > On 23/03/2021 16:25, Ricardo Ribalda Delgado wrote: > > Hi Hans > > > > Thanks for the patch. I like how uvc is ending :) > > > > On Mon, Mar 22, 2021 at 1:09 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: > >> > >> - If __uvc_query_ctrl() failed with a non-EPIPE error, then > >> report that with dev_err. If an error code is obtained, then > >> report that with dev_dbg. > >> > >> - For error 2 (Wrong state) return -EACCES instead of -EILSEQ. > >> EACCES is a much more appropriate error code. EILSEQ will return > >> "Invalid or incomplete multibyte or wide character." in strerror(), > >> which is a *very* confusing message. > >> > >> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> > >> --- > >> Ricardo, this too can be added to the uvc series. > >> --- > >> drivers/media/usb/uvc/uvc_video.c | 44 +++++++++++++++++-------------- > >> 1 file changed, 24 insertions(+), 20 deletions(-) > >> > >> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c > >> index b63c073ec30e..3f461bb4eeb9 100644 > >> --- a/drivers/media/usb/uvc/uvc_video.c > >> +++ b/drivers/media/usb/uvc/uvc_video.c > >> @@ -68,7 +68,7 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, > >> u8 intfnum, u8 cs, void *data, u16 size) > >> { > >> int ret; > >> - u8 error; > >> + u8 error = 0; > >> u8 tmp; > >> > >> ret = __uvc_query_ctrl(dev, query, unit, intfnum, cs, data, size, > >> @@ -76,35 +76,39 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, > >> if (likely(ret == size)) > >> return 0; > >> > >> - dev_dbg(&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); > >> + ret = ret < 0 ? ret : -EPIPE; > >> > >> - if (ret != -EPIPE) > >> - return ret; > >> - > >> - tmp = *(u8 *)data; > >> + if (ret == -EPIPE) { > >> + tmp = *(u8 *)data; > >> > >> - ret = __uvc_query_ctrl(dev, UVC_GET_CUR, 0, intfnum, > >> - UVC_VC_REQUEST_ERROR_CODE_CONTROL, data, 1, > >> - UVC_CTRL_CONTROL_TIMEOUT); > >> + ret = __uvc_query_ctrl(dev, UVC_GET_CUR, 0, intfnum, > >> + UVC_VC_REQUEST_ERROR_CODE_CONTROL, data, 1, > >> + UVC_CTRL_CONTROL_TIMEOUT); > >> > >> - error = *(u8 *)data; > >> - *(u8 *)data = tmp; > >> + if (ret == 1) > >> + error = *(u8 *)data; > >> + *(u8 *)data = tmp; > >> + if (ret != 1) > >> + ret = ret < 0 ? ret : -EPIPE; > >> + } > >> > >> - if (ret != 1) > >> - return ret < 0 ? ret : -EPIPE; > >> + if (error) > >> + dev_dbg(&dev->udev->dev, > >> + "Failed to query (%s) UVC control %u on unit %u: got error %u.\n", > >> + uvc_query_name(query), cs, unit, error); > >> + else > >> + 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 __uvc_query_ctrl and UVC_VC_REQUEST_ERROR_CODE_CONTROL failed, > > error is 0. And I think that you want to show a dev_err in that case. > > Maybe we can initialize error to 7 ? > > I'm confused, if error == 0, then it does show dev_err. My bad. Ignore the message. Can we write it as ?: if (!error) { dev_dbg(&dev->udev->dev, "Failed to query (%s) UVC control %u on unit %u: got error %u.\n", uvc_query_name(query), cs, unit, error); return ret; } 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); > > > > > > >> > >> - uvc_dbg(dev, CONTROL, "Control error %u\n", error); > >> + if (!error) > >> + return ret; > > I think we do not want these two lines (read next comment) > >> > >> switch (error) { > >> - case 0: > >> - /* Cannot happen - we received a STALL */ > >> - return -EPIPE; > >> case 1: /* Not ready */ > >> return -EBUSY; > >> case 2: /* Wrong state */ > >> - return -EILSEQ; > >> + return -EACCES; > >> case 3: /* Power */ > >> return -EREMOTE; > >> case 4: /* Out of range */ > > > > Maybe we want a dev_dbg if the error code is unknown and return ret? > > Make sense. > > Regards, > > Hans
On 25/03/2021 14:54, Ricardo Ribalda wrote: > hi Hans > > On Thu, Mar 25, 2021 at 1:18 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: >> >> On 23/03/2021 16:25, Ricardo Ribalda Delgado wrote: >>> Hi Hans >>> >>> Thanks for the patch. I like how uvc is ending :) >>> >>> On Mon, Mar 22, 2021 at 1:09 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: >>>> >>>> - If __uvc_query_ctrl() failed with a non-EPIPE error, then >>>> report that with dev_err. If an error code is obtained, then >>>> report that with dev_dbg. >>>> >>>> - For error 2 (Wrong state) return -EACCES instead of -EILSEQ. >>>> EACCES is a much more appropriate error code. EILSEQ will return >>>> "Invalid or incomplete multibyte or wide character." in strerror(), >>>> which is a *very* confusing message. >>>> >>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> >>>> --- >>>> Ricardo, this too can be added to the uvc series. >>>> --- >>>> drivers/media/usb/uvc/uvc_video.c | 44 +++++++++++++++++-------------- >>>> 1 file changed, 24 insertions(+), 20 deletions(-) >>>> >>>> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c >>>> index b63c073ec30e..3f461bb4eeb9 100644 >>>> --- a/drivers/media/usb/uvc/uvc_video.c >>>> +++ b/drivers/media/usb/uvc/uvc_video.c >>>> @@ -68,7 +68,7 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, >>>> u8 intfnum, u8 cs, void *data, u16 size) >>>> { >>>> int ret; >>>> - u8 error; >>>> + u8 error = 0; >>>> u8 tmp; >>>> >>>> ret = __uvc_query_ctrl(dev, query, unit, intfnum, cs, data, size, >>>> @@ -76,35 +76,39 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, >>>> if (likely(ret == size)) >>>> return 0; >>>> >>>> - dev_dbg(&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); >>>> + ret = ret < 0 ? ret : -EPIPE; >>>> >>>> - if (ret != -EPIPE) >>>> - return ret; >>>> - >>>> - tmp = *(u8 *)data; >>>> + if (ret == -EPIPE) { >>>> + tmp = *(u8 *)data; >>>> >>>> - ret = __uvc_query_ctrl(dev, UVC_GET_CUR, 0, intfnum, >>>> - UVC_VC_REQUEST_ERROR_CODE_CONTROL, data, 1, >>>> - UVC_CTRL_CONTROL_TIMEOUT); >>>> + ret = __uvc_query_ctrl(dev, UVC_GET_CUR, 0, intfnum, >>>> + UVC_VC_REQUEST_ERROR_CODE_CONTROL, data, 1, >>>> + UVC_CTRL_CONTROL_TIMEOUT); >>>> >>>> - error = *(u8 *)data; >>>> - *(u8 *)data = tmp; >>>> + if (ret == 1) >>>> + error = *(u8 *)data; >>>> + *(u8 *)data = tmp; >>>> + if (ret != 1) >>>> + ret = ret < 0 ? ret : -EPIPE; >>>> + } >>>> >>>> - if (ret != 1) >>>> - return ret < 0 ? ret : -EPIPE; >>>> + if (error) >>>> + dev_dbg(&dev->udev->dev, >>>> + "Failed to query (%s) UVC control %u on unit %u: got error %u.\n", >>>> + uvc_query_name(query), cs, unit, error); >>>> + else >>>> + 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 __uvc_query_ctrl and UVC_VC_REQUEST_ERROR_CODE_CONTROL failed, >>> error is 0. And I think that you want to show a dev_err in that case. >>> Maybe we can initialize error to 7 ? >> >> I'm confused, if error == 0, then it does show dev_err. > > My bad. > > Ignore the message. > > Can we write it as ?: > > if (!error) { > dev_dbg(&dev->udev->dev, > "Failed to query (%s) UVC control %u on unit %u: got error %u.\n", > uvc_query_name(query), cs, unit, error); > return ret; > } > > 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); Sure! Just take this patch as inspiration :-) Regards, Hans > > > >> >>> >>> >>>> >>>> - uvc_dbg(dev, CONTROL, "Control error %u\n", error); >>>> + if (!error) >>>> + return ret; >>> I think we do not want these two lines (read next comment) >>>> >>>> switch (error) { >>>> - case 0: >>>> - /* Cannot happen - we received a STALL */ >>>> - return -EPIPE; >>>> case 1: /* Not ready */ >>>> return -EBUSY; >>>> case 2: /* Wrong state */ >>>> - return -EILSEQ; >>>> + return -EACCES; >>>> case 3: /* Power */ >>>> return -EREMOTE; >>>> case 4: /* Out of range */ >>> >>> Maybe we want a dev_dbg if the error code is unknown and return ret? >> >> Make sense. >> >> Regards, >> >> Hans > > >
Hi Hans On Thu, Mar 25, 2021 at 2:57 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: > > On 25/03/2021 14:54, Ricardo Ribalda wrote: > > hi Hans > > > > On Thu, Mar 25, 2021 at 1:18 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: > >> > >> On 23/03/2021 16:25, Ricardo Ribalda Delgado wrote: > >>> Hi Hans > >>> > >>> Thanks for the patch. I like how uvc is ending :) > >>> > >>> On Mon, Mar 22, 2021 at 1:09 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: > >>>> > >>>> - If __uvc_query_ctrl() failed with a non-EPIPE error, then > >>>> report that with dev_err. If an error code is obtained, then > >>>> report that with dev_dbg. > >>>> > >>>> - For error 2 (Wrong state) return -EACCES instead of -EILSEQ. > >>>> EACCES is a much more appropriate error code. EILSEQ will return > >>>> "Invalid or incomplete multibyte or wide character." in strerror(), > >>>> which is a *very* confusing message. > >>>> > >>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> > >>>> --- > >>>> Ricardo, this too can be added to the uvc series. > >>>> --- > >>>> drivers/media/usb/uvc/uvc_video.c | 44 +++++++++++++++++-------------- > >>>> 1 file changed, 24 insertions(+), 20 deletions(-) > >>>> > >>>> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c > >>>> index b63c073ec30e..3f461bb4eeb9 100644 > >>>> --- a/drivers/media/usb/uvc/uvc_video.c > >>>> +++ b/drivers/media/usb/uvc/uvc_video.c > >>>> @@ -68,7 +68,7 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, > >>>> u8 intfnum, u8 cs, void *data, u16 size) > >>>> { > >>>> int ret; > >>>> - u8 error; > >>>> + u8 error = 0; > >>>> u8 tmp; > >>>> > >>>> ret = __uvc_query_ctrl(dev, query, unit, intfnum, cs, data, size, > >>>> @@ -76,35 +76,39 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, > >>>> if (likely(ret == size)) > >>>> return 0; > >>>> > >>>> - dev_dbg(&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); > >>>> + ret = ret < 0 ? ret : -EPIPE; > >>>> > >>>> - if (ret != -EPIPE) > >>>> - return ret; > >>>> - > >>>> - tmp = *(u8 *)data; > >>>> + if (ret == -EPIPE) { > >>>> + tmp = *(u8 *)data; > >>>> > >>>> - ret = __uvc_query_ctrl(dev, UVC_GET_CUR, 0, intfnum, > >>>> - UVC_VC_REQUEST_ERROR_CODE_CONTROL, data, 1, > >>>> - UVC_CTRL_CONTROL_TIMEOUT); > >>>> + ret = __uvc_query_ctrl(dev, UVC_GET_CUR, 0, intfnum, > >>>> + UVC_VC_REQUEST_ERROR_CODE_CONTROL, data, 1, > >>>> + UVC_CTRL_CONTROL_TIMEOUT); > >>>> > >>>> - error = *(u8 *)data; > >>>> - *(u8 *)data = tmp; > >>>> + if (ret == 1) > >>>> + error = *(u8 *)data; > >>>> + *(u8 *)data = tmp; > >>>> + if (ret != 1) > >>>> + ret = ret < 0 ? ret : -EPIPE; > >>>> + } > >>>> > >>>> - if (ret != 1) > >>>> - return ret < 0 ? ret : -EPIPE; > >>>> + if (error) > >>>> + dev_dbg(&dev->udev->dev, > >>>> + "Failed to query (%s) UVC control %u on unit %u: got error %u.\n", > >>>> + uvc_query_name(query), cs, unit, error); > >>>> + else > >>>> + 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 __uvc_query_ctrl and UVC_VC_REQUEST_ERROR_CODE_CONTROL failed, > >>> error is 0. And I think that you want to show a dev_err in that case. > >>> Maybe we can initialize error to 7 ? > >> > >> I'm confused, if error == 0, then it does show dev_err. > > > > My bad. > > > > Ignore the message. > > > > Can we write it as ?: > > > > if (!error) { > > dev_dbg(&dev->udev->dev, > > "Failed to query (%s) UVC control %u on unit %u: got error %u.\n", > > uvc_query_name(query), cs, unit, error); > > return ret; > > } > > > > 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); > > Sure! Just take this patch as inspiration :-) Do you mind if I modify it before resend? > > Regards, > > Hans > > > > > > > > >> > >>> > >>> > >>>> > >>>> - uvc_dbg(dev, CONTROL, "Control error %u\n", error); > >>>> + if (!error) > >>>> + return ret; > >>> I think we do not want these two lines (read next comment) > >>>> > >>>> switch (error) { > >>>> - case 0: > >>>> - /* Cannot happen - we received a STALL */ > >>>> - return -EPIPE; > >>>> case 1: /* Not ready */ > >>>> return -EBUSY; > >>>> case 2: /* Wrong state */ > >>>> - return -EILSEQ; > >>>> + return -EACCES; > >>>> case 3: /* Power */ > >>>> return -EREMOTE; > >>>> case 4: /* Out of range */ > >>> > >>> Maybe we want a dev_dbg if the error code is unknown and return ret? > >> > >> Make sense. > >> > >> Regards, > >> > >> Hans > > > > > > >
On 25/03/2021 15:02, Ricardo Ribalda wrote: > Hi Hans > > On Thu, Mar 25, 2021 at 2:57 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: >> >> On 25/03/2021 14:54, Ricardo Ribalda wrote: >>> hi Hans >>> >>> On Thu, Mar 25, 2021 at 1:18 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: >>>> >>>> On 23/03/2021 16:25, Ricardo Ribalda Delgado wrote: >>>>> Hi Hans >>>>> >>>>> Thanks for the patch. I like how uvc is ending :) >>>>> >>>>> On Mon, Mar 22, 2021 at 1:09 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: >>>>>> >>>>>> - If __uvc_query_ctrl() failed with a non-EPIPE error, then >>>>>> report that with dev_err. If an error code is obtained, then >>>>>> report that with dev_dbg. >>>>>> >>>>>> - For error 2 (Wrong state) return -EACCES instead of -EILSEQ. >>>>>> EACCES is a much more appropriate error code. EILSEQ will return >>>>>> "Invalid or incomplete multibyte or wide character." in strerror(), >>>>>> which is a *very* confusing message. >>>>>> >>>>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> >>>>>> --- >>>>>> Ricardo, this too can be added to the uvc series. >>>>>> --- >>>>>> drivers/media/usb/uvc/uvc_video.c | 44 +++++++++++++++++-------------- >>>>>> 1 file changed, 24 insertions(+), 20 deletions(-) >>>>>> >>>>>> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c >>>>>> index b63c073ec30e..3f461bb4eeb9 100644 >>>>>> --- a/drivers/media/usb/uvc/uvc_video.c >>>>>> +++ b/drivers/media/usb/uvc/uvc_video.c >>>>>> @@ -68,7 +68,7 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, >>>>>> u8 intfnum, u8 cs, void *data, u16 size) >>>>>> { >>>>>> int ret; >>>>>> - u8 error; >>>>>> + u8 error = 0; >>>>>> u8 tmp; >>>>>> >>>>>> ret = __uvc_query_ctrl(dev, query, unit, intfnum, cs, data, size, >>>>>> @@ -76,35 +76,39 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, >>>>>> if (likely(ret == size)) >>>>>> return 0; >>>>>> >>>>>> - dev_dbg(&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); >>>>>> + ret = ret < 0 ? ret : -EPIPE; >>>>>> >>>>>> - if (ret != -EPIPE) >>>>>> - return ret; >>>>>> - >>>>>> - tmp = *(u8 *)data; >>>>>> + if (ret == -EPIPE) { >>>>>> + tmp = *(u8 *)data; >>>>>> >>>>>> - ret = __uvc_query_ctrl(dev, UVC_GET_CUR, 0, intfnum, >>>>>> - UVC_VC_REQUEST_ERROR_CODE_CONTROL, data, 1, >>>>>> - UVC_CTRL_CONTROL_TIMEOUT); >>>>>> + ret = __uvc_query_ctrl(dev, UVC_GET_CUR, 0, intfnum, >>>>>> + UVC_VC_REQUEST_ERROR_CODE_CONTROL, data, 1, >>>>>> + UVC_CTRL_CONTROL_TIMEOUT); >>>>>> >>>>>> - error = *(u8 *)data; >>>>>> - *(u8 *)data = tmp; >>>>>> + if (ret == 1) >>>>>> + error = *(u8 *)data; >>>>>> + *(u8 *)data = tmp; >>>>>> + if (ret != 1) >>>>>> + ret = ret < 0 ? ret : -EPIPE; >>>>>> + } >>>>>> >>>>>> - if (ret != 1) >>>>>> - return ret < 0 ? ret : -EPIPE; >>>>>> + if (error) >>>>>> + dev_dbg(&dev->udev->dev, >>>>>> + "Failed to query (%s) UVC control %u on unit %u: got error %u.\n", >>>>>> + uvc_query_name(query), cs, unit, error); >>>>>> + else >>>>>> + 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 __uvc_query_ctrl and UVC_VC_REQUEST_ERROR_CODE_CONTROL failed, >>>>> error is 0. And I think that you want to show a dev_err in that case. >>>>> Maybe we can initialize error to 7 ? >>>> >>>> I'm confused, if error == 0, then it does show dev_err. >>> >>> My bad. >>> >>> Ignore the message. >>> >>> Can we write it as ?: >>> >>> if (!error) { >>> dev_dbg(&dev->udev->dev, >>> "Failed to query (%s) UVC control %u on unit %u: got error %u.\n", >>> uvc_query_name(query), cs, unit, error); >>> return ret; >>> } >>> >>> 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); >> >> Sure! Just take this patch as inspiration :-) > > Do you mind if I modify it before resend? No problem, feel free! Hans >> >> Regards, >> >> Hans >> >>> >>> >>> >>>> >>>>> >>>>> >>>>>> >>>>>> - uvc_dbg(dev, CONTROL, "Control error %u\n", error); >>>>>> + if (!error) >>>>>> + return ret; >>>>> I think we do not want these two lines (read next comment) >>>>>> >>>>>> switch (error) { >>>>>> - case 0: >>>>>> - /* Cannot happen - we received a STALL */ >>>>>> - return -EPIPE; >>>>>> case 1: /* Not ready */ >>>>>> return -EBUSY; >>>>>> case 2: /* Wrong state */ >>>>>> - return -EILSEQ; >>>>>> + return -EACCES; >>>>>> case 3: /* Power */ >>>>>> return -EREMOTE; >>>>>> case 4: /* Out of range */ >>>>> >>>>> Maybe we want a dev_dbg if the error code is unknown and return ret? >>>> >>>> Make sense. >>>> >>>> Regards, >>>> >>>> Hans >>> >>> >>> >> > >
Hi Hans On Thu, Mar 25, 2021 at 3:03 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: > > On 25/03/2021 15:02, Ricardo Ribalda wrote: > > Hi Hans > > > > On Thu, Mar 25, 2021 at 2:57 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: > >> > >> On 25/03/2021 14:54, Ricardo Ribalda wrote: > >>> hi Hans > >>> > >>> On Thu, Mar 25, 2021 at 1:18 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: > >>>> > >>>> On 23/03/2021 16:25, Ricardo Ribalda Delgado wrote: > >>>>> Hi Hans > >>>>> > >>>>> Thanks for the patch. I like how uvc is ending :) > >>>>> > >>>>> On Mon, Mar 22, 2021 at 1:09 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: > >>>>>> > >>>>>> - If __uvc_query_ctrl() failed with a non-EPIPE error, then > >>>>>> report that with dev_err. If an error code is obtained, then > >>>>>> report that with dev_dbg. > >>>>>> > >>>>>> - For error 2 (Wrong state) return -EACCES instead of -EILSEQ. > >>>>>> EACCES is a much more appropriate error code. EILSEQ will return > >>>>>> "Invalid or incomplete multibyte or wide character." in strerror(), > >>>>>> which is a *very* confusing message. > >>>>>> > >>>>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> > >>>>>> --- > >>>>>> Ricardo, this too can be added to the uvc series. > >>>>>> --- > >>>>>> drivers/media/usb/uvc/uvc_video.c | 44 +++++++++++++++++-------------- > >>>>>> 1 file changed, 24 insertions(+), 20 deletions(-) > >>>>>> > >>>>>> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c > >>>>>> index b63c073ec30e..3f461bb4eeb9 100644 > >>>>>> --- a/drivers/media/usb/uvc/uvc_video.c > >>>>>> +++ b/drivers/media/usb/uvc/uvc_video.c > >>>>>> @@ -68,7 +68,7 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, > >>>>>> u8 intfnum, u8 cs, void *data, u16 size) > >>>>>> { > >>>>>> int ret; > >>>>>> - u8 error; > >>>>>> + u8 error = 0; > >>>>>> u8 tmp; > >>>>>> > >>>>>> ret = __uvc_query_ctrl(dev, query, unit, intfnum, cs, data, size, > >>>>>> @@ -76,35 +76,39 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, > >>>>>> if (likely(ret == size)) > >>>>>> return 0; > >>>>>> > >>>>>> - dev_dbg(&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); > >>>>>> + ret = ret < 0 ? ret : -EPIPE; > >>>>>> > >>>>>> - if (ret != -EPIPE) > >>>>>> - return ret; > >>>>>> - > >>>>>> - tmp = *(u8 *)data; > >>>>>> + if (ret == -EPIPE) { > >>>>>> + tmp = *(u8 *)data; > >>>>>> > >>>>>> - ret = __uvc_query_ctrl(dev, UVC_GET_CUR, 0, intfnum, > >>>>>> - UVC_VC_REQUEST_ERROR_CODE_CONTROL, data, 1, > >>>>>> - UVC_CTRL_CONTROL_TIMEOUT); > >>>>>> + ret = __uvc_query_ctrl(dev, UVC_GET_CUR, 0, intfnum, > >>>>>> + UVC_VC_REQUEST_ERROR_CODE_CONTROL, data, 1, > >>>>>> + UVC_CTRL_CONTROL_TIMEOUT); > >>>>>> > >>>>>> - error = *(u8 *)data; > >>>>>> - *(u8 *)data = tmp; > >>>>>> + if (ret == 1) > >>>>>> + error = *(u8 *)data; > >>>>>> + *(u8 *)data = tmp; > >>>>>> + if (ret != 1) > >>>>>> + ret = ret < 0 ? ret : -EPIPE; > >>>>>> + } > >>>>>> > >>>>>> - if (ret != 1) > >>>>>> - return ret < 0 ? ret : -EPIPE; > >>>>>> + if (error) > >>>>>> + dev_dbg(&dev->udev->dev, > >>>>>> + "Failed to query (%s) UVC control %u on unit %u: got error %u.\n", > >>>>>> + uvc_query_name(query), cs, unit, error); > >>>>>> + else > >>>>>> + 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 __uvc_query_ctrl and UVC_VC_REQUEST_ERROR_CODE_CONTROL failed, > >>>>> error is 0. And I think that you want to show a dev_err in that case. > >>>>> Maybe we can initialize error to 7 ? > >>>> > >>>> I'm confused, if error == 0, then it does show dev_err. > >>> > >>> My bad. > >>> > >>> Ignore the message. > >>> > >>> Can we write it as ?: > >>> > >>> if (!error) { > >>> dev_dbg(&dev->udev->dev, > >>> "Failed to query (%s) UVC control %u on unit %u: got error %u.\n", > >>> uvc_query_name(query), cs, unit, error); > >>> return ret; > >>> } > >>> > >>> 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); > >> > >> Sure! Just take this patch as inspiration :-) > > > > Do you mind if I modify it before resend? > > No problem, feel free! > Something like? https://git.kernel.org/pub/scm/linux/kernel/git/ribalda/linux.git/commit/?h=uvc-compliance-v9&id=aaf96b6cab88059d4d70346669d26cde09276586 Will probably repost the series tomorrow. Thanks! > Hans > > >> > >> Regards, > >> > >> Hans > >> > >>> > >>> > >>> > >>>> > >>>>> > >>>>> > >>>>>> > >>>>>> - uvc_dbg(dev, CONTROL, "Control error %u\n", error); > >>>>>> + if (!error) > >>>>>> + return ret; > >>>>> I think we do not want these two lines (read next comment) > >>>>>> > >>>>>> switch (error) { > >>>>>> - case 0: > >>>>>> - /* Cannot happen - we received a STALL */ > >>>>>> - return -EPIPE; > >>>>>> case 1: /* Not ready */ > >>>>>> return -EBUSY; > >>>>>> case 2: /* Wrong state */ > >>>>>> - return -EILSEQ; > >>>>>> + return -EACCES; > >>>>>> case 3: /* Power */ > >>>>>> return -EREMOTE; > >>>>>> case 4: /* Out of range */ > >>>>> > >>>>> Maybe we want a dev_dbg if the error code is unknown and return ret? > >>>> > >>>> Make sense. > >>>> > >>>> Regards, > >>>> > >>>> Hans > >>> > >>> > >>> > >> > > > > >
diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c index b63c073ec30e..3f461bb4eeb9 100644 --- a/drivers/media/usb/uvc/uvc_video.c +++ b/drivers/media/usb/uvc/uvc_video.c @@ -68,7 +68,7 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, u8 intfnum, u8 cs, void *data, u16 size) { int ret; - u8 error; + u8 error = 0; u8 tmp; ret = __uvc_query_ctrl(dev, query, unit, intfnum, cs, data, size, @@ -76,35 +76,39 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, if (likely(ret == size)) return 0; - dev_dbg(&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); + ret = ret < 0 ? ret : -EPIPE; - if (ret != -EPIPE) - return ret; - - tmp = *(u8 *)data; + if (ret == -EPIPE) { + tmp = *(u8 *)data; - ret = __uvc_query_ctrl(dev, UVC_GET_CUR, 0, intfnum, - UVC_VC_REQUEST_ERROR_CODE_CONTROL, data, 1, - UVC_CTRL_CONTROL_TIMEOUT); + ret = __uvc_query_ctrl(dev, UVC_GET_CUR, 0, intfnum, + UVC_VC_REQUEST_ERROR_CODE_CONTROL, data, 1, + UVC_CTRL_CONTROL_TIMEOUT); - error = *(u8 *)data; - *(u8 *)data = tmp; + if (ret == 1) + error = *(u8 *)data; + *(u8 *)data = tmp; + if (ret != 1) + ret = ret < 0 ? ret : -EPIPE; + } - if (ret != 1) - return ret < 0 ? ret : -EPIPE; + if (error) + dev_dbg(&dev->udev->dev, + "Failed to query (%s) UVC control %u on unit %u: got error %u.\n", + uvc_query_name(query), cs, unit, error); + else + 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); - uvc_dbg(dev, CONTROL, "Control error %u\n", error); + if (!error) + return ret; switch (error) { - case 0: - /* Cannot happen - we received a STALL */ - return -EPIPE; case 1: /* Not ready */ return -EBUSY; case 2: /* Wrong state */ - return -EILSEQ; + return -EACCES; case 3: /* Power */ return -EREMOTE; case 4: /* Out of range */
- If __uvc_query_ctrl() failed with a non-EPIPE error, then report that with dev_err. If an error code is obtained, then report that with dev_dbg. - For error 2 (Wrong state) return -EACCES instead of -EILSEQ. EACCES is a much more appropriate error code. EILSEQ will return "Invalid or incomplete multibyte or wide character." in strerror(), which is a *very* confusing message. Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> --- Ricardo, this too can be added to the uvc series. --- drivers/media/usb/uvc/uvc_video.c | 44 +++++++++++++++++-------------- 1 file changed, 24 insertions(+), 20 deletions(-)