This series is the follow up on the single patch
mtd: spi-nor: implement OTP erase for Winbond and similar flashes
Pratyush Yadav discovered a likely problem with bigger flashes, the address
to access the security registers is either 3 or 4 byte (at least for
winbond flashes).
Changes since v4:
- add new patch to get rid taking the spi lock if length is zero. Please
note, that I didn't squash this into the "Fixes:" patch because it is
unrelated to the actual bug.
- add comment which explains that we could also branch on an error in
spi_nor_mtd_otp_range_is_locked()
- check zero length in spi_nor_mtd_otp_erase() and return early before
taking the lock
Changes since v3:
- new patch to check for read-only OTP regions before writing
- clarify term "security register"
- don't combine lock and erase functions anymore. there are now
more difference than similarities.
Changes since v2:
- fix 3/4 byte mode access
- use spi_nor_erase_sector() by swapping the nor->erase_opcode
- use more consistent wording regarding the security registers
Changes since v1:
- fixed kernel doc
Michael Walle (5):
mtd: spi-nor: otp: fix access to security registers in 4 byte mode
mtd: spi-nor: otp: use more consistent wording
mtd: spi-nor: otp: return -EROFS if region is read-only
mtd: spi-nor: otp: simplify length check
mtd: spi-nor: otp: implement erase for Winbond and similar flashes
drivers/mtd/spi-nor/core.c | 2 +-
drivers/mtd/spi-nor/core.h | 4 +
drivers/mtd/spi-nor/otp.c | 156 +++++++++++++++++++++++++++++++---
drivers/mtd/spi-nor/winbond.c | 1 +
4 files changed, 149 insertions(+), 14 deletions(-)
--
2.20.1
By moving the code around a bit, we can just check the length before
calling spi_nor_mtd_otp_range_is_locked() and drop the length check
there. This way we don't need to take the lock. This will also skip the
"*retlen = 0" assignment if the length is zero. But mtdcore already does
that for us. Thus we can drop that, too.
Signed-off-by: Michael Walle <[email protected]>
---
drivers/mtd/spi-nor/otp.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
index 063f8fb68649..89fe52e3851a 100644
--- a/drivers/mtd/spi-nor/otp.c
+++ b/drivers/mtd/spi-nor/otp.c
@@ -256,9 +256,6 @@ static int spi_nor_mtd_otp_range_is_locked(struct spi_nor *nor, loff_t ofs,
unsigned int region;
int locked;
- if (!len)
- return 0;
-
/*
* If any of the affected OTP regions are locked the entire range is
* considered locked.
@@ -290,13 +287,16 @@ static int spi_nor_mtd_otp_read_write(struct mtd_info *mtd, loff_t ofs,
if (ofs < 0 || ofs >= spi_nor_otp_size(nor))
return 0;
+ /* don't access beyond the end */
+ total_len = min_t(size_t, total_len, spi_nor_otp_size(nor) - ofs);
+
+ if (!total_len)
+ return 0;
+
ret = spi_nor_lock_and_prep(nor);
if (ret)
return ret;
- /* don't access beyond the end */
- total_len = min_t(size_t, total_len, spi_nor_otp_size(nor) - ofs);
-
if (is_write) {
ret = spi_nor_mtd_otp_range_is_locked(nor, ofs, total_len);
if (ret < 0) {
@@ -307,7 +307,6 @@ static int spi_nor_mtd_otp_read_write(struct mtd_info *mtd, loff_t ofs,
}
}
- *retlen = 0;
while (total_len) {
/*
* The OTP regions are mapped into a contiguous area starting
--
2.20.1
The security registers either take a 3 byte or a 4 byte address offset,
depending on the address mode of the flash. Thus just leave the
nor->addr_width as is.
Fixes: cad3193fe9d1 ("mtd: spi-nor: implement OTP support for Winbond and similar flashes")
Signed-off-by: Michael Walle <[email protected]>
Acked-by: Pratyush Yadav <[email protected]>
Reviewed-by: Tudor Ambarus <[email protected]>
---
drivers/mtd/spi-nor/otp.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
index 61036c716abb..91a4c510ed51 100644
--- a/drivers/mtd/spi-nor/otp.c
+++ b/drivers/mtd/spi-nor/otp.c
@@ -40,7 +40,6 @@ int spi_nor_otp_read_secr(struct spi_nor *nor, loff_t addr, size_t len, u8 *buf)
rdesc = nor->dirmap.rdesc;
nor->read_opcode = SPINOR_OP_RSECR;
- nor->addr_width = 3;
nor->read_dummy = 8;
nor->read_proto = SNOR_PROTO_1_1_1;
nor->dirmap.rdesc = NULL;
@@ -84,7 +83,6 @@ int spi_nor_otp_write_secr(struct spi_nor *nor, loff_t addr, size_t len,
wdesc = nor->dirmap.wdesc;
nor->program_opcode = SPINOR_OP_PSECR;
- nor->addr_width = 3;
nor->write_proto = SNOR_PROTO_1_1_1;
nor->dirmap.wdesc = NULL;
--
2.20.1
Winbond flashes with OTP support provide a command to erase the OTP
data. This might come in handy during development.
This was tested with a Winbond W25Q32JW on a LS1028A SoC with the
NXP FSPI controller.
Signed-off-by: Michael Walle <[email protected]>
Reviewed-by: Pratyush Yadav <[email protected]>
---
drivers/mtd/spi-nor/core.c | 2 +-
drivers/mtd/spi-nor/core.h | 4 ++
drivers/mtd/spi-nor/otp.c | 86 +++++++++++++++++++++++++++++++++++
drivers/mtd/spi-nor/winbond.c | 1 +
4 files changed, 92 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index f6a6ef2d8bd8..a21b0085de05 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -1318,7 +1318,7 @@ static u32 spi_nor_convert_addr(struct spi_nor *nor, loff_t addr)
/*
* Initiate the erasure of a single sector
*/
-static int spi_nor_erase_sector(struct spi_nor *nor, u32 addr)
+int spi_nor_erase_sector(struct spi_nor *nor, u32 addr)
{
int i;
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index 28a2e0be97a3..9398a8738857 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -207,6 +207,7 @@ struct spi_nor_otp_organization {
* @read: read from the SPI NOR OTP area.
* @write: write to the SPI NOR OTP area.
* @lock: lock an OTP region.
+ * @erase: erase an OTP region.
* @is_locked: check if an OTP region of the SPI NOR is locked.
*/
struct spi_nor_otp_ops {
@@ -214,6 +215,7 @@ struct spi_nor_otp_ops {
int (*write)(struct spi_nor *nor, loff_t addr, size_t len,
const u8 *buf);
int (*lock)(struct spi_nor *nor, unsigned int region);
+ int (*erase)(struct spi_nor *nor, loff_t addr);
int (*is_locked)(struct spi_nor *nor, unsigned int region);
};
@@ -503,10 +505,12 @@ ssize_t spi_nor_read_data(struct spi_nor *nor, loff_t from, size_t len,
u8 *buf);
ssize_t spi_nor_write_data(struct spi_nor *nor, loff_t to, size_t len,
const u8 *buf);
+int spi_nor_erase_sector(struct spi_nor *nor, u32 addr);
int spi_nor_otp_read_secr(struct spi_nor *nor, loff_t addr, size_t len, u8 *buf);
int spi_nor_otp_write_secr(struct spi_nor *nor, loff_t addr, size_t len,
const u8 *buf);
+int spi_nor_otp_erase_secr(struct spi_nor *nor, loff_t addr);
int spi_nor_otp_lock_sr2(struct spi_nor *nor, unsigned int region);
int spi_nor_otp_is_locked_sr2(struct spi_nor *nor, unsigned int region);
diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
index 89fe52e3851a..983e40b19134 100644
--- a/drivers/mtd/spi-nor/otp.c
+++ b/drivers/mtd/spi-nor/otp.c
@@ -120,6 +120,38 @@ int spi_nor_otp_write_secr(struct spi_nor *nor, loff_t addr, size_t len,
return ret ?: written;
}
+/**
+ * spi_nor_otp_erase_secr() - erase a security register
+ * @nor: pointer to 'struct spi_nor'
+ * @addr: offset of the security register to be erased
+ *
+ * Erase a security register by using the SPINOR_OP_ESECR command.
+ *
+ * For more information on the term "security register", see the documentation
+ * of spi_nor_otp_read_secr().
+ *
+ * This method is used on GigaDevice and Winbond flashes.
+ *
+ * Return: 0 on success, -errno otherwise
+ */
+int spi_nor_otp_erase_secr(struct spi_nor *nor, loff_t addr)
+{
+ u8 erase_opcode = nor->erase_opcode;
+ int ret;
+
+ ret = spi_nor_write_enable(nor);
+ if (ret)
+ return ret;
+
+ nor->erase_opcode = SPINOR_OP_ESECR;
+ ret = spi_nor_erase_sector(nor, addr);
+ nor->erase_opcode = erase_opcode;
+ if (ret)
+ return ret;
+
+ return spi_nor_wait_till_ready(nor);
+}
+
static int spi_nor_otp_lock_bit_cr(unsigned int region)
{
static const int lock_bits[] = { SR2_LB1, SR2_LB2, SR2_LB3 };
@@ -360,6 +392,59 @@ static int spi_nor_mtd_otp_write(struct mtd_info *mtd, loff_t to, size_t len,
return spi_nor_mtd_otp_read_write(mtd, to, len, retlen, buf, true);
}
+static int spi_nor_mtd_otp_erase(struct mtd_info *mtd, loff_t from, size_t len)
+{
+ struct spi_nor *nor = mtd_to_spi_nor(mtd);
+ const struct spi_nor_otp_ops *ops = nor->params->otp.ops;
+ const size_t rlen = spi_nor_otp_region_len(nor);
+ unsigned int region;
+ loff_t rstart;
+ int ret;
+
+ /* OTP erase is optional */
+ if (!ops->erase)
+ return -EOPNOTSUPP;
+
+ if (!len)
+ return 0;
+
+ if (from < 0 || (from + len) > spi_nor_otp_size(nor))
+ return -EINVAL;
+
+ /* the user has to explicitly ask for whole regions */
+ if (!IS_ALIGNED(len, rlen) || !IS_ALIGNED(from, rlen))
+ return -EINVAL;
+
+ ret = spi_nor_lock_and_prep(nor);
+ if (ret)
+ return ret;
+
+ ret = spi_nor_mtd_otp_range_is_locked(nor, from, len);
+ if (ret < 0) {
+ goto out;
+ } else if (ret) {
+ ret = -EROFS;
+ goto out;
+ }
+
+ while (len) {
+ region = spi_nor_otp_offset_to_region(nor, from);
+ rstart = spi_nor_otp_region_start(nor, region);
+
+ ret = ops->erase(nor, rstart);
+ if (ret)
+ goto out;
+
+ len -= rlen;
+ from += rlen;
+ }
+
+out:
+ spi_nor_unlock_and_unprep(nor);
+
+ return ret;
+}
+
static int spi_nor_mtd_otp_lock(struct mtd_info *mtd, loff_t from, size_t len)
{
struct spi_nor *nor = mtd_to_spi_nor(mtd);
@@ -418,4 +503,5 @@ void spi_nor_otp_init(struct spi_nor *nor)
mtd->_read_user_prot_reg = spi_nor_mtd_otp_read;
mtd->_write_user_prot_reg = spi_nor_mtd_otp_write;
mtd->_lock_user_prot_reg = spi_nor_mtd_otp_lock;
+ mtd->_erase_user_prot_reg = spi_nor_mtd_otp_erase;
}
diff --git a/drivers/mtd/spi-nor/winbond.c b/drivers/mtd/spi-nor/winbond.c
index 9a81c67a60c6..96573f61caf5 100644
--- a/drivers/mtd/spi-nor/winbond.c
+++ b/drivers/mtd/spi-nor/winbond.c
@@ -139,6 +139,7 @@ static int winbond_set_4byte_addr_mode(struct spi_nor *nor, bool enable)
static const struct spi_nor_otp_ops winbond_otp_ops = {
.read = spi_nor_otp_read_secr,
.write = spi_nor_otp_write_secr,
+ .erase = spi_nor_otp_erase_secr,
.lock = spi_nor_otp_lock_sr2,
.is_locked = spi_nor_otp_is_locked_sr2,
};
--
2.20.1
SPI NOR flashes will just ignore program commands if the OTP region is
locked. Thus, a user might not notice that the intended write didn't end
up in the flash. Return -EROFS to the user in this case. From what I can
tell, chips/cfi_cmdset_0001.c also return this error code.
One could optimize spi_nor_mtd_otp_range_is_locked() to read the status
register only once and not for every OTP region, but for that we would
need some more invasive changes. Given that this is
one-time-programmable memory and the normal access mode is reading, we
just live with the small overhead.
Fixes: 069089acf88b ("mtd: spi-nor: add OTP support")
Signed-off-by: Michael Walle <[email protected]>
Reviewed-by: Pratyush Yadav <[email protected]>
---
drivers/mtd/spi-nor/otp.c | 36 ++++++++++++++++++++++++++++++++++++
1 file changed, 36 insertions(+)
diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
index 3898ed67ba1c..063f8fb68649 100644
--- a/drivers/mtd/spi-nor/otp.c
+++ b/drivers/mtd/spi-nor/otp.c
@@ -249,6 +249,32 @@ static int spi_nor_mtd_otp_info(struct mtd_info *mtd, size_t len,
return ret;
}
+static int spi_nor_mtd_otp_range_is_locked(struct spi_nor *nor, loff_t ofs,
+ size_t len)
+{
+ const struct spi_nor_otp_ops *ops = nor->params->otp.ops;
+ unsigned int region;
+ int locked;
+
+ if (!len)
+ return 0;
+
+ /*
+ * If any of the affected OTP regions are locked the entire range is
+ * considered locked.
+ */
+ for (region = spi_nor_otp_offset_to_region(nor, ofs);
+ region <= spi_nor_otp_offset_to_region(nor, ofs + len - 1);
+ region++) {
+ locked = ops->is_locked(nor, region);
+ /* take the branch it is locked or in case of an error */
+ if (locked)
+ return locked;
+ }
+
+ return 0;
+}
+
static int spi_nor_mtd_otp_read_write(struct mtd_info *mtd, loff_t ofs,
size_t total_len, size_t *retlen,
const u8 *buf, bool is_write)
@@ -271,6 +297,16 @@ static int spi_nor_mtd_otp_read_write(struct mtd_info *mtd, loff_t ofs,
/* don't access beyond the end */
total_len = min_t(size_t, total_len, spi_nor_otp_size(nor) - ofs);
+ if (is_write) {
+ ret = spi_nor_mtd_otp_range_is_locked(nor, ofs, total_len);
+ if (ret < 0) {
+ goto out;
+ } else if (ret) {
+ ret = -EROFS;
+ goto out;
+ }
+ }
+
*retlen = 0;
while (total_len) {
/*
--
2.20.1
On 6/4/21 1:02 PM, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> Winbond flashes with OTP support provide a command to erase the OTP
> data. This might come in handy during development.
>
> This was tested with a Winbond W25Q32JW on a LS1028A SoC with the
> NXP FSPI controller.
>
> Signed-off-by: Michael Walle <[email protected]>
> Reviewed-by: Pratyush Yadav <[email protected]>
Reviewed-by: Tudor Ambarus <[email protected]>
> ---
> drivers/mtd/spi-nor/core.c | 2 +-
> drivers/mtd/spi-nor/core.h | 4 ++
> drivers/mtd/spi-nor/otp.c | 86 +++++++++++++++++++++++++++++++++++
> drivers/mtd/spi-nor/winbond.c | 1 +
> 4 files changed, 92 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index f6a6ef2d8bd8..a21b0085de05 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -1318,7 +1318,7 @@ static u32 spi_nor_convert_addr(struct spi_nor *nor, loff_t addr)
> /*
> * Initiate the erasure of a single sector
> */
> -static int spi_nor_erase_sector(struct spi_nor *nor, u32 addr)
> +int spi_nor_erase_sector(struct spi_nor *nor, u32 addr)
> {
> int i;
>
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 28a2e0be97a3..9398a8738857 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -207,6 +207,7 @@ struct spi_nor_otp_organization {
> * @read: read from the SPI NOR OTP area.
> * @write: write to the SPI NOR OTP area.
> * @lock: lock an OTP region.
> + * @erase: erase an OTP region.
> * @is_locked: check if an OTP region of the SPI NOR is locked.
> */
> struct spi_nor_otp_ops {
> @@ -214,6 +215,7 @@ struct spi_nor_otp_ops {
> int (*write)(struct spi_nor *nor, loff_t addr, size_t len,
> const u8 *buf);
> int (*lock)(struct spi_nor *nor, unsigned int region);
> + int (*erase)(struct spi_nor *nor, loff_t addr);
> int (*is_locked)(struct spi_nor *nor, unsigned int region);
> };
>
> @@ -503,10 +505,12 @@ ssize_t spi_nor_read_data(struct spi_nor *nor, loff_t from, size_t len,
> u8 *buf);
> ssize_t spi_nor_write_data(struct spi_nor *nor, loff_t to, size_t len,
> const u8 *buf);
> +int spi_nor_erase_sector(struct spi_nor *nor, u32 addr);
>
> int spi_nor_otp_read_secr(struct spi_nor *nor, loff_t addr, size_t len, u8 *buf);
> int spi_nor_otp_write_secr(struct spi_nor *nor, loff_t addr, size_t len,
> const u8 *buf);
> +int spi_nor_otp_erase_secr(struct spi_nor *nor, loff_t addr);
> int spi_nor_otp_lock_sr2(struct spi_nor *nor, unsigned int region);
> int spi_nor_otp_is_locked_sr2(struct spi_nor *nor, unsigned int region);
>
> diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
> index 89fe52e3851a..983e40b19134 100644
> --- a/drivers/mtd/spi-nor/otp.c
> +++ b/drivers/mtd/spi-nor/otp.c
> @@ -120,6 +120,38 @@ int spi_nor_otp_write_secr(struct spi_nor *nor, loff_t addr, size_t len,
> return ret ?: written;
> }
>
> +/**
> + * spi_nor_otp_erase_secr() - erase a security register
> + * @nor: pointer to 'struct spi_nor'
> + * @addr: offset of the security register to be erased
> + *
> + * Erase a security register by using the SPINOR_OP_ESECR command.
> + *
> + * For more information on the term "security register", see the documentation
> + * of spi_nor_otp_read_secr().
> + *
> + * This method is used on GigaDevice and Winbond flashes.
> + *
> + * Return: 0 on success, -errno otherwise
> + */
> +int spi_nor_otp_erase_secr(struct spi_nor *nor, loff_t addr)
> +{
> + u8 erase_opcode = nor->erase_opcode;
> + int ret;
> +
> + ret = spi_nor_write_enable(nor);
> + if (ret)
> + return ret;
> +
> + nor->erase_opcode = SPINOR_OP_ESECR;
> + ret = spi_nor_erase_sector(nor, addr);
> + nor->erase_opcode = erase_opcode;
> + if (ret)
> + return ret;
> +
> + return spi_nor_wait_till_ready(nor);
> +}
> +
> static int spi_nor_otp_lock_bit_cr(unsigned int region)
> {
> static const int lock_bits[] = { SR2_LB1, SR2_LB2, SR2_LB3 };
> @@ -360,6 +392,59 @@ static int spi_nor_mtd_otp_write(struct mtd_info *mtd, loff_t to, size_t len,
> return spi_nor_mtd_otp_read_write(mtd, to, len, retlen, buf, true);
> }
>
> +static int spi_nor_mtd_otp_erase(struct mtd_info *mtd, loff_t from, size_t len)
> +{
> + struct spi_nor *nor = mtd_to_spi_nor(mtd);
> + const struct spi_nor_otp_ops *ops = nor->params->otp.ops;
> + const size_t rlen = spi_nor_otp_region_len(nor);
> + unsigned int region;
> + loff_t rstart;
> + int ret;
> +
> + /* OTP erase is optional */
> + if (!ops->erase)
> + return -EOPNOTSUPP;
> +
> + if (!len)
> + return 0;
> +
> + if (from < 0 || (from + len) > spi_nor_otp_size(nor))
> + return -EINVAL;
> +
> + /* the user has to explicitly ask for whole regions */
> + if (!IS_ALIGNED(len, rlen) || !IS_ALIGNED(from, rlen))
> + return -EINVAL;
> +
> + ret = spi_nor_lock_and_prep(nor);
> + if (ret)
> + return ret;
> +
> + ret = spi_nor_mtd_otp_range_is_locked(nor, from, len);
> + if (ret < 0) {
> + goto out;
> + } else if (ret) {
> + ret = -EROFS;
> + goto out;
> + }
> +
> + while (len) {
> + region = spi_nor_otp_offset_to_region(nor, from);
> + rstart = spi_nor_otp_region_start(nor, region);
> +
> + ret = ops->erase(nor, rstart);
> + if (ret)
> + goto out;
> +
> + len -= rlen;
> + from += rlen;
> + }
> +
> +out:
> + spi_nor_unlock_and_unprep(nor);
> +
> + return ret;
> +}
> +
> static int spi_nor_mtd_otp_lock(struct mtd_info *mtd, loff_t from, size_t len)
> {
> struct spi_nor *nor = mtd_to_spi_nor(mtd);
> @@ -418,4 +503,5 @@ void spi_nor_otp_init(struct spi_nor *nor)
> mtd->_read_user_prot_reg = spi_nor_mtd_otp_read;
> mtd->_write_user_prot_reg = spi_nor_mtd_otp_write;
> mtd->_lock_user_prot_reg = spi_nor_mtd_otp_lock;
> + mtd->_erase_user_prot_reg = spi_nor_mtd_otp_erase;
> }
> diff --git a/drivers/mtd/spi-nor/winbond.c b/drivers/mtd/spi-nor/winbond.c
> index 9a81c67a60c6..96573f61caf5 100644
> --- a/drivers/mtd/spi-nor/winbond.c
> +++ b/drivers/mtd/spi-nor/winbond.c
> @@ -139,6 +139,7 @@ static int winbond_set_4byte_addr_mode(struct spi_nor *nor, bool enable)
> static const struct spi_nor_otp_ops winbond_otp_ops = {
> .read = spi_nor_otp_read_secr,
> .write = spi_nor_otp_write_secr,
> + .erase = spi_nor_otp_erase_secr,
> .lock = spi_nor_otp_lock_sr2,
> .is_locked = spi_nor_otp_is_locked_sr2,
> };
> --
> 2.20.1
>
On 6/4/21 1:02 PM, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> By moving the code around a bit, we can just check the length before
> calling spi_nor_mtd_otp_range_is_locked() and drop the length check
> there. This way we don't need to take the lock. This will also skip the
> "*retlen = 0" assignment if the length is zero. But mtdcore already does
> that for us. Thus we can drop that, too.
>
> Signed-off-by: Michael Walle <[email protected]>
> ---
> drivers/mtd/spi-nor/otp.c | 13 ++++++-------
> 1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
> index 063f8fb68649..89fe52e3851a 100644
> --- a/drivers/mtd/spi-nor/otp.c
> +++ b/drivers/mtd/spi-nor/otp.c
> @@ -256,9 +256,6 @@ static int spi_nor_mtd_otp_range_is_locked(struct spi_nor *nor, loff_t ofs,
> unsigned int region;
> int locked;
>
> - if (!len)
> - return 0;
these lines were just introduced in the previous patch. Can you please reorder 4 with 3,
so that we don't touch this twice? With that:
Reviewed-by: Tudor Ambarus <[email protected]>
> -
> /*
> * If any of the affected OTP regions are locked the entire range is
> * considered locked.
> @@ -290,13 +287,16 @@ static int spi_nor_mtd_otp_read_write(struct mtd_info *mtd, loff_t ofs,
> if (ofs < 0 || ofs >= spi_nor_otp_size(nor))
> return 0;
>
> + /* don't access beyond the end */
> + total_len = min_t(size_t, total_len, spi_nor_otp_size(nor) - ofs);
> +
> + if (!total_len)
> + return 0;
> +
> ret = spi_nor_lock_and_prep(nor);
> if (ret)
> return ret;
>
> - /* don't access beyond the end */
> - total_len = min_t(size_t, total_len, spi_nor_otp_size(nor) - ofs);
> -
> if (is_write) {
> ret = spi_nor_mtd_otp_range_is_locked(nor, ofs, total_len);
> if (ret < 0) {
> @@ -307,7 +307,6 @@ static int spi_nor_mtd_otp_read_write(struct mtd_info *mtd, loff_t ofs,
> }
> }
>
> - *retlen = 0;
> while (total_len) {
> /*
> * The OTP regions are mapped into a contiguous area starting
> --
> 2.20.1
>
On 6/4/21 1:02 PM, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> SPI NOR flashes will just ignore program commands if the OTP region is
> locked. Thus, a user might not notice that the intended write didn't end
> up in the flash. Return -EROFS to the user in this case. From what I can
> tell, chips/cfi_cmdset_0001.c also return this error code.
>
> One could optimize spi_nor_mtd_otp_range_is_locked() to read the status
> register only once and not for every OTP region, but for that we would
> need some more invasive changes. Given that this is
> one-time-programmable memory and the normal access mode is reading, we
> just live with the small overhead.
>
> Fixes: 069089acf88b ("mtd: spi-nor: add OTP support")
> Signed-off-by: Michael Walle <[email protected]>
> Reviewed-by: Pratyush Yadav <[email protected]>
> ---
> drivers/mtd/spi-nor/otp.c | 36 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 36 insertions(+)
>
> diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
> index 3898ed67ba1c..063f8fb68649 100644
> --- a/drivers/mtd/spi-nor/otp.c
> +++ b/drivers/mtd/spi-nor/otp.c
> @@ -249,6 +249,32 @@ static int spi_nor_mtd_otp_info(struct mtd_info *mtd, size_t len,
> return ret;
> }
>
> +static int spi_nor_mtd_otp_range_is_locked(struct spi_nor *nor, loff_t ofs,
> + size_t len)
> +{
> + const struct spi_nor_otp_ops *ops = nor->params->otp.ops;
> + unsigned int region;
> + int locked;
> +
> + if (!len)
> + return 0;
> +
You won't need this if you put patch 4/5 before this one. With this:
Reviewed-by: Tudor Ambarus <[email protected]>
> + /*
> + * If any of the affected OTP regions are locked the entire range is
> + * considered locked.
> + */
> + for (region = spi_nor_otp_offset_to_region(nor, ofs);
> + region <= spi_nor_otp_offset_to_region(nor, ofs + len - 1);
> + region++) {
> + locked = ops->is_locked(nor, region);
> + /* take the branch it is locked or in case of an error */
> + if (locked)
> + return locked;
> + }
> +
> + return 0;
> +}
> +
> static int spi_nor_mtd_otp_read_write(struct mtd_info *mtd, loff_t ofs,
> size_t total_len, size_t *retlen,
> const u8 *buf, bool is_write)
> @@ -271,6 +297,16 @@ static int spi_nor_mtd_otp_read_write(struct mtd_info *mtd, loff_t ofs,
> /* don't access beyond the end */
> total_len = min_t(size_t, total_len, spi_nor_otp_size(nor) - ofs);
>
> + if (is_write) {
> + ret = spi_nor_mtd_otp_range_is_locked(nor, ofs, total_len);
> + if (ret < 0) {
> + goto out;
> + } else if (ret) {
> + ret = -EROFS;
> + goto out;
> + }
> + }
> +
> *retlen = 0;
> while (total_len) {
> /*
> --
> 2.20.1
>
Am 2021-06-04 15:07, schrieb [email protected]:
> On 6/4/21 1:02 PM, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>> the content is safe
>>
>> SPI NOR flashes will just ignore program commands if the OTP region is
>> locked. Thus, a user might not notice that the intended write didn't
>> end
>> up in the flash. Return -EROFS to the user in this case. From what I
>> can
>> tell, chips/cfi_cmdset_0001.c also return this error code.
>>
>> One could optimize spi_nor_mtd_otp_range_is_locked() to read the
>> status
>> register only once and not for every OTP region, but for that we would
>> need some more invasive changes. Given that this is
>> one-time-programmable memory and the normal access mode is reading, we
>> just live with the small overhead.
>>
>> Fixes: 069089acf88b ("mtd: spi-nor: add OTP support")
>> Signed-off-by: Michael Walle <[email protected]>
>> Reviewed-by: Pratyush Yadav <[email protected]>
>> ---
>> drivers/mtd/spi-nor/otp.c | 36 ++++++++++++++++++++++++++++++++++++
>> 1 file changed, 36 insertions(+)
>>
>> diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
>> index 3898ed67ba1c..063f8fb68649 100644
>> --- a/drivers/mtd/spi-nor/otp.c
>> +++ b/drivers/mtd/spi-nor/otp.c
>> @@ -249,6 +249,32 @@ static int spi_nor_mtd_otp_info(struct mtd_info
>> *mtd, size_t len,
>> return ret;
>> }
>>
>> +static int spi_nor_mtd_otp_range_is_locked(struct spi_nor *nor,
>> loff_t ofs,
>> + size_t len)
>> +{
>> + const struct spi_nor_otp_ops *ops = nor->params->otp.ops;
>> + unsigned int region;
>> + int locked;
>> +
>> + if (!len)
>> + return 0;
>> +
>
> You won't need this if you put patch 4/5 before this one. With this:
This patch will get backported to the stable kernels. Patch 4 on the
other hand does not.
>
> Reviewed-by: Tudor Ambarus <[email protected]>
>
>> + /*
>> + * If any of the affected OTP regions are locked the entire
>> range is
>> + * considered locked.
>> + */
>> + for (region = spi_nor_otp_offset_to_region(nor, ofs);
>> + region <= spi_nor_otp_offset_to_region(nor, ofs + len -
>> 1);
>> + region++) {
>> + locked = ops->is_locked(nor, region);
>> + /* take the branch it is locked or in case of an error
>> */
>> + if (locked)
>> + return locked;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> static int spi_nor_mtd_otp_read_write(struct mtd_info *mtd, loff_t
>> ofs,
>> size_t total_len, size_t
>> *retlen,
>> const u8 *buf, bool is_write)
>> @@ -271,6 +297,16 @@ static int spi_nor_mtd_otp_read_write(struct
>> mtd_info *mtd, loff_t ofs,
>> /* don't access beyond the end */
>> total_len = min_t(size_t, total_len, spi_nor_otp_size(nor) -
>> ofs);
>>
>> + if (is_write) {
>> + ret = spi_nor_mtd_otp_range_is_locked(nor, ofs,
>> total_len);
>> + if (ret < 0) {
>> + goto out;
>> + } else if (ret) {
>> + ret = -EROFS;
>> + goto out;
>> + }
>> + }
>> +
>> *retlen = 0;
>> while (total_len) {
>> /*
>> --
>> 2.20.1
>>
On 6/4/21 6:45 PM, Michael Walle wrote:
> Am 2021-06-04 15:07, schrieb [email protected]:
>> On 6/4/21 1:02 PM, Michael Walle wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you
>>> know the content is safe
>>>
>>> SPI NOR flashes will just ignore program commands if the OTP region is
>>> locked. Thus, a user might not notice that the intended write didn't end
>>> up in the flash. Return -EROFS to the user in this case. From what I can
>>> tell, chips/cfi_cmdset_0001.c also return this error code.
>>>
>>> One could optimize spi_nor_mtd_otp_range_is_locked() to read the status
>>> register only once and not for every OTP region, but for that we would
>>> need some more invasive changes. Given that this is
>>> one-time-programmable memory and the normal access mode is reading, we
>>> just live with the small overhead.
>>>
>>> Fixes: 069089acf88b ("mtd: spi-nor: add OTP support")
>>> Signed-off-by: Michael Walle <[email protected]>
>>> Reviewed-by: Pratyush Yadav <[email protected]>
>>> ---
>>> drivers/mtd/spi-nor/otp.c | 36 ++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 36 insertions(+)
>>>
>>> diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
>>> index 3898ed67ba1c..063f8fb68649 100644
>>> --- a/drivers/mtd/spi-nor/otp.c
>>> +++ b/drivers/mtd/spi-nor/otp.c
>>> @@ -249,6 +249,32 @@ static int spi_nor_mtd_otp_info(struct mtd_info
>>> *mtd, size_t len,
>>> return ret;
>>> }
>>>
>>> +static int spi_nor_mtd_otp_range_is_locked(struct spi_nor *nor,
>>> loff_t ofs,
>>> + size_t len)
>>> +{
>>> + const struct spi_nor_otp_ops *ops = nor->params->otp.ops;
>>> + unsigned int region;
>>> + int locked;
>>> +
>>> + if (!len)
>>> + return 0;
>>> +
>>
>> You won't need this if you put patch 4/5 before this one. With this:
>
> This patch will get backported to the stable kernels. Patch 4 on the
> other hand does not.
>
I don't see why 4/5 cannot be marked for backport too as it makes 3/5
much cleaner?
Regards
Vignesh
Am 2021-06-07 07:46, schrieb Vignesh Raghavendra:
> On 6/4/21 6:45 PM, Michael Walle wrote:
>> Am 2021-06-04 15:07, schrieb [email protected]:
>>> On 6/4/21 1:02 PM, Michael Walle wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you
>>>> know the content is safe
>>>>
>>>> SPI NOR flashes will just ignore program commands if the OTP region
>>>> is
>>>> locked. Thus, a user might not notice that the intended write didn't
>>>> end
>>>> up in the flash. Return -EROFS to the user in this case. From what I
>>>> can
>>>> tell, chips/cfi_cmdset_0001.c also return this error code.
>>>>
>>>> One could optimize spi_nor_mtd_otp_range_is_locked() to read the
>>>> status
>>>> register only once and not for every OTP region, but for that we
>>>> would
>>>> need some more invasive changes. Given that this is
>>>> one-time-programmable memory and the normal access mode is reading,
>>>> we
>>>> just live with the small overhead.
>>>>
>>>> Fixes: 069089acf88b ("mtd: spi-nor: add OTP support")
>>>> Signed-off-by: Michael Walle <[email protected]>
>>>> Reviewed-by: Pratyush Yadav <[email protected]>
>>>> ---
>>>> drivers/mtd/spi-nor/otp.c | 36 ++++++++++++++++++++++++++++++++++++
>>>> 1 file changed, 36 insertions(+)
>>>>
>>>> diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
>>>> index 3898ed67ba1c..063f8fb68649 100644
>>>> --- a/drivers/mtd/spi-nor/otp.c
>>>> +++ b/drivers/mtd/spi-nor/otp.c
>>>> @@ -249,6 +249,32 @@ static int spi_nor_mtd_otp_info(struct mtd_info
>>>> *mtd, size_t len,
>>>> return ret;
>>>> }
>>>>
>>>> +static int spi_nor_mtd_otp_range_is_locked(struct spi_nor *nor,
>>>> loff_t ofs,
>>>> + size_t len)
>>>> +{
>>>> + const struct spi_nor_otp_ops *ops = nor->params->otp.ops;
>>>> + unsigned int region;
>>>> + int locked;
>>>> +
>>>> + if (!len)
>>>> + return 0;
>>>> +
>>>
>>> You won't need this if you put patch 4/5 before this one. With this:
>>
>> This patch will get backported to the stable kernels. Patch 4 on the
>> other hand does not.
>>
>
> I don't see why 4/5 cannot be marked for backport too as it makes 3/5
> much cleaner?
What kind of problem does 4/5 fix? I can't see how that patch would
apply to any rule in Documentation/process/stable-kernel-rules.rst.
But sure, adding the same Fixes: tag, I can swap those two.
-michael
On 6/7/21 11:38 AM, Michael Walle wrote:
> Am 2021-06-07 07:46, schrieb Vignesh Raghavendra:
>> On 6/4/21 6:45 PM, Michael Walle wrote:
>>> Am 2021-06-04 15:07, schrieb [email protected]:
>>>> On 6/4/21 1:02 PM, Michael Walle wrote:
>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you
>>>>> know the content is safe
>>>>>
>>>>> SPI NOR flashes will just ignore program commands if the OTP region is
>>>>> locked. Thus, a user might not notice that the intended write
>>>>> didn't end
>>>>> up in the flash. Return -EROFS to the user in this case. From what
>>>>> I can
>>>>> tell, chips/cfi_cmdset_0001.c also return this error code.
>>>>>
>>>>> One could optimize spi_nor_mtd_otp_range_is_locked() to read the
>>>>> status
>>>>> register only once and not for every OTP region, but for that we would
>>>>> need some more invasive changes. Given that this is
>>>>> one-time-programmable memory and the normal access mode is reading, we
>>>>> just live with the small overhead.
>>>>>
>>>>> Fixes: 069089acf88b ("mtd: spi-nor: add OTP support")
>>>>> Signed-off-by: Michael Walle <[email protected]>
>>>>> Reviewed-by: Pratyush Yadav <[email protected]>
>>>>> ---
>>>>> drivers/mtd/spi-nor/otp.c | 36 ++++++++++++++++++++++++++++++++++++
>>>>> 1 file changed, 36 insertions(+)
>>>>>
>>>>> diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
>>>>> index 3898ed67ba1c..063f8fb68649 100644
>>>>> --- a/drivers/mtd/spi-nor/otp.c
>>>>> +++ b/drivers/mtd/spi-nor/otp.c
>>>>> @@ -249,6 +249,32 @@ static int spi_nor_mtd_otp_info(struct mtd_info
>>>>> *mtd, size_t len,
>>>>> return ret;
>>>>> }
>>>>>
>>>>> +static int spi_nor_mtd_otp_range_is_locked(struct spi_nor *nor,
>>>>> loff_t ofs,
>>>>> + size_t len)
>>>>> +{
>>>>> + const struct spi_nor_otp_ops *ops = nor->params->otp.ops;
>>>>> + unsigned int region;
>>>>> + int locked;
>>>>> +
>>>>> + if (!len)
>>>>> + return 0;
>>>>> +
>>>>
>>>> You won't need this if you put patch 4/5 before this one. With this:
>>>
>>> This patch will get backported to the stable kernels. Patch 4 on the
>>> other hand does not.
>>>
>>
>> I don't see why 4/5 cannot be marked for backport too as it makes 3/5
>> much cleaner?
>
> What kind of problem does 4/5 fix? I can't see how that patch would
> apply to any rule in Documentation/process/stable-kernel-rules.rst.
>
Looking further, I don't see the need for 4/5 to be a separate patch.
Patch 4/5 is simplifying spi_nor_mtd_otp_range_is_locked() by ensuring
'len' passed is never 0 which can be done in 3/5 when introducing
spi_nor_mtd_otp_range_is_locked().
So why not squashed it into 3/5.
> But sure, adding the same Fixes: tag, I can swap those two.
>
> -michael
Am 2021-06-07 08:47, schrieb Vignesh Raghavendra:
> On 6/7/21 11:38 AM, Michael Walle wrote:
>> Am 2021-06-07 07:46, schrieb Vignesh Raghavendra:
>>> On 6/4/21 6:45 PM, Michael Walle wrote:
>>>> Am 2021-06-04 15:07, schrieb [email protected]:
>>>>> On 6/4/21 1:02 PM, Michael Walle wrote:
>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you
>>>>>> know the content is safe
>>>>>>
>>>>>> SPI NOR flashes will just ignore program commands if the OTP
>>>>>> region is
>>>>>> locked. Thus, a user might not notice that the intended write
>>>>>> didn't end
>>>>>> up in the flash. Return -EROFS to the user in this case. From what
>>>>>> I can
>>>>>> tell, chips/cfi_cmdset_0001.c also return this error code.
>>>>>>
>>>>>> One could optimize spi_nor_mtd_otp_range_is_locked() to read the
>>>>>> status
>>>>>> register only once and not for every OTP region, but for that we
>>>>>> would
>>>>>> need some more invasive changes. Given that this is
>>>>>> one-time-programmable memory and the normal access mode is
>>>>>> reading, we
>>>>>> just live with the small overhead.
>>>>>>
>>>>>> Fixes: 069089acf88b ("mtd: spi-nor: add OTP support")
>>>>>> Signed-off-by: Michael Walle <[email protected]>
>>>>>> Reviewed-by: Pratyush Yadav <[email protected]>
>>>>>> ---
>>>>>> drivers/mtd/spi-nor/otp.c | 36
>>>>>> ++++++++++++++++++++++++++++++++++++
>>>>>> 1 file changed, 36 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
>>>>>> index 3898ed67ba1c..063f8fb68649 100644
>>>>>> --- a/drivers/mtd/spi-nor/otp.c
>>>>>> +++ b/drivers/mtd/spi-nor/otp.c
>>>>>> @@ -249,6 +249,32 @@ static int spi_nor_mtd_otp_info(struct
>>>>>> mtd_info
>>>>>> *mtd, size_t len,
>>>>>> return ret;
>>>>>> }
>>>>>>
>>>>>> +static int spi_nor_mtd_otp_range_is_locked(struct spi_nor *nor,
>>>>>> loff_t ofs,
>>>>>> + size_t len)
>>>>>> +{
>>>>>> + const struct spi_nor_otp_ops *ops = nor->params->otp.ops;
>>>>>> + unsigned int region;
>>>>>> + int locked;
>>>>>> +
>>>>>> + if (!len)
>>>>>> + return 0;
>>>>>> +
>>>>>
>>>>> You won't need this if you put patch 4/5 before this one. With
>>>>> this:
>>>>
>>>> This patch will get backported to the stable kernels. Patch 4 on the
>>>> other hand does not.
>>>>
>>>
>>> I don't see why 4/5 cannot be marked for backport too as it makes 3/5
>>> much cleaner?
>>
>> What kind of problem does 4/5 fix? I can't see how that patch would
>> apply to any rule in Documentation/process/stable-kernel-rules.rst.
>>
>
> Looking further, I don't see the need for 4/5 to be a separate patch.
> Patch 4/5 is simplifying spi_nor_mtd_otp_range_is_locked() by ensuring
> 'len' passed is never 0 which can be done in 3/5 when introducing
> spi_nor_mtd_otp_range_is_locked().
>
> So why not squashed it into 3/5.
Because, strictly speaking, it is not part of that particular fix
and IMHO violates "It must fix only one thing". But if you're fine
with that, I can squash the two.
TBH I find it kinda funny to bend the rules, just to get rid of
these three lines of code or the ugliness that they will be removed
in the following patch.
-michael
On 6/7/21 3:26 PM, Michael Walle wrote:
> Am 2021-06-07 08:47, schrieb Vignesh Raghavendra:
>> On 6/7/21 11:38 AM, Michael Walle wrote:
>>> Am 2021-06-07 07:46, schrieb Vignesh Raghavendra:
>>>> On 6/4/21 6:45 PM, Michael Walle wrote:
>>>>> Am 2021-06-04 15:07, schrieb [email protected]:
>>>>>> On 6/4/21 1:02 PM, Michael Walle wrote:
>>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you
>>>>>>> know the content is safe
>>>>>>>
>>>>>>> SPI NOR flashes will just ignore program commands if the OTP
>>>>>>> region is
>>>>>>> locked. Thus, a user might not notice that the intended write
>>>>>>> didn't end
>>>>>>> up in the flash. Return -EROFS to the user in this case. From what
>>>>>>> I can
>>>>>>> tell, chips/cfi_cmdset_0001.c also return this error code.
>>>>>>>
>>>>>>> One could optimize spi_nor_mtd_otp_range_is_locked() to read the
>>>>>>> status
>>>>>>> register only once and not for every OTP region, but for that we
>>>>>>> would
>>>>>>> need some more invasive changes. Given that this is
>>>>>>> one-time-programmable memory and the normal access mode is
>>>>>>> reading, we
>>>>>>> just live with the small overhead.
>>>>>>>
>>>>>>> Fixes: 069089acf88b ("mtd: spi-nor: add OTP support")
>>>>>>> Signed-off-by: Michael Walle <[email protected]>
>>>>>>> Reviewed-by: Pratyush Yadav <[email protected]>
>>>>>>> ---
>>>>>>> drivers/mtd/spi-nor/otp.c | 36 ++++++++++++++++++++++++++++++++++++
>>>>>>> 1 file changed, 36 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
>>>>>>> index 3898ed67ba1c..063f8fb68649 100644
>>>>>>> --- a/drivers/mtd/spi-nor/otp.c
>>>>>>> +++ b/drivers/mtd/spi-nor/otp.c
>>>>>>> @@ -249,6 +249,32 @@ static int spi_nor_mtd_otp_info(struct mtd_info
>>>>>>> *mtd, size_t len,
>>>>>>> return ret;
>>>>>>> }
>>>>>>>
>>>>>>> +static int spi_nor_mtd_otp_range_is_locked(struct spi_nor *nor,
>>>>>>> loff_t ofs,
>>>>>>> + size_t len)
>>>>>>> +{
>>>>>>> + const struct spi_nor_otp_ops *ops = nor->params->otp.ops;
>>>>>>> + unsigned int region;
>>>>>>> + int locked;
>>>>>>> +
>>>>>>> + if (!len)
>>>>>>> + return 0;
>>>>>>> +
>>>>>>
>>>>>> You won't need this if you put patch 4/5 before this one. With this:
>>>>>
>>>>> This patch will get backported to the stable kernels. Patch 4 on the
>>>>> other hand does not.
>>>>>
>>>>
>>>> I don't see why 4/5 cannot be marked for backport too as it makes 3/5
>>>> much cleaner?
>>>
>>> What kind of problem does 4/5 fix? I can't see how that patch would
>>> apply to any rule in Documentation/process/stable-kernel-rules.rst.
>>>
>>
>> Looking further, I don't see the need for 4/5 to be a separate patch.
>> Patch 4/5 is simplifying spi_nor_mtd_otp_range_is_locked() by ensuring
>> 'len' passed is never 0 which can be done in 3/5 when introducing
>> spi_nor_mtd_otp_range_is_locked().
>>
>> So why not squashed it into 3/5.
>
> Because, strictly speaking, it is not part of that particular fix
> and IMHO violates "It must fix only one thing". But if you're fine
> with that, I can squash the two.
>
> TBH I find it kinda funny to bend the rules, just to get rid of
> these three lines of code or the ugliness that they will be removed
> in the following patch.
>
This is still fixing only one thing "Indicating OTP writes are ignored
when region is locked" (ie spi_nor_mtd_otp_range_is_locked() check).
But, spi_nor_mtd_otp_range_is_locked() (as in 3/5) can be simplified if
'len != 0' is checked prior to calling the function. That's what 4/5
does which I believe can be squashed here.
I just don't like code being refactored for the purpose of being able to
be backported. It feels weird to have a piece of code being added in one
commit, and then being deleted the very next commit.
So strictly speaking 4/5 has to come before 3/5.
But I am fine to live with this temporary ugliness if Tudor agrees.
Regards
Vignesh
Am 2021-06-07 12:30, schrieb Vignesh Raghavendra:
> On 6/7/21 3:26 PM, Michael Walle wrote:
>> Am 2021-06-07 08:47, schrieb Vignesh Raghavendra:
>>> On 6/7/21 11:38 AM, Michael Walle wrote:
>>>> Am 2021-06-07 07:46, schrieb Vignesh Raghavendra:
>>>>> On 6/4/21 6:45 PM, Michael Walle wrote:
>>>>>> Am 2021-06-04 15:07, schrieb [email protected]:
>>>>>>> On 6/4/21 1:02 PM, Michael Walle wrote:
>>>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless
>>>>>>>> you
>>>>>>>> know the content is safe
>>>>>>>>
>>>>>>>> SPI NOR flashes will just ignore program commands if the OTP
>>>>>>>> region is
>>>>>>>> locked. Thus, a user might not notice that the intended write
>>>>>>>> didn't end
>>>>>>>> up in the flash. Return -EROFS to the user in this case. From
>>>>>>>> what
>>>>>>>> I can
>>>>>>>> tell, chips/cfi_cmdset_0001.c also return this error code.
>>>>>>>>
>>>>>>>> One could optimize spi_nor_mtd_otp_range_is_locked() to read the
>>>>>>>> status
>>>>>>>> register only once and not for every OTP region, but for that we
>>>>>>>> would
>>>>>>>> need some more invasive changes. Given that this is
>>>>>>>> one-time-programmable memory and the normal access mode is
>>>>>>>> reading, we
>>>>>>>> just live with the small overhead.
>>>>>>>>
>>>>>>>> Fixes: 069089acf88b ("mtd: spi-nor: add OTP support")
>>>>>>>> Signed-off-by: Michael Walle <[email protected]>
>>>>>>>> Reviewed-by: Pratyush Yadav <[email protected]>
>>>>>>>> ---
>>>>>>>> drivers/mtd/spi-nor/otp.c | 36
>>>>>>>> ++++++++++++++++++++++++++++++++++++
>>>>>>>> 1 file changed, 36 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/mtd/spi-nor/otp.c
>>>>>>>> b/drivers/mtd/spi-nor/otp.c
>>>>>>>> index 3898ed67ba1c..063f8fb68649 100644
>>>>>>>> --- a/drivers/mtd/spi-nor/otp.c
>>>>>>>> +++ b/drivers/mtd/spi-nor/otp.c
>>>>>>>> @@ -249,6 +249,32 @@ static int spi_nor_mtd_otp_info(struct
>>>>>>>> mtd_info
>>>>>>>> *mtd, size_t len,
>>>>>>>> return ret;
>>>>>>>> }
>>>>>>>>
>>>>>>>> +static int spi_nor_mtd_otp_range_is_locked(struct spi_nor *nor,
>>>>>>>> loff_t ofs,
>>>>>>>> + size_t len)
>>>>>>>> +{
>>>>>>>> + const struct spi_nor_otp_ops *ops =
>>>>>>>> nor->params->otp.ops;
>>>>>>>> + unsigned int region;
>>>>>>>> + int locked;
>>>>>>>> +
>>>>>>>> + if (!len)
>>>>>>>> + return 0;
>>>>>>>> +
>>>>>>>
>>>>>>> You won't need this if you put patch 4/5 before this one. With
>>>>>>> this:
>>>>>>
>>>>>> This patch will get backported to the stable kernels. Patch 4 on
>>>>>> the
>>>>>> other hand does not.
>>>>>>
>>>>>
>>>>> I don't see why 4/5 cannot be marked for backport too as it makes
>>>>> 3/5
>>>>> much cleaner?
>>>>
>>>> What kind of problem does 4/5 fix? I can't see how that patch would
>>>> apply to any rule in Documentation/process/stable-kernel-rules.rst.
>>>>
>>>
>>> Looking further, I don't see the need for 4/5 to be a separate patch.
>>> Patch 4/5 is simplifying spi_nor_mtd_otp_range_is_locked() by
>>> ensuring
>>> 'len' passed is never 0 which can be done in 3/5 when introducing
>>> spi_nor_mtd_otp_range_is_locked().
>>>
>>> So why not squashed it into 3/5.
>>
>> Because, strictly speaking, it is not part of that particular fix
>> and IMHO violates "It must fix only one thing". But if you're fine
>> with that, I can squash the two.
>>
>> TBH I find it kinda funny to bend the rules, just to get rid of
>> these three lines of code or the ugliness that they will be removed
>> in the following patch.
>>
>
> This is still fixing only one thing "Indicating OTP writes are ignored
> when region is locked" (ie spi_nor_mtd_otp_range_is_locked() check).
> But, spi_nor_mtd_otp_range_is_locked() (as in 3/5) can be simplified if
> 'len != 0' is checked prior to calling the function. That's what 4/5
> does which I believe can be squashed here.
Correct, but it also skip the lock/unlock as well as the "*retlen = 0",
which isn't needed to just fix the error, IMHO. I've already asked Tudor
before adding this patch, but I guess I didn't make it clear enough that
one would be for the backporting and one for for-next.
> I just don't like code being refactored for the purpose of being able
> to
> be backported. It feels weird to have a piece of code being added in
> one
> commit, and then being deleted the very next commit.
> So strictly speaking 4/5 has to come before 3/5.
Fair enough. And I tend to agree. But see my reasoning above why I did
it that way.
> But I am fine to live with this temporary ugliness if Tudor agrees.
No need ;) I'll squash it. It might even be better, if the two versions
doesn't diverge that much.
-michael