[OPW,kernel] Removed warning on 64 bit OS of casting pointer from integer of different size
diff mbox

Message ID 52689e8f.c582440a.6809.ffffdb62@mx.google.com
State Rejected
Headers show

Commit Message

ritas1188@gmail.com Oct. 24, 2013, 4:13 a.m. UTC
From: Rita Sinha <ritas1188@gmail.com>

Signed-off-by: Rita Sinha <ritas1188@gmail.com>
---
 drivers/staging/bcm/CmHost.c |   13 +++++++++++++
 drivers/staging/bcm/CmHost.h |    1 +
 2 files changed, 14 insertions(+)

Comments

Josh Triplett Oct. 24, 2013, 5:11 a.m. UTC | #1
On Thu, Oct 24, 2013 at 09:43:43AM +0530, ritas1188@gmail.com wrote:

Sorry, this isn't the right patch, for several reasons:

- x86-32 is not the only 32-bit Linux platform.
- In most cases, ifdefs should not appear in .c files, especially in the
  middle of functions; that's a sign that some new function needs to
  exist that solves the problem generically.
- ntohl, despite the name, operates on 32-bit values, not longs, hence
  this warning.  *If* this byte-swapping made any sense (which isn't
  necessarily clear), the right fix would be to write functions like
  "le_ulong_to_cpu" and "be_ulong_to_cpu", defined on "unsigned long".
- As suggested by the comment "this can't possibly be right", there's a
  more fundamental issue here; for some crazy reason the driver is
  ending up with a byteswapped pointer from the card and needing to
  access the target of that pointer.


- Josh Triplett

> --- a/drivers/staging/bcm/CmHost.c
> +++ b/drivers/staging/bcm/CmHost.c
> @@ -1384,7 +1384,11 @@ ULONG StoreCmControlResponseMessage(struct bcm_mini_adapter *Adapter, PVOID pvBu
>  	}
>  
>  	/* this can't possibly be right */
> +#ifdef CONFIG_X86_32
>  	pstAddIndication->psfAuthorizedSet = (struct bcm_connect_mgr_params *)ntohl((ULONG)pstAddIndication->psfAuthorizedSet);
> +#else
> +	pstAddIndication->psfAuthorizedSet = (struct bcm_connect_mgr_params *)ntohl_64((ULONG)pstAddIndication->psfAuthorizedSet);
> +#endif
>  
>  	if (pstAddIndicationAlt->u8Type == DSA_REQ) {
g>  		struct bcm_add_request AddRequest;
> @@ -1423,7 +1427,11 @@ ULONG StoreCmControlResponseMessage(struct bcm_mini_adapter *Adapter, PVOID pvBu
>  		return 0;
>  	}
>  
> +#ifdef CONFIG_X86_32
>  	pstAddIndication->psfAdmittedSet = (struct bcm_connect_mgr_params *)ntohl((ULONG)pstAddIndication->psfAdmittedSet);
> +#else
> +	pstAddIndication->psfAdmittedSet = (struct bcm_connect_mgr_params *)ntohl_64((ULONG)pstAddIndication->psfAdmittedSet);
> +#endif
>  
>  	/* ACTIVE SET */
>  	pstAddIndication->psfActiveSet = (struct bcm_connect_mgr_params *)
> @@ -1437,7 +1445,12 @@ ULONG StoreCmControlResponseMessage(struct bcm_mini_adapter *Adapter, PVOID pvBu
>  		return 0;
>  	}
>  
> +#ifdef CONFIG_X86_32
> +
>  	pstAddIndication->psfActiveSet = (struct bcm_connect_mgr_params *)ntohl((ULONG)pstAddIndication->psfActiveSet);
> +#else
> +	pstAddIndication->psfActiveSet = (struct bcm_connect_mgr_params *)ntohl_64((ULONG)pstAddIndication->psfActiveSet);
> +#endif
>  
>  	(*puBufferLength) = sizeof(struct bcm_add_indication);
>  	*(struct bcm_add_indication *)pvBuffer = *pstAddIndication;
> diff --git a/drivers/staging/bcm/CmHost.h b/drivers/staging/bcm/CmHost.h
> index 4ddfc3d4..86e4d2a 100644
> --- a/drivers/staging/bcm/CmHost.h
> +++ b/drivers/staging/bcm/CmHost.h
> @@ -22,6 +22,7 @@
>  
>  #define DSX_MESSAGE_EXCHANGE_BUFFER        0xBF60AC84 /* This contains the pointer */
>  #define DSX_MESSAGE_EXCHANGE_BUFFER_SIZE   72000      /* 24 K Bytes */
> +#define ntohl_64 __be64_to_cpu
>  
>  struct bcm_add_indication_alt {
>  	u8	u8Type;
> -- 
> 1.7.9.5
> 
> -- 
> You received this message because you are subscribed to the Google Groups "opw-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to opw-kernel+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
ritas1188@gmail.com Oct. 24, 2013, 1:02 p.m. UTC | #2
Hi,

Please find my response inline.


Sorry, this isn't the right patch, for several reasons:
>
> - x86-32 is not the only 32-bit Linux platform.
>

I agree but I just want to state this that this warning was not coming on a
32 bit OS and was present on a 64 bit OS.


> - In most cases, ifdefs should not appear in .c files, especially in the
>   middle of functions; that's a sign that some new function needs to
>   exist that solves the problem generically.
>

I totally agree.

- ntohl, despite the name, operates on 32-bit values, not longs, hence
>   this warning.


In a 32 bit CPU, the " long " data type has a size of 4 bytes.
In a 64 bit CPU (AMD64 for example), data type long has 8 bytes.

I feel this is the cause of the warning.

In the kernel source tree, I find
http://lxr.free-electrons.com/source/include/linux/byteorder/generic.h#L135

#define ntohl(x) ___ntohl(x)
___ntohl(x) __be32_to_cpu(x)
be32_to_cpu __be32_to_cpu

but nothing like this existed for 64 bit implementation which surprised me
that is why I had to come with my own macro

> +#define ntohl_64 __be64_to_cpu

if there is a better way of doing it, please suggest.



> - As suggested by the comment "this can't possibly be right", there's a
>   more fundamental issue here; for some crazy reason the driver is
>   ending up with a byteswapped pointer from the card and needing to
>   access the target of that pointer.
>
> I do not have any idea as to how to figure this out, let alone solving it.

 Rita Sinha

Patch
diff mbox

diff --git a/drivers/staging/bcm/CmHost.c b/drivers/staging/bcm/CmHost.c
index 9765145..8e07813 100644
--- a/drivers/staging/bcm/CmHost.c
+++ b/drivers/staging/bcm/CmHost.c
@@ -1384,7 +1384,11 @@  ULONG StoreCmControlResponseMessage(struct bcm_mini_adapter *Adapter, PVOID pvBu
 	}
 
 	/* this can't possibly be right */
+#ifdef CONFIG_X86_32
 	pstAddIndication->psfAuthorizedSet = (struct bcm_connect_mgr_params *)ntohl((ULONG)pstAddIndication->psfAuthorizedSet);
+#else
+	pstAddIndication->psfAuthorizedSet = (struct bcm_connect_mgr_params *)ntohl_64((ULONG)pstAddIndication->psfAuthorizedSet);
+#endif
 
 	if (pstAddIndicationAlt->u8Type == DSA_REQ) {
 		struct bcm_add_request AddRequest;
@@ -1423,7 +1427,11 @@  ULONG StoreCmControlResponseMessage(struct bcm_mini_adapter *Adapter, PVOID pvBu
 		return 0;
 	}
 
+#ifdef CONFIG_X86_32
 	pstAddIndication->psfAdmittedSet = (struct bcm_connect_mgr_params *)ntohl((ULONG)pstAddIndication->psfAdmittedSet);
+#else
+	pstAddIndication->psfAdmittedSet = (struct bcm_connect_mgr_params *)ntohl_64((ULONG)pstAddIndication->psfAdmittedSet);
+#endif
 
 	/* ACTIVE SET */
 	pstAddIndication->psfActiveSet = (struct bcm_connect_mgr_params *)
@@ -1437,7 +1445,12 @@  ULONG StoreCmControlResponseMessage(struct bcm_mini_adapter *Adapter, PVOID pvBu
 		return 0;
 	}
 
+#ifdef CONFIG_X86_32
+
 	pstAddIndication->psfActiveSet = (struct bcm_connect_mgr_params *)ntohl((ULONG)pstAddIndication->psfActiveSet);
+#else
+	pstAddIndication->psfActiveSet = (struct bcm_connect_mgr_params *)ntohl_64((ULONG)pstAddIndication->psfActiveSet);
+#endif
 
 	(*puBufferLength) = sizeof(struct bcm_add_indication);
 	*(struct bcm_add_indication *)pvBuffer = *pstAddIndication;
diff --git a/drivers/staging/bcm/CmHost.h b/drivers/staging/bcm/CmHost.h
index 4ddfc3d4..86e4d2a 100644
--- a/drivers/staging/bcm/CmHost.h
+++ b/drivers/staging/bcm/CmHost.h
@@ -22,6 +22,7 @@ 
 
 #define DSX_MESSAGE_EXCHANGE_BUFFER        0xBF60AC84 /* This contains the pointer */
 #define DSX_MESSAGE_EXCHANGE_BUFFER_SIZE   72000      /* 24 K Bytes */
+#define ntohl_64 __be64_to_cpu
 
 struct bcm_add_indication_alt {
 	u8	u8Type;