Skip to contentSkip to conversation

ARM: imx6: Fix "BUG: scheduling while atomic" if PCIe switch is attached

PATCH New David Müller 4 participants 7 messages Jun 18, 2015 – Jul 14, 2015

Subsystems (2)
Arm i.MX and Layerscape PCI
lore
Share & tools
lore

David Müller

author
#0 lore raw

PATCH 1/1 ARM: imx6: Fix "BUG: scheduling while atomic" if PCIe switch is attached

This problem has already been reported as
https://bugzilla.kernel.org/show_bug.cgi?id=100031

Signed-off-by David M?ller <[email protected]>
---
 drivers/pci/host/pci-imx6.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/pci/host/pci-imx6.c +1 -1
index fdb9536..c63691c 100644
@@ -489,7 +489,7 @@ static int imx6_pcie_link_up(struct pcie_port *pp)
 		 * Wait a little bit, then re-check if the link finished
 		 * the training.
 		 */
-		usleep_range(1000, 2000);
+		mdelay(20);
 	}
 	/*
 	 * From L0, initiate MAC entry to gen2 if EP/RC supports gen2.
Signature · 1 line
1.8.4
#1 lore raw

PATCH 1/1 ARM: imx6: Fix "BUG: scheduling while atomic" if PCIe switch is attached

Am Donnerstag, den 18.06.2015, 09:56 +0200 schrieb David M?ller:

Quoted 13 lines

This problem has already been reported as
https://bugzilla.kernel.org/show_bug.cgi?id=100031

Signed-off-by David M?ller <[email protected]>
---
 drivers/pci/host/pci-imx6.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/pci/host/pci-imx6.c +0 -0
index fdb9536..c63691c 100644

drivers/pci/host/pci-imx6.c static int imx6_pcie_link_up(struct pcie_port *pp) · line 492

Quoted 5 lines
drivers/pci/host/pci-imx6.c +1 -1
@@ -489,7 +489,7 @@ static int imx6_pcie_link_up(struct pcie_port *pp)
 		 * Wait a little bit, then re-check if the link finished
 		 * the training.
 		 */
-		usleep_range(1000, 2000);
+		mdelay(20);

While switching to mdelay might be the right thing to do here, you are also changing the timeout. This is a change in behavior and so not okay. Especially as you keep the CPU spinning for the duration of that timeout.

Quoted 3 lines
drivers/pci/host/pci-imx6.c +0 -0
 	}
 	/*
 	 * From L0, initiate MAC entry to gen2 if EP/RC supports gen2.
Signature · 2 lines
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |
reviewer
#2 lore raw

PATCH 1/1 ARM: imx6: Fix "BUG: scheduling while atomic" if PCIe switch is attached

Quoted 5 lines · Lucas Stach
Quoted 5 lines
drivers/pci/host/pci-imx6.c +0 -0
index fdb9536..c63691c 100644
@@ -489,7 +489,7 @@ static int imx6_pcie_link_up(struct pcie_port *pp)
Quoted 8 lines

drivers/pci/host/pci-imx6.c static int imx6_pcie_link_up(struct pcie_port *pp) · line 492

Quoted 5 lines
drivers/pci/host/pci-imx6.c +1 -1
               * Wait a little bit, then re-check if the link finished
               * the training.
               */
-             usleep_range(1000, 2000);
+             mdelay(20);

While switching to mdelay might be the right thing to do here, you are also changing the timeout. This is a change in behavior and so not okay.

msleep(2) is not recommended according to Documentation/timers/timers-howto.txt.

What would be the proper fix then?

Regards,

Fabio Estevam

#3 lore raw

PATCH 1/1 ARM: imx6: Fix "BUG: scheduling while atomic" if PCIe switch is attached

Am Montag, den 22.06.2015, 10:30 -0300 schrieb Fabio Estevam:

Quoted 16 lines · Lucas Stach
Quoted 14 lines

drivers/pci/host/pci-imx6.c static int imx6_pcie_link_up(struct pcie_port *pp) · line 492

Quoted 11 lines
drivers/pci/host/pci-imx6.c +1 -1
index fdb9536..c63691c 100644
@@ -489,7 +489,7 @@ static int imx6_pcie_link_up(struct pcie_port *pp)
               * Wait a little bit, then re-check if the link finished
               * the training.
               */
-             usleep_range(1000, 2000);
+             mdelay(20);

While switching to mdelay might be the right thing to do here, you are also changing the timeout. This is a change in behavior and so not okay.

Quoted 4 lines

msleep(2) is not recommended according to Documentation/timers/timers-howto.txt.

What would be the proper fix then?

mdelay(2) is completely reasonable. Note the difference between delay and sleep.

The mid-term correct solution would be to move all the link-up handling out of the atomic path.

Regards,
Lucas

Signature · 2 lines
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |
reviewer
#4 lore raw

PATCH 1/1 ARM: imx6: Fix "BUG: scheduling while atomic" if PCIe switch is attached

Quoted 2 lines · Lucas Stach

mdelay(2) is completely reasonable. Note the difference between delay and sleep.

Oh, that's right :-)

Thanks

maintainer
#5 lore raw

PATCH 1/1 ARM: imx6: Fix "BUG: scheduling while atomic" if PCIe switch is attached

In reply to David Müller
Quoted 13 lines · David M?ller

This problem has already been reported as
https://bugzilla.kernel.org/show_bug.cgi?id=100031

Signed-off-by David M?ller <[email protected]>
---
 drivers/pci/host/pci-imx6.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/pci/host/pci-imx6.c +0 -0
index fdb9536..c63691c 100644

drivers/pci/host/pci-imx6.c static int imx6_pcie_link_up(struct pcie_port *pp) · line 495

Quoted 8 lines
drivers/pci/host/pci-imx6.c +1 -1
@@ -489,7 +489,7 @@ static int imx6_pcie_link_up(struct pcie_port *pp)
 		 * Wait a little bit, then re-check if the link finished
 		 * the training.
 		 */
-		usleep_range(1000, 2000);
+		mdelay(20);
 	}
 	/*
 	 * From L0, initiate MAC entry to gen2 if EP/RC supports gen2.

I asked in the bugzilla why imx6_pcie_link_up() looks nothing like the other pcie_host_ops.link_up() methods.

Lucas responded:

Quoted 9 lines

the i.MX6 link up routine is a bit more complex compared to the other designware drivers because of a hardware bug, where some FIFOs could get out of sync when changing the link speed. We need to start the link at Gen1, wait for it to be stable, then start a directed link speed change if the EP is Gen2 and wait for the link to be stable again. If the link doesn't come back, we need to start over.

I don't think any of the other DW PCIe implementations need this workaround.

That explanation makes sense, and a comment to that effect should be in the code. But I also wonder whether this workaround could be moved to imx6_pcie_start_link() (just renamed to imx6_pcie_establish_link()). Then imx6_pcie_link_up() could be a simple status check like the others.

Bjorn

maintainer
#6 lore raw

PATCH 1/1 ARM: imx6: Fix "BUG: scheduling while atomic" if PCIe switch is attached

In reply to David Müller
Quoted 4 lines · David M?ller

This problem has already been reported as
https://bugzilla.kernel.org/show_bug.cgi?id=100031

Signed-off-by David M?ller <[email protected]>

Dropping for now, hoping for an update addressing the review comments.

Quoted 20 lines
---
 drivers/pci/host/pci-imx6.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/pci/host/pci-imx6.c +1 -1
index fdb9536..c63691c 100644
@@ -489,7 +489,7 @@ static int imx6_pcie_link_up(struct pcie_port *pp)
 		 * Wait a little bit, then re-check if the link finished
 		 * the training.
 		 */
-		usleep_range(1000, 2000);
+		mdelay(20);
 	}
 	/*
 	 * From L0, initiate MAC entry to gen2 if EP/RC supports gen2.
Signature · 1 line
1.8.4