Two musb fixes for v4.10-rc cycle

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 <[email protected]>

Signed-off-by Tony Lindgren <[email protected]>

---
 drivers/usb/musb/musb_core.c | 15 ++-------------
 drivers/usb/musb/musb_core.h |  1 -
 2 files changed, 2 insertions(+), 14 deletions(-)

drivers/usb/musb/musb_core.c +2 -13

@@ -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)

drivers/usb/musb/musb_core.h +0 -1

@@ -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;

Signature · 1 line

2.11.0

At least with the cppi41 dma, size 1 in dma transfers will just wait

until the device is disconnected. This causes timeouts in cppi41 dma runtime PM.

Also the initial size 8 transfers take about 200ms to complete when plugging a USB mass storage device to a hub. But we probably want to keep those to avoid using PIO.

Fix the issue by adding a quirk for cppi41 and skip size 1 in dma if set.

Note that additional cpp41 patches are needed to avoid error -115 messages, but that can be applied separately.

Fixes fdea2d09b997 ("dmaengine: cppi41: Add basic PM runtime support")

Signed-off-by Tony Lindgren <[email protected]>

---
 drivers/usb/musb/musb_cppi41.c |  1 +
 drivers/usb/musb/musb_dma.h    |  3 +++
 drivers/usb/musb/musb_host.c   | 16 +++++++++++++++-
 3 files changed, 19 insertions(+), 1 deletion(-)

drivers/usb/musb/musb_cppi41.c +1 -0

@@ -695,6 +695,7 @@ cppi41_dma_controller_create(struct musb *musb, void __iomem *base)
 	controller->controller.channel_program = cppi41_dma_channel_program;
 	controller->controller.channel_abort = cppi41_dma_channel_abort;
 	controller->controller.is_compatible = cppi41_is_compatible;
+	controller->controller.quirks = MUSB_DMA_QUIRK_CPPI41_IN;
 
 	ret = cppi41_dma_controller_start(controller);
 	if (ret)

drivers/usb/musb/musb_dma.h +3 -0

@@ -171,6 +171,8 @@ dma_channel_status(struct dma_channel *c)
 	return (is_dma_capable() && c) ? c->status : MUSB_DMA_STATUS_UNKNOWN;
 }
 
+#define MUSB_DMA_QUIRK_CPPI41_IN	BIT(0)
+
 /**
  * struct dma_controller - A DMA Controller.
  * @start: call this to start a DMA controller;

@@ -196,6 +198,7 @@ struct dma_controller {
 	int			(*is_compatible)(struct dma_channel *channel,
 							u16 maxpacket,
 							void *buf, u32 length);
+	unsigned int quirks;
 };
 
 /* called after channel_program(), may indicate a fault */

drivers/usb/musb/musb_host.c +2 -0

@@ -743,6 +743,8 @@ static void musb_ep_program(struct musb *musb, u8 epnum,
 
 	musb_ep_select(mbase, epnum);
 
+	dma_controller = musb->dma_controller;
+
 	if (is_out && !len) {
 		use_dma = 0;
 		csr = musb_readw(epio, MUSB_TXCSR);

Quoted 1 line · Tony Lindgren

At least with the cppi41 dma, size 1 in dma transfers will just wait

In which case do you see the size 1 transfer? using testusb?

Quoted 7 lines

until the device is disconnected. This causes timeouts in cppi41 dma runtime PM.

Also the initial size 8 transfers take about 200ms to complete when plugging a USB mass storage device to a hub. But we probably want to keep those to avoid using PIO.

Quoted 2 lines

Fix the issue by adding a quirk for cppi41 and skip size 1 in dma if set.

It is fine to bypass dma for size 1 transfers, due to the dma setup overhead. But I'd like to know the test case to understand why it hangs.

Quoted 15 lines · Bin Liu

Quoted 1 line · Tony Lindgren

At least with the cppi41 dma, size 1 in dma transfers will just wait

In which case do you see the size 1 transfer? using testusb?

Quoted 9 lines

until the device is disconnected. This causes timeouts in cppi41 dma runtime PM.

Also the initial size 8 transfers take about 200ms to complete when plugging a USB mass storage device to a hub. But we probably want to keep those to avoid using PIO.

Fix the issue by adding a quirk for cppi41 and skip size 1 in dma if set.

Quoted 2 lines

It is fine to bypass dma for size 1 transfers, due to the dma setup overhead. But I'd like to know the test case to understand why it hangs.

The test case is the same old connect a USB mass storage device to a hub. There we see the initial size 1 transfer in the beginning. That transfer seems to never complete with cppi41 and the transfer will stay active until the device is disconnected. Currently we don't seem to have any timeout mechanism in cppi41 for that.

Quoted 16 lines · Tony Lindgren

Quoted 15 lines · Bin Liu

Quoted 1 line · Tony Lindgren

At least with the cppi41 dma, size 1 in dma transfers will just wait

In which case do you see the size 1 transfer? using testusb?

Quoted 9 lines

until the device is disconnected. This causes timeouts in cppi41 dma runtime PM.

Also the initial size 8 transfers take about 200ms to complete when plugging a USB mass storage device to a hub. But we probably want to keep those to avoid using PIO.

Fix the issue by adding a quirk for cppi41 and skip size 1 in dma if set.

Quoted 8 lines

Quoted 2 lines

It is fine to bypass dma for size 1 transfers, due to the dma setup overhead. But I'd like to know the test case to understand why it hangs.

The test case is the same old connect a USB mass storage device to a hub. There we see the initial size 1 transfer in the beginning. That transfer seems to never complete with cppi41 and the transfer will stay active until the device is disconnected. Currently we don't seem to have any timeout mechanism in cppi41 for that.

Ok, I will take a look maybe sometime next week to understand the traffic.

BTY, it seems you missed my comment at the end which is about limiting the transfer size.

Quoted 26 lines · Bin Liu

Quoted 24 lines · Tony Lindgren

Quoted 12 lines · Bin Liu

Quoted 1 line · Tony Lindgren

At least with the cppi41 dma, size 1 in dma transfers will just wait

In which case do you see the size 1 transfer? using testusb?

Quoted 7 lines

until the device is disconnected. This causes timeouts in cppi41 dma runtime PM.

Also the initial size 8 transfers take about 200ms to complete when plugging a USB mass storage device to a hub. But we probably want to keep those to avoid using PIO.

Quoted 5 lines

Quoted 2 lines

Fix the issue by adding a quirk for cppi41 and skip size 1 in dma if set.

It is fine to bypass dma for size 1 transfers, due to the dma setup overhead. But I'd like to know the test case to understand why it hangs.

The test case is the same old connect a USB mass storage device to a hub. There we see the initial size 1 transfer in the beginning. That transfer seems to never complete with cppi41 and the transfer will stay active until the device is disconnected. Currently we don't seem to have any timeout mechanism in cppi41 for that.

Quoted 5 lines

Ok, I will take a look maybe sometime next week to understand the traffic.

BTY, it seems you missed my comment at the end which is about limiting the transfer size.

Oops sorry yeah you're right, -ENOTENOUGHCOFFEEYET.

Quoted 33 lines · Tony Lindgren

Quoted 31 lines · Bin Liu

Quoted 16 lines · Tony Lindgren

Quoted 15 lines · Bin Liu

Quoted 1 line · Tony Lindgren

At least with the cppi41 dma, size 1 in dma transfers will just wait

In which case do you see the size 1 transfer? using testusb?

Quoted 9 lines

until the device is disconnected. This causes timeouts in cppi41 dma runtime PM.

Also the initial size 8 transfers take about 200ms to complete when plugging a USB mass storage device to a hub. But we probably want to keep those to avoid using PIO.

Fix the issue by adding a quirk for cppi41 and skip size 1 in dma if set.

Quoted 8 lines

Quoted 2 lines

It is fine to bypass dma for size 1 transfers, due to the dma setup overhead. But I'd like to know the test case to understand why it hangs.

The test case is the same old connect a USB mass storage device to a hub. There we see the initial size 1 transfer in the beginning. That transfer seems to never complete with cppi41 and the transfer will stay active until the device is disconnected. Currently we don't seem to have any timeout mechanism in cppi41 for that.

Ok, I will take a look maybe sometime next week to understand the traffic.

BTY, it seems you missed my comment at the end which is about limiting the transfer size.

Quoted 1 line

Oops sorry yeah you're right, -ENOTENOUGHCOFFEEYET.

Oh and testing that fix made me notice that this patch 2/2 problem is now gone with my current cppi41 dma fixes. So let's drop this patch 2/2, it seems to be just a workaround for a runtime PM autosuspend delay getting timed out for the dma transfer.

Patch 1/2 of this series should be still applied for the -rc cycle.

Quoted 36 lines · Tony Lindgren

Quoted 34 lines · Tony Lindgren

Quoted 26 lines · Bin Liu

Quoted 16 lines · Tony Lindgren

Quoted 15 lines · Bin Liu

Quoted 1 line · Tony Lindgren

At least with the cppi41 dma, size 1 in dma transfers will just wait

In which case do you see the size 1 transfer? using testusb?

Quoted 9 lines

until the device is disconnected. This causes timeouts in cppi41 dma runtime PM.

Also the initial size 8 transfers take about 200ms to complete when plugging a USB mass storage device to a hub. But we probably want to keep those to avoid using PIO.

Fix the issue by adding a quirk for cppi41 and skip size 1 in dma if set.

Quoted 8 lines

Quoted 2 lines

It is fine to bypass dma for size 1 transfers, due to the dma setup overhead. But I'd like to know the test case to understand why it hangs.

The test case is the same old connect a USB mass storage device to a hub. There we see the initial size 1 transfer in the beginning. That transfer seems to never complete with cppi41 and the transfer will stay active until the device is disconnected. Currently we don't seem to have any timeout mechanism in cppi41 for that.

Quoted 5 lines

Ok, I will take a look maybe sometime next week to understand the traffic.

BTY, it seems you missed my comment at the end which is about limiting the transfer size.

Oops sorry yeah you're right, -ENOTENOUGHCOFFEEYET.

Quoted 6 lines

Oh and testing that fix made me notice that this patch 2/2 problem is now gone with my current cppi41 dma fixes. So let's drop this patch 2/2, it seems to be just a workaround for a runtime PM autosuspend delay getting timed out for the dma transfer.

Patch 1/2 of this series should be still applied for the -rc cycle.

Sounds good. I wil do more check on 1/2 then send it to Greg.