| Message ID | 1675221286-23833-1-git-send-email-quic_linyyuan@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: dwc3: update link state when process wakeup interrupt | expand |
On Wed, Feb 01, 2023, Linyu Yuan wrote: > Consider there is interrpt sequences as suspend (U3) -> wakeup (U0) -> interrupt? > suspend (U3), as there is no update to link state in wakeup interrupt, Instead of "no update", can you note in the commit that the link state change event is not enabled for most devices, so the driver doesn't update its link_state. > the second suspend interrupt will not report to upper layer. > > Fix it by update link state in wakeup interrupt handler. > > Cc: stable@vger.kernel.org Can you add fix tag? > Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> > --- > drivers/usb/dwc3/gadget.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index 89dcfac..3533241 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -4066,7 +4066,7 @@ static void dwc3_gadget_conndone_interrupt(struct dwc3 *dwc) > */ > } > > -static void dwc3_gadget_wakeup_interrupt(struct dwc3 *dwc) > +static void dwc3_gadget_wakeup_interrupt(struct dwc3 *dwc, unsigned int evtinfo) > { > /* > * TODO take core out of low power mode when that's > @@ -4078,6 +4078,8 @@ static void dwc3_gadget_wakeup_interrupt(struct dwc3 *dwc) > 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, > @@ -4227,7 +4229,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); > + dwc3_gadget_wakeup_interrupt(dwc, event->event_info); > break; > case DWC3_DEVICE_EVENT_HIBER_REQ: > if (dev_WARN_ONCE(dwc->dev, !dwc->has_hibernation, > -- > 2.7.4 > Thanks, Thinh
On 2/2/2023 3:05 AM, Thinh Nguyen wrote: > On Wed, Feb 01, 2023, Linyu Yuan wrote: >> Consider there is interrpt sequences as suspend (U3) -> wakeup (U0) -> > interrupt? thanks, will change next version. > >> suspend (U3), as there is no update to link state in wakeup interrupt, > Instead of "no update", can you note in the commit that the link state > change event is not enabled for most devices, so the driver doesn't > update its link_state. thanks, will change next version. > >> the second suspend interrupt will not report to upper layer. >> >> Fix it by update link state in wakeup interrupt handler. >> >> Cc: stable@vger.kernel.org > Can you add fix tag? seem this change can apply to all current stable kernel. I think CC stable is good. also it is not good to find appreciate tag. > >> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> >> --- >> drivers/usb/dwc3/gadget.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >> index 89dcfac..3533241 100644 >> --- a/drivers/usb/dwc3/gadget.c >> +++ b/drivers/usb/dwc3/gadget.c >> @@ -4066,7 +4066,7 @@ static void dwc3_gadget_conndone_interrupt(struct dwc3 *dwc) >> */ >> } >> >> -static void dwc3_gadget_wakeup_interrupt(struct dwc3 *dwc) >> +static void dwc3_gadget_wakeup_interrupt(struct dwc3 *dwc, unsigned int evtinfo) >> { >> /* >> * TODO take core out of low power mode when that's >> @@ -4078,6 +4078,8 @@ static void dwc3_gadget_wakeup_interrupt(struct dwc3 *dwc) >> 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, >> @@ -4227,7 +4229,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); >> + dwc3_gadget_wakeup_interrupt(dwc, event->event_info); >> break; >> case DWC3_DEVICE_EVENT_HIBER_REQ: >> if (dev_WARN_ONCE(dwc->dev, !dwc->has_hibernation, >> -- >> 2.7.4 >> > Thanks, > Thinh
hi Thinh,
do you prefer below change ? will it be good for all cases ?
+static void dwc3_gadget_update_link_state(struct dwc3 *dwc,
+ const struct dwc3_event_devt *event)
+{
+ switch (event->type) {
+ case DWC3_DEVICE_EVENT_HIBER_REQ:
+ case DWC3_DEVICE_EVENT_LINK_STATUS_CHANGE:
+ case DWC3_DEVICE_EVENT_SUSPEND:
+ break;
+ default:
+ dwc->link_state = event->event_info & DWC3_LINK_STATE_MASK;
+ break;
+ }
+}
+
static void dwc3_gadget_interrupt(struct dwc3 *dwc,
const struct dwc3_event_devt *event)
{
+ dwc3_gadget_update_link_state(dwc3, event);
+
switch (event->type)
On Thu, Feb 02, 2023, Linyu Yuan wrote: > > On 2/2/2023 3:05 AM, Thinh Nguyen wrote: > > On Wed, Feb 01, 2023, Linyu Yuan wrote: > > > Consider there is interrpt sequences as suspend (U3) -> wakeup (U0) -> > > interrupt? > > > thanks, will change next version. > > > > > > > suspend (U3), as there is no update to link state in wakeup interrupt, > > Instead of "no update", can you note in the commit that the link state > > change event is not enabled for most devices, so the driver doesn't > > update its link_state. > > > thanks, will change next version. > > > > > > > the second suspend interrupt will not report to upper layer. > > > > > > Fix it by update link state in wakeup interrupt handler. > > > > > > Cc: stable@vger.kernel.org > > Can you add fix tag? > > > seem this change can apply to all current stable kernel. Did we have handling of suspend/resume since the beginning? If we did, please add a fix tag to the commit when the driver first added. That helps to know that this is a fix patch. Thanks, Thinh > > I think CC stable is good. also it is not good to find appreciate tag. >
On Thu, Feb 02, 2023, Linyu Yuan wrote: > > hi Thinh, > > > do you prefer below change ? will it be good for all cases ? > > > +static void dwc3_gadget_update_link_state(struct dwc3 *dwc, > + const struct dwc3_event_devt *event) > +{ > + switch (event->type) { > + case DWC3_DEVICE_EVENT_HIBER_REQ: > + case DWC3_DEVICE_EVENT_LINK_STATUS_CHANGE: > + case DWC3_DEVICE_EVENT_SUSPEND: > + break; > + default: > + dwc->link_state = event->event_info & DWC3_LINK_STATE_MASK; > + break; > + } > +} > + > static void dwc3_gadget_interrupt(struct dwc3 *dwc, > const struct dwc3_event_devt *event) > { > + dwc3_gadget_update_link_state(dwc3, event); > + > switch (event->type) > > This would break the check in dwc3_gadget_suspend_interrupt(). However, I'm actually not sure why we had that check in the beginning. I suppose certain setup may trigger suspend event multiple time consecutively? Thanks, Thinh
On Thu, Feb 02, 2023 at 08:10:01PM +0000, Thinh Nguyen wrote: > On Thu, Feb 02, 2023, Linyu Yuan wrote: > > > > On 2/2/2023 3:05 AM, Thinh Nguyen wrote: > > > On Wed, Feb 01, 2023, Linyu Yuan wrote: > > > > Consider there is interrpt sequences as suspend (U3) -> wakeup (U0) -> > > > interrupt? > > > > > > thanks, will change next version. > > > > > > > > > > > suspend (U3), as there is no update to link state in wakeup interrupt, > > > Instead of "no update", can you note in the commit that the link state > > > change event is not enabled for most devices, so the driver doesn't > > > update its link_state. > > > > > > thanks, will change next version. > > > > > > > > > > > the second suspend interrupt will not report to upper layer. > > > > > > > > Fix it by update link state in wakeup interrupt handler. > > > > > > > > Cc: stable@vger.kernel.org > > > Can you add fix tag? > > > > > > seem this change can apply to all current stable kernel. > > Did we have handling of suspend/resume since the beginning? If we did, > please add a fix tag to the commit when the driver first added. > > That helps to know that this is a fix patch. Suspend was added with my change: d1d90dd27254 ("usb: dwc3: gadget: Enable suspend events"), so arguably the link_state mismatch of $SUBJECT wouldn't have occurred prior to that if suspend_interrupt() never got called right? ;) But strictly speaking, wakeup_interrupt() was introduced all the way back in the first commit 72246da40f37 ("usb: Introduce DesignWare USB3 DRD Driver") and had not been changed since. Any preference which tag to choose for Fixes? Jack
On Thu, Feb 02, 2023, Jack Pham wrote: > On Thu, Feb 02, 2023 at 08:10:01PM +0000, Thinh Nguyen wrote: > > On Thu, Feb 02, 2023, Linyu Yuan wrote: > > > > > > On 2/2/2023 3:05 AM, Thinh Nguyen wrote: > > > > On Wed, Feb 01, 2023, Linyu Yuan wrote: > > > > > Consider there is interrpt sequences as suspend (U3) -> wakeup (U0) -> > > > > interrupt? > > > > > > > > > thanks, will change next version. > > > > > > > > > > > > > > > suspend (U3), as there is no update to link state in wakeup interrupt, > > > > Instead of "no update", can you note in the commit that the link state > > > > change event is not enabled for most devices, so the driver doesn't > > > > update its link_state. > > > > > > > > > thanks, will change next version. > > > > > > > > > > > > > > > the second suspend interrupt will not report to upper layer. > > > > > > > > > > Fix it by update link state in wakeup interrupt handler. > > > > > > > > > > Cc: stable@vger.kernel.org > > > > Can you add fix tag? > > > > > > > > > seem this change can apply to all current stable kernel. > > > > Did we have handling of suspend/resume since the beginning? If we did, > > please add a fix tag to the commit when the driver first added. > > > > That helps to know that this is a fix patch. > > Suspend was added with my change: d1d90dd27254 ("usb: dwc3: gadget: Enable suspend > events"), so arguably the link_state mismatch of $SUBJECT wouldn't have occurred > prior to that if suspend_interrupt() never got called right? ;) > > But strictly speaking, wakeup_interrupt() was introduced all the way back > in the first commit 72246da40f37 ("usb: Introduce DesignWare USB3 DRD Driver") > and had not been changed since. > > Any preference which tag to choose for Fixes? > Please pick how far back it can go while it still makes sense. This helps those who want to backport it. Thanks, Thinh
On Thu, Feb 02, 2023, Jack Pham wrote: > On Thu, Feb 02, 2023 at 08:10:01PM +0000, Thinh Nguyen wrote: > > On Thu, Feb 02, 2023, Linyu Yuan wrote: > > > > > > On 2/2/2023 3:05 AM, Thinh Nguyen wrote: > > > > On Wed, Feb 01, 2023, Linyu Yuan wrote: > > > > > Consider there is interrpt sequences as suspend (U3) -> wakeup (U0) -> > > > > interrupt? > > > > > > > > > thanks, will change next version. > > > > > > > > > > > > > > > suspend (U3), as there is no update to link state in wakeup interrupt, > > > > Instead of "no update", can you note in the commit that the link state > > > > change event is not enabled for most devices, so the driver doesn't > > > > update its link_state. > > > > > > > > > thanks, will change next version. > > > > > > > > > > > > > > > the second suspend interrupt will not report to upper layer. > > > > > > > > > > Fix it by update link state in wakeup interrupt handler. > > > > > > > > > > Cc: stable@vger.kernel.org > > > > Can you add fix tag? > > > > > > > > > seem this change can apply to all current stable kernel. > > > > Did we have handling of suspend/resume since the beginning? If we did, > > please add a fix tag to the commit when the driver first added. > > > > That helps to know that this is a fix patch. > > Suspend was added with my change: d1d90dd27254 ("usb: dwc3: gadget: Enable suspend > events"), so arguably the link_state mismatch of $SUBJECT wouldn't have occurred > prior to that if suspend_interrupt() never got called right? ;) > > But strictly speaking, wakeup_interrupt() was introduced all the way back > in the first commit 72246da40f37 ("usb: Introduce DesignWare USB3 DRD Driver") > and had not been changed since. > > Any preference which tag to choose for Fixes? > Perhaps this: 72704f876f50 ("dwc3: gadget: Implement the suspend entry event handler") Thanks, Thinh
On Thu, Feb 02, 2023, Thinh Nguyen wrote: > On Thu, Feb 02, 2023, Linyu Yuan wrote: > > > > hi Thinh, > > > > > > do you prefer below change ? will it be good for all cases ? > > > > > > +static void dwc3_gadget_update_link_state(struct dwc3 *dwc, > > + const struct dwc3_event_devt *event) > > +{ > > + switch (event->type) { > > + case DWC3_DEVICE_EVENT_HIBER_REQ: > > + case DWC3_DEVICE_EVENT_LINK_STATUS_CHANGE: > > + case DWC3_DEVICE_EVENT_SUSPEND: > > + break; > > + default: > > + dwc->link_state = event->event_info & DWC3_LINK_STATE_MASK; > > + break; > > + } > > +} > > + > > static void dwc3_gadget_interrupt(struct dwc3 *dwc, > > const struct dwc3_event_devt *event) > > { > > + dwc3_gadget_update_link_state(dwc3, event); > > + > > switch (event->type) > > > > > > This would break the check in dwc3_gadget_suspend_interrupt(). However, > I'm actually not sure why we had that check in the beginning. I suppose > certain setup may trigger suspend event multiple time consecutively? > Actually, looking at it again, you're skipping updating the events listed above and not for every event. So that should work. For some reason, I thought you want to update the link_state for every new event. However, this would complicate the driver. Now we have to remember when to set the link_state and when not to, and when to check the previous link_state. On top of that, dwc->link_state may not reflect the current state of the controller either, which makes it more confusing. Perhaps we should not update dwc->link_state outside of link state change event, and just track whether we called gadget_driver->suspend() to call the correspond resume() on wakeup? It can be tracked with dwc->gadget_driver_is_suspended. Thanks, Thinh
On 2/3/2023 7:58 AM, Thinh Nguyen wrote: > On Thu, Feb 02, 2023, Thinh Nguyen wrote: >> On Thu, Feb 02, 2023, Linyu Yuan wrote: >>> hi Thinh, >>> >>> >>> do you prefer below change ? will it be good for all cases ? >>> >>> >>> +static void dwc3_gadget_update_link_state(struct dwc3 *dwc, >>> + const struct dwc3_event_devt *event) >>> +{ >>> + switch (event->type) { >>> + case DWC3_DEVICE_EVENT_HIBER_REQ: >>> + case DWC3_DEVICE_EVENT_LINK_STATUS_CHANGE: >>> + case DWC3_DEVICE_EVENT_SUSPEND: >>> + break; >>> + default: >>> + dwc->link_state = event->event_info & DWC3_LINK_STATE_MASK; >>> + break; >>> + } >>> +} >>> + >>> static void dwc3_gadget_interrupt(struct dwc3 *dwc, >>> const struct dwc3_event_devt *event) >>> { >>> + dwc3_gadget_update_link_state(dwc3, event); >>> + >>> switch (event->type) >>> >>> >> This would break the check in dwc3_gadget_suspend_interrupt(). However, >> I'm actually not sure why we had that check in the beginning. I suppose >> certain setup may trigger suspend event multiple time consecutively? >> > Actually, looking at it again, you're skipping updating the events > listed above and not for every event. So that should work. For some > reason, I thought you want to update the link_state for every new event. the reason why suggest new change is because we found it also need update link state in other case, e.g. suspend -> reset ->conndone -> suspend > > However, this would complicate the driver. Now we have to remember when > to set the link_state and when not to, and when to check the previous > link_state. On top of that, dwc->link_state may not reflect the current > state of the controller either, which makes it more confusing. agree now it is complicate. > > Perhaps we should not update dwc->link_state outside of link state > change event, and just track whether we called gadget_driver->suspend() > to call the correspond resume() on wakeup? It can be tracked with > dwc->gadget_driver_is_suspended. could you help provide a change for this idea ? > > Thanks, > Thinh >
On Fri, Feb 03, 2023, Linyu Yuan wrote: > > > > Perhaps we should not update dwc->link_state outside of link state > > change event, and just track whether we called gadget_driver->suspend() > > to call the correspond resume() on wakeup? It can be tracked with > > dwc->gadget_driver_is_suspended. > > > could you help provide a change for this idea ? > > Sorry for the delay in response. I was hoping to be able to allocate some time to write an experimental patch for this, but it looks like it may take some more time. The idea is to separate the use of dwc->link_state for its own purpose and track suspend/resume separately as a toggle flag only. So that we won't repeat gadget driver suspend() or wakeup() operations. Ideally we don't even need to do this. My concern is this check in dwc3_gadget_suspend_interrupt(): if (dwc->link_state != next && next == DWC3_LINK_STATE_U3) This was done 7 years ago with little info on why it was handled that way in the commit. My suspicion is the setup from Baolin may disabled link state change event also, which would require this check to determine if it had toggled (however it's not a good check as you're working to fix it now). I don't expect this fix to be big. Thanks, Thinh
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 89dcfac..3533241 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -4066,7 +4066,7 @@ static void dwc3_gadget_conndone_interrupt(struct dwc3 *dwc) */ } -static void dwc3_gadget_wakeup_interrupt(struct dwc3 *dwc) +static void dwc3_gadget_wakeup_interrupt(struct dwc3 *dwc, unsigned int evtinfo) { /* * TODO take core out of low power mode when that's @@ -4078,6 +4078,8 @@ static void dwc3_gadget_wakeup_interrupt(struct dwc3 *dwc) 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, @@ -4227,7 +4229,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); + dwc3_gadget_wakeup_interrupt(dwc, event->event_info); break; case DWC3_DEVICE_EVENT_HIBER_REQ: if (dev_WARN_ONCE(dwc->dev, !dwc->has_hibernation,
Consider there is interrpt sequences as suspend (U3) -> wakeup (U0) -> suspend (U3), as there is no update to link state in wakeup interrupt, the second suspend interrupt will not report to upper layer. Fix it by update link state in wakeup interrupt handler. Cc: stable@vger.kernel.org Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> --- drivers/usb/dwc3/gadget.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)