mtd: spi-nor: Check that BP bits are set properly

Message ID 763349437.151011.1509293502020.JavaMail.zimbra@xes-inc.com
State Changes Requested
Delegated to: Cyrille Pitchen
Headers show
Series
  • mtd: spi-nor: Check that BP bits are set properly
Related show

Commit Message

Aaron Sierra Oct. 29, 2017, 4:11 p.m.
Previously, the lock and unlock functions returned success even if the
BP bits were not actually updated in the status register due to
hardware write protection. Introduce write_sr_and_check() to write and
read back the status register to ensure the desired BP bits are
actually set as requested.

Signed-off-by: Joe Schultz <jschultz@xes-inc.com>
Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
---
 drivers/mtd/spi-nor/spi-nor.c | 39 +++++++++++++++++++++++++++------------
 1 file changed, 27 insertions(+), 12 deletions(-)

Comments

Cyrille Pitchen Nov. 30, 2017, 1:57 p.m. | #1
Hi Aaron,

Le 29/10/2017 à 17:11, Aaron Sierra a écrit :
> Previously, the lock and unlock functions returned success even if the
> BP bits were not actually updated in the status register due to
> hardware write protection. Introduce write_sr_and_check() to write and
> read back the status register to ensure the desired BP bits are
> actually set as requested.
> 
> Signed-off-by: Joe Schultz <jschultz@xes-inc.com>
> Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 39 +++++++++++++++++++++++++++------------
>  1 file changed, 27 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 19c00072..8a87bd1 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -550,6 +550,31 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
>  	return ret;
>  }
>  
> +/* Write status register and ensure bits in mask match written values */
> +static int write_sr_and_check(struct spi_nor *nor, u8 new, u8 mask)

Maybe keep the previous name 'status_new' to be consistent with stm_lock()
and stm_unlock() and also because 'new' is a C++ reserved keyword so it might
be better to avoid using it to name some variable (editor syntax coloring issue
for instance).

> +{
> +	int ret;
> +
> +	ret = spi_nor_wait_till_ready(nor);
> +	if (ret)
> +		return ret;
> +

Why do you poll the status register here ?
Based on the source code before your patch, it should not be needed.
Do you think it is ?

Otherwise, your patch looks good to me :)

Best regards,

Cyrille

> +	write_enable(nor);
> +	ret = write_sr(nor, new);
> +	if (ret)
> +		return ret;
> +
> +	ret = spi_nor_wait_till_ready(nor);
> +	if (ret)
> +		return ret;
> +
> +	ret = read_sr(nor);
> +	if (ret < 0)
> +		return ret;
> +
> +	return ((ret & mask) != (new & mask)) ? -EIO : 0;
> +}
> +
>  static void stm_get_locked_range(struct spi_nor *nor, u8 sr, loff_t *ofs,
>  				 uint64_t *len)
>  {
> @@ -648,7 +673,6 @@ static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
>  	loff_t lock_len;
>  	bool can_be_top = true, can_be_bottom = nor->flags & SNOR_F_HAS_SR_TB;
>  	bool use_top;
> -	int ret;
>  
>  	status_old = read_sr(nor);
>  	if (status_old < 0)
> @@ -712,11 +736,7 @@ static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
>  	if ((status_new & mask) < (status_old & mask))
>  		return -EINVAL;
>  
> -	write_enable(nor);
> -	ret = write_sr(nor, status_new);
> -	if (ret)
> -		return ret;
> -	return spi_nor_wait_till_ready(nor);
> +	return write_sr_and_check(nor, status_new, mask);
>  }
>  
>  /*
> @@ -733,7 +753,6 @@ static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
>  	loff_t lock_len;
>  	bool can_be_top = true, can_be_bottom = nor->flags & SNOR_F_HAS_SR_TB;
>  	bool use_top;
> -	int ret;
>  
>  	status_old = read_sr(nor);
>  	if (status_old < 0)
> @@ -800,11 +819,7 @@ static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
>  	if ((status_new & mask) > (status_old & mask))
>  		return -EINVAL;
>  
> -	write_enable(nor);
> -	ret = write_sr(nor, status_new);
> -	if (ret)
> -		return ret;
> -	return spi_nor_wait_till_ready(nor);
> +	return write_sr_and_check(nor, status_new, mask);
>  }
>  
>  /*
>
Aaron Sierra Dec. 4, 2017, 4:56 p.m. | #2
----- Original Message -----
> From: "Cyrille Pitchen" <cyrille.pitchen@wedev4u.fr>
> Sent: Thursday, November 30, 2017 7:57:27 AM

Hi Cyrille,

> Hi Aaron,
> 
> Le 29/10/2017 à 17:11, Aaron Sierra a écrit :
>> Previously, the lock and unlock functions returned success even if the
>> BP bits were not actually updated in the status register due to
>> hardware write protection. Introduce write_sr_and_check() to write and
>> read back the status register to ensure the desired BP bits are
>> actually set as requested.
>> 
>> Signed-off-by: Joe Schultz <jschultz@xes-inc.com>
>> Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
>> ---
>>  drivers/mtd/spi-nor/spi-nor.c | 39 +++++++++++++++++++++++++++------------
>>  1 file changed, 27 insertions(+), 12 deletions(-)
>> 
>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>> index 19c00072..8a87bd1 100644
>> --- a/drivers/mtd/spi-nor/spi-nor.c
>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>> @@ -550,6 +550,31 @@ static int spi_nor_erase(struct mtd_info *mtd, struct
>> erase_info *instr)
>>  	return ret;
>>  }
>>  
>> +/* Write status register and ensure bits in mask match written values */
>> +static int write_sr_and_check(struct spi_nor *nor, u8 new, u8 mask)
> 
> Maybe keep the previous name 'status_new' to be consistent with stm_lock()
> and stm_unlock() and also because 'new' is a C++ reserved keyword so it might
> be better to avoid using it to name some variable (editor syntax coloring issue
> for instance).

Sure, both of those reasons seem reasonable.
 
>> +{
>> +	int ret;
>> +
>> +	ret = spi_nor_wait_till_ready(nor);
>> +	if (ret)
>> +		return ret;
>> +
> 
> Why do you poll the status register here ?
> Based on the source code before your patch, it should not be needed.
> Do you think it is ?

This polling was due to an excess of caution. I've reviewed the preceding
code and agree that this can be removed from the next version of this patch.

> Otherwise, your patch looks good to me :)

Thanks for your review!

-Aaron S.

> Best regards,
> 
> Cyrille
>

Patch

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 19c00072..8a87bd1 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -550,6 +550,31 @@  static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
 	return ret;
 }
 
+/* Write status register and ensure bits in mask match written values */
+static int write_sr_and_check(struct spi_nor *nor, u8 new, u8 mask)
+{
+	int ret;
+
+	ret = spi_nor_wait_till_ready(nor);
+	if (ret)
+		return ret;
+
+	write_enable(nor);
+	ret = write_sr(nor, new);
+	if (ret)
+		return ret;
+
+	ret = spi_nor_wait_till_ready(nor);
+	if (ret)
+		return ret;
+
+	ret = read_sr(nor);
+	if (ret < 0)
+		return ret;
+
+	return ((ret & mask) != (new & mask)) ? -EIO : 0;
+}
+
 static void stm_get_locked_range(struct spi_nor *nor, u8 sr, loff_t *ofs,
 				 uint64_t *len)
 {
@@ -648,7 +673,6 @@  static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 	loff_t lock_len;
 	bool can_be_top = true, can_be_bottom = nor->flags & SNOR_F_HAS_SR_TB;
 	bool use_top;
-	int ret;
 
 	status_old = read_sr(nor);
 	if (status_old < 0)
@@ -712,11 +736,7 @@  static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 	if ((status_new & mask) < (status_old & mask))
 		return -EINVAL;
 
-	write_enable(nor);
-	ret = write_sr(nor, status_new);
-	if (ret)
-		return ret;
-	return spi_nor_wait_till_ready(nor);
+	return write_sr_and_check(nor, status_new, mask);
 }
 
 /*
@@ -733,7 +753,6 @@  static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 	loff_t lock_len;
 	bool can_be_top = true, can_be_bottom = nor->flags & SNOR_F_HAS_SR_TB;
 	bool use_top;
-	int ret;
 
 	status_old = read_sr(nor);
 	if (status_old < 0)
@@ -800,11 +819,7 @@  static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 	if ((status_new & mask) > (status_old & mask))
 		return -EINVAL;
 
-	write_enable(nor);
-	ret = write_sr(nor, status_new);
-	if (ret)
-		return ret;
-	return spi_nor_wait_till_ready(nor);
+	return write_sr_and_check(nor, status_new, mask);
 }
 
 /*