linux-kernel - Re: [PATCH v2] PCI: mediatek-gen3: Assert MAC reset only if PHY reset also present

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: <30824df32636dec25b9a5972b1ee8de76b295feb.camel@mediatek.com>
Date: Fri, 1 Mar 2024 02:48:55 +0000
From: Jianjun Wang (王建军) <Jianjun.Wang@...iatek.com>
To: "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
	"angelogioacchino.delregno@...labora.com"
	<angelogioacchino.delregno@...labora.com>
CC: "linux-mediatek@...ts.infradead.org" <linux-mediatek@...ts.infradead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"wenst@...omium.org" <wenst@...omium.org>, "kernel@...labora.com"
	<kernel@...labora.com>, "p.zabel@...gutronix.de" <p.zabel@...gutronix.de>,
	"robh@...nel.org" <robh@...nel.org>, "kw@...ux.com" <kw@...ux.com>,
	"linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "bhelgaas@...gle.com"
	<bhelgaas@...gle.com>, "matthias.bgg@...il.com" <matthias.bgg@...il.com>,
	"lpieralisi@...nel.org" <lpieralisi@...nel.org>, Ryder Lee
	<Ryder.Lee@...iatek.com>, "nfraprado@...labora.com" <nfraprado@...labora.com>
Subject: Re: [PATCH v2] PCI: mediatek-gen3: Assert MAC reset only if PHY reset
 also present

Hi Angelo,

Thanks for your patch.

On Thu, 2024-02-29 at 10:24 +0100, AngeloGioacchino Del Regno wrote:
> Some SoCs have two PCI-Express controllers: in the case of MT8195,
> one of them is using a dedicated PHY, but the other uses a combo PHY
> that is shared with USB and in that case the PHY cannot be reset
> from the PCIe driver, or USB functionality will be unable to resume.
> 
> Resetting the PCIe MAC without also resetting the PHY will result in
> a full system lockup at PCIe resume time and the only option to
> resume operation is to hard reboot the system (with a PMIC cut-off).
> 
> To resolve this issue, check if we've got both a PHY and a MAC reset
> and, if not, never assert resets at PM suspend time: in that case,
> the link is still getting powered down as both the clocks and the
> power domains will go down anyway.
> 
> Fixes: d537dc125f07 ("PCI: mediatek-gen3: Add system PM support")
> Signed-off-by: AngeloGioacchino Del Regno <
> angelogioacchino.delregno@...labora.com>
> ---
> 
> Changes in v2:
>  - Rebased over next-20240229
> 
>  drivers/pci/controller/pcie-mediatek-gen3.c | 25 ++++++++++++++-----
> --
>  1 file changed, 17 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c
> b/drivers/pci/controller/pcie-mediatek-gen3.c
> index 975b3024fb08..99b5d7a49be1 100644
> --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> @@ -874,17 +874,26 @@ static int mtk_pcie_power_up(struct
> mtk_gen3_pcie *pcie)
>  	return err;
>  }
>  
> -static void mtk_pcie_power_down(struct mtk_gen3_pcie *pcie)
> +static void mtk_pcie_power_down(struct mtk_gen3_pcie *pcie, bool
> is_suspend)
>  {
> +	bool suspend_reset_supported = pcie->mac_reset && pcie-
> >phy_reset;
> +
>  	clk_bulk_disable_unprepare(pcie->num_clks, pcie->clks);
>  
>  	pm_runtime_put_sync(pcie->dev);
>  	pm_runtime_disable(pcie->dev);
> -	reset_control_assert(pcie->mac_reset);
> +
> +	/*
> +	 * Assert MAC reset only if we also got a PHY reset, otherwise
> +	 * the system will lockup at PM resume time.
> +	 */
> +	if (is_suspend && suspend_reset_supported)
> +		reset_control_assert(pcie->mac_reset);
>  
>  	phy_power_off(pcie->phy);
>  	phy_exit(pcie->phy);
> -	reset_control_assert(pcie->phy_reset);
> +	if (is_suspend && suspend_reset_supported)
> +		reset_control_assert(pcie->phy_reset);
>  }
>  
>  static int mtk_pcie_setup(struct mtk_gen3_pcie *pcie)
> @@ -920,7 +929,7 @@ static int mtk_pcie_setup(struct mtk_gen3_pcie
> *pcie)
>  	return 0;
>  
>  err_setup:
> -	mtk_pcie_power_down(pcie);
> +	mtk_pcie_power_down(pcie, false);
>  
>  	return err;
>  }
> @@ -951,7 +960,7 @@ static int mtk_pcie_probe(struct platform_device
> *pdev)
>  	err = pci_host_probe(host);
>  	if (err) {
>  		mtk_pcie_irq_teardown(pcie);
> -		mtk_pcie_power_down(pcie);
> +		mtk_pcie_power_down(pcie, false);
>  		return err;
>  	}
>  
> @@ -969,7 +978,7 @@ static void mtk_pcie_remove(struct
> platform_device *pdev)
>  	pci_unlock_rescan_remove();
>  
>  	mtk_pcie_irq_teardown(pcie);
> -	mtk_pcie_power_down(pcie);
> +	mtk_pcie_power_down(pcie, false);

Is there any reason not to reset the MAC and PHY when probe fails and
driver removing? Some SoCs may not have MTCMOS to cut off their power,
we need to assert the reset signal to save power in that case.

Thanks.

>  }
>  
>  static void mtk_pcie_irq_save(struct mtk_gen3_pcie *pcie)
> @@ -1044,7 +1053,7 @@ static int mtk_pcie_suspend_noirq(struct device
> *dev)
>  	dev_dbg(pcie->dev, "entered L2 states successfully");
>  
>  	mtk_pcie_irq_save(pcie);
> -	mtk_pcie_power_down(pcie);
> +	mtk_pcie_power_down(pcie, true);
>  
>  	return 0;
>  }
> @@ -1060,7 +1069,7 @@ static int mtk_pcie_resume_noirq(struct device
> *dev)
>  
>  	err = mtk_pcie_startup_port(pcie);
>  	if (err) {
> -		mtk_pcie_power_down(pcie);
> +		mtk_pcie_power_down(pcie, false);
>  		return err;
>  	}
>