[PATCH cciss: fix printk format warning

[PATCH cciss: fix printk format warning

[Randy Dunlap]

From: Randy Dunlap <randy.dunlap@oracle.com>

Fix printk format warnings:
drivers/block/cciss.c:2000: warning: long long int format, long unsigned int arg (arg 2)
drivers/block/cciss.c:2035: warning: long long int format, long unsigned int arg (arg 2)

Signed-off-by: Randy Dunlap <randy.dunlap@oracle.com>
---

 drivers/block/cciss.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

--- linux-2619-rc3-pv.orig/drivers/block/cciss.c
+++ linux-2619-rc3-pv/drivers/block/cciss.c
@@ -1992,8 +1992,8 @@ cciss_read_capacity(int ctlr, int logvol
 		*block_size = BLOCK_SIZE;
 	}
 	if (*total_size != (__u32) 0)
-		printk(KERN_INFO "      blocks= %lld block_size= %d\n",
-		*total_size, *block_size);
+		printk(KERN_INFO "      blocks= %llu block_size= %d\n",
+		(unsigned long long)*total_size, *block_size);
 	kfree(buf);
 	return;
 }
@@ -2027,8 +2027,8 @@ cciss_read_capacity_16(int ctlr, int log
 		*total_size = 0;
 		*block_size = BLOCK_SIZE;
 	}
-	printk(KERN_INFO "      blocks= %lld block_size= %d\n",
-	       *total_size, *block_size);
+	printk(KERN_INFO "      blocks= %llu block_size= %d\n",
+	       (unsigned long long)*total_size, *block_size);
 	kfree(buf);
 	return;
 }


---

RE: [PATCH cciss: fix printk format warning

[Miller, Mike (OS Dev)]

 

> -----Original Message-----
> From: Randy Dunlap [mailto:randy.dunlap@oracle.com] 
> Sent: Monday, October 23, 2006 11:46 PM
> To: ISS StorageDev; lkml
> Cc: akpm
> Subject: [PATCH cciss: fix printk format warning
> 
> From: Randy Dunlap <randy.dunlap@oracle.com>
> 
> Fix printk format warnings:
> drivers/block/cciss.c:2000: warning: long long int format, 
> long unsigned int arg (arg 2)
> drivers/block/cciss.c:2035: warning: long long int format, 
> long unsigned int arg (arg 2)
> 
> Signed-off-by: Randy Dunlap <randy.dunlap@oracle.com>

Acked-by: Mike Miller <mike.miller@hp.com>

> ---
> 
>  drivers/block/cciss.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> --- linux-2619-rc3-pv.orig/drivers/block/cciss.c
> +++ linux-2619-rc3-pv/drivers/block/cciss.c
> @@ -1992,8 +1992,8 @@ cciss_read_capacity(int ctlr, int logvol
>  		*block_size = BLOCK_SIZE;
>  	}
>  	if (*total_size != (__u32) 0)
> -		printk(KERN_INFO "      blocks= %lld block_size= %d\n",
> -		*total_size, *block_size);
> +		printk(KERN_INFO "      blocks= %llu block_size= %d\n",
> +		(unsigned long long)*total_size, *block_size);
>  	kfree(buf);
>  	return;
>  }
> @@ -2027,8 +2027,8 @@ cciss_read_capacity_16(int ctlr, int log
>  		*total_size = 0;
>  		*block_size = BLOCK_SIZE;
>  	}
> -	printk(KERN_INFO "      blocks= %lld block_size= %d\n",
> -	       *total_size, *block_size);
> +	printk(KERN_INFO "      blocks= %llu block_size= %d\n",
> +	       (unsigned long long)*total_size, *block_size);
>  	kfree(buf);
>  	return;
>  }
> 
> 
> ---
> 

Re: [PATCH cciss: fix printk format warning

[Andrew Morton]

On Mon, 23 Oct 2006 21:46:08 -0700
Randy Dunlap <randy.dunlap@oracle.com> wrote:

> --- linux-2619-rc3-pv.orig/drivers/block/cciss.c
> +++ linux-2619-rc3-pv/drivers/block/cciss.c
> @@ -1992,8 +1992,8 @@ cciss_read_capacity(int ctlr, int logvol
>  		*block_size = BLOCK_SIZE;
>  	}
>  	if (*total_size != (__u32) 0)

Why is cciss_read_capacity casting *total_size to u32?

It is a sector_t.  If it happens to have the value 0xnnnnnnnn00000000 then
this expression will incorrectly evaluate to `false'.

Re: [PATCH cciss: fix printk format warning

[Roland Dreier]

 > >  	if (*total_size != (__u32) 0)
 > 
 > Why is cciss_read_capacity casting *total_size to u32?

It's not -- it's actually casting 0 to __32 -- there's no cast on the
*total_size side of the comparison.  However that just makes the cast
look even fishier.

 - R.

Re: [PATCH cciss: fix printk format warning

[Randy Dunlap]

Roland Dreier wrote:
>  > >  	if (*total_size != (__u32) 0)
>  > 
>  > Why is cciss_read_capacity casting *total_size to u32?
> 
> It's not -- it's actually casting 0 to __32 -- there's no cast on the
> *total_size side of the comparison.  However that just makes the cast
> look even fishier.
> 
>  - R.

OK, how about this one then?


	c->busaddr = (__u32) cmd_dma_handle;

where cmd_dma_handle is a dma_addr_t (u32 or u64)

and then later:

		pci_free_consistent(h->pdev, sizeof(CommandList_struct),
				    c, (dma_addr_t) c->busaddr);

-- 
~Randy

Re: [PATCH cciss: fix printk format warning

[Randy Dunlap]

Randy Dunlap wrote:
> Roland Dreier wrote:
>>  > >      if (*total_size != (__u32) 0)
>>  >  > Why is cciss_read_capacity casting *total_size to u32?
>>
>> It's not -- it's actually casting 0 to __32 -- there's no cast on the
>> *total_size side of the comparison.  However that just makes the cast
>> look even fishier.
>>
>>  - R.
> 
> OK, how about this one then?
> 
> 
>     c->busaddr = (__u32) cmd_dma_handle;
> 
> where cmd_dma_handle is a dma_addr_t (u32 or u64)
> 
> and then later:
> 
>         pci_free_consistent(h->pdev, sizeof(CommandList_struct),
>                     c, (dma_addr_t) c->busaddr);
> 

One problem with this one is that it looks like the hardware
wants a 32-bit value for busaddr:

cciss.h:         writel(c->busaddr, h->vaddr + SA5_REQUEST_PORT_OFFSET);

-- 
~Randy

Re: [PATCH cciss: fix printk format warning

[Roland Dreier]

 > OK, how about this one then?
 > 
 > 	c->busaddr = (__u32) cmd_dma_handle;
 > 
 > where cmd_dma_handle is a dma_addr_t (u32 or u64)

It's super-fishy looking but actually I think it's OK, at least as
things stand now.  As you see later from how it's freed:

 > 		pci_free_consistent(h->pdev, sizeof(CommandList_struct),
 > 				    c, (dma_addr_t) c->busaddr);

this is the bus address of memory from pci_alloc_consistent(), and
since cciss never does pci_set_consistent_dma_mask(), the mask will
remain at the default 32 bits.  So the driver is actually safe in
assuming that cmd_dma_handle fits into 32 bits.  assuming that
cmd_dma_handle.

 - R.

Re: [PATCH cciss: fix printk format warning

[Randy Dunlap]

Roland Dreier wrote:
>  > OK, how about this one then?
>  > 
>  > 	c->busaddr = (__u32) cmd_dma_handle;
>  > 
>  > where cmd_dma_handle is a dma_addr_t (u32 or u64)
> 
> It's super-fishy looking but actually I think it's OK, at least as
> things stand now.  As you see later from how it's freed:
> 
>  > 		pci_free_consistent(h->pdev, sizeof(CommandList_struct),
>  > 				    c, (dma_addr_t) c->busaddr);
> 
> this is the bus address of memory from pci_alloc_consistent(), and
> since cciss never does pci_set_consistent_dma_mask(), the mask will
> remain at the default 32 bits.  So the driver is actually safe in
> assuming that cmd_dma_handle fits into 32 bits.  assuming that
> cmd_dma_handle.

Hm, OK.  Thanks.

-- 
~Randy

RE: [PATCH cciss: fix printk format warning

[Cameron, Steve]

> Roland Dreier wrote:
> >  > >  	if (*total_size != (__u32) 0)
> >  > 
> >  > Why is cciss_read_capacity casting *total_size to u32?
> > 
> > It's not -- it's actually casting 0 to __32 -- there's no cast on the
> > *total_size side of the comparison.  However that just makes the cast
> > look even fishier.
> > 
> >  - R.
> 
> OK, how about this one then?
> 
> 
> 	c->busaddr = (__u32) cmd_dma_handle;
> 
> where cmd_dma_handle is a dma_addr_t (u32 or u64)

The command register to which that value is written
is a 32 bit register.  Cast it or not, only 32 bits
will be used.  The DMA mask used to get that memory
should ensure it's 32 bit addressable.

> and then later:
>
>		pci_free_consistent(h->pdev, sizeof(CommandList_struct),
>				    c, (dma_addr_t) c->busaddr);

Re: [PATCH cciss: fix printk format warning

[Randy Dunlap]

On Fri, 27 Oct 2006 08:11:46 -0500 Cameron, Steve wrote:

> > Roland Dreier wrote:
> > >  > >  	if (*total_size != (__u32) 0)
> > >  > 
> > >  > Why is cciss_read_capacity casting *total_size to u32?
> > > 
> > > It's not -- it's actually casting 0 to __32 -- there's no cast on the
> > > *total_size side of the comparison.  However that just makes the cast
> > > look even fishier.
> > > 
> > >  - R.
> > 
> > OK, how about this one then?
> > 
> > 
> > 	c->busaddr = (__u32) cmd_dma_handle;
> > 
> > where cmd_dma_handle is a dma_addr_t (u32 or u64)
> 
> The command register to which that value is written
> is a 32 bit register.  Cast it or not, only 32 bits
> will be used.  The DMA mask used to get that memory
> should ensure it's 32 bit addressable.

Got it.  Thanks for replying.

> > and then later:
> >
> >		pci_free_consistent(h->pdev, sizeof(CommandList_struct),
> >				    c, (dma_addr_t) c->busaddr);

---
~Randy

RE: [PATCH cciss: fix printk format warning

[Miller, Mike (OS Dev)]

 

> -----Original Message-----
> From: Cameron, Steve 
> Sent: Friday, October 27, 2006 8:12 AM
> To: Randy Dunlap; Roland Dreier
> Cc: Andrew Morton; ISS StorageDev; lkml
> Subject: RE: [PATCH cciss: fix printk format warning
> 
> > Roland Dreier wrote:
> > >  > >  	if (*total_size != (__u32) 0)
> > >  >
> > >  > Why is cciss_read_capacity casting *total_size to u32?
> > > 
> > > It's not -- it's actually casting 0 to __32 -- there's no cast on 
> > > the *total_size side of the comparison.  However that 
> just makes the 
> > > cast look even fishier.

If the volume is >2TB read_capacity will return 8 F's. We've already
added 1 to total_size which equals 0. I only care if the lower 32 bits
are zero so that's the reason for the cast.
Does that make sense or am I out in the weeds?

mikem

> > > 
> > >  - R.
> > 
> > OK, how about this one then?
> > 
> > 
> > 	c->busaddr = (__u32) cmd_dma_handle;
> > 
> > where cmd_dma_handle is a dma_addr_t (u32 or u64)
> 
> The command register to which that value is written is a 32 
> bit register.  Cast it or not, only 32 bits will be used.  
> The DMA mask used to get that memory should ensure it's 32 
> bit addressable.
> 
> > and then later:
> >
> >		pci_free_consistent(h->pdev, sizeof(CommandList_struct),
> >				    c, (dma_addr_t) c->busaddr);
> 
> 
> 
> 
> 

Re: [PATCH cciss: fix printk format warning

[Andrew Morton]

On Thu, 26 Oct 2006 16:19:56 -0700
Roland Dreier <rdreier@cisco.com> wrote:

>  > >  	if (*total_size != (__u32) 0)
>  > 
>  > Why is cciss_read_capacity casting *total_size to u32?
> 
> It's not -- it's actually casting 0 to __32

bah.

> -- there's no cast on the
> *total_size side of the comparison.  However that just makes the cast
> look even fishier.

yup, it's harmless.  Just something which was put in there to entertain passers-by.