linux-kernel - Re: [PATCH 4/5 ver2] rt2x00: Compiler warning unmasked by fix of BUILD_BUG

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 1 Sep 2008 18:34:45 +0200
From:	Ivo van Doorn <ivdoorn@...il.com>
To:	Boaz Harrosh <bharrosh@...asas.com>
Cc:	Ingo Molnar <mingo@...e.hu>, Rusty Russell <rusty@...tcorp.com.au>,
	"David S. Miller" <davem@...emloft.net>,
	"John W. Linville" <linville@...driver.com>,
	Alexey Dobriyan <adobriyan@...il.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Theodore Tso <tytso@....edu>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Jan Beulich <jbeulich@...ell.com>,
	linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 4/5 ver2] rt2x00: Compiler warning unmasked by fix of BUILD_BUG_ON

On Monday 01 September 2008, Boaz Harrosh wrote:
> Ivo Van Doorn wrote:
> > On Mon, Sep 1, 2008 at 3:44 PM, Boaz Harrosh <bharrosh@...asas.com> wrote:
> >> A "Set" of a sign-bit in an "&" operation causes a compiler warning.
> >> Make calculations unsigned.
> >>
> >> [ The warning was masked by the use of (void)() cast in the old
> >>  BUILD_BUG_ON() ]
> >>
> >> Signed-off-by: Boaz Harrosh <bharrosh@...asas.com>
> >> TO: Ivo van Doorn <IvDoorn@...il.com>
> >> TO: John W. Linville <linville@...driver.com>
> >> CC: Ingo Molnar <mingo@...e.hu>
> >> CC: Rusty Russell <rusty@...tcorp.com.au>
> >> ---
> >>  drivers/net/wireless/rt2x00/rt2x00reg.h |    4 ++--
> >>  1 files changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/net/wireless/rt2x00/rt2x00reg.h b/drivers/net/wireless/rt2x00/rt2x00reg.h
> >> index 7e88ce5..e71b793 100644
> >> --- a/drivers/net/wireless/rt2x00/rt2x00reg.h
> >> +++ b/drivers/net/wireless/rt2x00/rt2x00reg.h
> >> @@ -136,8 +136,8 @@ struct rt2x00_field32 {
> >>  */
> >>  #define is_power_of_two(x)     ( !((x) & ((x)-1)) )
> >>  #define low_bit_mask(x)                ( ((x)-1) & ~(x) )
> >> -#define is_valid_mask(x)       is_power_of_two(1 + (x) + low_bit_mask(x))
> >> -
> >> +#define is_valid_mask(x)       is_power_of_two(1 + (x) + \
> >> +                                       low_bit_mask((unsigned long)x))
> > 
> > I know I typed it wrong, but you are missing the unsigned long cast for the
> > is_power_of_two argument here (Which could also be done in the
> > is_valid_mask() definition).
> > 
> 
> I thought you suggested that on purpose. Since at the end it is all
> one expression, the compiler propagates the cast to all participants.
> 
> Do you want that I send a fix for readability's sake?

Yes, thanks.

> >>  /*
> >>  * Macro's to find first set bit in a variable.
> >>  * These macro's behaves the same as the __ffs() function with
> >> --
> >> 1.5.6.rc1.5.gadf6
> >>
> 
> Is below what you mean? but if so then perhaps my original one is
> clearer. Note that it compiles and works as is.

Below patch is good. Your original one would have had the same comment
as the original one, where the cast was only done on 1 x instead of both usages.

I think the below version is the most clear solution. :)

Ivo

> ---
> diff --git a/drivers/net/wireless/rt2x00/rt2x00reg.h b/drivers/net/wireless/rt2x00/rt2x00reg.h
> index 7e88ce5..e71b793 100644
> --- a/drivers/net/wireless/rt2x00/rt2x00reg.h
> +++ b/drivers/net/wireless/rt2x00/rt2x00reg.h
> @@ -136,8 +136,8 @@ struct rt2x00_field32 {
>   */
>  #define is_power_of_two(x)	( !((x) & ((x)-1)) )
>  #define low_bit_mask(x)		( ((x)-1) & ~(x) )
> -#define is_valid_mask(x)	is_power_of_two(1 + (x) + low_bit_mask(x))
> -
> +#define is_valid_mask(x)	is_power_of_two(1LU + (unsigned long)(x) + \
> +					low_bit_mask((unsigned long)x))
>  /*
>   * Macro's to find first set bit in a variable.
>   * These macro's behaves the same as the __ffs() function with
> -- 1.5.6.rc1.5.gadf6 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ 
> 
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/