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

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

On 20/01/24 04:50, Bjorn Helgaas wrote:
> On Wed, Jan 10, 2024 at 11:35:24AM +0530, Siddharth Vadapalli wrote:
>> Hello Bjorn,
>>
>> On 10/01/24 02:53, Bjorn Helgaas wrote:

...

>>
>> 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]);
> 
> I thought the point was that you needed to guarantee that all PHYs are
> powered on and stay that way before initializing any of them.  If so,
> you would need a separate loop, e.g.,
> 
>   for (i = 0; i < num_lanes; i++)
>     phy_pm_runtime_get_sync(ks_pcie->phy[i]);
> 
>   for (i = 0; i < num_lanes; i++) {
>     ret = phy_reset(ks_pcie->phy[i]);
>     ...
> 

Yes, the above implementation seems better. The strict requirement will be that
post initialization of the first PHY (Serdes), it remains powered ON so that it
can provide its reference clock to the second PHY (Serdes). Therefore, getting
the reference to the PHYs within the loop will work too since the reference is
being release only outside the loop. Nevertheless I shall go ahead with the
implementation suggested by you since that looks much better and cleaner.

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

-- 
Regards,
Siddharth.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel