[PATCHv2 0/2] Two musb fixes for v4.10-rc cycle

[PATCHv2 0/2] Two musb fixes for v4.10-rc cycle

[Tony Lindgren]

Hi all,

Here are two fixes for v4.10-rc cycle to deal with error -75 and
-115 issues when plugging in USB mass storage device to a hub.

Note that I will also post two cppi41 dma related patches, but
those can be applied separately.

Regards,

Tony

Changes since v1:

- Handle in transfer quirk separately with a cpp41 flag and don't
  trash the TX registers in RX dma quirk

- Limit quirk to cpp41 in dma sizes 0 - 1 only

- Update description accordingly

Tony Lindgren (2):
  usb: musb: Fix host mode error -71 regression
  usb: musb: Size 1 dma in transfers won't complete with cpp41

 drivers/usb/musb/musb_core.c   | 15 ++-------------
 drivers/usb/musb/musb_core.h   |  1 -
 drivers/usb/musb/musb_cppi41.c |  1 +
 drivers/usb/musb/musb_dma.h    |  3 +++
 drivers/usb/musb/musb_host.c   | 16 +++++++++++++++-
 5 files changed, 21 insertions(+), 15 deletions(-)

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

[PATCH 1/2] usb: musb: Fix host mode error -71 regression

[Tony Lindgren]

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: [PATCH 1/2] usb: musb: Fix host mode error -71 regression

[Bin Liu]

On Wed, Jan 18, 2017 at 06:29:58PM -0800, Tony Lindgren wrote:
> 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>

Applied. Thanks.
-Bin.

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

[PATCH 2/2] usb: musb: Size 1 dma in transfers won't complete with cpp41

[Tony Lindgren]

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 <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
---
 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(-)

diff --git a/drivers/usb/musb/musb_cppi41.c b/drivers/usb/musb/musb_cppi41.c
--- a/drivers/usb/musb/musb_cppi41.c
+++ b/drivers/usb/musb/musb_cppi41.c
@@ -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)
diff --git a/drivers/usb/musb/musb_dma.h b/drivers/usb/musb/musb_dma.h
--- a/drivers/usb/musb/musb_dma.h
+++ b/drivers/usb/musb/musb_dma.h
@@ -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 */
diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
--- a/drivers/usb/musb/musb_host.c
+++ b/drivers/usb/musb/musb_host.c
@@ -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);
@@ -751,8 +753,20 @@ static void musb_ep_program(struct musb *musb, u8 epnum,
 		hw_ep->tx_channel = NULL;
 	}
 
+	/*
+	 * At least cppi41 in dma will just hang with size of 1 until the
+	 * device is disconnected.
+	 */
+	if (!is_out && dma_controller &&
+	    (dma_controller->quirks & MUSB_DMA_QUIRK_CPPI41_IN)) {
+		use_dma = 0;
+		csr = musb_readw(epio, MUSB_RXCSR);
+		csr &= ~MUSB_RXCSR_DMAENAB;
+		musb_writew(epio, MUSB_RXCSR, csr);
+		hw_ep->rx_channel = NULL;
+	}
+
 	/* candidate for DMA? */
-	dma_controller = musb->dma_controller;
 	if (use_dma && is_dma_capable() && epnum && dma_controller) {
 		dma_channel = is_out ? hw_ep->tx_channel : hw_ep->rx_channel;
 		if (!dma_channel) {
-- 
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: [PATCH 2/2] usb: musb: Size 1 dma in transfers won't complete with cpp41

[Bin Liu]

On Wed, Jan 18, 2017 at 06:29:59PM -0800, Tony Lindgren wrote:
> 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?

> 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.

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.

> 
> 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 <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
> ---
>  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(-)
> 
> diff --git a/drivers/usb/musb/musb_cppi41.c b/drivers/usb/musb/musb_cppi41.c
> --- a/drivers/usb/musb/musb_cppi41.c
> +++ b/drivers/usb/musb/musb_cppi41.c
> @@ -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)
> diff --git a/drivers/usb/musb/musb_dma.h b/drivers/usb/musb/musb_dma.h
> --- a/drivers/usb/musb/musb_dma.h
> +++ b/drivers/usb/musb/musb_dma.h
> @@ -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 */
> diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
> --- a/drivers/usb/musb/musb_host.c
> +++ b/drivers/usb/musb/musb_host.c
> @@ -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);
> @@ -751,8 +753,20 @@ static void musb_ep_program(struct musb *musb, u8 epnum,
>  		hw_ep->tx_channel = NULL;
>  	}
>  
> +	/*
> +	 * At least cppi41 in dma will just hang with size of 1 until the
> +	 * device is disconnected.
> +	 */
> +	if (!is_out && dma_controller &&
> +	    (dma_controller->quirks & MUSB_DMA_QUIRK_CPPI41_IN)) {

Forget to add size constraint here?

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: [PATCH 2/2] usb: musb: Size 1 dma in transfers won't complete with cpp41

[Tony Lindgren]

* Bin Liu <b-liu-l0cyMroinI0@public.gmane.org> [170118 19:42]:
> On Wed, Jan 18, 2017 at 06:29:59PM -0800, Tony Lindgren wrote:
> > 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?
> 
> > 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.
> 
> 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.

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: [PATCH 2/2] usb: musb: Size 1 dma in transfers won't complete with cpp41

[Bin Liu]

On Thu, Jan 19, 2017 at 07:04:57AM -0800, Tony Lindgren wrote:
> * Bin Liu <b-liu-l0cyMroinI0@public.gmane.org> [170118 19:42]:
> > On Wed, Jan 18, 2017 at 06:29:59PM -0800, Tony Lindgren wrote:
> > > 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?
> > 
> > > 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.
> > 
> > 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.

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: [PATCH 2/2] usb: musb: Size 1 dma in transfers won't complete with cpp41

[Tony Lindgren]

* Bin Liu <b-liu-l0cyMroinI0@public.gmane.org> [170119 07:14]:
> On Thu, Jan 19, 2017 at 07:04:57AM -0800, Tony Lindgren wrote:
> > * Bin Liu <b-liu-l0cyMroinI0@public.gmane.org> [170118 19:42]:
> > > On Wed, Jan 18, 2017 at 06:29:59PM -0800, Tony Lindgren wrote:
> > > > 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?
> > > 
> > > > 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.
> > > 
> > > 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.

Oops sorry yeah you're right, -ENOTENOUGHCOFFEEYET.

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: [PATCH 2/2] usb: musb: Size 1 dma in transfers won't complete with cpp41

[Tony Lindgren]

* Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> [170119 07:57]:
> * Bin Liu <b-liu-l0cyMroinI0@public.gmane.org> [170119 07:14]:
> > On Thu, Jan 19, 2017 at 07:04:57AM -0800, Tony Lindgren wrote:
> > > * Bin Liu <b-liu-l0cyMroinI0@public.gmane.org> [170118 19:42]:
> > > > On Wed, Jan 18, 2017 at 06:29:59PM -0800, Tony Lindgren wrote:
> > > > > 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?
> > > > 
> > > > > 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.
> > > > 
> > > > 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.
> 
> 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.

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: [PATCH 2/2] usb: musb: Size 1 dma in transfers won't complete with cpp41

[Bin Liu]

On Thu, Jan 19, 2017 at 08:15:45AM -0800, Tony Lindgren wrote:
> * Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> [170119 07:57]:
> > * Bin Liu <b-liu-l0cyMroinI0@public.gmane.org> [170119 07:14]:
> > > On Thu, Jan 19, 2017 at 07:04:57AM -0800, Tony Lindgren wrote:
> > > > * Bin Liu <b-liu-l0cyMroinI0@public.gmane.org> [170118 19:42]:
> > > > > On Wed, Jan 18, 2017 at 06:29:59PM -0800, Tony Lindgren wrote:
> > > > > > 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?
> > > > > 
> > > > > > 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.
> > > > > 
> > > > > 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.
> > 
> > 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.

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

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: [PATCH 2/2] usb: musb: Size 1 dma in transfers won't complete with cpp41

[Tony Lindgren]

* Bin Liu <b-liu-l0cyMroinI0@public.gmane.org> [170119 09:16]:
> On Thu, Jan 19, 2017 at 08:15:45AM -0800, Tony Lindgren wrote:
> > * Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> [170119 07:57]:
> > > * Bin Liu <b-liu-l0cyMroinI0@public.gmane.org> [170119 07:14]:
> > > > On Thu, Jan 19, 2017 at 07:04:57AM -0800, Tony Lindgren wrote:
> > > > > * Bin Liu <b-liu-l0cyMroinI0@public.gmane.org> [170118 19:42]:
> > > > > > On Wed, Jan 18, 2017 at 06:29:59PM -0800, Tony Lindgren wrote:
> > > > > > > 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?
> > > > > > 
> > > > > > > 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.
> > > > > > 
> > > > > > 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.
> > > 
> > > 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.
> 
> Sounds good. I wil do more check on 1/2 then send it to Greg.

OK thanks. I just also posted the two cppi41 dma regression fixes, the
first patch in that series makes patch 2/2 of this series obsolete.

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