Received-SPF: pass (google.com: domain of linux-kernel+bounces-32322-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <230c2000-1510-4da2-b5b7-df363cd9a45d@ti.com>
Date: Mon, 22 Jan 2024 10:08:21 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
CC: <lpieralisi@kernel.org>, <robh@kernel.org>, <kw@linux.com>,
        <bhelgaas@google.com>, <linux-pci@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <linux-arm-kernel@lists.infradead.org>,
        <ilpo.jarvinen@linux.intel.com>, <vigneshr@ti.com>,
        <r-gunasekaran@ti.com>, <srk@ti.com>, <s-vadapalli@ti.com>
Subject: Re: [PATCH v3] PCI: keystone: Fix race condition when initializing
 PHYs
To: Bjorn Helgaas <helgaas@kernel.org>
References: <20240119232032.GA192245@bhelgaas>
Content-Language: en-US
From: Siddharth Vadapalli <s-vadapalli@ti.com>
In-Reply-To: <20240119232032.GA192245@bhelgaas>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit


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.