[PATCH] Staging: mt29f_spinand: Replace udelay function with usleep_range

[PATCH] Staging: mt29f_spinand: Replace udelay function with usleep_range

[Eva Rachel Retuya]

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

Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>
---
 drivers/staging/mt29f_spinand/mt29f_spinand.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/mt29f_spinand/mt29f_spinand.c b/drivers/staging/mt29f_spinand/mt29f_spinand.c
index a8292a1..b6a1ffc 100644
--- a/drivers/staging/mt29f_spinand/mt29f_spinand.c
+++ b/drivers/staging/mt29f_spinand/mt29f_spinand.c
@@ -736,7 +736,7 @@ static void spinand_reset(struct spi_device *spi_nand)
 		pr_info("spinand reset failed!\n");
 
 	/* elapse 1ms before issuing any other command */
-	udelay(1000);
+	usleep_range(1000, 2000);
 
 	if (wait_till_ready(spi_nand))
 		dev_err(&spi_nand->dev, "wait timedout!\n");
@@ -802,7 +802,7 @@ static void spinand_cmdfunc(struct mtd_info *mtd, unsigned int command,
 		if (wait_till_ready(info->spi))
 			dev_err(&info->spi->dev, "WAIT timedout!!!\n");
 		/* a minimum of 250us must elapse before issuing RESET cmd*/
-		udelay(250);
+		usleep_range(250, 1000);
 		spinand_reset(info->spi);
 		break;
 	default:
-- 
1.9.1

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

[Arnd Bergmann]

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?

	Arnd

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

[Eva Rachel Retuya]

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?
> 
> 	Arnd

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.

Do you think it operates in atomic context?

Eva

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

[Arnd Bergmann]

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

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

[Eva Rachel Retuya]

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

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

[Arnd Bergmann]

On Friday 06 November 2015 02:19:59 Eva Rachel Retuya wrote:
> 
> 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.
> 

Yes, very good. FWIW, the wait_till_ready() function also sleeps
when it calls cond_resched().

	Arnd