usb: musb: fix idling for suspend after disconnect interrupt
diff mbox series

Message ID 20191126034151.38154-1-tony@atomide.com
State New
Headers show
Series
  • usb: musb: fix idling for suspend after disconnect interrupt
Related show

Commit Message

Tony Lindgren Nov. 26, 2019, 3:41 a.m. UTC
When disconnected as USB B-device, we sometimes get a suspend interrupt
after disconnect interrupt. In that case we have devctl set to 99 with
VBUS still valid and musb_pm_runtime_check_session() wrongly things we
have an active session. We have no other interrupts after disconnect
coming in this case at least with the omap2430 glue.

Let's fix the issue by checking the interrupt status again with
delayed work for the devctl 99 case. In the suspend after disconnect
case the devctl session bit has cleared by then and musb can idle.
For a typical USB B-device connect case we just continue with normal
interrupts.

Cc: Merlijn Wajer <merlijn@wizzup.org>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Sebastian Reichel <sre@kernel.org>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/usb/musb/musb_core.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Sergei Shtylyov Nov. 26, 2019, 10:20 a.m. UTC | #1
Hello!

On 26.11.2019 6:41, Tony Lindgren wrote:

> When disconnected as USB B-device, we sometimes get a suspend interrupt
> after disconnect interrupt. In that case we have devctl set to 99 with
> VBUS still valid and musb_pm_runtime_check_session() wrongly things we

     Thinks?

> have an active session. We have no other interrupts after disconnect
> coming in this case at least with the omap2430 glue.
> 
> Let's fix the issue by checking the interrupt status again with
> delayed work for the devctl 99 case. In the suspend after disconnect
> case the devctl session bit has cleared by then and musb can idle.
> For a typical USB B-device connect case we just continue with normal
> interrupts.
> 
> Cc: Merlijn Wajer <merlijn@wizzup.org>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Sebastian Reichel <sre@kernel.org>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
[...]

MBR, Sergei
Tony Lindgren Nov. 26, 2019, 5:45 p.m. UTC | #2
* Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> [191126 10:21]:
> Hello!
> 
> On 26.11.2019 6:41, Tony Lindgren wrote:
> 
> > When disconnected as USB B-device, we sometimes get a suspend interrupt
> > after disconnect interrupt. In that case we have devctl set to 99 with
> > VBUS still valid and musb_pm_runtime_check_session() wrongly things we
> 
>     Thinks?

Thanks will fix.

Tony
Bin Liu Dec. 12, 2019, 4 p.m. UTC | #3
Hi Tony,

Sorry for my late response.

On Mon, Nov 25, 2019 at 07:41:51PM -0800, Tony Lindgren wrote:
> When disconnected as USB B-device, we sometimes get a suspend interrupt
> after disconnect interrupt. In that case we have devctl set to 99 with
> VBUS still valid and musb_pm_runtime_check_session() wrongly things we
> have an active session. We have no other interrupts after disconnect
> coming in this case at least with the omap2430 glue.

I don't have an omap2430 platform to test, but its musb doesn't generate
DISCONNECT interrupt at all when disconnected from USB host in any case?
It is a surprise, the musb core driver expects the DISCONNECT interrupt
after VBUS is lost and relies on it to tear down the gadget driver and
the state machine. I am wondering how its USB is functional without the
DISCONNECT event...

-Bin.

> 
> Let's fix the issue by checking the interrupt status again with
> delayed work for the devctl 99 case. In the suspend after disconnect
> case the devctl session bit has cleared by then and musb can idle.
> For a typical USB B-device connect case we just continue with normal
> interrupts.
> 
> Cc: Merlijn Wajer <merlijn@wizzup.org>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Sebastian Reichel <sre@kernel.org>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/usb/musb/musb_core.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> 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
> @@ -1943,6 +1943,9 @@ ATTRIBUTE_GROUPS(musb);
>  #define MUSB_QUIRK_B_INVALID_VBUS_91	(MUSB_DEVCTL_BDEVICE | \
>  					 (2 << MUSB_DEVCTL_VBUS_SHIFT) | \
>  					 MUSB_DEVCTL_SESSION)
> +#define MUSB_QUIRK_B_DISCONNECT_99	(MUSB_DEVCTL_BDEVICE | \
> +					 (3 << MUSB_DEVCTL_VBUS_SHIFT) | \
> +					 MUSB_DEVCTL_SESSION)
>  #define MUSB_QUIRK_A_DISCONNECT_19	((3 << MUSB_DEVCTL_VBUS_SHIFT) | \
>  					 MUSB_DEVCTL_SESSION)
>  
> @@ -1965,6 +1968,11 @@ static void musb_pm_runtime_check_session(struct musb *musb)
>  	s = MUSB_DEVCTL_FSDEV | MUSB_DEVCTL_LSDEV |
>  		MUSB_DEVCTL_HR;
>  	switch (devctl & ~s) {
> +	case MUSB_QUIRK_B_DISCONNECT_99:
> +		musb_dbg(musb, "Poll devctl in case of suspend after disconnect\n");
> +		schedule_delayed_work(&musb->irq_work,
> +				      msecs_to_jiffies(1000));
> +		break;
>  	case MUSB_QUIRK_B_INVALID_VBUS_91:
>  		if (musb->quirk_retries && !musb->flush_irq_work) {
>  			musb_dbg(musb,
> -- 
> 2.24.0
Tony Lindgren Dec. 12, 2019, 4:09 p.m. UTC | #4
Hi,

* Bin Liu <b-liu@ti.com> [191212 16:02]:
> Hi Tony,
> 
> Sorry for my late response.

No worries.

> On Mon, Nov 25, 2019 at 07:41:51PM -0800, Tony Lindgren wrote:
> > When disconnected as USB B-device, we sometimes get a suspend interrupt
> > after disconnect interrupt. In that case we have devctl set to 99 with
> > VBUS still valid and musb_pm_runtime_check_session() wrongly things we
> > have an active session. We have no other interrupts after disconnect
> > coming in this case at least with the omap2430 glue.
> 
> I don't have an omap2430 platform to test, but its musb doesn't generate
> DISCONNECT interrupt at all when disconnected from USB host in any case?
> It is a surprise, the musb core driver expects the DISCONNECT interrupt
> after VBUS is lost and relies on it to tear down the gadget driver and
> the state machine. I am wondering how its USB is functional without the
> DISCONNECT event...

We do get DISCONNECT, but we can then get a SUSPEND after DISCONNECT
has already happened..

That will wake up musb waiting for further interrupts thinking it's
connected. But after that there are no more interrupts as the cable
is disconnected so we need to poll the status again.

If we see SUSPEND before DISCONNECT, then things idle fine.

Regards,

Tony
Bin Liu Dec. 12, 2019, 5:08 p.m. UTC | #5
On Thu, Dec 12, 2019 at 08:09:46AM -0800, Tony Lindgren wrote:
> Hi,
> 
> * Bin Liu <b-liu@ti.com> [191212 16:02]:
> > Hi Tony,
> > 
> > Sorry for my late response.
> 
> No worries.
> 
> > On Mon, Nov 25, 2019 at 07:41:51PM -0800, Tony Lindgren wrote:
> > > When disconnected as USB B-device, we sometimes get a suspend interrupt
> > > after disconnect interrupt. In that case we have devctl set to 99 with
> > > VBUS still valid and musb_pm_runtime_check_session() wrongly things we
> > > have an active session. We have no other interrupts after disconnect
> > > coming in this case at least with the omap2430 glue.
> > 
> > I don't have an omap2430 platform to test, but its musb doesn't generate
> > DISCONNECT interrupt at all when disconnected from USB host in any case?
> > It is a surprise, the musb core driver expects the DISCONNECT interrupt
> > after VBUS is lost and relies on it to tear down the gadget driver and
> > the state machine. I am wondering how its USB is functional without the
> > DISCONNECT event...
> 
> We do get DISCONNECT, but we can then get a SUSPEND after DISCONNECT
> has already happened..

Ahh, I missed the first sentence in the commit log...
Now I understand the issue. Thanks.

> 
> That will wake up musb waiting for further interrupts thinking it's
> connected. But after that there are no more interrupts as the cable
> is disconnected so we need to poll the status again.
> 
> If we see SUSPEND before DISCONNECT, then things idle fine.

Does SUSPEND always comes after DISCONNECT on omap2430, or just
sometimes? I guess the USB connector has some issue - when DP/DM pins
are disconnected, SUSPEND interrupt is generated; when VBUS/GND pins are
disconnected, DISCONNECT interrupt is generated. Because DP/DM pins are
shorter than VBUS/GND, SUSPEND should come before DISCONNECT.

-Bin.
Tony Lindgren Dec. 12, 2019, 5:15 p.m. UTC | #6
* Bin Liu <b-liu@ti.com> [191212 17:09]:
> On Thu, Dec 12, 2019 at 08:09:46AM -0800, Tony Lindgren wrote:
> > That will wake up musb waiting for further interrupts thinking it's
> > connected. But after that there are no more interrupts as the cable
> > is disconnected so we need to poll the status again.
> > 
> > If we see SUSPEND before DISCONNECT, then things idle fine.
> 
> Does SUSPEND always comes after DISCONNECT on omap2430, or just
> sometimes? I guess the USB connector has some issue - when DP/DM pins
> are disconnected, SUSPEND interrupt is generated; when VBUS/GND pins are
> disconnected, DISCONNECT interrupt is generated. Because DP/DM pins are
> shorter than VBUS/GND, SUSPEND should come before DISCONNECT.

No this does not always happen, but happens annoyingly often for
me to notice. Your explanation sounds good though, and also sounds
that can be affected by the wear on a micro-USB connector.

Regards,

Tony
Bin Liu Dec. 12, 2019, 5:21 p.m. UTC | #7
On Tue, Nov 26, 2019 at 09:45:15AM -0800, Tony Lindgren wrote:
> * Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> [191126 10:21]:
> > Hello!
> > 
> > On 26.11.2019 6:41, Tony Lindgren wrote:
> > 
> > > When disconnected as USB B-device, we sometimes get a suspend interrupt
> > > after disconnect interrupt. In that case we have devctl set to 99 with
> > > VBUS still valid and musb_pm_runtime_check_session() wrongly things we
> > 
> >     Thinks?
> 
> Thanks will fix.

Is this the only update needed? I can fix it locally when queuing the
patch.

-Bin.
Tony Lindgren Dec. 12, 2019, 5:26 p.m. UTC | #8
* Bin Liu <b-liu@ti.com> [191212 17:23]:
> On Tue, Nov 26, 2019 at 09:45:15AM -0800, Tony Lindgren wrote:
> > * Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> [191126 10:21]:
> > > Hello!
> > > 
> > > On 26.11.2019 6:41, Tony Lindgren wrote:
> > > 
> > > > When disconnected as USB B-device, we sometimes get a suspend interrupt
> > > > after disconnect interrupt. In that case we have devctl set to 99 with
> > > > VBUS still valid and musb_pm_runtime_check_session() wrongly things we
> > > 
> > >     Thinks?
> > 
> > Thanks will fix.
> 
> Is this the only update needed? I can fix it locally when queuing the
> patch.

So it seems. Maybe also add your explanation for why the wrong
ordering of interrupts can happen while at it.

Regards,

Tony

Patch
diff mbox series

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
@@ -1943,6 +1943,9 @@  ATTRIBUTE_GROUPS(musb);
 #define MUSB_QUIRK_B_INVALID_VBUS_91	(MUSB_DEVCTL_BDEVICE | \
 					 (2 << MUSB_DEVCTL_VBUS_SHIFT) | \
 					 MUSB_DEVCTL_SESSION)
+#define MUSB_QUIRK_B_DISCONNECT_99	(MUSB_DEVCTL_BDEVICE | \
+					 (3 << MUSB_DEVCTL_VBUS_SHIFT) | \
+					 MUSB_DEVCTL_SESSION)
 #define MUSB_QUIRK_A_DISCONNECT_19	((3 << MUSB_DEVCTL_VBUS_SHIFT) | \
 					 MUSB_DEVCTL_SESSION)
 
@@ -1965,6 +1968,11 @@  static void musb_pm_runtime_check_session(struct musb *musb)
 	s = MUSB_DEVCTL_FSDEV | MUSB_DEVCTL_LSDEV |
 		MUSB_DEVCTL_HR;
 	switch (devctl & ~s) {
+	case MUSB_QUIRK_B_DISCONNECT_99:
+		musb_dbg(musb, "Poll devctl in case of suspend after disconnect\n");
+		schedule_delayed_work(&musb->irq_work,
+				      msecs_to_jiffies(1000));
+		break;
 	case MUSB_QUIRK_B_INVALID_VBUS_91:
 		if (musb->quirk_retries && !musb->flush_irq_work) {
 			musb_dbg(musb,