[PATCH] fix compiler warning in drivers/media/video/video-buf.c

[PATCH] fix compiler warning in drivers/media/video/video-buf.c

[Martin Bligh]

[-- Attachment #1: Type: text/plain, Size: 231 bytes --]

Using a double cast to avoid compiler warnings when
building for PAE. Compiler doesn't like direct casting
of a 32 bit ptr to 64 bit integer.

From: Sujoy Gupta <sujoy@google.com>
Signed-off-by: Martin J. Bligh <mbligh@google.com>

[-- Attachment #2: 2.6.18-videobuf --]
[-- Type: text/plain, Size: 864 bytes --]

diff -aurpN -X /home/mbligh/.diff.exclude linux-2.6.18/drivers/media/video/video-buf.c 2.6.18-videobuf/drivers/media/video/video-buf.c
--- linux-2.6.18/drivers/media/video/video-buf.c	2006-06-17 18:49:35.000000000 -0700
+++ 2.6.18-videobuf/drivers/media/video/video-buf.c	2006-09-28 10:28:54.000000000 -0700
@@ -365,7 +365,12 @@ videobuf_iolock(struct videobuf_queue* q
 		if (NULL == fbuf)
 			return -EINVAL;
 		/* FIXME: need sanity checks for vb->boff */
-		bus   = (dma_addr_t)fbuf->base + vb->boff;
+		/*
+		 * Using a double cast to avoid compiler warnings when
+		 * building for PAE. Compiler doesn't like direct casting 
+		 * of a 32 bit ptr to 64 bit integer.
+		 */
+		bus   = (dma_addr_t)(size_t)fbuf->base + vb->boff;
 		pages = PAGE_ALIGN(vb->size) >> PAGE_SHIFT;
 		err = videobuf_dma_init_overlay(&vb->dma,PCI_DMA_FROMDEVICE,
 						bus, pages);

Re: [PATCH] fix compiler warning in drivers/media/video/video-buf.c

[Nish Aravamudan]

On 9/28/06, Martin Bligh <mbligh@google.com> wrote:
> Using a double cast to avoid compiler warnings when
> building for PAE. Compiler doesn't like direct casting
> of a 32 bit ptr to 64 bit integer.
>
> From: Sujoy Gupta <sujoy@google.com>

I believe this is supposed to be the first line in the mail?

> Signed-off-by: Martin J. Bligh <mbligh@google.com>

Thanks,
Nish

Re: [PATCH] fix compiler warning in drivers/media/video/video-buf.c

[Andrew Morton]

On Thu, 28 Sep 2006 10:31:58 -0700
Martin Bligh <mbligh@google.com> wrote:

> Using a double cast to avoid compiler warnings when
> building for PAE. Compiler doesn't like direct casting
> of a 32 bit ptr to 64 bit integer.
> 
> ...
>
> diff -aurpN -X /home/mbligh/.diff.exclude linux-2.6.18/drivers/media/video/video-buf.c 2.6.18-videobuf/drivers/media/video/video-buf.c
> --- linux-2.6.18/drivers/media/video/video-buf.c	2006-06-17 18:49:35.000000000 -0700
> +++ 2.6.18-videobuf/drivers/media/video/video-buf.c	2006-09-28 10:28:54.000000000 -0700
> @@ -365,7 +365,12 @@ videobuf_iolock(struct videobuf_queue* q
>  		if (NULL == fbuf)
>  			return -EINVAL;
>  		/* FIXME: need sanity checks for vb->boff */
> -		bus   = (dma_addr_t)fbuf->base + vb->boff;
> +		/*
> +		 * Using a double cast to avoid compiler warnings when
> +		 * building for PAE. Compiler doesn't like direct casting 
> +		 * of a 32 bit ptr to 64 bit integer.
> +		 */
> +		bus   = (dma_addr_t)(size_t)fbuf->base + vb->boff;
>  		pages = PAGE_ALIGN(vb->size) >> PAGE_SHIFT;
>  		err = videobuf_dma_init_overlay(&vb->dma,PCI_DMA_FROMDEVICE,
>  						bus, pages);
> 

We normally cast pointers to unsigned long prior to assigning them to
64-bit values.  I'll make that change.

That being said, this driver is wrong to be storing dma addresses in a
void*.  And indeed there is a FIXME regarding this at
include/linux/videodev2.h:476, so I guess hiding this warning won't obscure
any fault which wasn't already known about..

Re: [PATCH] fix compiler warning in drivers/media/video/video-buf.c

[Mauro Carvalho Chehab]

Em Qui, 2006-09-28 às 10:51 -0700, Andrew Morton escreveu:
> On Thu, 28 Sep 2006 10:31:58 -0700
> Martin Bligh <mbligh@google.com> wrote:

> That being said, this driver is wrong to be storing dma addresses in a
> void*.  And indeed there is a FIXME regarding this at
> include/linux/videodev2.h:476, so I guess hiding this warning won't obscure
> any fault which wasn't already known about..

Yes. The original structure is:

struct v4l2_framebuffer
{
        __u32                   capability;
        __u32                   flags;
        void*                   base;
        struct v4l2_pix_format  fmt;
};

Since this is used at ioctl definition, changing this would break
userspace apps. We might replace this to something like:

struct v4l2_framebuffer
{
        __u32                   capability;
        __u32                   flags;
	union {
	        void*		base_ptr; /*FOO definition to avoid breaking userpace apps */
		dma_addr_t	base;
	}
        struct v4l2_pix_format  fmt;
};

This way, base will have the expected type, and it won't break any
userspace app if sizeof(void *)<=sizeof(base). I think this is true for
all architectures (anyway, if it isn't, v4l is broken anyway).

What do you think?

Cheers, 
Mauro.

Re: [PATCH] fix compiler warning in drivers/media/video/video-buf.c

[Martin Bligh]

Mauro Carvalho Chehab wrote:
> Em Qui, 2006-09-28 às 10:51 -0700, Andrew Morton escreveu:
> 
>>On Thu, 28 Sep 2006 10:31:58 -0700
>>Martin Bligh <mbligh@google.com> wrote:
> 
> 
>>That being said, this driver is wrong to be storing dma addresses in a
>>void*.  And indeed there is a FIXME regarding this at
>>include/linux/videodev2.h:476, so I guess hiding this warning won't obscure
>>any fault which wasn't already known about..
> 
> 
> Yes. The original structure is:
> 
> struct v4l2_framebuffer
> {
>         __u32                   capability;
>         __u32                   flags;
>         void*                   base;
>         struct v4l2_pix_format  fmt;
> };
> 
> Since this is used at ioctl definition, changing this would break
> userspace apps. We might replace this to something like:
> 
> struct v4l2_framebuffer
> {
>         __u32                   capability;
>         __u32                   flags;
> 	union {
> 	        void*		base_ptr; /*FOO definition to avoid breaking userpace apps */
> 		dma_addr_t	base;
> 	}
>         struct v4l2_pix_format  fmt;
> };
> 
> This way, base will have the expected type, and it won't break any
> userspace app if sizeof(void *)<=sizeof(base). I think this is true for
> all architectures (anyway, if it isn't, v4l is broken anyway).

Won't that just make the userspace apps not work properly? Not that
they do right now, but how does masking the problem help?

M.