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: <BLUPR12MB0420841C92C2D403A12EA4CFEBF50@BLUPR12MB0420.namprd12.prod.outlook.com>
Date:	Thu, 7 Jan 2016 02:50:45 +0000
From:	"Yu, Xiangliang" <Xiangliang.Yu@....com>
To:	Jon Mason <jdmason@...zu.us>
CC:	Dave Jiang <dave.jiang@...el.com>,
	"Hubbe, Allen" <Allen.Hubbe@....com>,
	"linux-ntb@...glegroups.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

> > +
> > +#define        PCI_DEVICE_ID_AMD_NTB   0x145B
> 
> This looks like a tab and not a space

I'll update it.

> 
> > +#define AMD_LINK_HB_TIMEOUT    msecs_to_jiffies(1000)
> > +#define AMD_LINK_STATUS_OFFSET 0x68
> > +#define NTB_LIN_STA_ACTIVE_BIT 0x00000002
> > +#define NTB_LNK_STA_SPEED_MASK 0x000F0000
> > +#define NTB_LNK_STA_WIDTH_MASK 0x03F00000
> > +#define NTB_LNK_STA_ACTIVE(x)  (!!((x) & NTB_LIN_STA_ACTIVE_BIT))
> > +#define NTB_LNK_STA_SPEED(x)   (((x) & NTB_LNK_STA_SPEED_MASK) >>
> 16)
> > +#define NTB_LNK_STA_WIDTH(x)   (((x) &
> NTB_LNK_STA_WIDTH_MASK) >> 20)
> > +
> > +#ifndef ioread64
> > +#ifdef readq
> > +#define ioread64 readq
> > +#else
> > +#define ioread64 _ioread64
> > +static inline u64 _ioread64(void __iomem *mmio)
> > +{
> > +       u64 low, high;
> > +
> > +       low = ioread32(mmio);
> > +       high = ioread32(mmio + sizeof(u32));
> > +       return low | (high << 32);
> > +}
> > +#endif
> > +#endif
> > +
> > +#ifndef iowrite64
> > +#ifdef writeq
> > +#define iowrite64 writeq
> > +#else
> > +#define iowrite64 _iowrite64
> > +static inline void _iowrite64(u64 val, void __iomem *mmio)
> > +{
> > +       iowrite32(val, mmio);
> > +       iowrite32(val >> 32, mmio + sizeof(u32));
> > +}
> > +#endif
> > +#endif
> >

How about put ioread64/iowrite64 macro into ntb.h file? So low level driver can avoid to define these macro.

> > +#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.