netdev - Re: [PATCHv4 wl-drv-next 0/2] register-field manipulation macros

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]

Date:	Tue, 05 Jul 2016 16:24:18 +0300
From:	Kalle Valo <kvalo@...eaurora.org>
To:	Jakub Kicinski <jakub.kicinski@...ronome.com>
Cc:	linux-wireless@...r.kernel.org, netdev@...r.kernel.org,
	hannes@...essinduktion.org, nbd@....name,
	linux-kernel@...r.kernel.org, David Miller <davem@...emloft.net>,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCHv4 wl-drv-next 0/2] register-field manipulation macros

Jakub Kicinski <jakub.kicinski@...ronome.com> writes:

> This set moves to a global header file macros which I find
> very useful and worth popularising.  The basic problem is
> that since C bitfields are not very dependable accessing
> subfields of registers becomes slightly inconvenient.
> It is nice to have the necessary mask and shift operations
> wrapped in a macro.  It is also nice to have that macro
> compute the shift amount based on the mask automatically.
>
> My implementation follows what Felix Fietkau has done in
> mt76.  Hannes Frederic Sowa suggested more use of standard
> Linux/GCC functions.  Since the RFC I've also added a 
> compile-time check to validate that the value passed to
> setters fits in the mask.
>
> I attempted the use of static inlines instead of macros
> but it makes GCC < 6.0 barf at the BUILD_BUG_ON()s.
> I also noticed that forcing arguments to be u32 for inlines
> makes the compiler use 32bit arithmetic where it could
> get away with 64bit before (on 64bit machines, obviously).
> That's a potential performance concern but probably not
> a very practical one today.  Apart from looking "cleaner"
> static inlines would have the advantage that we could #undef
> the auxiliary macros at the end of the header.

This information about various compiler problems is good to have in the
commit log as the cover letter itself is not stored in the git log.

> v3:
> Build bot caught a build failure with -Os set.  AFAICT gcc
> did not handle temporary variable I put in the macro
> expression too well.  I work around that by defining
> __BUILD_BUG_ON_NOT_POWER_OF_2 and using it instead of
> BUILD_BUG_ON(!tmp || is_power_of_2(tmp)).
>
> Please review and advise on improvements.
>
> If accepted I think would be best to push this through
> Kalle's tree, since the only existing user is in
> drivers/net/wireless/.

I like it. But I feel much more comfortable to take these if I get an
ack from someone like Dave Miller or Andrew Morton (CCed).

Full patches here:

https://patchwork.kernel.org/patch/9214129/
https://patchwork.kernel.org/patch/9214135/

-- 
Kalle Valo