[PATCHv3] dmaengine: cppi41: Fix oops in cppi41_runtime_resume

[PATCHv3] dmaengine: cppi41: Fix oops in cppi41_runtime_resume

[Tony Lindgren]

Commit fdea2d09b997 ("dmaengine: cppi41: Add basic PM runtime support")
together with recent MUSB changes allowed USB and DMA on BeagleBone to idle
when no cable is connected. But looks like few corner case issues still
remain.

Looks like just by re-plugging USB cable about ten or so times on BeagleBone
when configured in USB peripheral mode we can get warnings and eventually
trigger an oops in cppi41 DMA:

WARNING: CPU: 0 PID: 14 at drivers/dma/cppi41.c:1154 cppi41_runtime_suspend+
x28/0x38 [cppi41]
...

WARNING: CPU: 0 PID: 14 at drivers/dma/cppi41.c:452
push_desc_queue+0x94/0x9c [cppi41]
...

Unable to handle kernel NULL pointer dereference at virtual
address 00000104
pgd = c0004000
[00000104] *pgd=00000000
Internal error: Oops: 805 [#1] SMP ARM
...
[<bf0d92cc>] (cppi41_runtime_resume [cppi41]) from [<c0589838>]
(__rpm_callback+0xc0/0x214)
[<c0589838>] (__rpm_callback) from [<c05899ac>] (rpm_callback+0x20/0x80)
[<c05899ac>] (rpm_callback) from [<c0589460>] (rpm_resume+0x504/0x78c)
[<c0589460>] (rpm_resume) from [<c058a1a0>] (pm_runtime_work+0x60/0xa8)
[<c058a1a0>] (pm_runtime_work) from [<c0156120>] (process_one_work+0x2b4/0x808)

This is because of a race with runtime PM and cppi41_dma_issue_pending()
as reported by Alexandre Bailon <abailon-rdvid1DuHRBWk0Htik3J/w@public.gmane.org> in earlier
set of patches. Based on mailing list discussions we however came to the
conclusion that a different fix from Alexandre's fix is needed in order
to guarantee that DMA is really active when we try to use it.

To fix the issue, we need to add a driver specific flag as we otherwise
can have -EINPROGRESS state set by runtime PM and can't rely on
pm_runtime_active() to tell us when we can use the DMA.

And we need to make sure the DMA transfers get triggered in the queued
order. So let's always queue the transfers, then flush the queue
from both cppi41_dma_issue_pending() and cppi41_runtime_resume()
as suggested by Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org> in an
earlier example patch.

For reference, this is also documented in Documentation/power/runtime_pm.txt
in the example at the end of the file as pointed out by Grygorii Strashko
<grygorii.strashko-l0cyMroinI0@public.gmane.org>.

Based on earlier patches from Alexandre Bailon <abailon-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
and Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org> modified based on
testing and what was discussed on the mailing lists.

Note that additional patches are still needed depending on how we want
to handle the cppi41 runtime PM. So far cppi41 is always coupled with
musb driver, and musb guarantees that cppi41 is always active during
use. So until we have a better solution, let's just set the cppi41
autosuspend delay 1 second to avoid pointless EINPROGRESS warnings during
USB mass storage enumeration. As cppi41 is properly clocked with musb
being active, we can safely do this.

Fixes: fdea2d09b997 ("dmaengine: cppi41: Add basic PM runtime support")
Cc: Andy Shevchenko <andy.shevchenko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Bin Liu <b-liu-l0cyMroinI0@public.gmane.org>
Cc: Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org>
Cc: Kevin Hilman <khilman-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
Cc: Patrick Titiano <ptitiano-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
Cc: Sergei Shtylyov <sergei.shtylyov-M4DtvfQ/ZS1MRgGoP+s0PdBPR1lH4CV8@public.gmane.org>
Reported-by: Alexandre Bailon <abailon-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
Signed-off-by: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
---

Here's what I think we should merge as a minimal fix for the -rc series,
can you guys please test?

Changes since v2:

- Change autosuspend delay to 1000 to avoid warnings with USB mass
  storage as cppi41 is always clocked with musb during use

- Drop warning changes to cppi41_irq(), those can be dealt with later
  with the autosuspend delay change

- Update description accordingly

---
 drivers/dma/cppi41.c | 42 ++++++++++++++++++++++++++----------------
 1 file changed, 26 insertions(+), 16 deletions(-)

diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c
--- a/drivers/dma/cppi41.c
+++ b/drivers/dma/cppi41.c
@@ -153,6 +153,8 @@ struct cppi41_dd {
 
 	/* context for suspend/resume */
 	unsigned int dma_tdfdq;
+
+	bool is_suspended;
 };
 
 #define FIST_COMPLETION_QUEUE	93
@@ -457,20 +459,26 @@ static void push_desc_queue(struct cppi41_channel *c)
 	cppi_writel(reg, cdd->qmgr_mem + QMGR_QUEUE_D(c->q_num));
 }
 
-static void pending_desc(struct cppi41_channel *c)
+/*
+ * Caller must hold cdd->lock to prevent push_desc_queue()
+ * getting called out of order. We have both cppi41_dma_issue_pending()
+ * and cppi41_runtime_resume() call this function.
+ */
+static void cppi41_run_queue(struct cppi41_dd *cdd)
 {
-	struct cppi41_dd *cdd = c->cdd;
-	unsigned long flags;
+	struct cppi41_channel *c, *_c;
 
-	spin_lock_irqsave(&cdd->lock, flags);
-	list_add_tail(&c->node, &cdd->pending);
-	spin_unlock_irqrestore(&cdd->lock, flags);
+	list_for_each_entry_safe(c, _c, &cdd->pending, node) {
+		push_desc_queue(c);
+		list_del(&c->node);
+	}
 }
 
 static void cppi41_dma_issue_pending(struct dma_chan *chan)
 {
 	struct cppi41_channel *c = to_cpp41_chan(chan);
 	struct cppi41_dd *cdd = c->cdd;
+	unsigned long flags;
 	int error;
 
 	error = pm_runtime_get(cdd->ddev.dev);
@@ -482,10 +490,11 @@ static void cppi41_dma_issue_pending(struct dma_chan *chan)
 		return;
 	}
 
-	if (likely(pm_runtime_active(cdd->ddev.dev)))
-		push_desc_queue(c);
-	else
-		pending_desc(c);
+	spin_lock_irqsave(&cdd->lock, flags);
+	list_add_tail(&c->node, &cdd->pending);
+	if (!cdd->is_suspended)
+		cppi41_run_queue(cdd);
+	spin_unlock_irqrestore(&cdd->lock, flags);
 
 	pm_runtime_mark_last_busy(cdd->ddev.dev);
 	pm_runtime_put_autosuspend(cdd->ddev.dev);
@@ -1020,7 +1029,7 @@ static int cppi41_dma_probe(struct platform_device *pdev)
 		return -ENXIO;
 
 	pm_runtime_enable(dev);
-	pm_runtime_set_autosuspend_delay(dev, 100);
+	pm_runtime_set_autosuspend_delay(dev, 1000);
 	pm_runtime_use_autosuspend(dev);
 	ret = pm_runtime_get_sync(dev);
 	if (ret < 0)
@@ -1150,8 +1159,12 @@ static int __maybe_unused cppi41_resume(struct device *dev)
 static int __maybe_unused cppi41_runtime_suspend(struct device *dev)
 {
 	struct cppi41_dd *cdd = dev_get_drvdata(dev);
+	unsigned long flags;
 
+	spin_lock_irqsave(&cdd->lock, flags);
+	cdd->is_suspended = true;
 	WARN_ON(!list_empty(&cdd->pending));
+	spin_unlock_irqrestore(&cdd->lock, flags);
 
 	return 0;
 }
@@ -1159,14 +1172,11 @@ static int __maybe_unused cppi41_runtime_suspend(struct device *dev)
 static int __maybe_unused cppi41_runtime_resume(struct device *dev)
 {
 	struct cppi41_dd *cdd = dev_get_drvdata(dev);
-	struct cppi41_channel *c, *_c;
 	unsigned long flags;
 
 	spin_lock_irqsave(&cdd->lock, flags);
-	list_for_each_entry_safe(c, _c, &cdd->pending, node) {
-		push_desc_queue(c);
-		list_del(&c->node);
-	}
+	cdd->is_suspended = false;
+	cppi41_run_queue(cdd);
 	spin_unlock_irqrestore(&cdd->lock, flags);
 
 	return 0;
-- 
2.11.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3] dmaengine: cppi41: Fix oops in cppi41_runtime_resume

[Bin Liu]

On Tue, Jan 17, 2017 at 09:55:24AM -0800, Tony Lindgren wrote:
> Commit fdea2d09b997 ("dmaengine: cppi41: Add basic PM runtime support")
> together with recent MUSB changes allowed USB and DMA on BeagleBone to idle
> when no cable is connected. But looks like few corner case issues still
> remain.
> 
> Looks like just by re-plugging USB cable about ten or so times on BeagleBone
> when configured in USB peripheral mode we can get warnings and eventually
> trigger an oops in cppi41 DMA:
> 
> WARNING: CPU: 0 PID: 14 at drivers/dma/cppi41.c:1154 cppi41_runtime_suspend+
> x28/0x38 [cppi41]
> ...
> 
> WARNING: CPU: 0 PID: 14 at drivers/dma/cppi41.c:452
> push_desc_queue+0x94/0x9c [cppi41]
> ...
> 
> Unable to handle kernel NULL pointer dereference at virtual
> address 00000104
> pgd = c0004000
> [00000104] *pgd=00000000
> Internal error: Oops: 805 [#1] SMP ARM
> ...
> [<bf0d92cc>] (cppi41_runtime_resume [cppi41]) from [<c0589838>]
> (__rpm_callback+0xc0/0x214)
> [<c0589838>] (__rpm_callback) from [<c05899ac>] (rpm_callback+0x20/0x80)
> [<c05899ac>] (rpm_callback) from [<c0589460>] (rpm_resume+0x504/0x78c)
> [<c0589460>] (rpm_resume) from [<c058a1a0>] (pm_runtime_work+0x60/0xa8)
> [<c058a1a0>] (pm_runtime_work) from [<c0156120>] (process_one_work+0x2b4/0x808)
> 
> This is because of a race with runtime PM and cppi41_dma_issue_pending()
> as reported by Alexandre Bailon <abailon-rdvid1DuHRBWk0Htik3J/w@public.gmane.org> in earlier
> set of patches. Based on mailing list discussions we however came to the
> conclusion that a different fix from Alexandre's fix is needed in order
> to guarantee that DMA is really active when we try to use it.
> 
> To fix the issue, we need to add a driver specific flag as we otherwise
> can have -EINPROGRESS state set by runtime PM and can't rely on
> pm_runtime_active() to tell us when we can use the DMA.
> 
> And we need to make sure the DMA transfers get triggered in the queued
> order. So let's always queue the transfers, then flush the queue
> from both cppi41_dma_issue_pending() and cppi41_runtime_resume()
> as suggested by Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org> in an
> earlier example patch.
> 
> For reference, this is also documented in Documentation/power/runtime_pm.txt
> in the example at the end of the file as pointed out by Grygorii Strashko
> <grygorii.strashko-l0cyMroinI0@public.gmane.org>.
> 
> Based on earlier patches from Alexandre Bailon <abailon-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
> and Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org> modified based on
> testing and what was discussed on the mailing lists.
> 
> Note that additional patches are still needed depending on how we want
> to handle the cppi41 runtime PM. So far cppi41 is always coupled with
> musb driver, and musb guarantees that cppi41 is always active during
> use. So until we have a better solution, let's just set the cppi41
> autosuspend delay 1 second to avoid pointless EINPROGRESS warnings during
> USB mass storage enumeration. As cppi41 is properly clocked with musb
> being active, we can safely do this.
> 
> Fixes: fdea2d09b997 ("dmaengine: cppi41: Add basic PM runtime support")
> Cc: Andy Shevchenko <andy.shevchenko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: Bin Liu <b-liu-l0cyMroinI0@public.gmane.org>
> Cc: Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org>
> Cc: Kevin Hilman <khilman-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
> Cc: Patrick Titiano <ptitiano-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
> Cc: Sergei Shtylyov <sergei.shtylyov-M4DtvfQ/ZS1MRgGoP+s0PdBPR1lH4CV8@public.gmane.org>
> Reported-by: Alexandre Bailon <abailon-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>

With this v3, I still get -71 error when a device is plugged to a hub to
musb. It doesn't happen though if the device is already plugged to the hub
before attaching the hub to musb.

[  177.579300] usb 1-1: reset high-speed USB device number 2 using musb-hdrc
[  177.719308] usb 1-1: device descriptor read/64, error -71
[  178.350297] usb 1-1.1: new high-speed USB device number 4 using musb-hdrc

And I still get -115 error flooding with thumb drive.

[  236.880068] cppi41-dma-engine 47400000.dma-controller: cppi41_irq pm runtime get: -115

I tested with TI AM335x GP EVM. The problems happen on both musb ports.

Regards,
-Bin.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3] dmaengine: cppi41: Fix oops in cppi41_runtime_resume

[Tony Lindgren]

Hi,

* Bin Liu <b-liu-l0cyMroinI0@public.gmane.org> [170118 06:26]:
> With this v3, I still get -71 error when a device is plugged to a hub to
> musb. It doesn't happen though if the device is already plugged to the hub
> before attaching the hub to musb.
> 
> [  177.579300] usb 1-1: reset high-speed USB device number 2 using musb-hdrc
> [  177.719308] usb 1-1: device descriptor read/64, error -71
> [  178.350297] usb 1-1.1: new high-speed USB device number 4 using musb-hdrc

I think the -71 issue is a different regression. I've verified that v4.8.y
does not have this, but v4.9.y does have it.

So far the easiest way for me to reproduce the -71 problem is by plugging
a USB drive into a connected hub. If I connect the hub with USB drive already
plugged into the hub, it does not happen.

With the hub connected musb is already active when the USB drive is plugged
in. So I'm now suspecting this -71 regression is not related to runtime PM
changes. Bisect and boot and plug devices time I think unless you have
better ideas?

> And I still get -115 error flooding with thumb drive.
> 
> [  236.880068] cppi41-dma-engine 47400000.dma-controller: cppi41_irq pm runtime get: -115
> 
> I tested with TI AM335x GP EVM. The problems happen on both musb ports.

OK. So it's pointless to try to play with the autosuspend timeout.

Let's just do a WARN(!pm_runtime_active(cdd->ddev.dev->parent)) there
until we have musb_cppi41.c dma calls properly paired and don't have
dma requests lingering.

Care to try the updated patch below? It won't help with the -71 issue
but the $subject issue should be fixed. And you should not see the
WARN() trigger with your tests presumably.

Regards,

Tony

8< ----------------------------
>From tony Mon Sep 17 00:00:00 2001
From: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
Date: Mon, 16 Jan 2017 09:52:10 -0800
Subject: [PATCH] dmaengine: cppi41: Fix oops in cppi41_runtime_resume

Commit fdea2d09b997 ("dmaengine: cppi41: Add basic PM runtime support")
together with recent MUSB changes allowed USB and DMA on BeagleBone to idle
when no cable is connected. But looks like few corner case issues still
remain.

Looks like just by re-plugging USB cable about ten or so times on BeagleBone
when configured in USB peripheral mode we can get warnings and eventually
trigger an oops in cppi41 DMA:

WARNING: CPU: 0 PID: 14 at drivers/dma/cppi41.c:1154 cppi41_runtime_suspend+
x28/0x38 [cppi41]
...

WARNING: CPU: 0 PID: 14 at drivers/dma/cppi41.c:452
push_desc_queue+0x94/0x9c [cppi41]
...

Unable to handle kernel NULL pointer dereference at virtual
address 00000104
pgd = c0004000
[00000104] *pgd=00000000
Internal error: Oops: 805 [#1] SMP ARM
...
[<bf0d92cc>] (cppi41_runtime_resume [cppi41]) from [<c0589838>]
(__rpm_callback+0xc0/0x214)
[<c0589838>] (__rpm_callback) from [<c05899ac>] (rpm_callback+0x20/0x80)
[<c05899ac>] (rpm_callback) from [<c0589460>] (rpm_resume+0x504/0x78c)
[<c0589460>] (rpm_resume) from [<c058a1a0>] (pm_runtime_work+0x60/0xa8)
[<c058a1a0>] (pm_runtime_work) from [<c0156120>] (process_one_work+0x2b4/0x808)

This is because of a race with runtime PM and cppi41_dma_issue_pending()
as reported by Alexandre Bailon <abailon-rdvid1DuHRBWk0Htik3J/w@public.gmane.org> in earlier
set of patches. Based on mailing list discussions we however came to the
conclusion that a different fix from Alexandre's fix is needed in order
to guarantee that DMA is really active when we try to use it.

To fix the issue, we need to add a driver specific flag as we otherwise
can have -EINPROGRESS state set by runtime PM and can't rely on
pm_runtime_active() to tell us when we can use the DMA.

And we need to make sure the DMA transfers get triggered in the queued
order. So let's always queue the transfers, then flush the queue
from both cppi41_dma_issue_pending() and cppi41_runtime_resume()
as suggested by Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org> in an
earlier example patch.

For reference, this is also documented in Documentation/power/runtime_pm.txt
in the example at the end of the file as pointed out by Grygorii Strashko
<grygorii.strashko-l0cyMroinI0@public.gmane.org>.

Based on earlier patches from Alexandre Bailon <abailon-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
and Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org> modified based on
testing and what was discussed on the mailing lists.

Note that additional patches are still needed depending on how we want
to handle the cppi41 runtime PM. So far cppi41 is always coupled with
musb driver, and musb guarantees that cppi41 is always active during
use. So until we have a better solution, let's just WARN() if the musb
parent is not active to avoid pointless EINPROGRESS warnings during
USB mass storage enumeration. As cppi41 is properly clocked with musb
being active, we can safely do this.

Fixes: fdea2d09b997 ("dmaengine: cppi41: Add basic PM runtime support")
Cc: Andy Shevchenko <andy.shevchenko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Bin Liu <b-liu-l0cyMroinI0@public.gmane.org>
Cc: Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org>
Cc: Kevin Hilman <khilman-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
Cc: Patrick Titiano <ptitiano-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
Cc: Sergei Shtylyov <sergei.shtylyov-M4DtvfQ/ZS1MRgGoP+s0PdBPR1lH4CV8@public.gmane.org>
Reported-by: Alexandre Bailon <abailon-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
Signed-off-by: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
---
 drivers/dma/cppi41.c | 51 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 28 insertions(+), 23 deletions(-)

diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c
--- a/drivers/dma/cppi41.c
+++ b/drivers/dma/cppi41.c
@@ -153,6 +153,8 @@ struct cppi41_dd {
 
 	/* context for suspend/resume */
 	unsigned int dma_tdfdq;
+
+	bool is_suspended;
 };
 
 #define FIST_COMPLETION_QUEUE	93
@@ -317,12 +319,10 @@ static irqreturn_t cppi41_irq(int irq, void *data)
 
 		while (val) {
 			u32 desc, len;
-			int error;
 
-			error = pm_runtime_get(cdd->ddev.dev);
-			if (error < 0)
-				dev_err(cdd->ddev.dev, "%s pm runtime get: %i\n",
-					__func__, error);
+			/* Revisit when musb_cppi41.c dma calls are paired */
+			WARN(!pm_runtime_active(cdd->ddev.dev->parent),
+			     "%s parent not active?\n", __func__);
 
 			q_num = __fls(val);
 			val &= ~(1 << q_num);
@@ -343,9 +343,6 @@ static irqreturn_t cppi41_irq(int irq, void *data)
 			c->residue = pd_trans_len(c->desc->pd6) - len;
 			dma_cookie_complete(&c->txd);
 			dmaengine_desc_get_callback_invoke(&c->txd, NULL);
-
-			pm_runtime_mark_last_busy(cdd->ddev.dev);
-			pm_runtime_put_autosuspend(cdd->ddev.dev);
 		}
 	}
 	return IRQ_HANDLED;
@@ -457,20 +454,26 @@ static void push_desc_queue(struct cppi41_channel *c)
 	cppi_writel(reg, cdd->qmgr_mem + QMGR_QUEUE_D(c->q_num));
 }
 
-static void pending_desc(struct cppi41_channel *c)
+/*
+ * Caller must hold cdd->lock to prevent push_desc_queue()
+ * getting called out of order. We have both cppi41_dma_issue_pending()
+ * and cppi41_runtime_resume() call this function.
+ */
+static void cppi41_run_queue(struct cppi41_dd *cdd)
 {
-	struct cppi41_dd *cdd = c->cdd;
-	unsigned long flags;
+	struct cppi41_channel *c, *_c;
 
-	spin_lock_irqsave(&cdd->lock, flags);
-	list_add_tail(&c->node, &cdd->pending);
-	spin_unlock_irqrestore(&cdd->lock, flags);
+	list_for_each_entry_safe(c, _c, &cdd->pending, node) {
+		push_desc_queue(c);
+		list_del(&c->node);
+	}
 }
 
 static void cppi41_dma_issue_pending(struct dma_chan *chan)
 {
 	struct cppi41_channel *c = to_cpp41_chan(chan);
 	struct cppi41_dd *cdd = c->cdd;
+	unsigned long flags;
 	int error;
 
 	error = pm_runtime_get(cdd->ddev.dev);
@@ -482,10 +485,11 @@ static void cppi41_dma_issue_pending(struct dma_chan *chan)
 		return;
 	}
 
-	if (likely(pm_runtime_active(cdd->ddev.dev)))
-		push_desc_queue(c);
-	else
-		pending_desc(c);
+	spin_lock_irqsave(&cdd->lock, flags);
+	list_add_tail(&c->node, &cdd->pending);
+	if (!cdd->is_suspended)
+		cppi41_run_queue(cdd);
+	spin_unlock_irqrestore(&cdd->lock, flags);
 
 	pm_runtime_mark_last_busy(cdd->ddev.dev);
 	pm_runtime_put_autosuspend(cdd->ddev.dev);
@@ -1150,8 +1154,12 @@ static int __maybe_unused cppi41_resume(struct device *dev)
 static int __maybe_unused cppi41_runtime_suspend(struct device *dev)
 {
 	struct cppi41_dd *cdd = dev_get_drvdata(dev);
+	unsigned long flags;
 
+	spin_lock_irqsave(&cdd->lock, flags);
+	cdd->is_suspended = true;
 	WARN_ON(!list_empty(&cdd->pending));
+	spin_unlock_irqrestore(&cdd->lock, flags);
 
 	return 0;
 }
@@ -1159,14 +1167,11 @@ static int __maybe_unused cppi41_runtime_suspend(struct device *dev)
 static int __maybe_unused cppi41_runtime_resume(struct device *dev)
 {
 	struct cppi41_dd *cdd = dev_get_drvdata(dev);
-	struct cppi41_channel *c, *_c;
 	unsigned long flags;
 
 	spin_lock_irqsave(&cdd->lock, flags);
-	list_for_each_entry_safe(c, _c, &cdd->pending, node) {
-		push_desc_queue(c);
-		list_del(&c->node);
-	}
+	cdd->is_suspended = false;
+	cppi41_run_queue(cdd);
 	spin_unlock_irqrestore(&cdd->lock, flags);
 
 	return 0;
-- 
2.11.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3] dmaengine: cppi41: Fix oops in cppi41_runtime_resume

[Grygorii Strashko]

On 01/18/2017 10:53 AM, Tony Lindgren wrote:
> Hi,
> 
> * Bin Liu <b-liu-l0cyMroinI0@public.gmane.org> [170118 06:26]:
>> With this v3, I still get -71 error when a device is plugged to a hub to
>> musb. It doesn't happen though if the device is already plugged to the hub
>> before attaching the hub to musb.
>>
>> [  177.579300] usb 1-1: reset high-speed USB device number 2 using musb-hdrc
>> [  177.719308] usb 1-1: device descriptor read/64, error -71
>> [  178.350297] usb 1-1.1: new high-speed USB device number 4 using musb-hdrc
> 
> I think the -71 issue is a different regression. I've verified that v4.8.y
> does not have this, but v4.9.y does have it.
> 
> So far the easiest way for me to reproduce the -71 problem is by plugging
> a USB drive into a connected hub. If I connect the hub with USB drive already
> plugged into the hub, it does not happen.
> 
> With the hub connected musb is already active when the USB drive is plugged

This is not exactly try, i think, because cppi can be in inactive state
because of autosuspend (all calls are paired).

> in. So I'm now suspecting this -71 regression is not related to runtime PM
> changes. Bisect and boot and plug devices time I think unless you have
> better ideas?
> 
>> And I still get -115 error flooding with thumb drive.
>>
>> [  236.880068] cppi41-dma-engine 47400000.dma-controller: cppi41_irq pm runtime get: -115
>>
>> I tested with TI AM335x GP EVM. The problems happen on both musb ports.
> 
> OK. So it's pointless to try to play with the autosuspend timeout.
> 
> Let's just do a WARN(!pm_runtime_active(cdd->ddev.dev->parent)) there
> until we have musb_cppi41.c dma calls properly paired and don't have
> dma requests lingering.
> 
> Care to try the updated patch below? It won't help with the -71 issue
> but the $subject issue should be fixed. And you should not see the
> WARN() trigger with your tests presumably.
> 

Sry, but wouldn't be more simple and safe just to drop PM runtime from this driver,
as it is part of musb and musb PM state expected to be handled properly by PM runtime now.

More over, There is another possibility related to the current PM runtime implementation and autosuspend
- it might introduce delay between time when musb request desc push and time when cppi will actually
put this desc in HW queue if cppi was not active.
For example, there might not be -71 error seen if CPPI autosuspend is disabled
(cppi is active all the time) before plug-in hub and then USB drive.


-- 
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3] dmaengine: cppi41: Fix oops in cppi41_runtime_resume

[Tony Lindgren]

* Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org> [170118 10:05]:
> 
> 
> On 01/18/2017 10:53 AM, Tony Lindgren wrote:
> > Hi,
> > 
> > * Bin Liu <b-liu-l0cyMroinI0@public.gmane.org> [170118 06:26]:
> >> With this v3, I still get -71 error when a device is plugged to a hub to
> >> musb. It doesn't happen though if the device is already plugged to the hub
> >> before attaching the hub to musb.
> >>
> >> [  177.579300] usb 1-1: reset high-speed USB device number 2 using musb-hdrc
> >> [  177.719308] usb 1-1: device descriptor read/64, error -71
> >> [  178.350297] usb 1-1.1: new high-speed USB device number 4 using musb-hdrc
> > 
> > I think the -71 issue is a different regression. I've verified that v4.8.y
> > does not have this, but v4.9.y does have it.
> > 
> > So far the easiest way for me to reproduce the -71 problem is by plugging
> > a USB drive into a connected hub. If I connect the hub with USB drive already
> > plugged into the hub, it does not happen.
> > 
> > With the hub connected musb is already active when the USB drive is plugged
> 
> This is not exactly try, i think, because cppi can be in inactive state
> because of autosuspend (all calls are paired).

Musb is active when there's something connected meaning cppi41 is clocked
when a hub is connected. The actual runtime PM implementation happens for
the interconnect target module for both musb and cppi41 in hwmod.c code.

> > in. So I'm now suspecting this -71 regression is not related to runtime PM
> > changes. Bisect and boot and plug devices time I think unless you have
> > better ideas?
> > 
> >> And I still get -115 error flooding with thumb drive.
> >>
> >> [  236.880068] cppi41-dma-engine 47400000.dma-controller: cppi41_irq pm runtime get: -115
> >>
> >> I tested with TI AM335x GP EVM. The problems happen on both musb ports.
> > 
> > OK. So it's pointless to try to play with the autosuspend timeout.
> > 
> > Let's just do a WARN(!pm_runtime_active(cdd->ddev.dev->parent)) there
> > until we have musb_cppi41.c dma calls properly paired and don't have
> > dma requests lingering.
> > 
> > Care to try the updated patch below? It won't help with the -71 issue
> > but the $subject issue should be fixed. And you should not see the
> > WARN() trigger with your tests presumably.
> > 
> 
> Sry, but wouldn't be more simple and safe just to drop PM runtime from this driver,
> as it is part of musb and musb PM state expected to be handled properly by PM runtime now.

Well we still need PM runtime in cppi41 to initialize it when it gets
probed and idle it when not used even if musb modules are not loaded.

Unfortunately until musb_cppi41.c dma calls are fixed, we can't do
any sane use counting in cppi41.c without adding timeout handling
to it.

> More over, There is another possibility related to the current PM runtime implementation and autosuspend
> - it might introduce delay between time when musb request desc push and time when cppi will actually
> put this desc in HW queue if cppi was not active.
> For example, there might not be -71 error seen if CPPI autosuspend is disabled
> (cppi is active all the time) before plug-in hub and then USB drive.

I already checked that. The -71 error seems to be a separate issue.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3] dmaengine: cppi41: Fix oops in cppi41_runtime_resume

[Tony Lindgren]

* Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> [170118 10:15]:
> * Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org> [170118 10:05]:
> > 
> > 
> > On 01/18/2017 10:53 AM, Tony Lindgren wrote:
> > > Hi,
> > > 
> > > * Bin Liu <b-liu-l0cyMroinI0@public.gmane.org> [170118 06:26]:
> > >> With this v3, I still get -71 error when a device is plugged to a hub to
> > >> musb. It doesn't happen though if the device is already plugged to the hub
> > >> before attaching the hub to musb.
> > >>
> > >> [  177.579300] usb 1-1: reset high-speed USB device number 2 using musb-hdrc
> > >> [  177.719308] usb 1-1: device descriptor read/64, error -71
> > >> [  178.350297] usb 1-1.1: new high-speed USB device number 4 using musb-hdrc
> > > 
> > > I think the -71 issue is a different regression. I've verified that v4.8.y
> > > does not have this, but v4.9.y does have it.
> > > 
> > > So far the easiest way for me to reproduce the -71 problem is by plugging
> > > a USB drive into a connected hub. If I connect the hub with USB drive already
> > > plugged into the hub, it does not happen.
...

> > Sry, but wouldn't be more simple and safe just to drop PM runtime from this driver,
> > as it is part of musb and musb PM state expected to be handled properly by PM runtime now.
> 
> Well we still need PM runtime in cppi41 to initialize it when it gets
> probed and idle it when not used even if musb modules are not loaded.
> 
> Unfortunately until musb_cppi41.c dma calls are fixed, we can't do
> any sane use counting in cppi41.c without adding timeout handling
> to it.
> 
> > More over, There is another possibility related to the current PM runtime implementation and autosuspend
> > - it might introduce delay between time when musb request desc push and time when cppi will actually
> > put this desc in HW queue if cppi was not active.
> > For example, there might not be -71 error seen if CPPI autosuspend is disabled
> > (cppi is active all the time) before plug-in hub and then USB drive.
> 
> I already checked that. The -71 error seems to be a separate issue.

OK found it. I can make v4.8.17 produce -71 errors with just commit
467d5c980709 ("usb: musb: Implement session bit based runtime PM for
musb-core") applied on top of it. So looks like we're missing some
musb quirk handling for the devctl session bit.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3] dmaengine: cppi41: Fix oops in cppi41_runtime_resume

[Bin Liu]

On Wed, Jan 18, 2017 at 10:44:32AM -0800, Tony Lindgren wrote:
> * Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> [170118 10:15]:
> > * Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org> [170118 10:05]:
> > > 
> > > 
> > > On 01/18/2017 10:53 AM, Tony Lindgren wrote:
> > > > Hi,
> > > > 
> > > > * Bin Liu <b-liu-l0cyMroinI0@public.gmane.org> [170118 06:26]:
> > > >> With this v3, I still get -71 error when a device is plugged to a hub to
> > > >> musb. It doesn't happen though if the device is already plugged to the hub
> > > >> before attaching the hub to musb.
> > > >>
> > > >> [  177.579300] usb 1-1: reset high-speed USB device number 2 using musb-hdrc
> > > >> [  177.719308] usb 1-1: device descriptor read/64, error -71
> > > >> [  178.350297] usb 1-1.1: new high-speed USB device number 4 using musb-hdrc
> > > > 
> > > > I think the -71 issue is a different regression. I've verified that v4.8.y
> > > > does not have this, but v4.9.y does have it.
> > > > 
> > > > So far the easiest way for me to reproduce the -71 problem is by plugging
> > > > a USB drive into a connected hub. If I connect the hub with USB drive already
> > > > plugged into the hub, it does not happen.
> ...
> 
> > > Sry, but wouldn't be more simple and safe just to drop PM runtime from this driver,
> > > as it is part of musb and musb PM state expected to be handled properly by PM runtime now.
> > 
> > Well we still need PM runtime in cppi41 to initialize it when it gets
> > probed and idle it when not used even if musb modules are not loaded.
> > 
> > Unfortunately until musb_cppi41.c dma calls are fixed, we can't do
> > any sane use counting in cppi41.c without adding timeout handling
> > to it.
> > 
> > > More over, There is another possibility related to the current PM runtime implementation and autosuspend
> > > - it might introduce delay between time when musb request desc push and time when cppi will actually
> > > put this desc in HW queue if cppi was not active.
> > > For example, there might not be -71 error seen if CPPI autosuspend is disabled
> > > (cppi is active all the time) before plug-in hub and then USB drive.
> > 
> > I already checked that. The -71 error seems to be a separate issue.
> 
> OK found it. I can make v4.8.17 produce -71 errors with just commit
> 467d5c980709 ("usb: musb: Implement session bit based runtime PM for
> musb-core") applied on top of it. So looks like we're missing some
> musb quirk handling for the devctl session bit.

Nice!

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3] dmaengine: cppi41: Fix oops in cppi41_runtime_resume

[Tony Lindgren]

* Bin Liu <b-liu-l0cyMroinI0@public.gmane.org> [170118 10:49]:
> On Wed, Jan 18, 2017 at 10:44:32AM -0800, Tony Lindgren wrote:
> > * Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> [170118 10:15]:
> > > * Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org> [170118 10:05]:
> > > > 
> > > > 
> > > > On 01/18/2017 10:53 AM, Tony Lindgren wrote:
> > > > > Hi,
> > > > > 
> > > > > * Bin Liu <b-liu-l0cyMroinI0@public.gmane.org> [170118 06:26]:
> > > > >> With this v3, I still get -71 error when a device is plugged to a hub to
> > > > >> musb. It doesn't happen though if the device is already plugged to the hub
> > > > >> before attaching the hub to musb.
> > > > >>
> > > > >> [  177.579300] usb 1-1: reset high-speed USB device number 2 using musb-hdrc
> > > > >> [  177.719308] usb 1-1: device descriptor read/64, error -71
> > > > >> [  178.350297] usb 1-1.1: new high-speed USB device number 4 using musb-hdrc
> > > > > 
> > > > > I think the -71 issue is a different regression. I've verified that v4.8.y
> > > > > does not have this, but v4.9.y does have it.
> > > > > 
> > > > > So far the easiest way for me to reproduce the -71 problem is by plugging
> > > > > a USB drive into a connected hub. If I connect the hub with USB drive already
> > > > > plugged into the hub, it does not happen.
> > ...
> > 
> > > > Sry, but wouldn't be more simple and safe just to drop PM runtime from this driver,
> > > > as it is part of musb and musb PM state expected to be handled properly by PM runtime now.
> > > 
> > > Well we still need PM runtime in cppi41 to initialize it when it gets
> > > probed and idle it when not used even if musb modules are not loaded.
> > > 
> > > Unfortunately until musb_cppi41.c dma calls are fixed, we can't do
> > > any sane use counting in cppi41.c without adding timeout handling
> > > to it.
> > > 
> > > > More over, There is another possibility related to the current PM runtime implementation and autosuspend
> > > > - it might introduce delay between time when musb request desc push and time when cppi will actually
> > > > put this desc in HW queue if cppi was not active.
> > > > For example, there might not be -71 error seen if CPPI autosuspend is disabled
> > > > (cppi is active all the time) before plug-in hub and then USB drive.
> > > 
> > > I already checked that. The -71 error seems to be a separate issue.
> > 
> > OK found it. I can make v4.8.17 produce -71 errors with just commit
> > 467d5c980709 ("usb: musb: Implement session bit based runtime PM for
> > musb-core") applied on top of it. So looks like we're missing some
> > musb quirk handling for the devctl session bit.
> 
> Nice!

And here's a fix for the error -71 regression.

Bin, can you review and test your earlier case mentioned in
commit 9298b4aad37e ("usb: musb: fix device hotplug behind hub") with
the patch below to make sure this is safe to do now starting with v4.9?

Regards,

Tony

8< -----------------------
>From tony Mon Sep 17 00:00:00 2001
From: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
Date: Wed, 18 Jan 2017 12:31:25 -0800
Subject: [PATCH] usb: musb: Fix host mode error -71 regression

Commit 467d5c980709 ("usb: musb: Implement session bit based runtime PM for
musb-core") started implementing musb generic runtime PM support by
introducing devctl register session bit based state control.

This caused a regression where if a USB mass storage device is connected
to a USB hub, we can get:

usb 1-1: reset high-speed USB device number 2 using musb-hdrc
usb 1-1: device descriptor read/64, error -71
usb 1-1.1: new high-speed USB device number 4 using musb-hdrc

This is because before the USB storage device is connected, musb is
in OTG_STATE_A_SUSPEND. And we currently only set need_finish_resume
in musb_stage0_irq() and the related code calling finish_resume_work
in musb_resume() and musb_runtime_resume() never gets called.

To fix the issue, we can call schedule_delayed_work() directly in
musb_stage0_irq() to have finish_resume_work run.

And we should no longer never get interrupts when when suspended.
We have changed musb to no longer need pm_runtime_irqsafe().
The need_finish_resume flag was added in commit 9298b4aad37e ("usb:
musb: fix device hotplug behind hub") and no longer applies as far
as I can tell. So let's just remove the earlier code that no longer
is needed.

Fixes: 467d5c980709 ("usb: musb: Implement session bit based
runtime PM for musb-core")
Reported-by: Bin Liu <b-liu-l0cyMroinI0@public.gmane.org>
Signed-off-by: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
---
 drivers/usb/musb/musb_core.c | 15 ++-------------
 drivers/usb/musb/musb_core.h |  1 -
 2 files changed, 2 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -594,11 +594,11 @@ static irqreturn_t musb_stage0_irq(struct musb *musb, u8 int_usb,
 						| MUSB_PORT_STAT_RESUME;
 				musb->rh_timer = jiffies
 					+ msecs_to_jiffies(USB_RESUME_TIMEOUT);
-				musb->need_finish_resume = 1;
-
 				musb->xceiv->otg->state = OTG_STATE_A_HOST;
 				musb->is_active = 1;
 				musb_host_resume_root_hub(musb);
+				schedule_delayed_work(&musb->finish_resume_work,
+					msecs_to_jiffies(USB_RESUME_TIMEOUT));
 				break;
 			case OTG_STATE_B_WAIT_ACON:
 				musb->xceiv->otg->state = OTG_STATE_B_PERIPHERAL;
@@ -2710,11 +2710,6 @@ static int musb_resume(struct device *dev)
 	mask = MUSB_DEVCTL_BDEVICE | MUSB_DEVCTL_FSDEV | MUSB_DEVCTL_LSDEV;
 	if ((devctl & mask) != (musb->context.devctl & mask))
 		musb->port1_status = 0;
-	if (musb->need_finish_resume) {
-		musb->need_finish_resume = 0;
-		schedule_delayed_work(&musb->finish_resume_work,
-				      msecs_to_jiffies(USB_RESUME_TIMEOUT));
-	}
 
 	/*
 	 * The USB HUB code expects the device to be in RPM_ACTIVE once it came
@@ -2766,12 +2761,6 @@ static int musb_runtime_resume(struct device *dev)
 
 	musb_restore_context(musb);
 
-	if (musb->need_finish_resume) {
-		musb->need_finish_resume = 0;
-		schedule_delayed_work(&musb->finish_resume_work,
-				msecs_to_jiffies(USB_RESUME_TIMEOUT));
-	}

Re: [PATCHv3] dmaengine: cppi41: Fix oops in cppi41_runtime_resume

[Bin Liu]

On Wed, Jan 18, 2017 at 12:48:59PM -0800, Tony Lindgren wrote:
> * Bin Liu <b-liu-l0cyMroinI0@public.gmane.org> [170118 10:49]:
> > On Wed, Jan 18, 2017 at 10:44:32AM -0800, Tony Lindgren wrote:
> > > * Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> [170118 10:15]:
> > > > * Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org> [170118 10:05]:
> > > > > 
> > > > > 
> > > > > On 01/18/2017 10:53 AM, Tony Lindgren wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > * Bin Liu <b-liu-l0cyMroinI0@public.gmane.org> [170118 06:26]:
> > > > > >> With this v3, I still get -71 error when a device is plugged to a hub to
> > > > > >> musb. It doesn't happen though if the device is already plugged to the hub
> > > > > >> before attaching the hub to musb.
> > > > > >>
> > > > > >> [  177.579300] usb 1-1: reset high-speed USB device number 2 using musb-hdrc
> > > > > >> [  177.719308] usb 1-1: device descriptor read/64, error -71
> > > > > >> [  178.350297] usb 1-1.1: new high-speed USB device number 4 using musb-hdrc
> > > > > > 
> > > > > > I think the -71 issue is a different regression. I've verified that v4.8.y
> > > > > > does not have this, but v4.9.y does have it.
> > > > > > 
> > > > > > So far the easiest way for me to reproduce the -71 problem is by plugging
> > > > > > a USB drive into a connected hub. If I connect the hub with USB drive already
> > > > > > plugged into the hub, it does not happen.
> > > ...
> > > 
> > > > > Sry, but wouldn't be more simple and safe just to drop PM runtime from this driver,
> > > > > as it is part of musb and musb PM state expected to be handled properly by PM runtime now.
> > > > 
> > > > Well we still need PM runtime in cppi41 to initialize it when it gets
> > > > probed and idle it when not used even if musb modules are not loaded.
> > > > 
> > > > Unfortunately until musb_cppi41.c dma calls are fixed, we can't do
> > > > any sane use counting in cppi41.c without adding timeout handling
> > > > to it.
> > > > 
> > > > > More over, There is another possibility related to the current PM runtime implementation and autosuspend
> > > > > - it might introduce delay between time when musb request desc push and time when cppi will actually
> > > > > put this desc in HW queue if cppi was not active.
> > > > > For example, there might not be -71 error seen if CPPI autosuspend is disabled
> > > > > (cppi is active all the time) before plug-in hub and then USB drive.
> > > > 
> > > > I already checked that. The -71 error seems to be a separate issue.
> > > 
> > > OK found it. I can make v4.8.17 produce -71 errors with just commit
> > > 467d5c980709 ("usb: musb: Implement session bit based runtime PM for
> > > musb-core") applied on top of it. So looks like we're missing some
> > > musb quirk handling for the devctl session bit.
> > 
> > Nice!
> 
> And here's a fix for the error -71 regression.
> 
> Bin, can you review and test your earlier case mentioned in
> commit 9298b4aad37e ("usb: musb: fix device hotplug behind hub") with
> the patch below to make sure this is safe to do now starting with v4.9?
> 

This solves the issue, and the patch looks good to me. Thanks for fixing
it up.

Regards,
-Bin.

> Regards,
> 
> Tony
> 
> 8< -----------------------
> From tony Mon Sep 17 00:00:00 2001
> From: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
> Date: Wed, 18 Jan 2017 12:31:25 -0800
> Subject: [PATCH] usb: musb: Fix host mode error -71 regression
> 
> Commit 467d5c980709 ("usb: musb: Implement session bit based runtime PM for
> musb-core") started implementing musb generic runtime PM support by
> introducing devctl register session bit based state control.
> 
> This caused a regression where if a USB mass storage device is connected
> to a USB hub, we can get:
> 
> usb 1-1: reset high-speed USB device number 2 using musb-hdrc
> usb 1-1: device descriptor read/64, error -71
> usb 1-1.1: new high-speed USB device number 4 using musb-hdrc
> 
> This is because before the USB storage device is connected, musb is
> in OTG_STATE_A_SUSPEND. And we currently only set need_finish_resume
> in musb_stage0_irq() and the related code calling finish_resume_work
> in musb_resume() and musb_runtime_resume() never gets called.
> 
> To fix the issue, we can call schedule_delayed_work() directly in
> musb_stage0_irq() to have finish_resume_work run.
> 
> And we should no longer never get interrupts when when suspended.
> We have changed musb to no longer need pm_runtime_irqsafe().
> The need_finish_resume flag was added in commit 9298b4aad37e ("usb:
> musb: fix device hotplug behind hub") and no longer applies as far
> as I can tell. So let's just remove the earlier code that no longer
> is needed.
> 
> Fixes: 467d5c980709 ("usb: musb: Implement session bit based
> runtime PM for musb-core")
> Reported-by: Bin Liu <b-liu-l0cyMroinI0@public.gmane.org>
> Signed-off-by: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
> ---
>  drivers/usb/musb/musb_core.c | 15 ++-------------
>  drivers/usb/musb/musb_core.h |  1 -
>  2 files changed, 2 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
> --- a/drivers/usb/musb/musb_core.c
> +++ b/drivers/usb/musb/musb_core.c
> @@ -594,11 +594,11 @@ static irqreturn_t musb_stage0_irq(struct musb *musb, u8 int_usb,
>  						| MUSB_PORT_STAT_RESUME;
>  				musb->rh_timer = jiffies
>  					+ msecs_to_jiffies(USB_RESUME_TIMEOUT);
> -				musb->need_finish_resume = 1;
> -
>  				musb->xceiv->otg->state = OTG_STATE_A_HOST;
>  				musb->is_active = 1;
>  				musb_host_resume_root_hub(musb);
> +				schedule_delayed_work(&musb->finish_resume_work,
> +					msecs_to_jiffies(USB_RESUME_TIMEOUT));
>  				break;
>  			case OTG_STATE_B_WAIT_ACON:
>  				musb->xceiv->otg->state = OTG_STATE_B_PERIPHERAL;
> @@ -2710,11 +2710,6 @@ static int musb_resume(struct device *dev)
>  	mask = MUSB_DEVCTL_BDEVICE | MUSB_DEVCTL_FSDEV | MUSB_DEVCTL_LSDEV;
>  	if ((devctl & mask) != (musb->context.devctl & mask))
>  		musb->port1_status = 0;
> -	if (musb->need_finish_resume) {
> -		musb->need_finish_resume = 0;
> -		schedule_delayed_work(&musb->finish_resume_work,
> -				      msecs_to_jiffies(USB_RESUME_TIMEOUT));
> -	}
>  
>  	/*
>  	 * The USB HUB code expects the device to be in RPM_ACTIVE once it came
> @@ -2766,12 +2761,6 @@ static int musb_runtime_resume(struct device *dev)
>  
>  	musb_restore_context(musb);
>  
> -	if (musb->need_finish_resume) {
> -		musb->need_finish_resume = 0;
> -		schedule_delayed_work(&musb->finish_resume_work,
> -				msecs_to_jiffies(USB_RESUME_TIMEOUT));
> -	}
> -
>  	spin_lock_irqsave(&musb->lock, flags);
>  	error = musb_run_resume_work(musb);
>  	if (error)
> diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h
> --- a/drivers/usb/musb/musb_core.h
> +++ b/drivers/usb/musb/musb_core.h
> @@ -410,7 +410,6 @@ struct musb {
>  
>  	/* is_suspended means USB B_PERIPHERAL suspend */
>  	unsigned		is_suspended:1;
> -	unsigned		need_finish_resume :1;
>  
>  	/* may_wakeup means remote wakeup is enabled */
>  	unsigned		may_wakeup:1;
> -- 
> 2.11.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3] dmaengine: cppi41: Fix oops in cppi41_runtime_resume

[Tony Lindgren]

* Bin Liu <b-liu-l0cyMroinI0@public.gmane.org> [170118 13:14]:
> On Wed, Jan 18, 2017 at 12:48:59PM -0800, Tony Lindgren wrote:
> > And here's a fix for the error -71 regression.
> > 
> > Bin, can you review and test your earlier case mentioned in
> > commit 9298b4aad37e ("usb: musb: fix device hotplug behind hub") with
> > the patch below to make sure this is safe to do now starting with v4.9?
> > 
> 
> This solves the issue, and the patch looks good to me. Thanks for fixing
> it up.

OK will post a two patch musb fixes series with this and the
short in dma fix as that's what's keeping cpp41 channels hanging.

Then after that I'll post a series of two cpp41 fixes.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3] dmaengine: cppi41: Fix oops in cppi41_runtime_resume

[Grygorii Strashko]

On 01/18/2017 12:15 PM, Tony Lindgren wrote:
> * Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org> [170118 10:05]:
>>
>>
>> On 01/18/2017 10:53 AM, Tony Lindgren wrote:
>>> Hi,
>>>
>>> * Bin Liu <b-liu-l0cyMroinI0@public.gmane.org> [170118 06:26]:
>>>> With this v3, I still get -71 error when a device is plugged to a hub to
>>>> musb. It doesn't happen though if the device is already plugged to the hub
>>>> before attaching the hub to musb.
>>>>
>>>> [  177.579300] usb 1-1: reset high-speed USB device number 2 using musb-hdrc
>>>> [  177.719308] usb 1-1: device descriptor read/64, error -71
>>>> [  178.350297] usb 1-1.1: new high-speed USB device number 4 using musb-hdrc
>>>
>>> I think the -71 issue is a different regression. I've verified that v4.8.y
>>> does not have this, but v4.9.y does have it.
>>>
>>> So far the easiest way for me to reproduce the -71 problem is by plugging
>>> a USB drive into a connected hub. If I connect the hub with USB drive already
>>> plugged into the hub, it does not happen.
>>>
>>> With the hub connected musb is already active when the USB drive is plugged
>>
>> This is not exactly try, i think, because cppi can be in inactive state
>> because of autosuspend (all calls are paired).
> 
> Musb is active when there's something connected meaning cppi41 is clocked
> when a hub is connected. The actual runtime PM implementation happens for
> the interconnect target module for both musb and cppi41 in hwmod.c code.
> 
>>> in. So I'm now suspecting this -71 regression is not related to runtime PM
>>> changes. Bisect and boot and plug devices time I think unless you have
>>> better ideas?
>>>
>>>> And I still get -115 error flooding with thumb drive.
>>>>
>>>> [  236.880068] cppi41-dma-engine 47400000.dma-controller: cppi41_irq pm runtime get: -115
>>>>
>>>> I tested with TI AM335x GP EVM. The problems happen on both musb ports.
>>>
>>> OK. So it's pointless to try to play with the autosuspend timeout.
>>>
>>> Let's just do a WARN(!pm_runtime_active(cdd->ddev.dev->parent)) there
>>> until we have musb_cppi41.c dma calls properly paired and don't have
>>> dma requests lingering.
>>>
>>> Care to try the updated patch below? It won't help with the -71 issue
>>> but the $subject issue should be fixed. And you should not see the
>>> WARN() trigger with your tests presumably.
>>>
>>
>> Sry, but wouldn't be more simple and safe just to drop PM runtime from this driver,
>> as it is part of musb and musb PM state expected to be handled properly by PM runtime now.
> 
> Well we still need PM runtime in cppi41 to initialize it when it gets
> probed and idle it when not used even if musb modules are not loaded.
> 
> Unfortunately until musb_cppi41.c dma calls are fixed, we can't do
> any sane use counting in cppi41.c without adding timeout handling
> to it.
> 

Just thinking, may be cppi41 should not be platform device at all
and it might be reasonable to have it as lib with cppi41_init()/cppi41_remove(),
so musb SoC glue layer will initialize it, because it provides services to
other HW blocks withing musb only.


-- 
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3] dmaengine: cppi41: Fix oops in cppi41_runtime_resume

[Tony Lindgren]

* Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org> [170118 11:18]:
> Just thinking, may be cppi41 should not be platform device at all
> and it might be reasonable to have it as lib with cppi41_init()/cppi41_remove(),
> so musb SoC glue layer will initialize it, because it provides services to
> other HW blocks withing musb only.

Nah, we already have things almost working :) And if some TI SoC with
general purpose cppi41 pops up like a DSP running Linux, we're in big
trouble. And musb is already a mess as is..

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3] dmaengine: cppi41: Fix oops in cppi41_runtime_resume

[Bin Liu]

On Wed, Jan 18, 2017 at 08:53:09AM -0800, Tony Lindgren wrote:
> Hi,
> 
> * Bin Liu <b-liu-l0cyMroinI0@public.gmane.org> [170118 06:26]:
> > With this v3, I still get -71 error when a device is plugged to a hub to
> > musb. It doesn't happen though if the device is already plugged to the hub
> > before attaching the hub to musb.
> > 
> > [  177.579300] usb 1-1: reset high-speed USB device number 2 using musb-hdrc
> > [  177.719308] usb 1-1: device descriptor read/64, error -71
> > [  178.350297] usb 1-1.1: new high-speed USB device number 4 using musb-hdrc
> 
> I think the -71 issue is a different regression. I've verified that v4.8.y
> does not have this, but v4.9.y does have it.

I will take a look on this one.

> 
> So far the easiest way for me to reproduce the -71 problem is by plugging
> a USB drive into a connected hub. If I connect the hub with USB drive already
> plugged into the hub, it does not happen.
> 
> With the hub connected musb is already active when the USB drive is plugged
> in. So I'm now suspecting this -71 regression is not related to runtime PM
> changes. Bisect and boot and plug devices time I think unless you have
> better ideas?
> 
> > And I still get -115 error flooding with thumb drive.
> > 
> > [  236.880068] cppi41-dma-engine 47400000.dma-controller: cppi41_irq pm runtime get: -115
> > 
> > I tested with TI AM335x GP EVM. The problems happen on both musb ports.
> 
> OK. So it's pointless to try to play with the autosuspend timeout.
> 
> Let's just do a WARN(!pm_runtime_active(cdd->ddev.dev->parent)) there
> until we have musb_cppi41.c dma calls properly paired and don't have
> dma requests lingering.
> 
> Care to try the updated patch below? It won't help with the -71 issue
> but the $subject issue should be fixed. And you should not see the
> WARN() trigger with your tests presumably.

Yes, now no WARN() and no -115 any more. Thanks.

Regards,
-Bin.

> 
> Regards,
> 
> Tony
> 
> 8< ----------------------------
> From tony Mon Sep 17 00:00:00 2001
> From: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
> Date: Mon, 16 Jan 2017 09:52:10 -0800
> Subject: [PATCH] dmaengine: cppi41: Fix oops in cppi41_runtime_resume
> 
> Commit fdea2d09b997 ("dmaengine: cppi41: Add basic PM runtime support")
> together with recent MUSB changes allowed USB and DMA on BeagleBone to idle
> when no cable is connected. But looks like few corner case issues still
> remain.
> 
> Looks like just by re-plugging USB cable about ten or so times on BeagleBone
> when configured in USB peripheral mode we can get warnings and eventually
> trigger an oops in cppi41 DMA:
> 
> WARNING: CPU: 0 PID: 14 at drivers/dma/cppi41.c:1154 cppi41_runtime_suspend+
> x28/0x38 [cppi41]
> ...
> 
> WARNING: CPU: 0 PID: 14 at drivers/dma/cppi41.c:452
> push_desc_queue+0x94/0x9c [cppi41]
> ...
> 
> Unable to handle kernel NULL pointer dereference at virtual
> address 00000104
> pgd = c0004000
> [00000104] *pgd=00000000
> Internal error: Oops: 805 [#1] SMP ARM
> ...
> [<bf0d92cc>] (cppi41_runtime_resume [cppi41]) from [<c0589838>]
> (__rpm_callback+0xc0/0x214)
> [<c0589838>] (__rpm_callback) from [<c05899ac>] (rpm_callback+0x20/0x80)
> [<c05899ac>] (rpm_callback) from [<c0589460>] (rpm_resume+0x504/0x78c)
> [<c0589460>] (rpm_resume) from [<c058a1a0>] (pm_runtime_work+0x60/0xa8)
> [<c058a1a0>] (pm_runtime_work) from [<c0156120>] (process_one_work+0x2b4/0x808)
> 
> This is because of a race with runtime PM and cppi41_dma_issue_pending()
> as reported by Alexandre Bailon <abailon-rdvid1DuHRBWk0Htik3J/w@public.gmane.org> in earlier
> set of patches. Based on mailing list discussions we however came to the
> conclusion that a different fix from Alexandre's fix is needed in order
> to guarantee that DMA is really active when we try to use it.
> 
> To fix the issue, we need to add a driver specific flag as we otherwise
> can have -EINPROGRESS state set by runtime PM and can't rely on
> pm_runtime_active() to tell us when we can use the DMA.
> 
> And we need to make sure the DMA transfers get triggered in the queued
> order. So let's always queue the transfers, then flush the queue
> from both cppi41_dma_issue_pending() and cppi41_runtime_resume()
> as suggested by Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org> in an
> earlier example patch.
> 
> For reference, this is also documented in Documentation/power/runtime_pm.txt
> in the example at the end of the file as pointed out by Grygorii Strashko
> <grygorii.strashko-l0cyMroinI0@public.gmane.org>.
> 
> Based on earlier patches from Alexandre Bailon <abailon-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
> and Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org> modified based on
> testing and what was discussed on the mailing lists.
> 
> Note that additional patches are still needed depending on how we want
> to handle the cppi41 runtime PM. So far cppi41 is always coupled with
> musb driver, and musb guarantees that cppi41 is always active during
> use. So until we have a better solution, let's just WARN() if the musb
> parent is not active to avoid pointless EINPROGRESS warnings during
> USB mass storage enumeration. As cppi41 is properly clocked with musb
> being active, we can safely do this.
> 
> Fixes: fdea2d09b997 ("dmaengine: cppi41: Add basic PM runtime support")
> Cc: Andy Shevchenko <andy.shevchenko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: Bin Liu <b-liu-l0cyMroinI0@public.gmane.org>
> Cc: Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org>
> Cc: Kevin Hilman <khilman-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
> Cc: Patrick Titiano <ptitiano-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
> Cc: Sergei Shtylyov <sergei.shtylyov-M4DtvfQ/ZS1MRgGoP+s0PdBPR1lH4CV8@public.gmane.org>
> Reported-by: Alexandre Bailon <abailon-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
> ---
>  drivers/dma/cppi41.c | 51 ++++++++++++++++++++++++++++-----------------------
>  1 file changed, 28 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c
> --- a/drivers/dma/cppi41.c
> +++ b/drivers/dma/cppi41.c
> @@ -153,6 +153,8 @@ struct cppi41_dd {
>  
>  	/* context for suspend/resume */
>  	unsigned int dma_tdfdq;
> +
> +	bool is_suspended;
>  };
>  
>  #define FIST_COMPLETION_QUEUE	93
> @@ -317,12 +319,10 @@ static irqreturn_t cppi41_irq(int irq, void *data)
>  
>  		while (val) {
>  			u32 desc, len;
> -			int error;
>  
> -			error = pm_runtime_get(cdd->ddev.dev);
> -			if (error < 0)
> -				dev_err(cdd->ddev.dev, "%s pm runtime get: %i\n",
> -					__func__, error);
> +			/* Revisit when musb_cppi41.c dma calls are paired */
> +			WARN(!pm_runtime_active(cdd->ddev.dev->parent),
> +			     "%s parent not active?\n", __func__);
>  
>  			q_num = __fls(val);
>  			val &= ~(1 << q_num);
> @@ -343,9 +343,6 @@ static irqreturn_t cppi41_irq(int irq, void *data)
>  			c->residue = pd_trans_len(c->desc->pd6) - len;
>  			dma_cookie_complete(&c->txd);
>  			dmaengine_desc_get_callback_invoke(&c->txd, NULL);
> -
> -			pm_runtime_mark_last_busy(cdd->ddev.dev);
> -			pm_runtime_put_autosuspend(cdd->ddev.dev);
>  		}
>  	}
>  	return IRQ_HANDLED;
> @@ -457,20 +454,26 @@ static void push_desc_queue(struct cppi41_channel *c)
>  	cppi_writel(reg, cdd->qmgr_mem + QMGR_QUEUE_D(c->q_num));
>  }
>  
> -static void pending_desc(struct cppi41_channel *c)
> +/*
> + * Caller must hold cdd->lock to prevent push_desc_queue()
> + * getting called out of order. We have both cppi41_dma_issue_pending()
> + * and cppi41_runtime_resume() call this function.
> + */
> +static void cppi41_run_queue(struct cppi41_dd *cdd)
>  {
> -	struct cppi41_dd *cdd = c->cdd;
> -	unsigned long flags;
> +	struct cppi41_channel *c, *_c;
>  
> -	spin_lock_irqsave(&cdd->lock, flags);
> -	list_add_tail(&c->node, &cdd->pending);
> -	spin_unlock_irqrestore(&cdd->lock, flags);
> +	list_for_each_entry_safe(c, _c, &cdd->pending, node) {
> +		push_desc_queue(c);
> +		list_del(&c->node);
> +	}
>  }
>  
>  static void cppi41_dma_issue_pending(struct dma_chan *chan)
>  {
>  	struct cppi41_channel *c = to_cpp41_chan(chan);
>  	struct cppi41_dd *cdd = c->cdd;
> +	unsigned long flags;
>  	int error;
>  
>  	error = pm_runtime_get(cdd->ddev.dev);
> @@ -482,10 +485,11 @@ static void cppi41_dma_issue_pending(struct dma_chan *chan)
>  		return;
>  	}
>  
> -	if (likely(pm_runtime_active(cdd->ddev.dev)))
> -		push_desc_queue(c);
> -	else
> -		pending_desc(c);
> +	spin_lock_irqsave(&cdd->lock, flags);
> +	list_add_tail(&c->node, &cdd->pending);
> +	if (!cdd->is_suspended)
> +		cppi41_run_queue(cdd);
> +	spin_unlock_irqrestore(&cdd->lock, flags);
>  
>  	pm_runtime_mark_last_busy(cdd->ddev.dev);
>  	pm_runtime_put_autosuspend(cdd->ddev.dev);
> @@ -1150,8 +1154,12 @@ static int __maybe_unused cppi41_resume(struct device *dev)
>  static int __maybe_unused cppi41_runtime_suspend(struct device *dev)
>  {
>  	struct cppi41_dd *cdd = dev_get_drvdata(dev);
> +	unsigned long flags;
>  
> +	spin_lock_irqsave(&cdd->lock, flags);
> +	cdd->is_suspended = true;
>  	WARN_ON(!list_empty(&cdd->pending));
> +	spin_unlock_irqrestore(&cdd->lock, flags);
>  
>  	return 0;
>  }
> @@ -1159,14 +1167,11 @@ static int __maybe_unused cppi41_runtime_suspend(struct device *dev)
>  static int __maybe_unused cppi41_runtime_resume(struct device *dev)
>  {
>  	struct cppi41_dd *cdd = dev_get_drvdata(dev);
> -	struct cppi41_channel *c, *_c;
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&cdd->lock, flags);
> -	list_for_each_entry_safe(c, _c, &cdd->pending, node) {
> -		push_desc_queue(c);
> -		list_del(&c->node);
> -	}
> +	cdd->is_suspended = false;
> +	cppi41_run_queue(cdd);
>  	spin_unlock_irqrestore(&cdd->lock, flags);
>  
>  	return 0;
> -- 
> 2.11.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3] dmaengine: cppi41: Fix oops in cppi41_runtime_resume

[Tony Lindgren]

* Bin Liu <b-liu-l0cyMroinI0@public.gmane.org> [170118 10:43]:
> On Wed, Jan 18, 2017 at 08:53:09AM -0800, Tony Lindgren wrote:
> > Hi,
> > 
> > * Bin Liu <b-liu-l0cyMroinI0@public.gmane.org> [170118 06:26]:
> > > With this v3, I still get -71 error when a device is plugged to a hub to
> > > musb. It doesn't happen though if the device is already plugged to the hub
> > > before attaching the hub to musb.
> > > 
> > > [  177.579300] usb 1-1: reset high-speed USB device number 2 using musb-hdrc
> > > [  177.719308] usb 1-1: device descriptor read/64, error -71
> > > [  178.350297] usb 1-1.1: new high-speed USB device number 4 using musb-hdrc
> > 
> > I think the -71 issue is a different regression. I've verified that v4.8.y
> > does not have this, but v4.9.y does have it.
> 
> I will take a look on this one.

Found the commit causing, see the mail I just sent. It's 467d5c980709.

> > Care to try the updated patch below? It won't help with the -71 issue
> > but the $subject issue should be fixed. And you should not see the
> > WARN() trigger with your tests presumably.
> 
> Yes, now no WARN() and no -115 any more. Thanks.

OK. I'll take a look at what additional quirk handling the devctl
session bit needs for the -71 error and then with these we should
have things working again.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html