| Message ID | 1682476780-2367-1-git-send-email-quic_linyyuan@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v5] usb: dwc3: update link state on each device level interrupt | expand |
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@quicinc.com> > --- > > v5: (refer v4 https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/1682393256-15572-1-git-send-email-quic_linyyuan@quicinc.com/__;!!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@quicinc.com/__;!!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@quicinc.com/__;!!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@quicinc.com/__;!!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 >
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; } 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); + 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:
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> --- v5: (refer v4 https://lore.kernel.org/linux-usb/1682393256-15572-1-git-send-email-quic_linyyuan@quicinc.com/) 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://lore.kernel.org/linux-usb/1682053861-21737-1-git-send-email-quic_linyyuan@quicinc.com/) 1) remove link state checking in dwc3_gadget_wakeup_interrupt() 2) remove two switch/case to if opeartion v3: (refer v2 https://lore.kernel.org/linux-usb/1682042472-21222-1-git-send-email-quic_linyyuan@quicinc.com/) 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://lore.kernel.org/linux-usb/1675221286-23833-1-git-send-email-quic_linyyuan@quicinc.com/ drivers/usb/dwc3/core.h | 4 ++++ drivers/usb/dwc3/gadget.c | 43 +++++++++++++++++++++++++------------------ 2 files changed, 29 insertions(+), 18 deletions(-)