| Message ID | 20230926063638.1005124-1-s-vadapalli@ti.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] PCI: keystone: Fix race condition when initializing PHYs | expand |
On Tue, 26 Sep 2023, 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-20230925. > > 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 | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c > index 49aea6ce3e87..e4d43306a7e3 100644 > --- a/drivers/pci/controller/dwc/pci-keystone.c > +++ b/drivers/pci/controller/dwc/pci-keystone.c > @@ -1218,12 +1218,24 @@ 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); You could do the put loop here before checking ret to avoid duplicating the put loop. > if (ret) { > dev_err(dev, "failed to enable phy\n"); > + /* Release reference(s) to the phy(s) */ > + for (i = 0; i < num_lanes; i++) > + phy_pm_runtime_put_sync(ks_pcie->phy[i]); > + > goto err_link; > } > > + /* Release reference(s) to the phy(s) */ > + for (i = 0; i < num_lanes; i++) > + phy_pm_runtime_put_sync(ks_pcie->phy[i]); > + > platform_set_drvdata(pdev, ks_pcie); > pm_runtime_enable(dev); > ret = pm_runtime_get_sync(dev); >
On 26/09/23 22:08, Ilpo Järvinen wrote: > On Tue, 26 Sep 2023, 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-20230925. >> >> 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 | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c >> index 49aea6ce3e87..e4d43306a7e3 100644 >> --- a/drivers/pci/controller/dwc/pci-keystone.c >> +++ b/drivers/pci/controller/dwc/pci-keystone.c >> @@ -1218,12 +1218,24 @@ 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); > > You could do the put loop here before checking ret to avoid duplicating > the put loop. Right! Thank you for reviewing the patch and sharing this suggestion. I will implement this and post the v3 patch. > >> if (ret) { >> dev_err(dev, "failed to enable phy\n"); >> + /* Release reference(s) to the phy(s) */ >> + for (i = 0; i < num_lanes; i++) >> + phy_pm_runtime_put_sync(ks_pcie->phy[i]); >> + >> goto err_link; >> } >> >> + /* Release reference(s) to the phy(s) */ >> + for (i = 0; i < num_lanes; i++) >> + phy_pm_runtime_put_sync(ks_pcie->phy[i]); >> + >> platform_set_drvdata(pdev, ks_pcie); >> pm_runtime_enable(dev); >> ret = pm_runtime_get_sync(dev); >> >
diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c index 49aea6ce3e87..e4d43306a7e3 100644 --- a/drivers/pci/controller/dwc/pci-keystone.c +++ b/drivers/pci/controller/dwc/pci-keystone.c @@ -1218,12 +1218,24 @@ 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); if (ret) { dev_err(dev, "failed to enable phy\n"); + /* Release reference(s) to the phy(s) */ + for (i = 0; i < num_lanes; i++) + phy_pm_runtime_put_sync(ks_pcie->phy[i]); + goto err_link; } + /* Release reference(s) to the phy(s) */ + for (i = 0; i < num_lanes; i++) + phy_pm_runtime_put_sync(ks_pcie->phy[i]); + platform_set_drvdata(pdev, ks_pcie); pm_runtime_enable(dev); ret = pm_runtime_get_sync(dev);
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-20230925. 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 | 12 ++++++++++++ 1 file changed, 12 insertions(+)