* [PATCH 1/3] media: em28xx: use v4l2_device release callback
2026-05-19 11:54 [PATCH 0/3] media: em28xx: fix lifecycle issues Hans Verkuil
@ 2026-05-19 11:54 ` Hans Verkuil
2026-05-19 11:54 ` [PATCH 2/3] media: em28xx: drop 'users' field Hans Verkuil
2026-05-19 11:54 ` [PATCH 3/3] media: em28xx: use vb2_video_unregister_device Hans Verkuil
2 siblings, 0 replies; 4+ messages in thread
From: Hans Verkuil @ 2026-05-19 11:54 UTC (permalink / raw)
To: linux-media; +Cc: Mauricio Faria de Oliveira, 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
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH 2/3] media: em28xx: drop 'users' field
2026-05-19 11:54 [PATCH 0/3] media: em28xx: fix lifecycle issues Hans Verkuil
2026-05-19 11:54 ` [PATCH 1/3] media: em28xx: use v4l2_device release callback Hans Verkuil
@ 2026-05-19 11:54 ` Hans Verkuil
2026-05-19 11:54 ` [PATCH 3/3] media: em28xx: use vb2_video_unregister_device Hans Verkuil
2 siblings, 0 replies; 4+ messages in thread
From: Hans Verkuil @ 2026-05-19 11:54 UTC (permalink / raw)
To: linux-media; +Cc: Mauricio Faria de Oliveira, 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
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH 3/3] media: em28xx: use vb2_video_unregister_device
2026-05-19 11:54 [PATCH 0/3] media: em28xx: fix lifecycle issues Hans Verkuil
2026-05-19 11:54 ` [PATCH 1/3] media: em28xx: use v4l2_device release callback Hans Verkuil
2026-05-19 11:54 ` [PATCH 2/3] media: em28xx: drop 'users' field Hans Verkuil
@ 2026-05-19 11:54 ` Hans Verkuil
2 siblings, 0 replies; 4+ messages in thread
From: Hans Verkuil @ 2026-05-19 11:54 UTC (permalink / raw)
To: linux-media; +Cc: Mauricio Faria de Oliveira, 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
^ permalink raw reply related [flat|nested] 4+ messages in thread