On Tue, Apr 25, 2023, Linyu Yuan wrote: > > On 4/25/2023 5:56 AM, Thinh Nguyen wrote: > > On Fri, Apr 21, 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> > > > --- > > > v3: (refer v2 https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/1682042472-21222-1-git-send-email-quic_linyyuan@xxxxxxxxxxx/__;!!A4F2R9G_pg!ZHas9LWZu81rU6xkgMdpCBejHkR05uTBSNFu7zWFFdgX_n_-RAQ7vuuCVAdI5IN17IhGhriXIP2H3y-Fbhk3VT_QE-hQ9Q$ ) > > > 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!ZHas9LWZu81rU6xkgMdpCBejHkR05uTBSNFu7zWFFdgX_n_-RAQ7vuuCVAdI5IN17IhGhriXIP2H3y-Fbhk3VT95OG0RmA$ > > > > > > drivers/usb/dwc3/core.h | 1 + > > > drivers/usb/dwc3/gadget.c | 40 ++++++++++++++++++++++++++++------------ > > > 2 files changed, 29 insertions(+), 12 deletions(-) > > > > > > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > > > index d56457c..8622f9c 100644 > > > --- a/drivers/usb/dwc3/core.h > > > +++ b/drivers/usb/dwc3/core.h > > > @@ -1332,6 +1332,7 @@ struct dwc3 { > > > unsigned dis_split_quirk:1; > > > unsigned async_callbacks:1; > > > unsigned wakeup_configured:1; > > > + unsigned suspend_irq_happen:1; > > Can we rename this to suspended. Also, please document new struct > > member. > > > i don't know if it is good to name suspended, as there are some power > management > > state like runtime suspend. This is in the context of dwc3 event, it should be fine. > > > > > u16 imod_interval; > > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > > > index 9f492c8..239cfc1 100644 > > > --- a/drivers/usb/dwc3/gadget.c > > > +++ b/drivers/usb/dwc3/gadget.c > > > @@ -2422,7 +2422,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,8 +4189,6 @@ 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, > > > @@ -4298,20 +4295,39 @@ static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc, > > > dwc->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->suspend_irq_happen && dwc->link_state == DWC3_LINK_STATE_U3) { > > Do we still need to check for dwc->link_state if we use > > suspend_irq_happen flag? > > > sure, I will remove it and just trust suspend irq. > > > > > > > + dwc->suspend_irq_happen = 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) > > > +{ > > > + switch (event->type) { > > > + case DWC3_DEVICE_EVENT_LINK_STATUS_CHANGE: > > > + break; > > > + default: > > > + dwc->link_state = event->event_info & DWC3_LINK_STATE_MASK; > > > + break; > > > + } > > > + > > > + switch (event->type) { > > > + case DWC3_DEVICE_EVENT_SUSPEND: > > > + break; > > > + default: > > > + dwc->suspend_irq_happen = false; > > > + break; > > > + } > > Why 2 switches to the same event->type here? This logic is confusing. > > > change to two if / else ? we need to handle alls setting one function, > > i don't want to add two functions here. > My point is that it doesn't make sense to split the switch() with a single case for each to the same event->type. It's difficult to follow that logic. > > > > > > } > > > static void dwc3_gadget_interrupt(struct dwc3 *dwc, > > > const struct dwc3_event_devt *event) > > > { > > > + dwc3_gadget_interrupt_early(dwc, event); > > This is not what we discussed. Previously I suggested to leave the > > dwc->link_state for the quirk (usb_dwc3 revision <2.50a) and not use it > > for other scenarios. The dwc->link_state was used as the _previous_ > > captured state, and not the current captured link_state. Now, you > > changed it to the current link_state of an event. Because this old vs > > new link_state was confusing that I suggested not to use it at all out > > side of the quirk context it was in. > > > still can't fully understand your idea. > > if following your suggestion, and remove usage in > dwc3_gadget_suspend_interrupt(), > > link_state only used in link state change interrupt, we can only think it > has old link_state meaning. You're reusing dwc->link_state for something outside of the quirk, and you update it outside link state change event. The condition checking for the quirk may no longer be the same anymore. This can cause regression. BR, Thinh > > > > > > I believe it's sufficient if we can just use a toggle flag (your > > suspend_irq_happen) to handle your case. > > > > Thanks, > > Thinh > > > > > + > > > switch (event->type) { > > > case DWC3_DEVICE_EVENT_DISCONNECT: > > > dwc3_gadget_disconnect_interrupt(dwc); > > > @@ -4323,7 +4339,7 @@ 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"); > > > @@ -4334,7 +4350,7 @@ static void dwc3_gadget_interrupt(struct dwc3 *dwc, > > > 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
