linux-kernel - Re: [PATCH v4 1/2] PCI: dwc: Always stop link in the dw_pcie_suspend

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: <20250114181518.GA473181@bhelgaas>
Date: Tue, 14 Jan 2025 12:15:18 -0600
From: Bjorn Helgaas <helgaas@...nel.org>
To: Richard Zhu <hongxing.zhu@....com>
Cc: dlemoal@...nel.org, jingoohan1@...il.com, bhelgaas@...gle.com,
	lpieralisi@...nel.org, kw@...ux.com,
	manivannan.sadhasivam@...aro.org, robh@...nel.org, frank.li@....com,
	quic_krichai@...cinc.com, imx@...ts.linux.dev,
	kernel@...gutronix.de, linux-pci@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 1/2] PCI: dwc: Always stop link in the
 dw_pcie_suspend_noirq

On Tue, Dec 10, 2024 at 04:15:56PM +0800, Richard Zhu wrote:
> On i.MX8QM, PCIe link can't be re-established again in
> dw_pcie_resume_noirq(), if the LTSSM_EN bit is not cleared properly in
> dw_pcie_suspend_noirq().
>
> Add dw_pcie_stop_link() into dw_pcie_suspend_noirq() to fix this issue and
> keep symmetric in suspend/resume function since there is
> dw_pcie_start_link() in dw_pcie_resume_noirq().
> 
> Fixes: 4774faf854f5 ("PCI: dwc: Implement generic suspend/resume functionality")
> Signed-off-by: Richard Zhu <hongxing.zhu@....com>
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
> ---
>  drivers/pci/controller/dwc/pcie-designware-host.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index f882b11fd7b94..f56cb7b9e6f99 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -1001,6 +1001,7 @@ int dw_pcie_suspend_noirq(struct dw_pcie *pci)
>  		return ret;
>  	}
>  
> +	dw_pcie_stop_link(pci);

We should try to avoid changes to the generic DWC path just to
accommodate one controller.  Since other DWC-based controllers
apparently don't need dw_pcie_stop_link() here, this seems like it
might be the wrong place for this change.

If doing dw_pcie_stop_link() here is really helpful for all DWC
controllers, this would be fine, but the commit log should then explain
why it helps everybody, not why one particular controller benefits.

If it's only needed for i.MX8QM, we do already have a
controller-specific hook (.deinit()) just below; maybe that could be
used?

>  	if (pci->pp.ops->deinit)
>  		pci->pp.ops->deinit(&pci->pp);
>  
> -- 
> 2.37.1
>