[PATCH v5 0/5] mtd: spi-nor: otp: 4 byte mode fix and erase support

[PATCH v5 0/5] mtd: spi-nor: otp: 4 byte mode fix and erase support

[Michael Walle]

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

[PATCH v5 0/5] mtd: spi-nor: otp: 4 byte mode fix and erase support

[Michael Walle]

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


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v5 1/5] mtd: spi-nor: otp: fix access to security registers in 4 byte mode

[Michael Walle]

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 <michael@walle.cc>
Acked-by: Pratyush Yadav <p.yadav@ti.com>
Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 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

[PATCH v5 1/5] mtd: spi-nor: otp: fix access to security registers in 4 byte mode

[Michael Walle]

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 <michael@walle.cc>
Acked-by: Pratyush Yadav <p.yadav@ti.com>
Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 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


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v5 2/5] mtd: spi-nor: otp: use more consistent wording

[Michael Walle]

Use the wording as used in the datasheet to describe the access methods
of the security registers (aka OTP storage). This will also match the
function names.

Signed-off-by: Michael Walle <michael@walle.cc>
Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/mtd/spi-nor/otp.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
index 91a4c510ed51..3898ed67ba1c 100644
--- a/drivers/mtd/spi-nor/otp.c
+++ b/drivers/mtd/spi-nor/otp.c
@@ -15,14 +15,21 @@
 #define spi_nor_otp_n_regions(nor) ((nor)->params->otp.org->n_regions)
 
 /**
- * spi_nor_otp_read_secr() - read OTP data
+ * spi_nor_otp_read_secr() - read security register
  * @nor:	pointer to 'struct spi_nor'
  * @addr:       offset to read from
  * @len:        number of bytes to read
  * @buf:        pointer to dst buffer
  *
- * Read OTP data from one region by using the SPINOR_OP_RSECR commands. This
- * method is used on GigaDevice and Winbond flashes.
+ * Read a security register by using the SPINOR_OP_RSECR commands.
+ *
+ * In Winbond/GigaDevice datasheets the term "security register" stands for
+ * an one-time-programmable memory area, consisting of multiple bytes (usually
+ * 256). Thus one "security register" maps to one OTP region.
+ *
+ * This method is used on GigaDevice and Winbond flashes.
+ *
+ * Please note, the read must not span multiple registers.
  *
  * Return: number of bytes read successfully, -errno otherwise
  */
@@ -56,16 +63,20 @@ int spi_nor_otp_read_secr(struct spi_nor *nor, loff_t addr, size_t len, u8 *buf)
 }
 
 /**
- * spi_nor_otp_write_secr() - write OTP data
+ * spi_nor_otp_write_secr() - write security register
  * @nor:        pointer to 'struct spi_nor'
  * @addr:       offset to write to
  * @len:        number of bytes to write
  * @buf:        pointer to src buffer
  *
- * Write OTP data to one region by using the SPINOR_OP_PSECR commands. This
- * method is used on GigaDevice and Winbond flashes.
+ * Write a security register by using the SPINOR_OP_PSECR commands.
+ *
+ * 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.
  *
- * Please note, the write must not span multiple OTP regions.
+ * Please note, the write must not span multiple registers.
  *
  * Return: number of bytes written successfully, -errno otherwise
  */
@@ -88,7 +99,7 @@ int spi_nor_otp_write_secr(struct spi_nor *nor, loff_t addr, size_t len,
 
 	/*
 	 * We only support a write to one single page. For now all winbond
-	 * flashes only have one page per OTP region.
+	 * flashes only have one page per security register.
 	 */
 	ret = spi_nor_write_enable(nor);
 	if (ret)
-- 
2.20.1

[PATCH v5 2/5] mtd: spi-nor: otp: use more consistent wording

[Michael Walle]

Use the wording as used in the datasheet to describe the access methods
of the security registers (aka OTP storage). This will also match the
function names.

Signed-off-by: Michael Walle <michael@walle.cc>
Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/mtd/spi-nor/otp.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
index 91a4c510ed51..3898ed67ba1c 100644
--- a/drivers/mtd/spi-nor/otp.c
+++ b/drivers/mtd/spi-nor/otp.c
@@ -15,14 +15,21 @@
 #define spi_nor_otp_n_regions(nor) ((nor)->params->otp.org->n_regions)
 
 /**
- * spi_nor_otp_read_secr() - read OTP data
+ * spi_nor_otp_read_secr() - read security register
  * @nor:	pointer to 'struct spi_nor'
  * @addr:       offset to read from
  * @len:        number of bytes to read
  * @buf:        pointer to dst buffer
  *
- * Read OTP data from one region by using the SPINOR_OP_RSECR commands. This
- * method is used on GigaDevice and Winbond flashes.
+ * Read a security register by using the SPINOR_OP_RSECR commands.
+ *
+ * In Winbond/GigaDevice datasheets the term "security register" stands for
+ * an one-time-programmable memory area, consisting of multiple bytes (usually
+ * 256). Thus one "security register" maps to one OTP region.
+ *
+ * This method is used on GigaDevice and Winbond flashes.
+ *
+ * Please note, the read must not span multiple registers.
  *
  * Return: number of bytes read successfully, -errno otherwise
  */
@@ -56,16 +63,20 @@ int spi_nor_otp_read_secr(struct spi_nor *nor, loff_t addr, size_t len, u8 *buf)
 }
 
 /**
- * spi_nor_otp_write_secr() - write OTP data
+ * spi_nor_otp_write_secr() - write security register
  * @nor:        pointer to 'struct spi_nor'
  * @addr:       offset to write to
  * @len:        number of bytes to write
  * @buf:        pointer to src buffer
  *
- * Write OTP data to one region by using the SPINOR_OP_PSECR commands. This
- * method is used on GigaDevice and Winbond flashes.
+ * Write a security register by using the SPINOR_OP_PSECR commands.
+ *
+ * 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.
  *
- * Please note, the write must not span multiple OTP regions.
+ * Please note, the write must not span multiple registers.
  *
  * Return: number of bytes written successfully, -errno otherwise
  */
@@ -88,7 +99,7 @@ int spi_nor_otp_write_secr(struct spi_nor *nor, loff_t addr, size_t len,
 
 	/*
 	 * We only support a write to one single page. For now all winbond
-	 * flashes only have one page per OTP region.
+	 * flashes only have one page per security register.
 	 */
 	ret = spi_nor_write_enable(nor);
 	if (ret)
-- 
2.20.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v5 3/5] mtd: spi-nor: otp: return -EROFS if region is read-only

[Michael Walle]

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 <michael@walle.cc>
Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
---
 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

[PATCH v5 3/5] mtd: spi-nor: otp: return -EROFS if region is read-only

[Michael Walle]

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 <michael@walle.cc>
Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
---
 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


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v5 4/5] mtd: spi-nor: otp: simplify length check

[Michael Walle]

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 <michael@walle.cc>
---
 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

[PATCH v5 4/5] mtd: spi-nor: otp: simplify length check

[Michael Walle]

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 <michael@walle.cc>
---
 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


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v5 5/5] mtd: spi-nor: otp: implement erase for Winbond and similar flashes

[Michael Walle]

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 <michael@walle.cc>
Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
---
 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

[PATCH v5 5/5] mtd: spi-nor: otp: implement erase for Winbond and similar flashes

[Michael Walle]

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 <michael@walle.cc>
Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
---
 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


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v5 5/5] mtd: spi-nor: otp: implement erase for Winbond and similar flashes

[Tudor.Ambarus]

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 <michael@walle.cc>
> Reviewed-by: Pratyush Yadav <p.yadav@ti.com>

Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>

> ---
>  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
> 

Re: [PATCH v5 5/5] mtd: spi-nor: otp: implement erase for Winbond and similar flashes

[Tudor.Ambarus]

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 <michael@walle.cc>
> Reviewed-by: Pratyush Yadav <p.yadav@ti.com>

Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>

> ---
>  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
> 

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v5 4/5] mtd: spi-nor: otp: simplify length check

[Tudor.Ambarus]

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 <michael@walle.cc>
> ---
>  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 <tudor.ambarus@microchip.com>

> -
>         /*
>          * 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
> 

Re: [PATCH v5 4/5] mtd: spi-nor: otp: simplify length check

[Tudor.Ambarus]

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 <michael@walle.cc>
> ---
>  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 <tudor.ambarus@microchip.com>

> -
>         /*
>          * 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
> 

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v5 3/5] mtd: spi-nor: otp: return -EROFS if region is read-only

[Tudor.Ambarus]

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 <michael@walle.cc>
> Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
> ---
>  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 <tudor.ambarus@microchip.com>

> +       /*
> +        * 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
> 

Re: [PATCH v5 3/5] mtd: spi-nor: otp: return -EROFS if region is read-only

[Tudor.Ambarus]

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 <michael@walle.cc>
> Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
> ---
>  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 <tudor.ambarus@microchip.com>

> +       /*
> +        * 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
> 

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v5 3/5] mtd: spi-nor: otp: return -EROFS if region is read-only

[Michael Walle]

Am 2021-06-04 15:07, schrieb Tudor.Ambarus@microchip.com:
> 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 <michael@walle.cc>
>> Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
>> ---
>>  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 <tudor.ambarus@microchip.com>
> 
>> +       /*
>> +        * 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
>> 

Re: [PATCH v5 3/5] mtd: spi-nor: otp: return -EROFS if region is read-only

[Michael Walle]

Am 2021-06-04 15:07, schrieb Tudor.Ambarus@microchip.com:
> 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 <michael@walle.cc>
>> Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
>> ---
>>  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 <tudor.ambarus@microchip.com>
> 
>> +       /*
>> +        * 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
>> 

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v5 3/5] mtd: spi-nor: otp: return -EROFS if region is read-only

[Vignesh Raghavendra]

On 6/4/21 6:45 PM, Michael Walle wrote:
> Am 2021-06-04 15:07, schrieb Tudor.Ambarus@microchip.com:
>> 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 <michael@walle.cc>
>>> Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
>>> ---
>>>  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

Re: [PATCH v5 3/5] mtd: spi-nor: otp: return -EROFS if region is read-only

[Vignesh Raghavendra]

On 6/4/21 6:45 PM, Michael Walle wrote:
> Am 2021-06-04 15:07, schrieb Tudor.Ambarus@microchip.com:
>> 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 <michael@walle.cc>
>>> Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
>>> ---
>>>  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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v5 3/5] mtd: spi-nor: otp: return -EROFS if region is read-only

[Michael Walle]

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 Tudor.Ambarus@microchip.com:
>>> 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 <michael@walle.cc>
>>>> Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
>>>> ---
>>>>  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

Re: [PATCH v5 3/5] mtd: spi-nor: otp: return -EROFS if region is read-only

[Michael Walle]

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 Tudor.Ambarus@microchip.com:
>>> 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 <michael@walle.cc>
>>>> Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
>>>> ---
>>>>  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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v5 3/5] mtd: spi-nor: otp: return -EROFS if region is read-only

[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 Tudor.Ambarus@microchip.com:
>>>> 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 <michael@walle.cc>
>>>>> Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
>>>>> ---
>>>>>  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

Re: [PATCH v5 3/5] mtd: spi-nor: otp: return -EROFS if region is read-only

[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 Tudor.Ambarus@microchip.com:
>>>> 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 <michael@walle.cc>
>>>>> Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
>>>>> ---
>>>>>  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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v5 3/5] mtd: spi-nor: otp: return -EROFS if region is read-only

[Michael Walle]

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 Tudor.Ambarus@microchip.com:
>>>>> 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 <michael@walle.cc>
>>>>>> Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
>>>>>> ---
>>>>>>  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

Re: [PATCH v5 3/5] mtd: spi-nor: otp: return -EROFS if region is read-only

[Michael Walle]

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 Tudor.Ambarus@microchip.com:
>>>>> 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 <michael@walle.cc>
>>>>>> Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
>>>>>> ---
>>>>>>  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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v5 3/5] mtd: spi-nor: otp: return -EROFS if region is read-only

[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 Tudor.Ambarus@microchip.com:
>>>>>> 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 <michael@walle.cc>
>>>>>>> Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
>>>>>>> ---
>>>>>>>  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

Re: [PATCH v5 3/5] mtd: spi-nor: otp: return -EROFS if region is read-only

[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 Tudor.Ambarus@microchip.com:
>>>>>> 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 <michael@walle.cc>
>>>>>>> Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
>>>>>>> ---
>>>>>>>  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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v5 3/5] mtd: spi-nor: otp: return -EROFS if region is read-only

[Michael Walle]

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 Tudor.Ambarus@microchip.com:
>>>>>>> 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 <michael@walle.cc>
>>>>>>>> Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
>>>>>>>> ---
>>>>>>>>  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

Re: [PATCH v5 3/5] mtd: spi-nor: otp: return -EROFS if region is read-only

[Michael Walle]

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 Tudor.Ambarus@microchip.com:
>>>>>>> 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 <michael@walle.cc>
>>>>>>>> Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
>>>>>>>> ---
>>>>>>>>  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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/