linux-kernel - Re: [RFC PATCH] intel: Use upper_32_bits and lower_32

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] [day] [month] [year] [list]

Message-ID: <1483968790.2106.10.camel@perches.com>
Date:   Mon, 09 Jan 2017 05:33:10 -0800
From:   Joe Perches <joe@...ches.com>
To:     David Laight <David.Laight@...LAB.COM>,
        Jeff Kirsher <jeffrey.t.kirsher@...el.com>
Cc:     Julia Lawall <julia.lawall@...6.fr>,
        "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH] intel: Use upper_32_bits and lower_32_bits

On Mon, 2017-01-09 at 12:55 +0000, David Laight wrote:
> From: Joe Perches
> > Sent: 07 January 2017 18:33
> > Shifting and masking various types can be made a bit
> > simpler to read by using the available kernel macros.
> 
> ...
> > -		ew32(TDBAH, (tdba >> 32));
> > -		ew32(TDBAL, (tdba & 0x00000000ffffffffULL));
> > +		ew32(TDBAH, upper_32_bits(tdba));
> > +		ew32(TDBAL, lower_32_bits(tdba));
> 
> Personally I find the original code easier to understand
> since I don't have to look up another silly macro.

It's already a pretty common usage and I believe
the naming is fairly obvious.  Also you don't have
to count the "f" characters to see how many bits
are being used.  After about 6 consecutive chars,
it can be error prone.

The leading zeros?  ugh.  The ULL too.

$ git grep -w -E "upper_32_bits|lower_32_bits" | wc -l
1569

> I'd normally not even explicitly mask the low bits
> relying on the implicit truncation of the assignment.

Relying on implicit behaviors can be noisy when
compilers complain about implicit conversions and
truncations.

> At least modern compilers aren't stupid enough to add two
> 'mask with 0xff' instructions for:
> 	*uchar_ptr = (unsigned char)(foo & 0xff);

I agree it's visual noise.