Re: [PATCH v3 01/10] media: synopsys: hdmirx: free the driver data when the last user is gone

[Hans Verkuil <hverkuil+cisco@kernel.org>]

On 04/11/2026 15:18, Dmitry Osipenko wrote:
> On 10/16/26 14:06, Sascha Hauer wrote:
>> Hi Dmitry,
>>
>> On 2026-10-07 14:20, Dmitry Osipenko wrote:
>>> On 10/5/26 16:28, Sascha Hauer wrote:
>>>> @@ -2792,16 +2807,13 @@ static void hdmirx_remove(struct platform_device *pdev)
>>>>   	hdmirx_disable_irq(dev);
>>>>   
>>>>   	vb2_video_unregister_device(&hdmirx_dev->stream.vdev);
>>>> -	v4l2_ctrl_handler_free(&hdmirx_dev->hdl);
>>>>   	v4l2_device_unregister(&hdmirx_dev->v4l2_dev);
>>>>   
>>>> -	/* touched by hdmirx_disable()->hdmirx_plugout() */
>>>> -	hdmirx_dev->rgb_range = NULL;
>>>> -	hdmirx_dev->content_type = NULL;
>>>> -
>>>>   	hdmirx_disable(dev);
>>>>   
>>>>   	reset_control_bulk_assert(HDMIRX_NUM_RST, hdmirx_dev->resets);
>>>> +
>>>> +	v4l2_device_put(&hdmirx_dev->v4l2_dev);
>>>>   }
>>>
>>> This is uncommon v4l driver removal pattern, I suspect it's either not
>>> correct or needs to be fixed in the core code.
>>
>> The pattern looks uncommon because many drivers in the tree are affected
>> by the same issue ;)
>>
>> Just like many other drivers the hdmirx embeds a struct v4l2_device in
>> its private data struct which is allocated with devm_*, thus freed
>> during driver removal. With an opened file descriptor the video_device
>> outlives the driver removal, and v4l2_release(), called on
>> the last file descriptors close then references the already freed memory
>> via vdev->v4l2_dev
>>
>> This is a long standing problem. See [1] for example for an attempt to
>> fix it in the core. It seems consencsus at that time was that it must be
>> fixed in the drivers which is what this patch does for the hdmirx.
>>
>> [1] https://lore.kernel.org/all/20240610100530.1107771-1-sakari.ailus@linux.intel.com/
>>
>>>
>>> What is the user-visible or sanitizer-visible problem? How to reproduce it?
>>
>> It can be reproduced with Kasan on affected drivers, below on the hdmirx
>> driver, but it should be reproducible like this on hantro and others as
>> well.
>> ...
> Thanks, the patches are good to me (tested on 5b with no problems found) 
> apart from this one that feels like it's not a proper solution.
> 
> Will be good to have input from the v4l core maintainers, or have this 
> fix made separately from the series to not hold rest of the patches.
> 

There are two possible options a driver can use: if the driver has just a single
video device that it registers, then it can use the release callback of the
video_device to do the cleanup. If there are multiple video_device structs, then
the release callback of v4l2_device can be used.

Using the release callback of v4l2_device is the more generic option, so this
is fine.

Many older v4l2 drivers do this wrong, mostly due to a poor understanding of
how this worked among media developers at the time.

In any case, I'll take this patch. It's good.

Regards,

	Hans