[PATCH] [media] gspca: replaced static allocation by video_device_alloc/video_device_release

[PATCH] [media] gspca: replaced static allocation by video_device_alloc/video_device_release

[Ezequiel]

Pushed video_device initialization into a separate function.
Replaced static allocation of struct video_device by 
video_device_alloc/video_device_release usage.

Signed-off-by: Ezequiel Garcia <elezegarcia@gmail.com>
---

diff --git a/drivers/media/video/gspca/gspca.c b/drivers/media/video/gspca/gspca.c
index 881e04c..1f27f05 100644
--- a/drivers/media/video/gspca/gspca.c
+++ b/drivers/media/video/gspca/gspca.c
@@ -1292,10 +1292,12 @@ static int vidioc_enum_frameintervals(struct file *filp, void *priv,
 
 static void gspca_release(struct video_device *vfd)
 {
-	struct gspca_dev *gspca_dev = container_of(vfd, struct gspca_dev, vdev);
+	struct gspca_dev *gspca_dev = video_get_drvdata(vfd);
 
 	PDEBUG(D_PROBE, "%s released",
-		video_device_node_name(&gspca_dev->vdev));
+		video_device_node_name(gspca_dev->vdev));
+
+	video_device_release(vfd);
 
 	kfree(gspca_dev->usb_buf);
 	kfree(gspca_dev);
@@ -1304,9 +1306,11 @@ static void gspca_release(struct video_device *vfd)
 static int dev_open(struct file *file)
 {
 	struct gspca_dev *gspca_dev;
+	struct video_device *vdev;
 
 	PDEBUG(D_STREAM, "[%s] open", current->comm);
-	gspca_dev = (struct gspca_dev *) video_devdata(file);
+	vdev = video_devdata(file);
+	gspca_dev = video_get_drvdata(vdev);
 	if (!gspca_dev->present)
 		return -ENODEV;
 
@@ -1318,10 +1322,10 @@ static int dev_open(struct file *file)
 #ifdef GSPCA_DEBUG
 	/* activate the v4l2 debug */
 	if (gspca_debug & D_V4L2)
-		gspca_dev->vdev.debug |= V4L2_DEBUG_IOCTL
+		gspca_dev->vdev->debug |= V4L2_DEBUG_IOCTL
 					| V4L2_DEBUG_IOCTL_ARG;
 	else
-		gspca_dev->vdev.debug &= ~(V4L2_DEBUG_IOCTL
+		gspca_dev->vdev->debug &= ~(V4L2_DEBUG_IOCTL
 					| V4L2_DEBUG_IOCTL_ARG);

Re: [PATCH] [media] gspca: replaced static allocation by video_device_alloc/video_device_release

[Hans de Goede]

Hi,

On 11/19/2011 07:50 PM, Ezequiel wrote:
> Pushed video_device initialization into a separate function.
> Replaced static allocation of struct video_device by
> video_device_alloc/video_device_release usage.

NACK! I see a video_device_release call here, but not a
video_device_alloc, also you're messing with quite sensitive code
here (because a usb device can be unplugged at any time, including
when the /dev/video node is open by a process), and changing it
from static to dynamic allocation my have more consequences
then you see at first (I did not analyze all the code paths
for the proposed change, since the last time I audited them for
the current static allocation of the videodevice struct code took
me hours).

Also static allocation (as part of the driver struct) in general is
better then dynamic as it needs less code and helps avoiding memory
fragmentation.

All in all I cannot help but feel that you're diving into a piece
of code with some drive by shooting style patch without knowing
the code in question at all, please don't do that!

Regards,

Hans



>
> Signed-off-by: Ezequiel Garcia<elezegarcia@gmail.com>
> ---
>
> diff --git a/drivers/media/video/gspca/gspca.c b/drivers/media/video/gspca/gspca.c
> index 881e04c..1f27f05 100644
> --- a/drivers/media/video/gspca/gspca.c
> +++ b/drivers/media/video/gspca/gspca.c
> @@ -1292,10 +1292,12 @@ static int vidioc_enum_frameintervals(struct file *filp, void *priv,
>
>   static void gspca_release(struct video_device *vfd)
>   {
> -	struct gspca_dev *gspca_dev = container_of(vfd, struct gspca_dev, vdev);
> +	struct gspca_dev *gspca_dev = video_get_drvdata(vfd);
>
>   	PDEBUG(D_PROBE, "%s released",
> -		video_device_node_name(&gspca_dev->vdev));
> +		video_device_node_name(gspca_dev->vdev));
> +
> +	video_device_release(vfd);
>
>   	kfree(gspca_dev->usb_buf);
>   	kfree(gspca_dev);
> @@ -1304,9 +1306,11 @@ static void gspca_release(struct video_device *vfd)
>   static int dev_open(struct file *file)
>   {
>   	struct gspca_dev *gspca_dev;
> +	struct video_device *vdev;
>
>   	PDEBUG(D_STREAM, "[%s] open", current->comm);
> -	gspca_dev = (struct gspca_dev *) video_devdata(file);
> +	vdev = video_devdata(file);
> +	gspca_dev = video_get_drvdata(vdev);
>   	if (!gspca_dev->present)
>   		return -ENODEV;
>
> @@ -1318,10 +1322,10 @@ static int dev_open(struct file *file)
>   #ifdef GSPCA_DEBUG
>   	/* activate the v4l2 debug */
>   	if (gspca_debug&  D_V4L2)
> -		gspca_dev->vdev.debug |= V4L2_DEBUG_IOCTL
> +		gspca_dev->vdev->debug |= V4L2_DEBUG_IOCTL
>   					| V4L2_DEBUG_IOCTL_ARG;
>   	else
> -		gspca_dev->vdev.debug&= ~(V4L2_DEBUG_IOCTL
> +		gspca_dev->vdev->debug&= ~(V4L2_DEBUG_IOCTL
>   					| V4L2_DEBUG_IOCTL_ARG);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [media] gspca: replaced static allocation by video_device_alloc/video_device_release

[Ezequiel]

On Sat, Nov 19, 2011 at 08:20:22PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 11/19/2011 07:50 PM, Ezequiel wrote:
> > Pushed video_device initialization into a separate function.
> > Replaced static allocation of struct video_device by
> > video_device_alloc/video_device_release usage.
> 
> NACK! I see a video_device_release call here, but not a
> video_device_alloc, also you're messing with quite sensitive code
> here (because a usb device can be unplugged at any time, including
> when the /dev/video node is open by a process), and changing it
> from static to dynamic allocation my have more consequences
> then you see at first (I did not analyze all the code paths
> for the proposed change, since the last time I audited them for
> the current static allocation of the videodevice struct code took
> me hours).
> 
> Also static allocation (as part of the driver struct) in general is
> better then dynamic as it needs less code and helps avoiding memory
> fragmentation.
> 
> All in all I cannot help but feel that you're diving into a piece
> of code with some drive by shooting style patch without knowing
> the code in question at all, please don't do that!
> 
> Regards,
> 
> Hans
> 

Hi Hans,

Sorry, really dont know what happened, 
I sent an incomplete patch version.
(some vim yank-key error).

I understand your observations about static vs dynamic, 
but please could you review the right patch.

Thanks,
Ezequiel.