linux-kernel - RE: [PATCH V5 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] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 21 Jan 2016 05:58:01 +0000
From:	"Yu, Xiangliang" <Xiangliang.Yu@....com>
To:	Allen Hubbe <Allen.Hubbe@....com>,
	"jdmason@...zu.us" <jdmason@...zu.us>,
	"dave.jiang@...el.com" <dave.jiang@...el.com>,
	"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 V5 1/1] NTB: Add support for AMD PCI-Express
 Non-Transparent Bridge



> From: Xiangliang Yu <Xiangliang.Yu@....com>
> > This adds support for AMD's PCI-Express Non-Transparent Bridge
> > (NTB) device on the Zeppelin platform. The driver connnects to the
> > standard NTB sub-system interface, with modification to add hooks for
> > power management in a separate patch. The AMD NTB device has 3
> memory
> > windows, 16 doorbell, 16 scratch-pad registers, and supports up to 16
> > PCIe lanes running a Gen3 speeds.
> >
> > Signed-off-by: Xiangliang Yu <Xiangliang.Yu@....com>
> 
> > Signed-off-by: Jon Mason <jdmason@...zu.us>
> > Signed-off-by: Allen Hubbe <Allen.Hubbe@....com>
> 
> NO.

Ok, I'll change it if you doesn't want to change it.

> 
> > +		/* set and verify setting the translation address */
> > +		write64(addr, peer_mmio + xlat_reg);
> > +		reg_val = read64(peer_mmio + xlat_reg);
> > +		if (reg_val != addr) {
> > +			write64(0, peer_mmio + xlat_reg);
> > +			return -EIO;
> > +		}
> > +
> > +		/* set and verify setting the limit */
> > +		writel(limit, mmio + limit_reg);
> > +		reg_val = readl(mmio + limit_reg);
> > +		if (reg_val != limit) {
> > +			writel(base_addr, mmio + limit_reg);
> > +			writel(0, peer_mmio + xlat_reg);
> > +			return -EIO;
> > +		}
> 
> I see what you did there, change iowrite64 to write64.
> 
> What I meant was:
>  - change readl to ioread32.
>  - change writel to iowrite32.
>  - change readb, readw, writeb, writew (if there are any)
>  - leave ioread64 and iowrite64 as they were.
> 
> Why: http://www.makelinux.net/ldd3/chp-9-sect-4
> 
> Quote: "If you read through the kernel source, you see many calls to an older
> set of functions when I/O memory is being used. These functions still work,
> but their use in new code is discouraged. Among other things, they are less
> safe because they do not perform the same sort of type checking."
> 
> The "older set of functions" are read[bwl], write[bwl].  This is a new driver,
> with all new code.  Please use the ioread/iowrite variants.

I don’t think so. In here, the i/o memory is only happened when pci_iomap return
Success, so the register can't be accessed through IO port way. And ioread* will
Check if the memory type is mmio type or IO port type (please see the definition).
 I don’t think we need to check It, so I use read* because It can make more efficient. 
I think we need to think about actual usage, not only follow book.
And, I have said it in previous version, I don’t like explain it again, and again.
If you have any concern, please tell me after my comment.

> > +static int amd_link_is_up(struct amd_ntb_dev *ndev) {
> > +	if (!ndev->peer_sta)
> > +		return NTB_LNK_STA_ACTIVE(ndev->cntl_sta);
> > +
> > +	/* If peer_sta is reset or D0 event, the ISR has
> > +	 * started a timer to check link status of hardware.
> > +	 * So here just clear status bit. And if peer_sta is
> > +	 * D3 or PME_TO, D0/reset event will be happened when
> > +	 * system wakeup/poweron, so do nothing here.
> > +	 */
> > +	if (ndev->peer_sta & AMD_PEER_RESET_EVENT)
> > +		ndev->peer_sta &= ~AMD_PEER_RESET_EVENT;
> > +	else if (ndev->peer_sta & AMD_PEER_D0_EVENT)
> > +		ndev->peer_sta = 0;
> > +
> > +	return 0;
> > +}
> 
> Thanks.  This is much better.
> 
> > +static void amd_handle_event(struct amd_ntb_dev *ndev, int vec)
> ...
> > +	case AMD_PEER_D0_EVENT:
> ...
> > +		/* start a timer to poll link status */
> > +		schedule_delayed_work(&ndev->hb_timer,
> > +					      AMD_LINK_HB_TIMEOUT);
> 
> This is different from v4.  It used to be:
> 
> if (amd_link_is_up())
> 	ntb_link_event();
> else
> 	schedule_delayed_work();
> 
> Why is v5 correct?
> Why was v4 incorrect?

Because peer_sta is change to 0, so amd_link_is_up will return 0 (offline)
And will not check hardware link status. So It maybe make it offline forever

> I'm nervous about ndev->peer_sta, the behavior of link_is_up, timers...
> unexplained changes to a fragile bit of code - not just this code, but any code
> that deals with parallel or asynchronous behaviors.  With the comment in
> link_is_up, this code is much better, but any changes to this whole link state
> mechanism need to be explained.
Actually, the code is designed according to Atom NTB, except for the peer_sta. 
I'll add the explaination when having changes.