[PATCH v2] mtd: spi-nor: keep lock bits if they are non-volatile

[PATCH v2] mtd: spi-nor: keep lock bits if they are non-volatile

[Michael Walle]

Traditionally, linux unlocks the whole flash because there are legacy
devices which has the write protections bits set by default at startup.
If you actually want to use the flash protection bits, eg. because there
is a read-only part for a bootloader, this automatic unlocking is
harmful. If there is no hardware write protection in place (usually
called WP#), a startup of the kernel just discards this protection.

I've gone through the datasheets of all the flashes (except the Intel
ones where I could not find any datasheet nor reference) which supports
the unlocking feature and looked how the sector protection was
implemented. The currently supported flashes can be divided into the
following two categories:
 (1) block protection bits are non-volatile. Thus they keep their values
     at reset and power-cycle
 (2) flashes where these bits are volatile. After reset or power-cycle,
     the whole memory array is protected.
     (a) some devices needs a special "Global Unprotect" command, eg.
         the Atmel AT25DF041A.
     (b) some devices require to clear the BPn bits in the status
         register.

Due to the reasons above, we do not want to clear the bits for flashes
which belong to category (1). Fortunately for us, the flashes in (2a)
and (2b) are compatible with each other in a sense that the "Global
Unprotect" command will clear the block protection bits in all the (2b)
flashes.

This patch adds a new flag to indicate the case (2). Only if we have
such a flash we perform a "Global Unprotect". Hopefully, this will clean
up "unlock the entire flash for legacy devices" once and for all.

For reference here are the actually commits which introduced the legacy
behaviour (and extended the behaviour to other chip manufacturers):

commit f80e521c916cb ("mtd: m25p80: add support for the Intel/Numonyx {16,32,64}0S33B SPI flash chips")
commit ea60658a08f8f ("mtd: m25p80: disable SST software protection bits by default")
commit 7228982442365 ("[MTD] m25p80: fix bug - ATmel spi flash fails to be copied to")

Actually, this might also fix handling of the Atmel AT25DF flashes,
because the original commit 7228982442365 ("[MTD] m25p80: fix bug -
ATmel spi flash fails to be copied to") was writing a 0 to the status
register, which is a "Global Unprotect". This might not be the case in
the current code which only handles the block protection bits BP2, BP1
and BP0. Thus, it depends on the current contents of the status register
if this unlock actually corresponds to a "Global Unprotect" command. In
the worst case, the current code might leave the AT25DF flashes in a
write protected state.

The commit 191f5c2ed4b6f ("mtd: spi-nor: use 16-bit WRR command when QE
is set on spansion flashes") changed that behaviour by just clearing BP2
to BP0 instead of writing a 0 to the status register.

Fixes: 191f5c2ed4b6f ("mtd: spi-nor: use 16-bit WRR command when QE is set on spansion flashes")
Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/mtd/spi-nor/spi-nor.c | 95 ++++++++++++++++++++++-------------
 include/linux/mtd/spi-nor.h   |  7 ++-
 2 files changed, 67 insertions(+), 35 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index addb6319fcbb..66dfb4396146 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -238,6 +238,11 @@ struct flash_info {
 					 * status register. Must be used with
 					 * SPI_NOR_HAS_TB.
 					 */
+#define SPI_NOR_UNPROTECT	BIT(17)	/*
+					 * Flash is write-protected after
+					 * power-up and needs a global
+					 * unprotect.
+					 */
 
 	/* Part specific fixup hooks. */
 	const struct spi_nor_fixups *fixups;
@@ -2318,15 +2323,15 @@ static const struct flash_info spi_nor_ids[] = {
 	{ "at25fs010",  INFO(0x1f6601, 0, 32 * 1024,   4, SECT_4K) },
 	{ "at25fs040",  INFO(0x1f6604, 0, 64 * 1024,   8, SECT_4K) },
 
-	{ "at25df041a", INFO(0x1f4401, 0, 64 * 1024,   8, SECT_4K) },
-	{ "at25df321",  INFO(0x1f4700, 0, 64 * 1024,  64, SECT_4K) },
-	{ "at25df321a", INFO(0x1f4701, 0, 64 * 1024,  64, SECT_4K) },
-	{ "at25df641",  INFO(0x1f4800, 0, 64 * 1024, 128, SECT_4K) },
+	{ "at25df041a", INFO(0x1f4401, 0, 64 * 1024,   8, SECT_4K | SPI_NOR_UNPROTECT) },
+	{ "at25df321",  INFO(0x1f4700, 0, 64 * 1024,  64, SECT_4K | SPI_NOR_UNPROTECT) },
+	{ "at25df321a", INFO(0x1f4701, 0, 64 * 1024,  64, SECT_4K | SPI_NOR_UNPROTECT) },
+	{ "at25df641",  INFO(0x1f4800, 0, 64 * 1024, 128, SECT_4K | SPI_NOR_UNPROTECT) },
 
-	{ "at26f004",   INFO(0x1f0400, 0, 64 * 1024,  8, SECT_4K) },
-	{ "at26df081a", INFO(0x1f4501, 0, 64 * 1024, 16, SECT_4K) },
-	{ "at26df161a", INFO(0x1f4601, 0, 64 * 1024, 32, SECT_4K) },
-	{ "at26df321",  INFO(0x1f4700, 0, 64 * 1024, 64, SECT_4K) },
+	{ "at26f004",   INFO(0x1f0400, 0, 64 * 1024,  8, SECT_4K | SPI_NOR_UNPROTECT) },
+	{ "at26df081a", INFO(0x1f4501, 0, 64 * 1024, 16, SECT_4K | SPI_NOR_UNPROTECT) },
+	{ "at26df161a", INFO(0x1f4601, 0, 64 * 1024, 32, SECT_4K | SPI_NOR_UNPROTECT) },
+	{ "at26df321",  INFO(0x1f4700, 0, 64 * 1024, 64, SECT_4K | SPI_NOR_UNPROTECT) },
 
 	{ "at45db081d", INFO(0x1f2500, 0, 64 * 1024, 16, SECT_4K) },
 
@@ -2348,9 +2353,9 @@ static const struct flash_info spi_nor_ids[] = {
 	{ "en25s64",	INFO(0x1c3817, 0, 64 * 1024,  128, SECT_4K) },
 
 	/* ESMT */
-	{ "f25l32pa", INFO(0x8c2016, 0, 64 * 1024, 64, SECT_4K | SPI_NOR_HAS_LOCK) },
-	{ "f25l32qa", INFO(0x8c4116, 0, 64 * 1024, 64, SECT_4K | SPI_NOR_HAS_LOCK) },
-	{ "f25l64qa", INFO(0x8c4117, 0, 64 * 1024, 128, SECT_4K | SPI_NOR_HAS_LOCK) },
+	{ "f25l32pa", INFO(0x8c2016, 0, 64 * 1024, 64, SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_UNPROTECT) },
+	{ "f25l32qa", INFO(0x8c4116, 0, 64 * 1024, 64, SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_UNPROTECT) },
+	{ "f25l64qa", INFO(0x8c4117, 0, 64 * 1024, 128, SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_UNPROTECT) },
 
 	/* Everspin */
 	{ "mr25h128", CAT25_INFO( 16 * 1024, 1, 256, 2, SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) },
@@ -2406,9 +2411,9 @@ static const struct flash_info spi_nor_ids[] = {
 	},
 
 	/* Intel/Numonyx -- xxxs33b */
-	{ "160s33b",  INFO(0x898911, 0, 64 * 1024,  32, 0) },
-	{ "320s33b",  INFO(0x898912, 0, 64 * 1024,  64, 0) },
-	{ "640s33b",  INFO(0x898913, 0, 64 * 1024, 128, 0) },
+	{ "160s33b",  INFO(0x898911, 0, 64 * 1024,  32, SPI_NOR_UNPROTECT) },
+	{ "320s33b",  INFO(0x898912, 0, 64 * 1024,  64, SPI_NOR_UNPROTECT) },
+	{ "640s33b",  INFO(0x898913, 0, 64 * 1024, 128, SPI_NOR_UNPROTECT) },
 
 	/* ISSI */
 	{ "is25cd512",  INFO(0x7f9d20, 0, 32 * 1024,   2, SECT_4K) },
@@ -2563,18 +2568,18 @@ static const struct flash_info spi_nor_ids[] = {
 	{ "s25fl256l",  INFO(0x016019,      0,  64 * 1024, 512, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
 
 	/* SST -- large erase sizes are "overlays", "sectors" are 4K */
-	{ "sst25vf040b", INFO(0xbf258d, 0, 64 * 1024,  8, SECT_4K | SST_WRITE) },
-	{ "sst25vf080b", INFO(0xbf258e, 0, 64 * 1024, 16, SECT_4K | SST_WRITE) },
-	{ "sst25vf016b", INFO(0xbf2541, 0, 64 * 1024, 32, SECT_4K | SST_WRITE) },
-	{ "sst25vf032b", INFO(0xbf254a, 0, 64 * 1024, 64, SECT_4K | SST_WRITE) },
-	{ "sst25vf064c", INFO(0xbf254b, 0, 64 * 1024, 128, SECT_4K) },
-	{ "sst25wf512",  INFO(0xbf2501, 0, 64 * 1024,  1, SECT_4K | SST_WRITE) },
-	{ "sst25wf010",  INFO(0xbf2502, 0, 64 * 1024,  2, SECT_4K | SST_WRITE) },
-	{ "sst25wf020",  INFO(0xbf2503, 0, 64 * 1024,  4, SECT_4K | SST_WRITE) },
+	{ "sst25vf040b", INFO(0xbf258d, 0, 64 * 1024,  8, SECT_4K | SST_WRITE | SPI_NOR_UNPROTECT) },
+	{ "sst25vf080b", INFO(0xbf258e, 0, 64 * 1024, 16, SECT_4K | SST_WRITE | SPI_NOR_UNPROTECT) },
+	{ "sst25vf016b", INFO(0xbf2541, 0, 64 * 1024, 32, SECT_4K | SST_WRITE | SPI_NOR_UNPROTECT) },
+	{ "sst25vf032b", INFO(0xbf254a, 0, 64 * 1024, 64, SECT_4K | SST_WRITE | SPI_NOR_UNPROTECT) },
+	{ "sst25vf064c", INFO(0xbf254b, 0, 64 * 1024, 128, SECT_4K | SPI_NOR_UNPROTECT) },
+	{ "sst25wf512",  INFO(0xbf2501, 0, 64 * 1024,  1, SECT_4K | SST_WRITE | SPI_NOR_UNPROTECT) },
+	{ "sst25wf010",  INFO(0xbf2502, 0, 64 * 1024,  2, SECT_4K | SST_WRITE | SPI_NOR_UNPROTECT) },
+	{ "sst25wf020",  INFO(0xbf2503, 0, 64 * 1024,  4, SECT_4K | SST_WRITE | SPI_NOR_UNPROTECT) },
 	{ "sst25wf020a", INFO(0x621612, 0, 64 * 1024,  4, SECT_4K) },
 	{ "sst25wf040b", INFO(0x621613, 0, 64 * 1024,  8, SECT_4K) },
-	{ "sst25wf040",  INFO(0xbf2504, 0, 64 * 1024,  8, SECT_4K | SST_WRITE) },
-	{ "sst25wf080",  INFO(0xbf2505, 0, 64 * 1024, 16, SECT_4K | SST_WRITE) },
+	{ "sst25wf040",  INFO(0xbf2504, 0, 64 * 1024,  8, SECT_4K | SST_WRITE | SPI_NOR_UNPROTECT) },
+	{ "sst25wf080",  INFO(0xbf2505, 0, 64 * 1024, 16, SECT_4K | SST_WRITE | SPI_NOR_UNPROTECT) },
 	{ "sst26wf016b", INFO(0xbf2651, 0, 64 * 1024, 32, SECT_4K |
 			      SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
 	{ "sst26vf064b", INFO(0xbf2643, 0, 64 * 1024, 128, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
@@ -4941,20 +4946,38 @@ static int spi_nor_quad_enable(struct spi_nor *nor)
 }
 
 /**
- * spi_nor_unlock_all() - Unlocks the entire flash memory array.
+ * spi_nor_global_unprotect() - Perform a global unprotect of the memory area.
  * @nor:	pointer to a 'struct spi_nor'.
  *
  * Some SPI NOR flashes are write protected by default after a power-on reset
  * cycle, in order to avoid inadvertent writes during power-up. Backward
  * compatibility imposes to unlock the entire flash memory array at power-up
- * by default.
+ * by default. Do it only for flashes where the block protection bits
+ * are volatile, this is indicated by SNOR_F_NEED_UNPROTECT.
+ *
+ * We cannot use spi_nor_unlock(nor->params.size) here because there are
+ * legacy devices (eg. AT25DF041A) which need a "global unprotect" command.
+ * This is done by writing 0b0x0000xx to the status register. This will also
+ * work for all other flashes which have these bits mapped to BP0 to BP3.
+ * The top most bit is ususally some kind of lock bit for the block
+ * protection bits.
  */
-static int spi_nor_unlock_all(struct spi_nor *nor)
+static int spi_nor_global_unprotect(struct spi_nor *nor)
 {
-	if (nor->flags & SNOR_F_HAS_LOCK)
-		return spi_nor_unlock(&nor->mtd, 0, nor->params.size);
+	int ret;
 
-	return 0;
+	dev_dbg(nor->dev, "unprotecting entire flash\n");
+	ret = spi_nor_read_sr(nor, nor->bouncebuf);
+	if (ret)
+		return ret;
+
+	nor->bouncebuf[0] &= ~SR_GLOBAL_UNPROTECT_MASK;
+
+	/*
+	 * Don't use spi_nor_write_sr1_and_check() because writing the status
+	 * register might fail if the flash is hardware write protected.
+	 */
+	return spi_nor_write_sr(nor, nor->bouncebuf, 1);
 }
 
 static int spi_nor_init(struct spi_nor *nor)
@@ -4967,10 +4990,12 @@ static int spi_nor_init(struct spi_nor *nor)
 		return err;
 	}
 
-	err = spi_nor_unlock_all(nor);
-	if (err) {
-		dev_dbg(nor->dev, "Failed to unlock the entire flash memory array\n");
-		return err;
+	if (nor->flags & SNOR_F_NEED_UNPROTECT) {
+		err = spi_nor_global_unprotect(nor);
+		if (err) {
+			dev_err(nor->dev, "global unprotect failed\n");
+			return err;
+		}
 	}
 
 	if (nor->addr_width == 4 && !(nor->flags & SNOR_F_4B_OPCODES)) {
@@ -5193,6 +5218,8 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
 		nor->flags |= SNOR_F_NO_OP_CHIP_ERASE;
 	if (info->flags & USE_CLSR)
 		nor->flags |= SNOR_F_USE_CLSR;
+	if (info->flags & SPI_NOR_UNPROTECT)
+		nor->flags |= SNOR_F_NEED_UNPROTECT;
 
 	if (info->flags & SPI_NOR_NO_ERASE)
 		mtd->flags |= MTD_NO_ERASE;
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 7e32adce72f7..89c96c943172 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -137,6 +137,11 @@
 
 #define SR1_QUAD_EN_BIT6	BIT(6)
 
+/* Global unprotect is performed by writing the 0b0x0000xx to the status
+ * register.
+ */
+#define SR_GLOBAL_UNPROTECT_MASK 0xbc
+
 /* Enhanced Volatile Configuration Register bits */
 #define EVCR_QUAD_EN_MICRON	BIT(7)	/* Micron Quad I/O */
 
@@ -246,7 +251,7 @@ enum spi_nor_option_flags {
 	SNOR_F_HAS_16BIT_SR	= BIT(9),
 	SNOR_F_NO_READ_CR	= BIT(10),
 	SNOR_F_HAS_SR_TB_BIT6	= BIT(11),
-
+	SNOR_F_NEED_UNPROTECT	= BIT(12),
 };
 
 /**
-- 
2.20.1


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

Re: [PATCH v2] mtd: spi-nor: keep lock bits if they are non-volatile

[Tudor.Ambarus]

Hi, Michael,

On Saturday, January 4, 2020 12:12:29 AM EET Michael Walle wrote:
> Traditionally, linux unlocks the whole flash because there are legacy
> devices which has the write protections bits set by default at startup.
> If you actually want to use the flash protection bits, eg. because there
> is a read-only part for a bootloader, this automatic unlocking is
> harmful. If there is no hardware write protection in place (usually
> called WP#), a startup of the kernel just discards this protection.
> 
> I've gone through the datasheets of all the flashes (except the Intel
> ones where I could not find any datasheet nor reference) which supports
> the unlocking feature and looked how the sector protection was
> implemented. The currently supported flashes can be divided into the
> following two categories:
>  (1) block protection bits are non-volatile. Thus they keep their values
>      at reset and power-cycle
>  (2) flashes where these bits are volatile. After reset or power-cycle,
>      the whole memory array is protected.
>      (a) some devices needs a special "Global Unprotect" command, eg.
>          the Atmel AT25DF041A.
>      (b) some devices require to clear the BPn bits in the status
>          register.
> 
> Due to the reasons above, we do not want to clear the bits for flashes
> which belong to category (1). Fortunately for us, the flashes in (2a)
> and (2b) are compatible with each other in a sense that the "Global
> Unprotect" command will clear the block protection bits in all the (2b)
> flashes.
> 
> This patch adds a new flag to indicate the case (2). Only if we have
> such a flash we perform a "Global Unprotect". Hopefully, this will clean
> up "unlock the entire flash for legacy devices" once and for all.

Thanks for the detailed explanation. Unlocking the flash at probe time was 
badly designed from the beginning, we should disable the write protection only 
on request, to avoid destructive commands during power-up.

Breaking the backward compatibility is a no-go, and looks like you break it, 
by not treating case (1). We can indeed continue your idea and treat both (1) 
and (2), thus disabling the write protection at power-up for all the flashes 
that we support as of now (in order to not break backward compat), and to not 
disable the block protection for the new flashes that will come. This means to 
have some point in time before which some less fortunate flashes don't benefit 
of write protection at power-up, and after which the others benefit. I 
wouldn't got this way, I prefer a generic method that handles all the flashes 
in the same way.

I see three choices:
1/ dt prop which gives a per flash granularity. The prop is related to hw 
protection and there might be some chances to get this accepted, maybe it is 
worth to involve Rob. But I tend to share Vignesh's opinion, this would 
configure the flash and not describe it.

2/ kconfig option, the behavior would be enforced on all the flashes. It would 
be similar to what we have with CONFIG_MTD_SPI_NOR_USE_4K_SECTORS. I did a 
patch to address this some time ago: https://patchwork.ozlabs.org/patch/
1133278/

3/ module param, the behavior would be enforced on all the flashes.

Preferences or suggestions?

Cheers,
ta



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

Re: [PATCH v2] mtd: spi-nor: keep lock bits if they are non-volatile

[Michael Walle]

Hi Tudor,

thanks for looking into this.

Am 2020-01-11 14:46, schrieb Tudor.Ambarus@microchip.com:
> Hi, Michael,
> 
> On Saturday, January 4, 2020 12:12:29 AM EET Michael Walle wrote:
>> Traditionally, linux unlocks the whole flash because there are legacy
>> devices which has the write protections bits set by default at 
>> startup.
>> If you actually want to use the flash protection bits, eg. because 
>> there
>> is a read-only part for a bootloader, this automatic unlocking is
>> harmful. If there is no hardware write protection in place (usually
>> called WP#), a startup of the kernel just discards this protection.
>> 
>> I've gone through the datasheets of all the flashes (except the Intel
>> ones where I could not find any datasheet nor reference) which 
>> supports
>> the unlocking feature and looked how the sector protection was
>> implemented. The currently supported flashes can be divided into the
>> following two categories:
>>  (1) block protection bits are non-volatile. Thus they keep their 
>> values
>>      at reset and power-cycle
>>  (2) flashes where these bits are volatile. After reset or 
>> power-cycle,
>>      the whole memory array is protected.
>>      (a) some devices needs a special "Global Unprotect" command, eg.
>>          the Atmel AT25DF041A.
>>      (b) some devices require to clear the BPn bits in the status
>>          register.
>> 
>> Due to the reasons above, we do not want to clear the bits for flashes
>> which belong to category (1). Fortunately for us, the flashes in (2a)
>> and (2b) are compatible with each other in a sense that the "Global
>> Unprotect" command will clear the block protection bits in all the 
>> (2b)
>> flashes.
>> 
>> This patch adds a new flag to indicate the case (2). Only if we have
>> such a flash we perform a "Global Unprotect". Hopefully, this will 
>> clean
>> up "unlock the entire flash for legacy devices" once and for all.
> 
> Thanks for the detailed explanation. Unlocking the flash at probe time 
> was
> badly designed from the beginning, we should disable the write 
> protection only
> on request, to avoid destructive commands during power-up.
> 
> Breaking the backward compatibility is a no-go, and looks like you 
> break it,
> by not treating case (1).

Yes but that was the whole idea of this patch. So if I get you correct 
it is
not possible to change that even if:

(1) it was never intended that way. Eg. the original patch(es) were 
about
removing the volatile write protection (which makes perfectly sense, 
even
during probe time) to be able to write to the flash. But it was never 
intended
to disable the non-volatile write protection.

(2) it might be even harmful. It is still an open question wether the 
write
to the non-volatile bits (even if it is the same value) might wear them 
out.
Unfortunately our FAE didn't answered yet..

(3) it makes the write protection utterly useless, because if you lock 
the
flash it will be automatically unlocked after the next reboot. Even 
worse, the
user likely won't notice it.


> We can indeed continue your idea and treat both (1)
> and (2), thus disabling the write protection at power-up for all the 
> flashes
> that we support as of now (in order to not break backward compat), and 
> to not
> disable the block protection for the new flashes that will come. This 
> means to
> have some point in time before which some less fortunate flashes don't 
> benefit
> of write protection at power-up, and after which the others benefit. I
> wouldn't got this way, I prefer a generic method that handles all the 
> flashes
> in the same way.

I'd also prefer a solution for all existing flashes. But it seems that 
the rule
above makes that almost impossible. Esp. this behaviour will then be 
there until
eternity.

> I see three choices:
> 1/ dt prop which gives a per flash granularity. The prop is related to 
> hw
> protection and there might be some chances to get this accepted, maybe 
> it is
> worth to involve Rob. But I tend to share Vignesh's opinion, this would
> configure the flash and not describe it.

Still my preferred way. but also see below. But I wouldn't say it 
configures the
flash but describe that the user want to use the write protection.

> 2/ kconfig option, the behavior would be enforced on all the flashes. 
> It would
> be similar to what we have with CONFIG_MTD_SPI_NOR_USE_4K_SECTORS. I 
> did a
> patch to address this some time ago: 
> https://patchwork.ozlabs.org/patch/
> 1133278/

Mhh. If we would combine this with this patch that would be at least a 
step into
the right direction. At least a distro could enable that kernel option 
without
breaking old boards/flashes. Because as outlined about you need that for 
flashes
in category (2). Or you'd have to do a flash_unlock every time you want 
to write
to it. But that would be really a backwards incompatible change.. ;)

> 
> 3/ module param, the behavior would be enforced on all the flashes.
> 
> Preferences or suggestions?

-michael

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

Re: [PATCH v2] mtd: spi-nor: keep lock bits if they are non-volatile

[Tudor.Ambarus]

Hi, Michael, Vignesh,

On Sunday, January 12, 2020 12:50:57 AM EET Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> Hi Tudor,
> 
> thanks for looking into this.
> 
> Am 2020-01-11 14:46, schrieb Tudor.Ambarus@microchip.com:
> > Hi, Michael,
> > 
> > On Saturday, January 4, 2020 12:12:29 AM EET Michael Walle wrote:
> >> Traditionally, linux unlocks the whole flash because there are legacy
> >> devices which has the write protections bits set by default at
> >> startup.
> >> If you actually want to use the flash protection bits, eg. because
> >> there
> >> is a read-only part for a bootloader, this automatic unlocking is
> >> harmful. If there is no hardware write protection in place (usually
> >> called WP#), a startup of the kernel just discards this protection.
> >> 
> >> I've gone through the datasheets of all the flashes (except the Intel
> >> ones where I could not find any datasheet nor reference) which
> >> supports
> >> the unlocking feature and looked how the sector protection was
> >> implemented. The currently supported flashes can be divided into the
> >> 
> >> following two categories:
> >>  (1) block protection bits are non-volatile. Thus they keep their
> >> 
> >> values
> >> 
> >>      at reset and power-cycle
> >>  
> >>  (2) flashes where these bits are volatile. After reset or
> >> 
> >> power-cycle,
> >> 
> >>      the whole memory array is protected.
> >>      (a) some devices needs a special "Global Unprotect" command, eg.
> >>      
> >>          the Atmel AT25DF041A.
> >>      
> >>      (b) some devices require to clear the BPn bits in the status
> >>      
> >>          register.
> >> 
> >> Due to the reasons above, we do not want to clear the bits for flashes
> >> which belong to category (1). Fortunately for us, the flashes in (2a)
> >> and (2b) are compatible with each other in a sense that the "Global
> >> Unprotect" command will clear the block protection bits in all the
> >> (2b)
> >> flashes.
> >> 
> >> This patch adds a new flag to indicate the case (2). Only if we have
> >> such a flash we perform a "Global Unprotect". Hopefully, this will
> >> clean
> >> up "unlock the entire flash for legacy devices" once and for all.
> > 
> > Thanks for the detailed explanation. Unlocking the flash at probe time
> > was
> > badly designed from the beginning, we should disable the write
> > protection only
> > on request, to avoid destructive commands during power-up.
> > 
> > Breaking the backward compatibility is a no-go, and looks like you
> > break it,
> > by not treating case (1).
> 
> Yes but that was the whole idea of this patch. So if I get you correct
> it is
> not possible to change that even if:
> 
> (1) it was never intended that way. Eg. the original patch(es) were
> about
> removing the volatile write protection (which makes perfectly sense,
> even
> during probe time) to be able to write to the flash. But it was never
> intended
> to disable the non-volatile write protection.
> 
> (2) it might be even harmful. It is still an open question wether the
> write
> to the non-volatile bits (even if it is the same value) might wear them
> out.
> Unfortunately our FAE didn't answered yet..
> 
> (3) it makes the write protection utterly useless, because if you lock
> the
> flash it will be automatically unlocked after the next reboot. Even
> worse, the
> user likely won't notice it.

Breaking backward compatibility and keeping the locking state of the spi-nor 
flashes at probe is a no-go, because there might be user space apps that 
expect that all the spi-nor flashes are by default unlocked. The unlocking of 
the flash at probe time was introduced 12 years ago, we definitely can't 
change this now.

> 
> > We can indeed continue your idea and treat both (1)
> > and (2), thus disabling the write protection at power-up for all the
> > flashes
> > that we support as of now (in order to not break backward compat), and
> > to not
> > disable the block protection for the new flashes that will come. This
> > means to
> > have some point in time before which some less fortunate flashes don't
> > benefit
> > of write protection at power-up, and after which the others benefit. I
> > wouldn't got this way, I prefer a generic method that handles all the
> > flashes
> > in the same way.
> 
> I'd also prefer a solution for all existing flashes. But it seems that
> the rule
> above makes that almost impossible. Esp. this behaviour will then be
> there until
> eternity.
> 
> > I see three choices:
> > 1/ dt prop which gives a per flash granularity. The prop is related to
> > hw
> > protection and there might be some chances to get this accepted, maybe
> > it is
> > worth to involve Rob. But I tend to share Vignesh's opinion, this would
> > configure the flash and not describe it.
> 
> Still my preferred way. but also see below. But I wouldn't say it

Try to convince Rob.

> configures the
> flash but describe that the user want to use the write protection.
> 
> > 2/ kconfig option, the behavior would be enforced on all the flashes.
> > It would
> > be similar to what we have with CONFIG_MTD_SPI_NOR_USE_4K_SECTORS. I
> > did a
> > patch to address this some time ago:
> > https://patchwork.ozlabs.org/patch/
> > 1133278/
> 
> Mhh. If we would combine this with this patch that would be at least a
> step into
> the right direction. At least a distro could enable that kernel option
> without
> breaking old boards/flashes. Because as outlined about you need that for
> flashes
> in category (2). Or you'd have to do a flash_unlock every time you want
> to write
> to it. But that would be really a backwards incompatible change.. ;)
> 
> > 3/ module param, the behavior would be enforced on all the flashes.
> > 
> > Preferences or suggestions?
> 
I would go with 2/ or 3/. Vignesh, what do you prefer and why?

Cheers,
ta




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

Re: [PATCH v2] mtd: spi-nor: keep lock bits if they are non-volatile

[Tudor.Ambarus]

Michael,

To be more explicit:

On Tuesday, January 21, 2020 8:53:20 PM EET Tudor Ambarus wrote:
> Yes but that was the whole idea of this patch. So if I get you correct
> 
> > it is
> > not possible to change that even if:
> > 
> > (1) it was never intended that way. Eg. the original patch(es) were
> > about
> > removing the volatile write protection (which makes perfectly sense,
> > even
> > during probe time) to be able to write to the flash. But it was never
> > intended
> > to disable the non-volatile write protection.

Even if this is true, we can't break backward compat.

> > 
> > (2) it might be even harmful. It is still an open question wether the
> > write
> > to the non-volatile bits (even if it is the same value) might wear them
> > out.
> > Unfortunately our FAE didn't answered yet..
> > 

We'll think about this when we know for sure.

> > (3) it makes the write protection utterly useless, because if you lock
> > the
> > flash it will be automatically unlocked after the next reboot. Even
> > worse, the
> > user likely won't notice it.

Even if this is true, we can't break backward compat.

> 
> Breaking backward compatibility and keeping the locking state of the spi-nor
> flashes at probe is a no-go, because there might be user space apps that
> expect that all the spi-nor flashes are by default unlocked. The unlocking
> of the flash at probe time was introduced 12 years ago, we definitely can't
> change this now.

Kconfig option or module param will fix this without breaking backward compat, 
we should focus on this direction.

Cheers,
ta


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

Re: [PATCH v2] mtd: spi-nor: keep lock bits if they are non-volatile

[Vignesh Raghavendra]

On 22/01/20 12:23 am, Tudor.Ambarus@microchip.com wrote:
> Hi, Michael, Vignesh,
> 
> On Sunday, January 12, 2020 12:50:57 AM EET Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
>> content is safe
[...]

>>> I see three choices:
>>> 1/ dt prop which gives a per flash granularity. The prop is related to
>>> hw
>>> protection and there might be some chances to get this accepted, maybe
>>> it is
>>> worth to involve Rob. But I tend to share Vignesh's opinion, this would
>>> configure the flash and not describe it.
>>
>> Still my preferred way. but also see below. But I wouldn't say it
> 
> Try to convince Rob.
> 
>> configures the
>> flash but describe that the user want to use the write protection.
>>
>>> 2/ kconfig option, the behavior would be enforced on all the flashes.
>>> It would
>>> be similar to what we have with CONFIG_MTD_SPI_NOR_USE_4K_SECTORS. I
>>> did a
>>> patch to address this some time ago:
>>> https://patchwork.ozlabs.org/patch/
>>> 1133278/
>>
>> Mhh. If we would combine this with this patch that would be at least a
>> step into
>> the right direction. At least a distro could enable that kernel option
>> without
>> breaking old boards/flashes. Because as outlined about you need that for
>> flashes
>> in category (2). Or you'd have to do a flash_unlock every time you want
>> to write
>> to it. But that would be really a backwards incompatible change.. ;)
>>
>>> 3/ module param, the behavior would be enforced on all the flashes.
>>>
>>> Preferences or suggestions?
>>
> I would go with 2/ or 3/. Vignesh, what do you prefer and why?
> 

I dont like option 1, because I am not convinced that this is a HW
description to be put in DT.  IIUC, problem is more of what to do with
locking configuration that is done before Linux comes up(either in
previous boot or by bootloader or POR default). Current code just
discards it and unlocks entire flash.
But proposal is not to touch those bits at probe time and leave this
upto userspace to handle.

Adding a Kconfig does not scale well for multi-platform builds. There
would not be a way to have protection enabled on one platform but
disabled on other. Does not scale for multiple flashes either

Option 3 sounds least bad among all. If module param can be designed to
be a string then, we could control locking behavior to be per flash
using flash name.


-- 
Regards
Vignesh

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

Re: [PATCH v2] mtd: spi-nor: keep lock bits if they are non-volatile

[Michael Walle]

Hi Vignesh,

Am 2020-01-22 13:10, schrieb Vignesh Raghavendra:
> On 22/01/20 12:23 am, Tudor.Ambarus@microchip.com wrote:
>> Hi, Michael, Vignesh,
>> 
>> On Sunday, January 12, 2020 12:50:57 AM EET Michael Walle wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you 
>>> know the
>>> content is safe
> [...]
> 
>>>> I see three choices:
>>>> 1/ dt prop which gives a per flash granularity. The prop is related 
>>>> to
>>>> hw
>>>> protection and there might be some chances to get this accepted, 
>>>> maybe
>>>> it is
>>>> worth to involve Rob. But I tend to share Vignesh's opinion, this 
>>>> would
>>>> configure the flash and not describe it.
>>> 
>>> Still my preferred way. but also see below. But I wouldn't say it
>> 
>> Try to convince Rob.
>> 
>>> configures the
>>> flash but describe that the user want to use the write protection.
>>> 
>>>> 2/ kconfig option, the behavior would be enforced on all the 
>>>> flashes.
>>>> It would
>>>> be similar to what we have with CONFIG_MTD_SPI_NOR_USE_4K_SECTORS. I
>>>> did a
>>>> patch to address this some time ago:
>>>> https://patchwork.ozlabs.org/patch/
>>>> 1133278/
>>> 
>>> Mhh. If we would combine this with this patch that would be at least 
>>> a
>>> step into
>>> the right direction. At least a distro could enable that kernel 
>>> option
>>> without
>>> breaking old boards/flashes. Because as outlined about you need that 
>>> for
>>> flashes
>>> in category (2). Or you'd have to do a flash_unlock every time you 
>>> want
>>> to write
>>> to it. But that would be really a backwards incompatible change.. ;)
>>> 
>>>> 3/ module param, the behavior would be enforced on all the flashes.
>>>> 
>>>> Preferences or suggestions?
>>> 
>> I would go with 2/ or 3/. Vignesh, what do you prefer and why?
>> 
> 
> I dont like option 1, because I am not convinced that this is a HW
> description to be put in DT.  IIUC, problem is more of what to do with
> locking configuration that is done before Linux comes up(either in
> previous boot or by bootloader or POR default). Current code just
> discards it and unlocks entire flash.

But this is not the main problem. It is rather the intention of the
user to actually want write protect the flash (for flashes who has
proper support for them, that is the ones which have non-volatile
bits).

Flashes with volatile bits are another subject. Here it might be useful
to unlock them either at probe time or when we first write to them, so
the user doesn't need to know if its this kind of flash and he would
actually have to unlock the flash before writing. I've left the
behaviour for these flashes as it was before.

And yes, u-boot suffers from the same problem, eg. it unlocks the whole
flash too. I guess they inherited the behaviour from linux. But I
wanted to start with linux first.

> But proposal is not to touch those bits at probe time and leave this
> upto userspace to handle.

No, my proposal was to divide the flashes into two categories. The
unlocking is only done on the flashes which have volatile locking bits,
thus even when the new option is enabled it won't break access to these
flashes.

> Adding a Kconfig does not scale well for multi-platform builds. There
> would not be a way to have protection enabled on one platform but
> disabled on other. Does not scale for multiple flashes either
> 
> Option 3 sounds least bad among all. If module param can be designed to
> be a string then, we could control locking behavior to be per flash
> using flash name.

What about both? A kconfig option which defines the default for the
kernel parameter? My fear is that once it is a kernel parameter it is
easy to forget (thus having the non-volatile bits, the flash is
completely unlocked again) and it is not very handy; for proper write
protection you'd always have to have this parameter.

btw. I don't see a need to have this option per flash, because once
the user actually enables it, he is aware that its for all of his
flashes. I haven't seen flashes which has non-volatile protection bits
_and_ are protected by default. There shouldn't be a noticable
difference for the user if the option when enabled.

-michael

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

Re: [PATCH v2] mtd: spi-nor: keep lock bits if they are non-volatile

[Vignesh Raghavendra]

On 1/22/2020 6:14 PM, Michael Walle wrote:
> Hi Vignesh,
> 
> Am 2020-01-22 13:10, schrieb Vignesh Raghavendra:
>> On 22/01/20 12:23 am, Tudor.Ambarus@microchip.com wrote:
>>> Hi, Michael, Vignesh,
>>>
>>> On Sunday, January 12, 2020 12:50:57 AM EET Michael Walle wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you
>>>> know the
>>>> content is safe
>> [...]
>>

[...]

>>>>> Preferences or suggestions?
>>>>
>>> I would go with 2/ or 3/. Vignesh, what do you prefer and why?
>>>
>>
>> I dont like option 1, because I am not convinced that this is a HW
>> description to be put in DT.  IIUC, problem is more of what to do with
>> locking configuration that is done before Linux comes up(either in
>> previous boot or by bootloader or POR default). Current code just
>> discards it and unlocks entire flash.
> 
> But this is not the main problem. It is rather the intention of the
> user to actually want write protect the flash (for flashes who has
> proper support for them, that is the ones which have non-volatile
> bits).
> 
> Flashes with volatile bits are another subject. Here it might be useful
> to unlock them either at probe time or when we first write to them, so
> the user doesn't need to know if its this kind of flash and he would
> actually have to unlock the flash before writing. I've left the
> behaviour for these flashes as it was before.
> 
> And yes, u-boot suffers from the same problem, eg. it unlocks the whole
> flash too. I guess they inherited the behaviour from linux. But I
> wanted to start with linux first.
> 

U-Boot only unlocks entire flash in case of Atmel or SST or Intel.

>> But proposal is not to touch those bits at probe time and leave this
>> upto userspace to handle.
> 
> No, my proposal was to divide the flashes into two categories. The
> unlocking is only done on the flashes which have volatile locking bits,
> thus even when the new option is enabled it won't break access to these
> flashes.
> 

Hmm, looks like before commit 3e0930f109e7 ("mtd: spi-nor: Rework the
disabling of block write protection") global unlock was being done only
for Atmel, SST and Intel flashes. So 3e0930f109e7 definitely changes
this behavior to unlock all flashes that have SPI_NOR_HAS_LOCK set (in
addition to Atmel,SST and Intel).
I think we should just revert to the behavior before 3e0930f109e7 so as
not to break userspace expectation of preserving non volatile BP setting
across boots

Are we sure there are no Atmel and SST flashes that have non-volatile BP
bits? If so, then I have no objection for this patch as this effectively
reverts back to behavior before 3e0930f109e7.

Regards
Vignesh

>> Adding a Kconfig does not scale well for multi-platform builds. There
>> would not be a way to have protection enabled on one platform but
>> disabled on other. Does not scale for multiple flashes either
>>
>> Option 3 sounds least bad among all. If module param can be designed to
>> be a string then, we could control locking behavior to be per flash
>> using flash name.
> 
> What about both? A kconfig option which defines the default for the
> kernel parameter? My fear is that once it is a kernel parameter it is
> easy to forget (thus having the non-volatile bits, the flash is
> completely unlocked again) and it is not very handy; for proper write
> protection you'd always have to have this parameter.
> 
> btw. I don't see a need to have this option per flash, because once
> the user actually enables it, he is aware that its for all of his
> flashes. I haven't seen flashes which has non-volatile protection bits
> _and_ are protected by default. There shouldn't be a noticable
> difference for the user if the option when enabled.
> 
> -michael

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

Re: [PATCH v2] mtd: spi-nor: keep lock bits if they are non-volatile

[Michael Walle]

Am 2020-01-23 18:20, schrieb Vignesh Raghavendra:
> On 1/22/2020 6:14 PM, Michael Walle wrote:
>> Hi Vignesh,
>> 
>> Am 2020-01-22 13:10, schrieb Vignesh Raghavendra:
>>> On 22/01/20 12:23 am, Tudor.Ambarus@microchip.com wrote:
>>>> Hi, Michael, Vignesh,
>>>> 
>>>> On Sunday, January 12, 2020 12:50:57 AM EET Michael Walle wrote:
>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you
>>>>> know the
>>>>> content is safe
>>> [...]
>>> 
> 
> [...]
> 
>>>>>> Preferences or suggestions?
>>>>> 
>>>> I would go with 2/ or 3/. Vignesh, what do you prefer and why?
>>>> 
>>> 
>>> I dont like option 1, because I am not convinced that this is a HW
>>> description to be put in DT.  IIUC, problem is more of what to do 
>>> with
>>> locking configuration that is done before Linux comes up(either in
>>> previous boot or by bootloader or POR default). Current code just
>>> discards it and unlocks entire flash.
>> 
>> But this is not the main problem. It is rather the intention of the
>> user to actually want write protect the flash (for flashes who has
>> proper support for them, that is the ones which have non-volatile
>> bits).
>> 
>> Flashes with volatile bits are another subject. Here it might be 
>> useful
>> to unlock them either at probe time or when we first write to them, so
>> the user doesn't need to know if its this kind of flash and he would
>> actually have to unlock the flash before writing. I've left the
>> behaviour for these flashes as it was before.
>> 
>> And yes, u-boot suffers from the same problem, eg. it unlocks the 
>> whole
>> flash too. I guess they inherited the behaviour from linux. But I
>> wanted to start with linux first.
>> 
> 
> U-Boot only unlocks entire flash in case of Atmel or SST or Intel.

well for now.. until someone thinks its a good idea to follow linux
behaviour.

>>> But proposal is not to touch those bits at probe time and leave this
>>> upto userspace to handle.
>> 
>> No, my proposal was to divide the flashes into two categories. The
>> unlocking is only done on the flashes which have volatile locking 
>> bits,
>> thus even when the new option is enabled it won't break access to 
>> these
>> flashes.
>> 
> 
> Hmm, looks like before commit 3e0930f109e7 ("mtd: spi-nor: Rework the
> disabling of block write protection") global unlock was being done only
> for Atmel, SST and Intel flashes. So 3e0930f109e7 definitely changes
> this behavior to unlock all flashes that have SPI_NOR_HAS_LOCK set (in
> addition to Atmel,SST and Intel).
> I think we should just revert to the behavior before 3e0930f109e7 so as
> not to break userspace expectation of preserving non volatile BP 
> setting
> across boots
> 
> Are we sure there are no Atmel and SST flashes that have non-volatile 
> BP
> bits? If so, then I have no objection for this patch as this 
> effectively
> reverts back to behavior before 3e0930f109e7.

I've gone through all the flashes which were supported (at that time), 
all
which have non-volatile have the SPI_NOR_UNPROTECT flag. Also some ESMT
parts have non-volatile flags.

But this patch doesn't restore the state as of before 3e0930f109e7. 
Before
this commit any Atmel, Intel and SST flashes were unlocked. With this
patch only the ones are unlocked which have volatile bits.

I'll send a v3 which introduces a new kernel config option with three
choices:
  (1) Disable WP on any flashes (legacy behaviour)
  (2) Disable WP on flashes w/ volatile protection bits
  (3) Keep write protection as is

(1) is enabled by default. One should choose (2) though, because that is
the sane thing to do and (3) is for completeness and the special case if
anyone will actually keep the write protection on flashes with volatile
bits.

I still don't think a kernel parameter is a good idea because you may
miss it and end up with your flash being unprotected.

And i still hope that sometime, (2) may be the default..

-michael


> 
> Regards
> Vignesh
> 
>>> Adding a Kconfig does not scale well for multi-platform builds. There
>>> would not be a way to have protection enabled on one platform but
>>> disabled on other. Does not scale for multiple flashes either
>>> 
>>> Option 3 sounds least bad among all. If module param can be designed 
>>> to
>>> be a string then, we could control locking behavior to be per flash
>>> using flash name.
>> 
>> What about both? A kconfig option which defines the default for the
>> kernel parameter? My fear is that once it is a kernel parameter it is
>> easy to forget (thus having the non-volatile bits, the flash is
>> completely unlocked again) and it is not very handy; for proper write
>> protection you'd always have to have this parameter.
>> 
>> btw. I don't see a need to have this option per flash, because once
>> the user actually enables it, he is aware that its for all of his
>> flashes. I haven't seen flashes which has non-volatile protection bits
>> _and_ are protected by default. There shouldn't be a noticable
>> difference for the user if the option when enabled.
>> 
>> -michael

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