Re: Fwd: [PATCH] NTB: Fix an error in get link status

From: Jiasen Lin
Date: Mon Dec 02 2019 - 21:58:03 EST



On 2019/11/26 22:35, Nath, Arindam wrote:
-----Original Message-----
From: Mehta, Sanju <Sanju.Mehta@xxxxxxx>
Sent: Tuesday, November 26, 2019 18:40
To: Jiasen Lin <linjiasen@xxxxxxxx>
Cc: S-k, Shyam-sundar <Shyam-sundar.S-k@xxxxxxx> Dave Jiang
<dave.jiang@xxxxxxxxx>; Nath, Arindam <Arindam.Nath@xxxxxxx>; Allen
Hubbe <allenbh@xxxxxxxxx>; linux-kernel <linux-kernel@xxxxxxxxxxxxxxx>;
linux-ntb <linux-ntb@xxxxxxxxxxxxxxxx>; linjiasen007@xxxxxxxxx
Subject: Re: Fwd: [PATCH] NTB: Fix an error in get link status


Hi Sanjay R Mehta

In more complex topology, read the Link Status and Control register of
RP is also wrong. Suppose that a PCIe switch bridge to the Secondary RP,
and Secondary internal SW.ds is a child device for switch's downstream
port as illustrated in the following topology.

In secondary PCI domain:
Secondary RP--Switch UP--Switch DP--Secondary internal SW.us--
Secondary
internal SW.ds--Secondary NTB

pci_rp = pci_find_pcie_root_port(ndev->ntb.pdev) will return the
Secondary RP, and pcie_capability_read_dword(pci_rp,
PCI_EXP_LNKCTL,&stat) will get the link status between Secondary RP and
Switch UP. Maybe, read the Link Status and control register of Secondary
internal SW.us is appropriate.

Hi Jiansen Lin,

I modified the code as per your suggestion and is working fine.
Adding Arindam for comments who was the co-author of the patch I was
about to send for upstream review.

Hi Jiansen Lin,

I am okay with the changes proposed by you. But one thing we need to keep
in mind is that, the configuration SWUS+SWDS+EP as visible from the NTB
secondary, might change in future AMD implementations where these internal
switches are not present anymore. So we might have to re-visit the patch
again later.

Thanks,
Arindam


Hi Adridam
We can define a depth value that from secondary NTB EP to the real link training device for the various devices, if internal switch is not
presnt in future. In amd_poll_link we traverse up the parent chain utill the depth is reached.
Now, for device 145b, the depth is defined to 2, because only one
internal switch is implemented for secondary NTB. For device 148b, maybe
also one internal switch, I guess.

static const struct ntb_dev_data dev_data[] = {
{ /* for device 145b */
.mw_count = 3,
.mw_idx = 1,
.depth = 2,
},
{ /* for device 148b */
.mw_count = 2,
.mw_idx = 2,
.depth = 2,/*maybe is 2, according to implementation of the 148b */
},
};

static const struct pci_device_id amd_ntb_pci_tbl[] = {
{ PCI_VDEVICE(AMD, 0x145b), (kernel_ulong_t)&dev_data[0] },
{ PCI_VDEVICE(AMD, 0x148b), (kernel_ulong_t)&dev_data[1] },
{ PCI_VDEVICE(HYGON, 0x145b), (kernel_ulong_t)&dev_data[0] },
{ 0, }
};

Thanks,
Jiasen Lin

Thanks,
Sanjay Mehta
struct pci_dev *pci_up = NULL;
struct pci_dev *pci_dp = NULL;

if (ndev->ntb.topo == NTB_TOPO_SEC) {
ÂÂÂ /* Locate the pointer to Secondary up for this device */
ÂÂÂ pci_dp = pci_upstream_bridge(ndev->ntb.pdev);
ÂÂÂ /* Read the PCIe Link Control and Status register */
ÂÂÂ if (pci_dp) {
ÂÂÂÂÂÂ pci_up = pci_upstream_bridge(pci_dp);
ÂÂÂÂÂÂ if (pci_up) {
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ rc = pcie_capability_read_dword(pci_up, PCI_EXP_LNKCTL,
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ &stat);
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ if (rc)
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ return 0;
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ }
ÂÂÂÂÂÂ }
}

Thanks,
Jiansen Lin

I have modified the code according to your suggestion and tested it
on Dhyana platform, it works well, expect to receice your patch.

Before modify the code, read the Link Status and control register of the
secondary NTB device to get link status.

cat /sys/kernel/debug/ntb_hw_amd/0000\:43\:00.1/info
NTB Device Information:
Connection Topology -ÂÂ NTB_TOPO_SEC
LNK STA -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ 0x11030042
Link Status -ÂÂÂÂÂÂÂÂÂÂ Up
Link Speed -ÂÂÂÂÂÂÂÂÂÂÂ PCI-E Gen 3
Link Width -ÂÂÂÂÂÂÂÂÂÂÂ x16

After modify the code, read the Link Status and control register of the
secondary RP to get link status.

cat /sys/kernel/debug/ntb_hw_amd/0000\:43\:00.1/info
NTB Device Information:
Connection Topology -ÂÂ NTB_TOPO_SEC
LNK STA -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ 0x70830042
Link Status -ÂÂÂÂÂÂÂÂÂÂ Up
Link Speed -ÂÂÂÂÂÂÂÂÂÂÂ PCI-E Gen 3
Link Width -ÂÂÂÂÂÂÂÂÂÂÂ x8

Thanks,
Jiasen Lin

---
 drivers/ntb/hw/amd/ntb_hw_amd.c | 27
+++++++++++++++++++++++----
 drivers/ntb/hw/amd/ntb_hw_amd.h | 1 -
 2 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/drivers/ntb/hw/amd/ntb_hw_amd.c
b/drivers/ntb/hw/amd/ntb_hw_amd.c
index 14ad69c..91e1966 100644
--- a/drivers/ntb/hw/amd/ntb_hw_amd.c
+++ b/drivers/ntb/hw/amd/ntb_hw_amd.c
@@ -842,6 +842,8 @@ static inline void ndev_init_struct(struct
amd_ntb_dev *ndev,
 static int amd_poll_link(struct amd_ntb_dev *ndev)
 {
ÂÂÂÂÂ void __iomem *mmio = ndev->peer_mmio;
+ÂÂÂ struct pci_dev *pci_rp = NULL;
+ÂÂÂ struct pci_dev *pdev = NULL;
ÂÂÂÂÂ u32 reg, stat;
ÂÂÂÂÂ int rc;
@@ -855,10 +857,27 @@ static int amd_poll_link(struct amd_ntb_dev
*ndev)
ÂÂÂÂÂ ndev->cntl_sta = reg;
-ÂÂÂ rc = pci_read_config_dword(ndev->ntb.pdev,
-ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ AMD_LINK_STATUS_OFFSET, &stat);
-ÂÂÂ if (rc)
-ÂÂÂÂÂÂÂ return 0;
+ÂÂÂ if (ndev->ntb.topo == NTB_TOPO_SEC) {
+ÂÂÂÂÂÂÂ /* Locate the pointer to PCIe Root Port for this device */
+ÂÂÂÂÂÂÂ pci_rp = pci_find_pcie_root_port(ndev->ntb.pdev);
+ÂÂÂÂÂÂÂ /* Read the PCIe Link Control and Status register */
+ÂÂÂÂÂÂÂ if (pci_rp) {
+ÂÂÂÂÂÂÂÂÂÂÂ rc = pcie_capability_read_dword(pci_rp, PCI_EXP_LNKCTL,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ &stat);
+ÂÂÂÂÂÂÂÂÂÂÂ if (rc)
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ return 0;
+ÂÂÂÂÂÂÂ }
+ÂÂÂ } else if (ndev->ntb.topo == NTB_TOPO_PRI) {
+ÂÂÂÂÂÂÂ /*
+ÂÂÂÂÂÂÂÂ * For NTB primary, we simply read the Link Status and control
+ÂÂÂÂÂÂÂÂ * register of the NTB device itself.
+ÂÂÂÂÂÂÂÂ */
+ÂÂÂÂÂÂÂ pdev = ndev->ntb.pdev;
+ÂÂÂÂÂÂÂ rc = pcie_capability_read_dword(pdev, PCI_EXP_LNKCTL, &stat);
+ÂÂÂÂÂÂÂ if (rc)
+ÂÂÂÂÂÂÂÂÂÂÂ return 0;
+ÂÂÂ }
+
ÂÂÂÂÂ ndev->lnk_sta = stat;
ÂÂÂÂÂ return 1;
diff --git a/drivers/ntb/hw/amd/ntb_hw_amd.h
b/drivers/ntb/hw/amd/ntb_hw_amd.h
index 139a307..39e5d18 100644
--- a/drivers/ntb/hw/amd/ntb_hw_amd.h
+++ b/drivers/ntb/hw/amd/ntb_hw_amd.h
@@ -53,7 +53,6 @@
 #include <linux/pci.h>
 #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