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@xxxxxxxxxxx> > --- > v6: (refer v5 https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/1682476780-2367-1-git-send-email-quic_linyyuan@xxxxxxxxxxx/__;!!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@xxxxxxxxxxx/__;!!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@xxxxxxxxxxx/__;!!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@xxxxxxxxxxx/__;!!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@xxxxxxxxxxx/__;!!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" > * @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 (. 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. > + dwc->suspended = false; > + > switch (event->type) { > case DWC3_DEVICE_EVENT_DISCONNECT: > dwc3_gadget_disconnect_interrupt(dwc); > -- > 2.17.1 > Thanks, Thinh
