Re: Fwd: [PATCH] ARM: OMAP2: Camera: Add OMAP2 24xx camera driver.

[Sakari Ailus <sakari.ailus@nokia.com>]

André Goddard Rosa wrote:
> Hi, Sakari!

Hello,

And thanks for the comments!

...
>> +static int
>> +omap24xxcam_mmap(struct file *file, struct vm_area_struct *vma)
>> +{
>> +       struct omap24xxcam_fh *fh = file->private_data;
>> +       int err;
>> +
>> +       /* let the video-buf mapper check arguments and set-up structures */
>> +       err = videobuf_mmap_mapper(&fh->vbq, vma);
>> +       if (err)
>> +               goto err1;
>> +
>> +       vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
>> +
>> +       /* do mapping to our allocated buffers */
>> +       err = omap24xxcam_mmap_buffers(file, vma);
>> +       if (err)
>> +               goto err2;
> 
> Same comments of Trilok here, but also please find that
> videobuf_mmap_mapper() allocates memory. Where is it
> being released in this case?
> 
> Should we call videobuf_mmap_free() in this error case?

It's that kfree. If you look at the other drivers, no special attention 
is paid on this one. It's because the vidobuf_mmap_mapper is the last 
function called, i.e. there is no case where videobuf_mmap_mapper would 
succeed but the driver's mmap call would fail.

...
>> +       omap24xxcam_sensor_exit(cam);
>> +
>> +       if (cam->vfd) {
>> +               if (cam->vfd->minor == -1) {
>> +                       /* The device was never registered, so release the
>> +                        * video_device struct directly.
>> +                        */
>> +                       video_device_release(cam->vfd);
>> +               } else {
>> +                       /* The unregister function will release the
>> video_device
>> +                        * struct as well as unregistering it.
>> +                        */
>> +                       video_unregister_device(cam->vfd);
> 
> Are you sure that video_unregister_device() calls  video_device_release(cam->vfd)
> or does kfree(cam->vfd) in its regular path?

video_unregister_device doesn't do that directly. I have to check 
further what's happening here.

It indeed looks like a potential memory leak.

Regards,

-- 
Sakari Ailus
sakari.ailus@nokia.com