linux-kernel - RE: [PATCH V2 1/3] NTB: Add AMD PCI-Express NTB 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]

Message-ID: <000d01d14a2a$c0879d50$4196d7f0$@emc.com>
Date:	Fri, 8 Jan 2016 10:39:24 -0500
From:	"Allen Hubbe" <Allen.Hubbe@....com>
To:	"'Jon Mason'" <jdmason@...zu.us>,
	"'Yu, Xiangliang'" <Xiangliang.Yu@....com>
Cc:	"'Dave Jiang'" <dave.jiang@...el.com>,
	<linux-ntb@...glegroups.com>,
	"'linux-kernel'" <linux-kernel@...r.kernel.org>,
	"'SPG_Linux_Kernel'" <SPG_Linux_Kernel@....com>
Subject: RE: [PATCH V2 1/3] NTB: Add AMD PCI-Express NTB driver

From: Jon Mason <jdmason@...zu.us>
> On Wed, Jan 6, 2016 at 9:50 PM, Yu, Xiangliang <Xiangliang.Yu@....com>
> wrote:
> >> > +#define NTB_READ_REG(base, r) (ioread32(base + AMD_ ## r ##
> >> _OFFSET))
> >> > +#define NTB_WRITE_REG(base, val, r) (iowrite32(val, base +     \
> >> > +                                               AMD_ ## r ##
> _OFFSET))
> >> > +#define NTB_READ_OFFSET(base, r, of) (ioread32(base + of +
> \
> >> > +                                               AMD_ ## r ##
> _OFFSET))
> >> > +#define NTB_WRITE_OFFSET(base, val, r, of) (iowrite32(val, base +
> \
> >> > +                                               of + AMD_ ## r ##
> _OFFSET))
> >>
> >> Please do not use marcos to hide ioread/iowrite.  Call iorwad/iowrite
> directly.
> >
> > I don't see any wrong to hide ioread/iowrite, and I think the macros
> can make code readable and easy to maintain.
> 
> I disagree.  It is an unnecessary layer and can add to confusion.
> Please make the change.

I don't like AMD_##r##_OFFSET in these macros.  It hides the use of a globally named constant like AMD_FOO_OFFSET, since one would read only FOO in the code.  It makes cross referencing difficult, since the reader needs to know FOO is really AMD_FOO_OFFSET.  This would defeat automatic cross referencing like cscope and lxr.

#define AMD_FOO_OFFSET 0xc0ff33
vs
NTB_READ_OFFSET(dev->foo_base, FOO, offset_in_foo)
// Where is FOO defined?

This macro would have at least been better written without ##; so cross referencing would still work.

NTB_READ_OFFSET(dev->foo_base, FOO, offset_in_foo)
vs
NTB_READ_OFFSET(dev->foo_base, AMD_FOO_OFFSET, offset_in_foo)
// AMD_FOO_OFFSET is 0xcoff33 (obviously)

But without ##, the macro is just the addition of its parameters.  Change the commas to addition, and the macro to ioread, and you'll see there is no benefit for having this macro any more.

NTB_READ_OFFSET(dev->foo_base, AMD_FOO_OFFSET, offset_in_foo)
vs
ioread32(dev->foo_base + AMD_FOO_OFFSET + offset_in_foo)

I second Jon's opinion.  Please make the change.  This would be better as simply ioread/write in the code.

Allen