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

[Bjorn Helgaas]

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;