On Wed, Apr 26, 2023, 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@xxxxxxxxxxx> > --- > > v5: (refer v4 https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/1682393256-15572-1-git-send-email-quic_linyyuan@xxxxxxxxxxx/__;!!A4F2R9G_pg!YpoUkW2jmCBMlIh0yFvGiN_b0ibfdR4Oq616mVXpTMEXjgm5LeZnbYgxbr9SQJ2d31IK4CWntYGFg6xTX7IvHGXQu-RCGQ$ ) > 1) rename suspend_irq_happen to suspended and document it > 2) add old_link_state for link change interrupt usage and change accordingly > > v4: (refer v3 https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/1682053861-21737-1-git-send-email-quic_linyyuan@xxxxxxxxxxx/__;!!A4F2R9G_pg!YpoUkW2jmCBMlIh0yFvGiN_b0ibfdR4Oq616mVXpTMEXjgm5LeZnbYgxbr9SQJ2d31IK4CWntYGFg6xTX7IvHGXjJP986w$ ) > 1) remove link state checking in dwc3_gadget_wakeup_interrupt() > 2) remove two switch/case to if opeartion > > v3: (refer v2 https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/1682042472-21222-1-git-send-email-quic_linyyuan@xxxxxxxxxxx/__;!!A4F2R9G_pg!YpoUkW2jmCBMlIh0yFvGiN_b0ibfdR4Oq616mVXpTMEXjgm5LeZnbYgxbr9SQJ2d31IK4CWntYGFg6xTX7IvHGW6fTctBg$ ) > no code change since v2, changes compare v1 as below, > 1) add a flag suspend_irq_happen to simplify dwc3_gadget_suspend_interrupt(), > it will avoid refer to software level link_state, finally link_state will > only used in dwc3_gadget_linksts_change_interrupt(). > 2) remove sw setting of link_state in dwc3_gadget_func_wakeup() > 3) add dwc3_gadget_interrupt_early() to correct setting of link_state > and suspend_irq_happen. > > v2: update according v1 discussion > v1: https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/1675221286-23833-1-git-send-email-quic_linyyuan@xxxxxxxxxxx/__;!!A4F2R9G_pg!YpoUkW2jmCBMlIh0yFvGiN_b0ibfdR4Oq616mVXpTMEXjgm5LeZnbYgxbr9SQJ2d31IK4CWntYGFg6xTX7IvHGU8lh84sA$ > > drivers/usb/dwc3/core.h | 4 ++++ > drivers/usb/dwc3/gadget.c | 43 +++++++++++++++++++++++++------------------ > 2 files changed, 29 insertions(+), 18 deletions(-) > > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > index d56457c..140866a 100644 > --- a/drivers/usb/dwc3/core.h > +++ b/drivers/usb/dwc3/core.h > @@ -1038,6 +1038,7 @@ struct dwc3_scratchpad_array { > * @ep0_next_event: hold the next expected event > * @ep0state: state of endpoint zero > * @link_state: link state > + * @old_link_state: cache link state for link change interrupt usage > * @speed: device speed (super, high, full, low) > * @hwparams: copy of hwparams registers > * @regset: debugfs pointer to regdump file > @@ -1116,6 +1117,7 @@ struct dwc3_scratchpad_array { > * @dis_metastability_quirk: set to disable metastability quirk. > * @dis_split_quirk: set to disable split boundary. > * @wakeup_configured: set if the device is configured for remote wakeup. > + * @suspended: set if suspend irq happen to avoid possible consective suspend. > * @imod_interval: set the interrupt moderation interval in 250ns > * increments or 0 to disable. > * @max_cfg_eps: current max number of IN eps used across all USB configs. > @@ -1254,6 +1256,7 @@ struct dwc3 { > enum dwc3_ep0_next ep0_next_event; > enum dwc3_ep0_state ep0state; > enum dwc3_link_state link_state; > + enum dwc3_link_state old_link_state; > > u16 u2sel; > u16 u2pel; > @@ -1332,6 +1335,7 @@ struct dwc3 { > unsigned dis_split_quirk:1; > unsigned async_callbacks:1; > unsigned wakeup_configured:1; > + unsigned suspended:1; > > u16 imod_interval; > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index c0ca4d1..a1aba3a 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -2440,7 +2440,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; The dwc3_gadget_func_wakeup() shouldn't update the dwc->link_state in the beginning. I missed this when reviewing for that. > } > > 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,14 +4189,11 @@ 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, > - unsigned int evtinfo) > + enum dwc3_link_state next) > { > - enum dwc3_link_state next = evtinfo & DWC3_LINK_STATE_MASK; > unsigned int pwropt; > > /* > @@ -4220,7 +4216,7 @@ static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc, > pwropt = DWC3_GHWPARAMS1_EN_PWROPT(dwc->hwparams.hwparams1); > if (DWC3_VER_IS_PRIOR(DWC3, 250A) && > (pwropt != DWC3_GHWPARAMS1_EN_PWROPT_HIB)) { > - if ((dwc->link_state == DWC3_LINK_STATE_U3) && > + if ((dwc->old_link_state == DWC3_LINK_STATE_U3) && > (next == DWC3_LINK_STATE_RESUME)) { > return; > } > @@ -4249,7 +4245,7 @@ static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc, > u32 u1u2; > u32 reg; > > - switch (dwc->link_state) { > + switch (dwc->old_link_state) { > case DWC3_LINK_STATE_U1: > case DWC3_LINK_STATE_U2: > reg = dwc3_readl(dwc->regs, DWC3_DCTL); > @@ -4295,23 +4291,34 @@ static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc, > break; > } > > - dwc->link_state = next; > + dwc->old_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->suspended) { > + dwc->suspended = 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) > +{ > + dwc->link_state = event->event_info & DWC3_LINK_STATE_MASK; > + > + if (event->type != DWC3_DEVICE_EVENT_LINK_STATUS_CHANGE) > + dwc->old_link_state = dwc->link_state; > + > + if (event->type != DWC3_DEVICE_EVENT_SUSPEND) > + dwc->suspended = false; > } > > static void dwc3_gadget_interrupt(struct dwc3 *dwc, > const struct dwc3_event_devt *event) > { > + dwc3_gadget_interrupt_early(dwc, event); > + What's dwc->link_state current use case here? We're not using it for your fix right? Why do we change how it's updated now? Thanks, Thinh > switch (event->type) { > case DWC3_DEVICE_EVENT_DISCONNECT: > dwc3_gadget_disconnect_interrupt(dwc); > @@ -4323,18 +4330,18 @@ 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"); > break; > case DWC3_DEVICE_EVENT_LINK_STATUS_CHANGE: > - dwc3_gadget_linksts_change_interrupt(dwc, event->event_info); > + dwc3_gadget_linksts_change_interrupt(dwc, dwc->link_state); > break; > 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 >
