[PATCH] mtd: spi-nor: Check the return value from read_sr()

[PATCH] mtd: spi-nor: Check the return value from read_sr()

[Fabio Estevam]

We should better check the return value from read_sr() and
propagate it in the case of error.

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
 drivers/mtd/spi-nor/spi-nor.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 12041e1..4d858f7 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -459,11 +459,14 @@ static int stm_is_locked_sr(struct spi_nor *nor, loff_t ofs, uint64_t len,
 static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 {
 	struct mtd_info *mtd = &nor->mtd;
-	u8 status_old, status_new;
+	u8 status_new;
 	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
 	u8 shift = ffs(mask) - 1, pow, val;
+	int status_old;
 
 	status_old = read_sr(nor);
+	if (status_old < 0)
+		return status_old;
 
 	/* SPI NOR always locks to the end */
 	if (ofs + len != mtd->size) {
@@ -509,11 +512,14 @@ static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 {
 	struct mtd_info *mtd = &nor->mtd;
-	uint8_t status_old, status_new;
+	uint8_t status_new;
+	int status_old;
 	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
 	u8 shift = ffs(mask) - 1, pow, val;
 
 	status_old = read_sr(nor);
+	if (status_old < 0)
+		return status_old;
 
 	/* Cannot unlock; would unlock larger region than requested */
 	if (stm_is_locked_sr(nor, status_old, ofs - mtd->erasesize,
@@ -1013,6 +1019,8 @@ static int macronix_quad_enable(struct spi_nor *nor)
 	int ret, val;
 
 	val = read_sr(nor);
+	if (val < 0)
+		return val;
 	write_enable(nor);
 
 	write_sr(nor, val | SR_QUAD_EN_MX);
-- 
1.9.1

Re: [PATCH] mtd: spi-nor: Check the return value from read_sr()

[Brian Norris]

On Tue, Nov 17, 2015 at 12:50:24PM -0200, Fabio Estevam wrote:
> We should better check the return value from read_sr() and
> propagate it in the case of error.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 12041e1..4d858f7 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -459,11 +459,14 @@ static int stm_is_locked_sr(struct spi_nor *nor, loff_t ofs, uint64_t len,
>  static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
>  {
>  	struct mtd_info *mtd = &nor->mtd;
> -	u8 status_old, status_new;
> +	u8 status_new;
>  	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
>  	u8 shift = ffs(mask) - 1, pow, val;
> +	int status_old;

Hmm, by changing status_old from u8 to int, do we propagate any
sign-extension issues in this function? What if BIT(7) is set? If I
recall my C promotion/extension rules correctly, then I guess it's safe
(we get zero-extension, not sign-extension), but I wouldn't always put
my faith in my memory...

>  
>  	status_old = read_sr(nor);
> +	if (status_old < 0)
> +		return status_old;
>  
>  	/* SPI NOR always locks to the end */
>  	if (ofs + len != mtd->size) {
> @@ -509,11 +512,14 @@ static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
>  static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
>  {
>  	struct mtd_info *mtd = &nor->mtd;
> -	uint8_t status_old, status_new;
> +	uint8_t status_new;
> +	int status_old;

Same here.

>  	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
>  	u8 shift = ffs(mask) - 1, pow, val;
>  
>  	status_old = read_sr(nor);
> +	if (status_old < 0)
> +		return status_old;
>  
>  	/* Cannot unlock; would unlock larger region than requested */
>  	if (stm_is_locked_sr(nor, status_old, ofs - mtd->erasesize,
> @@ -1013,6 +1019,8 @@ static int macronix_quad_enable(struct spi_nor *nor)
>  	int ret, val;
>  
>  	val = read_sr(nor);
> +	if (val < 0)
> +		return val;
>  	write_enable(nor);
>  
>  	write_sr(nor, val | SR_QUAD_EN_MX);

If we wanted to sidestep the issue, we could still keep status_old as
u8, but just use an 'int ret' as an intermediate place-holder, so we can
do signed error checking.

Brian

Re: [PATCH] mtd: spi-nor: Check the return value from read_sr()

[Fabio Estevam]

On Thu, Nov 19, 2015 at 7:54 PM, Brian Norris
<computersforpeace@gmail.com> wrote:

> Hmm, by changing status_old from u8 to int, do we propagate any
> sign-extension issues in this function? What if BIT(7) is set? If I

If the value returned by read_sr() has BIT(7) set then it is not an
error, right?

read_sr() can return negative on error and u8 on success.

Not sure I understand your concern.

Re: [PATCH] mtd: spi-nor: Check the return value from read_sr()

[Brian Norris]

On Thu, Nov 19, 2015 at 08:48:23PM -0200, Fabio Estevam wrote:
> On Thu, Nov 19, 2015 at 7:54 PM, Brian Norris
> <computersforpeace@gmail.com> wrote:
> 
> > Hmm, by changing status_old from u8 to int, do we propagate any
> > sign-extension issues in this function? What if BIT(7) is set? If I
> 
> If the value returned by read_sr() has BIT(7) set then it is not an
> error, right?
> 
> read_sr() can return negative on error and u8 on success.
> 
> Not sure I understand your concern.

I guess my comment was based on my misplaced uncertainty on whether u8
would sign-extend or zero-extend when converted to int. It appears the
standard says that unsigned values get zero-extended. i.e.:

	u8 val = BIT(7); // 0x80

gets zero-extended to:

	return val; // that is, 0x00000080, not 0xffffff80

If we stick with u8, then it doesn't really matter which one it is.

The only remaining concern is over the mild awkwardness of comparing and
manipulating status_old and status_new, where one is int and the other
is u8. But I guess there are no issues yet, AFAICT.

Brian

Re: [PATCH] mtd: spi-nor: Check the return value from read_sr()

[Fabio Estevam]

On Thu, Nov 19, 2015 at 8:58 PM, Brian Norris
<computersforpeace@gmail.com> wrote:

> The only remaining concern is over the mild awkwardness of comparing and
> manipulating status_old and status_new, where one is int and the other
> is u8. But I guess there are no issues yet, AFAICT.

Good point. I will change status_old and status_new to int in v2, thanks.