linux-kernel - Re: [PATCH] usb: cdns3: Fix: ARM core hang after connect/disconnect operation.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 9 Jan 2020 07:38:41 +0100
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Pawel Laszczak <pawell@...ence.com>
Cc:     "felipe.balbi@...ux.intel.com" <felipe.balbi@...ux.intel.com>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "rogerq@...com" <rogerq@...com>,
        "jbergsagel@...com" <jbergsagel@...com>,
        "nsekhar@...com" <nsekhar@...com>, "nm@...com" <nm@...com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Jayshri Dajiram Pawar <jpawar@...ence.com>,
        Rahul Kumar <kurahul@...ence.com>,
        Sanket Parmar <sparmar@...ence.com>,
        Peter Chan <peter.chan@....com>
Subject: Re: [PATCH] usb: cdns3: Fix: ARM core hang after connect/disconnect
 operation.

On Thu, Jan 09, 2020 at 06:27:02AM +0000, Pawel Laszczak wrote:
> >> +	writel(EP_CMD_EPRST, &priv_dev->regs->ep_cmd);
> >> +
> >> +	ret = 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 
> critical controller bug. In this case driver can only inform  about this event. 

"Should never occur" is a fun thing to say :)

If it can never occur, then don't even check for it.

If it can, then check for it and handle it properly.

What about this controller in systems with removable busses (like PCI?)
What happens then (hint, I bet this could occur...)

> For timeouts used in driver I've never see an errors. Because debugging these 
> kind of errors is very hard I decided to leave dev_err in such case to inform that
> probably something is wrong in HW project. 
> 
> I will remove unlikely.  
> 
> >> +		dev_err(priv_dev->dev, "Timeout: %s resetting failed.\n",
> >> +			priv_ep->name);
> >> +
> >> +	priv_ep->flags |= 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. 

Then no need to warn anyone, just wait and move on.

Or properly handle it.

thanks,

greg k-h