video_register_device() / __video_register_device() registers vdev->dev
with device_register(). Before the call the video core sets

	vdev->dev.release = v4l2_device_release;

v4l2_device_release() invokes vdev->release(vdev) as its last step, and
the driver's vdev->release hook is commonly video_device_release(), which
kfree()s the vdev that the driver allocated with video_device_alloc().

When device_register() fails inside __video_register_device() the core
does

	put_device(&vdev->dev);
	return ret;

which drops the only reference and fires the v4l2_device_release()
chain:

  __video_register_device()
    device_register() -> -E*
    put_device(&vdev->dev)
      -> v4l2_device_release()
         -> vdev->release(vdev)
            -> video_device_release(vdev)   /* kfree(vdev), free #1 */

video_register_device() returns the error to the driver. Drivers that
follow the documented ownership contract release vdev on their own error
path, e.g.

  driver_probe()
    if (video_register_device(vdev, ...))
      goto err_release_vdev;
    ...
  err_release_vdev:
    video_device_release(vdev);   /* free #2 -- DOUBLE FREE */

This is the contract documented in
Documentation/driver-api/media/v4l2-dev.rst: the driver owns vdev and
is responsible for releasing it if video_register_device() fails. As
Hans Verkuil pointed out, the right place to fix this is the v4l2 core
rather than every individual driver, because drivers are expected to
follow the documented ownership contract.

Neutralise vdev->release around put_device() in the device_register()
failure path so the device core cleanup does not run the driver's
release hook. The driver-supplied release is restored before returning
so the caller can release vdev according to the documented contract.
Successful registration is unchanged, so the normal teardown sequence
continues to call the driver's release hook and free vdev exactly once on
unregister.

Fixes: 2a934fdb01db ("media: v4l2-dev: fix error handling in __video_register_device()")
Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
---
 drivers/media/v4l2-core/v4l2-dev.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index 6ce623a1245a..73648549eb2a 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -1075,9 +1075,14 @@ int __video_register_device(struct video_device *vdev,
 	mutex_lock(&videodev_lock);
 	ret = device_register(&vdev->dev);
 	if (ret < 0) {
+		void (*release)(struct video_device *) = vdev->release;
+
 		mutex_unlock(&videodev_lock);
 		pr_err("%s: device_register failed\n", __func__);
+
+		vdev->release = video_device_release_empty;
 		put_device(&vdev->dev);
+		vdev->release = release;
 		return ret;
 	}
 
-- 
2.43.0

Hi Guangshuo,

Thanks for the patch.

On Wed, May 20, 2026 at 05:06:24PM +0800, Guangshuo Li wrote:
> video_register_device() / __video_register_device() registers vdev->dev
> with device_register(). Before the call the video core sets
> 
> 	vdev->dev.release = v4l2_device_release;
> 
> v4l2_device_release() invokes vdev->release(vdev) as its last step, and
> the driver's vdev->release hook is commonly video_device_release(), which
> kfree()s the vdev that the driver allocated with video_device_alloc().
> 
> When device_register() fails inside __video_register_device() the core
> does
> 
> 	put_device(&vdev->dev);
> 	return ret;
> 
> which drops the only reference and fires the v4l2_device_release()
> chain:
> 
>   __video_register_device()
>     device_register() -> -E*
>     put_device(&vdev->dev)
>       -> v4l2_device_release()
>          -> vdev->release(vdev)
>             -> video_device_release(vdev)   /* kfree(vdev), free #1 */
> 
> video_register_device() returns the error to the driver. Drivers that
> follow the documented ownership contract release vdev on their own error
> path, e.g.
> 
>   driver_probe()
>     if (video_register_device(vdev, ...))
>       goto err_release_vdev;
>     ...
>   err_release_vdev:
>     video_device_release(vdev);   /* free #2 -- DOUBLE FREE */
> 
> This is the contract documented in
> Documentation/driver-api/media/v4l2-dev.rst: the driver owns vdev and
> is responsible for releasing it if video_register_device() fails. As
> Hans Verkuil pointed out, the right place to fix this is the v4l2 core
> rather than every individual driver, because drivers are expected to
> follow the documented ownership contract.
> 
> Neutralise vdev->release around put_device() in the device_register()
> failure path so the device core cleanup does not run the driver's
> release hook. The driver-supplied release is restored before returning
> so the caller can release vdev according to the documented contract.
> Successful registration is unchanged, so the normal teardown sequence
> continues to call the driver's release hook and free vdev exactly once on
> unregister.

May I ask how the issue was found?

> 
> Fixes: 2a934fdb01db ("media: v4l2-dev: fix error handling in __video_register_device()")
> Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
> ---
>  drivers/media/v4l2-core/v4l2-dev.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> index 6ce623a1245a..73648549eb2a 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -1075,9 +1075,14 @@ int __video_register_device(struct video_device *vdev,
>  	mutex_lock(&videodev_lock);
>  	ret = device_register(&vdev->dev);
>  	if (ret < 0) {
> +		void (*release)(struct video_device *) = vdev->release;
> +
>  		mutex_unlock(&videodev_lock);
>  		pr_err("%s: device_register failed\n", __func__);
> +
> +		vdev->release = video_device_release_empty;
>  		put_device(&vdev->dev);
> +		vdev->release = release;

This would largely solve the problem but not quite. The release callback
may well do more than just release the the video device. This is the case
for e.g. the rp1-cfe driver.

I wonder what Hans thinks.

>  		return ret;
>  	}
>  

-- 
Regards,

Sakari Ailus

Hi Sakari,

Thanks for reviewing.

On Wed, 20 May 2026 at 18:01, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
>
> Hi Guangshuo,
>
> Thanks for the patch.
>
> On Wed, May 20, 2026 at 05:06:24PM +0800, Guangshuo Li wrote:
> > video_register_device() / __video_register_device() registers vdev->dev
> > with device_register(). Before the call the video core sets
> >
> >       vdev->dev.release = v4l2_device_release;
> >
> > v4l2_device_release() invokes vdev->release(vdev) as its last step, and
> > the driver's vdev->release hook is commonly video_device_release(), which
> > kfree()s the vdev that the driver allocated with video_device_alloc().
> >
> > When device_register() fails inside __video_register_device() the core
> > does
> >
> >       put_device(&vdev->dev);
> >       return ret;
> >
> > which drops the only reference and fires the v4l2_device_release()
> > chain:
> >
> >   __video_register_device()
> >     device_register() -> -E*
> >     put_device(&vdev->dev)
> >       -> v4l2_device_release()
> >          -> vdev->release(vdev)
> >             -> video_device_release(vdev)   /* kfree(vdev), free #1 */
> >
> > video_register_device() returns the error to the driver. Drivers that
> > follow the documented ownership contract release vdev on their own error
> > path, e.g.
> >
> >   driver_probe()
> >     if (video_register_device(vdev, ...))
> >       goto err_release_vdev;
> >     ...
> >   err_release_vdev:
> >     video_device_release(vdev);   /* free #2 -- DOUBLE FREE */
> >
> > This is the contract documented in
> > Documentation/driver-api/media/v4l2-dev.rst: the driver owns vdev and
> > is responsible for releasing it if video_register_device() fails. As
> > Hans Verkuil pointed out, the right place to fix this is the v4l2 core
> > rather than every individual driver, because drivers are expected to
> > follow the documented ownership contract.
> >
> > Neutralise vdev->release around put_device() in the device_register()
> > failure path so the device core cleanup does not run the driver's
> > release hook. The driver-supplied release is restored before returning
> > so the caller can release vdev according to the documented contract.
> > Successful registration is unchanged, so the normal teardown sequence
> > continues to call the driver's release hook and free vdev exactly once on
> > unregister.
>
> May I ask how the issue was found?
>

The issue was found by a static analysis tool that I am currently developing.

The tool reported a few double-free issues around video_device
lifetime handling, especially in error paths after
video_register_device() failures. I first prepared patches for the
individual drivers where the pattern was reported.

After discussing this with Hans and others, we concluded that the
problem is better fixed in the V4L2 core, since drivers are following
the documented ownership model and the problematic case comes from the
device_register() failure path in __video_register_device().

Best regards,
Guangshuo

On Wed, May 20, 2026 at 05:06:24PM +0800, Guangshuo Li wrote:
> video_register_device() / __video_register_device() registers vdev->dev
> with device_register(). Before the call the video core sets
> 
> 	vdev->dev.release = v4l2_device_release;
> 
> v4l2_device_release() invokes vdev->release(vdev) as its last step, and
> the driver's vdev->release hook is commonly video_device_release(), which
> kfree()s the vdev that the driver allocated with video_device_alloc().
> 
> When device_register() fails inside __video_register_device() the core
> does
> 
> 	put_device(&vdev->dev);
> 	return ret;
> 
> which drops the only reference and fires the v4l2_device_release()
> chain:
> 
>   __video_register_device()
>     device_register() -> -E*
>     put_device(&vdev->dev)
>       -> v4l2_device_release()
>          -> vdev->release(vdev)
>             -> video_device_release(vdev)   /* kfree(vdev), free #1 */
> 
> video_register_device() returns the error to the driver. Drivers that
> follow the documented ownership contract release vdev on their own error
> path, e.g.
> 
>   driver_probe()
>     if (video_register_device(vdev, ...))
>       goto err_release_vdev;
>     ...
>   err_release_vdev:
>     video_device_release(vdev);   /* free #2 -- DOUBLE FREE */
> 
> This is the contract documented in
> Documentation/driver-api/media/v4l2-dev.rst: the driver owns vdev and
> is responsible for releasing it if video_register_device() fails. As
> Hans Verkuil pointed out, the right place to fix this is the v4l2 core
> rather than every individual driver, because drivers are expected to
> follow the documented ownership contract.
> 
> Neutralise vdev->release around put_device() in the device_register()
> failure path so the device core cleanup does not run the driver's
> release hook. The driver-supplied release is restored before returning
> so the caller can release vdev according to the documented contract.
> Successful registration is unchanged, so the normal teardown sequence
> continues to call the driver's release hook and free vdev exactly once on
> unregister.
> 
> Fixes: 2a934fdb01db ("media: v4l2-dev: fix error handling in __video_register_device()")
> Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
> ---
>  drivers/media/v4l2-core/v4l2-dev.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> index 6ce623a1245a..73648549eb2a 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -1075,9 +1075,14 @@ int __video_register_device(struct video_device *vdev,
>  	mutex_lock(&videodev_lock);
>  	ret = device_register(&vdev->dev);
>  	if (ret < 0) {
> +		void (*release)(struct video_device *) = vdev->release;
> +
>  		mutex_unlock(&videodev_lock);
>  		pr_err("%s: device_register failed\n", __func__);
> +
> +		vdev->release = video_device_release_empty;
>  		put_device(&vdev->dev);
> +		vdev->release = release;

That looks like a big hack. There must be something wrong somewhere else
in the design.

>  		return ret;
>  	}
>  

-- 
Regards,

Laurent Pinchart

On 20/05/2026 11:34, Laurent Pinchart wrote:
> On Wed, May 20, 2026 at 05:06:24PM +0800, Guangshuo Li wrote:
>> video_register_device() / __video_register_device() registers vdev->dev
>> with device_register(). Before the call the video core sets
>>
>> 	vdev->dev.release = v4l2_device_release;
>>
>> v4l2_device_release() invokes vdev->release(vdev) as its last step, and
>> the driver's vdev->release hook is commonly video_device_release(), which
>> kfree()s the vdev that the driver allocated with video_device_alloc().
>>
>> When device_register() fails inside __video_register_device() the core
>> does
>>
>> 	put_device(&vdev->dev);
>> 	return ret;
>>
>> which drops the only reference and fires the v4l2_device_release()
>> chain:
>>
>>   __video_register_device()
>>     device_register() -> -E*
>>     put_device(&vdev->dev)
>>       -> v4l2_device_release()
>>          -> vdev->release(vdev)
>>             -> video_device_release(vdev)   /* kfree(vdev), free #1 */
>>
>> video_register_device() returns the error to the driver. Drivers that
>> follow the documented ownership contract release vdev on their own error
>> path, e.g.
>>
>>   driver_probe()
>>     if (video_register_device(vdev, ...))
>>       goto err_release_vdev;
>>     ...
>>   err_release_vdev:
>>     video_device_release(vdev);   /* free #2 -- DOUBLE FREE */
>>
>> This is the contract documented in
>> Documentation/driver-api/media/v4l2-dev.rst: the driver owns vdev and
>> is responsible for releasing it if video_register_device() fails. As
>> Hans Verkuil pointed out, the right place to fix this is the v4l2 core
>> rather than every individual driver, because drivers are expected to
>> follow the documented ownership contract.
>>
>> Neutralise vdev->release around put_device() in the device_register()
>> failure path so the device core cleanup does not run the driver's
>> release hook. The driver-supplied release is restored before returning
>> so the caller can release vdev according to the documented contract.
>> Successful registration is unchanged, so the normal teardown sequence
>> continues to call the driver's release hook and free vdev exactly once on
>> unregister.
>>
>> Fixes: 2a934fdb01db ("media: v4l2-dev: fix error handling in __video_register_device()")
>> Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
>> ---
>>  drivers/media/v4l2-core/v4l2-dev.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
>> index 6ce623a1245a..73648549eb2a 100644
>> --- a/drivers/media/v4l2-core/v4l2-dev.c
>> +++ b/drivers/media/v4l2-core/v4l2-dev.c
>> @@ -1075,9 +1075,14 @@ int __video_register_device(struct video_device *vdev,
>>  	mutex_lock(&videodev_lock);
>>  	ret = device_register(&vdev->dev);
>>  	if (ret < 0) {
>> +		void (*release)(struct video_device *) = vdev->release;
>> +
>>  		mutex_unlock(&videodev_lock);
>>  		pr_err("%s: device_register failed\n", __func__);
>> +
>> +		vdev->release = video_device_release_empty;
>>  		put_device(&vdev->dev);
>> +		vdev->release = release;
> 
> That looks like a big hack. There must be something wrong somewhere else
> in the design.

There is, unfortunately the design was wrong since the beginning of V4L2.

Documentation/driver-api/media/v4l2-dev.rst explicitly says that you should use:

        err = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
        if (err) {
                video_device_release(vdev); /* or kfree(my_vdev); */
                return err;
        }

So everyone does that. Luckily device_register never fails in practice (you probably have
bigger problems if it fails then just a double-free).

The reality is that we don't handle this failure well at all, and this change wouldn't
help in all cases either. E.g. drivers/media/platform/renesas/renesas-ceu.c actually relies
on the release() callback in that it doesn't call video_device_release().

But then it would fail on the v4l2_err(vdev->v4l2_dev, ...) call since vdev would be freed
already.

There are probably more drivers like that. (drivers/media/i2c/video-i2c.c)

I'm not sure what is wisdom here.

See also commit 2a934fdb01db ("media: v4l2-dev: fix error handling in __video_register_device()"),
which is where the put_device was introduced in the first place.

Perhaps that should be reverted instead?

Regards,

	Hans

> 
>>  		return ret;
>>  	}
>>  
>

Hi Hans,

On Wed, May 20, 2026 at 12:01:46PM +0200, Hans Verkuil wrote:
> On 20/05/2026 11:34, Laurent Pinchart wrote:
> > On Wed, May 20, 2026 at 05:06:24PM +0800, Guangshuo Li wrote:
> >> video_register_device() / __video_register_device() registers vdev->dev
> >> with device_register(). Before the call the video core sets
> >>
> >> 	vdev->dev.release = v4l2_device_release;
> >>
> >> v4l2_device_release() invokes vdev->release(vdev) as its last step, and
> >> the driver's vdev->release hook is commonly video_device_release(), which
> >> kfree()s the vdev that the driver allocated with video_device_alloc().
> >>
> >> When device_register() fails inside __video_register_device() the core
> >> does
> >>
> >> 	put_device(&vdev->dev);
> >> 	return ret;
> >>
> >> which drops the only reference and fires the v4l2_device_release()
> >> chain:
> >>
> >>   __video_register_device()
> >>     device_register() -> -E*
> >>     put_device(&vdev->dev)
> >>       -> v4l2_device_release()
> >>          -> vdev->release(vdev)
> >>             -> video_device_release(vdev)   /* kfree(vdev), free #1 */
> >>
> >> video_register_device() returns the error to the driver. Drivers that
> >> follow the documented ownership contract release vdev on their own error
> >> path, e.g.
> >>
> >>   driver_probe()
> >>     if (video_register_device(vdev, ...))
> >>       goto err_release_vdev;
> >>     ...
> >>   err_release_vdev:
> >>     video_device_release(vdev);   /* free #2 -- DOUBLE FREE */
> >>
> >> This is the contract documented in
> >> Documentation/driver-api/media/v4l2-dev.rst: the driver owns vdev and
> >> is responsible for releasing it if video_register_device() fails. As
> >> Hans Verkuil pointed out, the right place to fix this is the v4l2 core
> >> rather than every individual driver, because drivers are expected to
> >> follow the documented ownership contract.
> >>
> >> Neutralise vdev->release around put_device() in the device_register()
> >> failure path so the device core cleanup does not run the driver's
> >> release hook. The driver-supplied release is restored before returning
> >> so the caller can release vdev according to the documented contract.
> >> Successful registration is unchanged, so the normal teardown sequence
> >> continues to call the driver's release hook and free vdev exactly once on
> >> unregister.
> >>
> >> Fixes: 2a934fdb01db ("media: v4l2-dev: fix error handling in __video_register_device()")
> >> Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
> >> ---
> >>  drivers/media/v4l2-core/v4l2-dev.c | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >>
> >> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> >> index 6ce623a1245a..73648549eb2a 100644
> >> --- a/drivers/media/v4l2-core/v4l2-dev.c
> >> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> >> @@ -1075,9 +1075,14 @@ int __video_register_device(struct video_device *vdev,
> >>  	mutex_lock(&videodev_lock);
> >>  	ret = device_register(&vdev->dev);
> >>  	if (ret < 0) {
> >> +		void (*release)(struct video_device *) = vdev->release;
> >> +
> >>  		mutex_unlock(&videodev_lock);
> >>  		pr_err("%s: device_register failed\n", __func__);
> >> +
> >> +		vdev->release = video_device_release_empty;
> >>  		put_device(&vdev->dev);
> >> +		vdev->release = release;
> > 
> > That looks like a big hack. There must be something wrong somewhere else
> > in the design.
> 
> There is, unfortunately the design was wrong since the beginning of V4L2.
> 
> Documentation/driver-api/media/v4l2-dev.rst explicitly says that you should use:
> 
>         err = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
>         if (err) {
>                 video_device_release(vdev); /* or kfree(my_vdev); */
>                 return err;
>         }
> 
> So everyone does that. Luckily device_register never fails in practice (you probably have
> bigger problems if it fails then just a double-free).
> 
> The reality is that we don't handle this failure well at all, and this change wouldn't
> help in all cases either. E.g. drivers/media/platform/renesas/renesas-ceu.c actually relies
> on the release() callback in that it doesn't call video_device_release().
> 
> But then it would fail on the v4l2_err(vdev->v4l2_dev, ...) call since vdev would be freed
> already.
> 
> There are probably more drivers like that. (drivers/media/i2c/video-i2c.c)
> 
> I'm not sure what is wisdom here.
> 
> See also commit 2a934fdb01db ("media: v4l2-dev: fix error handling in __video_register_device()"),
> which is where the put_device was introduced in the first place.
> 
> Perhaps that should be reverted instead?

If we want a short term fix I think that would be better.

Have you seen
https://lore.kernel.org/all/aeCOdWLaVpH-5w8s@hovoldconsulting.com/ ?

Having an API contract different from device_register() will likely
cause issues one way or another.

> >>  		return ret;
> >>  	}
> >>  

-- 
Regards,

Laurent Pinchart

On 20/05/2026 12:48, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Wed, May 20, 2026 at 12:01:46PM +0200, Hans Verkuil wrote:
>> On 20/05/2026 11:34, Laurent Pinchart wrote:
>>> On Wed, May 20, 2026 at 05:06:24PM +0800, Guangshuo Li wrote:
>>>> video_register_device() / __video_register_device() registers vdev->dev
>>>> with device_register(). Before the call the video core sets
>>>>
>>>> 	vdev->dev.release = v4l2_device_release;
>>>>
>>>> v4l2_device_release() invokes vdev->release(vdev) as its last step, and
>>>> the driver's vdev->release hook is commonly video_device_release(), which
>>>> kfree()s the vdev that the driver allocated with video_device_alloc().
>>>>
>>>> When device_register() fails inside __video_register_device() the core
>>>> does
>>>>
>>>> 	put_device(&vdev->dev);
>>>> 	return ret;
>>>>
>>>> which drops the only reference and fires the v4l2_device_release()
>>>> chain:
>>>>
>>>>   __video_register_device()
>>>>     device_register() -> -E*
>>>>     put_device(&vdev->dev)
>>>>       -> v4l2_device_release()
>>>>          -> vdev->release(vdev)
>>>>             -> video_device_release(vdev)   /* kfree(vdev), free #1 */
>>>>
>>>> video_register_device() returns the error to the driver. Drivers that
>>>> follow the documented ownership contract release vdev on their own error
>>>> path, e.g.
>>>>
>>>>   driver_probe()
>>>>     if (video_register_device(vdev, ...))
>>>>       goto err_release_vdev;
>>>>     ...
>>>>   err_release_vdev:
>>>>     video_device_release(vdev);   /* free #2 -- DOUBLE FREE */
>>>>
>>>> This is the contract documented in
>>>> Documentation/driver-api/media/v4l2-dev.rst: the driver owns vdev and
>>>> is responsible for releasing it if video_register_device() fails. As
>>>> Hans Verkuil pointed out, the right place to fix this is the v4l2 core
>>>> rather than every individual driver, because drivers are expected to
>>>> follow the documented ownership contract.
>>>>
>>>> Neutralise vdev->release around put_device() in the device_register()
>>>> failure path so the device core cleanup does not run the driver's
>>>> release hook. The driver-supplied release is restored before returning
>>>> so the caller can release vdev according to the documented contract.
>>>> Successful registration is unchanged, so the normal teardown sequence
>>>> continues to call the driver's release hook and free vdev exactly once on
>>>> unregister.
>>>>
>>>> Fixes: 2a934fdb01db ("media: v4l2-dev: fix error handling in __video_register_device()")
>>>> Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
>>>> ---
>>>>  drivers/media/v4l2-core/v4l2-dev.c | 5 +++++
>>>>  1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
>>>> index 6ce623a1245a..73648549eb2a 100644
>>>> --- a/drivers/media/v4l2-core/v4l2-dev.c
>>>> +++ b/drivers/media/v4l2-core/v4l2-dev.c
>>>> @@ -1075,9 +1075,14 @@ int __video_register_device(struct video_device *vdev,
>>>>  	mutex_lock(&videodev_lock);
>>>>  	ret = device_register(&vdev->dev);
>>>>  	if (ret < 0) {
>>>> +		void (*release)(struct video_device *) = vdev->release;
>>>> +
>>>>  		mutex_unlock(&videodev_lock);
>>>>  		pr_err("%s: device_register failed\n", __func__);
>>>> +
>>>> +		vdev->release = video_device_release_empty;
>>>>  		put_device(&vdev->dev);
>>>> +		vdev->release = release;
>>>
>>> That looks like a big hack. There must be something wrong somewhere else
>>> in the design.
>>
>> There is, unfortunately the design was wrong since the beginning of V4L2.
>>
>> Documentation/driver-api/media/v4l2-dev.rst explicitly says that you should use:
>>
>>         err = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
>>         if (err) {
>>                 video_device_release(vdev); /* or kfree(my_vdev); */
>>                 return err;
>>         }
>>
>> So everyone does that. Luckily device_register never fails in practice (you probably have
>> bigger problems if it fails then just a double-free).
>>
>> The reality is that we don't handle this failure well at all, and this change wouldn't
>> help in all cases either. E.g. drivers/media/platform/renesas/renesas-ceu.c actually relies
>> on the release() callback in that it doesn't call video_device_release().
>>
>> But then it would fail on the v4l2_err(vdev->v4l2_dev, ...) call since vdev would be freed
>> already.
>>
>> There are probably more drivers like that. (drivers/media/i2c/video-i2c.c)
>>
>> I'm not sure what is wisdom here.
>>
>> See also commit 2a934fdb01db ("media: v4l2-dev: fix error handling in __video_register_device()"),
>> which is where the put_device was introduced in the first place.
>>
>> Perhaps that should be reverted instead?
> 
> If we want a short term fix I think that would be better.
> 
> Have you seen
> https://lore.kernel.org/all/aeCOdWLaVpH-5w8s@hovoldconsulting.com/ ?

I hadn't seen it. Interesting.

> 
> Having an API contract different from device_register() will likely
> cause issues one way or another.

Looking closely how the driver core works and what commit 2a934fdb01db changed,
I think this might fix it:

diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index 5516b2bbb08f..6ffd385e880f 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -1071,25 +1071,27 @@ int __video_register_device(struct video_device *vdev,
 	vdev->dev.class = &video_class;
 	vdev->dev.devt = MKDEV(VIDEO_MAJOR, vdev->minor);
 	vdev->dev.parent = vdev->dev_parent;
-	vdev->dev.release = v4l2_device_release;
 	dev_set_name(&vdev->dev, "%s%d", name_base, vdev->num);

-	/* Increase v4l2_device refcount */
-	v4l2_device_get(vdev->v4l2_dev);
-
 	mutex_lock(&videodev_lock);
 	ret = device_register(&vdev->dev);
 	if (ret < 0) {
 		mutex_unlock(&videodev_lock);
 		pr_err("%s: device_register failed\n", __func__);
 		put_device(&vdev->dev);
-		return ret;
+		goto cleanup;
 	}
+	/* Register the release callback that will be called when the last
+	   reference to the device goes away. */
+	vdev->dev.release = v4l2_device_release;

 	if (nr != -1 && nr != vdev->num && warn_if_nr_in_use)
 		pr_warn("%s: requested %s%d, got %s\n", __func__,
 			name_base, nr, video_device_node_name(vdev));

+	/* Increase v4l2_device refcount */
+	v4l2_device_get(vdev->v4l2_dev);
+
 	/* Part 5: Register the entity. */
 	ret = video_register_media_controller(vdev);

This mostly reverts 2a934fdb01db, except that we keep the put_device.
But when this is called, vdev->dev.release isn't set yet, so it only
frees the device-related data (device_release in drivers/base/core.c).

Am I missing something?

Regards,

	Hans

> 
>>>>  		return ret;
>>>>  	}
>>>>  
>

Hi Hans,

On Wed, May 20, 2026 at 01:26:30PM +0200, Hans Verkuil wrote:
> On 20/05/2026 12:48, Laurent Pinchart wrote:
> > Hi Hans,
> > 
> > On Wed, May 20, 2026 at 12:01:46PM +0200, Hans Verkuil wrote:
> >> On 20/05/2026 11:34, Laurent Pinchart wrote:
> >>> On Wed, May 20, 2026 at 05:06:24PM +0800, Guangshuo Li wrote:
> >>>> video_register_device() / __video_register_device() registers vdev->dev
> >>>> with device_register(). Before the call the video core sets
> >>>>
> >>>> 	vdev->dev.release = v4l2_device_release;
> >>>>
> >>>> v4l2_device_release() invokes vdev->release(vdev) as its last step, and
> >>>> the driver's vdev->release hook is commonly video_device_release(), which
> >>>> kfree()s the vdev that the driver allocated with video_device_alloc().
> >>>>
> >>>> When device_register() fails inside __video_register_device() the core
> >>>> does
> >>>>
> >>>> 	put_device(&vdev->dev);
> >>>> 	return ret;
> >>>>
> >>>> which drops the only reference and fires the v4l2_device_release()
> >>>> chain:
> >>>>
> >>>>   __video_register_device()
> >>>>     device_register() -> -E*
> >>>>     put_device(&vdev->dev)
> >>>>       -> v4l2_device_release()
> >>>>          -> vdev->release(vdev)
> >>>>             -> video_device_release(vdev)   /* kfree(vdev), free #1 */
> >>>>
> >>>> video_register_device() returns the error to the driver. Drivers that
> >>>> follow the documented ownership contract release vdev on their own error
> >>>> path, e.g.
> >>>>
> >>>>   driver_probe()
> >>>>     if (video_register_device(vdev, ...))
> >>>>       goto err_release_vdev;
> >>>>     ...
> >>>>   err_release_vdev:
> >>>>     video_device_release(vdev);   /* free #2 -- DOUBLE FREE */
> >>>>
> >>>> This is the contract documented in
> >>>> Documentation/driver-api/media/v4l2-dev.rst: the driver owns vdev and
> >>>> is responsible for releasing it if video_register_device() fails. As
> >>>> Hans Verkuil pointed out, the right place to fix this is the v4l2 core
> >>>> rather than every individual driver, because drivers are expected to
> >>>> follow the documented ownership contract.
> >>>>
> >>>> Neutralise vdev->release around put_device() in the device_register()
> >>>> failure path so the device core cleanup does not run the driver's
> >>>> release hook. The driver-supplied release is restored before returning
> >>>> so the caller can release vdev according to the documented contract.
> >>>> Successful registration is unchanged, so the normal teardown sequence
> >>>> continues to call the driver's release hook and free vdev exactly once on
> >>>> unregister.
> >>>>
> >>>> Fixes: 2a934fdb01db ("media: v4l2-dev: fix error handling in __video_register_device()")
> >>>> Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
> >>>> ---
> >>>>  drivers/media/v4l2-core/v4l2-dev.c | 5 +++++
> >>>>  1 file changed, 5 insertions(+)
> >>>>
> >>>> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> >>>> index 6ce623a1245a..73648549eb2a 100644
> >>>> --- a/drivers/media/v4l2-core/v4l2-dev.c
> >>>> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> >>>> @@ -1075,9 +1075,14 @@ int __video_register_device(struct video_device *vdev,
> >>>>  	mutex_lock(&videodev_lock);
> >>>>  	ret = device_register(&vdev->dev);
> >>>>  	if (ret < 0) {
> >>>> +		void (*release)(struct video_device *) = vdev->release;
> >>>> +
> >>>>  		mutex_unlock(&videodev_lock);
> >>>>  		pr_err("%s: device_register failed\n", __func__);
> >>>> +
> >>>> +		vdev->release = video_device_release_empty;
> >>>>  		put_device(&vdev->dev);
> >>>> +		vdev->release = release;
> >>>
> >>> That looks like a big hack. There must be something wrong somewhere else
> >>> in the design.
> >>
> >> There is, unfortunately the design was wrong since the beginning of V4L2.
> >>
> >> Documentation/driver-api/media/v4l2-dev.rst explicitly says that you should use:
> >>
> >>         err = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
> >>         if (err) {
> >>                 video_device_release(vdev); /* or kfree(my_vdev); */
> >>                 return err;
> >>         }
> >>
> >> So everyone does that. Luckily device_register never fails in practice (you probably have
> >> bigger problems if it fails then just a double-free).
> >>
> >> The reality is that we don't handle this failure well at all, and this change wouldn't
> >> help in all cases either. E.g. drivers/media/platform/renesas/renesas-ceu.c actually relies
> >> on the release() callback in that it doesn't call video_device_release().
> >>
> >> But then it would fail on the v4l2_err(vdev->v4l2_dev, ...) call since vdev would be freed
> >> already.
> >>
> >> There are probably more drivers like that. (drivers/media/i2c/video-i2c.c)
> >>
> >> I'm not sure what is wisdom here.
> >>
> >> See also commit 2a934fdb01db ("media: v4l2-dev: fix error handling in __video_register_device()"),
> >> which is where the put_device was introduced in the first place.
> >>
> >> Perhaps that should be reverted instead?
> > 
> > If we want a short term fix I think that would be better.
> > 
> > Have you seen
> > https://lore.kernel.org/all/aeCOdWLaVpH-5w8s@hovoldconsulting.com/ ?
> 
> I hadn't seen it. Interesting.
> 
> > 
> > Having an API contract different from device_register() will likely
> > cause issues one way or another.
> 
> Looking closely how the driver core works and what commit 2a934fdb01db changed,
> I think this might fix it:
> 
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> index 5516b2bbb08f..6ffd385e880f 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -1071,25 +1071,27 @@ int __video_register_device(struct video_device *vdev,
>  	vdev->dev.class = &video_class;
>  	vdev->dev.devt = MKDEV(VIDEO_MAJOR, vdev->minor);
>  	vdev->dev.parent = vdev->dev_parent;
> -	vdev->dev.release = v4l2_device_release;
>  	dev_set_name(&vdev->dev, "%s%d", name_base, vdev->num);
> 
> -	/* Increase v4l2_device refcount */
> -	v4l2_device_get(vdev->v4l2_dev);
> -
>  	mutex_lock(&videodev_lock);
>  	ret = device_register(&vdev->dev);
>  	if (ret < 0) {
>  		mutex_unlock(&videodev_lock);
>  		pr_err("%s: device_register failed\n", __func__);
>  		put_device(&vdev->dev);
> -		return ret;
> +		goto cleanup;
>  	}
> +	/* Register the release callback that will be called when the last
> +	   reference to the device goes away. */
> +	vdev->dev.release = v4l2_device_release;
> 
>  	if (nr != -1 && nr != vdev->num && warn_if_nr_in_use)
>  		pr_warn("%s: requested %s%d, got %s\n", __func__,
>  			name_base, nr, video_device_node_name(vdev));
> 
> +	/* Increase v4l2_device refcount */
> +	v4l2_device_get(vdev->v4l2_dev);
> +
>  	/* Part 5: Register the entity. */
>  	ret = video_register_media_controller(vdev);
> 
> This mostly reverts 2a934fdb01db, except that we keep the put_device.
> But when this is called, vdev->dev.release isn't set yet, so it only
> frees the device-related data (device_release in drivers/base/core.c).
> 
> Am I missing something?

I guess this could be workable. This way the caller knows the release
callback won't be called.

Regarding error handling, the return value from
video_register_media_controller() is ignored. It'd probably be good to fix
that in a separate patch though. I could submit one as well.

-- 
Regards,

Sakari Ailus

On Wed, May 20, 2026 at 03:02:19PM +0300, Sakari Ailus wrote:
> On Wed, May 20, 2026 at 01:26:30PM +0200, Hans Verkuil wrote:
> > On 20/05/2026 12:48, Laurent Pinchart wrote:
> > > On Wed, May 20, 2026 at 12:01:46PM +0200, Hans Verkuil wrote:
> > >> On 20/05/2026 11:34, Laurent Pinchart wrote:
> > >>> On Wed, May 20, 2026 at 05:06:24PM +0800, Guangshuo Li wrote:
> > >>>> video_register_device() / __video_register_device() registers vdev->dev
> > >>>> with device_register(). Before the call the video core sets
> > >>>>
> > >>>> 	vdev->dev.release = v4l2_device_release;
> > >>>>
> > >>>> v4l2_device_release() invokes vdev->release(vdev) as its last step, and
> > >>>> the driver's vdev->release hook is commonly video_device_release(), which
> > >>>> kfree()s the vdev that the driver allocated with video_device_alloc().
> > >>>>
> > >>>> When device_register() fails inside __video_register_device() the core
> > >>>> does
> > >>>>
> > >>>> 	put_device(&vdev->dev);
> > >>>> 	return ret;
> > >>>>
> > >>>> which drops the only reference and fires the v4l2_device_release()
> > >>>> chain:
> > >>>>
> > >>>>   __video_register_device()
> > >>>>     device_register() -> -E*
> > >>>>     put_device(&vdev->dev)
> > >>>>       -> v4l2_device_release()
> > >>>>          -> vdev->release(vdev)
> > >>>>             -> video_device_release(vdev)   /* kfree(vdev), free #1 */
> > >>>>
> > >>>> video_register_device() returns the error to the driver. Drivers that
> > >>>> follow the documented ownership contract release vdev on their own error
> > >>>> path, e.g.
> > >>>>
> > >>>>   driver_probe()
> > >>>>     if (video_register_device(vdev, ...))
> > >>>>       goto err_release_vdev;
> > >>>>     ...
> > >>>>   err_release_vdev:
> > >>>>     video_device_release(vdev);   /* free #2 -- DOUBLE FREE */
> > >>>>
> > >>>> This is the contract documented in
> > >>>> Documentation/driver-api/media/v4l2-dev.rst: the driver owns vdev and
> > >>>> is responsible for releasing it if video_register_device() fails. As
> > >>>> Hans Verkuil pointed out, the right place to fix this is the v4l2 core
> > >>>> rather than every individual driver, because drivers are expected to
> > >>>> follow the documented ownership contract.
> > >>>>
> > >>>> Neutralise vdev->release around put_device() in the device_register()
> > >>>> failure path so the device core cleanup does not run the driver's
> > >>>> release hook. The driver-supplied release is restored before returning
> > >>>> so the caller can release vdev according to the documented contract.
> > >>>> Successful registration is unchanged, so the normal teardown sequence
> > >>>> continues to call the driver's release hook and free vdev exactly once on
> > >>>> unregister.
> > >>>>
> > >>>> Fixes: 2a934fdb01db ("media: v4l2-dev: fix error handling in __video_register_device()")
> > >>>> Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
> > >>>> ---
> > >>>>  drivers/media/v4l2-core/v4l2-dev.c | 5 +++++
> > >>>>  1 file changed, 5 insertions(+)
> > >>>>
> > >>>> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> > >>>> index 6ce623a1245a..73648549eb2a 100644
> > >>>> --- a/drivers/media/v4l2-core/v4l2-dev.c
> > >>>> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> > >>>> @@ -1075,9 +1075,14 @@ int __video_register_device(struct video_device *vdev,
> > >>>>  	mutex_lock(&videodev_lock);
> > >>>>  	ret = device_register(&vdev->dev);
> > >>>>  	if (ret < 0) {
> > >>>> +		void (*release)(struct video_device *) = vdev->release;
> > >>>> +
> > >>>>  		mutex_unlock(&videodev_lock);
> > >>>>  		pr_err("%s: device_register failed\n", __func__);
> > >>>> +
> > >>>> +		vdev->release = video_device_release_empty;
> > >>>>  		put_device(&vdev->dev);
> > >>>> +		vdev->release = release;
> > >>>
> > >>> That looks like a big hack. There must be something wrong somewhere else
> > >>> in the design.
> > >>
> > >> There is, unfortunately the design was wrong since the beginning of V4L2.
> > >>
> > >> Documentation/driver-api/media/v4l2-dev.rst explicitly says that you should use:
> > >>
> > >>         err = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
> > >>         if (err) {
> > >>                 video_device_release(vdev); /* or kfree(my_vdev); */
> > >>                 return err;
> > >>         }
> > >>
> > >> So everyone does that. Luckily device_register never fails in practice (you probably have
> > >> bigger problems if it fails then just a double-free).
> > >>
> > >> The reality is that we don't handle this failure well at all, and this change wouldn't
> > >> help in all cases either. E.g. drivers/media/platform/renesas/renesas-ceu.c actually relies
> > >> on the release() callback in that it doesn't call video_device_release().
> > >>
> > >> But then it would fail on the v4l2_err(vdev->v4l2_dev, ...) call since vdev would be freed
> > >> already.
> > >>
> > >> There are probably more drivers like that. (drivers/media/i2c/video-i2c.c)
> > >>
> > >> I'm not sure what is wisdom here.
> > >>
> > >> See also commit 2a934fdb01db ("media: v4l2-dev: fix error handling in __video_register_device()"),
> > >> which is where the put_device was introduced in the first place.
> > >>
> > >> Perhaps that should be reverted instead?
> > > 
> > > If we want a short term fix I think that would be better.
> > > 
> > > Have you seen
> > > https://lore.kernel.org/all/aeCOdWLaVpH-5w8s@hovoldconsulting.com/ ?
> > 
> > I hadn't seen it. Interesting.
> > 
> > > 
> > > Having an API contract different from device_register() will likely
> > > cause issues one way or another.
> > 
> > Looking closely how the driver core works and what commit 2a934fdb01db changed,
> > I think this might fix it:
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> > index 5516b2bbb08f..6ffd385e880f 100644
> > --- a/drivers/media/v4l2-core/v4l2-dev.c
> > +++ b/drivers/media/v4l2-core/v4l2-dev.c
> > @@ -1071,25 +1071,27 @@ int __video_register_device(struct video_device *vdev,
> >  	vdev->dev.class = &video_class;
> >  	vdev->dev.devt = MKDEV(VIDEO_MAJOR, vdev->minor);
> >  	vdev->dev.parent = vdev->dev_parent;
> > -	vdev->dev.release = v4l2_device_release;
> >  	dev_set_name(&vdev->dev, "%s%d", name_base, vdev->num);
> > 
> > -	/* Increase v4l2_device refcount */
> > -	v4l2_device_get(vdev->v4l2_dev);
> > -
> >  	mutex_lock(&videodev_lock);
> >  	ret = device_register(&vdev->dev);
> >  	if (ret < 0) {
> >  		mutex_unlock(&videodev_lock);
> >  		pr_err("%s: device_register failed\n", __func__);
> >  		put_device(&vdev->dev);
> > -		return ret;
> > +		goto cleanup;
> >  	}
> > +	/* Register the release callback that will be called when the last
> > +	   reference to the device goes away. */
> > +	vdev->dev.release = v4l2_device_release;
> > 
> >  	if (nr != -1 && nr != vdev->num && warn_if_nr_in_use)
> >  		pr_warn("%s: requested %s%d, got %s\n", __func__,
> >  			name_base, nr, video_device_node_name(vdev));
> > 
> > +	/* Increase v4l2_device refcount */
> > +	v4l2_device_get(vdev->v4l2_dev);
> > +
> >  	/* Part 5: Register the entity. */
> >  	ret = video_register_media_controller(vdev);
> > 
> > This mostly reverts 2a934fdb01db, except that we keep the put_device.
> > But when this is called, vdev->dev.release isn't set yet, so it only
> > frees the device-related data (device_release in drivers/base/core.c).
> > 
> > Am I missing something?
> 
> I guess this could be workable. This way the caller knows the release
> callback won't be called.

I think it's a short term hack at best though :-( If the device core
requires reference-counting for struct device, with a requirement to
call put_device() instead of just freeing the structure, then anything
that embeds a struct device should do the same. That means exposing
video_put() (which should probably be renamed to video_device_put()) and
calling that in the error path, in the caller.

We may also want to split video_register_device() into
video_device_init() and video_device_add(), but that's a separate
question.

> Regarding error handling, the return value from
> video_register_media_controller() is ignored. It'd probably be good to fix
> that in a separate patch though. I could submit one as well.

-- 
Regards,

Laurent Pinchart

On 20/05/2026 14:41, Laurent Pinchart wrote:
> On Wed, May 20, 2026 at 03:02:19PM +0300, Sakari Ailus wrote:
>> On Wed, May 20, 2026 at 01:26:30PM +0200, Hans Verkuil wrote:
>>> On 20/05/2026 12:48, Laurent Pinchart wrote:
>>>> On Wed, May 20, 2026 at 12:01:46PM +0200, Hans Verkuil wrote:
>>>>> On 20/05/2026 11:34, Laurent Pinchart wrote:
>>>>>> On Wed, May 20, 2026 at 05:06:24PM +0800, Guangshuo Li wrote:
>>>>>>> video_register_device() / __video_register_device() registers vdev->dev
>>>>>>> with device_register(). Before the call the video core sets
>>>>>>>
>>>>>>> 	vdev->dev.release = v4l2_device_release;
>>>>>>>
>>>>>>> v4l2_device_release() invokes vdev->release(vdev) as its last step, and
>>>>>>> the driver's vdev->release hook is commonly video_device_release(), which
>>>>>>> kfree()s the vdev that the driver allocated with video_device_alloc().
>>>>>>>
>>>>>>> When device_register() fails inside __video_register_device() the core
>>>>>>> does
>>>>>>>
>>>>>>> 	put_device(&vdev->dev);
>>>>>>> 	return ret;
>>>>>>>
>>>>>>> which drops the only reference and fires the v4l2_device_release()
>>>>>>> chain:
>>>>>>>
>>>>>>>   __video_register_device()
>>>>>>>     device_register() -> -E*
>>>>>>>     put_device(&vdev->dev)
>>>>>>>       -> v4l2_device_release()
>>>>>>>          -> vdev->release(vdev)
>>>>>>>             -> video_device_release(vdev)   /* kfree(vdev), free #1 */
>>>>>>>
>>>>>>> video_register_device() returns the error to the driver. Drivers that
>>>>>>> follow the documented ownership contract release vdev on their own error
>>>>>>> path, e.g.
>>>>>>>
>>>>>>>   driver_probe()
>>>>>>>     if (video_register_device(vdev, ...))
>>>>>>>       goto err_release_vdev;
>>>>>>>     ...
>>>>>>>   err_release_vdev:
>>>>>>>     video_device_release(vdev);   /* free #2 -- DOUBLE FREE */
>>>>>>>
>>>>>>> This is the contract documented in
>>>>>>> Documentation/driver-api/media/v4l2-dev.rst: the driver owns vdev and
>>>>>>> is responsible for releasing it if video_register_device() fails. As
>>>>>>> Hans Verkuil pointed out, the right place to fix this is the v4l2 core
>>>>>>> rather than every individual driver, because drivers are expected to
>>>>>>> follow the documented ownership contract.
>>>>>>>
>>>>>>> Neutralise vdev->release around put_device() in the device_register()
>>>>>>> failure path so the device core cleanup does not run the driver's
>>>>>>> release hook. The driver-supplied release is restored before returning
>>>>>>> so the caller can release vdev according to the documented contract.
>>>>>>> Successful registration is unchanged, so the normal teardown sequence
>>>>>>> continues to call the driver's release hook and free vdev exactly once on
>>>>>>> unregister.
>>>>>>>
>>>>>>> Fixes: 2a934fdb01db ("media: v4l2-dev: fix error handling in __video_register_device()")
>>>>>>> Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
>>>>>>> ---
>>>>>>>  drivers/media/v4l2-core/v4l2-dev.c | 5 +++++
>>>>>>>  1 file changed, 5 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
>>>>>>> index 6ce623a1245a..73648549eb2a 100644
>>>>>>> --- a/drivers/media/v4l2-core/v4l2-dev.c
>>>>>>> +++ b/drivers/media/v4l2-core/v4l2-dev.c
>>>>>>> @@ -1075,9 +1075,14 @@ int __video_register_device(struct video_device *vdev,
>>>>>>>  	mutex_lock(&videodev_lock);
>>>>>>>  	ret = device_register(&vdev->dev);
>>>>>>>  	if (ret < 0) {
>>>>>>> +		void (*release)(struct video_device *) = vdev->release;
>>>>>>> +
>>>>>>>  		mutex_unlock(&videodev_lock);
>>>>>>>  		pr_err("%s: device_register failed\n", __func__);
>>>>>>> +
>>>>>>> +		vdev->release = video_device_release_empty;
>>>>>>>  		put_device(&vdev->dev);
>>>>>>> +		vdev->release = release;
>>>>>>
>>>>>> That looks like a big hack. There must be something wrong somewhere else
>>>>>> in the design.
>>>>>
>>>>> There is, unfortunately the design was wrong since the beginning of V4L2.
>>>>>
>>>>> Documentation/driver-api/media/v4l2-dev.rst explicitly says that you should use:
>>>>>
>>>>>         err = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
>>>>>         if (err) {
>>>>>                 video_device_release(vdev); /* or kfree(my_vdev); */
>>>>>                 return err;
>>>>>         }
>>>>>
>>>>> So everyone does that. Luckily device_register never fails in practice (you probably have
>>>>> bigger problems if it fails then just a double-free).
>>>>>
>>>>> The reality is that we don't handle this failure well at all, and this change wouldn't
>>>>> help in all cases either. E.g. drivers/media/platform/renesas/renesas-ceu.c actually relies
>>>>> on the release() callback in that it doesn't call video_device_release().
>>>>>
>>>>> But then it would fail on the v4l2_err(vdev->v4l2_dev, ...) call since vdev would be freed
>>>>> already.
>>>>>
>>>>> There are probably more drivers like that. (drivers/media/i2c/video-i2c.c)
>>>>>
>>>>> I'm not sure what is wisdom here.
>>>>>
>>>>> See also commit 2a934fdb01db ("media: v4l2-dev: fix error handling in __video_register_device()"),
>>>>> which is where the put_device was introduced in the first place.
>>>>>
>>>>> Perhaps that should be reverted instead?
>>>>
>>>> If we want a short term fix I think that would be better.
>>>>
>>>> Have you seen
>>>> https://lore.kernel.org/all/aeCOdWLaVpH-5w8s@hovoldconsulting.com/ ?
>>>
>>> I hadn't seen it. Interesting.
>>>
>>>>
>>>> Having an API contract different from device_register() will likely
>>>> cause issues one way or another.
>>>
>>> Looking closely how the driver core works and what commit 2a934fdb01db changed,
>>> I think this might fix it:
>>>
>>> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
>>> index 5516b2bbb08f..6ffd385e880f 100644
>>> --- a/drivers/media/v4l2-core/v4l2-dev.c
>>> +++ b/drivers/media/v4l2-core/v4l2-dev.c
>>> @@ -1071,25 +1071,27 @@ int __video_register_device(struct video_device *vdev,
>>>  	vdev->dev.class = &video_class;
>>>  	vdev->dev.devt = MKDEV(VIDEO_MAJOR, vdev->minor);
>>>  	vdev->dev.parent = vdev->dev_parent;
>>> -	vdev->dev.release = v4l2_device_release;
>>>  	dev_set_name(&vdev->dev, "%s%d", name_base, vdev->num);
>>>
>>> -	/* Increase v4l2_device refcount */
>>> -	v4l2_device_get(vdev->v4l2_dev);
>>> -
>>>  	mutex_lock(&videodev_lock);
>>>  	ret = device_register(&vdev->dev);
>>>  	if (ret < 0) {
>>>  		mutex_unlock(&videodev_lock);
>>>  		pr_err("%s: device_register failed\n", __func__);
>>>  		put_device(&vdev->dev);
>>> -		return ret;
>>> +		goto cleanup;
>>>  	}
>>> +	/* Register the release callback that will be called when the last
>>> +	   reference to the device goes away. */
>>> +	vdev->dev.release = v4l2_device_release;
>>>
>>>  	if (nr != -1 && nr != vdev->num && warn_if_nr_in_use)
>>>  		pr_warn("%s: requested %s%d, got %s\n", __func__,
>>>  			name_base, nr, video_device_node_name(vdev));
>>>
>>> +	/* Increase v4l2_device refcount */
>>> +	v4l2_device_get(vdev->v4l2_dev);
>>> +
>>>  	/* Part 5: Register the entity. */
>>>  	ret = video_register_media_controller(vdev);
>>>
>>> This mostly reverts 2a934fdb01db, except that we keep the put_device.
>>> But when this is called, vdev->dev.release isn't set yet, so it only
>>> frees the device-related data (device_release in drivers/base/core.c).
>>>
>>> Am I missing something?
>>
>> I guess this could be workable. This way the caller knows the release
>> callback won't be called.
> 
> I think it's a short term hack at best though :-( If the device core
> requires reference-counting for struct device, with a requirement to
> call put_device() instead of just freeing the structure, then anything
> that embeds a struct device should do the same. That means exposing
> video_put() (which should probably be renamed to video_device_put()) and
> calling that in the error path, in the caller.
> 
> We may also want to split video_register_device() into
> video_device_init() and video_device_add(), but that's a separate
> question.

I believe that would be the right approach long-term. I actually started
on that once, but very quickly ran out of time.

Regards,

	Hans

> 
>> Regarding error handling, the return value from
>> video_register_media_controller() is ignored. It'd probably be good to fix
>> that in a separate patch though. I could submit one as well.
>

Hi Hans,

On Wed, 20 May 2026 at 20:45, Hans Verkuil <hverkuil+cisco@kernel.org> wrote:
>
> On 20/05/2026 14:41, Laurent Pinchart wrote:
> > On Wed, May 20, 2026 at 03:02:19PM +0300, Sakari Ailus wrote:
> >> On Wed, May 20, 2026 at 01:26:30PM +0200, Hans Verkuil wrote:
> >>> On 20/05/2026 12:48, Laurent Pinchart wrote:
> >>>> On Wed, May 20, 2026 at 12:01:46PM +0200, Hans Verkuil wrote:
> >>>>> On 20/05/2026 11:34, Laurent Pinchart wrote:
> >>>>>> On Wed, May 20, 2026 at 05:06:24PM +0800, Guangshuo Li wrote:
> >>>>>>> video_register_device() / __video_register_device() registers vdev->dev
> >>>>>>> with device_register(). Before the call the video core sets
> >>>>>>>
> >>>>>>>         vdev->dev.release = v4l2_device_release;
> >>>>>>>
> >>>>>>> v4l2_device_release() invokes vdev->release(vdev) as its last step, and
> >>>>>>> the driver's vdev->release hook is commonly video_device_release(), which
> >>>>>>> kfree()s the vdev that the driver allocated with video_device_alloc().
> >>>>>>>
> >>>>>>> When device_register() fails inside __video_register_device() the core
> >>>>>>> does
> >>>>>>>
> >>>>>>>         put_device(&vdev->dev);
> >>>>>>>         return ret;
> >>>>>>>
> >>>>>>> which drops the only reference and fires the v4l2_device_release()
> >>>>>>> chain:
> >>>>>>>
> >>>>>>>   __video_register_device()
> >>>>>>>     device_register() -> -E*
> >>>>>>>     put_device(&vdev->dev)
> >>>>>>>       -> v4l2_device_release()
> >>>>>>>          -> vdev->release(vdev)
> >>>>>>>             -> video_device_release(vdev)   /* kfree(vdev), free #1 */
> >>>>>>>
> >>>>>>> video_register_device() returns the error to the driver. Drivers that
> >>>>>>> follow the documented ownership contract release vdev on their own error
> >>>>>>> path, e.g.
> >>>>>>>
> >>>>>>>   driver_probe()
> >>>>>>>     if (video_register_device(vdev, ...))
> >>>>>>>       goto err_release_vdev;
> >>>>>>>     ...
> >>>>>>>   err_release_vdev:
> >>>>>>>     video_device_release(vdev);   /* free #2 -- DOUBLE FREE */
> >>>>>>>
> >>>>>>> This is the contract documented in
> >>>>>>> Documentation/driver-api/media/v4l2-dev.rst: the driver owns vdev and
> >>>>>>> is responsible for releasing it if video_register_device() fails. As
> >>>>>>> Hans Verkuil pointed out, the right place to fix this is the v4l2 core
> >>>>>>> rather than every individual driver, because drivers are expected to
> >>>>>>> follow the documented ownership contract.
> >>>>>>>
> >>>>>>> Neutralise vdev->release around put_device() in the device_register()
> >>>>>>> failure path so the device core cleanup does not run the driver's
> >>>>>>> release hook. The driver-supplied release is restored before returning
> >>>>>>> so the caller can release vdev according to the documented contract.
> >>>>>>> Successful registration is unchanged, so the normal teardown sequence
> >>>>>>> continues to call the driver's release hook and free vdev exactly once on
> >>>>>>> unregister.
> >>>>>>>
> >>>>>>> Fixes: 2a934fdb01db ("media: v4l2-dev: fix error handling in __video_register_device()")
> >>>>>>> Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
> >>>>>>> ---
> >>>>>>>  drivers/media/v4l2-core/v4l2-dev.c | 5 +++++
> >>>>>>>  1 file changed, 5 insertions(+)
> >>>>>>>
> >>>>>>> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> >>>>>>> index 6ce623a1245a..73648549eb2a 100644
> >>>>>>> --- a/drivers/media/v4l2-core/v4l2-dev.c
> >>>>>>> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> >>>>>>> @@ -1075,9 +1075,14 @@ int __video_register_device(struct video_device *vdev,
> >>>>>>>         mutex_lock(&videodev_lock);
> >>>>>>>         ret = device_register(&vdev->dev);
> >>>>>>>         if (ret < 0) {
> >>>>>>> +               void (*release)(struct video_device *) = vdev->release;
> >>>>>>> +
> >>>>>>>                 mutex_unlock(&videodev_lock);
> >>>>>>>                 pr_err("%s: device_register failed\n", __func__);
> >>>>>>> +
> >>>>>>> +               vdev->release = video_device_release_empty;
> >>>>>>>                 put_device(&vdev->dev);
> >>>>>>> +               vdev->release = release;
> >>>>>>
> >>>>>> That looks like a big hack. There must be something wrong somewhere else
> >>>>>> in the design.
> >>>>>
> >>>>> There is, unfortunately the design was wrong since the beginning of V4L2.
> >>>>>
> >>>>> Documentation/driver-api/media/v4l2-dev.rst explicitly says that you should use:
> >>>>>
> >>>>>         err = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
> >>>>>         if (err) {
> >>>>>                 video_device_release(vdev); /* or kfree(my_vdev); */
> >>>>>                 return err;
> >>>>>         }
> >>>>>
> >>>>> So everyone does that. Luckily device_register never fails in practice (you probably have
> >>>>> bigger problems if it fails then just a double-free).
> >>>>>
> >>>>> The reality is that we don't handle this failure well at all, and this change wouldn't
> >>>>> help in all cases either. E.g. drivers/media/platform/renesas/renesas-ceu.c actually relies
> >>>>> on the release() callback in that it doesn't call video_device_release().
> >>>>>
> >>>>> But then it would fail on the v4l2_err(vdev->v4l2_dev, ...) call since vdev would be freed
> >>>>> already.
> >>>>>
> >>>>> There are probably more drivers like that. (drivers/media/i2c/video-i2c.c)
> >>>>>
> >>>>> I'm not sure what is wisdom here.
> >>>>>
> >>>>> See also commit 2a934fdb01db ("media: v4l2-dev: fix error handling in __video_register_device()"),
> >>>>> which is where the put_device was introduced in the first place.
> >>>>>
> >>>>> Perhaps that should be reverted instead?
> >>>>
> >>>> If we want a short term fix I think that would be better.
> >>>>
> >>>> Have you seen
> >>>> https://lore.kernel.org/all/aeCOdWLaVpH-5w8s@hovoldconsulting.com/ ?
> >>>
> >>> I hadn't seen it. Interesting.
> >>>
> >>>>
> >>>> Having an API contract different from device_register() will likely
> >>>> cause issues one way or another.
> >>>
> >>> Looking closely how the driver core works and what commit 2a934fdb01db changed,
> >>> I think this might fix it:
> >>>
> >>> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> >>> index 5516b2bbb08f..6ffd385e880f 100644
> >>> --- a/drivers/media/v4l2-core/v4l2-dev.c
> >>> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> >>> @@ -1071,25 +1071,27 @@ int __video_register_device(struct video_device *vdev,
> >>>     vdev->dev.class = &video_class;
> >>>     vdev->dev.devt = MKDEV(VIDEO_MAJOR, vdev->minor);
> >>>     vdev->dev.parent = vdev->dev_parent;
> >>> -   vdev->dev.release = v4l2_device_release;
> >>>     dev_set_name(&vdev->dev, "%s%d", name_base, vdev->num);
> >>>
> >>> -   /* Increase v4l2_device refcount */
> >>> -   v4l2_device_get(vdev->v4l2_dev);
> >>> -
> >>>     mutex_lock(&videodev_lock);
> >>>     ret = device_register(&vdev->dev);
> >>>     if (ret < 0) {
> >>>             mutex_unlock(&videodev_lock);
> >>>             pr_err("%s: device_register failed\n", __func__);
> >>>             put_device(&vdev->dev);
> >>> -           return ret;
> >>> +           goto cleanup;
> >>>     }
> >>> +   /* Register the release callback that will be called when the last
> >>> +      reference to the device goes away. */
> >>> +   vdev->dev.release = v4l2_device_release;
> >>>
> >>>     if (nr != -1 && nr != vdev->num && warn_if_nr_in_use)
> >>>             pr_warn("%s: requested %s%d, got %s\n", __func__,
> >>>                     name_base, nr, video_device_node_name(vdev));
> >>>
> >>> +   /* Increase v4l2_device refcount */
> >>> +   v4l2_device_get(vdev->v4l2_dev);
> >>> +
> >>>     /* Part 5: Register the entity. */
> >>>     ret = video_register_media_controller(vdev);
> >>>
> >>> This mostly reverts 2a934fdb01db, except that we keep the put_device.
> >>> But when this is called, vdev->dev.release isn't set yet, so it only
> >>> frees the device-related data (device_release in drivers/base/core.c).
> >>>
> >>> Am I missing something?
> >>
> >> I guess this could be workable. This way the caller knows the release
> >> callback won't be called.
> >
I may be missing something, but I think the proposed change could
still cause a driver-core warning.

With `vdev->dev.release = v4l2_device_release` moved after
`device_register()`, the error path would call:

put_device(&vdev->dev);

while `vdev->dev.release` is still NULL. Unless either
`vdev->dev.type->release` or `vdev->dev.class->dev_release` is set,
`device_release()` will fall through to the WARN path:

WARN(1, KERN_ERR "Device '%s' does not have a release() function, it
is broken and must be fixed. ...",
     dev_name(dev));

See:
https://codebrowser.dev/linux/linux/drivers/base/core.c.html

So I think the object still needs a valid release path before
`put_device()` can drop the last reference. Delaying
`vdev->dev.release` would avoid calling `v4l2_device_release()` and
thus avoid the double-free, but it may trade that for the driver-core
“no release() function” warning.

Maybe the short-term fix still needs a release callback that keeps the
driver core happy without invoking the driver-provided
`vdev->release()` on the `device_register()` failure path.

> > I think it's a short term hack at best though :-( If the device core
> > requires reference-counting for struct device, with a requirement to
> > call put_device() instead of just freeing the structure, then anything
> > that embeds a struct device should do the same. That means exposing
> > video_put() (which should probably be renamed to video_device_put()) and
> > calling that in the error path, in the caller.
> >
> > We may also want to split video_register_device() into
> > video_device_init() and video_device_add(), but that's a separate
> > question.
>
> I believe that would be the right approach long-term. I actually started
> on that once, but very quickly ran out of time.
>
> Regards,
>
>         Hans
>
> >
> >> Regarding error handling, the return value from
> >> video_register_media_controller() is ignored. It'd probably be good to fix
> >> that in a separate patch though. I could submit one as well.
> >
>
Long-term, I agree with Laurent that a `video_device_put()`-style API,
possibly together with an init/add split, would make the lifetime
rules much clearer.

Regards,
Guangshuo