Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Minas Harutyunyan <minas.harutyunyan@synopsys.com>
To:     Dan Carpenter <dan.carpenter@oracle.com>,
        Minas Harutyunyan <minas.harutyunyan@synopsys.com>
CC:     Marek Szyprowski <m.szyprowski@samsung.com>,
        Maynard CABIENTE <maynard.cabiente@raritan.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Felipe Balbi <felipe.balbi@linux.intel.com>,
        Geert Uytterhoeven <geert@linux-m68k.org>,
        Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Subject: Re: [PATCH] usb: dwc2: Revert "usb: dwc2: Disable all EP's on
 disconnect"
Thread-Topic: [PATCH] usb: dwc2: Revert "usb: dwc2: Disable all EP's on
 disconnect"
Thread-Index: AQHUgbE8ouTBXi+nzkarEEkfP9zP5Q==
Date:   Wed, 5 Dec 2018 12:52:22 +0000
Message-ID: <410670D7E743164D87FA6160E7907A56013A7B2847@am04wembxa.internal.synopsys.com>
References: <CGME20181121154518eucas1p2cc7ffd4d071ff420c747e1d0c387ee5d@eucas1p2.samsung.com>
 <20181121154504.13052-1-m.szyprowski@samsung.com>
 <410670D7E743164D87FA6160E7907A56013A7AA1EE@am04wembxa.internal.synopsys.com>
 <20181123144307.GC2970@unbuntlaptop>
 <410670D7E743164D87FA6160E7907A56013A7B19C2@am04wembxa.internal.synopsys.com>
 <20181204132913.GH3073@unbuntlaptop>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi,

On 12/4/2018 5:29 PM, Dan Carpenter wrote:
> On Tue, Dec 04, 2018 at 12:34:08PM +0000, Minas Harutyunyan wrote:
>> @@ -3185,12 +3183,13 @@ void dwc2_hsotg_disconnect(struct dwc2_hsotg *hs=
otg)
>>           hsotg->connected =3D 0;
>>           hsotg->test_mode =3D 0;
>>
>> -       /* all endpoints should be shutdown */
>>           for (ep =3D 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);
>=20
>=20
> Should this part be in a separate patch?
>=20
> I'm not trying to be rhetorical at all.  I literally don't know the
> code very well.  Hopefully the full commit message will explain it.
>=20

Actually, this fragment of patch revert changes from V2 and keep=20
untouched dwc2_hsotg_disconnect() function.

>>           }
>>
>>           call_gadget(hsotg, disconnect);
>> @@ -3234,6 +3233,8 @@ static void dwc2_hsotg_irq_fifoempty(struct
>> dwc2_hsotg *hsotg, bool periodic)
>>                           GINTSTS_PTXFEMP |  \
>>                           GINTSTS_RXFLVL)
>>
>> +static int dwc2_hsotg_ep_disable(struct usb_ep *ep);
>> +
>>    /**
>>     * dwc2_hsotg_core_init - issue softreset to the core
>>     * @hsotg: The device state
>> @@ -3258,12 +3259,14 @@ void dwc2_hsotg_core_init_disconnected(struct
>> dwc2_hsotg *hsotg,
>>                           return;
>>           } else {
>>                   /* all endpoints should be shutdown */
>> +               spin_unlock(&hsotg->lock);
>>                   for (ep =3D 1; ep < hsotg->num_of_eps; ep++) {
>>                           if (hsotg->eps_in[ep])
>>  =20
>> dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
>>                           if (hsotg->eps_out[ep])
>>  =20
>> dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
>>                   }
>> +               spin_lock(&hsotg->lock);
>>           }
>>
>>           /*
>=20
> The idea here is that this is the only caller which is holding the
> lock and we drop it here and take it again inside dwc2_hsotg_ep_disable()=
.
> I don't know the code very well and can't totally swear that this
> doesn't introduce a small race condition...
>=20
Above fragment of patch allow to keep untouched dwc2_hsotg_ep_disble()=20
function also, without changing spin_lock/_unlock stuff inside function.

My approach here minimally update code to add any races. Just in=20
dwc2_hsotg_core_init_disconnected() function on USB reset interrupt=20
perform disabling all EP's. Because on USB reset interrupt, called from int=
errupt=20
handler with acquired lock and dwc2_hsotg_ep_disble() function (without=20
changes) acquire lock, just need to unlock lock to avoid any troubles.

> Another option would be to introduce a new function which takes the lock
> and change all the other callers instead.  To me that would be easier to
> review...  See below for how it might look:
>=20
> regards,
> dan carpenter
>=20
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index 94f3ba995580..b17a5dbefd5f 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -3166,6 +3166,7 @@ static void kill_all_requests(struct dwc2_hsotg *hs=
otg,
>   }
>  =20
>   static int dwc2_hsotg_ep_disable(struct usb_ep *ep);
> +static int dwc2_hsotg_ep_disable_lock(struct usb_ep *ep);
>  =20
>   /**
>    * dwc2_hsotg_disconnect - disconnect service
> @@ -3188,9 +3189,9 @@ void dwc2_hsotg_disconnect(struct dwc2_hsotg *hsotg=
)
>   	/* all endpoints should be shutdown */
>   	for (ep =3D 0; ep < hsotg->num_of_eps; ep++) {
>   		if (hsotg->eps_in[ep])
> -			dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
> +			dwc2_hsotg_ep_disable_lock(&hsotg->eps_in[ep]->ep);
>   		if (hsotg->eps_out[ep])
> -			dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
> +			dwc2_hsotg_ep_disable_lock(&hsotg->eps_out[ep]->ep);
>   	}
>  =20
>   	call_gadget(hsotg, disconnect);
> @@ -4069,10 +4070,8 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep=
)
>   	struct dwc2_hsotg *hsotg =3D hs_ep->parent;
>   	int dir_in =3D hs_ep->dir_in;
>   	int index =3D hs_ep->index;
> -	unsigned long flags;
>   	u32 epctrl_reg;
>   	u32 ctrl;
> -	int locked;
>  =20
>   	dev_dbg(hsotg->dev, "%s(ep %p)\n", __func__, ep);
>  =20
> @@ -4088,10 +4087,6 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep=
)
>  =20
>   	epctrl_reg =3D dir_in ? DIEPCTL(index) : DOEPCTL(index);
>  =20
> -	locked =3D spin_is_locked(&hsotg->lock);
> -	if (!locked)
> -		spin_lock_irqsave(&hsotg->lock, flags);
> -
>   	ctrl =3D dwc2_readl(hsotg, epctrl_reg);
>  =20
>   	if (ctrl & DXEPCTL_EPENA)
> @@ -4114,12 +4109,23 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *e=
p)
>   	hs_ep->fifo_index =3D 0;
>   	hs_ep->fifo_size =3D 0;
>  =20
> -	if (!locked)
> -		spin_unlock_irqrestore(&hsotg->lock, flags);
> -
>   	return 0;
>   }
>  =20
> +static int dwc2_hsotg_ep_disable_lock(struct usb_ep *ep)
> +{
> +	struct dwc2_hsotg_ep *hs_ep =3D our_ep(ep);
> +	struct dwc2_hsotg *hsotg =3D hs_ep->parent;
> +	unsigned long flags;
> +	int ret;
> +
> +	spin_lock_irqsave(&hsotg->lock, flags);
> +	ret =3D dwc2_hsotg_ep_disable(ep);
> +	spin_unlock_irqrestore(&hsotg->lock, flags);
> +
> +	return ret;
> +}
> +
>   /**
>    * on_list - check request is on the given endpoint
>    * @ep: The endpoint to check.
> @@ -4267,7 +4273,7 @@ static int dwc2_hsotg_ep_sethalt_lock(struct usb_ep=
 *ep, int value)
>  =20
>   static const struct usb_ep_ops dwc2_hsotg_ep_ops =3D {
>   	.enable		=3D dwc2_hsotg_ep_enable,
> -	.disable	=3D dwc2_hsotg_ep_disable,
> +	.disable	=3D dwc2_hsotg_ep_disable_lock,
>   	.alloc_request	=3D dwc2_hsotg_ep_alloc_request,
>   	.free_request	=3D dwc2_hsotg_ep_free_request,
>   	.queue		=3D dwc2_hsotg_ep_queue_lock,
> @@ -4407,9 +4413,9 @@ static int dwc2_hsotg_udc_stop(struct usb_gadget *g=
adget)
>   	/* all endpoints should be shutdown */
>   	for (ep =3D 1; ep < hsotg->num_of_eps; ep++) {
>   		if (hsotg->eps_in[ep])
> -			dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
> +			dwc2_hsotg_ep_disable_lock(&hsotg->eps_in[ep]->ep);
>   		if (hsotg->eps_out[ep])
> -			dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
> +			dwc2_hsotg_ep_disable_lock(&hsotg->eps_out[ep]->ep);
>   	}
>  =20
>   	spin_lock_irqsave(&hsotg->lock, flags);
> @@ -4857,9 +4863,9 @@ int dwc2_hsotg_suspend(struct dwc2_hsotg *hsotg)
>  =20
>   		for (ep =3D 0; ep < hsotg->num_of_eps; ep++) {
>   			if (hsotg->eps_in[ep])
> -				dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
> +				dwc2_hsotg_ep_disable_lock(&hsotg->eps_in[ep]->ep);
>   			if (hsotg->eps_out[ep])
> -				dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
> +				dwc2_hsotg_ep_disable_lock(&hsotg->eps_out[ep]->ep);
>   		}
>   	}
>  =20
>=20

Your code doesn't take care about fifo_map warnings from=20
dwc2_hsotg_init_fifo() function. Before calling dwc2_hsotg_init_fifo()=20
from dwc2_hsotg_core_init_disconnected() function all Ep's should=20
disabled and fifo bitmap should be cleared.


Thanks,
Minas