> >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. Yes, on existing platforms it can never occur. > >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...) It's good question. Nobody from our customer currently use such system. The only platform with PCI is used by me for testing purpose. I will talk with HW team about such cases. Maybe in the feature such improvement will be necessary. It's important fix for our customer so I will delete this checking for now. > >> 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 Thanks, Pawell
