Re: [PATCH] media: em28xx: return error on media_device_register() failure

[Mauro Carvalho Chehab]

Em Thu, 11 Jan 2024 07:10:10 -0800
Nikita Zhandarovich <n.zhandarovich@xxxxxxxxxx> escreveu:

> On 1/10/24 22:49, Mauro Carvalho Chehab wrote:
> > Em Wed, 10 Jan 2024 09:39:58 -0800
> > Nikita Zhandarovich <n.zhandarovich@xxxxxxxxxx> escreveu:
> >   
> >> In an unlikely case of failure in media_device_register(), release
> >> resources and return the erroneous value. Otherwise, possible issues
> >> with registering the device will continue to be ignored.
> >>
> >> Found by Linux Verification Center (linuxtesting.org) with static
> >> analysis tool SVACE.
> >>
> >> Fixes: 37ecc7b1278f ("[media] em28xx: add media controller support")
> >> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@xxxxxxxxxx>
> >> ---
> >>  drivers/media/usb/em28xx/em28xx-cards.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
> >> index 4d037c92af7c..dae731dfc569 100644
> >> --- a/drivers/media/usb/em28xx/em28xx-cards.c
> >> +++ b/drivers/media/usb/em28xx/em28xx-cards.c
> >> @@ -4095,6 +4095,8 @@ static int em28xx_usb_probe(struct usb_interface *intf,
> >>  	 */
> >>  #ifdef CONFIG_MEDIA_CONTROLLER
> >>  	retval = media_device_register(dev->media_dev);
> >> +	if (retval)
> >> +		goto err_free;  
> > 
> > Not freeing resources here is intentional. See, the media controller 
> > API is optional on this driver. It will just provide a way to identify
> > the device's topology, but the device is completely usable without
> > it.
> > 
> > Perhaps we need, instead, a patch documenting it, and preventing
> > static analysis tools to point it as an issue.
> > 
> > Thanks,
> > Mauro  
> 
> Thank you for your feedback, however I had a few questions...
> 
> While I understand what you mean about optional nature of media
> controller registration in this case, a quick glance into other calls to
> media_device_register() across the source code shows that usually
> failure with registering is handled as a proper error regardless of
> whether the device is still usable. But if you think that we can make an
> exception here, I'll happily oblige.

It depends on how the actual device is controlled. "Normal"
media devices are fully controlled via v4l2 API. On those, the
media controller API is there just to let userspace to query
about the internal settings, but the actual pipelines are
created via V4L2 API. Almost all normal applications will
just ignore the media controller API.

Embedded hardware, however, require setting pipelines via media 
controller for they to actually work. Almost all drivers
implementing the media controller API fall on this category.

> 
> Then if I am to continue on this path, would the following comment above
> the call to media_device_register() suffice?
> 
>  #ifdef CONFIG_MEDIA_CONTROLLER
> +	/*
> +	 * No need to check the return value, the device will still be 	     +
>  * usable without media controller API.
> +	 */
>  	retval = media_device_register(dev->media_dev);

That works for me. It would still produce alerts at static
analyzers, as they'll notice that we're storing retval there
without actually using it.

Thanks,
Mauro