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

[Eva Rachel Retuya <eraretuya@gmail.com>]

On Mon, Nov 02, 2015 at 09:12:10PM +0100, Arnd Bergmann wrote:
> 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
>

I'm sorry for the late response!

The change from udelay to usleep is a safe conversion because the
spinand_reset() calls spinand_cmd(). The return value of the latter
depends on the return value of spi_sync() which is a function that is
allowed to sleep therefore, I have confirmed that spinand_reset() is
not in atomic context.

I hope I got it right this time. Thank you for bringing this up.
Eva