Re: [PATCH v3] PCI: keystone: Fix race condition when initializing PHYs

[Siddharth Vadapalli <s-vadapalli@ti.com>]

Hello Bjorn,

On 10/01/24 02:53, Bjorn Helgaas wrote:
> On Wed, Sep 27, 2023 at 09:48:45AM +0530, Siddharth Vadapalli wrote:
>> The PCI driver invokes the PHY APIs using the ks_pcie_enable_phy()
>> function. The PHY in this case is the Serdes. It is possible that the
>> PCI instance is configured for 2 lane operation across two different

...

>>  
>> +	/* Obtain reference(s) to the phy(s) */
>> +	for (i = 0; i < num_lanes; i++)
>> +		phy_pm_runtime_get_sync(ks_pcie->phy[i]);
>> +
>>  	ret = ks_pcie_enable_phy(ks_pcie);
>> +
>> +	/* Release reference(s) to the phy(s) */
>> +	for (i = 0; i < num_lanes; i++)
>> +		phy_pm_runtime_put_sync(ks_pcie->phy[i]);
> 
> This looks good and has already been applied, so no immediate action
> required.
> 
> This is the only call to ks_pcie_enable_phy(), and these loops get and
> put the PM references for the same PHYs initialized in
> ks_pcie_enable_phy(), so it seems like maybe these loops could be
> moved *into* ks_pcie_enable_phy().

Does the following look fine?
===============================================================================
diff --git a/drivers/pci/controller/dwc/pci-keystone.c
b/drivers/pci/controller/dwc/pci-keystone.c
index e02236003b46..6e9f9589d26c 100644
--- a/drivers/pci/controller/dwc/pci-keystone.c
+++ b/drivers/pci/controller/dwc/pci-keystone.c
@@ -962,6 +962,9 @@ static int ks_pcie_enable_phy(struct keystone_pcie *ks_pcie)
        int num_lanes = ks_pcie->num_lanes;

        for (i = 0; i < num_lanes; i++) {
+               /* Obtain reference to the phy */
+               phy_pm_runtime_get_sync(ks_pcie->phy[i]);
+
                ret = phy_reset(ks_pcie->phy[i]);
                if (ret < 0)
                        goto err_phy;
@@ -977,12 +980,18 @@ static int ks_pcie_enable_phy(struct keystone_pcie *ks_pcie)
                }
        }

+       /* Release reference(s) to the phy(s) */
+       for (i = 0; i < num_lanes; i++)
+               phy_pm_runtime_put_sync(ks_pcie->phy[i]);
+
        return 0;

 err_phy:
        while (--i >= 0) {
                phy_power_off(ks_pcie->phy[i]);
                phy_exit(ks_pcie->phy[i]);
+               /* Release reference to the phy */
+               phy_pm_runtime_put_sync(ks_pcie->phy[i]);
        }

        return ret;
===============================================================================

> 
> Is there any similar issue in ks_pcie_disable_phy()?  What if we
> power-off a PHY that provides a reference clock to other PHYs that are
> still powered-up?  Will the dependent PHYs still power-off cleanly?

While debugging the issue fixed by this patch, I had bisected and identified
that prior to the following commit:
https://github.com/torvalds/linux/commit/e611f8cd8717c8fe7d4229997e6cd029a1465253
despite the race condition being present, there was no issue. While I am not
fully certain, I believe that the above observation indicates that prior to the
aforementioned commit, the race condition did exist, but there was a slightly
longer delay between the PHY providing the reference clock being powered off
within "ks_pcie_enable_phy()". That delay was sufficient for the dependent PHY
to lock its PLL based on the reference clock provided, following which, despite
the PHY providing the reference clock being powered off and the dependent PHY
staying powered on, there was no issue observed. Therefore, it appears to me
that holding reference to the PHY providing the reference clock isn't necessary
once the dependent PHY's PLL is locked.

...

-- 
Regards,
Siddharth.