On Thu, May 11, 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.
>
> Fixes: 6f26ebb79a84 ("usb: dwc3: gadget: Rename EOPF event macros to Suspend")
That change is unrelated to what this fixes. Change to this:
Fixes: 799e9dc82968 ("usb: dwc3: gadget: conditionally disable Link State change events")
Thanks,
Thinh
> Acked-by: Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx>
> Signed-off-by: Linyu Yuan <quic_linyyuan@xxxxxxxxxxx>
> ---
>
> v9: (refer v8 https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/20230510014718.13872-1-quic_linyyuan@xxxxxxxxxxx/__;!!A4F2R9G_pg!fXFLPApL2C4HhrxpqTxcoAHO1ze8zfNE4AuzweO-KJw9EbeP1cBbHII8xLUdcpCgIHivCp_GAiFNyRVQSYNpJczpAuIfoA$ )
> 1) add a Fixes tag
> 2) add Acked-by tag from Thinh
>
> v8: (refer v7 https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/20230509050743.5781-1-quic_linyyuan@xxxxxxxxxxx/__;!!A4F2R9G_pg!fXFLPApL2C4HhrxpqTxcoAHO1ze8zfNE4AuzweO-KJw9EbeP1cBbHII8xLUdcpCgIHivCp_GAiFNyRVQSYNpJczy3KrvGg$ )
> 1) move some suspended flag clear to specific event handler
>
> v7: (refer v6 https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/20230505014902.27313-1-quic_linyyuan@xxxxxxxxxxx/__;!!A4F2R9G_pg!fXFLPApL2C4HhrxpqTxcoAHO1ze8zfNE4AuzweO-KJw9EbeP1cBbHII8xLUdcpCgIHivCp_GAiFNyRVQSYNpJcxpYUi6EA$ )
> 1) reword suspended flag comment
> 2) remove one extra space in if operation
> 4) clear suspended flag for wakeup/reset/disconnect interrupt
> 3) clear suspended flag for remote wakeup related case.
>
> v6: (refer v5 https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/1682476780-2367-1-git-send-email-quic_linyyuan@xxxxxxxxxxx/__;!!A4F2R9G_pg!fXFLPApL2C4HhrxpqTxcoAHO1ze8zfNE4AuzweO-KJw9EbeP1cBbHII8xLUdcpCgIHivCp_GAiFNyRVQSYNpJcx4sSbscA$ )
> 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!fXFLPApL2C4HhrxpqTxcoAHO1ze8zfNE4AuzweO-KJw9EbeP1cBbHII8xLUdcpCgIHivCp_GAiFNyRVQSYNpJczljSAAXQ$ )
> 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!fXFLPApL2C4HhrxpqTxcoAHO1ze8zfNE4AuzweO-KJw9EbeP1cBbHII8xLUdcpCgIHivCp_GAiFNyRVQSYNpJcxgG00LTA$ )
> 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!fXFLPApL2C4HhrxpqTxcoAHO1ze8zfNE4AuzweO-KJw9EbeP1cBbHII8xLUdcpCgIHivCp_GAiFNyRVQSYNpJcyY04T_6w$ )
> 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!fXFLPApL2C4HhrxpqTxcoAHO1ze8zfNE4AuzweO-KJw9EbeP1cBbHII8xLUdcpCgIHivCp_GAiFNyRVQSYNpJcwl52Uxdg$
>
> drivers/usb/dwc3/core.h | 2 ++
> drivers/usb/dwc3/gadget.c | 12 +++++++++++-
> 2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index d56457c02996..1f043c31a096 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 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..f244bebf1ea0 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2440,6 +2440,7 @@ static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int intf_id)
> return -EINVAL;
> }
> dwc3_resume_gadget(dwc);
> + dwc->suspended = false;
> dwc->link_state = DWC3_LINK_STATE_U0;
> }
>
> @@ -3938,6 +3939,8 @@ static void dwc3_gadget_disconnect_interrupt(struct dwc3 *dwc)
> {
> int reg;
>
> + dwc->suspended = false;
> +
> dwc3_gadget_set_link_state(dwc, DWC3_LINK_STATE_RX_DET);
>
> reg = dwc3_readl(dwc->regs, DWC3_DCTL);
> @@ -3962,6 +3965,8 @@ static void dwc3_gadget_reset_interrupt(struct dwc3 *dwc)
> {
> u32 reg;
>
> + dwc->suspended = false;
> +
> /*
> * Ideally, dwc3_reset_gadget() would trigger the function
> * drivers to stop any active transfers through ep disable.
> @@ -4180,6 +4185,8 @@ static void dwc3_gadget_conndone_interrupt(struct dwc3 *dwc)
>
> static void dwc3_gadget_wakeup_interrupt(struct dwc3 *dwc, unsigned int evtinfo)
> {
> + dwc->suspended = false;
> +
> /*
> * TODO take core out of low power mode when that's
> * implemented.
> @@ -4277,6 +4284,7 @@ static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc,
> if (dwc->gadget->wakeup_armed) {
> dwc3_gadget_enable_linksts_evts(dwc, false);
> dwc3_resume_gadget(dwc);
> + dwc->suspended = false;
> }
> break;
> case DWC3_LINK_STATE_U1:
> @@ -4303,8 +4311,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;
> }
> --
> 2.17.1
>
