Hi,
On 20-Nov-24 11:50 AM, Hans de Goede wrote:
> Hi Ricardo,
>
> On 18-Nov-24 5:57 PM, Ricardo Ribalda wrote:
>> On Mon, 18 Nov 2024 at 17:41, Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>>>
>>> Hi Ricardo,
>>>
>>> Thank you for your patch.
>>>
>>> On 8-Oct-24 5:00 PM, Ricardo Ribalda wrote:
>>>> Some cameras, like the ELMO MX-P3, do not return all the bytes
>>>> requested from a control if it can fit in less bytes.
>>>> Eg: Returning 0xab instead of 0x00ab.
>>>> usb 3-9: Failed to query (GET_DEF) UVC control 3 on unit 2: 1 (exp. 2).
>>>>
>>>> Extend the returned value from the camera and return it.
>>>>
>>>> Cc: stable@xxxxxxxxxxxxxxx
>>>> Fixes: a763b9fb58be ("media: uvcvideo: Do not return positive errors in uvc_query_ctrl()")
>>>> Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
>>>> ---
>>>> drivers/media/usb/uvc/uvc_video.c | 19 +++++++++++++++++--
>>>> 1 file changed, 17 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
>>>> index cd9c29532fb0..f125b3ba50f2 100644
>>>> --- a/drivers/media/usb/uvc/uvc_video.c
>>>> +++ b/drivers/media/usb/uvc/uvc_video.c
>>>> @@ -76,14 +76,29 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
>>>>
>>>> ret = __uvc_query_ctrl(dev, query, unit, intfnum, cs, data, size,
>>>> UVC_CTRL_CONTROL_TIMEOUT);
>>>> - if (likely(ret == size))
>>>> + if (ret > 0) {
>>>> + if (size == ret)
>>>> + return 0;
>>>> +
>>>> + /*
>>>> + * In UVC the data is represented in little-endian by default.
>>>> + * Some devices return shorter control packages that expected
>>>> + * for GET_DEF/MAX/MIN if the return value can fit in less
>>>> + * bytes.
>>>
>>> What about GET_CUR/GET_RES ? are those not affected?
>>>
>>> And if it is not affected should we limit this special handling to
>>> GET_DEF/MAX/MIN ?
>>
>> I have only seen it with GET_DEF, but I would not be surprised if it
>> happens for all of them.
>>
>> before:
>> a763b9fb58be ("media: uvcvideo: Do not return positive errors in
>> uvc_query_ctrl()")
>> We were applying the quirk to all the call types, so I'd rather keep
>> the old behaviour.
>>
>> The extra logging will help us find bugs (if any).
>>
>> Let me fix the doc.
>>
>>>
>>>
>>>> + * Zero all the bytes that the device have not written.
>>>> + */
>>>> + memset(data + ret, 0, size - ret);
>>>
>>> So your new work around automatically applies to all UVC devices which
>>> gives us a short return. I think that is both good and bad at the same
>>> time. Good because it avoids the need to add quirks. Bad because what
>>> if we get a short return for another reason.
>>>
>>> You do warn on the short return. So if we get bugs due to hitting the short
>>> return for another reason the warning will be i the logs.
>>>
>>> So all in all think the good outways the bad.
>>>
>>> So yes this seems like a good solution.
>>>
>>>> + dev_warn(&dev->udev->dev,
>>>> + "UVC non compliance: %s control %u on unit %u returned %d bytes when we expected %u.\n",
>>>> + uvc_query_name(query), cs, unit, ret, size);
>>>
>>> I do wonder if we need to use dev_warn_ratelimited()
>>> or dev_warn_once() here though.
>>>
>>> If this only impacts GET_DEF/MAX/MIN we will only hit this
>>> once per ctrl, after which the cache will be populated.
>>>
>>> But if GET_CUR is also affected then userspace can trigger
>>> this warning. So in that case I think we really should use
>>> dev_warn_once() or have a flag per ctrl to track this
>>> and only warn once per ctrl if we want to know which
>>> ctrls exactly are buggy.
>>
>> Let me use dev_warn_once()
>
> Great, thank you.
>
> Re-reading this I think what would be best here is to combine
> dev_warn_once() with a dev_dbg logging the same thing.
>
> This way if we want the more fine grained messages for all
> controls / all of GET_* and not just the first call we can
> still get them by enabling the debug messages with dyndbg.
>
> This combination is used for similar reasons in other places
> of the kernel.
>
> Not sure what Laurent thinks of this though, Laurent ?
>
> I wonder if we need some sort of helper for this:
>
> dev_warn_once_and_debug(...(
Nevermind I see that you've already send a v3.
Lets stick with just the dev_warn_once() for now
and then we can revisit this if necessary.
Regards,
Hans
>>> What we really do not want is userspace repeatedly calling
>>> VIDIOC_G_CTRL / VIDIOC_G_EXT_CTRLS resulting in a message
>>> in dmesg every call.
>>>
>>>> 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 < 0 ? ret : -EPIPE;
>>>> + return ret ? ret : -EPIPE;
>>>
>>> It took me a minute to wrap my brain around this and even
>>> though I now understand this change I do not like it.
>>>
>>> There is no need to optimize an error-handling path like this
>>> and IMHO the original code is much easier to read:
>>>
>>> return ret < 0 ? ret : -ESOMETHING;
>>>
>>> is a well known pattern to check results from functions which
>>> return a negative errno, or the amount of bytes read, combined
>>> with an earlier success check for ret == amount-expected .
>>>
>>> By changing this to:
>>>
>>> return ret ? ret : -EPIPE;
>>>
>>> You are breaking the pattern recognition people familiar with
>>> this kinda code have and IMHO this is not necessary.
>>>
>>> Also not changing this reduces the patch-size / avoids code-churn
>>> which also is a good thing.
>>>
>>> Please drop this part of the patch.
>> ack
>>>
>>> Regards,
>>>
>>> Hans
>>>
>>>
>>
>>
>
