Re: [PATCH] usb: dwc2: Revert "usb: dwc2: Disable all EP's on disconnect"

[Minas Harutyunyan <minas.harutyunyan@synopsys.com>]

Hi Marek,

On 11/21/2018 7:45 PM, Marek Szyprowski wrote:
> This reverts commit dccf1bad4be7eaa096c1f3697bd37883f9a08ecb.
> 
> The reverted commit breaks locking in the DWC2 driver. It causes random
> crashes or memory corruption related issues on SMP machines. Here is an
> example of the observed reproducible issue (other are a bit more random):
> 
> =====================================
> WARNING: bad unlock balance detected!
> 4.19.0-rc6-00027-gdccf1bad4be7-dirty #1119 Not tainted
> -------------------------------------
> ip/1464 is trying to release lock (&(&hsotg->lock)->rlock) at:
> [<c0615494>] dwc2_hsotg_complete_request+0x84/0x218
> but there are no more locks to release!
> 
> other info that might help us debug this:
> 2 locks held by ip/1464:
>   #0: d69babd3 (rtnl_mutex){+.+.}, at: rtnetlink_rcv_msg+0x224/0x488
>   #1: 5fb350d2 (&(&dev->lock)->rlock){-.-.}, at: eth_stop+0x24/0xa8
> 
> stack backtrace:
> CPU: 1 PID: 1464 Comm: ip Not tainted 4.19.0-rc6-00027-gdccf1bad4be7-dirty #1119
> Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> [<c0111f9c>] (unwind_backtrace) from [<c010deb8>] (show_stack+0x10/0x14)
> [<c010deb8>] (show_stack) from [<c09d3398>] (dump_stack+0x90/0xc8)
> [<c09d3398>] (dump_stack) from [<c017d02c>] (print_unlock_imbalance_bug+0xb0/0xe0)
> [<c017d02c>] (print_unlock_imbalance_bug) from [<c01800dc>] (lock_release+0x1a4/0x374)
> [<c01800dc>] (lock_release) from [<c09ef7fc>] (_raw_spin_unlock+0x18/0x54)
> [<c09ef7fc>] (_raw_spin_unlock) from [<c0615494>] (dwc2_hsotg_complete_request+0x84/0x218)
> [<c0615494>] (dwc2_hsotg_complete_request) from [<c0617530>] (kill_all_requests+0x44/0xb4)
> [<c0617530>] (kill_all_requests) from [<c0617690>] (dwc2_hsotg_ep_disable+0xf0/0x200)
> [<c0617690>] (dwc2_hsotg_ep_disable) from [<c0659698>] (usb_ep_disable+0xd0/0x1c8)
> [<c0659698>] (usb_ep_disable) from [<c065d4a8>] (eth_stop+0x68/0xa8)
> [<c065d4a8>] (eth_stop) from [<c077e0b8>] (__dev_close_many+0x94/0xfc)
> [<c077e0b8>] (__dev_close_many) from [<c078a064>] (__dev_change_flags+0xa0/0x198)
> [<c078a064>] (__dev_change_flags) from [<c078a17c>] (dev_change_flags+0x18/0x48)
> [<c078a17c>] (dev_change_flags) from [<c07a1a98>] (do_setlink+0x298/0x990)
> [<c07a1a98>] (do_setlink) from [<c07a29f0>] (rtnl_newlink+0x4a0/0x6fc)
> [<c07a29f0>] (rtnl_newlink) from [<c079dd74>] (rtnetlink_rcv_msg+0x254/0x488)
> [<c079dd74>] (rtnetlink_rcv_msg) from [<c07c47f4>] (netlink_rcv_skb+0xe0/0x118)
> [<c07c47f4>] (netlink_rcv_skb) from [<c07c4094>] (netlink_unicast+0x180/0x1c8)
> [<c07c4094>] (netlink_unicast) from [<c07c4428>] (netlink_sendmsg+0x2bc/0x348)
> [<c07c4428>] (netlink_sendmsg) from [<c0760b9c>] (sock_sendmsg+0x14/0x24)
> [<c0760b9c>] (sock_sendmsg) from [<c0761af8>] (___sys_sendmsg+0x22c/0x248)
> [<c0761af8>] (___sys_sendmsg) from [<c0762740>] (__sys_sendmsg+0x40/0x6c)
> [<c0762740>] (__sys_sendmsg) from [<c0101000>] (ret_fast_syscall+0x0/0x28)
> Exception stack(0xede65fa8 to 0xede65ff0)
> ...
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
> The suspicious locking has been already pointed in the review of the
> first version of this patch, but sadly the v2 didn't change it and
> landed in v4.20-rc1.
> ---
>   drivers/usb/dwc2/gadget.c | 30 +++++++-----------------------
>   1 file changed, 7 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index 79189db4bf17..220c0f9b89b0 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -3109,8 +3109,6 @@ static void kill_all_requests(struct dwc2_hsotg *hsotg,
>   		dwc2_hsotg_txfifo_flush(hsotg, ep->fifo_index);
>   }
>   
> -static int dwc2_hsotg_ep_disable(struct usb_ep *ep);
> -
>   /**
>    * dwc2_hsotg_disconnect - disconnect service
>    * @hsotg: The device state.
> @@ -3129,12 +3127,13 @@ void dwc2_hsotg_disconnect(struct dwc2_hsotg *hsotg)
>   	hsotg->connected = 0;
>   	hsotg->test_mode = 0;
>   
> -	/* all endpoints should be shutdown */
>   	for (ep = 0; ep < hsotg->num_of_eps; ep++) {
>   		if (hsotg->eps_in[ep])
> -			dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
> +			kill_all_requests(hsotg, hsotg->eps_in[ep],
> +					  -ESHUTDOWN);
>   		if (hsotg->eps_out[ep])
> -			dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
> +			kill_all_requests(hsotg, hsotg->eps_out[ep],
> +					  -ESHUTDOWN);
>   	}
>   
>   	call_gadget(hsotg, disconnect);
> @@ -3192,23 +3191,13 @@ void dwc2_hsotg_core_init_disconnected(struct dwc2_hsotg *hsotg,
>   	u32 val;
>   	u32 usbcfg;
>   	u32 dcfg = 0;
> -	int ep;
>   
>   	/* Kill any ep0 requests as controller will be reinitialized */
>   	kill_all_requests(hsotg, hsotg->eps_out[0], -ECONNRESET);
>   
> -	if (!is_usb_reset) {
> +	if (!is_usb_reset)
>   		if (dwc2_core_reset(hsotg, true))
>   			return;
> -	} else {
> -		/* all endpoints should be shutdown */
> -		for (ep = 1; ep < hsotg->num_of_eps; ep++) {
> -			if (hsotg->eps_in[ep])
> -				dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
> -			if (hsotg->eps_out[ep])
> -				dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
> -		}
> -	}
>   
>   	/*
>   	 * we must now enable ep0 ready for host detection and then
> @@ -4004,7 +3993,6 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
>   	unsigned long flags;
>   	u32 epctrl_reg;
>   	u32 ctrl;
> -	int locked;
>   
>   	dev_dbg(hsotg->dev, "%s(ep %p)\n", __func__, ep);
>   
> @@ -4020,9 +4008,7 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
>   
>   	epctrl_reg = dir_in ? DIEPCTL(index) : DOEPCTL(index);
>   
> -	locked = spin_is_locked(&hsotg->lock);
> -	if (!locked)
> -		spin_lock_irqsave(&hsotg->lock, flags);
> +	spin_lock_irqsave(&hsotg->lock, flags);
>   
>   	ctrl = dwc2_readl(hsotg, epctrl_reg);
>   
> @@ -4046,9 +4032,7 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
>   	hs_ep->fifo_index = 0;
>   	hs_ep->fifo_size = 0;
>   
> -	if (!locked)
> -		spin_unlock_irqrestore(&hsotg->lock, flags);
> -
> +	spin_unlock_irqrestore(&hsotg->lock, flags);
>   	return 0;
>   }
>   
> 

Could you please apply "[PATCH v2] usb: dwc2: Disable all EP's on 
disconnect" and fix for that patch "[PATCH] usb: dwc2: Fix ep disable 
spinlock flow.". Let me know on test results.

Thanks,
Minas