[PATCH]: hpilo: fix pointer warning in ilo_ccb_setup [v2]

[PATCH]: hpilo: fix pointer warning in ilo_ccb_setup [v2]

[Prarit Bhargava]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 3018 bytes --]

Fix i386 PAE compile warning:

drivers/misc/hpilo.c: In function ‘ilo_ccb_setup’:
drivers/misc/hpilo.c:274: warning: cast to pointer from integer of different size

dma_addr_t is 64 on i386 PAE which causes a size mismatch.

Signed-off-by: Prarit Bhargava <prarit@redhat.com>

diff --git a/drivers/misc/hpilo.c b/drivers/misc/hpilo.c
index 98ad012..557a8c2 100644
--- a/drivers/misc/hpilo.c
+++ b/drivers/misc/hpilo.c
@@ -256,7 +256,8 @@ static void ilo_ccb_close(struct pci_dev *pdev, struct ccb_data *data)
 
 static int ilo_ccb_setup(struct ilo_hwinfo *hw, struct ccb_data *data, int slot)
 {
-	char *dma_va, *dma_pa;
+	char *dma_va;
+	dma_addr_t dma_pa;
 	struct ccb *driver_ccb, *ilo_ccb;
 
 	driver_ccb = &data->driver_ccb;
@@ -272,12 +273,12 @@ static int ilo_ccb_setup(struct ilo_hwinfo *hw, struct ccb_data *data, int slot)
 		return -ENOMEM;
 
 	dma_va = (char *)data->dma_va;
-	dma_pa = (char *)data->dma_pa;
+	dma_pa = data->dma_pa;
 
 	memset(dma_va, 0, data->dma_size);
 
 	dma_va = (char *)roundup((unsigned long)dma_va, ILO_START_ALIGN);
-	dma_pa = (char *)roundup((unsigned long)dma_pa, ILO_START_ALIGN);
+	dma_pa = roundup(dma_pa, ILO_START_ALIGN);
 
 	/*
 	 * Create two ccb's, one with virt addrs, one with phys addrs.
@@ -288,26 +289,26 @@ static int ilo_ccb_setup(struct ilo_hwinfo *hw, struct ccb_data *data, int slot)
 
 	fifo_setup(dma_va, NR_QENTRY);
 	driver_ccb->ccb_u1.send_fifobar = dma_va + FIFOHANDLESIZE;
-	ilo_ccb->ccb_u1.send_fifobar = dma_pa + FIFOHANDLESIZE;
+	ilo_ccb->ccb_u1.send_fifobar_pa = dma_pa + FIFOHANDLESIZE;
 	dma_va += fifo_sz(NR_QENTRY);
 	dma_pa += fifo_sz(NR_QENTRY);
 
 	dma_va = (char *)roundup((unsigned long)dma_va, ILO_CACHE_SZ);
-	dma_pa = (char *)roundup((unsigned long)dma_pa, ILO_CACHE_SZ);
+	dma_pa = roundup(dma_pa, ILO_CACHE_SZ);
 
 	fifo_setup(dma_va, NR_QENTRY);
 	driver_ccb->ccb_u3.recv_fifobar = dma_va + FIFOHANDLESIZE;
-	ilo_ccb->ccb_u3.recv_fifobar = dma_pa + FIFOHANDLESIZE;
+	ilo_ccb->ccb_u3.recv_fifobar_pa = dma_pa + FIFOHANDLESIZE;
 	dma_va += fifo_sz(NR_QENTRY);
 	dma_pa += fifo_sz(NR_QENTRY);
 
 	driver_ccb->ccb_u2.send_desc = dma_va;
-	ilo_ccb->ccb_u2.send_desc = dma_pa;
+	ilo_ccb->ccb_u2.send_desc_pa = dma_pa;
 	dma_pa += desc_mem_sz(NR_QENTRY);
 	dma_va += desc_mem_sz(NR_QENTRY);
 
 	driver_ccb->ccb_u4.recv_desc = dma_va;
-	ilo_ccb->ccb_u4.recv_desc = dma_pa;
+	ilo_ccb->ccb_u4.recv_desc_pa = dma_pa;
 
 	driver_ccb->channel = slot;
 	ilo_ccb->channel = slot;
diff --git a/drivers/misc/hpilo.h b/drivers/misc/hpilo.h
index 247eb38..54e43ad 100644
--- a/drivers/misc/hpilo.h
+++ b/drivers/misc/hpilo.h
@@ -79,21 +79,21 @@ struct ilo_hwinfo {
 struct ccb {
 	union {
 		char *send_fifobar;
-		u64 padding1;
+		u64 send_fifobar_pa;
 	} ccb_u1;
 	union {
 		char *send_desc;
-		u64 padding2;
+		u64 send_desc_pa;
 	} ccb_u2;
 	u64 send_ctrl;
 
 	union {
 		char *recv_fifobar;
-		u64 padding3;
+		u64 recv_fifobar_pa;
 	} ccb_u3;
 	union {
 		char *recv_desc;
-		u64 padding4;
+		u64 recv_desc_pa;
 	} ccb_u4;
 	u64 recv_ctrl;
 

Re: [PATCH]: hpilo: fix pointer warning in ilo_ccb_setup [v2]

[Andrew Morton]

On Tue, 8 Jun 2010 11:25:36 -0400
Prarit Bhargava <prarit@redhat.com> wrote:

> Fix i386 PAE compile warning:
> 
> drivers/misc/hpilo.c: In function ___ilo_ccb_setup___:
> drivers/misc/hpilo.c:274: warning: cast to pointer from integer of different size
> 
> dma_addr_t is 64 on i386 PAE which causes a size mismatch.
> 
> Signed-off-by: Prarit Bhargava <prarit@redhat.com>
> 
> diff --git a/drivers/misc/hpilo.c b/drivers/misc/hpilo.c
> index 98ad012..557a8c2 100644
> --- a/drivers/misc/hpilo.c
> +++ b/drivers/misc/hpilo.c
> @@ -256,7 +256,8 @@ static void ilo_ccb_close(struct pci_dev *pdev, struct ccb_data *data)
>  
>  static int ilo_ccb_setup(struct ilo_hwinfo *hw, struct ccb_data *data, int slot)
>  {
> -	char *dma_va, *dma_pa;
> +	char *dma_va;
> +	dma_addr_t dma_pa;
>  	struct ccb *driver_ccb, *ilo_ccb;
>  
>  	driver_ccb = &data->driver_ccb;
> @@ -272,12 +273,12 @@ static int ilo_ccb_setup(struct ilo_hwinfo *hw, struct ccb_data *data, int slot)
>  		return -ENOMEM;
>  
>  	dma_va = (char *)data->dma_va;
> -	dma_pa = (char *)data->dma_pa;
> +	dma_pa = data->dma_pa;
>  
>  	memset(dma_va, 0, data->dma_size);
>  
>  	dma_va = (char *)roundup((unsigned long)dma_va, ILO_START_ALIGN);
> -	dma_pa = (char *)roundup((unsigned long)dma_pa, ILO_START_ALIGN);
> +	dma_pa = roundup(dma_pa, ILO_START_ALIGN);

Well.  roundup() does division, and 64-bit divides can cause a linkage
error on i386.  But the compiler will save us because ILO_START_ALIGN
is a power-of-2.  But as we require that ILO_START_ALIGN be a
power-of-2, we may as well use round_up(), which doesn't use division.

> --- a/drivers/misc/hpilo.h
> +++ b/drivers/misc/hpilo.h
> @@ -79,21 +79,21 @@ struct ilo_hwinfo {
>  struct ccb {
>  	union {
>  		char *send_fifobar;
> -		u64 padding1;
> +		u64 send_fifobar_pa;
>  	} ccb_u1;
>  	union {
>  		char *send_desc;
> -		u64 padding2;
> +		u64 send_desc_pa;
>  	} ccb_u2;
>  	u64 send_ctrl;
>  
>  	union {
>  		char *recv_fifobar;
> -		u64 padding3;
> +		u64 recv_fifobar_pa;
>  	} ccb_u3;
>  	union {
>  		char *recv_desc;
> -		u64 padding4;
> +		u64 recv_desc_pa;
>  	} ccb_u4;
>  	u64 recv_ctrl;

So what's happening here.  As send_fifobar_pa, send_desc_pa,
recv_fifobar_pa and recv_desc_pa are only ever written to, I assume
that we're writing a 64-bit value and then reading it back via the
32-bit value which shares the same storage.

Which means that this code doesn't have a hope of working on big-endian
machines?

RE: [PATCH]: hpilo: fix pointer warning in ilo_ccb_setup [v2]

[Altobelli, David]

> From Andrew Morton:
> So what's happening here.  As send_fifobar_pa, send_desc_pa,
> recv_fifobar_pa and recv_desc_pa are only ever written to, I assume
> that we're writing a 64-bit value and then reading it back via the
> 32-bit value which shares the same storage.

The physical addresses are used to fill out a structure defined 
by iLO firmware that has 64 bit fields for the various queues or descriptors.
A copy with physical addrs is copied to device memory.  The size of the 
structure matters for the physical copy, which is why those fields need to be u64,
not a variable type like dma_addr_t, or a pointer.
We keep a copy with virtual addresses for use by the kernel.  The size of the 
kernel pointer doesn't really matter, it just tells the driver where to write 
in the shared memory.  Does that answer your question, maybe I missed 
what concerned you?