lkml.org 
[lkml]   [2019]   [Nov]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: Fwd: [PATCH] NTB: Fix an error in get link status
From
Date


On 2019/11/21 21:30, Jiasen Lin wrote:
>
>
> On 2019/11/20 22:13, Sanjay R Mehta wrote:
>> From: *Jiasen Lin* <linjiasen@hygon.cn <mailto:linjiasen@hygon.cn>>
>>> Date: Wed, Nov 20, 2019 at 3:25 PM
>>> Subject: Re: [PATCH] NTB: Fix an error in get link status
>>> To: Jon Mason <jdmason@kudzu.us <mailto:jdmason@kudzu.us>>
>>> Cc: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com
>>> <mailto:Shyam-sundar.S-k@amd.com>>, Dave Jiang <dave.jiang@intel.com
>>> <mailto:dave.jiang@intel.com>>, Allen Hubbe <allenbh@gmail.com
>>> <mailto:allenbh@gmail.com>>, linux-kernel
>>> <linux-kernel@vger.kernel.org <mailto:linux-kernel@vger.kernel.org>>,
>>> linux-ntb <linux-ntb@googlegroups.com
>>> <mailto:linux-ntb@googlegroups.com>>,
>>> <linjiasen007@gmail.com <mailto:linjiasen007@gmail.com>>, Jiasen Lin
>>> <linjiasen@hygon.cn <mailto:linjiasen@hygon.cn>>
>>>
>>>
>>>
>>>
>>> On 2019/11/18 18:17, Jiasen Lin wrote:
>>>>
>>>>
>>>> On 2019/11/18 7:00, Jon Mason wrote:
>>>>> On Thu, Nov 7, 2019 at 4:37 AM Jiasen Lin <linjiasen@hygon.cn
>>>>> <mailto:linjiasen@hygon.cn>> wrote:
>>>>>>
>>>>>> The offset of PCIe Capability Header for AMD and HYGON NTB is 0x64,
>>>>>> but the macro which named "AMD_LINK_STATUS_OFFSET" is defined as
>>>>>> 0x68.
>>>>>> It is offset of Device Capabilities Reg rather than Link Control Reg.
>>>>>>
>>>>>> This code trigger an error in get link statsus:
>>>>>>
>>>>>>           cat /sys/kernel/debug/ntb_hw_amd/0000:43:00.1/info
>>>>>>                   LNK STA -               0x8fa1
>>>>>>                   Link Status -           Up
>>>>>>                   Link Speed -            PCI-E Gen 0
>>>>>>                   Link Width -            x0
>>>>>>
>>>>>> This patch use pcie_capability_read_dword to get link status.
>>>>>> After fix this issue, we can get link status accurately:
>>>>>>
>>>>>>           cat /sys/kernel/debug/ntb_hw_amd/0000:43:00.1/info
>>>>>>                   LNK STA -               0x11030042
>>>>>>                   Link Status -           Up
>>>>>>                   Link Speed -            PCI-E Gen 3
>>>>>>                   Link Width -            x16
>>>>>
>>>>> No response from AMD maintainers, but it looks like you are correct.
>>>>>
>>>>> This needs a "Fixes:" line here.  I took the liberty of adding one to
>>>>> this patch.
>>>>>
>>>>
>>>> Thank you for your suggestions. Yes, this patch fix the commit id:
>>>> a1b3695 ("NTB: Add support for AMD PCI-Express Non-Transparent
>>>> Bridge").
>>>>
>>>>>> Signed-off-by: Jiasen Lin <linjiasen@hygon.cn
>>>>>> <mailto:linjiasen@hygon.cn>>
>>>>>> ---
>>>>>>    drivers/ntb/hw/amd/ntb_hw_amd.c | 5 +++--
>>>>>>    drivers/ntb/hw/amd/ntb_hw_amd.h | 1 -
>>>>>>    2 files changed, 3 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/ntb/hw/amd/ntb_hw_amd.c
>>>>>> b/drivers/ntb/hw/amd/ntb_hw_amd.c
>>>>>> index 156c2a1..ae91105 100644
>>>>>> --- a/drivers/ntb/hw/amd/ntb_hw_amd.c
>>>>>> +++ b/drivers/ntb/hw/amd/ntb_hw_amd.c
>>>>>> @@ -855,8 +855,8 @@ 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);
>>>>>> +       rc = pcie_capability_read_dword(ndev->ntb.pdev,
>>>>>> +                                  PCI_EXP_LNKCTL, &stat);
>>>>>>           if (rc)
>>>>>>                   return 0;
>>>>>>           ndev->lnk_sta = stat;
>>>>>> @@ -1139,6 +1139,7 @@ static const struct ntb_dev_data dev_data[] = {
>>>>>>    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] },
>>>>>
>>>>> This should be a separate patch.  I took the liberty of splitting it
>>>>> off into a unique patch and attributing it to you.  I've pushed them
>>>>> to the ntb-next branch on
>>>>> https://github.com/jonmason/ntb
>>>>>
>>>> Thank you for your comment. We appreciate the time and effort you have
>>>> spent to split it off, I will test it ASAP.
>>>>
>>>>> Please verify everything looks acceptable to you (given the changes I
>>>>> did above that are attributed to you).  Also, testing of the latest
>>>>> code is always appreciated.
>>>>>
>>>>> Thanks,
>>>>> Jon
>>>>>
>>>
>>> I have tested these patches that are pushed to ntb-next branch, they
>>> work well on HYGON platforms.
>>>
>>> Thanks,
>>> Jiasen Lin
>>
>> Hi Jiasen Lin,
>>
>> Apologies for my delayed response. I was on vacation.
>>
>> Your patch is a correct fix, but that would work only for primary system.
>>
>> The design of AMD NTB implementation is such that NTB primary acts as
>> an endpoint device and NTB secondary is a PCIe Root Port (RP).
>> Considering that,
>> the link status and control register needs to be accessed differently
>> based on the NTB topology.
>>
>> So in the case of NTB secondary, we read the link status and control
>> register of the PCIe RP capabilities structure and in the case of NTB
>> primary, we read the
>> link status and control register from NTB device capabilities structure.
>>
>> The code below is the proper fix for AMD platform. I will be sending
>> incremental change above your patch.
>>
>> would appreciate if you could test my patch and let me know whether
>> that works for you.
>>
>
> Dhyana CPU dones not support data transfer while both sides of PCIe link
> are configured as NTB, in other word, Dhyana only support NTB that is
> connected to RP rather than NT to NT.
>
> As illustrated in the following topology, NTB consists of two PCIe
> endpoints, a Primary NTB, and a Secondary NTB. Primary CPU can find
> Priamry NTB, while Secondary NTB, Secondary internal SW.ds and Secondary
> internal SW.ds are enumerated by secondary CPU.
>
> In this topology, to remove any ambiguity, your suggestion is more
> accurate method to get link status of NTB.
>
> In primary PCI domain:
> Primary RP--Primary NTB----------------------------------------
> 40:04.1-------41:00.1(Pri NTB)                                |
>                                                               | In
> secondary PCI domain:                                      |
> Secondary RP--Secondary SW.us--Secondary SW.ds--Secondary NTB--
> 40:03.1---------41:00.0---------42:00.0---------43:00.1(Sec NTB)
>

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.

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

\
 
 \ /
  Last update: 2019-11-22 02:31    [W:0.630 / U:0.124 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site