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:   Pawel Laszczak <pawell@cadence.com>
To:     Greg KH <gregkh@linuxfoundation.org>
CC:     "felipe.balbi@linux.intel.com" <felipe.balbi@linux.intel.com>,
        "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
        "rogerq@ti.com" <rogerq@ti.com>,
        "jbergsagel@ti.com" <jbergsagel@ti.com>,
        "nsekhar@ti.com" <nsekhar@ti.com>, "nm@ti.com" <nm@ti.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Jayshri Dajiram Pawar <jpawar@cadence.com>,
        Rahul Kumar <kurahul@cadence.com>,
        Sanket Parmar <sparmar@cadence.com>,
        Peter Chan <peter.chan@nxp.com>
Subject: RE: [PATCH] usb: cdns3: Fix: ARM core hang after connect/disconnect
 operation.
Thread-Topic: [PATCH] usb: cdns3: Fix: ARM core hang after connect/disconnect
 operation.
Thread-Index: AQHVxhgd/L7cK1vWyke0JuU/54YkXqfg05qAgADt7MA=
Date:   Thu, 9 Jan 2020 06:27:02 +0000
Message-ID: <BYAPR07MB4709983A2DF70AA0058C737FDD390@BYAPR07MB4709.namprd07.prod.outlook.com>
References: <20200108113719.21551-1-pawell@cadence.com>
 <20200108142829.GB2383861@kroah.com>
In-Reply-To: <20200108142829.GB2383861@kroah.com>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: cadence.com does not designate
 permitted sender hosts)
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 18ed0f36-a779-4fe4-256b-08d794ccf06c
X-MS-Exchange-CrossTenant-originalarrivaltime: 09 Jan 2020 06:27:02.2054
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: d36035c5-6ce6-4662-a3dc-e762e61ae4c9
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: B3PbRLqRonvGFS28OzG8GfwNO5LjFyhQ6a7HoH2jeGpJkN9cCZ6Djs0t3qK666rQcRJqbuNRXSbXeoY8x1pgwJOtKZJzVD6e1dIEXp3BQ6E=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR07MB5862
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.138,18.0.572
 definitions=2020-01-09_01:2020-01-08,2020-01-09 signatures=0
X-Proofpoint-Spam-Details: rule=outbound_check_notspam policy=outbound_check score=0 mlxlogscore=999
 impostorscore=0 suspectscore=0 clxscore=1015 priorityscore=1501 mlxscore=0
 lowpriorityscore=0 malwarescore=0 bulkscore=0 adultscore=0 spamscore=0
 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-1910280000 definitions=main-2001090054
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Greg,

+ linux-usb@vger.kernel.org

>
>On Wed, Jan 08, 2020 at 12:37:18PM +0100, Pawel Laszczak wrote:
>> The ARM core hang when access USB register after tens of thousands
>> connect/disconnect operation.
>>
>> The issue was observed on platform with android system and is not easy
>> to reproduce. During test controller works at HS device mode with host
>> connected.
>>
>> The test is based on continuous disabling/enabling USB device function
>> what cause continuous setting DEVDS/DEVEN bit in USB_CONF register.
>>
>> For testing was used composite device consisting from ADP and RNDIS
>> function.
>>
>> Presumably the problem was caused by DMA transfer made after setting
>> DEVDS bit. To resolve this issue fix stops all DMA transfer before
>> setting DEVDS bit.
>>
>> Signed-off-by: Pawel Laszczak <pawell@cadence.com>
>> Signed-off-by: Peter Chan <peter.chan@nxp.com>
>> Reported-by: Peter Chan <peter.chan@nxp.com>
>> Fixes: 7733f6c32e36 ("usb: cdns3: Add Cadence USB3 DRD Driver")
>
>As this is in the 5.4 kernel release, you should have a "Cc: stable..."
>line in here, right?

Ok,

>
>> ---
>>  drivers/usb/cdns3/gadget.c | 84 ++++++++++++++++++++++++++------------
>>  drivers/usb/cdns3/gadget.h |  1 +
>>  2 files changed, 58 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
>> index 4c1e75509303..277ed8484032 100644
>> --- a/drivers/usb/cdns3/gadget.c
>> +++ b/drivers/usb/cdns3/gadget.c
>> @@ -1516,6 +1516,49 @@ static int cdns3_ep_onchip_buffer_reserve(struct =
cdns3_device *priv_dev,
>>  	return 0;
>>  }
>>
>> +static int cdns3_disable_reset_ep(struct cdns3_device *priv_dev,
>> +				  struct cdns3_endpoint *priv_ep)
>> +{
>> +	unsigned long flags;
>> +	u32 val;
>> +	int ret;
>> +
>> +	spin_lock_irqsave(&priv_dev->lock, flags);
>> +
>> +	if (priv_ep->flags & EP_HW_RESETED) {
>
>Is "reseted" a word?  :)
>
>> +		spin_unlock_irqrestore(&priv_dev->lock, flags);
>> +		return 0;
>> +	}
>> +
>> +	cdns3_select_ep(priv_dev, priv_ep->endpoint.desc->bEndpointAddress);
>> +
>> +	val =3D readl(&priv_dev->regs->ep_cfg);
>> +	val &=3D ~EP_CFG_ENABLE;
>> +	writel(val, &priv_dev->regs->ep_cfg);
>> +
>> +	/**
>
>No need for kernel-doc comment style please.
>
>> +	 * Driver needs some time before resetting endpoint.
>> +	 * It need waits for clearing DBUSY bit or for timeout expired.
>> +	 * 10us is enough time for controller to stop transfer.
>> +	 */
>> +	readl_poll_timeout_atomic(&priv_dev->regs->ep_sts, val,
>> +				  !(val & EP_STS_DBUSY), 1, 10);
>
>You don't care if you time out?
I don't need care of it. The next timeout is critical.
>
>> +	writel(EP_CMD_EPRST, &priv_dev->regs->ep_cmd);
>> +
>> +	ret =3D readl_poll_timeout_atomic(&priv_dev->regs->ep_cmd, val,
>> +					!(val & (EP_CMD_CSTALL | EP_CMD_EPRST)),
>> +					1, 1000);
>> +
>> +	if (unlikely(ret))
>
>Unless you can measure the difference of using/not using a
>unlikely/likely mark, NEVER use it.  The compiler and cpu can almost
>always do better than you can, we have the tests to prove it.
>

The both of the above timeout should never occur. If they occurred it would=
 be a=20
critical controller bug. In this case driver can only inform  about this ev=
ent.=20
For timeouts used in driver I've never see an errors. Because debugging the=
se=20
kind of errors is very hard I decided to leave dev_err in such case to info=
rm that
probably something is wrong in HW project.=20

I will remove unlikely. =20

>> +		dev_err(priv_dev->dev, "Timeout: %s resetting failed.\n",
>> +			priv_ep->name);
>> +
>> +	priv_ep->flags |=3D EP_HW_RESETED;
>
>So an error happens, but you still claim the device is reset?  What can
>a user do about this error?

The error should never occur.=20

>
>Yes, I know you copied this from other code in this driver, but it
>doesn't look right to me.

>
>> +	spin_unlock_irqrestore(&priv_dev->lock, flags);
>
>Why print while a spinlock is held?  That's mean, you could have printed
>here, right?
>

Yes, it's true.=20

>> +
>> +	return ret;
>> +}
>> +
>>  void cdns3_configure_dmult(struct cdns3_device *priv_dev,
>>  			   struct cdns3_endpoint *priv_ep)
>>  {
>> @@ -1893,8 +1936,6 @@ static int cdns3_gadget_ep_disable(struct usb_ep *=
ep)
>>  	struct usb_request *request;
>>  	unsigned long flags;
>>  	int ret =3D 0;
>> -	u32 ep_cfg;
>> -	int val;
>>
>>  	if (!ep) {
>>  		pr_err("usbss: invalid parameters\n");
>> @@ -1908,32 +1949,11 @@ static int cdns3_gadget_ep_disable(struct usb_ep=
 *ep)
>>  			  "%s is already disabled\n", priv_ep->name))
>>  		return 0;
>>
>> -	spin_lock_irqsave(&priv_dev->lock, flags);
>> -
>>  	trace_cdns3_gadget_ep_disable(priv_ep);
>>
>> -	cdns3_select_ep(priv_dev, ep->desc->bEndpointAddress);
>> -
>> -	ep_cfg =3D readl(&priv_dev->regs->ep_cfg);
>> -	ep_cfg &=3D ~EP_CFG_ENABLE;
>> -	writel(ep_cfg, &priv_dev->regs->ep_cfg);
>> -
>> -	/**
>> -	 * Driver needs some time before resetting endpoint.
>> -	 * It need waits for clearing DBUSY bit or for timeout expired.
>> -	 * 10us is enough time for controller to stop transfer.
>> -	 */
>> -	readl_poll_timeout_atomic(&priv_dev->regs->ep_sts, val,
>> -				  !(val & EP_STS_DBUSY), 1, 10);
>> -	writel(EP_CMD_EPRST, &priv_dev->regs->ep_cmd);
>> -
>> -	readl_poll_timeout_atomic(&priv_dev->regs->ep_cmd, val,
>> -				  !(val & (EP_CMD_CSTALL | EP_CMD_EPRST)),
>> -				  1, 1000);
>> -	if (unlikely(ret))
>> -		dev_err(priv_dev->dev, "Timeout: %s resetting failed.\n",
>> -			priv_ep->name);
>> +	cdns3_disable_reset_ep(priv_dev, priv_ep);
>>
>> +	spin_lock_irqsave(&priv_dev->lock, flags);
>>  	while (!list_empty(&priv_ep->pending_req_list)) {
>>  		request =3D cdns3_next_request(&priv_ep->pending_req_list);
>>
>> @@ -1962,6 +1982,7 @@ static int cdns3_gadget_ep_disable(struct usb_ep *=
ep)
>>
>>  	ep->desc =3D NULL;
>>  	priv_ep->flags &=3D ~EP_ENABLED;
>> +	priv_ep->flags |=3D EP_CLAIMED | EP_HW_RESETED;
>
>Why do you now set EP_CLAIMED here when you never set that before?  Is
>this a different type of change?

My mistake. Thanks for letting me know. I merged two different patches fixi=
ng the same issue and=20
I missed it.=20

>
>thanks,
>
>greg k-h

Thanks,
Pawell=20