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-ci...@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.pinch...@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;
>> }
>
