Re: [Outreachy kernel] [PATCH] Staging: mt29f_spinand: Replace udelay function with usleep_range

[Arnd Bergmann <arnd@arndb.de>]

On Saturday 31 October 2015 16:53:31 Eva Rachel Retuya wrote:
> On Fri, Oct 30, 2015 at 10:32:40PM +0100, Arnd Bergmann wrote:
> > On Sunday 25 October 2015 01:24:51 Eva Rachel Retuya wrote:
> > > Use 'usleep_range' instead of 'udelay' to elapse time. For
> > > spinand_reset, define the upper limit by a factor of 2 to keep the wait
> > > short while still allowing a "good enough" range for wakeup. Define the
> > > range 250us - 1ms for spinand_cmdfunc to provide enough leeway before
> > > issuing spinand_reset. Checkpatch found this issue.
> > > 
> > > CHECK: usleep_range is preferred over udelay; see
> > > Documentation/timers/timers-howto.txt
> > 
> > You are missing here an explanation about how you verified that the
> > conversion is safe: Are you sure that the spinand_reset() function
> > is never called from "atomic" context?
> 
> Please correct me if I'm wrong. I look up similar patches before this
> and scanned the code taking into consideration what was discussed. In
> this case, I didn't see any indication that it is not allowed to sleep
> and also by looking at the probe function containing
> "devm_kzalloc(..., GFP_KERNEL). The caller/s of the reset function does
> not indicate whether it is on "lock status" and from my analysis, the
> use of "udelay()" simply wants to elapse time before proceeding.

Your analysis is a bit flawed, because you look at the probe function
that uses a pointer to spinand_reset(), and the probe function can clearly
sleep as you describe here, but it is not obvious whether the callers
of the 'cmdfunc' pointers are allowed to sleep as well.
 
> Do you think it operates in atomic context?

I was commenting on the fact that your changelog text did not describe
whether you checked at all, or how you came to the conclusion that it
was safe.

I have checked the code myself now, and found that the function already
sleeps, so it is safe to add other sleeping function calls, and the
change in your patch is good.

This is important to get right, especially when you modify someone
else's code, because whether code is called in atomic context or not
is fundamental knowledge in the kernel, and getting it wrong can
result in bugs that are hard to find.

Can you try again to figure out why this is a safe conversion?

	Arnd