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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240119232032.GA192245@bhelgaas>
Date: Fri, 19 Jan 2024 17:20:32 -0600
From: Bjorn Helgaas <helgaas@...nel.org>
To: Siddharth Vadapalli <s-vadapalli@...com>
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
Subject: Re: [PATCH v3] PCI: keystone: Fix race condition when initializing
 PHYs

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;