linux-kernel - Re: [RFC v2 1/8] video: tegra: Add nvhost driver

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:	Tue, 4 Dec 2012 22:31:53 +0100
From:	Thierry Reding <thierry.reding@...onic-design.de>
To:	Stephen Warren <swarren@...dotorg.org>
Cc:	Terje Bergström <tbergstrom@...dia.com>,
	"linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
	"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RFC v2 1/8] video: tegra: Add nvhost driver

On Mon, Dec 03, 2012 at 12:23:32PM -0700, Stephen Warren wrote:
> On 12/01/2012 07:58 AM, Thierry Reding wrote:
> > On Sat, Dec 01, 2012 at 01:31:02PM +0200, Terje Bergström wrote:
> ...
> >> I was thinking of definitions like this:
> >> 
> >> static inline u32 host1x_sync_cfpeek_ctrl_cfpeek_addr_f(u32 v) { 
> >> return (v & 0x1ff) << 0; }
> >> 
> >> versus
> >> 
> >> #define host1x_sync_cfpeek_ctrl_cfpeek_addr_f(v) ((v) >> 16) &
> >> 0x3ff
> >> 
> >> Both of these produce the same machine code and have same usage,
> >> but the latter has type checking and code coverage analysis and
> >> the former is (in my eyes) clearer. In both of these cases the
> >> usage is like this:
> >> 
> >> writel(host1x_sync_cfpeek_ctrl_cfpeek_ena_f(1) |
> >> host1x_sync_cfpeek_ctrl_cfpeek_channr_f(chid) |
> >> host1x_sync_cfpeek_ctrl_cfpeek_addr_f(rd_ptr), m->sync_aperture +
> >> host1x_sync_cfpeek_ctrl_r());
> > 
> > Again there's no precedent for doing this with static inline
> > functions. You can do the same with macros. Type checking isn't an
> > issue in these cases since we're talking about bitfields for which
> > no proper type exists.
> 
> I suspect the inline functions could encode signed-vs-unsigned fields,
> perhaps catch u8 variables when they should have been u32, etc.?

I don't see how this would be relevant here. These definitions are only
used in the driver internally and not a public API, therefore none of
those checks should really be needed. If somebody writes code for this
driver and uses the register definitions, they better know what they're
doing. Or at least wrong usage should be filtered out through review.

In my opinion the consistency with how other drivers are written far
outweigh the benefits provided by inline functions. That said, I'm out
of arguments and I don't have a final say anyway, so if it is decided
to stick with inline functions I can find a way to live with them.

Thierry

Content of type "application/pgp-signature" skipped