[PATCH v2] usb: dwc3: update link state on each device level interrupt

[PATCH v2] usb: dwc3: update link state on each device level interrupt

[Linyu Yuan]

When work in gadget mode, currently driver doesn't update software level
link_state correctly as link state change event is not enabled for most
devices, in function dwc3_gadget_suspend_interrupt(), it will only pass
suspend event to UDC core when software level link state changes, so when
interrupt generated in sequences of suspend -> reset -> conndone ->
suspend, link state is not updated during reset and conndone, so second
suspend interrupt event will not pass to UDC core.

As in gadget mode, device level interrupt event have link state
information, let's trust it and update software level link state to it
when process each device level interrupt.

Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
---
v2: update according v1 discussion
v1: https://lore.kernel.org/linux-usb/1675221286-23833-1-git-send-email-quic_linyyuan@quicinc.com/

 drivers/usb/dwc3/core.h   |  1 +
 drivers/usb/dwc3/gadget.c | 40 ++++++++++++++++++++++++++++------------
 2 files changed, 29 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index d56457c..8622f9c 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -1332,6 +1332,7 @@ struct dwc3 {
 	unsigned		dis_split_quirk:1;
 	unsigned		async_callbacks:1;
 	unsigned		wakeup_configured:1;
+	unsigned		suspend_irq_happen:1;
 
 	u16			imod_interval;
 
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 9f492c8..239cfc1 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2422,7 +2422,6 @@ static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int intf_id)
 			return -EINVAL;
 		}
 		dwc3_resume_gadget(dwc);
-		dwc->link_state = DWC3_LINK_STATE_U0;
 	}
 
 	ret = dwc3_send_gadget_generic_command(dwc, DWC3_DGCMD_DEV_NOTIFICATION,
@@ -4178,7 +4177,7 @@ static void dwc3_gadget_conndone_interrupt(struct dwc3 *dwc)
 	 */
 }
 
-static void dwc3_gadget_wakeup_interrupt(struct dwc3 *dwc, unsigned int evtinfo)
+static void dwc3_gadget_wakeup_interrupt(struct dwc3 *dwc)
 {
 	/*
 	 * TODO take core out of low power mode when that's
@@ -4190,8 +4189,6 @@ static void dwc3_gadget_wakeup_interrupt(struct dwc3 *dwc, unsigned int evtinfo)
 		dwc->gadget_driver->resume(dwc->gadget);
 		spin_lock(&dwc->lock);
 	}
-
-	dwc->link_state = evtinfo & DWC3_LINK_STATE_MASK;
 }
 
 static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc,
@@ -4298,20 +4295,39 @@ static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc,
 	dwc->link_state = next;
 }
 
-static void dwc3_gadget_suspend_interrupt(struct dwc3 *dwc,
-					  unsigned int evtinfo)
+static void dwc3_gadget_suspend_interrupt(struct dwc3 *dwc)
 {
-	enum dwc3_link_state next = evtinfo & DWC3_LINK_STATE_MASK;
-
-	if (dwc->link_state != next && next == DWC3_LINK_STATE_U3)
+	if (!dwc->suspend_irq_happen && dwc->link_state == DWC3_LINK_STATE_U3) {
+		dwc->suspend_irq_happen = true;
 		dwc3_suspend_gadget(dwc);
+	}
+}
 
-	dwc->link_state = next;
+static inline void dwc3_gadget_interrupt_early(struct dwc3 *dwc,
+			const struct dwc3_event_devt *event)
+{
+	switch (event->type) {
+	case DWC3_DEVICE_EVENT_LINK_STATUS_CHANGE:
+		break;
+	default:
+		dwc->link_state = event->event_info & DWC3_LINK_STATE_MASK;
+		break;
+	}
+
+	switch (event->type) {
+	case DWC3_DEVICE_EVENT_SUSPEND:
+		break;
+	default:
+		dwc->suspend_irq_happen = false;
+		break;
+	}
 }
 
 static void dwc3_gadget_interrupt(struct dwc3 *dwc,
 		const struct dwc3_event_devt *event)
 {
+	dwc3_gadget_interrupt_early(dwc, event);
+
 	switch (event->type) {
 	case DWC3_DEVICE_EVENT_DISCONNECT:
 		dwc3_gadget_disconnect_interrupt(dwc);
@@ -4323,7 +4339,7 @@ static void dwc3_gadget_interrupt(struct dwc3 *dwc,
 		dwc3_gadget_conndone_interrupt(dwc);
 		break;
 	case DWC3_DEVICE_EVENT_WAKEUP:
-		dwc3_gadget_wakeup_interrupt(dwc, event->event_info);
+		dwc3_gadget_wakeup_interrupt(dwc);
 		break;
 	case DWC3_DEVICE_EVENT_HIBER_REQ:
 		dev_WARN_ONCE(dwc->dev, true, "unexpected hibernation event\n");
@@ -4334,7 +4350,7 @@ static void dwc3_gadget_interrupt(struct dwc3 *dwc,
 	case DWC3_DEVICE_EVENT_SUSPEND:
 		/* It changed to be suspend event for version 2.30a and above */
 		if (!DWC3_VER_IS_PRIOR(DWC3, 230A))
-			dwc3_gadget_suspend_interrupt(dwc, event->event_info);
+			dwc3_gadget_suspend_interrupt(dwc);
 		break;
 	case DWC3_DEVICE_EVENT_SOF:
 	case DWC3_DEVICE_EVENT_ERRATIC_ERROR:
-- 
2.7.4

Re: [PATCH v2] usb: dwc3: update link state on each device level interrupt

[Greg Kroah-Hartman]

On Fri, Apr 21, 2023 at 10:01:12AM +0800, Linyu Yuan wrote:
> When work in gadget mode, currently driver doesn't update software level
> link_state correctly as link state change event is not enabled for most
> devices, in function dwc3_gadget_suspend_interrupt(), it will only pass
> suspend event to UDC core when software level link state changes, so when
> interrupt generated in sequences of suspend -> reset -> conndone ->
> suspend, link state is not updated during reset and conndone, so second
> suspend interrupt event will not pass to UDC core.
> 
> As in gadget mode, device level interrupt event have link state
> information, let's trust it and update software level link state to it
> when process each device level interrupt.
> 
> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
> ---
> v2: update according v1 discussion

That does not describe what changed at all, please say what changed,
otherwise we have to guess, and I'm going to guess "nothing" :)

Please fix up and send a v3.

thanks,

greg k-h