PCI: imx6: fix downstream bus scanning

The change in Linux 4.12 to make PCI configuartion requests non-posted means that we are now getting a synchronous abort when the CFG space read to probe for downstream devices times out.

Synchronous aborts need to be handled differently from the async aborts we were getting before, in particular the PC needs to be advanced when resolving the abort. This is mostly a copy of what other PCI drivers do on ARM to handle those aborts.

Signed-off-by Lucas Stach <[email protected]>

---
This is a fix that needs to go in for 4.12, but I would hope to get some thorough testing before.

Hi Lucas,

Quoted 9 lines · Lucas Stach

The change in Linux 4.12 to make PCI configuartion requests non-posted means that we are now getting a synchronous abort when the CFG space read to probe for downstream devices times out.

Synchronous aborts need to be handled differently from the async aborts we were getting before, in particular the PC needs to be advanced when resolving the abort. This is mostly a copy of what other PCI drivers do on ARM to handle those aborts.

Quoted 4 lines

Signed-off-by Lucas Stach <[email protected]>

---
This is a fix that needs to go in for 4.12, but I would hope to get some thorough testing before.

This fixes the kernel crash on my mx6q board with a PCI switch, thanks!

PCI Wifi card is also correctly detected.

The change in Linux 4.12 to make PCI configuartion requests non-posted means that we are now getting a synchronous abort when the CFG space read to probe for downstream devices times out.

Synchronous aborts need to be handled differently from the async aborts we were getting before, in particular the PC needs to be advanced when resolving the abort. This is mostly a copy of what other PCI drivers do on ARM to handle those aborts.

Signed-off-by Lucas Stach <[email protected]>

---
This is a fix that needs to go in for 4.12, but I would hope to get some thorough testing before.

---
 drivers/pci/dwc/pci-imx6.c | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

drivers/pci/dwc/pci-imx6.c +30 -3

index a98cba55c7f0..19a289b8cc94 100644
@@ -252,7 +252,34 @@ static void imx6_pcie_reset_phy(struct imx6_pcie *imx6_pcie)
 static int imx6q_pcie_abort_handler(unsigned long addr,
 		unsigned int fsr, struct pt_regs *regs)
 {
-	return 0;
+	unsigned long pc = instruction_pointer(regs);
+	unsigned long instr = *(unsigned long *)pc;
+	int reg = (instr >> 12) & 15;
+
+	/*
+	 * If the instruction being executed was a read,
+	 * make it look like it read all-ones.
+	 */
+	if ((instr & 0x0c100000) == 0x04100000) {
+		unsigned long val;
+
+		if (instr & 0x00400000)
+			val = 255;
+		else
+			val = -1;
+
+		regs->uregs[reg] = val;
+		regs->ARM_pc += 4;
+		return 0;
+	}
+
+	if ((instr & 0x0e100090) == 0x00100090) {
+		regs->uregs[reg] = -1;
+		regs->ARM_pc += 4;
+		return 0;
+	}
+
+	return 1;
 }
 
 static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)

@@ -819,8 +846,8 @@ static int __init imx6_pcie_init(void)
 	 * we can install the handler here without risking it
 	 * accessing some uninitialized driver state.
 	 */
-	hook_fault_code(16 + 6, imx6q_pcie_abort_handler, SIGBUS, 0,
-			"imprecise external abort");
+	hook_fault_code(8, imx6q_pcie_abort_handler, SIGBUS, 0,
+			"external abort on non-linefetch");
 
 	return platform_driver_register(&imx6_pcie_driver);
 }

Signature · 1 line

2.11.0

-----Original Message-----
From: Lucas Stach [mailto:l.stach at pengutronix.de]
Sent: Thursday, May 11, 2017 1:58 AM
To: linux-pci at vger.kernel.org
Cc: Peter Senna Tschudin <[email protected]>; Richard Zhu <[email protected]>; festevam at gmail.com; bhelgaas at google.com; tharvey at gateworks.com; lorenzo.pieralisi at arm.com; linux-arm- kernel at lists.infradead.org; kernel at pengutronix.de; patchwork- lst at pengutronix.de
Subject: [PATCH] PCI: imx6: fix downstream bus scanning

The change in Linux 4.12 to make PCI configuartion requests non-posted means that we are now getting a synchronous abort when the CFG space read to probe for downstream devices times out.

Synchronous aborts need to be handled differently from the async aborts we were getting before, in particular the PC needs to be advanced when resolving the abort. This is mostly a copy of what other PCI drivers do on ARM to handle those aborts.

Signed-off-by Lucas Stach <[email protected]>

---
This is a fix that needs to go in for 4.12, but I would hope to get some thorough testing before.

---
 drivers/pci/dwc/pci-imx6.c | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

drivers/pci/dwc/pci-imx6.c index renamed from drivers/pci/dwc/pci-imx6.c +0 -0

a98cba55c7f0..19a289b8cc94 100644

drivers/pci/dwc/pci-imx6.c +28 -1

@@ -252,7 +252,34 @@ static void imx6_pcie_reset_phy(struct imx6_pcie
 *imx6_pcie)  static int imx6q_pcie_abort_handler(unsigned long addr,
 		unsigned int fsr, struct pt_regs *regs)  {
-	return 0;
+	unsigned long pc = instruction_pointer(regs);
+	unsigned long instr = *(unsigned long *)pc;
+	int reg = (instr >> 12) & 15;
+
+	/*
+	 * If the instruction being executed was a read,
+	 * make it look like it read all-ones.
+	 */
+	if ((instr & 0x0c100000) == 0x04100000) {
+		unsigned long val;
+
+		if (instr & 0x00400000)
+			val = 255;
+		else
+			val = -1;
+
+		regs->uregs[reg] = val;
+		regs->ARM_pc += 4;
+		return 0;
+	}
+
+	if ((instr & 0x0e100090) == 0x00100090) {
+		regs->uregs[reg] = -1;
+		regs->ARM_pc += 4;
+		return 0;
+	}
+
+	return 1;
 }
 
 static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie) @@ -
 819,8 +846,8 @@ static int __init imx6_pcie_init(void)

drivers/pci/dwc/pci-imx6.c +2 -3

 	 * we can install the handler here without risking it
 	 * accessing some uninitialized driver state.
 	 */
-	hook_fault_code(16 + 6, imx6q_pcie_abort_handler, SIGBUS, 0,
-			"imprecise external abort");
+	hook_fault_code(8, imx6q_pcie_abort_handler, SIGBUS, 0,
+			"external abort on non-linefetch");
 
 	return platform_driver_register(&imx6_pcie_driver);
 }
--

2.11.0

The change in Linux 4.12 to make PCI configuartion requests non-posted means that we are now getting a synchronous abort when the CFG space read to probe for downstream devices times out.

Synchronous aborts need to be handled differently from the async aborts we were getting before, in particular the PC needs to be advanced when resolving the abort. This is mostly a copy of what other PCI drivers do on ARM to handle those aborts.

Signed-off-by Lucas Stach <[email protected]>

---
This is a fix that needs to go in for 4.12, but I would hope to get some thorough testing before.

---
 drivers/pci/dwc/pci-imx6.c | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

drivers/pci/dwc/pci-imx6.c +30 -3

index a98cba55c7f0..19a289b8cc94 100644
@@ -252,7 +252,34 @@ static void imx6_pcie_reset_phy(struct imx6_pcie *imx6_pcie)
 static int imx6q_pcie_abort_handler(unsigned long addr,
 		unsigned int fsr, struct pt_regs *regs)
 {
-	return 0;
+	unsigned long pc = instruction_pointer(regs);
+	unsigned long instr = *(unsigned long *)pc;
+	int reg = (instr >> 12) & 15;
+
+	/*
+	 * If the instruction being executed was a read,
+	 * make it look like it read all-ones.
+	 */
+	if ((instr & 0x0c100000) == 0x04100000) {
+		unsigned long val;
+
+		if (instr & 0x00400000)
+			val = 255;
+		else
+			val = -1;
+
+		regs->uregs[reg] = val;
+		regs->ARM_pc += 4;
+		return 0;
+	}
+
+	if ((instr & 0x0e100090) == 0x00100090) {
+		regs->uregs[reg] = -1;
+		regs->ARM_pc += 4;
+		return 0;
+	}
+
+	return 1;
 }
 
 static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)

@@ -819,8 +846,8 @@ static int __init imx6_pcie_init(void)
 	 * we can install the handler here without risking it
 	 * accessing some uninitialized driver state.
 	 */
-	hook_fault_code(16 + 6, imx6q_pcie_abort_handler, SIGBUS, 0,
-			"imprecise external abort");
+	hook_fault_code(8, imx6q_pcie_abort_handler, SIGBUS, 0,
+			"external abort on non-linefetch");
 
 	return platform_driver_register(&imx6_pcie_driver);
 }

Signature · 7 lines

2.11.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 10 lines · Lucas Stach

The change in Linux 4.12 to make PCI configuartion requests non-posted means that we are now getting a synchronous abort when the CFG space read to probe for downstream devices times out.

Synchronous aborts need to be handled differently from the async aborts we were getting before, in particular the PC needs to be advanced when resolving the abort. This is mostly a copy of what other PCI drivers do on ARM to handle those aborts.

Signed-off-by Lucas Stach <[email protected]>

Applied to for-linus for v4.12 with tested-by and acks from Fabio, Peter, and Richard. I updated the subject line to:

PCI: imx6: Fix config read timeout handling

because I don't think this change is specific to bus scanning and I wanted a hint that it's related to the config space mapping changes.

In fact, I'd really like to include the specific commit that caused this problem so that if the original commit is backported, there's a hint that we should backport this one, too. I *think* it might be cc7b0d495589, so I updated the changelog to this:

Commit cc7b0d495589 ("PCI: designware: Update PCI config space remap function") made PCI configuration requests non-posted, which means we now get a synchronous abort when the CFG space read to probe for downstream devices times out.

Synchronous aborts need to be handled differently from the async aborts we were getting before, in particular the PC needs to be advanced when resolving the abort. This is mostly a copy of what other PCI drivers do on ARM to handle those aborts.

[bhelgaas: changelog, "Fixes"]

Fixes cc7b0d495589 ("PCI: designware: Update PCI config space remap function")

Please let me know if this is the wrong commit.

Quoted 65 lines

This is a fix that needs to go in for 4.12, but I would hope to get some thorough testing before.

---
 drivers/pci/dwc/pci-imx6.c | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

drivers/pci/dwc/pci-imx6.c +30 -3

index a98cba55c7f0..19a289b8cc94 100644
@@ -252,7 +252,34 @@ static void imx6_pcie_reset_phy(struct imx6_pcie *imx6_pcie)
 static int imx6q_pcie_abort_handler(unsigned long addr,
 		unsigned int fsr, struct pt_regs *regs)
 {
-	return 0;
+	unsigned long pc = instruction_pointer(regs);
+	unsigned long instr = *(unsigned long *)pc;
+	int reg = (instr >> 12) & 15;
+
+	/*
+	 * If the instruction being executed was a read,
+	 * make it look like it read all-ones.
+	 */
+	if ((instr & 0x0c100000) == 0x04100000) {
+		unsigned long val;
+
+		if (instr & 0x00400000)
+			val = 255;
+		else
+			val = -1;
+
+		regs->uregs[reg] = val;
+		regs->ARM_pc += 4;
+		return 0;
+	}
+
+	if ((instr & 0x0e100090) == 0x00100090) {
+		regs->uregs[reg] = -1;
+		regs->ARM_pc += 4;
+		return 0;
+	}
+
+	return 1;
 }
 
 static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)

@@ -819,8 +846,8 @@ static int __init imx6_pcie_init(void)
 	 * we can install the handler here without risking it
 	 * accessing some uninitialized driver state.
 	 */
-	hook_fault_code(16 + 6, imx6q_pcie_abort_handler, SIGBUS, 0,
-			"imprecise external abort");
+	hook_fault_code(8, imx6q_pcie_abort_handler, SIGBUS, 0,
+			"external abort on non-linefetch");
 
 	return platform_driver_register(&imx6_pcie_driver);
 }

Signature · 7 lines

2.11.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Applied to for-linus for v4.12 with tested-by and acks from Fabio, Peter, and Richard. I updated the subject line to:

PCI: imx6: Fix config read timeout handling

because I don't think this change is specific to bus scanning and I wanted a hint that it's related to the config space mapping changes.

In fact, I'd really like to include the specific commit that caused this problem so that if the original commit is backported, there's a hint that we should backport this one, too. I *think* it might be cc7b0d495589, so I updated the changelog to this:

Commit cc7b0d495589 ("PCI: designware: Update PCI config space remap function") made PCI configuration requests non-posted, which means we now get a synchronous abort when the CFG space read to probe for downstream devices times out.

Synchronous aborts need to be handled differently from the async aborts we were getting before, in particular the PC needs to be advanced when resolving the abort. This is mostly a copy of what other PCI drivers do on ARM to handle those aborts.

[bhelgaas: changelog, "Fixes"]

Fixes cc7b0d495589 ("PCI: designware: Update PCI config space remap function")

Please let me know if this is the wrong commit.

Hi Bjorn,

Quoted 23 lines · Bjorn Helgaas

Applied to for-linus for v4.12 with tested-by and acks from Fabio, Peter, and Richard. I updated the subject line to:

PCI: imx6: Fix config read timeout handling

because I don't think this change is specific to bus scanning and I wanted a hint that it's related to the config space mapping changes.

In fact, I'd really like to include the specific commit that caused this problem so that if the original commit is backported, there's a hint that we should backport this one, too. I *think* it might be cc7b0d495589, so I updated the changelog to this:

Commit cc7b0d495589 ("PCI: designware: Update PCI config space remap function") made PCI configuration requests non-posted, which means we now get a synchronous abort when the CFG space read to probe for downstream devices times out.

Synchronous aborts need to be handled differently from the async aborts we were getting before, in particular the PC needs to be advanced when resolving the abort. This is mostly a copy of what other PCI drivers do on ARM to handle those aborts.

Quoted 4 lines

[bhelgaas: changelog, "Fixes"]

Fixes cc7b0d495589 ("PCI: designware: Update PCI config space remap function")

Please let me know if this is the wrong commit.

I just tested on a mx6q board and can confirm that this is the correct offending commit, thanks.