| Message ID | 20230510014718.13872-1-quic_linyyuan@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v8] usb: dwc3: fix gadget mode suspend interrupt handler issue | expand |
On Wed, May 10, 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. > > Remove link_state compare in dwc3_gadget_suspend_interrupt() and add a > suspended flag to replace the compare function. > > Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> Can you add a Fixes tag? Here's the ack after that's added: Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> Thanks! Thinh > --- > > v8: (refer v7 https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/20230509050743.5781-1-quic_linyyuan@quicinc.com/__;!!A4F2R9G_pg!cqiMrgjxUEhR1uUAwO-h_BXCRaT5KGgDgwKOLRgY6dA7g1CC9HPc9dqTVCkZ3nuQ4Ap_wdgd7zZp9BdqjvIK5BIm3_vF0g$ ) > 1) move some suspended flag clear to specific event handler > > v7: (refer v6 https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/20230505014902.27313-1-quic_linyyuan@quicinc.com/__;!!A4F2R9G_pg!cqiMrgjxUEhR1uUAwO-h_BXCRaT5KGgDgwKOLRgY6dA7g1CC9HPc9dqTVCkZ3nuQ4Ap_wdgd7zZp9BdqjvIK5BL4B9xruw$ ) > 1) reword suspended flag comment > 2) remove one extra space in if operation > 4) clear suspended flag for wakeup/reset/disconnect interrupt > 3) clear suspended flag for remote wakeup related case. > > v6: (refer v5 https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/1682476780-2367-1-git-send-email-quic_linyyuan@quicinc.com/__;!!A4F2R9G_pg!cqiMrgjxUEhR1uUAwO-h_BXCRaT5KGgDgwKOLRgY6dA7g1CC9HPc9dqTVCkZ3nuQ4Ap_wdgd7zZp9BdqjvIK5BINLsp2Lw$ ) > 1) change subject > 2) only keep suspended flag related change > > 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!cqiMrgjxUEhR1uUAwO-h_BXCRaT5KGgDgwKOLRgY6dA7g1CC9HPc9dqTVCkZ3nuQ4Ap_wdgd7zZp9BdqjvIK5BK9PBiyoQ$ ) > 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!cqiMrgjxUEhR1uUAwO-h_BXCRaT5KGgDgwKOLRgY6dA7g1CC9HPc9dqTVCkZ3nuQ4Ap_wdgd7zZp9BdqjvIK5BJLhh4Jgg$ ) > 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!cqiMrgjxUEhR1uUAwO-h_BXCRaT5KGgDgwKOLRgY6dA7g1CC9HPc9dqTVCkZ3nuQ4Ap_wdgd7zZp9BdqjvIK5BJT63ZMiA$ ) > 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. > > drivers/usb/dwc3/core.h | 2 ++ > drivers/usb/dwc3/gadget.c | 12 +++++++++++- > 2 files changed, 13 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > index d56457c02996..1f043c31a096 100644 > --- a/drivers/usb/dwc3/core.h > +++ b/drivers/usb/dwc3/core.h > @@ -1116,6 +1116,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 to track suspend event due to U3/L2. > * @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. > @@ -1332,6 +1333,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 c0ca4d12f95d..f244bebf1ea0 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -2440,6 +2440,7 @@ static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int intf_id) > return -EINVAL; > } > dwc3_resume_gadget(dwc); > + dwc->suspended = false; > dwc->link_state = DWC3_LINK_STATE_U0; > } > > @@ -3938,6 +3939,8 @@ static void dwc3_gadget_disconnect_interrupt(struct dwc3 *dwc) > { > int reg; > > + dwc->suspended = false; > + > dwc3_gadget_set_link_state(dwc, DWC3_LINK_STATE_RX_DET); > > reg = dwc3_readl(dwc->regs, DWC3_DCTL); > @@ -3962,6 +3965,8 @@ static void dwc3_gadget_reset_interrupt(struct dwc3 *dwc) > { > u32 reg; > > + dwc->suspended = false; > + > /* > * Ideally, dwc3_reset_gadget() would trigger the function > * drivers to stop any active transfers through ep disable. > @@ -4180,6 +4185,8 @@ static void dwc3_gadget_conndone_interrupt(struct dwc3 *dwc) > > static void dwc3_gadget_wakeup_interrupt(struct dwc3 *dwc, unsigned int evtinfo) > { > + dwc->suspended = false; > + > /* > * TODO take core out of low power mode when that's > * implemented. > @@ -4277,6 +4284,7 @@ static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc, > if (dwc->gadget->wakeup_armed) { > dwc3_gadget_enable_linksts_evts(dwc, false); > dwc3_resume_gadget(dwc); > + dwc->suspended = false; > } > break; > case DWC3_LINK_STATE_U1: > @@ -4303,8 +4311,10 @@ 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 && next == DWC3_LINK_STATE_U3) { > + dwc->suspended = true; > dwc3_suspend_gadget(dwc); > + } > > dwc->link_state = next; > } > -- > 2.17.1 >
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index d56457c02996..1f043c31a096 100644 --- a/drivers/usb/dwc3/core.h +++ b/drivers/usb/dwc3/core.h @@ -1116,6 +1116,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 to track suspend event due to U3/L2. * @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. @@ -1332,6 +1333,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 c0ca4d12f95d..f244bebf1ea0 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -2440,6 +2440,7 @@ static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int intf_id) return -EINVAL; } dwc3_resume_gadget(dwc); + dwc->suspended = false; dwc->link_state = DWC3_LINK_STATE_U0; } @@ -3938,6 +3939,8 @@ static void dwc3_gadget_disconnect_interrupt(struct dwc3 *dwc) { int reg; + dwc->suspended = false; + dwc3_gadget_set_link_state(dwc, DWC3_LINK_STATE_RX_DET); reg = dwc3_readl(dwc->regs, DWC3_DCTL); @@ -3962,6 +3965,8 @@ static void dwc3_gadget_reset_interrupt(struct dwc3 *dwc) { u32 reg; + dwc->suspended = false; + /* * Ideally, dwc3_reset_gadget() would trigger the function * drivers to stop any active transfers through ep disable. @@ -4180,6 +4185,8 @@ static void dwc3_gadget_conndone_interrupt(struct dwc3 *dwc) static void dwc3_gadget_wakeup_interrupt(struct dwc3 *dwc, unsigned int evtinfo) { + dwc->suspended = false; + /* * TODO take core out of low power mode when that's * implemented. @@ -4277,6 +4284,7 @@ static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc, if (dwc->gadget->wakeup_armed) { dwc3_gadget_enable_linksts_evts(dwc, false); dwc3_resume_gadget(dwc); + dwc->suspended = false; } break; case DWC3_LINK_STATE_U1: @@ -4303,8 +4311,10 @@ 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 && next == DWC3_LINK_STATE_U3) { + dwc->suspended = true; dwc3_suspend_gadget(dwc); + } dwc->link_state = next; }
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. Remove link_state compare in dwc3_gadget_suspend_interrupt() and add a suspended flag to replace the compare function. Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> --- v8: (refer v7 https://lore.kernel.org/linux-usb/20230509050743.5781-1-quic_linyyuan@quicinc.com/) 1) move some suspended flag clear to specific event handler v7: (refer v6 https://lore.kernel.org/linux-usb/20230505014902.27313-1-quic_linyyuan@quicinc.com/) 1) reword suspended flag comment 2) remove one extra space in if operation 4) clear suspended flag for wakeup/reset/disconnect interrupt 3) clear suspended flag for remote wakeup related case. v6: (refer v5 https://lore.kernel.org/linux-usb/1682476780-2367-1-git-send-email-quic_linyyuan@quicinc.com/) 1) change subject 2) only keep suspended flag related change 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. drivers/usb/dwc3/core.h | 2 ++ drivers/usb/dwc3/gadget.c | 12 +++++++++++- 2 files changed, 13 insertions(+), 1 deletion(-)