[PATCH] bcm/CmHost.c: Fix noisy compile warnings

[PATCH] bcm/CmHost.c: Fix noisy compile warnings

[Jeff Kirsher]

The Beceem WIMAX was generating compile warnings on 64bit machines,
which were:

drivers/staging/bcm/CmHost.c: In function ‘StoreCmControlResponseMessage’:
drivers/staging/bcm/CmHost.c:1503:3: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
   (struct bcm_connect_mgr_params *) ntohl(
   ^
drivers/staging/bcm/CmHost.c:1546:3: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
   (struct bcm_connect_mgr_params *) ntohl(
   ^
drivers/staging/bcm/CmHost.c:1564:3: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
   (struct bcm_connect_mgr_params *) ntohl(

This resolves the issue by generating 64bit friendly code.

Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/staging/bcm/CmHost.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/staging/bcm/CmHost.c b/drivers/staging/bcm/CmHost.c
index adca0ce..29fa05d 100644
--- a/drivers/staging/bcm/CmHost.c
+++ b/drivers/staging/bcm/CmHost.c
@@ -1499,9 +1499,15 @@ ULONG StoreCmControlResponseMessage(struct bcm_mini_adapter *Adapter,
 	}
 
 	/* this can't possibly be right */
+#ifdef CONFIG_32BIT
 	pstAddIndication->psfAuthorizedSet =
 		(struct bcm_connect_mgr_params *) ntohl(
 				(ULONG)pstAddIndication->psfAuthorizedSet);
+#else
+	pstAddIndication->psfAuthorizedSet =
+		(struct bcm_connect_mgr_params *)(u64)ntohl(
+				(ULONG)pstAddIndication->psfAuthorizedSet);
+#endif
 
 	if (pstAddIndicationAlt->u8Type == DSA_REQ) {
 		struct bcm_add_request AddRequest;
@@ -1542,9 +1548,15 @@ ULONG StoreCmControlResponseMessage(struct bcm_mini_adapter *Adapter,
 		return 0;
 	}
 
+#ifdef CONFIG_32BIT
 	pstAddIndication->psfAdmittedSet =
 		(struct bcm_connect_mgr_params *) ntohl(
 				(ULONG) pstAddIndication->psfAdmittedSet);
+#else
+	pstAddIndication->psfAdmittedSet =
+		(struct bcm_connect_mgr_params *)(u64)ntohl(
+				(ULONG) pstAddIndication->psfAdmittedSet);
+#endif
 
 	/* ACTIVE SET */
 	pstAddIndication->psfActiveSet = (struct bcm_connect_mgr_params *)
@@ -1560,9 +1572,15 @@ ULONG StoreCmControlResponseMessage(struct bcm_mini_adapter *Adapter,
 		return 0;
 	}
 
+#ifdef CONFIG_32BIT
 	pstAddIndication->psfActiveSet =
 		(struct bcm_connect_mgr_params *) ntohl(
 				(ULONG)pstAddIndication->psfActiveSet);
+#else
+	pstAddIndication->psfActiveSet =
+		(struct bcm_connect_mgr_params *)(u64)ntohl(
+				(ULONG)pstAddIndication->psfActiveSet);
+#endif
 
 	(*puBufferLength) = sizeof(struct bcm_add_indication);
 	*(struct bcm_add_indication *)pvBuffer = *pstAddIndication;
-- 
1.9.3

Re: [PATCH] bcm/CmHost.c: Fix noisy compile warnings

[Joe Perches]

On Wed, 2014-10-15 at 05:26 -0700, Jeff Kirsher wrote:
> The Beceem WIMAX was generating compile warnings on 64bit machines,
> which were:
> 
> drivers/staging/bcm/CmHost.c: In function ‘StoreCmControlResponseMessage’:
> drivers/staging/bcm/CmHost.c:1503:3: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
>    (struct bcm_connect_mgr_params *) ntohl(
>    ^
[]
> diff --git a/drivers/staging/bcm/CmHost.c b/drivers/staging/bcm/CmHost.c
[]
> @@ -1499,9 +1499,15 @@ ULONG StoreCmControlResponseMessage(struct bcm_mini_adapter *Adapter,
>  	}
>  
>  	/* this can't possibly be right */
> +#ifdef CONFIG_32BIT
>  	pstAddIndication->psfAuthorizedSet =
>  		(struct bcm_connect_mgr_params *) ntohl(
>  				(ULONG)pstAddIndication->psfAuthorizedSet);
> +#else
> +	pstAddIndication->psfAuthorizedSet =
> +		(struct bcm_connect_mgr_params *)(u64)ntohl(
> +				(ULONG)pstAddIndication->psfAuthorizedSet);
> +#endif

no ifdefs necessary

(void *)(unsigned long)

Re: [PATCH] bcm/CmHost.c: Fix noisy compile warnings

[Dan Carpenter]

On Wed, Oct 15, 2014 at 05:26:39AM -0700, Jeff Kirsher wrote:
> The Beceem WIMAX was generating compile warnings on 64bit machines,
> which were:
> 
> drivers/staging/bcm/CmHost.c: In function ‘StoreCmControlResponseMessage’:
> drivers/staging/bcm/CmHost.c:1503:3: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
>    (struct bcm_connect_mgr_params *) ntohl(
>    ^
> drivers/staging/bcm/CmHost.c:1546:3: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
>    (struct bcm_connect_mgr_params *) ntohl(
>    ^
> drivers/staging/bcm/CmHost.c:1564:3: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
>    (struct bcm_connect_mgr_params *) ntohl(
> 
> This resolves the issue by generating 64bit friendly code.
> 

As far as I can tell this isn't a bugfix, it just hides a real 64 bit
bug.  Let's leave the bug how it is so it's easy to see from a long way
away.

regards,
dan carpenter

Re: [PATCH] bcm/CmHost.c: Fix noisy compile warnings

[Dan Carpenter]

Or we could fix it but add a multi-line comment with lots of capital
letters and exclamation marks.  I guess we would need to make a function
since this cast is in several places.

void *silence_buggy_casting(u32 pointer)
{
	/*
	 * DANGER! DANGER! DANGER! DANGER! DANGER! DANGER! DANGER!
	 * FIXME!!!  We know this cast is totally buggy.  The BCM driver
	 * doesn't work on 64 bits.  But no one knows how to make this
	 * work.  Oh well.  Let's silence the GCC warning.
	 * DANGER! DANGER! DANGER! DANGER! DANGER! DANGER! DANGER!
	 *
	 */
	return (void *)(long)pointer;
}

Something really ugly and gnarly like that would be hard to ignore.

regards,
dan carpenter

Re: [PATCH] bcm/CmHost.c: Fix noisy compile warnings

[Jeff Kirsher]

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

On Wed, 2014-10-15 at 15:59 +0300, Dan Carpenter wrote:
> Or we could fix it but add a multi-line comment with lots of capital
> letters and exclamation marks.  I guess we would need to make a
> function
> since this cast is in several places.
> 
> void *silence_buggy_casting(u32 pointer)
> {
>         /*
>          * DANGER! DANGER! DANGER! DANGER! DANGER! DANGER! DANGER!
>          * FIXME!!!  We know this cast is totally buggy.  The BCM
> driver
>          * doesn't work on 64 bits.  But no one knows how to make this
>          * work.  Oh well.  Let's silence the GCC warning.
>          * DANGER! DANGER! DANGER! DANGER! DANGER! DANGER! DANGER!
>          *
>          */
>         return (void *)(long)pointer;
> }
> 
> Something really ugly and gnarly like that would be hard to ignore.

I thought I remember Greg saying something about getting rid of this
driver anyway, but I could be wrong.  If Greg decides to keep this
driver around, then I think we should something like your suggestion
above.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] bcm/CmHost.c: Fix noisy compile warnings

[Joe Perches]

On Wed, 2014-10-15 at 06:03 -0700, Jeff Kirsher wrote:
[]
> I thought I remember Greg saying something about getting rid of this
> driver anyway, but I could be wrong.  If Greg decides to keep this
> driver around, then I think we should something like your suggestion
> above.

Or maybe just make the Kconfig depend on X86_32
---
 drivers/staging/bcm/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/bcm/Kconfig b/drivers/staging/bcm/Kconfig
index 8acf4b2..fa5a3a4 100644
--- a/drivers/staging/bcm/Kconfig
+++ b/drivers/staging/bcm/Kconfig
@@ -1,6 +1,6 @@
 config BCM_WIMAX
        tristate "Beceem BCS200/BCS220-3 and BCSM250 wimax support"
-       depends on USB && NET
+       depends on USB && NET && X86_32
        help
          This is an experimental driver for the Beceem WIMAX chipset used
 	 by Sprint 4G.

Re: [PATCH] bcm/CmHost.c: Fix noisy compile warnings

[Jeff Kirsher]

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

On Wed, 2014-10-15 at 09:11 -0700, Joe Perches wrote:
> On Wed, 2014-10-15 at 06:03 -0700, Jeff Kirsher wrote:
> []
> > I thought I remember Greg saying something about getting rid of this
> > driver anyway, but I could be wrong.  If Greg decides to keep this
> > driver around, then I think we should something like your suggestion
> > above.
> 
> Or maybe just make the Kconfig depend on X86_32

I like the idea, but won't this exclude other 32 bit systems like MIPS
and I am sure there might be others.  I could assume that Beceem WIMAX
was intended for x86 arch's that are 32 bit only, which may be a safe
bet.  The only thing we know for sure is that is broken on 64bit.

> ---
>  drivers/staging/bcm/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/bcm/Kconfig b/drivers/staging/bcm/Kconfig
> index 8acf4b2..fa5a3a4 100644
> --- a/drivers/staging/bcm/Kconfig
> +++ b/drivers/staging/bcm/Kconfig
> @@ -1,6 +1,6 @@
>  config BCM_WIMAX
>         tristate "Beceem BCS200/BCS220-3 and BCSM250 wimax support"
> -       depends on USB && NET
> +       depends on USB && NET && X86_32
>         help
>           This is an experimental driver for the Beceem WIMAX chipset used
>  	 by Sprint 4G.
> 
> 



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] bcm/CmHost.c: Fix noisy compile warnings

[Fabio Estevam]

On Wed, Oct 15, 2014 at 2:42 PM, Jeff Kirsher
<jeffrey.t.kirsher@intel.com> wrote:

> I like the idea, but won't this exclude other 32 bit systems like MIPS
> and I am sure there might be others.  I could assume that Beceem WIMAX
> was intended for x86 arch's that are 32 bit only, which may be a safe
> bet.  The only thing we know for sure is that is broken on 64bit.

What about this?

diff --git a/drivers/staging/bcm/Kconfig b/drivers/staging/bcm/Kconfig
index 8acf4b2..fa5a3a4 100644
--- a/drivers/staging/bcm/Kconfig
+++ b/drivers/staging/bcm/Kconfig
@@ -1,6 +1,6 @@
 config BCM_WIMAX
        tristate "Beceem BCS200/BCS220-3 and BCSM250 wimax support"
-       depends on USB && NET
+       depends on USB && NET && !64BIT
        help
          This is an experimental driver for the Beceem WIMAX chipset used
         by Sprint 4G.

Re: [PATCH] bcm/CmHost.c: Fix noisy compile warnings

[Jeff Kirsher]

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

On Wed, 2014-10-15 at 15:24 -0300, Fabio Estevam wrote:
> On Wed, Oct 15, 2014 at 2:42 PM, Jeff Kirsher
> <jeffrey.t.kirsher@intel.com> wrote:
> 
> > I like the idea, but won't this exclude other 32 bit systems like MIPS
> > and I am sure there might be others.  I could assume that Beceem WIMAX
> > was intended for x86 arch's that are 32 bit only, which may be a safe
> > bet.  The only thing we know for sure is that is broken on 64bit.
> 
> What about this?

That works for me.  I am putting together a v2 patch now, since you came
up with the change, I will put you as the author Fabio, ok?

> 
> diff --git a/drivers/staging/bcm/Kconfig b/drivers/staging/bcm/Kconfig
> index 8acf4b2..fa5a3a4 100644
> --- a/drivers/staging/bcm/Kconfig
> +++ b/drivers/staging/bcm/Kconfig
> @@ -1,6 +1,6 @@
>  config BCM_WIMAX
>         tristate "Beceem BCS200/BCS220-3 and BCSM250 wimax support"
> -       depends on USB && NET
> +       depends on USB && NET && !64BIT
>         help
>           This is an experimental driver for the Beceem WIMAX chipset used
>          by Sprint 4G.



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] bcm/CmHost.c: Fix noisy compile warnings

[Joe Perches]

On Wed, 2014-10-15 at 11:54 -0700, Jeff Kirsher wrote:
> On Wed, 2014-10-15 at 15:24 -0300, Fabio Estevam wrote:
> > On Wed, Oct 15, 2014 at 2:42 PM, Jeff Kirsher
> > <jeffrey.t.kirsher@intel.com> wrote:
> > 
> > > I like the idea, but won't this exclude other 32 bit systems like MIPS
> > > and I am sure there might be others.  I could assume that Beceem WIMAX
> > > was intended for x86 arch's that are 32 bit only, which may be a safe
> > > bet.  The only thing we know for sure is that is broken on 64bit.
> > 
> > What about this?
> 
> That works for me.  I am putting together a v2 patch now, since you came
> up with the change, I will put you as the author Fabio, ok?

Good idea.

Re: [PATCH] bcm/CmHost.c: Fix noisy compile warnings

[Fabio Estevam]

On Wed, Oct 15, 2014 at 3:54 PM, Jeff Kirsher
<jeffrey.t.kirsher@intel.com> wrote:
> On Wed, 2014-10-15 at 15:24 -0300, Fabio Estevam wrote:
>> On Wed, Oct 15, 2014 at 2:42 PM, Jeff Kirsher
>> <jeffrey.t.kirsher@intel.com> wrote:
>>
>> > I like the idea, but won't this exclude other 32 bit systems like MIPS
>> > and I am sure there might be others.  I could assume that Beceem WIMAX
>> > was intended for x86 arch's that are 32 bit only, which may be a safe
>> > bet.  The only thing we know for sure is that is broken on 64bit.
>>
>> What about this?
>
> That works for me.  I am putting together a v2 patch now, since you came
> up with the change, I will put you as the author Fabio, ok?

That's OK, Jeff.

Please use: Fabio Estevam <fabio.estevam@freescale.com>

Thanks

Re: [PATCH] bcm/CmHost.c: Fix noisy compile warnings

[Dan Carpenter]

On Wed, Oct 15, 2014 at 09:11:36AM -0700, Joe Perches wrote:
> On Wed, 2014-10-15 at 06:03 -0700, Jeff Kirsher wrote:
> []
> > I thought I remember Greg saying something about getting rid of this
> > driver anyway, but I could be wrong.  If Greg decides to keep this
> > driver around, then I think we should something like your suggestion
> > above.
> 
> Or maybe just make the Kconfig depend on X86_32

What I like about your patches is that they are pure theoretical work
and I don't have to think about them like regular proper patches with a
signed off by etc.  All the fun, none of the responsibility.

>  config BCM_WIMAX
>         tristate "Beceem BCS200/BCS220-3 and BCSM250 wimax support"
> -       depends on USB && NET
> +       depends on USB && NET && X86_32

And also COMPILE_TEST.

regards,
dan carpenter

Re: [PATCH] bcm/CmHost.c: Fix noisy compile warnings

[Joe Perches]

On Wed, 2014-10-15 at 22:53 +0300, Dan Carpenter wrote:
> On Wed, Oct 15, 2014 at 09:11:36AM -0700, Joe Perches wrote:
> > On Wed, 2014-10-15 at 06:03 -0700, Jeff Kirsher wrote:
> > []
> > > I thought I remember Greg saying something about getting rid of this
> > > driver anyway, but I could be wrong.  If Greg decides to keep this
> > > driver around, then I think we should something like your suggestion
> > > above.
> > 
> > Or maybe just make the Kconfig depend on X86_32
> 
> What I like about your patches is that they are pure theoretical work
> and I don't have to think about them like regular proper patches with a
> signed off by etc.  All the fun, none of the responsibility.

I've sent a lot of signed-off patches.
These are simple suggestions.

I think COMPILE_TEST isn't useful here.

I doubt very much beceem has been tested
on anything other than an x86_32 board.

I once spent a few hours fixing up a bunch
of defects in this directory a few years ago.

Now I will not submit any more patches for
cmhost or anything else in that directory.

Re: [PATCH] bcm/CmHost.c: Fix noisy compile warnings

[Matthias Beyer]

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

On 15-10-2014 06:03:33, Jeff Kirsher wrote:
> 
> I thought I remember Greg saying something about getting rid of this
> driver anyway, but I could be wrong.  If Greg decides to keep this
> driver around, then I think we should something like your suggestion
> above.

Let me throw in my comment here: Yes, Greg stated in several places
that this whole driver should be removed. I was about to ask when this
will happen, so let me add a suggestion:

    Lets delete this _now_!!1!1!

If you care, I can search the statements where Greg stated that this
should be removed, but I guess this some effort... I can barely
remember that one time was on the linuxnewbies ML and one or two times
here.

-- 
Mit freundlichen Grüßen,
Kind regards,
Matthias Beyer

Proudly sent with mutt.
Happily signed with gnupg.

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] bcm/CmHost.c: Fix noisy compile warnings

[Jeff Kirsher]

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

On Wed, 2014-10-15 at 23:41 +0200, Matthias Beyer wrote:
> On 15-10-2014 06:03:33, Jeff Kirsher wrote:
> > 
> > I thought I remember Greg saying something about getting rid of this
> > driver anyway, but I could be wrong.  If Greg decides to keep this
> > driver around, then I think we should something like your suggestion
> > above.
> 
> Let me throw in my comment here: Yes, Greg stated in several places
> that this whole driver should be removed. I was about to ask when this
> will happen, so let me add a suggestion:
> 
>     Lets delete this _now_!!1!1!
> 
> If you care, I can search the statements where Greg stated that this
> should be removed, but I guess this some effort... I can barely
> remember that one time was on the linuxnewbies ML and one or two times
> here.

No need to dig up where and when Greg stated that it should be removed,
I was pretty sure he mentioned it on several lists.  I think the most
recent was on the linuxnewbies ML.

I will just go ahead and create a patch to remove the driver.  Greg can
then decide to either accept the patch to fix the Kconfig or accept the
patch to remove the driver.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] bcm/CmHost.c: Fix noisy compile warnings

[Matthias Beyer]

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

On 15-10-2014 15:19:44, Jeff Kirsher wrote:
> I will just go ahead and create a patch to remove the driver.  Greg can
> then decide to either accept the patch to fix the Kconfig or accept the
> patch to remove the driver.

Don't forget the maintainers file :-)

-- 
Mit freundlichen Grüßen,
Kind regards,
Matthias Beyer

Proudly sent with mutt.
Happily signed with gnupg.

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] bcm/CmHost.c: Fix noisy compile warnings

[Jeff Kirsher]

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

On Thu, 2014-10-16 at 00:24 +0200, Matthias Beyer wrote:
> On 15-10-2014 15:19:44, Jeff Kirsher wrote:
> > I will just go ahead and create a patch to remove the driver.  Greg
> can
> > then decide to either accept the patch to fix the Kconfig or accept
> the
> > patch to remove the driver.
> 
> Don't forget the maintainers file :-)

Yep, got it.  Thanks for the reminder.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] bcm/CmHost.c: Fix noisy compile warnings

[Dan Carpenter]

On Wed, Oct 15, 2014 at 01:03:49PM -0700, Joe Perches wrote:
> On Wed, 2014-10-15 at 22:53 +0300, Dan Carpenter wrote:
> > On Wed, Oct 15, 2014 at 09:11:36AM -0700, Joe Perches wrote:
> > > On Wed, 2014-10-15 at 06:03 -0700, Jeff Kirsher wrote:
> > > []
> > > > I thought I remember Greg saying something about getting rid of this
> > > > driver anyway, but I could be wrong.  If Greg decides to keep this
> > > > driver around, then I think we should something like your suggestion
> > > > above.
> > > 
> > > Or maybe just make the Kconfig depend on X86_32
> > 
> > What I like about your patches is that they are pure theoretical work
> > and I don't have to think about them like regular proper patches with a
> > signed off by etc.  All the fun, none of the responsibility.
> 
> I've sent a lot of signed-off patches.
> These are simple suggestions.
> 
> I think COMPILE_TEST isn't useful here.

I don't understand...  COMPILE_TEST means runing a static checker on
the driver.  Why would we not want to do QA?

regards,
dan carpenter

Re: [PATCH] bcm/CmHost.c: Fix noisy compile warnings

[Jeff Kirsher]

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

On Thu, 2014-10-16 at 10:48 +0300, Dan Carpenter wrote:
> On Wed, Oct 15, 2014 at 01:03:49PM -0700, Joe Perches wrote:
> > On Wed, 2014-10-15 at 22:53 +0300, Dan Carpenter wrote:
> > > On Wed, Oct 15, 2014 at 09:11:36AM -0700, Joe Perches wrote:
> > > > On Wed, 2014-10-15 at 06:03 -0700, Jeff Kirsher wrote:
> > > > []
> > > > > I thought I remember Greg saying something about getting rid of this
> > > > > driver anyway, but I could be wrong.  If Greg decides to keep this
> > > > > driver around, then I think we should something like your suggestion
> > > > > above.
> > > > 
> > > > Or maybe just make the Kconfig depend on X86_32
> > > 
> > > What I like about your patches is that they are pure theoretical work
> > > and I don't have to think about them like regular proper patches with a
> > > signed off by etc.  All the fun, none of the responsibility.
> > 
> > I've sent a lot of signed-off patches.
> > These are simple suggestions.
> > 
> > I think COMPILE_TEST isn't useful here.
> 
> I don't understand...  COMPILE_TEST means runing a static checker on
> the driver.  Why would we not want to do QA?

I think we can drop this patch altogether since I have submitted another
patch to remove this driver from the kernel completely, since Matthias
Beyer (and Greg KH in earlier discussions) suggested we do so.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]