Message ID | 20230927041845.1222080-1-s-vadapalli@ti.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:cae8:0:b0:403:3b70:6f57 with SMTP id r8csp2403704vqu; Tue, 26 Sep 2023 22:41:06 -0700 (PDT) X-Google-Smtp-Source: AGHT+IFnJJ10QZufb7l+3zndQOpbAnc+cwspLJEINGQY2rsczOqaEEYAo1GvZvjn1q87UpeRB2xd X-Received: by 2002:aa7:8893:0:b0:690:3a0f:4165 with SMTP id z19-20020aa78893000000b006903a0f4165mr1254933pfe.32.1695793265686; Tue, 26 Sep 2023 22:41:05 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1695793265; cv=none; d=google.com; s=arc-20160816; b=jr3j0KXfn8jCRVZTDde2/wGJfJFlqiLlMtQd0R2yYS9EuQ7STy2BtK72Lh4tx95CXA OY9cN4dKDvNNFnX31t69aYHkht5FpxIa9G6z6LdMq+iiyxzDn2IJfmP+ODj5G7XNdp2s MesTGl7PgtIJ3mOLeTcrJr6JEqME1Dol1a+hMMqc4APrmkKk8ltv9Lv/RnJHm2tTXDH8 zvzZfrQEUwwUVKvDy+mqhedFwIvzmakaU1ZSpmhrXE9BnU7QsA9/s8n7cXw1K5fRgAyj BPEI7rsibNaTMLAznhoM7v268oSbFopptXNflCAKvW97WqUo8j/FcLzMosgt7KjGUXzD s4vg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=b+v0kXcbL/x8WGX8epMNqGlxkmAzB2Gr+GgRtU6z/Ss=; fh=RrYF632oxhPQLSnQi9n4jRNADSN05ZDoq1S3PNjSUPg=; b=ukpHl9d2pxBZvgdiavZ8MZHORfbXBatnqPWqMxRpjFTvWJUr2e8OvVPFRs2otB0cB0 4o5wq76UCKgB98sV0pqGm/R0eoumx9p3FwIdFaDDjpQ1TsXsqt5O/5CWLErPRicjMc0b PPp+J2a2i6VgVryWNLOPuh7kRf6DChSAzAGqC9Uu+1BaS9SwdRSMCI3Al5mfFlTyzSci YH8ZckPrlupzDoJmK8V9xu3pohMZYy4imjsv0M538VSoruX5i0SHLLeRy4gAxYqNBmx1 zusBcgtFVm1VTzMFuYWW/FL1GjQQRDrPkhgW+xZL1ndRhMv+axx9sTvGwyAA+4bc092d ienw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@ti.com header.s=ti-com-17Q1 header.b=S9GWVUfN; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=NONE dis=NONE) header.from=ti.com Received: from howler.vger.email (howler.vger.email. [2620:137:e000::3:4]) by mx.google.com with ESMTPS id w14-20020a63474e000000b00573f93787e4si14572787pgk.103.2023.09.26.22.41.05 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 26 Sep 2023 22:41:05 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4; Authentication-Results: mx.google.com; dkim=pass header.i=@ti.com header.s=ti-com-17Q1 header.b=S9GWVUfN; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=NONE dis=NONE) header.from=ti.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by howler.vger.email (Postfix) with ESMTP id 0BCFB8112479; Tue, 26 Sep 2023 21:54:13 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at howler.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229723AbjI0EyE (ORCPT <rfc822;pwkd43@gmail.com> + 26 others); Wed, 27 Sep 2023 00:54:04 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35920 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229509AbjI0ExU (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 27 Sep 2023 00:53:20 -0400 Received: from lelv0143.ext.ti.com (lelv0143.ext.ti.com [198.47.23.248]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 8907B3A85; Tue, 26 Sep 2023 21:19:08 -0700 (PDT) Received: from fllv0034.itg.ti.com ([10.64.40.246]) by lelv0143.ext.ti.com (8.15.2/8.15.2) with ESMTP id 38R4Ipcg064494; Tue, 26 Sep 2023 23:18:51 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ti.com; s=ti-com-17Q1; t=1695788331; bh=b+v0kXcbL/x8WGX8epMNqGlxkmAzB2Gr+GgRtU6z/Ss=; h=From:To:CC:Subject:Date; b=S9GWVUfNGSqeP/G8Qrr/cqRFpqfWiEQzfZnOBvNqahHcM0KzNmshQ/YkGq0sH7hWp 2CbsAodbidGI1QRDp0dVfhe4kW51h9vEYQGaV3CxD2BQ/pAmCxlhInEOJLX/031Y4D /HJ0/5Mu3DpNTEU34336ORjh9LjvYufc4ws4rQDA= Received: from DFLE106.ent.ti.com (dfle106.ent.ti.com [10.64.6.27]) by fllv0034.itg.ti.com (8.15.2/8.15.2) with ESMTPS id 38R4IoWx003148 (version=TLSv1.2 cipher=AES256-GCM-SHA384 bits=256 verify=FAIL); Tue, 26 Sep 2023 23:18:51 -0500 Received: from DFLE103.ent.ti.com (10.64.6.24) by DFLE106.ent.ti.com (10.64.6.27) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2507.23; Tue, 26 Sep 2023 23:18:50 -0500 Received: from fllv0039.itg.ti.com (10.64.41.19) by DFLE103.ent.ti.com (10.64.6.24) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2507.23 via Frontend Transport; Tue, 26 Sep 2023 23:18:50 -0500 Received: from uda0492258.dhcp.ti.com (ileaxei01-snat2.itg.ti.com [10.180.69.6]) by fllv0039.itg.ti.com (8.15.2/8.15.2) with ESMTP id 38R4IkJp057625; Tue, 26 Sep 2023 23:18:46 -0500 From: Siddharth Vadapalli <s-vadapalli@ti.com> To: <lpieralisi@kernel.org>, <robh@kernel.org>, <kw@linux.com>, <bhelgaas@google.com> CC: <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: [PATCH v3] PCI: keystone: Fix race condition when initializing PHYs Date: Wed, 27 Sep 2023 09:48:45 +0530 Message-ID: <20230927041845.1222080-1-s-vadapalli@ti.com> X-Mailer: git-send-email 2.34.1 MIME-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit X-EXCLAIMER-MD-CONFIG: e1e8a2fd-e40a-4ac6-ac9b-f7e9cc9ee180 X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED, RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (howler.vger.email [0.0.0.0]); Tue, 26 Sep 2023 21:54:13 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1778168119716685607 X-GMAIL-MSGID: 1778168119716685607 |
Series |
[v3] PCI: keystone: Fix race condition when initializing PHYs
|
|
Commit Message
Siddharth Vadapalli
Sept. 27, 2023, 4:18 a.m. UTC
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
Serdes instances, using 1 lane of each Serdes. In such a configuration,
if the reference clock for one Serdes is provided by the other Serdes,
it results in a race condition. After the Serdes providing the reference
clock is initialized by the PCI driver by invoking its PHY APIs, it is
not guaranteed that this Serdes remains powered on long enough for the
PHY APIs based initialization of the dependent Serdes. In such cases,
the PLL of the dependent Serdes fails to lock due to the absence of the
reference clock from the former Serdes which has been powered off by the
PM Core.
Fix this by obtaining reference to the PHYs before invoking the PHY
initialization APIs and releasing reference after the initialization is
complete.
Fixes: 49229238ab47 ("PCI: keystone: Cleanup PHY handling")
Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---
NOTE: This patch is based on linux-next tagged next-20230927.
v2:
https://lore.kernel.org/r/20230926063638.1005124-1-s-vadapalli@ti.com/
Changes since v2:
- Implement suggestion by Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
moving the phy_pm_runtime_put_sync() For-Loop section before the
return value of ks_pcie_enable_phy(ks_pcie) is checked, thereby
preventing duplication of the For-Loop.
- Rebase patch on next-20230927.
v1:
https://lore.kernel.org/r/20230926054200.963803-1-s-vadapalli@ti.com/
Changes since v1:
- Add code to release reference(s) to the phy(s) when
ks_pcie_enable_phy(ks_pcie) fails.
Regards,
Siddharth.
drivers/pci/controller/dwc/pci-keystone.c | 9 +++++++++
1 file changed, 9 insertions(+)
Comments
On 9/27/23 9:48 AM, 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 > Serdes instances, using 1 lane of each Serdes. In such a configuration, > if the reference clock for one Serdes is provided by the other Serdes, > it results in a race condition. After the Serdes providing the reference > clock is initialized by the PCI driver by invoking its PHY APIs, it is > not guaranteed that this Serdes remains powered on long enough for the > PHY APIs based initialization of the dependent Serdes. In such cases, > the PLL of the dependent Serdes fails to lock due to the absence of the > reference clock from the former Serdes which has been powered off by the > PM Core. > > Fix this by obtaining reference to the PHYs before invoking the PHY > initialization APIs and releasing reference after the initialization is > complete. Sounds reasonable. Acked-by: Ravi Gunasekaran <r-gunasekaran@ti.com> Ravi > > Fixes: 49229238ab47 ("PCI: keystone: Cleanup PHY handling") > Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> > --- > > NOTE: This patch is based on linux-next tagged next-20230927. > > v2: > https://lore.kernel.org/r/20230926063638.1005124-1-s-vadapalli@ti.com/ > > Changes since v2: > - Implement suggestion by Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > moving the phy_pm_runtime_put_sync() For-Loop section before the > return value of ks_pcie_enable_phy(ks_pcie) is checked, thereby > preventing duplication of the For-Loop. > - Rebase patch on next-20230927. > > v1: > https://lore.kernel.org/r/20230926054200.963803-1-s-vadapalli@ti.com/ > > Changes since v1: > - Add code to release reference(s) to the phy(s) when > ks_pcie_enable_phy(ks_pcie) fails. > > Regards, > Siddharth. > > drivers/pci/controller/dwc/pci-keystone.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c > index 49aea6ce3e87..0ec6720cc2df 100644 > --- a/drivers/pci/controller/dwc/pci-keystone.c > +++ b/drivers/pci/controller/dwc/pci-keystone.c > @@ -1218,7 +1218,16 @@ static int __init ks_pcie_probe(struct platform_device *pdev) > goto err_link; > } > > + /* 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]); > + > if (ret) { > dev_err(dev, "failed to enable phy\n"); > goto err_link;
Hello Bjorn, Could you please merge this patch? On 28/09/23 13:21, Ravi Gunasekaran wrote: > > > On 9/27/23 9:48 AM, 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 >> Serdes instances, using 1 lane of each Serdes. In such a configuration, >> if the reference clock for one Serdes is provided by the other Serdes, >> it results in a race condition. After the Serdes providing the reference >> clock is initialized by the PCI driver by invoking its PHY APIs, it is >> not guaranteed that this Serdes remains powered on long enough for the >> PHY APIs based initialization of the dependent Serdes. In such cases, >> the PLL of the dependent Serdes fails to lock due to the absence of the >> reference clock from the former Serdes which has been powered off by the >> PM Core. >> >> Fix this by obtaining reference to the PHYs before invoking the PHY >> initialization APIs and releasing reference after the initialization is >> complete. > > Sounds reasonable. > > Acked-by: Ravi Gunasekaran <r-gunasekaran@ti.com> > > Ravi >> >> Fixes: 49229238ab47 ("PCI: keystone: Cleanup PHY handling") >> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> >> --- >> >> NOTE: This patch is based on linux-next tagged next-20230927. >> >> v2: >> https://lore.kernel.org/r/20230926063638.1005124-1-s-vadapalli@ti.com/ >> >> Changes since v2: >> - Implement suggestion by Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> >> moving the phy_pm_runtime_put_sync() For-Loop section before the >> return value of ks_pcie_enable_phy(ks_pcie) is checked, thereby >> preventing duplication of the For-Loop. >> - Rebase patch on next-20230927. >> >> v1: >> https://lore.kernel.org/r/20230926054200.963803-1-s-vadapalli@ti.com/ >> >> Changes since v1: >> - Add code to release reference(s) to the phy(s) when >> ks_pcie_enable_phy(ks_pcie) fails. >> >> Regards, >> Siddharth. >> >> drivers/pci/controller/dwc/pci-keystone.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c >> index 49aea6ce3e87..0ec6720cc2df 100644 >> --- a/drivers/pci/controller/dwc/pci-keystone.c >> +++ b/drivers/pci/controller/dwc/pci-keystone.c >> @@ -1218,7 +1218,16 @@ static int __init ks_pcie_probe(struct platform_device *pdev) >> goto err_link; >> } >> >> + /* 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]); >> + >> if (ret) { >> dev_err(dev, "failed to enable phy\n"); >> goto err_link; >
Hello, > 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 > Serdes instances, using 1 lane of each Serdes. In such a configuration, > if the reference clock for one Serdes is provided by the other Serdes, > it results in a race condition. After the Serdes providing the reference > clock is initialized by the PCI driver by invoking its PHY APIs, it is > not guaranteed that this Serdes remains powered on long enough for the > PHY APIs based initialization of the dependent Serdes. In such cases, > the PLL of the dependent Serdes fails to lock due to the absence of the > reference clock from the former Serdes which has been powered off by the > PM Core. > > Fix this by obtaining reference to the PHYs before invoking the PHY > initialization APIs and releasing reference after the initialization is > complete. Applied to controller/keystone, thank you! [1/1] PCI: keystone: Fix race condition when initializing PHYs https://git.kernel.org/pci/pci/c/c12ca110c613 Krzysztof
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 > Serdes instances, using 1 lane of each Serdes. In such a configuration, > if the reference clock for one Serdes is provided by the other Serdes, > it results in a race condition. After the Serdes providing the reference > clock is initialized by the PCI driver by invoking its PHY APIs, it is > not guaranteed that this Serdes remains powered on long enough for the > PHY APIs based initialization of the dependent Serdes. In such cases, > the PLL of the dependent Serdes fails to lock due to the absence of the > reference clock from the former Serdes which has been powered off by the > PM Core. > > Fix this by obtaining reference to the PHYs before invoking the PHY > initialization APIs and releasing reference after the initialization is > complete. > > Fixes: 49229238ab47 ("PCI: keystone: Cleanup PHY handling") > Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> > --- > > NOTE: This patch is based on linux-next tagged next-20230927. > > v2: > https://lore.kernel.org/r/20230926063638.1005124-1-s-vadapalli@ti.com/ > > Changes since v2: > - Implement suggestion by Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > moving the phy_pm_runtime_put_sync() For-Loop section before the > return value of ks_pcie_enable_phy(ks_pcie) is checked, thereby > preventing duplication of the For-Loop. > - Rebase patch on next-20230927. > > v1: > https://lore.kernel.org/r/20230926054200.963803-1-s-vadapalli@ti.com/ > > Changes since v1: > - Add code to release reference(s) to the phy(s) when > ks_pcie_enable_phy(ks_pcie) fails. > > Regards, > Siddharth. > > drivers/pci/controller/dwc/pci-keystone.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c > index 49aea6ce3e87..0ec6720cc2df 100644 > --- a/drivers/pci/controller/dwc/pci-keystone.c > +++ b/drivers/pci/controller/dwc/pci-keystone.c > @@ -1218,7 +1218,16 @@ static int __init ks_pcie_probe(struct platform_device *pdev) > goto err_link; > } > > + /* 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(). 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? > if (ret) { > dev_err(dev, "failed to enable phy\n"); > goto err_link; > -- > 2.34.1 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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. ..
On Wed, Jan 10, 2024 at 11:35:24AM +0530, Siddharth Vadapalli wrote: > 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]); 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]); ... > 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;
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;
diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c index 49aea6ce3e87..0ec6720cc2df 100644 --- a/drivers/pci/controller/dwc/pci-keystone.c +++ b/drivers/pci/controller/dwc/pci-keystone.c @@ -1218,7 +1218,16 @@ static int __init ks_pcie_probe(struct platform_device *pdev) goto err_link; } + /* 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]); + if (ret) { dev_err(dev, "failed to enable phy\n"); goto err_link;