linux-kernel - Re: [PATCH v1 3/3] PCI: imx6: reset link on resume

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: <ZsRAJrWTxKCol7-g@eichest-laptop>
Date: Tue, 20 Aug 2024 09:05:10 +0200
From: Stefan Eichenberger <eichest@...il.com>
To: Frank Li <Frank.li@....com>
Cc: hongxing.zhu@....com, l.stach@...gutronix.de, lpieralisi@...nel.org,
	kw@...ux.com, robh@...nel.org, bhelgaas@...gle.com,
	shawnguo@...nel.org, s.hauer@...gutronix.de, kernel@...gutronix.de,
	festevam@...il.com, francesco.dolcini@...adex.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 v1 3/3] PCI: imx6: reset link on resume

On Mon, Aug 19, 2024 at 10:39:45AM -0400, Frank Li wrote:
> On Mon, Aug 19, 2024 at 11:03:19AM +0200, Stefan Eichenberger wrote:
> > From: Stefan Eichenberger <stefan.eichenberger@...adex.com>
> >
> > According to the https://www.nxp.com/docs/en/errata/IMX6DQCE.pdf errata,
> 
> Can you show errata number here?
> 

I will include it in the next version of the patch. If I understand it
correct it is ERR005723.

> > the i.MX6Q PCIe controller does not support suspend/resume. So suspend
> > and resume was omitted. However, this does not seem to work because it
> > looks like the PCIe link is still expecting a reset. If we do not reset
> > the link, we end up with a frozen system after resume. The last message
> > we see is:
> > ath10k_pci 0000:01:00.0: Unable to change power state from D3hot to D0,
> > device inaccessible
> >
> > Besides resetting the link, we also need to enable msi again, otherwise
> > DMA access will not work and we can still end up with a frozen system.
> > With these changes we can suspend and resume the system properly with a
> > PCIe device attached. This was tested with a Compex WLE900VX miniPCIe
> > Wifi module.
> >
> > Signed-off-by: Stefan Eichenberger <stefan.eichenberger@...adex.com>
> > ---
> >  drivers/pci/controller/dwc/pci-imx6.c | 45 ++++++++++++++++++++++++++-
> >  1 file changed, 44 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> > index f17561791e35a..751243f4c519e 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -1213,14 +1213,57 @@ static int imx6_pcie_suspend_noirq(struct device *dev)
> >  	return 0;
> >  }
> >
> > +static int imx6_pcie_reset_link(struct imx6_pcie *imx6_pcie)
> > +{
> > +	int ret;
> > +
> > +	regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
> > +			   IMX6Q_GPR1_PCIE_TEST_PD, 1 << 18);
> > +	regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
> > +			   IMX6Q_GPR1_PCIE_REF_CLK_EN, 0 << 16);
> > +
> > +	/* Reset the PCIe device */
> > +	gpiod_set_value_cansleep(imx6_pcie->reset_gpiod, 1);
> > +
> > +	ret = imx6_pcie_enable_ref_clk(imx6_pcie);
> > +	if (ret) {
> > +		dev_err(imx6_pcie->pci->dev, "unable to enable pcie ref clock\n");
> > +		return ret;
> > +	}
> > +
> > +	imx6_pcie_deassert_reset_gpio(imx6_pcie);
> 
> In my patch https://lore.kernel.org/linux-pci/Zr4XG6r+HnbIlu8S@lizhi-Precision-Tower-5810/T/#mc5f38934b6cef95eca90f1a6a63b3193e45179de
> 
> imx6qp_pcie_core_reset() and imx6q_pcie_core_reset() is not symatic for
> assert/desert() to match origin code. I plan fix it after above patch
> merged.
> 
> Does it work if make above code symatic?
> 

I will give it a try with your patches applied and let you know.

> > +
> > +	/*
> > +	 * Setup the root complex again and enable msi. Without this PCIe will
> > +	 * not work in msi mode and drivers will crash if they try to access
> > +	 * the device memory area
> > +	 */
> > +	dw_pcie_setup_rc(&imx6_pcie->pci->pp);
> > +	if (pci_msi_enabled()) {
> > +		u32 val;
> > +		u8 offset = dw_pcie_find_capability(imx6_pcie->pci, PCI_CAP_ID_MSI);
> > +
> > +		val = dw_pcie_readw_dbi(imx6_pcie->pci, offset + PCI_MSI_FLAGS);
> > +		val |= PCI_MSI_FLAGS_ENABLE;
> > +		dw_pcie_writew_dbi(imx6_pcie->pci, offset + PCI_MSI_FLAGS, val);
> > +	}
> 
> there are already have imx6_pcie_msi_save_restore(imx6_pcie, true); in
> suspend/resume, why need addtional one here?
> 

I took the part from the probe function and added it here. I will see if
I can rework that part.

> > +
> > +	return 0;
> > +}
> > +
> >  static int imx6_pcie_resume_noirq(struct device *dev)
> >  {
> >  	int ret;
> >  	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> >  	struct dw_pcie_rp *pp = &imx6_pcie->pci->pp;
> >
> > +	/*
> > +	 * Even though the i.MX6Q does not support suspend/resume, we need to
> > +	 * reset the link after resume or the memory mapped PCIe I/O space will
> > +	 * be inaccessible. This will cause the system to freeze.
> > +	 */
> >  	if (!(imx6_pcie->drvdata->flags & IMX6_PCIE_FLAG_SUPPORTS_SUSPEND))
> > -		return 0;
> > +		return imx6_pcie_reset_link(imx6_pcie);
> 
> If reset everything, I supposed we can add IMX6_PCIE_FLAG_SUPPORTS_SUSPEND
> at driver data.
> 

This didn't work for the current version. It seems we do too much in the
suspend function and therefore it is still not working. However, maybe
it is really better to just add the flag and try to make suspend/resume
work by using the function pointers you introduced. I will have a look
at it, thanks.

> >
> >  	ret = imx6_pcie_host_init(pp);
> >  	if (ret)
> > --
> > 2.43.0
> >