| Message ID | 20230505014902.27313-1-quic_linyyuan@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v6] usb: dwc3: fix gadget mode suspend interrupt handler issue | expand |
On Mon, May 08, 2023, Linyu Yuan wrote: > > On 5/6/2023 8:32 AM, Thinh Nguyen wrote: > > On Fri, May 05, 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> > > > --- > > > 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!dIQ2VHccmZTXp1-59hTFfEhc65W0gatf1n2wmBfs5Yb0ipjHksD_ESQSmgXky1o1sc4wEZ5qp9JKV7IKmk5Km4bJzi2pGg$ ) > > > 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!dIQ2VHccmZTXp1-59hTFfEhc65W0gatf1n2wmBfs5Yb0ipjHksD_ESQSmgXky1o1sc4wEZ5qp9JKV7IKmk5Km4ZXmsKSvQ$ ) > > > 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!dIQ2VHccmZTXp1-59hTFfEhc65W0gatf1n2wmBfs5Yb0ipjHksD_ESQSmgXky1o1sc4wEZ5qp9JKV7IKmk5Km4aqWNUh-Q$ ) > > > 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!dIQ2VHccmZTXp1-59hTFfEhc65W0gatf1n2wmBfs5Yb0ipjHksD_ESQSmgXky1o1sc4wEZ5qp9JKV7IKmk5Km4bRmn0HNA$ ) > > > 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!dIQ2VHccmZTXp1-59hTFfEhc65W0gatf1n2wmBfs5Yb0ipjHksD_ESQSmgXky1o1sc4wEZ5qp9JKV7IKmk5Km4ZBn_TPEw$ > > > > > > drivers/usb/dwc3/core.h | 2 ++ > > > drivers/usb/dwc3/gadget.c | 7 ++++++- > > > 2 files changed, 8 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > > > index d56457c02996..efacaacbbeb2 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 if suspend irq happen to avoid possible consective suspend. > > Can we rephrase to: "set to track suspend event due to U3/L2" > > > thanks, sure. > > > > > > > * @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..2c750534b405 100644 > > > --- a/drivers/usb/dwc3/gadget.c > > > +++ b/drivers/usb/dwc3/gadget.c > > > @@ -4303,8 +4303,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; > > > } > > > @@ -4312,6 +4314,9 @@ static void dwc3_gadget_suspend_interrupt(struct dwc3 *dwc, > > > static void dwc3_gadget_interrupt(struct dwc3 *dwc, > > > const struct dwc3_event_devt *event) > > > { > > > + if (event->type != DWC3_DEVICE_EVENT_SUSPEND) > > Minor nit: spacing between if and (. > > > thanks for point it out. > > > > > > While this may work because the next suspend event won't come until a > > resume/reset/disconnect event occur, let's clear this only on reset, > > resume, and disconnect events. > > > seem only clear on these three events is not correct. > > how about suspend -> wakeup -> suspend interrupt sequences ? > > this is mentioned in V1. > > I meant "Wakeup" event, there's no resume event. When the host sends a resume signal and the device is able to process it, then the driver gets the Wakeup event. Thanks, Thinh
On Mon, May 08, 2023, Thinh Nguyen wrote: > On Mon, May 08, 2023, Linyu Yuan wrote: > > > > On 5/6/2023 8:32 AM, Thinh Nguyen wrote: > > > On Fri, May 05, 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> > > > > --- > > > > 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!dIQ2VHccmZTXp1-59hTFfEhc65W0gatf1n2wmBfs5Yb0ipjHksD_ESQSmgXky1o1sc4wEZ5qp9JKV7IKmk5Km4bJzi2pGg$ ) > > > > 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!dIQ2VHccmZTXp1-59hTFfEhc65W0gatf1n2wmBfs5Yb0ipjHksD_ESQSmgXky1o1sc4wEZ5qp9JKV7IKmk5Km4ZXmsKSvQ$ ) > > > > 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!dIQ2VHccmZTXp1-59hTFfEhc65W0gatf1n2wmBfs5Yb0ipjHksD_ESQSmgXky1o1sc4wEZ5qp9JKV7IKmk5Km4aqWNUh-Q$ ) > > > > 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!dIQ2VHccmZTXp1-59hTFfEhc65W0gatf1n2wmBfs5Yb0ipjHksD_ESQSmgXky1o1sc4wEZ5qp9JKV7IKmk5Km4bRmn0HNA$ ) > > > > 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!dIQ2VHccmZTXp1-59hTFfEhc65W0gatf1n2wmBfs5Yb0ipjHksD_ESQSmgXky1o1sc4wEZ5qp9JKV7IKmk5Km4ZBn_TPEw$ > > > > > > > > drivers/usb/dwc3/core.h | 2 ++ > > > > drivers/usb/dwc3/gadget.c | 7 ++++++- > > > > 2 files changed, 8 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > > > > index d56457c02996..efacaacbbeb2 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 if suspend irq happen to avoid possible consective suspend. > > > Can we rephrase to: "set to track suspend event due to U3/L2" > > > > > > thanks, sure. > > > > > > > > > > > * @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..2c750534b405 100644 > > > > --- a/drivers/usb/dwc3/gadget.c > > > > +++ b/drivers/usb/dwc3/gadget.c > > > > @@ -4303,8 +4303,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; > > > > } > > > > @@ -4312,6 +4314,9 @@ static void dwc3_gadget_suspend_interrupt(struct dwc3 *dwc, > > > > static void dwc3_gadget_interrupt(struct dwc3 *dwc, > > > > const struct dwc3_event_devt *event) > > > > { > > > > + if (event->type != DWC3_DEVICE_EVENT_SUSPEND) > > > Minor nit: spacing between if and (. > > > > > > thanks for point it out. > > > > > > > > > > While this may work because the next suspend event won't come until a > > > resume/reset/disconnect event occur, let's clear this only on reset, > > > resume, and disconnect events. > > > > > > seem only clear on these three events is not correct. > > > > how about suspend -> wakeup -> suspend interrupt sequences ? > > > > this is mentioned in V1. > > > > > > I meant "Wakeup" event, there's no resume event. When the host sends a > resume signal and the device is able to process it, then the driver gets > the Wakeup event. > Actually, we do call it "resume/remote wakeup event", but we just named it "wakeup" interrupt here in dwc3. Another thing, can you clear the suspended flag when remote wakeup is successful also? Thanks, Thinh
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index d56457c02996..efacaacbbeb2 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 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. @@ -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..2c750534b405 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -4303,8 +4303,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; } @@ -4312,6 +4314,9 @@ static void dwc3_gadget_suspend_interrupt(struct dwc3 *dwc, static void dwc3_gadget_interrupt(struct dwc3 *dwc, const struct dwc3_event_devt *event) { + if (event->type != DWC3_DEVICE_EVENT_SUSPEND) + dwc->suspended = false; + switch (event->type) { case DWC3_DEVICE_EVENT_DISCONNECT: dwc3_gadget_disconnect_interrupt(dwc);
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> --- 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. 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 | 2 ++ drivers/usb/dwc3/gadget.c | 7 ++++++- 2 files changed, 8 insertions(+), 1 deletion(-)