Received-SPF: pass (google.com: domain of linux-kernel+bounces-31626-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Fri, 19 Jan 2024 17:20:32 -0600
From: Bjorn Helgaas <helgaas@kernel.org>
To: Siddharth Vadapalli <s-vadapalli@ti.com>
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
Subject: Re: [PATCH v3] PCI: keystone: Fix race condition when initializing
 PHYs
Message-ID: <20240119232032.GA192245@bhelgaas>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <f8dbbffd-c209-44bc-8d1e-42b6f1b08aef@ti.com>

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;