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 | expand |
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); > } > > /* >
----- 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 >
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); } /*