linux-kernel - RE: [PATCH V4 1/1] NTB: Add support for AMD PCI-Express Non-Transparent Bridge

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 20 Jan 2016 11:26:42 -0500
From:	"Allen Hubbe" <Allen.Hubbe@....com>
To:	"'Yu, Xiangliang'" <Xiangliang.Yu@....com>, <jdmason@...zu.us>,
	<linux-kernel@...r.kernel.org>, <linux-ntb@...glegroups.com>
Cc:	"'SPG_Linux_Kernel'" <SPG_Linux_Kernel@....com>
Subject: RE: [PATCH V4 1/1] NTB: Add support for AMD PCI-Express Non-Transparent Bridge

> > There is some concept of "split bar" in this function, and I want to
> be sure to
> > understand it correctly.
> >
> > BAR0 - configuration?
> > BAR1 - 32bit memory window? i.e. "split" bar?
> > BAR2+3 - 64bit memory window?
> > BAR4+5 - 64bit memory window?
> 
> Yes

Ok.

> > Note that "split" in the intel driver refers to BAR4+5, which is
> normally a 64bit
> > memory window, split into independent 32bit windows BAR4 and BAR5 by
> > bios configuration.  Calling it "split" there makes sense.  Here,
> calling it "split"
> > is confusing, but as long as the code is correct, I think it's ok.
> 
> AMD NTB has similar design.

If by similar design, you mean that BAR2+3 or BAR4+5 can be split, those configurations are not currently supported by this driver.  If needed, support for those configurations can be added later, as a patch.

Ok with the current implementation.

> > I see a lot of readl/writel and ioread/iowrite in the same file, even
> in the
> > same function as there is here.  Pick one variant of the functions,
> preferably
> > the ioread/iowrite variant, and be consistent in its usage throughout
> the file.
> 
> Ioread/iowrite is only for 64bit read/write, I don't think it has any
> confusion.

It's not just for 64bit: there are ioread8,16,32.  In fact, ioread64 is the one that's not provided by io.h: not all hardware can do a true 64bit read or write.  We don't need a true 64bit operation, so ioread64 is provided locally, for convenience within this driver.

> Actually, Intel NTB driver has same behavior.

Actually, Intel NTB driver does not use readl/writel.

Would you please fix this?

> > It's also interesting that if it hits the last branch, ndev->peer_sta
> is assigned
> > zero, but the function returns zero, not LNK_STA.  If the function
> were
> > immediately called again, it would return LNK_STA.  Can you please
> explain
> > the logic here?
> 
> Force to read link status register again because the opposite side maybe
> still
> In resuming progress, the status variable can't show the right state,
> have to
> Read register directly.

There's no indication in this function that it interacts with the timer.  With your suggestion, I'll take a closer look at the interactions between this function and the timer.