Re: [PATCH 5/7] vim2m: replace devm_kzalloc by kzalloc

[Hans Verkuil <hverkuil-cisco@xs4all.nl>]

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;
>>  }
>