| 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
| ||
|
Message-ID: <20241022155349.GA880566@bhelgaas>
Date: Tue, 22 Oct 2024 10:53:49 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: Stefan Eichenberger <eichest@...il.com>
Cc: hongxing.zhu@....com, l.stach@...gutronix.de, lpieralisi@...nel.org,
kw@...ux.com, manivannan.sadhasivam@...aro.org, robh@...nel.org,
bhelgaas@...gle.com, shawnguo@...nel.org, s.hauer@...gutronix.de,
kernel@...gutronix.de, festevam@...il.com,
francesco.dolcini@...adex.com, Frank.li@....com,
linux-pci@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
imx@...ts.linux.dev, linux-kernel@...r.kernel.org,
Stefan Eichenberger <stefan.eichenberger@...adex.com>
Subject: Re: [PATCH v3] PCI: imx6: Add suspend/resume support for i.MX6QDL
On Mon, Oct 21, 2024 at 02:49:13PM +0200, Stefan Eichenberger wrote:
> From: Stefan Eichenberger <stefan.eichenberger@...adex.com>
>
> The suspend/resume support is broken on the i.MX6QDL platform. This
> patch resets the link upon resuming to recover functionality. It shares
> most of the sequences with other i.MX devices but does not touch the
> critical registers, which might break PCIe. This patch addresses the
> same issue as the following downstream commit:
> https://github.com/nxp-imx/linux-imx/commit/4e92355e1f79d225ea842511fcfd42b343b32995
> In comparison this patch will also reset the device if possible because
> the downstream patch alone would still make the ath10k driver crash.
> Without this patch suspend/resume will not work if a PCIe device is
> connected. The kernel will hang on resume and print an error:
> ath10k_pci 0000:01:00.0: Unable to change power state from D3hot to D0, device inaccessible
> 8<--- cut here ---
> Unhandled fault: imprecise external abort (0x1406) at 0x0106f944
https://chris.beams.io/posts/git-commit/
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/maintainer-tip.rst?id=v6.11#n134
Add blank lines between paragraphs. Drop the "8<--- cut here" thing.
What does "reset the link" mean? Please use the same terminology as
the PCIe spec when possible.
The downstream commit log ("WARNING: this is not the official
workaround; user should take own risk to use it") doesn't exactly
inspire confidence.
It sounds like this resets *endpoints*? That sounds scary and
unexpected in suspend/resume.
> Signed-off-by: Stefan Eichenberger <stefan.eichenberger@...adex.com>
> ---
> Changes in v3:
> - Added a new flag to the driver data to indicate that the suspend/resume
> is broken on the i.MX6QDL platform. (Frank)
> - Fix comments to be more relevant (Mani)
> - Use imx_pcie_assert_core_reset in suspend (Mani)
>
> drivers/pci/controller/dwc/pci-imx6.c | 57 +++++++++++++++++++++------
> 1 file changed, 46 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 808d1f1054173..09e3b15f0908a 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -82,6 +82,11 @@ enum imx_pcie_variants {
> #define IMX_PCIE_FLAG_HAS_SERDES BIT(6)
> #define IMX_PCIE_FLAG_SUPPORT_64BIT BIT(7)
> #define IMX_PCIE_FLAG_CPU_ADDR_FIXUP BIT(8)
> +/**
> + * Because of ERR005723 (PCIe does not support L2 power down) we need to
> + * workaround suspend resume on some devices which are affected by this errata.
> + */
> +#define IMX_PCIE_FLAG_BROKEN_SUSPEND BIT(9)
>
> #define imx_check_flag(pci, val) (pci->drvdata->flags & val)
>
> @@ -1237,9 +1242,19 @@ static int imx_pcie_suspend_noirq(struct device *dev)
> return 0;
>
> imx_pcie_msi_save_restore(imx_pcie, true);
> - imx_pcie_pm_turnoff(imx_pcie);
> - imx_pcie_stop_link(imx_pcie->pci);
> - imx_pcie_host_exit(pp);
> + if (imx_check_flag(imx_pcie, IMX_PCIE_FLAG_BROKEN_SUSPEND)) {
> + /**
Single asterisks above, here, and below.
> + * The minimum for a workaround would be to set PERST# and to
> + * set the PCIE_TEST_PD flag. However, we can also disable the
> + * clock which saves some power.
> + */
> + imx_pcie_assert_core_reset(imx_pcie);
> + imx_pcie->drvdata->enable_ref_clk(imx_pcie, false);
> + } else {
> + imx_pcie_pm_turnoff(imx_pcie);
> + imx_pcie_stop_link(imx_pcie->pci);
> + imx_pcie_host_exit(pp);
> + }
>
> return 0;
> }
> @@ -1253,14 +1268,32 @@ static int imx_pcie_resume_noirq(struct device *dev)
> if (!(imx_pcie->drvdata->flags & IMX_PCIE_FLAG_SUPPORTS_SUSPEND))
> return 0;
>
> - ret = imx_pcie_host_init(pp);
> - if (ret)
> - return ret;
> - imx_pcie_msi_save_restore(imx_pcie, false);
> - dw_pcie_setup_rc(pp);
> + if (imx_check_flag(imx_pcie, IMX_PCIE_FLAG_BROKEN_SUSPEND)) {
> + ret = imx_pcie->drvdata->enable_ref_clk(imx_pcie, true);
> + if (ret)
> + return ret;
> + ret = imx_pcie_deassert_core_reset(imx_pcie);
> + if (ret)
> + return ret;
> + /**
> + * Using PCIE_TEST_PD seems to disable msi and powers down the
> + * root complex. This is why we have to setup the rc again and
> + * why we have to restore the msi register.
s/msi/MSI/
> + */
> + ret = dw_pcie_setup_rc(&imx_pcie->pci->pp);
> + if (ret)
> + return ret;
> + imx_pcie_msi_save_restore(imx_pcie, false);
> + } else {
> + ret = imx_pcie_host_init(pp);
> + if (ret)
> + return ret;
> + imx_pcie_msi_save_restore(imx_pcie, false);
> + dw_pcie_setup_rc(pp);
>
> - if (imx_pcie->link_is_up)
> - imx_pcie_start_link(imx_pcie->pci);
> + if (imx_pcie->link_is_up)
> + imx_pcie_start_link(imx_pcie->pci);
> + }
>
> return 0;
> }
> @@ -1485,7 +1518,9 @@ static const struct imx_pcie_drvdata drvdata[] = {
> [IMX6Q] = {
> .variant = IMX6Q,
> .flags = IMX_PCIE_FLAG_IMX_PHY |
> - IMX_PCIE_FLAG_IMX_SPEED_CHANGE,
> + IMX_PCIE_FLAG_IMX_SPEED_CHANGE |
> + IMX_PCIE_FLAG_BROKEN_SUSPEND |
> + IMX_PCIE_FLAG_SUPPORTS_SUSPEND,
> .dbi_length = 0x200,
> .gpr = "fsl,imx6q-iomuxc-gpr",
> .clk_names = imx6q_clks,
> --
> 2.43.0
>
Powered by blists - more mailing lists