From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
To: Igor Matheus Andrade Torrente <igormtorrente@gmail.com>,
mchehab@kernel.org, skhan@linuxfoundation.org
Cc: linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
kernel test robot <lkp@intel.com>,
syzbot+b2391895514ed9ef4a8e@syzkaller.appspotmail.com
Subject: Re: [PATCH v4] media: em28xx: Fix race condition between open and init function
Date: Wed, 5 May 2021 13:21:39 +0200 [thread overview]
Message-ID: <f7721e24-e153-7fa7-6c2d-505f91bac112@xs4all.nl> (raw)
In-Reply-To: <20210503173716.21652-2-igormtorrente@gmail.com>
Hi Igor,
On 03/05/2021 19:37, Igor Matheus Andrade Torrente wrote:
> Fixes a race condition - for lack of a more precise term - between
> em28xx_v4l2_open and em28xx_v4l2_init, by detaching the v4l2_dev,
> media_pad and vdev structs from the em28xx_v4l2, and managing the
> lifetime of those objects more dynamicaly.
>
> The race happens when a thread[1] - containing the em28xx_v4l2_init()
> code - calls the v4l2_mc_create_media_graph(), and it return a error,
> if a thread[2] - running v4l2_open() - pass the verification point
> and reaches the em28xx_v4l2_open() before the thread[1] finishes
> the deregistration of v4l2 subsystem, the thread[1] will free all
> resources before the em28xx_v4l2_open() can process their things,
> because the em28xx_v4l2_init() has the dev->lock. And all this lead
> the thread[2] to cause a user-after-free.
This isn't the right approach. This driver is quite old and tried to do
life-time management itself (and poorly at that), while today there are
better mechanisms, something that your patch tries to use to some extent.
The cleanup for em28xx-video.c has to take place in the release callback
of struct v4l2_device. All allocated memory can be cleaned up there. The
release callback of the video_device structs can just remains as it is today,
i.e. video_device_release_empty.
Also, the video_unregister_device calls should be replaced by
vb2_video_unregister_device, see include/media/videobuf2-v4l2.h why.
The v4l2->ref can be removed, since struct v4l2_device takes over that role.
And when the release callback of v4l2_device is called, then you can call
kref_get(&dev->ref). That reference count really just needs to be incremented
once in em28xx_v4l2_init and decremented once in the v4l2_device release callback,
and not for every open and close.
I hope I haven't forgotten anything.
Regards,
Hans
>
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-and-tested-by: syzbot+b2391895514ed9ef4a8e@syzkaller.appspotmail.com
> Signed-off-by: Igor Matheus Andrade Torrente <igormtorrente@gmail.com>
> ---
>
> V2: Add v4l2_i2c_new_subdev null check
> Deal with v4l2 subdevs dependencies
>
> V3: Fix link error when compiled as a module
>
> V4: Remove duplicated v4l2_device_disconnect
> in the em28xx_v4l2_fini
>
> ---
> drivers/media/usb/em28xx/em28xx-camera.c | 4 +-
> drivers/media/usb/em28xx/em28xx-video.c | 299 +++++++++++++++--------
> drivers/media/usb/em28xx/em28xx.h | 6 +-
> 3 files changed, 208 insertions(+), 101 deletions(-)
>
> diff --git a/drivers/media/usb/em28xx/em28xx-camera.c b/drivers/media/usb/em28xx/em28xx-camera.c
> index d1e66b503f4d..436c5a8cbbb6 100644
> --- a/drivers/media/usb/em28xx/em28xx-camera.c
> +++ b/drivers/media/usb/em28xx/em28xx-camera.c
> @@ -340,7 +340,7 @@ int em28xx_init_camera(struct em28xx *dev)
> v4l2->sensor_xtal = 4300000;
> pdata.xtal = v4l2->sensor_xtal;
> if (NULL ==
> - v4l2_i2c_new_subdev_board(&v4l2->v4l2_dev, adap,
> + v4l2_i2c_new_subdev_board(v4l2->v4l2_dev, adap,
> &mt9v011_info, NULL))
> return -ENODEV;
> v4l2->vinmode = EM28XX_VINMODE_RGB8_GRBG;
> @@ -394,7 +394,7 @@ int em28xx_init_camera(struct em28xx *dev)
> v4l2->sensor_yres = 480;
>
> subdev =
> - v4l2_i2c_new_subdev_board(&v4l2->v4l2_dev, adap,
> + v4l2_i2c_new_subdev_board(v4l2->v4l2_dev, adap,
> &ov2640_info, NULL);
> if (!subdev)
> return -ENODEV;
> diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
> index 6b84c3413e83..6bc6baf88644 100644
> --- a/drivers/media/usb/em28xx/em28xx-video.c
> +++ b/drivers/media/usb/em28xx/em28xx-video.c
> @@ -184,7 +184,7 @@ static int em28xx_vbi_supported(struct em28xx *dev)
> */
> static void em28xx_wake_i2c(struct em28xx *dev)
> {
> - struct v4l2_device *v4l2_dev = &dev->v4l2->v4l2_dev;
> + struct v4l2_device *v4l2_dev = dev->v4l2->v4l2_dev;
>
> v4l2_device_call_all(v4l2_dev, 0, core, reset, 0);
> v4l2_device_call_all(v4l2_dev, 0, video, s_routing,
> @@ -974,9 +974,17 @@ static void em28xx_v4l2_create_entities(struct em28xx *dev)
> struct em28xx_v4l2 *v4l2 = dev->v4l2;
> int ret, i;
>
> + v4l2->video_pad = kzalloc(sizeof(*v4l2->video_pad), GFP_KERNEL);
> + if (!v4l2->video_pad) {
> + dev_err(&dev->intf->dev,
> + "failed to allocate video pad memory!\n");
> + v4l2->vdev->entity.num_pads = 0;
> + return;
> + }
> +
> /* Initialize Video, VBI and Radio pads */
> - v4l2->video_pad.flags = MEDIA_PAD_FL_SINK;
> - ret = media_entity_pads_init(&v4l2->vdev.entity, 1, &v4l2->video_pad);
> + v4l2->video_pad->flags = MEDIA_PAD_FL_SINK;
> + ret = media_entity_pads_init(&v4l2->vdev->entity, 1, v4l2->video_pad);
> if (ret < 0)
> dev_err(&dev->intf->dev,
> "failed to initialize video media entity!\n");
> @@ -1132,11 +1140,11 @@ int em28xx_start_analog_streaming(struct vb2_queue *vq, unsigned int count)
> f.type = V4L2_TUNER_RADIO;
> else
> f.type = V4L2_TUNER_ANALOG_TV;
> - v4l2_device_call_all(&v4l2->v4l2_dev,
> + v4l2_device_call_all(v4l2->v4l2_dev,
> 0, tuner, s_frequency, &f);
>
> /* Enable video stream at TV decoder */
> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_stream, 1);
> + v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_stream, 1);
> }
>
> v4l2->streaming_users++;
> @@ -1157,7 +1165,7 @@ static void em28xx_stop_streaming(struct vb2_queue *vq)
>
> if (v4l2->streaming_users-- == 1) {
> /* Disable video stream at TV decoder */
> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_stream, 0);
> + v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_stream, 0);
>
> /* Last active user, so shutdown all the URBS */
> em28xx_uninit_usb_xfer(dev, EM28XX_ANALOG_MODE);
> @@ -1192,7 +1200,7 @@ void em28xx_stop_vbi_streaming(struct vb2_queue *vq)
>
> if (v4l2->streaming_users-- == 1) {
> /* Disable video stream at TV decoder */
> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_stream, 0);
> + v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_stream, 0);
>
> /* Last active user, so shutdown all the URBS */
> em28xx_uninit_usb_xfer(dev, EM28XX_ANALOG_MODE);
> @@ -1286,7 +1294,7 @@ static int em28xx_vb2_setup(struct em28xx *dev)
>
> static void video_mux(struct em28xx *dev, int index)
> {
> - struct v4l2_device *v4l2_dev = &dev->v4l2->v4l2_dev;
> + struct v4l2_device *v4l2_dev = dev->v4l2->v4l2_dev;
>
> dev->ctl_input = index;
> dev->ctl_ainput = INPUT(index)->amux;
> @@ -1565,7 +1573,7 @@ static int vidioc_querystd(struct file *file, void *priv, v4l2_std_id *norm)
> {
> struct em28xx *dev = video_drvdata(file);
>
> - v4l2_device_call_all(&dev->v4l2->v4l2_dev, 0, video, querystd, norm);
> + v4l2_device_call_all(dev->v4l2->v4l2_dev, 0, video, querystd, norm);
>
> return 0;
> }
> @@ -1596,7 +1604,7 @@ static int vidioc_s_std(struct file *file, void *priv, v4l2_std_id norm)
> &v4l2->hscale, &v4l2->vscale);
>
> em28xx_resolution_set(dev);
> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_std, v4l2->norm);
> + v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_std, v4l2->norm);
>
> return 0;
> }
> @@ -1616,7 +1624,7 @@ static int vidioc_g_parm(struct file *file, void *priv,
> p->parm.capture.readbuffers = EM28XX_MIN_BUF;
> p->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
> if (dev->is_webcam) {
> - rc = v4l2_device_call_until_err(&v4l2->v4l2_dev, 0,
> + rc = v4l2_device_call_until_err(v4l2->v4l2_dev, 0,
> video, g_frame_interval, &ival);
> if (!rc)
> p->parm.capture.timeperframe = ival.interval;
> @@ -1648,7 +1656,7 @@ static int vidioc_s_parm(struct file *file, void *priv,
> memset(&p->parm, 0, sizeof(p->parm));
> p->parm.capture.readbuffers = EM28XX_MIN_BUF;
> p->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
> - rc = v4l2_device_call_until_err(&dev->v4l2->v4l2_dev, 0,
> + rc = v4l2_device_call_until_err(dev->v4l2->v4l2_dev, 0,
> video, s_frame_interval, &ival);
> if (!rc)
> p->parm.capture.timeperframe = ival.interval;
> @@ -1675,7 +1683,7 @@ static int vidioc_enum_input(struct file *file, void *priv,
> if (INPUT(n)->type == EM28XX_VMUX_TELEVISION)
> i->type = V4L2_INPUT_TYPE_TUNER;
>
> - i->std = dev->v4l2->vdev.tvnorms;
> + i->std = dev->v4l2->vdev->tvnorms;
> /* webcams do not have the STD API */
> if (dev->is_webcam)
> i->capabilities = 0;
> @@ -1839,7 +1847,7 @@ static int vidioc_g_tuner(struct file *file, void *priv,
>
> strscpy(t->name, "Tuner", sizeof(t->name));
>
> - v4l2_device_call_all(&dev->v4l2->v4l2_dev, 0, tuner, g_tuner, t);
> + v4l2_device_call_all(dev->v4l2->v4l2_dev, 0, tuner, g_tuner, t);
> return 0;
> }
>
> @@ -1851,7 +1859,7 @@ static int vidioc_s_tuner(struct file *file, void *priv,
> if (t->index != 0)
> return -EINVAL;
>
> - v4l2_device_call_all(&dev->v4l2->v4l2_dev, 0, tuner, s_tuner, t);
> + v4l2_device_call_all(dev->v4l2->v4l2_dev, 0, tuner, s_tuner, t);
> return 0;
> }
>
> @@ -1878,8 +1886,8 @@ static int vidioc_s_frequency(struct file *file, void *priv,
> if (f->tuner != 0)
> return -EINVAL;
>
> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, s_frequency, f);
> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, g_frequency, &new_freq);
> + v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, s_frequency, f);
> + v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, g_frequency, &new_freq);
> v4l2->frequency = new_freq.frequency;
>
> return 0;
> @@ -1897,7 +1905,7 @@ static int vidioc_g_chip_info(struct file *file, void *priv,
> strscpy(chip->name, "ac97", sizeof(chip->name));
> else
> strscpy(chip->name,
> - dev->v4l2->v4l2_dev.name, sizeof(chip->name));
> + dev->v4l2->v4l2_dev->name, sizeof(chip->name));
> return 0;
> }
>
> @@ -2095,7 +2103,7 @@ static int radio_g_tuner(struct file *file, void *priv,
>
> strscpy(t->name, "Radio", sizeof(t->name));
>
> - v4l2_device_call_all(&dev->v4l2->v4l2_dev, 0, tuner, g_tuner, t);
> + v4l2_device_call_all(dev->v4l2->v4l2_dev, 0, tuner, g_tuner, t);
>
> return 0;
> }
> @@ -2108,7 +2116,7 @@ static int radio_s_tuner(struct file *file, void *priv,
> if (t->index != 0)
> return -EINVAL;
>
> - v4l2_device_call_all(&dev->v4l2->v4l2_dev, 0, tuner, s_tuner, t);
> + v4l2_device_call_all(dev->v4l2->v4l2_dev, 0, tuner, s_tuner, t);
>
> return 0;
> }
> @@ -2160,6 +2168,11 @@ static int em28xx_v4l2_open(struct file *filp)
> if (mutex_lock_interruptible(&dev->lock))
> return -ERESTARTSYS;
>
> + if (!dev->v4l2) {
> + mutex_unlock(&dev->lock);
> + return -ENODEV;
> + }
> +
> ret = v4l2_fh_open(filp);
> if (ret) {
> dev_err(&dev->intf->dev,
> @@ -2184,9 +2197,10 @@ static int em28xx_v4l2_open(struct file *filp)
>
> if (vdev->vfl_type == VFL_TYPE_RADIO) {
> em28xx_videodbg("video_open: setting radio device\n");
> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, s_radio);
> + v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, s_radio);
> }
>
> + v4l2_device_get(v4l2->v4l2_dev);
> kref_get(&dev->ref);
> kref_get(&v4l2->ref);
> v4l2->users++;
> @@ -2222,7 +2236,7 @@ static int em28xx_v4l2_fini(struct em28xx *dev)
>
> mutex_lock(&dev->lock);
>
> - v4l2_device_disconnect(&v4l2->v4l2_dev);
> + v4l2_device_disconnect(v4l2->v4l2_dev);
>
> em28xx_uninit_usb_xfer(dev, EM28XX_ANALOG_MODE);
>
> @@ -2238,14 +2252,14 @@ static int em28xx_v4l2_fini(struct em28xx *dev)
> video_device_node_name(&v4l2->vbi_dev));
> video_unregister_device(&v4l2->vbi_dev);
> }
> - if (video_is_registered(&v4l2->vdev)) {
> + 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);
> + video_device_node_name(v4l2->vdev));
> + video_unregister_device(v4l2->vdev);
> }
>
> v4l2_ctrl_handler_free(&v4l2->ctrl_handler);
> - v4l2_device_unregister(&v4l2->v4l2_dev);
> + v4l2_device_put(v4l2->v4l2_dev);
>
> kref_put(&v4l2->ref, em28xx_free_v4l2);
>
> @@ -2305,7 +2319,7 @@ static int em28xx_v4l2_close(struct file *filp)
> goto exit;
>
> /* Save some power by putting tuner to sleep */
> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, standby);
> + v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, standby);
>
> /* do this before setting alternate! */
> em28xx_set_mode(dev, EM28XX_SUSPEND);
> @@ -2322,6 +2336,7 @@ static int em28xx_v4l2_close(struct file *filp)
> }
>
> exit:
> + v4l2_device_put(v4l2->v4l2_dev);
> v4l2->users--;
> kref_put(&v4l2->ref, em28xx_free_v4l2);
> mutex_unlock(&dev->lock);
> @@ -2330,6 +2345,17 @@ static int em28xx_v4l2_close(struct file *filp)
> return 0;
> }
>
> +static void em28xx_vdev_release(struct video_device *vdev)
> +{
> +#ifdef CONFIG_MEDIA_CONTROLLER
> + int i;
> +
> + for (i = 0; i < vdev->entity.num_pads; i++)
> + kfree(&vdev->entity.pads[i]);
> +#endif
> + kfree(vdev);
> +}
> +
> static const struct v4l2_file_operations em28xx_v4l_fops = {
> .owner = THIS_MODULE,
> .open = em28xx_v4l2_open,
> @@ -2387,7 +2413,7 @@ static const struct v4l2_ioctl_ops video_ioctl_ops = {
> static const struct video_device em28xx_video_template = {
> .fops = &em28xx_v4l_fops,
> .ioctl_ops = &video_ioctl_ops,
> - .release = video_device_release_empty,
> + .release = em28xx_vdev_release,
> .tvnorms = V4L2_STD_ALL,
> };
>
> @@ -2445,7 +2471,7 @@ static void em28xx_vdev_init(struct em28xx *dev,
> const char *type_name)
> {
> *vfd = *template;
> - vfd->v4l2_dev = &dev->v4l2->v4l2_dev;
> + vfd->v4l2_dev = dev->v4l2->v4l2_dev;
> vfd->lock = &dev->lock;
> if (dev->is_webcam)
> vfd->tvnorms = 0;
> @@ -2459,7 +2485,7 @@ static void em28xx_vdev_init(struct em28xx *dev,
> static void em28xx_tuner_setup(struct em28xx *dev, unsigned short tuner_addr)
> {
> struct em28xx_v4l2 *v4l2 = dev->v4l2;
> - struct v4l2_device *v4l2_dev = &v4l2->v4l2_dev;
> + struct v4l2_device *v4l2_dev = v4l2->v4l2_dev;
> struct tuner_setup tun_setup;
> struct v4l2_frequency f;
>
> @@ -2517,6 +2543,22 @@ static void em28xx_tuner_setup(struct em28xx *dev, unsigned short tuner_addr)
> v4l2_device_call_all(v4l2_dev, 0, tuner, s_frequency, &f);
> }
>
> +static void em28xx_v4l2_dev_release(struct v4l2_device *v4l2_dev)
> +{
> + struct v4l2_subdev *sd, *next;
> + struct i2c_client *client;
> +
> + list_for_each_entry_safe(sd, next, &v4l2_dev->subdevs, list) {
> + v4l2_device_unregister_subdev(sd);
> + client = sd->dev_priv;
> + if (client && !client->dev.of_node && !client->dev.fwnode &&
> + sd->flags & V4L2_SUBDEV_FL_IS_I2C)
> + i2c_unregister_device(client);
> + }
> +
> + kfree(v4l2_dev);
> +}
> +
> static int em28xx_v4l2_init(struct em28xx *dev)
> {
> u8 val;
> @@ -2524,6 +2566,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
> unsigned int maxw;
> struct v4l2_ctrl_handler *hdl;
> struct em28xx_v4l2 *v4l2;
> + struct v4l2_subdev *sd;
>
> if (dev->is_audio_only) {
> /* Shouldn't initialize IR for this interface */
> @@ -2541,26 +2584,35 @@ static int em28xx_v4l2_init(struct em28xx *dev)
>
> v4l2 = kzalloc(sizeof(*v4l2), GFP_KERNEL);
> if (!v4l2) {
> - mutex_unlock(&dev->lock);
> - return -ENOMEM;
> + ret = -ENOMEM;
> + goto v4l2_error;
> }
> +
> kref_init(&v4l2->ref);
> v4l2->dev = dev;
> dev->v4l2 = v4l2;
>
> + v4l2->v4l2_dev = kzalloc(sizeof(*v4l2->v4l2_dev), GFP_KERNEL);
> + if (!v4l2->v4l2_dev) {
> + ret = -ENOMEM;
> + goto v4l2_dev_error;
> + }
> +
> + v4l2->v4l2_dev->release = em28xx_v4l2_dev_release;
> +
> #ifdef CONFIG_MEDIA_CONTROLLER
> - v4l2->v4l2_dev.mdev = dev->media_dev;
> + v4l2->v4l2_dev->mdev = dev->media_dev;
> #endif
> - ret = v4l2_device_register(&dev->intf->dev, &v4l2->v4l2_dev);
> + ret = v4l2_device_register(&dev->intf->dev, v4l2->v4l2_dev);
> if (ret < 0) {
> dev_err(&dev->intf->dev,
> "Call to v4l2_device_register() failed!\n");
> - goto err;
> + goto v4l2_device_register_error;
> }
>
> hdl = &v4l2->ctrl_handler;
> v4l2_ctrl_handler_init(hdl, 8);
> - v4l2->v4l2_dev.ctrl_handler = hdl;
> + v4l2->v4l2_dev->ctrl_handler = hdl;
>
> if (dev->is_webcam)
> v4l2->progressive = true;
> @@ -2574,25 +2626,49 @@ static int em28xx_v4l2_init(struct em28xx *dev)
>
> /* request some modules */
>
> - if (dev->has_msp34xx)
> - v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
> - &dev->i2c_adap[dev->def_i2c_bus],
> - "msp3400", 0, msp3400_addrs);
> + if (dev->has_msp34xx) {
> + sd = v4l2_i2c_new_subdev(v4l2->v4l2_dev,
> + &dev->i2c_adap[dev->def_i2c_bus],
> + "msp3400", 0, msp3400_addrs);
> + if (!sd) {
> + dev_err(&dev->intf->dev,
> + "Error while registering msp34xx v4l2 subdevice!\n");
> + goto v4l2_subdev_register_error;
> + }
> + }
>
> - if (dev->board.decoder == EM28XX_SAA711X)
> - v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
> - &dev->i2c_adap[dev->def_i2c_bus],
> - "saa7115_auto", 0, saa711x_addrs);
> + if (dev->board.decoder == EM28XX_SAA711X) {
> + sd = v4l2_i2c_new_subdev(v4l2->v4l2_dev,
> + &dev->i2c_adap[dev->def_i2c_bus],
> + "saa7115_auto", 0, saa711x_addrs);
> + if (!sd) {
> + dev_err(&dev->intf->dev,
> + "Error while registering EM28XX_SAA711X v4l2 subdevice!\n");
> + goto v4l2_subdev_register_error;
> + }
> + }
>
> - if (dev->board.decoder == EM28XX_TVP5150)
> - v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
> - &dev->i2c_adap[dev->def_i2c_bus],
> - "tvp5150", 0, tvp5150_addrs);
> + if (dev->board.decoder == EM28XX_TVP5150) {
> + sd = v4l2_i2c_new_subdev(v4l2->v4l2_dev,
> + &dev->i2c_adap[dev->def_i2c_bus],
> + "tvp5150", 0, tvp5150_addrs);
> + if (!sd) {
> + dev_err(&dev->intf->dev,
> + "Error while registering EM28XX_TVP5150 v4l2 subdevice!\n");
> + goto v4l2_subdev_register_error;
> + }
> + }
>
> - if (dev->board.adecoder == EM28XX_TVAUDIO)
> - v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
> - &dev->i2c_adap[dev->def_i2c_bus],
> - "tvaudio", dev->board.tvaudio_addr, NULL);
> + if (dev->board.adecoder == EM28XX_TVAUDIO) {
> + sd = v4l2_i2c_new_subdev(v4l2->v4l2_dev,
> + &dev->i2c_adap[dev->def_i2c_bus],
> + "tvaudio", dev->board.tvaudio_addr, NULL);
> + if (!sd) {
> + dev_err(&dev->intf->dev,
> + "Error while registering EM28XX_TVAUDIO v4l2 subdevice!\n");
> + goto v4l2_subdev_register_error;
> + }
> + }
>
> /* Initialize tuner and camera */
>
> @@ -2600,33 +2676,55 @@ static int em28xx_v4l2_init(struct em28xx *dev)
> unsigned short tuner_addr = dev->board.tuner_addr;
> int has_demod = (dev->board.tda9887_conf & TDA9887_PRESENT);
>
> - if (dev->board.radio.type)
> - v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
> - &dev->i2c_adap[dev->def_i2c_bus],
> - "tuner", dev->board.radio_addr,
> - NULL);
> -
> - if (has_demod)
> - v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
> - &dev->i2c_adap[dev->def_i2c_bus],
> - "tuner", 0,
> - v4l2_i2c_tuner_addrs(ADDRS_DEMOD));
> + if (dev->board.radio.type) {
> + sd = v4l2_i2c_new_subdev(v4l2->v4l2_dev,
> + &dev->i2c_adap[dev->def_i2c_bus],
> + "tuner", dev->board.radio_addr,
> + NULL);
> + if (!sd) {
> + dev_err(&dev->intf->dev,
> + "Error while registering <name1> v4l2 subdevice!\n");
> + goto v4l2_subdev_register_error;
> + }
> + }
> +
> + if (has_demod) {
> + sd = v4l2_i2c_new_subdev(v4l2->v4l2_dev,
> + &dev->i2c_adap[dev->def_i2c_bus],
> + "tuner", 0,
> + v4l2_i2c_tuner_addrs(ADDRS_DEMOD));
> + if (!sd) {
> + dev_err(&dev->intf->dev,
> + "Error while registering <name2> v4l2 subdevice!\n");
> + goto v4l2_subdev_register_error;
> + }
> + }
> +
> if (tuner_addr == 0) {
> enum v4l2_i2c_tuner_type type =
> has_demod ? ADDRS_TV_WITH_DEMOD : ADDRS_TV;
> - struct v4l2_subdev *sd;
>
> - sd = v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
> + sd = v4l2_i2c_new_subdev(v4l2->v4l2_dev,
> &dev->i2c_adap[dev->def_i2c_bus],
> "tuner", 0,
> v4l2_i2c_tuner_addrs(type));
> -
> - if (sd)
> + if (sd) {
> tuner_addr = v4l2_i2c_subdev_addr(sd);
> + } else {
> + dev_err(&dev->intf->dev,
> + "Error while registering <name3> v4l2 subdevice!\n");
> + goto v4l2_subdev_register_error;
> + }
> +
> } else {
> - v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
> - &dev->i2c_adap[dev->def_i2c_bus],
> - "tuner", tuner_addr, NULL);
> + sd = v4l2_i2c_new_subdev(v4l2->v4l2_dev,
> + &dev->i2c_adap[dev->def_i2c_bus],
> + "tuner", tuner_addr, NULL);
> + if (!sd) {
> + dev_err(&dev->intf->dev,
> + "Error while registering <name4> v4l2 subdevice!\n");
> + goto v4l2_subdev_register_error;
> + }
> }
>
> em28xx_tuner_setup(dev, tuner_addr);
> @@ -2686,7 +2784,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
>
> /* set default norm */
> v4l2->norm = V4L2_STD_PAL;
> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_std, v4l2->norm);
> + v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_std, v4l2->norm);
> v4l2->interlaced_fieldmode = EM28XX_INTERLACED_DEFAULT;
>
> /* Analog specific initialization */
> @@ -2756,40 +2854,45 @@ static int em28xx_v4l2_init(struct em28xx *dev)
> goto unregister_dev;
>
> /* allocate and fill video video_device struct */
> - em28xx_vdev_init(dev, &v4l2->vdev, &em28xx_video_template, "video");
> + v4l2->vdev = kzalloc(sizeof(*v4l2->vdev), GFP_KERNEL);
> + if (!v4l2->vdev) {
> + ret = -ENOMEM;
> + goto unregister_dev;
> + }
> +
> + em28xx_vdev_init(dev, v4l2->vdev, &em28xx_video_template, "video");
> mutex_init(&v4l2->vb_queue_lock);
> mutex_init(&v4l2->vb_vbi_queue_lock);
> - v4l2->vdev.queue = &v4l2->vb_vidq;
> - v4l2->vdev.queue->lock = &v4l2->vb_queue_lock;
> - v4l2->vdev.device_caps = V4L2_CAP_READWRITE | V4L2_CAP_VIDEO_CAPTURE |
> + v4l2->vdev->queue = &v4l2->vb_vidq;
> + v4l2->vdev->queue->lock = &v4l2->vb_queue_lock;
> + v4l2->vdev->device_caps = V4L2_CAP_READWRITE | V4L2_CAP_VIDEO_CAPTURE |
> V4L2_CAP_STREAMING;
> if (dev->int_audio_type != EM28XX_INT_AUDIO_NONE)
> - v4l2->vdev.device_caps |= V4L2_CAP_AUDIO;
> + v4l2->vdev->device_caps |= V4L2_CAP_AUDIO;
> if (dev->tuner_type != TUNER_ABSENT)
> - v4l2->vdev.device_caps |= V4L2_CAP_TUNER;
> -
> + v4l2->vdev->device_caps |= V4L2_CAP_TUNER;
>
> /* disable inapplicable ioctls */
> if (dev->is_webcam) {
> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_QUERYSTD);
> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_G_STD);
> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_STD);
> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_QUERYSTD);
> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_STD);
> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_STD);
> } else {
> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_PARM);
> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_PARM);
> }
> if (dev->tuner_type == TUNER_ABSENT) {
> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_G_TUNER);
> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_TUNER);
> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_G_FREQUENCY);
> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_FREQUENCY);
> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_TUNER);
> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_TUNER);
> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_FREQUENCY);
> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_FREQUENCY);
> }
> if (dev->int_audio_type == EM28XX_INT_AUDIO_NONE) {
> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_G_AUDIO);
> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_AUDIO);
> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_AUDIO);
> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_AUDIO);
> }
>
> /* register v4l2 video video_device */
> - ret = video_register_device(&v4l2->vdev, VFL_TYPE_VIDEO,
> + ret = video_register_device(v4l2->vdev, VFL_TYPE_VIDEO,
> video_nr[dev->devno]);
> if (ret) {
> dev_err(&dev->intf->dev,
> @@ -2863,7 +2966,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
>
> dev_info(&dev->intf->dev,
> "V4L2 video device registered as %s\n",
> - video_device_node_name(&v4l2->vdev));
> + video_device_node_name(v4l2->vdev));
>
> if (video_is_registered(&v4l2->vbi_dev))
> dev_info(&dev->intf->dev,
> @@ -2871,7 +2974,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
> video_device_node_name(&v4l2->vbi_dev));
>
> /* Save some power by putting tuner to sleep */
> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, standby);
> + v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, standby);
>
> /* initialize videobuf2 stuff */
> em28xx_vb2_setup(dev);
> @@ -2897,18 +3000,22 @@ static int em28xx_v4l2_init(struct em28xx *dev)
> video_device_node_name(&v4l2->vbi_dev));
> video_unregister_device(&v4l2->vbi_dev);
> }
> - if (video_is_registered(&v4l2->vdev)) {
> + 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);
> + video_device_node_name(v4l2->vdev));
> + video_unregister_device(v4l2->vdev);
> }
>
> v4l2_ctrl_handler_free(&v4l2->ctrl_handler);
> - v4l2_device_unregister(&v4l2->v4l2_dev);
> -err:
> +v4l2_subdev_register_error:
> + v4l2_device_disconnect(v4l2->v4l2_dev);
> +v4l2_device_register_error:
> + v4l2_device_put(v4l2->v4l2_dev);
> +v4l2_dev_error:
> dev->v4l2 = NULL;
> kref_put(&v4l2->ref, em28xx_free_v4l2);
> +v4l2_error:
> mutex_unlock(&dev->lock);
> return ret;
> }
> diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
> index 6648e11f1271..dbcc297b5a0d 100644
> --- a/drivers/media/usb/em28xx/em28xx.h
> +++ b/drivers/media/usb/em28xx/em28xx.h
> @@ -552,10 +552,10 @@ struct em28xx_v4l2 {
> struct kref ref;
> struct em28xx *dev;
>
> - struct v4l2_device v4l2_dev;
> + struct v4l2_device *v4l2_dev;
> struct v4l2_ctrl_handler ctrl_handler;
>
> - struct video_device vdev;
> + struct video_device *vdev;
> struct video_device vbi_dev;
> struct video_device radio_dev;
>
> @@ -601,7 +601,7 @@ struct em28xx_v4l2 {
> unsigned int field_count;
>
> #ifdef CONFIG_MEDIA_CONTROLLER
> - struct media_pad video_pad, vbi_pad;
> + struct media_pad *video_pad, vbi_pad;
> struct media_entity *decoder;
> #endif
> };
>
next prev parent reply other threads:[~2021-05-05 11:21 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-03 17:37 [PATCH] media: em28xx: Fix possible memory leak of em28xx struct Igor Matheus Andrade Torrente
2021-05-03 17:37 ` [PATCH v4] media: em28xx: Fix race condition between open and init function Igor Matheus Andrade Torrente
2021-05-05 11:21 ` Hans Verkuil [this message]
2021-05-05 19:54 ` Igor Torrente
2021-05-06 7:47 ` Hans Verkuil
2021-05-06 13:37 ` Igor Torrente
2021-05-03 20:06 ` [PATCH] media: em28xx: Fix possible memory leak of em28xx struct Shuah Khan
2021-05-04 19:07 ` Igor Torrente
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f7721e24-e153-7fa7-6c2d-505f91bac112@xs4all.nl \
--to=hverkuil-cisco@xs4all.nl \
--cc=igormtorrente@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=lkp@intel.com \
--cc=mchehab@kernel.org \
--cc=skhan@linuxfoundation.org \
--cc=syzbot+b2391895514ed9ef4a8e@syzkaller.appspotmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.