Message-ID: <e9e1543f-0f38-89ed-f179-71ac8724f4e0@ti.com>
Date:   Thu, 8 Jun 2023 15:23:49 +0530
From:   Siddharth Vadapalli <s-vadapalli@...com>
To:     Lorenzo Pieralisi <lpieralisi@...nel.org>
CC:     <tjoseph@...ence.com>, <robh@...nel.org>, <kw@...ux.com>,
        <bhelgaas@...gle.com>, <nadeem@...ence.com>,
        <linux-pci@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>, <vigneshr@...com>,
        <srk@...com>, <nm@...com>, <s-vadapalli@...com>
Subject: Re: [PATCH v3] PCI: cadence: Fix Gen2 Link Retraining process



On 08/06/23 15:14, Lorenzo Pieralisi wrote:
> On Thu, Jun 08, 2023 at 09:31:59AM +0530, Siddharth Vadapalli wrote:
>> Hello Lorenzo,
>>
>> Thank you for reviewing this patch.
>>
>> On 07/06/23 15:53, Lorenzo Pieralisi wrote:
>>> On Wed, Jun 07, 2023 at 02:44:27PM +0530, Siddharth Vadapalli wrote:
>>>> The Link Retraining process is initiated to account for the Gen2 defect in
>>>> the Cadence PCIe controller in J721E SoC. The errata corresponding to this
>>>> is i2085, documented at:
>>>> https://www.ti.com/lit/er/sprz455c/sprz455c.pdf
>>>>
>>>> The existing workaround implemented for the errata waits for the Data Link
>>>> initialization to complete and assumes that the link retraining process
>>>> at the Physical Layer has completed. However, it is possible that the
>>>> Physical Layer training might be ongoing as indicated by the
>>>> PCI_EXP_LNKSTA_LT bit in the PCI_EXP_LNKSTA register.
>>>>
>>>> Fix the existing workaround, to ensure that the Physical Layer training
>>>> has also completed, in addition to the Data Link initialization.
>>>>
>>>> Fixes: 4740b969aaf5 ("PCI: cadence: Retrain Link to work around Gen2 training defect")
>>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@...com>
>>>> Reviewed-by: Vignesh Raghavendra <vigneshr@...com>
>>>> ---
>>>>
>>>> Hello,
>>>>
>>>> This patch is based on linux-next tagged next-20230606.
>>>>
>>>> v2:
>>>> https://lore.kernel.org/r/20230315070800.1615527-1-s-vadapalli@ti.com/
>>>> Changes since v2:
>>>> - Merge the cdns_pcie_host_training_complete() function with the
>>>>   cdns_pcie_host_wait_for_link() function, as suggested by Bjorn
>>>>   for the v2 patch.
>>>> - Add dev_err() to notify when Link Training fails, since this is a
>>>>   fatal error and proceeding from this point will almost always crash
>>>>   the kernel.
>>>>
>>>> v1:
>>>> https://lore.kernel.org/r/20230102075656.260333-1-s-vadapalli@ti.com/
>>>> Changes since v1:
>>>> - Collect Reviewed-by tag from Vignesh Raghavendra.
>>>> - Rebase on next-20230315.
>>>>
>>>> Regards,
>>>> Siddharth.
>>>>
>>>>  .../controller/cadence/pcie-cadence-host.c    | 20 +++++++++++++++++++
>>>>  1 file changed, 20 insertions(+)
>>>>
>>>> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
>>>> index 940c7dd701d6..70a5f581ff4f 100644
>>>> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
>>>> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
>>>> @@ -12,6 +12,8 @@
>>>>  
>>>>  #include "pcie-cadence.h"
>>>>  
>>>> +#define LINK_RETRAIN_TIMEOUT HZ
>>>> +
>>>>  static u64 bar_max_size[] = {
>>>>  	[RP_BAR0] = _ULL(128 * SZ_2G),
>>>>  	[RP_BAR1] = SZ_2G,
>>>> @@ -80,8 +82,26 @@ static struct pci_ops cdns_pcie_host_ops = {
>>>>  static int cdns_pcie_host_wait_for_link(struct cdns_pcie *pcie)
>>>>  {
>>>>  	struct device *dev = pcie->dev;
>>>> +	unsigned long end_jiffies;
>>>> +	u16 link_status;
>>>>  	int retries;
>>>>  
>>>> +	/* Wait for link training to complete */
>>>> +	end_jiffies = jiffies + LINK_RETRAIN_TIMEOUT;
>>>> +	do {
>>>> +		link_status = cdns_pcie_rp_readw(pcie, CDNS_PCIE_RP_CAP_OFFSET + PCI_EXP_LNKSTA);
>>>> +		if (!(link_status & PCI_EXP_LNKSTA_LT))
>>>> +			break;
>>>
>>> You can use a bool variable eg link_trained and use that below.
>>
>> Sure, I will do that. link_trained = !(link_status & PCI_EXP_LNKSTA_LT); within
>> the do-while loop and checking for it to be true in the loop as well as below.
>>
>>>
>>>> +		usleep_range(0, 1000);
>>>> +	} while (time_before(jiffies, end_jiffies));
>>>> +
>>>> +	if (!(link_status & PCI_EXP_LNKSTA_LT)) {
>>>> +		dev_info(dev, "Link training complete\n");
>>>> +	} else {
>>>> +		dev_err(dev, "Fatal! Link training incomplete\n");
>>>> +		return -ETIMEDOUT;
>>>> +	}
>>>
>>> I don't necessarily see the reason why you are adding additional
>>> logging, more so given that this now does not affect just the
>>> workaround but all cadence controllers.
>>>
>>> Actually, is that something you have tested and considered ?
>>
>> While I agree that I could have performed the entire Link Training check only if
>> the Gen2 Link Retraining Quirk is set for the RC, considering that the
>> completion of the Link Training is a necessity irrespective of whether or not
>> the Quirk exists, I preferred to add the check unconditionally. I would like to
>> point out that the race condition responsible for the crash is the following:
>> Without the completion of the Physical Layer link training, the call to the
>> cdns_pci_map_bus() function in order to access the End Point's registers (if an
>> EP device is connected) results in the crash. This is primarily observed only on
>> RT Linux where the software call to cdns_pci_map_bus() by PCI subsystem occurs
>> quite fast, before the Physical Layer link training is complete. For this
>> reason, irrespective of whether the Physical Layer link training occurs only
>> once because of the default flow or occurs a second time due to the Gen2 Link
>> Retraining Quirk, it appears to me that the crash could potentially occur in
>> both cases if we don't wait for the Physical Layer link training to complete.
> 
> This means that you are fixing the controller code for all Cadence platforms
> and current code is broken (I am not really sure what you mean wrt RT Linux
> - please elaborate, this does not sound good to me at all), so the
> commit log must be rewritten.

By RT Linux, I am referring to Linux with Real Time configs enabled such as
PREEMPT_RT, which results in faster code execution and lower latency.
All along, it appears to be the case that with the Link Training initiated in
PCIe controller hardware, the driver proceeds to finish the probe and therefore
allow the PCI core subsystem to perform enumeration. However, as it turns out,
the driver appears to have been depending on the latency of other processes
running in the background, due to which the device enumeration process by the
PCI core subsystem got delayed, thereby ensuring that the Link Training process
in the PCIe controller hardware had successfully finished by that point in time.
With RT configs, that latency has been reduced, resulting in the bus enumeration
process starting off much earlier, and occurring while the PCIe controller is
still performing Link Training.

> 
>> Please let me know if this sounds acceptable. If not, I will check if the quirk
>> is set before proceeding to verify link training completion and implement this
>> in the v4 patch.
> 
> Not only this is acceptable but that's what you should do. Current code
> relies on assumptions (that you will have to explain to me please) wrt
> link training and that's not OK.

Please let me know if further clarification is required in addition to my
comment above.

> 
> Lorenzo
> 
>>
>>>
>>> Thanks,
>>> Lorenzo
>>>
>>>> +
>>>>  	/* Check if the link is up or not */
>>>>  	for (retries = 0; retries < LINK_WAIT_MAX_RETRIES; retries++) {
>>>>  		if (cdns_pcie_link_up(pcie)) {
>>>> -- 
>>>> 2.25.1
>>>>
>>
>> -- 
>> Regards,
>> Siddharth.

-- 
Regards,
Siddharth.

Powered by blists - more mailing lists