Re: [PATCH v4] usb: dwc3: update link state on each device level interrupt

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

 



On Tue, Apr 25, 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>
> ---
> 
> v4: (refer v3 https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/1682053861-21737-1-git-send-email-quic_linyyuan@xxxxxxxxxxx/__;!!A4F2R9G_pg!a12WTwnynMWevTe8tSyxx3MoF0XhKz9k38CjFuYKOc2vD19DEMloyUsVztwSyMqoNepUxoxv6DvVIqK777AtShabXLCHgg$ )
> 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!a12WTwnynMWevTe8tSyxx3MoF0XhKz9k38CjFuYKOc2vD19DEMloyUsVztwSyMqoNepUxoxv6DvVIqK777AtShZcfcckDg$ )
> 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!a12WTwnynMWevTe8tSyxx3MoF0XhKz9k38CjFuYKOc2vD19DEMloyUsVztwSyMqoNepUxoxv6DvVIqK777AtShY6L1v3RA$ 
> 
>  drivers/usb/dwc3/core.h   |  1 +
>  drivers/usb/dwc3/gadget.c | 30 ++++++++++++++++++------------
>  2 files changed, 19 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;

Still not documenting this... Why?

>  
>  	u16			imod_interval;
>  
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index c0ca4d1..9dc64a4 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2440,7 +2440,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,29 @@ 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->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)
> +{
> +	if (event->type != DWC3_DEVICE_EVENT_LINK_STATUS_CHANGE)
> +		dwc->link_state = event->event_info & DWC3_LINK_STATE_MASK;
> +
> +	if  (event->type != DWC3_DEVICE_EVENT_SUSPEND)
> +		dwc->suspend_irq_happen = false;
>  }
>  
>  static void dwc3_gadget_interrupt(struct dwc3 *dwc,
>  		const struct dwc3_event_devt *event)
>  {
> +	dwc3_gadget_interrupt_early(dwc, event);
> +

This may cause regression for the quirk for dwc_usb3 v2.50a and
previous as noted earlier. Why are you still doing it like this?

BR,
Thinh

>  	switch (event->type) {
>  	case DWC3_DEVICE_EVENT_DISCONNECT:
>  		dwc3_gadget_disconnect_interrupt(dwc);
> @@ -4323,7 +4329,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 +4340,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
> 




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

  Powered by Linux