Re: [PATCH v2 2/2] mtd: spi-nor: sst: Add support for Global Unlock on sst26vf

From: Michael Walle
Date: Wed Jan 20 2021 - 12:08:13 EST

Am 2021-01-20 16:39, schrieb Tudor.Ambarus@xxxxxxxxxxxxx:
On 1/20/21 5:02 PM, Michael Walle wrote:
EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

Am 2021-01-20 15:52, schrieb Tudor.Ambarus@xxxxxxxxxxxxx:
On 1/20/21 4:05 PM, Michael Walle wrote:
diff --git a/drivers/mtd/spi-nor/sst.c b/drivers/mtd/spi-nor/sst.c
index 00e48da0744a..d6e1396abb96 100644
--- a/drivers/mtd/spi-nor/sst.c
+++ b/drivers/mtd/spi-nor/sst.c
@@ -8,6 +8,39 @@

 #include "core.h"

+static int sst26vf_lock(struct spi_nor *nor, loff_t ofs, uint64_t
+     return -EOPNOTSUPP;
+static int sst26vf_unlock(struct spi_nor *nor, loff_t ofs, uint64_t
+     if (ofs == 0 && len == nor->params->size)
+             return spi_nor_global_block_unlock(nor);

Some blocks might not be unlocked because they are permanently
locked. Does it make sense to read BPNV of the control register
and add a debug message here?

It would, yes. If any block is permanently locked in the unlock_all
I'll just print a dbg message and return -EINVAL. Sounds good?

spi_nor_sr_unlock(), atmel_at25fs_unlock() and atmel_global_unprotect()
will return -EIO in case the SR wasn't writable.

You mean in the spi_nor_write_sr_and_check() calls. -EIO is fine
there if what we wrote is different than what we read back, it would
indicate an IO error.

GBULK command clears all the write-protection bits in the Block
Protection register, except for those bits that have been permanently
locked down. So even if we have few blocks permanently locked, i.e.
CR.BPNV == 1, the GBULK can clear the protection for the remaining
blocks. So not really an IO error, but rather an -EINVAL, because
the user asks to unlock more than we can.

Doesn't EINVAL indicate wrong parameters, but does nothing? In this
case, unlock would be partially successful.

In any case, my point was that depending on the underlying locking
ops, either -EIO or -EINVAL is returned if spi_nor_unlock() fails
for the same reason, that is unlock() wasn't possible because of
some sort of hardware write protection. And IMHO it should return
the same errno (whatever the correct errno is in this case).