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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <230c2000-1510-4da2-b5b7-df363cd9a45d@ti.com>
Date: Mon, 22 Jan 2024 10:08:21 +0530
From: Siddharth Vadapalli <s-vadapalli@...com>
To: Bjorn Helgaas <helgaas@...nel.org>
CC: <lpieralisi@...nel.org>, <robh@...nel.org>, <kw@...ux.com>,
        <bhelgaas@...gle.com>, <linux-pci@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <linux-arm-kernel@...ts.infradead.org>,
        <ilpo.jarvinen@...ux.intel.com>, <vigneshr@...com>,
        <r-gunasekaran@...com>, <srk@...com>, <s-vadapalli@...com>
Subject: Re: [PATCH v3] PCI: keystone: Fix race condition when initializing
 PHYs



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.