Re: [PATCH v6] usb: dwc3: fix gadget mode suspend interrupt handler issue

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@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"
> 
> 
> 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




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux