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

[permalink] [raw]

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

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)

2014-10-15 12:34:43

[permalink] [raw]

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

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

2014-10-15 12:59:50

[permalink] [raw]

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

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

2014-10-15 13:05:05

[permalink] [raw]

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

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.

Attachments:

signature.asc (819.00 B)
This is a digitally signed message part

2014-10-15 16:11:41

[permalink] [raw]

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

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.

2014-10-15 17:45:01

[permalink] [raw]

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

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.
>
>

Attachments:

signature.asc (819.00 B)
This is a digitally signed message part

2014-10-15 18:24:22

by Fabio Estevam

[permalink] [raw]

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

On Wed, Oct 15, 2014 at 2:42 PM, Jeff Kirsher
<[email protected]> 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.

2014-10-15 18:54:18

[permalink] [raw]

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

On Wed, 2014-10-15 at 15:24 -0300, Fabio Estevam wrote:
> On Wed, Oct 15, 2014 at 2:42 PM, Jeff Kirsher
> <[email protected]> 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.

Attachments:

signature.asc (819.00 B)
This is a digitally signed message part

2014-10-15 18:56:28

by Fabio Estevam

[permalink] [raw]

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

On Wed, Oct 15, 2014 at 3:54 PM, Jeff Kirsher
<[email protected]> wrote:
> On Wed, 2014-10-15 at 15:24 -0300, Fabio Estevam wrote:
>> On Wed, Oct 15, 2014 at 2:42 PM, Jeff Kirsher
>> <[email protected]> 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 <[email protected]>

Thanks

2014-10-15 18:56:26

[permalink] [raw]

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

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
> > <[email protected]> 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.

2014-10-15 19:53:57

[permalink] [raw]

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

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

2014-10-15 20:04:50

[permalink] [raw]

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

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.

2014-10-15 21:42:00

by Matthias Beyer

[permalink] [raw]

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

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.

Attachments:

(No filename) (804.00 B)
(No filename) (819.00 B)
Download all attachments

2014-10-15 22:20:08

[permalink] [raw]

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

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.

Attachments:

signature.asc (819.00 B)
This is a digitally signed message part

2014-10-15 22:24:55

by Matthias Beyer

[permalink] [raw]

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

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.

Attachments:

(No filename) (372.00 B)
(No filename) (819.00 B)
Download all attachments

2014-10-15 22:42:37

[permalink] [raw]

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

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.

Attachments:

signature.asc (819.00 B)
This is a digitally signed message part

2014-10-16 07:49:12

[permalink] [raw]

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

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

2014-10-16 07:55:28