| Message ID | CAPGEy5fnBopTW6wsTiSxxALWKK3s25ZP_Lu_rEoMLZTRFbkmNg@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Jose, On Sat, May 11, 2013 at 8:00 AM, Jose Pablo Carballo <jose.carballo@ridgerun.com> wrote: > Hi, > > In file drivers/media/platform/davinci/vpfe_capture.c, function > vpfe_remove(): > > diff --git a/drivers/media/platform/davinci/vpfe_capture.c > b/drivers/media/platform/davinci/vpfe_capture.c > index 28d019d..f0f272f 100644 > --- a/drivers/media/platform/davinci/vpfe_capture.c > +++ b/drivers/media/platform/davinci/vpfe_capture.c > @@ -2045,6 +2045,7 @@ static int vpfe_remove(struct platform_device *pdev) > kfree(vpfe_dev->sd); > v4l2_device_unregister(&vpfe_dev->v4l2_dev); > video_unregister_device(vpfe_dev->video_dev); > + video_device_release(vpfe_dev->video_dev); > kfree(vpfe_dev); > kfree(ccdc_cfg); > return 0; > > > According to my understanding, the story of vpfe_dev->video_dev is as > follows: > > 1. It starts to exist at vpfe_probe(), through the function > video_device_alloc(): > > vfd = video_device_alloc(); > ... > vfd->release = video_device_release; > ... > vpfe_dev->video_dev = vfd; > ... > ret = video_register_device(vpfe_dev->video_dev, > VFL_TYPE_GRABBER, -1); > > > Note that video_device_alloc() is fundamentally a call to kzalloc(), and > video_device_release() is the respective call to kfree(). > > 2. It ends to exist in vpfe_remove(): > > video_unregister_device(vpfe_dev->video_dev); > ... > kfree(vpfe_dev); <- I believe this won't free the memory pointed by > vpfe_dev->video_dev > > > If you look at the implementation of video_unregister_device(), you would > notice it doesn't call video_device_release(). Unless that calls happens > automatically at some point, given that vpfe_dev->video_dev->release points > to video_device_release(), the explicit call to video_device_release() is > needed. Another hint to this, is that if you look at the error handling > logic at the vpfe_probe() function, you will see the proposed chain of > events do happen: > > probe_out_v4l2_unregister: > v4l2_device_unregister(&vpfe_dev->v4l2_dev); > probe_out_video_release: > if (!video_is_registered(vpfe_dev->video_dev)) > video_device_release(vpfe_dev->video_dev); <- Explicitly freed this > memory > NACK, refer this link https://www.kernel.org/doc/Documentation/video4linux/v4l2-framework.txt for more details especially refer "video_device cleanup" section. Regards, --Prabhakar Lad
Hi Prabhakar, "When the last user of the video device node exits, then the vdev->release() callback is called and you can do the final cleanup there." Got it, thanks, On Sun, May 12, 2013 at 1:21 AM, Prabhakar Lad <prabhakar.csengg@gmail.com>wrote: > Hi Jose, > > On Sat, May 11, 2013 at 8:00 AM, Jose Pablo Carballo > <jose.carballo@ridgerun.com> wrote: > > Hi, > > > > In file drivers/media/platform/davinci/vpfe_capture.c, function > > vpfe_remove(): > > > > diff --git a/drivers/media/platform/davinci/vpfe_capture.c > > b/drivers/media/platform/davinci/vpfe_capture.c > > index 28d019d..f0f272f 100644 > > --- a/drivers/media/platform/davinci/vpfe_capture.c > > +++ b/drivers/media/platform/davinci/vpfe_capture.c > > @@ -2045,6 +2045,7 @@ static int vpfe_remove(struct platform_device > *pdev) > > kfree(vpfe_dev->sd); > > v4l2_device_unregister(&vpfe_dev->v4l2_dev); > > video_unregister_device(vpfe_dev->video_dev); > > + video_device_release(vpfe_dev->video_dev); > > kfree(vpfe_dev); > > kfree(ccdc_cfg); > > return 0; > > > > > > According to my understanding, the story of vpfe_dev->video_dev is as > > follows: > > > > 1. It starts to exist at vpfe_probe(), through the function > > video_device_alloc(): > > > > vfd = video_device_alloc(); > > ... > > vfd->release = video_device_release; > > ... > > vpfe_dev->video_dev = vfd; > > ... > > ret = video_register_device(vpfe_dev->video_dev, > > VFL_TYPE_GRABBER, -1); > > > > > > Note that video_device_alloc() is fundamentally a call to kzalloc(), and > > video_device_release() is the respective call to kfree(). > > > > 2. It ends to exist in vpfe_remove(): > > > > video_unregister_device(vpfe_dev->video_dev); > > ... > > kfree(vpfe_dev); <- I believe this won't free the memory pointed by > > vpfe_dev->video_dev > > > > > > If you look at the implementation of video_unregister_device(), you would > > notice it doesn't call video_device_release(). Unless that calls happens > > automatically at some point, given that vpfe_dev->video_dev->release > points > > to video_device_release(), the explicit call to video_device_release() is > > needed. Another hint to this, is that if you look at the error handling > > logic at the vpfe_probe() function, you will see the proposed chain of > > events do happen: > > > > probe_out_v4l2_unregister: > > v4l2_device_unregister(&vpfe_dev->v4l2_dev); > > probe_out_video_release: > > if (!video_is_registered(vpfe_dev->video_dev)) > > video_device_release(vpfe_dev->video_dev); <- Explicitly freed this > > memory > > > > NACK, refer this link > https://www.kernel.org/doc/Documentation/video4linux/v4l2-framework.txt > for more details especially refer "video_device cleanup" section. > > Regards, > --Prabhakar Lad >
diff --git a/drivers/media/platform/davinci/vpfe_capture.c b/drivers/media/platform/davinci/vpfe_capture.c index 28d019d..f0f272f 100644 --- a/drivers/media/platform/davinci/vpfe_capture.c +++ b/drivers/media/platform/davinci/vpfe_capture.c @@ -2045,6 +2045,7 @@ static int vpfe_remove(struct platform_device *pdev) kfree(vpfe_dev->sd); v4l2_device_unregister(&vpfe_dev->v4l2_dev); video_unregister_device(vpfe_dev->video_dev); + video_device_release(vpfe_dev->video_dev); kfree(vpfe_dev); kfree(ccdc_cfg); return 0;
Hi, In file drivers/media/platform/davinci/vpfe_capture.c, function vpfe_remove(): According to my understanding, the story of vpfe_dev->video_dev is as follows: 1. It starts to exist at vpfe_probe(), through the function video_device_alloc(): vfd = video_device_alloc(); ... vfd->release = video_device_release; ... vpfe_dev->video_dev = vfd; ... ret = video_register_device(vpfe_dev->video_dev, VFL_TYPE_GRABBER, -1); Note that video_device_alloc() is fundamentally a call to kzalloc(), and video_device_release() is the respective call to kfree(). 2. It ends to exist in vpfe_remove(): video_unregister_device(vpfe_dev->video_dev); ... kfree(vpfe_dev); <- I believe this won't free the memory pointed by vpfe_dev->video_dev If you look at the implementation of video_unregister_device(), you would notice it doesn't call video_device_release(). Unless that calls happens automatically at some point, given that vpfe_dev->video_dev->release points to video_device_release(), the explicit call to video_device_release() is needed. Another hint to this, is that if you look at the error handling logic at the vpfe_probe() function, you will see the proposed chain of events do happen: probe_out_v4l2_unregister: v4l2_device_unregister(&vpfe_dev->v4l2_dev); probe_out_video_release: if (!video_is_registered(vpfe_dev->video_dev)) video_device_release(vpfe_dev->video_dev); <- Explicitly freed this memory Thanks, JP