Re: [PATCH v3 1/2] media: uvcvideo: Fix deferred probing error

[Hans de Goede <hdegoede@redhat.com>]

Hi Ricardo,

On 23-Apr-25 01:18, Ricardo Ribalda wrote:
> On Wed, 23 Apr 2025 at 07:05, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
>>
>> On Wed, Apr 23, 2025 at 06:50:10AM +0800, Ricardo Ribalda wrote:
>>> On Wed, 23 Apr 2025 at 02:06, Laurent Pinchart wrote:
>>>> On Thu, Mar 13, 2025 at 12:20:39PM +0000, Ricardo Ribalda wrote:
>>>>> uvc_gpio_parse() can return -EPROBE_DEFER when the GPIOs it depends on
>>>>> have not yet been probed. This return code should be propagated to the
>>>>> caller of uvc_probe() to ensure that probing is retried when the required
>>>>> GPIOs become available.
>>>>>
>>>>> Currently, this error code is incorrectly converted to -ENODEV,
>>>>> causing some internal cameras to be ignored.
>>>>>
>>>>> This commit fixes this issue by propagating the -EPROBE_DEFER error.
>>>>>
>>>>> Cc: stable@vger.kernel.org
>>>>> Fixes: 2886477ff987 ("media: uvcvideo: Implement UVC_EXT_GPIO_UNIT")
>>>>> Reviewed-by: Douglas Anderson <dianders@chromium.org>
>>>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>>>>> ---
>>>>>  drivers/media/usb/uvc/uvc_driver.c | 27 +++++++++++++++++++--------
>>>>>  1 file changed, 19 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
>>>>> index deadbcea5e227c832976fd176c7cdbfd7809c608..e966bdb9239f345fd157588ebdad2b3ebe45168d 100644
>>>>> --- a/drivers/media/usb/uvc/uvc_driver.c
>>>>> +++ b/drivers/media/usb/uvc/uvc_driver.c
>>>>> @@ -2231,13 +2231,16 @@ static int uvc_probe(struct usb_interface *intf,
>>>>>  #endif
>>>>>
>>>>>       /* Parse the Video Class control descriptor. */
>>>>> -     if (uvc_parse_control(dev) < 0) {
>>>>> +     ret = uvc_parse_control(dev);
>>>>> +     if (ret < 0) {
>>>>> +             ret = -ENODEV;
>>>>
>>>> Why do you set ret to -ENODEV here...
>>>>
>>>>>               uvc_dbg(dev, PROBE, "Unable to parse UVC descriptors\n");
>>>>>               goto error;
>>>>>       }
>>>>>
>>>>>       /* Parse the associated GPIOs. */
>>>>> -     if (uvc_gpio_parse(dev) < 0) {
>>>>> +     ret = uvc_gpio_parse(dev);
>>>>> +     if (ret < 0) {
>>>>>               uvc_dbg(dev, PROBE, "Unable to parse UVC GPIOs\n");
>>>>>               goto error;
>>>>>       }
>>>>> @@ -2263,24 +2266,32 @@ static int uvc_probe(struct usb_interface *intf,
>>>>>       }
>>>>>
>>>>>       /* Register the V4L2 device. */
>>>>> -     if (v4l2_device_register(&intf->dev, &dev->vdev) < 0)
>>>>> +     ret = v4l2_device_register(&intf->dev, &dev->vdev);
>>>>> +     if (ret < 0)
>>>>
>>>> ... but not here ? The code below is also not very consistant.
>>>
>>> For all the "external" functions I was looking into populating their
>>> error code to probe(). Other drivers (check vivid for example) do
>>> exactly this.
>>>
>>> There is more value in returning the real cause of the error (ENOMEM,
>>> EINVAL) that the plain ENODEV.
>>
>> Yes, I got that, my question was why you override the return value of
>> e.g. uvc_parse_control() or uvc_scan_device() with -ENODEV, but not for
>> e.g. uvc_gpio_parse() or v4l2_device_register(). There's no explanation
>> in the commit message regarding why they're treated differently.
> 
> Because it is less risky that way. There are plenty of examples where
> the framework functions return code is passed to probe().
> 
> The uvc_* functions might or might not work this way. When I do that
> assessment for every function I can post a different patch. I thought
> that this approach was safer, especially if we are cc-ing stable.
> 
> A note in the commit message would have been a nice thing to have I agree :).

I agree with Laurent that just properly propagating the error code of
all functions, without overriding the return value with another -EXXXX code
in some places seems a better and cleaner way to handle this.

In the end the return value of uvc_probe() does not matter that much,
the only difference is that for errors other then -ENODEV the driver-core
will print an extra error message. But we should not fail to probe anyways.

If we get bug reports about this we can revisit, but for simplicity and
consistency reasons I would prefer to just always return the error of
the called function as is.

Regards,

Hans




>>>>>               goto error;
>>>>>
>>>>>       /* Scan the device for video chains. */
>>>>> -     if (uvc_scan_device(dev) < 0)
>>>>> +     if (uvc_scan_device(dev) < 0) {
>>>>> +             ret = -ENODEV;
>>>>>               goto error;
>>>>> +     }
>>>>>
>>>>>       /* Initialize controls. */
>>>>> -     if (uvc_ctrl_init_device(dev) < 0)
>>>>> +     if (uvc_ctrl_init_device(dev) < 0) {
>>>>> +             ret = -ENODEV;
>>>>>               goto error;
>>>>> +     }
>>>>>
>>>>>       /* Register video device nodes. */
>>>>> -     if (uvc_register_chains(dev) < 0)
>>>>> +     if (uvc_register_chains(dev) < 0) {
>>>>> +             ret = -ENODEV;
>>>>>               goto error;
>>>>> +     }
>>>>>
>>>>>  #ifdef CONFIG_MEDIA_CONTROLLER
>>>>>       /* Register the media device node */
>>>>> -     if (media_device_register(&dev->mdev) < 0)
>>>>> +     ret = media_device_register(&dev->mdev);
>>>>> +     if (ret < 0)
>>>>>               goto error;
>>>>>  #endif
>>>>>       /* Save our data pointer in the interface data. */
>>>>> @@ -2314,7 +2325,7 @@ static int uvc_probe(struct usb_interface *intf,
>>>>>  error:
>>>>>       uvc_unregister_video(dev);
>>>>>       kref_put(&dev->ref, uvc_delete);
>>>>> -     return -ENODEV;
>>>>> +     return ret;
>>>>>  }
>>>>>
>>>>>  static void uvc_disconnect(struct usb_interface *intf)
>>
>> --
>> Regards,
>>
>> Laurent Pinchart
> 
> 
>