Re: [PATCH 2/3] mtd: mediatek: driver for MTK Smart Device Gen1 NAND

[Brian Norris <computersforpeace@gmail.com>]

Hi Jorge,

On Tue, Mar 08, 2016 at 03:57:44PM -0500, Jorge Ramirez-Ortiz wrote:
> On 03/08/2016 03:20 PM, Brian Norris wrote:
> >> > If you feel strongly about it I don't mind adding an additional check after any
> >> > form of sleep (not so sure about adding it after a cpu_relax) but I don't think
> >> > it is needed.
> > It is non-negotiable that your timeout loops must be logically correct.
> > That is, you must recheck the exit condition before you declare a
> > timeout.
> 
> Hi Brian,
> 
> My point was that the current timeout loops (except one which is just
> implementing its own version of readx_poll_timeout) are logically correct as
> they are since they are not involving the scheduler: so doing the additional
> check after cpu_relax() is unnecessary - cpu_relax is a dmb instruction.

No, they're not correct just because you're not invoking the scheduler
directly. What about CONFIG_PREEMPT?

> > If you just follow Boris's suggestion of using the helper macros, then
> > you'll be fine.
> 
> I am sorry (not trying to be difficult here) but relaxed_poll_timeout calls
> usleep_range and involving the scheduler brings in a level of undeterminism (so
> we could have slept for 100 useconds or 1000)
> am I wrong? is under that case that we need to check after exiting the loop.

Just bound sleep_us to the largest amount of nondeterminism you can
accept. And that number can be 0.

> a different discussion is if using cpu_relax (busy loop) at all is a good idea:
> the way I see it, that should depend on the case but I suppose a silver bullet
> solution via the helper macros is ok too - and certainly more readable and
> easier to maintain - so will do as you suggest (correct all loops).

I'm not specifically saying you must use those helpers, but you have to
get your timeout loops correct. And given the confusion so far, it looks
like it's reasonable to assume we'd get fewer bugs if you used the
helpers...

Brian