Message-ID: <ac84df80-5f11-4b05-bf13-8e2388ebfb41@redhat.com>
Date: Wed, 20 Nov 2024 14:01:05 +0100
From: Hans de Goede <hdegoede@...hat.com>
To: Ricardo Ribalda <ribalda@...omium.org>
Cc: Laurent Pinchart <laurent.pinchart@...asonboard.com>,
 Mauro Carvalho Chehab <mchehab@...nel.org>, linux-media@...r.kernel.org,
 linux-kernel@...r.kernel.org, Sakari Ailus <sakari.ailus@...ux.intel.com>,
 stable@...r.kernel.org
Subject: Re: [PATCH v2 1/2] media: uvcvideo: Support partial control reads

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@...hat.com> 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@...r.kernel.org
>>>> Fixes: a763b9fb58be ("media: uvcvideo: Do not return positive errors in uvc_query_ctrl()")
>>>> Signed-off-by: Ricardo Ribalda <ribalda@...omium.org>
>>>> ---
>>>>  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
>>>
>>>
>>
>>
> 

Powered by blists - more mailing lists