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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <BLUPR12MB042040E105A046796188B2A1EBC30@BLUPR12MB0420.namprd12.prod.outlook.com>
Date:	Thu, 21 Jan 2016 02:17:15 +0000
From:	"Yu, Xiangliang" <Xiangliang.Yu@....com>
To:	Allen Hubbe <Allen.Hubbe@....com>,
	"jdmason@...zu.us" <jdmason@...zu.us>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-ntb@...glegroups.com" <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?

I'll change ioread64/iowrite64 to read64/write64, keep consistent witch readl/readw

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

I'll add a comment.