| Message ID | 20190221142148.3412-6-hverkuil-cisco@xs4all.nl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Various core and virtual driver fixes | expand |
Hi Hans, Thank you for the patch. On Thu, Feb 21, 2019 at 03:21:46PM +0100, Hans Verkuil wrote: > It is not possible to use devm_kzalloc since that memory is > freed immediately when the device instance is unbound. > > Various objects like the video device may still be in use > since someone has the device node open, and when that is closed > it expects the memory to be around. > > So use kzalloc and release it at the appropriate time. You're opening a can of worms, we have tons of drivers that use devm_kzalloc() :-) I however believe this is the right course of action. > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > --- > drivers/media/platform/vim2m.c | 20 +++++++++++++++----- > 1 file changed, 15 insertions(+), 5 deletions(-) > > diff --git a/drivers/media/platform/vim2m.c b/drivers/media/platform/vim2m.c > index a27d3052bb62..bfb3e3eb48d1 100644 > --- a/drivers/media/platform/vim2m.c > +++ b/drivers/media/platform/vim2m.c > @@ -1087,6 +1087,16 @@ static int vim2m_release(struct file *file) > return 0; > } > > +static void vim2m_device_release(struct video_device *vdev) > +{ > + struct vim2m_dev *dev = container_of(vdev, struct vim2m_dev, vfd); > + > + dprintk(dev, "Releasing last dev\n"); Do we really need a debug printk here ? > + v4l2_device_unregister(&dev->v4l2_dev); > + v4l2_m2m_release(dev->m2m_dev); > + kfree(dev); > +} > + > static const struct v4l2_file_operations vim2m_fops = { > .owner = THIS_MODULE, > .open = vim2m_open, > @@ -1102,7 +1112,7 @@ static const struct video_device vim2m_videodev = { > .fops = &vim2m_fops, > .ioctl_ops = &vim2m_ioctl_ops, > .minor = -1, > - .release = video_device_release_empty, > + .release = vim2m_device_release, > .device_caps = V4L2_CAP_VIDEO_M2M | V4L2_CAP_STREAMING, > }; > > @@ -1123,13 +1133,13 @@ static int vim2m_probe(struct platform_device *pdev) > struct video_device *vfd; > int ret; > > - dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL); > + dev = kzalloc(sizeof(*dev), GFP_KERNEL); > if (!dev) > return -ENOMEM; > > ret = v4l2_device_register(&pdev->dev, &dev->v4l2_dev); > if (ret) > - return ret; > + goto unreg_free; > > atomic_set(&dev->num_inst, 0); > mutex_init(&dev->dev_mutex); > @@ -1192,6 +1202,8 @@ static int vim2m_probe(struct platform_device *pdev) > video_unregister_device(&dev->vfd); > unreg_v4l2: > v4l2_device_unregister(&dev->v4l2_dev); > +unreg_free: I'd call the label error_free, and rename the other ones with an error_ prefix, as you don't register anything here. With these two small issues fixes, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + kfree(dev); > > return ret; > } > @@ -1207,9 +1219,7 @@ static int vim2m_remove(struct platform_device *pdev) > v4l2_m2m_unregister_media_controller(dev->m2m_dev); > media_device_cleanup(&dev->mdev); > #endif > - v4l2_m2m_release(dev->m2m_dev); > video_unregister_device(&dev->vfd); > - v4l2_device_unregister(&dev->v4l2_dev); > > return 0; > }
On 2/22/19 12:20 PM, Laurent Pinchart wrote: > Hi Hans, > > Thank you for the patch. > > On Thu, Feb 21, 2019 at 03:21:46PM +0100, Hans Verkuil wrote: >> It is not possible to use devm_kzalloc since that memory is >> freed immediately when the device instance is unbound. >> >> Various objects like the video device may still be in use >> since someone has the device node open, and when that is closed >> it expects the memory to be around. >> >> So use kzalloc and release it at the appropriate time. > > You're opening a can of worms, we have tons of drivers that use > devm_kzalloc() :-) I however believe this is the right course of action. > >> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> >> --- >> drivers/media/platform/vim2m.c | 20 +++++++++++++++----- >> 1 file changed, 15 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/media/platform/vim2m.c b/drivers/media/platform/vim2m.c >> index a27d3052bb62..bfb3e3eb48d1 100644 >> --- a/drivers/media/platform/vim2m.c >> +++ b/drivers/media/platform/vim2m.c >> @@ -1087,6 +1087,16 @@ static int vim2m_release(struct file *file) >> return 0; >> } >> >> +static void vim2m_device_release(struct video_device *vdev) >> +{ >> + struct vim2m_dev *dev = container_of(vdev, struct vim2m_dev, vfd); >> + >> + dprintk(dev, "Releasing last dev\n"); > > Do we really need a debug printk here ? Oops, that's a left-over debug message. I'll remove it. > >> + v4l2_device_unregister(&dev->v4l2_dev); >> + v4l2_m2m_release(dev->m2m_dev); >> + kfree(dev); >> +} >> + >> static const struct v4l2_file_operations vim2m_fops = { >> .owner = THIS_MODULE, >> .open = vim2m_open, >> @@ -1102,7 +1112,7 @@ static const struct video_device vim2m_videodev = { >> .fops = &vim2m_fops, >> .ioctl_ops = &vim2m_ioctl_ops, >> .minor = -1, >> - .release = video_device_release_empty, >> + .release = vim2m_device_release, >> .device_caps = V4L2_CAP_VIDEO_M2M | V4L2_CAP_STREAMING, >> }; >> >> @@ -1123,13 +1133,13 @@ static int vim2m_probe(struct platform_device *pdev) >> struct video_device *vfd; >> int ret; >> >> - dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL); >> + dev = kzalloc(sizeof(*dev), GFP_KERNEL); >> if (!dev) >> return -ENOMEM; >> >> ret = v4l2_device_register(&pdev->dev, &dev->v4l2_dev); >> if (ret) >> - return ret; >> + goto unreg_free; >> >> atomic_set(&dev->num_inst, 0); >> mutex_init(&dev->dev_mutex); >> @@ -1192,6 +1202,8 @@ static int vim2m_probe(struct platform_device *pdev) >> video_unregister_device(&dev->vfd); >> unreg_v4l2: >> v4l2_device_unregister(&dev->v4l2_dev); >> +unreg_free: > > I'd call the label error_free, and rename the other ones with an error_ > prefix, as you don't register anything here. OK > > With these two small issues fixes, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Regards, Hans > >> + kfree(dev); >> >> return ret; >> } >> @@ -1207,9 +1219,7 @@ static int vim2m_remove(struct platform_device *pdev) >> v4l2_m2m_unregister_media_controller(dev->m2m_dev); >> media_device_cleanup(&dev->mdev); >> #endif >> - v4l2_m2m_release(dev->m2m_dev); >> video_unregister_device(&dev->vfd); >> - v4l2_device_unregister(&dev->v4l2_dev); >> >> return 0; >> } >
diff --git a/drivers/media/platform/vim2m.c b/drivers/media/platform/vim2m.c index a27d3052bb62..bfb3e3eb48d1 100644 --- a/drivers/media/platform/vim2m.c +++ b/drivers/media/platform/vim2m.c @@ -1087,6 +1087,16 @@ static int vim2m_release(struct file *file) return 0; } +static void vim2m_device_release(struct video_device *vdev) +{ + struct vim2m_dev *dev = container_of(vdev, struct vim2m_dev, vfd); + + dprintk(dev, "Releasing last dev\n"); + v4l2_device_unregister(&dev->v4l2_dev); + v4l2_m2m_release(dev->m2m_dev); + kfree(dev); +} + static const struct v4l2_file_operations vim2m_fops = { .owner = THIS_MODULE, .open = vim2m_open, @@ -1102,7 +1112,7 @@ static const struct video_device vim2m_videodev = { .fops = &vim2m_fops, .ioctl_ops = &vim2m_ioctl_ops, .minor = -1, - .release = video_device_release_empty, + .release = vim2m_device_release, .device_caps = V4L2_CAP_VIDEO_M2M | V4L2_CAP_STREAMING, }; @@ -1123,13 +1133,13 @@ static int vim2m_probe(struct platform_device *pdev) struct video_device *vfd; int ret; - dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL); + dev = kzalloc(sizeof(*dev), GFP_KERNEL); if (!dev) return -ENOMEM; ret = v4l2_device_register(&pdev->dev, &dev->v4l2_dev); if (ret) - return ret; + goto unreg_free; atomic_set(&dev->num_inst, 0); mutex_init(&dev->dev_mutex); @@ -1192,6 +1202,8 @@ static int vim2m_probe(struct platform_device *pdev) video_unregister_device(&dev->vfd); unreg_v4l2: v4l2_device_unregister(&dev->v4l2_dev); +unreg_free: + kfree(dev); return ret; } @@ -1207,9 +1219,7 @@ static int vim2m_remove(struct platform_device *pdev) v4l2_m2m_unregister_media_controller(dev->m2m_dev); media_device_cleanup(&dev->mdev); #endif - v4l2_m2m_release(dev->m2m_dev); video_unregister_device(&dev->vfd); - v4l2_device_unregister(&dev->v4l2_dev); return 0; }
It is not possible to use devm_kzalloc since that memory is freed immediately when the device instance is unbound. Various objects like the video device may still be in use since someone has the device node open, and when that is closed it expects the memory to be around. So use kzalloc and release it at the appropriate time. Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> --- drivers/media/platform/vim2m.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-)