Re: [PATCHv3 1/6] media: core: v4l2-async.c: unreg subdev if asc_list is empty

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

On 6/30/26 10:45, Sakari Ailus wrote:
> Hi Hans,
> 
> On the subject -- I'd write this as, to align with the existing practice:
> 
> media: v4l2-async: Unregister sub-device if asc_list is empty
> 
> On Tue, Jun 30, 2026 at 10:32:20AM +0200, Hans Verkuil wrote:
>> On 6/30/26 09:58, Sakari Ailus wrote:
>>> Hi Hans,
>>>
>>> Thanks for the patch.
>>>
>>> On Mon, Jun 29, 2026 at 08:55:49AM +0200, Hans Verkuil wrote:
>>>> In v4l2_device_unregister_subdev(), if sd->asc_list is empty,
> 
> s/device/async/
> 
> ?

Yes. Oops, that's a confusing typo!

> 
>>>> then v4l2_device_unregister_subdev() is never called, but that
>>>> should still happen.
>>>>
>>>> This causes crashes with em28xx that uses tvp5150: that i2c
>>>> module uses v4l2_async, but em28xx does not as it predates
>>>> v4l2_async.
>>>
>>> Isn't the sub-device unregistered via v4l2_device_unregister() or do I miss
>>> something? em28xx_v4l2_fini() calls it.
>>>
>>> Or do you mean the issue happens at tvp5150 unbind time?
>>
>> Yes, that's when it happens. The tvp5150 driver is removed, it calls
>> v4l2_async_unregister_subdev(), but that never calls
>> v4l2_device_unregister_subdev() since em28xx doesn't use v4l2-async
>> and so sd->asc_list is empty.
>>
>> This happens when unplugging the USB device.
> 
> Ack.
> 
> Could you add this to the commit message? It'd help understanding what
> really goes on here. I'd also add a comment, see below.

OK.

> 
>>
>>>
>>>>
>>>> So if sd->asc_list is empty, then just call
>>>> v4l2_device_unregister_subdev().
>>>>
>>>> Fixes: 28a1295795d8 ("media: v4l: async: Allow multiple connections between entities")
>>
>> It was introduced by commit 28a1295795d8, before that commit this was done correctly.
>>
>> Regards,
>>
>> 	Hans
>>
>>>> Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
>>>> ---
>>>>  drivers/media/v4l2-core/v4l2-async.c | 10 +++++++---
>>>>  1 file changed, 7 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
>>>> index 888a2e213b08..d7437c958028 100644
>>>> --- a/drivers/media/v4l2-core/v4l2-async.c
>>>> +++ b/drivers/media/v4l2-core/v4l2-async.c
>>>> @@ -898,9 +898,13 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd)
>>>>  	sd->subdev_notifier = NULL;
>>>>  
>>>>  	if (sd->asc_list.next) {
>>>> -		list_for_each_entry_safe(asc, asc_tmp, &sd->asc_list,
>>>> -					 asc_subdev_entry) {
>>>> -			v4l2_async_unbind_subdev_one(asc->notifier, asc);
>>>> +		if (list_empty(&sd->asc_list)) {
> 
> How about:
> 
> 			/*
> 			 * If the sub-device was registered through other means
> 			 * than v4l2-async, there are no async connections but
> 			 * the sub-device may still well be registered.
> 			 * Unregister it now.
> 			 */
> 
>>>> +			v4l2_device_unregister_subdev(sd);
>>>> +		} else {
>>>> +			list_for_each_entry_safe(asc, asc_tmp, &sd->asc_list,
>>>> +						 asc_subdev_entry) {
>>>> +				v4l2_async_unbind_subdev_one(asc->notifier, asc);
>>>> +			}
> 
> These braces are redundant.
> 
>>>>  		}
>>>>  	}
>>>>  
>>>
>>
> 

I'll prep a v4.

Regards,

Hans