[PATCH 0/4] i.MX6 sleep while atomic bugfix

[PATCH 0/4] i.MX6 sleep while atomic bugfix

[Lucas Stach]

There have been some reports about a possible scheduling while atomic bug
in the i.MX6 PCIe host driver, like the following one
https://bugzilla.kernel.org/show_bug.cgi?id=100031

Although there were some proposed patches, they all had some flaws, which
lead to none of them being included in the mainline kernel. The following
patch series fixes this bug in a proper way, instead of papering over the
problem, making the fix a bit bigger.

Regards
Lucas

Lucas Stach (4):
  PCI: imx6: move PHY reset function to other PHY handling functions
  PCI: imx6: move PHY reset into establish_link
  PCI: imx6: remove broken Gen2 workaround
  PCI: imx6: move link up check into establish link path

 drivers/pci/host/pci-imx6.c | 137 +++++++++++++++++---------------------------
 1 file changed, 54 insertions(+), 83 deletions(-)

-- 
2.6.4

[PATCH 1/4] PCI: imx6: move PHY reset function to other PHY handling functions

[Lucas Stach]

This moves the PHY reset function to the other PHY related
functions in the file, which is mostly a cosmetic change, but also
allows to do the following changes without introducing needless
forward declarations.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/pci/host/pci-imx6.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
index 22e8224126fd..48ee2ed26be3 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -202,6 +202,23 @@ static int pcie_phy_write(void __iomem *dbi_base, int addr, int data)
 	return 0;
 }
 
+static void imx6_pcie_reset_phy(struct pcie_port *pp)
+{
+	u32 tmp;
+
+	pcie_phy_read(pp->dbi_base, PHY_RX_OVRD_IN_LO, &tmp);
+	tmp |= (PHY_RX_OVRD_IN_LO_RX_DATA_EN |
+		PHY_RX_OVRD_IN_LO_RX_PLL_EN);
+	pcie_phy_write(pp->dbi_base, PHY_RX_OVRD_IN_LO, tmp);
+
+	usleep_range(2000, 3000);
+
+	pcie_phy_read(pp->dbi_base, PHY_RX_OVRD_IN_LO, &tmp);
+	tmp &= ~(PHY_RX_OVRD_IN_LO_RX_DATA_EN |
+		  PHY_RX_OVRD_IN_LO_RX_PLL_EN);
+	pcie_phy_write(pp->dbi_base, PHY_RX_OVRD_IN_LO, tmp);
+}
+
 /*  Added for PCI abort handling */
 static int imx6q_pcie_abort_handler(unsigned long addr,
 		unsigned int fsr, struct pt_regs *regs)
@@ -441,23 +458,6 @@ static void imx6_pcie_host_init(struct pcie_port *pp)
 		dw_pcie_msi_init(pp);
 }
 
-static void imx6_pcie_reset_phy(struct pcie_port *pp)
-{
-	u32 tmp;
-
-	pcie_phy_read(pp->dbi_base, PHY_RX_OVRD_IN_LO, &tmp);
-	tmp |= (PHY_RX_OVRD_IN_LO_RX_DATA_EN |
-		PHY_RX_OVRD_IN_LO_RX_PLL_EN);
-	pcie_phy_write(pp->dbi_base, PHY_RX_OVRD_IN_LO, tmp);
-
-	usleep_range(2000, 3000);
-
-	pcie_phy_read(pp->dbi_base, PHY_RX_OVRD_IN_LO, &tmp);
-	tmp &= ~(PHY_RX_OVRD_IN_LO_RX_DATA_EN |
-		  PHY_RX_OVRD_IN_LO_RX_PLL_EN);
-	pcie_phy_write(pp->dbi_base, PHY_RX_OVRD_IN_LO, tmp);
-}
-
 static int imx6_pcie_link_up(struct pcie_port *pp)
 {
 	u32 rc, debug_r0, rx_valid;
-- 
2.6.4

[PATCH 2/4] PCI: imx6: move PHY reset into establish_link

[Lucas Stach]

This adds the PHY reset into a common error path of the
establish_link function, deduplicating some of the debug
prints. Also reduce the severity of the "no-link" message
in the one place where it is expected to be hit when no
peripheral is attached.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/pci/host/pci-imx6.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
index 48ee2ed26be3..3bb8ccc68792 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -355,10 +355,6 @@ static int imx6_pcie_wait_for_link(struct pcie_port *pp)
 		usleep_range(100, 1000);
 	}
 
-	dev_err(pp->dev, "phy link never came up\n");
-	dev_dbg(pp->dev, "DEBUG_R0: 0x%08x, DEBUG_R1: 0x%08x\n",
-		readl(pp->dbi_base + PCIE_PHY_DEBUG_R0),
-		readl(pp->dbi_base + PCIE_PHY_DEBUG_R1));
 	return -EINVAL;
 }
 
@@ -407,8 +403,10 @@ static int imx6_pcie_establish_link(struct pcie_port *pp)
 			IMX6Q_GPR12_PCIE_CTL_2, 1 << 10);
 
 	ret = imx6_pcie_wait_for_link(pp);
-	if (ret)
-		return ret;
+	if (ret) {
+		dev_info(pp->dev, "Link never came up\n");
+		goto err_reset_phy;
+	}
 
 	/* Allow Gen2 mode after the link is up. */
 	tmp = readl(pp->dbi_base + PCIE_RC_LCR);
@@ -427,19 +425,28 @@ static int imx6_pcie_establish_link(struct pcie_port *pp)
 	ret = imx6_pcie_wait_for_speed_change(pp);
 	if (ret) {
 		dev_err(pp->dev, "Failed to bring link up!\n");
-		return ret;
+		goto err_reset_phy;
 	}
 
 	/* Make sure link training is finished as well! */
 	ret = imx6_pcie_wait_for_link(pp);
 	if (ret) {
 		dev_err(pp->dev, "Failed to bring link up!\n");
-		return ret;
+		goto err_reset_phy;
 	}
 
 	tmp = readl(pp->dbi_base + PCIE_RC_LCSR);
 	dev_dbg(pp->dev, "Link up, Gen=%i\n", (tmp >> 16) & 0xf);
+
 	return 0;
+
+err_reset_phy:
+	dev_dbg(pp->dev, "PHY DEBUG_R0=0x%08x DEBUG_R1=0x%08x\n",
+		readl(pp->dbi_base + PCIE_PHY_DEBUG_R0),
+		readl(pp->dbi_base + PCIE_PHY_DEBUG_R1));
+	imx6_pcie_reset_phy(pp);
+
+	return ret;
 }
 
 static void imx6_pcie_host_init(struct pcie_port *pp)
@@ -510,11 +517,6 @@ static int imx6_pcie_link_up(struct pcie_port *pp)
 	if ((debug_r0 & 0x3f) != 0x0d)
 		return 0;
 
-	dev_err(pp->dev, "transition to gen2 is stuck, reset PHY!\n");
-	dev_dbg(pp->dev, "debug_r0=%08x debug_r1=%08x\n", debug_r0, rc);
-
-	imx6_pcie_reset_phy(pp);
-
 	return 0;
 }
 
-- 
2.6.4

[PATCH 3/4] PCI: imx6: remove broken Gen2 workaround

[Lucas Stach]

This removes the remnants of the workaround for erratum ERR005184
which was never completely implemented. The checks alone don't
carry any value as we don't act properly on the result.

A proper implementation of the workaround should be added to
the lane speed change in establish_link later.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/pci/host/pci-imx6.c | 17 +----------------
 1 file changed, 1 insertion(+), 16 deletions(-)

diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
index 3bb8ccc68792..f38aa7678868 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -467,7 +467,7 @@ static void imx6_pcie_host_init(struct pcie_port *pp)
 
 static int imx6_pcie_link_up(struct pcie_port *pp)
 {
-	u32 rc, debug_r0, rx_valid;
+	u32 rc;
 	int count = 5;
 
 	/*
@@ -501,21 +501,6 @@ static int imx6_pcie_link_up(struct pcie_port *pp)
 		 */
 		usleep_range(1000, 2000);
 	}
-	/*
-	 * From L0, initiate MAC entry to gen2 if EP/RC supports gen2.
-	 * Wait 2ms (LTSSM timeout is 24ms, PHY lock is ~5us in gen2).
-	 * If (MAC/LTSSM.state == Recovery.RcvrLock)
-	 * && (PHY/rx_valid==0) then pulse PHY/rx_reset. Transition
-	 * to gen2 is stuck
-	 */
-	pcie_phy_read(pp->dbi_base, PCIE_PHY_RX_ASIC_OUT, &rx_valid);
-	debug_r0 = readl(pp->dbi_base + PCIE_PHY_DEBUG_R0);
-
-	if (rx_valid & PCIE_PHY_RX_ASIC_OUT_VALID)
-		return 0;
-
-	if ((debug_r0 & 0x3f) != 0x0d)
-		return 0;
 
 	return 0;
 }
-- 
2.6.4

[PATCH 4/4] PCI: imx6: move link up check into establish link path

[Lucas Stach]

Instead of calling dw_pcie_link_up(), which just reflects into
our link_up() function, inline the relevant checks into the
wait_for_link() function.

This simplifies the code a lot and moves out any possible
sleeping waits from the link_up() hotpath, which may be called
with the config spinlock held, leaving only a lightweight
register read in this path.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/pci/host/pci-imx6.c | 60 +++++++++++++++++----------------------------
 1 file changed, 22 insertions(+), 38 deletions(-)

diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
index f38aa7678868..6ab4657dee57 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -349,10 +349,28 @@ static int imx6_pcie_wait_for_link(struct pcie_port *pp)
 {
 	unsigned int retries;
 
+	/*
+	 * Test if the PHY reports that the link is up and also that the LTSSM
+	 * training finished. There are three possible states of the link when
+	 * this code is called:
+	 * 1) The link is DOWN (unlikely)
+	 *    The link didn't come up yet for some reason. This usually means
+	 *    we have a real problem somewhere, if it happens with a peripheral
+	 *    connected. This state calls for inspection of the DEBUG registers.
+	 * 2) The link is UP, but still in LTSSM training
+	 *    Wait for the training to finish, which should take a very short
+	 *    time. If the training does not finish, we have a problem and we
+	 *    need to inspect the DEBUG registers. If the training does finish,
+	 *    the link is up and operating correctly.
+	 * 3) The link is UP and no longer in LTSSM training
+	 *    The link is up and operating correctly.
+	 */
 	for (retries = 0; retries < 200; retries++) {
-		if (dw_pcie_link_up(pp))
+		u32 reg = readl(pp->dbi_base + PCIE_PHY_DEBUG_R1);
+		if ((reg & PCIE_PHY_DEBUG_R1_XMLH_LINK_UP) &&
+		    !(reg & PCIE_PHY_DEBUG_R1_XMLH_LINK_IN_TRAINING))
 			return 0;
-		usleep_range(100, 1000);
+		usleep_range(1000, 2000);
 	}
 
 	return -EINVAL;
@@ -467,42 +485,8 @@ static void imx6_pcie_host_init(struct pcie_port *pp)
 
 static int imx6_pcie_link_up(struct pcie_port *pp)
 {
-	u32 rc;
-	int count = 5;
-
-	/*
-	 * Test if the PHY reports that the link is up and also that the LTSSM
-	 * training finished. There are three possible states of the link when
-	 * this code is called:
-	 * 1) The link is DOWN (unlikely)
-	 *     The link didn't come up yet for some reason. This usually means
-	 *     we have a real problem somewhere. Reset the PHY and exit. This
-	 *     state calls for inspection of the DEBUG registers.
-	 * 2) The link is UP, but still in LTSSM training
-	 *     Wait for the training to finish, which should take a very short
-	 *     time. If the training does not finish, we have a problem and we
-	 *     need to inspect the DEBUG registers. If the training does finish,
-	 *     the link is up and operating correctly.
-	 * 3) The link is UP and no longer in LTSSM training
-	 *     The link is up and operating correctly.
-	 */
-	while (1) {
-		rc = readl(pp->dbi_base + PCIE_PHY_DEBUG_R1);
-		if (!(rc & PCIE_PHY_DEBUG_R1_XMLH_LINK_UP))
-			break;
-		if (!(rc & PCIE_PHY_DEBUG_R1_XMLH_LINK_IN_TRAINING))
-			return 1;
-		if (!count--)
-			break;
-		dev_dbg(pp->dev, "Link is up, but still in training\n");
-		/*
-		 * Wait a little bit, then re-check if the link finished
-		 * the training.
-		 */
-		usleep_range(1000, 2000);
-	}
-
-	return 0;
+	return readl(pp->dbi_base + PCIE_PHY_DEBUG_R1) &
+			PCIE_PHY_DEBUG_R1_XMLH_LINK_UP;
 }
 
 static struct pcie_host_ops imx6_pcie_host_ops = {
-- 
2.6.4

Re: [PATCH 0/4] i.MX6 sleep while atomic bugfix

[Bjorn Helgaas]

On Fri, Jan 15, 2016 at 07:56:46PM +0100, Lucas Stach wrote:
> There have been some reports about a possible scheduling while atomic bug
> in the i.MX6 PCIe host driver, like the following one
> https://bugzilla.kernel.org/show_bug.cgi?id=100031
> 
> Although there were some proposed patches, they all had some flaws, which
> lead to none of them being included in the mainline kernel. The following
> patch series fixes this bug in a proper way, instead of papering over the
> problem, making the fix a bit bigger.
> 
> Regards
> Lucas
> 
> Lucas Stach (4):
>   PCI: imx6: move PHY reset function to other PHY handling functions
>   PCI: imx6: move PHY reset into establish_link
>   PCI: imx6: remove broken Gen2 workaround
>   PCI: imx6: move link up check into establish link path
> 
>  drivers/pci/host/pci-imx6.c | 137 +++++++++++++++++---------------------------
>  1 file changed, 54 insertions(+), 83 deletions(-)

Applied to pci/host-imx6 for v4.6, thanks!

Re: [PATCH 0/4] i.MX6 sleep while atomic bugfix

[Bjorn Helgaas]

[+cc bugzilla folks]

On Mon, Jan 25, 2016 at 04:58:11PM -0600, Bjorn Helgaas wrote:
> On Fri, Jan 15, 2016 at 07:56:46PM +0100, Lucas Stach wrote:
> > There have been some reports about a possible scheduling while atomic bug
> > in the i.MX6 PCIe host driver, like the following one
> > https://bugzilla.kernel.org/show_bug.cgi?id=100031
> > 
> > Although there were some proposed patches, they all had some flaws, which
> > lead to none of them being included in the mainline kernel. The following
> > patch series fixes this bug in a proper way, instead of papering over the
> > problem, making the fix a bit bigger.
> > 
> > Regards
> > Lucas
> > 
> > Lucas Stach (4):
> >   PCI: imx6: move PHY reset function to other PHY handling functions
> >   PCI: imx6: move PHY reset into establish_link
> >   PCI: imx6: remove broken Gen2 workaround
> >   PCI: imx6: move link up check into establish link path
> > 
> >  drivers/pci/host/pci-imx6.c | 137 +++++++++++++++++---------------------------
> >  1 file changed, 54 insertions(+), 83 deletions(-)
> 
> Applied to pci/host-imx6 for v4.6, thanks!