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

[Nath, Arindam]

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

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