[PATCHv3 0/6] media: em28xx: fix lifecycle issues

[PATCHv3 0/6] media: em28xx: fix lifecycle issues

[Hans Verkuil]

The first patch fixes (I believe) a bug in v4l2-async that
was introduced back in 2013. Sakari, can you take a look at this?

The second patch switches the em28xx driver to use the v4l2_device
release callback, letting the v4l2 core do all the refcounting
until the last user is gone and it is safe to free all memory.

The third patch drops the 'users' counter and uses the internal
v4l2_fh counter instead. The fourth patch switches
video_unregister_device to vb2_video_unregister_device: this
ensures all streaming is stopped when the video device is unregistered.

The fifth patch fixes a use-after-freed, and the last patch correctly
gives the vb2 buffers back to the vb2 framework if start_streaming
fails.

This has all been tested on my em28xx device.

Regards,

	Hans

Changes since v2:
- keep check if sd->asc_list.next is non-NULL in patch 1/6

Hans Verkuil (6):
  media: core: v4l2-async.c: unreg subdev if asc_list is empty
  media: em28xx: use v4l2_device release callback
  media: em28xx: drop 'users' field
  media: em28xx: use vb2_video_unregister_device
  media: em28xx: dev_info->pr_info since dev has been freed
  media: em28xx: requeue buffers if start_streaming fails

 drivers/media/usb/em28xx/em28xx-cards.c |   2 +-
 drivers/media/usb/em28xx/em28xx-video.c | 114 +++++++++++++++---------
 drivers/media/usb/em28xx/em28xx.h       |   2 -
 drivers/media/v4l2-core/v4l2-async.c    |  10 ++-
 4 files changed, 78 insertions(+), 50 deletions(-)

-- 
2.53.0

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

[Hans Verkuil]

In v4l2_device_unregister_subdev(), if sd->asc_list is empty,
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.

So if sd->asc_list is empty, then just call
v4l2_device_unregister_subdev().

Fixes: 28a1295795d8 ("media: v4l: async: Allow multiple connections between entities")
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)) {
+			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);
+			}
 		}
 	}
 
-- 
2.53.0

[PATCHv3 2/6] media: em28xx: use v4l2_device release callback

[Hans Verkuil]

The em28xx driver creates a lot of video devices, but life-time management
is really bad. Instead use the struct v4l2_device release() callback to
have a single place where memory can be freed once the last user has gone.

Assisted-by: Claude:claude-opus-4-7
Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
---
 drivers/media/usb/em28xx/em28xx-video.c | 66 +++++++++++++++----------
 drivers/media/usb/em28xx/em28xx.h       |  1 -
 2 files changed, 41 insertions(+), 26 deletions(-)

diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
index da0422c65e5f..42f5e7547cc4 100644
--- a/drivers/media/usb/em28xx/em28xx-video.c
+++ b/drivers/media/usb/em28xx/em28xx-video.c
@@ -2275,18 +2275,27 @@ static int radio_s_tuner(struct file *file, void *priv,
 }
 
 /*
- * em28xx_free_v4l2() - Free struct em28xx_v4l2
+ * em28xx_free_v4l2() - v4l2_device release callback
  *
- * @ref: struct kref for struct em28xx_v4l2
+ * @v4l2_dev: pointer to struct v4l2_device embedded in struct em28xx_v4l2
  *
- * Called when all users of struct em28xx_v4l2 are gone
+ * Called by the v4l2 core when the last reference to the v4l2_device is
+ * released. At this point no userspace file handle nor video_device node
+ * keeps the v4l2 instance alive anymore, so it is safe to release all
+ * v4l2-related resources and drop the em28xx device reference taken when
+ * the v4l2 extension was initialized.
  */
-static void em28xx_free_v4l2(struct kref *ref)
+static void em28xx_free_v4l2(struct v4l2_device *v4l2_dev)
 {
-	struct em28xx_v4l2 *v4l2 = container_of(ref, struct em28xx_v4l2, ref);
+	struct em28xx_v4l2 *v4l2 =
+		container_of(v4l2_dev, struct em28xx_v4l2, v4l2_dev);
+	struct em28xx *dev = v4l2->dev;
 
-	v4l2->dev->v4l2 = NULL;
+	v4l2_ctrl_handler_free(&v4l2->ctrl_handler);
+	v4l2_device_unregister(v4l2_dev);
+	dev->v4l2 = NULL;
 	kfree(v4l2);
+	kref_put(&dev->ref, em28xx_free_device);
 }
 
 /*
@@ -2354,8 +2363,6 @@ static int em28xx_v4l2_open(struct file *filp)
 		v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, s_radio);
 	}
 
-	kref_get(&dev->ref);
-	kref_get(&v4l2->ref);
 	v4l2->users++;
 
 	mutex_unlock(&dev->lock);
@@ -2411,14 +2418,14 @@ static int em28xx_v4l2_fini(struct em28xx *dev)
 		video_unregister_device(&v4l2->vdev);
 	}
 
-	v4l2_ctrl_handler_free(&v4l2->ctrl_handler);
-	v4l2_device_unregister(&v4l2->v4l2_dev);
-
-	kref_put(&v4l2->ref, em28xx_free_v4l2);
-
 	mutex_unlock(&dev->lock);
 
-	kref_put(&dev->ref, em28xx_free_device);
+	/*
+	 * Drop the initial reference taken at v4l2_device_register() time.
+	 * The em28xx_free_v4l2() release callback will be invoked once all
+	 * userspace file handles to the video device nodes are closed.
+	 */
+	v4l2_device_put(&v4l2->v4l2_dev);
 
 	return 0;
 }
@@ -2490,9 +2497,7 @@ static int em28xx_v4l2_close(struct file *filp)
 
 exit:
 	v4l2->users--;
-	kref_put(&v4l2->ref, em28xx_free_v4l2);
 	mutex_unlock(&dev->lock);
-	kref_put(&dev->ref, em28xx_free_device);
 
 	return 0;
 }
@@ -2711,7 +2716,6 @@ static int em28xx_v4l2_init(struct em28xx *dev)
 		mutex_unlock(&dev->lock);
 		return -ENOMEM;
 	}
-	kref_init(&v4l2->ref);
 	v4l2->dev = dev;
 	dev->v4l2 = v4l2;
 
@@ -2722,9 +2726,21 @@ static int em28xx_v4l2_init(struct em28xx *dev)
 	if (ret < 0) {
 		dev_err(&dev->intf->dev,
 			"Call to v4l2_device_register() failed!\n");
-		goto err;
+		dev->v4l2 = NULL;
+		kfree(v4l2);
+		mutex_unlock(&dev->lock);
+		return ret;
 	}
 
+	/*
+	 * From this point on, em28xx_free_v4l2() will be used to release
+	 * v4l2-related resources when the v4l2_device refcount reaches
+	 * zero. Take a reference to the em28xx device so that it cannot
+	 * be freed before the v4l2 instance is released.
+	 */
+	v4l2->v4l2_dev.release = em28xx_free_v4l2;
+	kref_get(&dev->ref);
+
 	hdl = &v4l2->ctrl_handler;
 	v4l2_ctrl_handler_init(hdl, 9);
 	v4l2->v4l2_dev.ctrl_handler = hdl;
@@ -3048,8 +3064,6 @@ static int em28xx_v4l2_init(struct em28xx *dev)
 	dev_info(&dev->intf->dev,
 		 "V4L2 extension successfully initialized\n");
 
-	kref_get(&dev->ref);
-
 	mutex_unlock(&dev->lock);
 	return 0;
 
@@ -3073,12 +3087,14 @@ static int em28xx_v4l2_init(struct em28xx *dev)
 		video_unregister_device(&v4l2->vdev);
 	}
 
-	v4l2_ctrl_handler_free(&v4l2->ctrl_handler);
-	v4l2_device_unregister(&v4l2->v4l2_dev);
-err:
-	dev->v4l2 = NULL;
-	kref_put(&v4l2->ref, em28xx_free_v4l2);
 	mutex_unlock(&dev->lock);
+
+	/*
+	 * Drop the initial reference. em28xx_free_v4l2() will be called
+	 * once the last video_device node release has decremented the
+	 * v4l2_device refcount to zero.
+	 */
+	v4l2_device_put(&v4l2->v4l2_dev);
 	return ret;
 }
 
diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
index 21c912403efc..90b83e4598ac 100644
--- a/drivers/media/usb/em28xx/em28xx.h
+++ b/drivers/media/usb/em28xx/em28xx.h
@@ -557,7 +557,6 @@ struct em28xx_eeprom {
 #define EM28XX_RESOURCE_VBI   0x02
 
 struct em28xx_v4l2 {
-	struct kref ref;
 	struct em28xx *dev;
 
 	struct v4l2_device v4l2_dev;
-- 
2.53.0

[PATCHv3 3/6] media: em28xx: drop 'users' field

[Hans Verkuil]

Drop the em28xx_v4l2 'users' field, use v4l2_fh_is_singular_file()
instead.

Assisted-by: Claude:claude-opus-4-7
Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
---
 drivers/media/usb/em28xx/em28xx-video.c | 18 +++++++-----------
 drivers/media/usb/em28xx/em28xx.h       |  1 -
 2 files changed, 7 insertions(+), 12 deletions(-)

diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
index 42f5e7547cc4..6c726764a5f3 100644
--- a/drivers/media/usb/em28xx/em28xx-video.c
+++ b/drivers/media/usb/em28xx/em28xx-video.c
@@ -2332,9 +2332,8 @@ static int em28xx_v4l2_open(struct file *filp)
 		return -ENODEV;
 	}
 
-	em28xx_videodbg("open dev=%s type=%s users=%d\n",
-			video_device_node_name(vdev), v4l2_type_names[fh_type],
-			v4l2->users);
+	em28xx_videodbg("open dev=%s type=%s\n",
+			video_device_node_name(vdev), v4l2_type_names[fh_type]);
 
 	ret = v4l2_fh_open(filp);
 	if (ret) {
@@ -2345,7 +2344,7 @@ static int em28xx_v4l2_open(struct file *filp)
 		return ret;
 	}
 
-	if (v4l2->users == 0) {
+	if (v4l2_fh_is_singular_file(filp)) {
 		em28xx_set_mode(dev, EM28XX_ANALOG_MODE);
 
 		if (vdev->vfl_type != VFL_TYPE_RADIO)
@@ -2363,8 +2362,6 @@ static int em28xx_v4l2_open(struct file *filp)
 		v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, s_radio);
 	}
 
-	v4l2->users++;
-
 	mutex_unlock(&dev->lock);
 
 	return 0;
@@ -2467,13 +2464,13 @@ static int em28xx_v4l2_close(struct file *filp)
 	struct em28xx_v4l2    *v4l2 = dev->v4l2;
 	struct usb_device *udev = interface_to_usbdev(dev->intf);
 	int              err;
+	bool last_user;
 
-	em28xx_videodbg("users=%d\n", v4l2->users);
-
-	vb2_fop_release(filp);
 	mutex_lock(&dev->lock);
+	last_user = v4l2_fh_is_singular_file(filp);
+	_vb2_fop_release(filp, NULL);
 
-	if (v4l2->users == 1) {
+	if (last_user) {
 		/* No sense to try to write to the device */
 		if (dev->disconnected)
 			goto exit;
@@ -2496,7 +2493,6 @@ static int em28xx_v4l2_close(struct file *filp)
 	}
 
 exit:
-	v4l2->users--;
 	mutex_unlock(&dev->lock);
 
 	return 0;
diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
index 90b83e4598ac..f68159fab345 100644
--- a/drivers/media/usb/em28xx/em28xx.h
+++ b/drivers/media/usb/em28xx/em28xx.h
@@ -580,7 +580,6 @@ struct em28xx_v4l2 {
 	int sensor_yres;
 	int sensor_xtal;
 
-	int users;		/* user count for exclusive use */
 	int streaming_users;    /* number of actively streaming users */
 
 	u32 frequency;		/* selected tuner frequency */
-- 
2.53.0

[PATCHv3 4/6] media: em28xx: use vb2_video_unregister_device

[Hans Verkuil]

Use vb2_video_unregister_device instead of video_unregister_device
to ensure any streaming is correctly stopped at unregister time.

Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
---
 drivers/media/usb/em28xx/em28xx-video.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
index 6c726764a5f3..e4554d015944 100644
--- a/drivers/media/usb/em28xx/em28xx-video.c
+++ b/drivers/media/usb/em28xx/em28xx-video.c
@@ -2402,17 +2402,17 @@ static int em28xx_v4l2_fini(struct em28xx *dev)
 	if (video_is_registered(&v4l2->radio_dev)) {
 		dev_info(&dev->intf->dev, "V4L2 device %s deregistered\n",
 			 video_device_node_name(&v4l2->radio_dev));
-		video_unregister_device(&v4l2->radio_dev);
+		vb2_video_unregister_device(&v4l2->radio_dev);
 	}
 	if (video_is_registered(&v4l2->vbi_dev)) {
 		dev_info(&dev->intf->dev, "V4L2 device %s deregistered\n",
 			 video_device_node_name(&v4l2->vbi_dev));
-		video_unregister_device(&v4l2->vbi_dev);
+		vb2_video_unregister_device(&v4l2->vbi_dev);
 	}
 	if (video_is_registered(&v4l2->vdev)) {
 		dev_info(&dev->intf->dev, "V4L2 device %s deregistered\n",
 			 video_device_node_name(&v4l2->vdev));
-		video_unregister_device(&v4l2->vdev);
+		vb2_video_unregister_device(&v4l2->vdev);
 	}
 
 	mutex_unlock(&dev->lock);
@@ -3068,19 +3068,19 @@ static int em28xx_v4l2_init(struct em28xx *dev)
 		dev_info(&dev->intf->dev,
 			 "V4L2 device %s deregistered\n",
 			 video_device_node_name(&v4l2->radio_dev));
-		video_unregister_device(&v4l2->radio_dev);
+		vb2_video_unregister_device(&v4l2->radio_dev);
 	}
 	if (video_is_registered(&v4l2->vbi_dev)) {
 		dev_info(&dev->intf->dev,
 			 "V4L2 device %s deregistered\n",
 			 video_device_node_name(&v4l2->vbi_dev));
-		video_unregister_device(&v4l2->vbi_dev);
+		vb2_video_unregister_device(&v4l2->vbi_dev);
 	}
 	if (video_is_registered(&v4l2->vdev)) {
 		dev_info(&dev->intf->dev,
 			 "V4L2 device %s deregistered\n",
 			 video_device_node_name(&v4l2->vdev));
-		video_unregister_device(&v4l2->vdev);
+		vb2_video_unregister_device(&v4l2->vdev);
 	}
 
 	mutex_unlock(&dev->lock);
-- 
2.53.0

[PATCHv3 5/6] media: em28xx: dev_info->pr_info since dev has been freed

[Hans Verkuil]

In em28xx_free_device() dev_info passed &dev->intf->dev,
but that device can be freed already.

Just use pr_info instead.

Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
---
 drivers/media/usb/em28xx/em28xx-cards.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
index fbfb74eab475..4d1e48c86ce8 100644
--- a/drivers/media/usb/em28xx/em28xx-cards.c
+++ b/drivers/media/usb/em28xx/em28xx-cards.c
@@ -3758,7 +3758,7 @@ void em28xx_free_device(struct kref *ref)
 {
 	struct em28xx *dev = kref_to_dev(ref);
 
-	dev_info(&dev->intf->dev, "Freeing device\n");
+	pr_info("%s: Freeing device\n", dev->name);
 
 	if (!dev->disconnected)
 		em28xx_release_resources(dev);
-- 
2.53.0

[PATCHv3 6/6] media: em28xx: requeue buffers if start_streaming fails

[Hans Verkuil]

If start_streaming fails, then all queued buffers must be
returned to vb2 in state QUEUED.

Otherwise it will trigger a WARN_ON.

Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
---
 drivers/media/usb/em28xx/em28xx-video.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
index e4554d015944..c418add65bb5 100644
--- a/drivers/media/usb/em28xx/em28xx-video.c
+++ b/drivers/media/usb/em28xx/em28xx-video.c
@@ -1213,6 +1213,9 @@ int em28xx_start_analog_streaming(struct vb2_queue *vq, unsigned int count)
 {
 	struct em28xx *dev = vb2_get_drv_priv(vq);
 	struct em28xx_v4l2 *v4l2 = dev->v4l2;
+	struct em28xx_dmaqueue *dmaq = vq->type == V4L2_BUF_TYPE_VBI_CAPTURE ?
+		&dev->vbiq : &dev->vidq;
+	unsigned long flags = 0;
 	struct v4l2_frequency f;
 	struct v4l2_fh *owner;
 	int rc = 0;
@@ -1227,7 +1230,7 @@ int em28xx_start_analog_streaming(struct vb2_queue *vq, unsigned int count)
 	 */
 	rc = res_get(dev, vq->type);
 	if (rc)
-		return rc;
+		goto exit;
 
 	if (v4l2->streaming_users == 0) {
 		/* First active streaming user, so allocate all the URBs */
@@ -1250,7 +1253,7 @@ int em28xx_start_analog_streaming(struct vb2_queue *vq, unsigned int count)
 					  em28xx_urb_data_copy);
 		if (rc < 0) {
 			res_free(dev, vq->type);
-			return rc;
+			goto exit;
 		}
 
 		/*
@@ -1275,7 +1278,18 @@ int em28xx_start_analog_streaming(struct vb2_queue *vq, unsigned int count)
 	}
 
 	v4l2->streaming_users++;
+	return 0;
 
+exit:
+	spin_lock_irqsave(&dev->slock, flags);
+	while (!list_empty(&dmaq->active)) {
+		struct em28xx_buffer *buf;
+
+		buf = list_entry(dmaq->active.next, struct em28xx_buffer, list);
+		list_del(&buf->list);
+		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_QUEUED);
+	}
+	spin_unlock_irqrestore(&dev->slock, flags);
 	return rc;
 }
 
-- 
2.53.0

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

[Sakari Ailus]

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,
> 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?

> 
> So if sd->asc_list is empty, then just call
> v4l2_device_unregister_subdev().
> 
> Fixes: 28a1295795d8 ("media: v4l: async: Allow multiple connections between entities")
> 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)) {
> +			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);
> +			}
>  		}
>  	}
>  

-- 
Kind regards,

Sakari Ailus

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

[Hans Verkuil]

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

> 
>>
>> 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)) {
>> +			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);
>> +			}
>>  		}
>>  	}
>>  
> 

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

[Sakari Ailus]

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

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

[Hans Verkuil]

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

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

[Hans Verkuil]

When my em28xx USB device that uses the i2c tvp5150 driver is
disconnected, it crashes.

The cause is that the tvp5150 i2c module uses v4l2_async, but
the em28xx driver does not since it predates v4l2_async.

In that corner case sd->asc_list is empty, so
v4l2_async_unregister_subdev() never calls v4l2_device_unregister_subdev().

Modify the code so that, if sd->asc_list is empty,
v4l2_device_unregister_subdev() is still called.

Fixes: 28a1295795d8 ("media: v4l: async: Allow multiple connections between entities")
Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
---
Changes since v3:
- updated commit message
- added comment to the code

Note: I did not post the other patches in this series, those are unchanged.
---
 drivers/media/v4l2-core/v4l2-async.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index 888a2e213b08..f36d60e6ff41 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -898,9 +898,18 @@ 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)) {
+			/*
+			 * 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);
 		}
 	}

-- 
2.53.0

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

[Sakari Ailus]

Hi Hans,

Thanks for the update.

Did you notice my comment on the subject?

On Tue, Jun 30, 2026 at 12:02:53PM +0200, Hans Verkuil wrote:
> When my em28xx USB device that uses the i2c tvp5150 driver is
> disconnected, it crashes.
> 
> The cause is that the tvp5150 i2c module uses v4l2_async, but
> the em28xx driver does not since it predates v4l2_async.
> 
> In that corner case sd->asc_list is empty, so
> v4l2_async_unregister_subdev() never calls v4l2_device_unregister_subdev().
> 
> Modify the code so that, if sd->asc_list is empty,
> v4l2_device_unregister_subdev() is still called.
> 
> Fixes: 28a1295795d8 ("media: v4l: async: Allow multiple connections between entities")
> Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

-- 
Kind regards,

Sakari Ailus

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

[Hans Verkuil]

On 6/30/26 12:13, Sakari Ailus wrote:
> Hi Hans,
> 
> Thanks for the update.
> 
> Did you notice my comment on the subject?

No, I missed that :-)

Fixed in my branch.

> 
> On Tue, Jun 30, 2026 at 12:02:53PM +0200, Hans Verkuil wrote:
>> When my em28xx USB device that uses the i2c tvp5150 driver is
>> disconnected, it crashes.
>>
>> The cause is that the tvp5150 i2c module uses v4l2_async, but
>> the em28xx driver does not since it predates v4l2_async.
>>
>> In that corner case sd->asc_list is empty, so
>> v4l2_async_unregister_subdev() never calls v4l2_device_unregister_subdev().
>>
>> Modify the code so that, if sd->asc_list is empty,
>> v4l2_device_unregister_subdev() is still called.
>>
>> Fixes: 28a1295795d8 ("media: v4l: async: Allow multiple connections between entities")
>> Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
> 
> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> 

Much appreciated!

Hopefully once this patch series lands it will finally stop people from posting
bad em28xx patches 'fixing' the syzkaller issues due to this.

That reminds me, I need to add a:

Tested-by: Hans Verkuil <hverkuil+cisco@kernel.org>

to this series.

Regards,

	Hans

Re: [PATCHv3 5/6] media: em28xx: dev_info->pr_info since dev has been freed

[Sakari Ailus]

Hi Hans,

On Mon, Jun 29, 2026 at 08:55:53AM +0200, Hans Verkuil wrote:
> In em28xx_free_device() dev_info passed &dev->intf->dev,
> but that device can be freed already.
> 
> Just use pr_info instead.
> 
> Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
> ---
>  drivers/media/usb/em28xx/em28xx-cards.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
> index fbfb74eab475..4d1e48c86ce8 100644
> --- a/drivers/media/usb/em28xx/em28xx-cards.c
> +++ b/drivers/media/usb/em28xx/em28xx-cards.c
> @@ -3758,7 +3758,7 @@ void em28xx_free_device(struct kref *ref)
>  {
>  	struct em28xx *dev = kref_to_dev(ref);
>  
> -	dev_info(&dev->intf->dev, "Freeing device\n");
> +	pr_info("%s: Freeing device\n", dev->name);

Is there a need to print anything at this point? I'd use pr_debug() here,
if not nothing at all.

>  
>  	if (!dev->disconnected)
>  		em28xx_release_resources(dev);

-- 
Kind regards,

Sakari Ailus