[PATCH] mtd: spi-nor: improve wait-till-ready timeout loop

[PATCH] mtd: spi-nor: improve wait-till-ready timeout loop

[Brian Norris]

There are a few small issues with the timeout loop in
spi_nor_wait_till_ready():

 * The first operation should not be a reschedule; we should check the
   status register at least once to see if we're complete!

 * We should check the status register one last time after declaring the
   deadline has passed, to prevent a premature timeout erro (this is
   theoretically possible if we sleep for a long time after the previous
   status register check).

 * Add an error message, so it's obvious if we ever hit a timeout.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/spi-nor/spi-nor.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 5e3a1d363895..d0dcaa1372ec 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -202,19 +202,24 @@ static int spi_nor_ready(struct spi_nor *nor)
 static int spi_nor_wait_till_ready(struct spi_nor *nor)
 {
 	unsigned long deadline;
-	int ret;
+	int timeout = 0, ret;
 
 	deadline = jiffies + MAX_READY_WAIT_JIFFIES;
 
-	do {
-		cond_resched();
+	while (!timeout) {
+		if (time_after_eq(jiffies, deadline))
+			timeout = 1;
 
 		ret = spi_nor_ready(nor);
 		if (ret < 0)
 			return ret;
 		if (ret)
 			return 0;
-	} while (!time_after_eq(jiffies, deadline));
+
+		cond_resched();
+	}
+
+	dev_err(nor->dev, "flash operation timed out\n");
 
 	return -ETIMEDOUT;
 }
-- 
1.9.1

Re: [PATCH] mtd: spi-nor: improve wait-till-ready timeout loop

[Huang Shijie]

On Wed, Nov 05, 2014 at 02:32:03AM -0800, Brian Norris wrote:
> There are a few small issues with the timeout loop in
> spi_nor_wait_till_ready():
> 
>  * The first operation should not be a reschedule; we should check the
>    status register at least once to see if we're complete!
> 
>  * We should check the status register one last time after declaring the
>    deadline has passed, to prevent a premature timeout erro (this is
>    theoretically possible if we sleep for a long time after the previous
>    status register check).
> 
>  * Add an error message, so it's obvious if we ever hit a timeout.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 5e3a1d363895..d0dcaa1372ec 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -202,19 +202,24 @@ static int spi_nor_ready(struct spi_nor *nor)
>  static int spi_nor_wait_till_ready(struct spi_nor *nor)
>  {
>  	unsigned long deadline;
> -	int ret;
> +	int timeout = 0, ret;
>  
>  	deadline = jiffies + MAX_READY_WAIT_JIFFIES;
>  
> -	do {
> -		cond_resched();
> +	while (!timeout) {
> +		if (time_after_eq(jiffies, deadline))
> +			timeout = 1;
>  
>  		ret = spi_nor_ready(nor);
>  		if (ret < 0)
>  			return ret;
>  		if (ret)
>  			return 0;
> -	} while (!time_after_eq(jiffies, deadline));
> +
> +		cond_resched();
> +	}
> +
> +	dev_err(nor->dev, "flash operation timed out\n");
>  
>  	return -ETIMEDOUT;
>  }
look good.

Acked-by: Huang Shijie <shijie.huang@intel.com>

Re: [PATCH] mtd: spi-nor: improve wait-till-ready timeout loop

[Ezequiel Garcia]

On 11/05/2014 07:32 AM, Brian Norris wrote:
> There are a few small issues with the timeout loop in
> spi_nor_wait_till_ready():
> 
>  * The first operation should not be a reschedule; we should check the
>    status register at least once to see if we're complete!
> 
>  * We should check the status register one last time after declaring the
>    deadline has passed, to prevent a premature timeout erro (this is
>    theoretically possible if we sleep for a long time after the previous
>    status register check).
> 
>  * Add an error message, so it's obvious if we ever hit a timeout.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 5e3a1d363895..d0dcaa1372ec 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -202,19 +202,24 @@ static int spi_nor_ready(struct spi_nor *nor)
>  static int spi_nor_wait_till_ready(struct spi_nor *nor)
>  {
>  	unsigned long deadline;
> -	int ret;
> +	int timeout = 0, ret;
>  
>  	deadline = jiffies + MAX_READY_WAIT_JIFFIES;
>  
> -	do {
> -		cond_resched();
> +	while (!timeout) {
> +		if (time_after_eq(jiffies, deadline))
> +			timeout = 1;
>  
>  		ret = spi_nor_ready(nor);
>  		if (ret < 0)
>  			return ret;
>  		if (ret)
>  			return 0;
> -	} while (!time_after_eq(jiffies, deadline));
> +
> +		cond_resched();
> +	}
> +
> +	dev_err(nor->dev, "flash operation timed out\n");
>  
>  	return -ETIMEDOUT;
>  }
> 

Looks good to me.

Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
-- 
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH] mtd: spi-nor: improve wait-till-ready timeout loop

[Ezequiel Garcia]

On 11/23/2014 06:16 PM, Ezequiel Garcia wrote:
> On 11/05/2014 07:32 AM, Brian Norris wrote:
>> There are a few small issues with the timeout loop in
>> spi_nor_wait_till_ready():
>>
>>  * The first operation should not be a reschedule; we should check the
>>    status register at least once to see if we're complete!
>>
>>  * We should check the status register one last time after declaring the
>>    deadline has passed, to prevent a premature timeout erro (this is
>>    theoretically possible if we sleep for a long time after the previous
>>    status register check).
>>
>>  * Add an error message, so it's obvious if we ever hit a timeout.
>>
>> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
>> ---
>>  drivers/mtd/spi-nor/spi-nor.c | 13 +++++++++----
>>  1 file changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>> index 5e3a1d363895..d0dcaa1372ec 100644
>> --- a/drivers/mtd/spi-nor/spi-nor.c
>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>> @@ -202,19 +202,24 @@ static int spi_nor_ready(struct spi_nor *nor)
>>  static int spi_nor_wait_till_ready(struct spi_nor *nor)
>>  {
>>  	unsigned long deadline;
>> -	int ret;
>> +	int timeout = 0, ret;
>>  
>>  	deadline = jiffies + MAX_READY_WAIT_JIFFIES;
>>  
>> -	do {
>> -		cond_resched();
>> +	while (!timeout) {
>> +		if (time_after_eq(jiffies, deadline))
>> +			timeout = 1;
>>  
>>  		ret = spi_nor_ready(nor);
>>  		if (ret < 0)
>>  			return ret;
>>  		if (ret)
>>  			return 0;
>> -	} while (!time_after_eq(jiffies, deadline));
>> +
>> +		cond_resched();
>> +	}
>> +
>> +	dev_err(nor->dev, "flash operation timed out\n");
>>  
>>  	return -ETIMEDOUT;
>>  }
>>
> 
> Looks good to me.
> 
> Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> 

On a second look... I find more readable to use a bool type for
booleans, but it's just a silly nitpick.

-- 
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH] mtd: spi-nor: improve wait-till-ready timeout loop

[Brian Norris]

On Wed, Nov 05, 2014 at 02:32:03AM -0800, Brian Norris wrote:
> There are a few small issues with the timeout loop in
> spi_nor_wait_till_ready():
> 
>  * The first operation should not be a reschedule; we should check the
>    status register at least once to see if we're complete!
> 
>  * We should check the status register one last time after declaring the
>    deadline has passed, to prevent a premature timeout erro (this is
>    theoretically possible if we sleep for a long time after the previous
>    status register check).
> 
>  * Add an error message, so it's obvious if we ever hit a timeout.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>

Applied to l2-mtd.git.

Brian