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

[Sakari Ailus <sakari.ailus@linux.intel.com>]

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/

?

> >> 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.

> 
> > 
> >>
> >> 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.

> >>  		}
> >>  	}
> >>  
> > 
> 

-- 
Regards,

Sakari Ailus