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

[Greg KH]

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