| Message ID | 20190129220705.5143-2-jonas@norrbonn.se |
|---|---|
| State | Changes Requested |
| Delegated to: | Ambarus Tudor |
| Headers | show |
| Series | spi-nor block protection | expand |
+ James and Yong from Cypress. Hi, Jonas, James, Yong, On 01/30/2019 12:07 AM, Jonas Bonn wrote: > The status register bit SRWD (status register write disable) is > described in many words in the datasheets but effectively boils down to: > > i) if set, respect WP# when trying to change protection bits; > ii) if unset, ignore WP# when trying to change protection bits > > In short, the bit determines whether the WP# signal is honored or not. > > It's difficult to imagine the use-case where the WP# is connected and > asserted but the user doesn't want to respect its setting. As such, > this patch sets the SRWD bit unconditionally so that the WP# is _always_ > respected; hardware that doesn't care about WP# normally won't even have > it connected. > Always setting SRWD to 1, dismisses the need of a SRWD bit in the first place. Do you know why Cypress made this bit configurable and didn't enable it by default? Cheers, ta > Tested on a Cypress s25fl512s. With this patch, the WP# is always > respected, irregardless of whether any flash protection bits are set. > > Signed-off-by: Jonas Bonn <jonas@norrbonn.se> > CC: Marek Vasut <marek.vasut@gmail.com> > CC: David Woodhouse <dwmw2@infradead.org> > CC: Brian Norris <computersforpeace@gmail.com> > CC: Boris Brezillon <bbrezillon@kernel.org> > CC: Richard Weinberger <richard@nod.at> > CC: linux-mtd@lists.infradead.org > --- > drivers/mtd/spi-nor/spi-nor.c | 17 +++++++++-------- > 1 file changed, 9 insertions(+), 8 deletions(-) > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > index 13a5055e5f3f..4c8ce2b90838 100644 > --- a/drivers/mtd/spi-nor/spi-nor.c > +++ b/drivers/mtd/spi-nor/spi-nor.c > @@ -1231,9 +1231,6 @@ static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len) > > status_new = (status_old & ~mask & ~SR_TB) | val; > > - /* Disallow further writes if WP pin is asserted */ > - status_new |= SR_SRWD; > - > if (!use_top) > status_new |= SR_TB; > > @@ -1313,10 +1310,6 @@ static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len) > > status_new = (status_old & ~mask & ~SR_TB) | val; > > - /* Don't protect status register if we're fully unlocked */ > - if (lock_len == 0) > - status_new &= ~SR_SRWD; > - > if (!use_top) > status_new |= SR_TB; > > @@ -3898,6 +3891,7 @@ static int spi_nor_setup(struct spi_nor *nor, > static int spi_nor_init(struct spi_nor *nor) > { > int err; > + int sr; > > /* > * Atmel, SST, Intel/Numonyx, and others serial NOR tend to power up > @@ -3933,7 +3927,14 @@ static int spi_nor_init(struct spi_nor *nor) > set_4byte(nor, true); > } > > - return 0; > + /* Always respect the WP# (write-protect) input */ > + sr = read_sr(nor); > + if (sr < 0) { > + dev_err(nor->dev, "error while reading status register\n"); > + return -EINVAL; > + } > + sr |= SR_SRWD; > + return write_sr_and_check(nor, sr, SR_SRWD); > } > > /* mtd resume handler */ >
Hi, On 10/03/2019 10:58, Tudor.Ambarus@microchip.com wrote: > + James and Yong from Cypress. > > Hi, Jonas, James, Yong, > > On 01/30/2019 12:07 AM, Jonas Bonn wrote: >> The status register bit SRWD (status register write disable) is >> described in many words in the datasheets but effectively boils down to: >> >> i) if set, respect WP# when trying to change protection bits; >> ii) if unset, ignore WP# when trying to change protection bits >> >> In short, the bit determines whether the WP# signal is honored or not. >> >> It's difficult to imagine the use-case where the WP# is connected and >> asserted but the user doesn't want to respect its setting. As such, >> this patch sets the SRWD bit unconditionally so that the WP# is _always_ >> respected; hardware that doesn't care about WP# normally won't even have >> it connected. >> > > Always setting SRWD to 1, dismisses the need of a SRWD bit in the first place. > Do you know why Cypress made this bit configurable and didn't enable it by default? I suspect it has some historical relevance; as things currently stand, I don't see how it's useful in its current form. We initially thought that the WP# signal turned on/off block protection but that's not the case; the block protection bits BP0-BP2 turn on/off block protection and the WP# signal only protects the BP[0-2] bits from being modified. Turning off the WP# functionality via the SRWD bits means that the BP[0-2] bits are always modifiable and therefore the flash device is never really protected from writes by a malicious actor. The key question here is: if the WP# signal is connected, why would you ever want its state to be ignored by the device? De-asserting the signal has the same effect as setting the SRWD bit. And the default state is un-asserted if the signal is not connected, so that case is covered, too. I see no reason not to just always set SRWD and thereby make the WP# signal do what one expects of it, always. /Jonas > > Cheers, > ta > >> Tested on a Cypress s25fl512s. With this patch, the WP# is always >> respected, irregardless of whether any flash protection bits are set. >> >> Signed-off-by: Jonas Bonn <jonas@norrbonn.se> >> CC: Marek Vasut <marek.vasut@gmail.com> >> CC: David Woodhouse <dwmw2@infradead.org> >> CC: Brian Norris <computersforpeace@gmail.com> >> CC: Boris Brezillon <bbrezillon@kernel.org> >> CC: Richard Weinberger <richard@nod.at> >> CC: linux-mtd@lists.infradead.org >> --- >> drivers/mtd/spi-nor/spi-nor.c | 17 +++++++++-------- >> 1 file changed, 9 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c >> index 13a5055e5f3f..4c8ce2b90838 100644 >> --- a/drivers/mtd/spi-nor/spi-nor.c >> +++ b/drivers/mtd/spi-nor/spi-nor.c >> @@ -1231,9 +1231,6 @@ static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len) >> >> status_new = (status_old & ~mask & ~SR_TB) | val; >> >> - /* Disallow further writes if WP pin is asserted */ >> - status_new |= SR_SRWD; >> - >> if (!use_top) >> status_new |= SR_TB; >> >> @@ -1313,10 +1310,6 @@ static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len) >> >> status_new = (status_old & ~mask & ~SR_TB) | val; >> >> - /* Don't protect status register if we're fully unlocked */ >> - if (lock_len == 0) >> - status_new &= ~SR_SRWD; >> - >> if (!use_top) >> status_new |= SR_TB; >> >> @@ -3898,6 +3891,7 @@ static int spi_nor_setup(struct spi_nor *nor, >> static int spi_nor_init(struct spi_nor *nor) >> { >> int err; >> + int sr; >> >> /* >> * Atmel, SST, Intel/Numonyx, and others serial NOR tend to power up >> @@ -3933,7 +3927,14 @@ static int spi_nor_init(struct spi_nor *nor) >> set_4byte(nor, true); >> } >> >> - return 0; >> + /* Always respect the WP# (write-protect) input */ >> + sr = read_sr(nor); >> + if (sr < 0) { >> + dev_err(nor->dev, "error while reading status register\n"); >> + return -EINVAL; >> + } >> + sr |= SR_SRWD; >> + return write_sr_and_check(nor, sr, SR_SRWD); >> } >> >> /* mtd resume handler */ >>
Hi, Jonas, On 03/10/2019 12:42 PM, Jonas Bonn wrote: > Hi, > > On 10/03/2019 10:58, Tudor.Ambarus@microchip.com wrote: >> + James and Yong from Cypress. >> >> Hi, Jonas, James, Yong, >> >> On 01/30/2019 12:07 AM, Jonas Bonn wrote: >>> The status register bit SRWD (status register write disable) is >>> described in many words in the datasheets but effectively boils down to: >>> >>> i) if set, respect WP# when trying to change protection bits; >>> ii) if unset, ignore WP# when trying to change protection bits >>> >>> In short, the bit determines whether the WP# signal is honored or not. >>> >>> It's difficult to imagine the use-case where the WP# is connected and >>> asserted but the user doesn't want to respect its setting. As such, >>> this patch sets the SRWD bit unconditionally so that the WP# is _always_ >>> respected; hardware that doesn't care about WP# normally won't even have >>> it connected. >>> >> >> Always setting SRWD to 1, dismisses the need of a SRWD bit in the first place. >> Do you know why Cypress made this bit configurable and didn't enable it by default? > > I suspect it has some historical relevance; as things currently stand, I don't see how it's useful in its current form. > > We initially thought that the WP# signal turned on/off block protection but that's not the case; the block protection bits BP0-BP2 turn on/off block protection and the WP# signal only protects the BP[0-2] bits from being modified. Turning off the WP# functionality via the SRWD bits means that the BP[0-2] bits are always modifiable and therefore the flash device is never really protected from writes by a malicious actor. > > The key question here is: if the WP# signal is connected, why would you ever want its state to be ignored by the device? De-asserting the signal has the same effect as setting the SRWD bit. And the default state is un-asserted if the signal is not connected, so that case is covered, too. I see no reason not to just always set SRWD and thereby make the WP# signal do what one expects of it, always. > I tend to agree with you. Let's wait for a few days, maybe James or Yong will tell us the Cypress's reasons of making SRWD configurable. In the meantime I'll check the datasheets of the flashes that have SPI_NOR_HAS_LOCK declared, see if SRWD is configurable and what were the reasons of making it so. Cheers, ta
Hello all, SRWD bit (along with WP#) provides a way to protect Status and Configuration Registers from been modified unintendedly or by a malicious actor. By default, SRWD bit is 0, which means no protection on registers alternations. Registers can be modified easily by WRR command. (this is most of the application use cases). If set SRWD bit to 1, then when WP# is driven low during WRR command, WRR command will be ignored and Registers can't be modified. This provides a way to protect Registers, meanwhile still reserve the capability to modify Registers when necessary by driving WP# to high during WRR command. Flash data array can be protected by DYB, or PPB, or BP0-2 and TBPROT, or combination of these three methods. These 3 sector protection mechanisms and combinations provide flexibility (different security level) of data protection. If a sector is protected by any of DYB, or PPB, or BP bit, then it is not programmable or erasable. If a sector has none of DYB, PPB, BP bit set, then it is not protected and is programmable and erasable. DYB is volatile bit. It can be easily and fast modified on the fly, and will be set to default value upon power cycle or hardware reset. So it provides least security level protection, normally is used to prevent unintended program/erase commands. It also does not have wear limitation (i.e., unlimited times of alternations). One example of the use case is that set DYB bits to protect all the data array sectors right after system powers on. Whenever program or erase the flash data array, clear the DYB bit (set to unprotect) for the target sector prior to sending program/erase command. After program/erase complete, set the relevant DYB bit to protect the sector again. PPB is non-volatile bit. It is a little difficult to change than DYB. It provides medium security protection level. PPB bits have the same erase/program lifecycles as the flash data array. BP0-2 and TBPROT provide another level of security protection. SRWD bit and WP# can prevent BP0-2 and TBPROT bits to be modified, which can provide one more level of security protection. Best Regards, Yong -----Original Message----- From: Tudor.Ambarus@microchip.com <Tudor.Ambarus@microchip.com> Sent: Monday, March 11, 2019 10:06 AM To: jonas@norrbonn.se; James Tomasetta <James.Tomasetta@cypress.com>; Yong Qin <Yong.Qin@cypress.com> Cc: linux-mtd@lists.infradead.org; bbrezillon@kernel.org; richard@nod.at; marek.vasut@gmail.com; computersforpeace@gmail.com; dwmw2@infradead.org Subject: Re: [PATCH v2 1/3] mtd: spi-nor: always respect write-protect input Hi, Jonas, On 03/10/2019 12:42 PM, Jonas Bonn wrote: > Hi, > > On 10/03/2019 10:58, Tudor.Ambarus@microchip.com wrote: >> + James and Yong from Cypress. >> >> Hi, Jonas, James, Yong, >> >> On 01/30/2019 12:07 AM, Jonas Bonn wrote: >>> The status register bit SRWD (status register write disable) is >>> described in many words in the datasheets but effectively boils down to: >>> >>> i) if set, respect WP# when trying to change protection bits; >>> ii) if unset, ignore WP# when trying to change protection bits >>> >>> In short, the bit determines whether the WP# signal is honored or not. >>> >>> It's difficult to imagine the use-case where the WP# is connected >>> and asserted but the user doesn't want to respect its setting. As >>> such, this patch sets the SRWD bit unconditionally so that the WP# >>> is _always_ respected; hardware that doesn't care about WP# normally >>> won't even have it connected. >>> >> >> Always setting SRWD to 1, dismisses the need of a SRWD bit in the first place. >> Do you know why Cypress made this bit configurable and didn't enable it by default? > > I suspect it has some historical relevance; as things currently stand, I don't see how it's useful in its current form. > > We initially thought that the WP# signal turned on/off block protection but that's not the case; the block protection bits BP0-BP2 turn on/off block protection and the WP# signal only protects the BP[0-2] bits from being modified. Turning off the WP# functionality via the SRWD bits means that the BP[0-2] bits are always modifiable and therefore the flash device is never really protected from writes by a malicious actor. > > The key question here is: if the WP# signal is connected, why would you ever want its state to be ignored by the device? De-asserting the signal has the same effect as setting the SRWD bit. And the default state is un-asserted if the signal is not connected, so that case is covered, too. I see no reason not to just always set SRWD and thereby make the WP# signal do what one expects of it, always. > I tend to agree with you. Let's wait for a few days, maybe James or Yong will tell us the Cypress's reasons of making SRWD configurable. In the meantime I'll check the datasheets of the flashes that have SPI_NOR_HAS_LOCK declared, see if SRWD is configurable and what were the reasons of making it so. Cheers, ta This message and any attachments may contain confidential information from Cypress or its subsidiaries. If it has been received in error, please advise the sender and immediately delete this message.
Hi, Yong, Thank you for the explanation. There are still few things to clarify. On 03/11/2019 10:14 PM, Yong Qin wrote: > SRWD bit (along with WP#) provides a way to protect Status and Configuration Registers from been modified unintendedly or by a malicious actor. > > By default, SRWD bit is 0, which means no protection on registers alternations. Registers can be modified easily by WRR command. (this is most of the application use cases). > > If set SRWD bit to 1, then when WP# is driven low during WRR command, WRR command will be ignored and Registers can't be modified. This provides a way to protect Registers, meanwhile still reserve the capability to modify Registers when necessary by driving WP# to high during WRR command. Does the SRWD bit protect the Status and Configuration Register bits even when in Quad Mode? WP# function is not available in Quad mode. How can one release this protection when in Quad Mode and SRWD set to 1? If SRWD bit is ignored in Quad Mode, then why didn't Cypress enable Status and Configuration Register bits protection by default? I.e., remove SRWD bit from SR1, make BIT(7) a NOP, and consider the Status and Configuration Register bits protection enabled by default when not in Quad Mode. Cheers, ta
Hi Tudor, Good question. The WP# function is not available when the Quad mode is enabled (CR[1]=1). The WP# function is replaced by IO2 for input and output during Quad mode. With that said, if CR[1] is set to 1 (Quad mode enabled), the registers will not be protected. Technically default SRWD bit can be set as either 0 or 1. However, as most customers (applications) don't use this feature to protect registers, the default SRWD bit set to 0 might be a better choice, and reserve the option to change to 1 for the applications do need this feature. Thanks, Yong -----Original Message----- From: Tudor.Ambarus@microchip.com <Tudor.Ambarus@microchip.com> Sent: Tuesday, March 12, 2019 5:30 AM To: Yong Qin <Yong.Qin@cypress.com>; jonas@norrbonn.se; James Tomasetta <James.Tomasetta@cypress.com> Cc: linux-mtd@lists.infradead.org; bbrezillon@kernel.org; richard@nod.at; marek.vasut@gmail.com; computersforpeace@gmail.com; dwmw2@infradead.org Subject: Re: [PATCH v2 1/3] mtd: spi-nor: always respect write-protect input Hi, Yong, Thank you for the explanation. There are still few things to clarify. On 03/11/2019 10:14 PM, Yong Qin wrote: > SRWD bit (along with WP#) provides a way to protect Status and Configuration Registers from been modified unintendedly or by a malicious actor. > > By default, SRWD bit is 0, which means no protection on registers alternations. Registers can be modified easily by WRR command. (this is most of the application use cases). > > If set SRWD bit to 1, then when WP# is driven low during WRR command, WRR command will be ignored and Registers can't be modified. This provides a way to protect Registers, meanwhile still reserve the capability to modify Registers when necessary by driving WP# to high during WRR command. Does the SRWD bit protect the Status and Configuration Register bits even when in Quad Mode? WP# function is not available in Quad mode. How can one release this protection when in Quad Mode and SRWD set to 1? If SRWD bit is ignored in Quad Mode, then why didn't Cypress enable Status and Configuration Register bits protection by default? I.e., remove SRWD bit from SR1, make BIT(7) a NOP, and consider the Status and Configuration Register bits protection enabled by default when not in Quad Mode. Cheers, ta This message and any attachments may contain confidential information from Cypress or its subsidiaries. If it has been received in error, please advise the sender and immediately delete this message.
Jonas, Yong, On 03/12/2019 09:27 PM, Yong Qin wrote: > Hi Tudor, > > Good question. > > The WP# function is not available when the Quad mode is enabled (CR[1]=1). The WP# function is replaced by IO2 for input and output during Quad mode. With that said, if CR[1] is set to 1 (Quad mode enabled), the registers will not be protected. > > Technically default SRWD bit can be set as either 0 or 1. However, as most customers (applications) don't use this feature to protect registers, the default SRWD bit set to 0 might be a better choice, and reserve the option to change to 1 for the applications do need this feature. I think I found the reason why SRWD bit is configurable, and disabled by default: => to allow the installation of the flash in a system with a grounded WP# pin while still enabling write to the BP bits. Jonas, Yong, what do you think? Cheers, ta > > Thanks, > Yong > > -----Original Message----- > From: Tudor.Ambarus@microchip.com <Tudor.Ambarus@microchip.com> > Sent: Tuesday, March 12, 2019 5:30 AM > To: Yong Qin <Yong.Qin@cypress.com>; jonas@norrbonn.se; James Tomasetta <James.Tomasetta@cypress.com> > Cc: linux-mtd@lists.infradead.org; bbrezillon@kernel.org; richard@nod.at; marek.vasut@gmail.com; computersforpeace@gmail.com; dwmw2@infradead.org > Subject: Re: [PATCH v2 1/3] mtd: spi-nor: always respect write-protect input > > Hi, Yong, > > Thank you for the explanation. There are still few things to clarify. > > On 03/11/2019 10:14 PM, Yong Qin wrote: >> SRWD bit (along with WP#) provides a way to protect Status and Configuration Registers from been modified unintendedly or by a malicious actor. >> >> By default, SRWD bit is 0, which means no protection on registers alternations. Registers can be modified easily by WRR command. (this is most of the application use cases). >> >> If set SRWD bit to 1, then when WP# is driven low during WRR command, WRR command will be ignored and Registers can't be modified. This provides a way to protect Registers, meanwhile still reserve the capability to modify Registers when necessary by driving WP# to high during WRR command. > > Does the SRWD bit protect the Status and Configuration Register bits even when in Quad Mode? WP# function is not available in Quad mode. How can one release this protection when in Quad Mode and SRWD set to 1? > > If SRWD bit is ignored in Quad Mode, then why didn't Cypress enable Status and Configuration Register bits protection by default? I.e., remove SRWD bit from SR1, make BIT(7) a NOP, and consider the Status and Configuration Register bits protection enabled by default when not in Quad Mode. > > Cheers, > ta > > This message and any attachments may contain confidential information from Cypress or its subsidiaries. If it has been received in error, please advise the sender and immediately delete this message. >
On 19/03/2019 07:59, Tudor.Ambarus@microchip.com wrote: > Jonas, Yong, > > On 03/12/2019 09:27 PM, Yong Qin wrote: >> Hi Tudor, >> >> Good question. >> >> The WP# function is not available when the Quad mode is enabled (CR[1]=1). The WP# function is replaced by IO2 for input and output during Quad mode. With that said, if CR[1] is set to 1 (Quad mode enabled), the registers will not be protected. >> >> Technically default SRWD bit can be set as either 0 or 1. However, as most customers (applications) don't use this feature to protect registers, the default SRWD bit set to 0 might be a better choice, and reserve the option to change to 1 for the applications do need this feature. > > I think I found the reason why SRWD bit is configurable, and disabled by > default: => to allow the installation of the flash in a system with a grounded > WP# pin while still enabling write to the BP bits. I think this is bogus. Why would you ground the SRWD pin? That's a design error. /Jonas > > Jonas, Yong, what do you think? > > Cheers, > ta > >> >> Thanks, >> Yong >> >> -----Original Message----- >> From: Tudor.Ambarus@microchip.com <Tudor.Ambarus@microchip.com> >> Sent: Tuesday, March 12, 2019 5:30 AM >> To: Yong Qin <Yong.Qin@cypress.com>; jonas@norrbonn.se; James Tomasetta <James.Tomasetta@cypress.com> >> Cc: linux-mtd@lists.infradead.org; bbrezillon@kernel.org; richard@nod.at; marek.vasut@gmail.com; computersforpeace@gmail.com; dwmw2@infradead.org >> Subject: Re: [PATCH v2 1/3] mtd: spi-nor: always respect write-protect input >> >> Hi, Yong, >> >> Thank you for the explanation. There are still few things to clarify. >> >> On 03/11/2019 10:14 PM, Yong Qin wrote: >>> SRWD bit (along with WP#) provides a way to protect Status and Configuration Registers from been modified unintendedly or by a malicious actor. >>> >>> By default, SRWD bit is 0, which means no protection on registers alternations. Registers can be modified easily by WRR command. (this is most of the application use cases). >>> >>> If set SRWD bit to 1, then when WP# is driven low during WRR command, WRR command will be ignored and Registers can't be modified. This provides a way to protect Registers, meanwhile still reserve the capability to modify Registers when necessary by driving WP# to high during WRR command. >> >> Does the SRWD bit protect the Status and Configuration Register bits even when in Quad Mode? WP# function is not available in Quad mode. How can one release this protection when in Quad Mode and SRWD set to 1? >> >> If SRWD bit is ignored in Quad Mode, then why didn't Cypress enable Status and Configuration Register bits protection by default? I.e., remove SRWD bit from SR1, make BIT(7) a NOP, and consider the Status and Configuration Register bits protection enabled by default when not in Quad Mode. >> >> Cheers, >> ta >> >> This message and any attachments may contain confidential information from Cypress or its subsidiaries. If it has been received in error, please advise the sender and immediately delete this message. >>
Jonas, On 03/19/2019 09:13 AM, Jonas Bonn wrote: > > On 19/03/2019 07:59, Tudor.Ambarus@microchip.com wrote: >> Jonas, Yong, >> >> On 03/12/2019 09:27 PM, Yong Qin wrote: >>> Hi Tudor, >>> >>> Good question. >>> >>> The WP# function is not available when the Quad mode is enabled (CR[1]=1). The WP# function is replaced by IO2 for input and output during Quad mode. With that said, if CR[1] is set to 1 (Quad mode enabled), the registers will not be protected. >>> >>> Technically default SRWD bit can be set as either 0 or 1. However, as most customers (applications) don't use this feature to protect registers, the default SRWD bit set to 0 might be a better choice, and reserve the option to change to 1 for the applications do need this feature. >> >> I think I found the reason why SRWD bit is configurable, and disabled by >> default: => to allow the installation of the flash in a system with a grounded >> WP# pin while still enabling write to the BP bits. > > I think this is bogus. Why would you ground the SRWD pin? That's a design error. I've talked with Microchip hardware team, we have this feature on sst26 flashes too. Grounding WP would not necessarily be a design error. The intention is that some users might choose to populate the Flash chip onto their PCB, program the memory in-circuit, and then lock down the write protection to use the chip like a ROM. Grounding WP allows them to do this. Since SRWD is set to '0' from the factory, so users can program the memory, set the block protection (BP) bits to protect the memory, and then set the SRWD bit to enable the grounded WP input and prevent changing the BP bits. The grounded WP pin + SRWD bit method is an older, legacy approach to the problem. We don't know how many users actually ground the WP pin in this manner, but there are probably some users out there who do. Cheers, ta > > /Jonas > >> >> Jonas, Yong, what do you think? >> >> Cheers, >> ta >> >>> >>> Thanks, >>> Yong >>> >>> -----Original Message----- >>> From: Tudor.Ambarus@microchip.com <Tudor.Ambarus@microchip.com> >>> Sent: Tuesday, March 12, 2019 5:30 AM >>> To: Yong Qin <Yong.Qin@cypress.com>; jonas@norrbonn.se; James Tomasetta <James.Tomasetta@cypress.com> >>> Cc: linux-mtd@lists.infradead.org; bbrezillon@kernel.org; richard@nod.at; marek.vasut@gmail.com; computersforpeace@gmail.com; dwmw2@infradead.org >>> Subject: Re: [PATCH v2 1/3] mtd: spi-nor: always respect write-protect input >>> >>> Hi, Yong, >>> >>> Thank you for the explanation. There are still few things to clarify. >>> >>> On 03/11/2019 10:14 PM, Yong Qin wrote: >>>> SRWD bit (along with WP#) provides a way to protect Status and Configuration Registers from been modified unintendedly or by a malicious actor. >>>> >>>> By default, SRWD bit is 0, which means no protection on registers alternations. Registers can be modified easily by WRR command. (this is most of the application use cases). >>>> >>>> If set SRWD bit to 1, then when WP# is driven low during WRR command, WRR command will be ignored and Registers can't be modified. This provides a way to protect Registers, meanwhile still reserve the capability to modify Registers when necessary by driving WP# to high during WRR command. >>> >>> Does the SRWD bit protect the Status and Configuration Register bits even when in Quad Mode? WP# function is not available in Quad mode. How can one release this protection when in Quad Mode and SRWD set to 1? >>> >>> If SRWD bit is ignored in Quad Mode, then why didn't Cypress enable Status and Configuration Register bits protection by default? I.e., remove SRWD bit from SR1, make BIT(7) a NOP, and consider the Status and Configuration Register bits protection enabled by default when not in Quad Mode. >>> >>> Cheers, >>> ta >>> >>> This message and any attachments may contain confidential information from Cypress or its subsidiaries. If it has been received in error, please advise the sender and immediately delete this message. >>>
On 20/03/2019 07:33, Tudor.Ambarus@microchip.com wrote: > Jonas, > > On 03/19/2019 09:13 AM, Jonas Bonn wrote: >> >> On 19/03/2019 07:59, Tudor.Ambarus@microchip.com wrote: >>> Jonas, Yong, >>> >>> On 03/12/2019 09:27 PM, Yong Qin wrote: >>>> Hi Tudor, >>>> >>>> Good question. >>>> >>>> The WP# function is not available when the Quad mode is enabled (CR[1]=1). The WP# function is replaced by IO2 for input and output during Quad mode. With that said, if CR[1] is set to 1 (Quad mode enabled), the registers will not be protected. >>>> >>>> Technically default SRWD bit can be set as either 0 or 1. However, as most customers (applications) don't use this feature to protect registers, the default SRWD bit set to 0 might be a better choice, and reserve the option to change to 1 for the applications do need this feature. >>> >>> I think I found the reason why SRWD bit is configurable, and disabled by >>> default: => to allow the installation of the flash in a system with a grounded >>> WP# pin while still enabling write to the BP bits. >> >> I think this is bogus. Why would you ground the SRWD pin? That's a design error. > > I've talked with Microchip hardware team, we have this feature on sst26 flashes too. > > Grounding WP would not necessarily be a design error. The intention is that some > users might choose to populate the Flash chip onto their PCB, program the memory > in-circuit, and then lock down the write protection to use the chip like a ROM. > Grounding WP allows them to do this. Since SRWD is set to '0' from the factory, > so users can program the memory, set the block protection (BP) bits to protect > the memory, and then set the SRWD bit to enable the grounded WP input and > prevent changing the BP bits. Again, this is bogus. The SRWD bit is non-volatile so just resetting the flash clears the bit and the BP bits can be changed. This does not magically turn the flash into a ROM. Grounded or not, the WP# is NOT respected until the SRWD bit is set; what you've described is yet another reason to default the SRWD to set. With the BPNV bit (see patch 3 that I posted in the v2 series), at least the flash can be made to start up write-protected; however, there is nothing preventing modification of the PROTection bits until SRWD is set AND WP# is asserted (through grounding or otherwise). Linux currently defaults SRWD to cleared which is "unprotected", irregardless of the state of WP#. > > The grounded WP pin + SRWD bit method is an older, legacy approach to the > problem. We don't know how many users actually ground the WP pin in this manner, > but there are probably some users out there who do. If they do so, they are fooling themselves... or they are running a patched kernel! :) /Jonas > > Cheers, > ta > >> >> /Jonas >> >>> >>> Jonas, Yong, what do you think? >>> >>> Cheers, >>> ta >>> >>>> >>>> Thanks, >>>> Yong >>>> >>>> -----Original Message----- >>>> From: Tudor.Ambarus@microchip.com <Tudor.Ambarus@microchip.com> >>>> Sent: Tuesday, March 12, 2019 5:30 AM >>>> To: Yong Qin <Yong.Qin@cypress.com>; jonas@norrbonn.se; James Tomasetta <James.Tomasetta@cypress.com> >>>> Cc: linux-mtd@lists.infradead.org; bbrezillon@kernel.org; richard@nod.at; marek.vasut@gmail.com; computersforpeace@gmail.com; dwmw2@infradead.org >>>> Subject: Re: [PATCH v2 1/3] mtd: spi-nor: always respect write-protect input >>>> >>>> Hi, Yong, >>>> >>>> Thank you for the explanation. There are still few things to clarify. >>>> >>>> On 03/11/2019 10:14 PM, Yong Qin wrote: >>>>> SRWD bit (along with WP#) provides a way to protect Status and Configuration Registers from been modified unintendedly or by a malicious actor. >>>>> >>>>> By default, SRWD bit is 0, which means no protection on registers alternations. Registers can be modified easily by WRR command. (this is most of the application use cases). >>>>> >>>>> If set SRWD bit to 1, then when WP# is driven low during WRR command, WRR command will be ignored and Registers can't be modified. This provides a way to protect Registers, meanwhile still reserve the capability to modify Registers when necessary by driving WP# to high during WRR command. >>>> >>>> Does the SRWD bit protect the Status and Configuration Register bits even when in Quad Mode? WP# function is not available in Quad mode. How can one release this protection when in Quad Mode and SRWD set to 1? >>>> >>>> If SRWD bit is ignored in Quad Mode, then why didn't Cypress enable Status and Configuration Register bits protection by default? I.e., remove SRWD bit from SR1, make BIT(7) a NOP, and consider the Status and Configuration Register bits protection enabled by default when not in Quad Mode. >>>> >>>> Cheers, >>>> ta >>>> >>>> This message and any attachments may contain confidential information from Cypress or its subsidiaries. If it has been received in error, please advise the sender and immediately delete this message. >>>>
On 03/20/2019 09:06 AM, Jonas Bonn wrote: > External E-Mail > > > > On 20/03/2019 07:33, Tudor.Ambarus@microchip.com wrote: >> Jonas, >> >> On 03/19/2019 09:13 AM, Jonas Bonn wrote: >>> >>> On 19/03/2019 07:59, Tudor.Ambarus@microchip.com wrote: >>>> Jonas, Yong, >>>> >>>> On 03/12/2019 09:27 PM, Yong Qin wrote: >>>>> Hi Tudor, >>>>> >>>>> Good question. >>>>> >>>>> The WP# function is not available when the Quad mode is enabled (CR[1]=1). The WP# function is replaced by IO2 for input and output during Quad mode. With that said, if CR[1] is set to 1 (Quad mode enabled), the registers will not be protected. >>>>> >>>>> Technically default SRWD bit can be set as either 0 or 1. However, as most customers (applications) don't use this feature to protect registers, the default SRWD bit set to 0 might be a better choice, and reserve the option to change to 1 for the applications do need this feature. >>>> >>>> I think I found the reason why SRWD bit is configurable, and disabled by >>>> default: => to allow the installation of the flash in a system with a grounded >>>> WP# pin while still enabling write to the BP bits. >>> >>> I think this is bogus. Why would you ground the SRWD pin? That's a design error. >> >> I've talked with Microchip hardware team, we have this feature on sst26 flashes too. >> >> Grounding WP would not necessarily be a design error. The intention is that some >> users might choose to populate the Flash chip onto their PCB, program the memory >> in-circuit, and then lock down the write protection to use the chip like a ROM. >> Grounding WP allows them to do this. Since SRWD is set to '0' from the factory, >> so users can program the memory, set the block protection (BP) bits to protect >> the memory, and then set the SRWD bit to enable the grounded WP input and >> prevent changing the BP bits. > > Again, this is bogus. The SRWD bit is non-volatile so just resetting the flash clears the bit and the BP bits can be changed. This does not magically turn the flash into a ROM. Grounded or not, the WP# is NOT respected until the SRWD bit is set; what you've described is yet another reason to default the SRWD to set. SRWD is a non-volatile bit: default at power-up will be the setting prior to power-down. Cheers, ta > > With the BPNV bit (see patch 3 that I posted in the v2 series), at least the flash can be made to start up write-protected; however, there is nothing preventing modification of the PROTection bits until SRWD is set AND WP# is asserted (through grounding or otherwise). Linux currently defaults SRWD to cleared which is "unprotected", irregardless of the state of WP#. > >> >> The grounded WP pin + SRWD bit method is an older, legacy approach to the >> problem. We don't know how many users actually ground the WP pin in this manner, >> but there are probably some users out there who do. > > If they do so, they are fooling themselves... or they are running a patched kernel! :) > > /Jonas > > > > >> >> Cheers, >> ta >> >>> >>> /Jonas >>> >>>> >>>> Jonas, Yong, what do you think? >>>> >>>> Cheers, >>>> ta >>>> >>>>> >>>>> Thanks, >>>>> Yong >>>>> >>>>> -----Original Message----- >>>>> From: Tudor.Ambarus@microchip.com <Tudor.Ambarus@microchip.com> >>>>> Sent: Tuesday, March 12, 2019 5:30 AM >>>>> To: Yong Qin <Yong.Qin@cypress.com>; jonas@norrbonn.se; James Tomasetta <James.Tomasetta@cypress.com> >>>>> Cc: linux-mtd@lists.infradead.org; bbrezillon@kernel.org; richard@nod.at; marek.vasut@gmail.com; computersforpeace@gmail.com; dwmw2@infradead.org >>>>> Subject: Re: [PATCH v2 1/3] mtd: spi-nor: always respect write-protect input >>>>> >>>>> Hi, Yong, >>>>> >>>>> Thank you for the explanation. There are still few things to clarify. >>>>> >>>>> On 03/11/2019 10:14 PM, Yong Qin wrote: >>>>>> SRWD bit (along with WP#) provides a way to protect Status and Configuration Registers from been modified unintendedly or by a malicious actor. >>>>>> >>>>>> By default, SRWD bit is 0, which means no protection on registers alternations. Registers can be modified easily by WRR command. (this is most of the application use cases). >>>>>> >>>>>> If set SRWD bit to 1, then when WP# is driven low during WRR command, WRR command will be ignored and Registers can't be modified. This provides a way to protect Registers, meanwhile still reserve the capability to modify Registers when necessary by driving WP# to high during WRR command. >>>>> >>>>> Does the SRWD bit protect the Status and Configuration Register bits even when in Quad Mode? WP# function is not available in Quad mode. How can one release this protection when in Quad Mode and SRWD set to 1? >>>>> >>>>> If SRWD bit is ignored in Quad Mode, then why didn't Cypress enable Status and Configuration Register bits protection by default? I.e., remove SRWD bit from SR1, make BIT(7) a NOP, and consider the Status and Configuration Register bits protection enabled by default when not in Quad Mode. >>>>> >>>>> Cheers, >>>>> ta >>>>> >>>>> This message and any attachments may contain confidential information from Cypress or its subsidiaries. If it has been received in error, please advise the sender and immediately delete this message. >>>>> > > ______________________________________________________ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/
On 20/03/2019 08:33, Tudor.Ambarus@microchip.com wrote: > > > On 03/20/2019 09:06 AM, Jonas Bonn wrote: >> External E-Mail >> >> >> >> On 20/03/2019 07:33, Tudor.Ambarus@microchip.com wrote: >>> Jonas, >>> >>> On 03/19/2019 09:13 AM, Jonas Bonn wrote: >>>> >>>> On 19/03/2019 07:59, Tudor.Ambarus@microchip.com wrote: >>>>> Jonas, Yong, >>>>> >>>>> On 03/12/2019 09:27 PM, Yong Qin wrote: >>>>>> Hi Tudor, >>>>>> >>>>>> Good question. >>>>>> >>>>>> The WP# function is not available when the Quad mode is enabled (CR[1]=1). The WP# function is replaced by IO2 for input and output during Quad mode. With that said, if CR[1] is set to 1 (Quad mode enabled), the registers will not be protected. >>>>>> >>>>>> Technically default SRWD bit can be set as either 0 or 1. However, as most customers (applications) don't use this feature to protect registers, the default SRWD bit set to 0 might be a better choice, and reserve the option to change to 1 for the applications do need this feature. >>>>> >>>>> I think I found the reason why SRWD bit is configurable, and disabled by >>>>> default: => to allow the installation of the flash in a system with a grounded >>>>> WP# pin while still enabling write to the BP bits. >>>> >>>> I think this is bogus. Why would you ground the SRWD pin? That's a design error. >>> >>> I've talked with Microchip hardware team, we have this feature on sst26 flashes too. >>> >>> Grounding WP would not necessarily be a design error. The intention is that some >>> users might choose to populate the Flash chip onto their PCB, program the memory >>> in-circuit, and then lock down the write protection to use the chip like a ROM. >>> Grounding WP allows them to do this. Since SRWD is set to '0' from the factory, >>> so users can program the memory, set the block protection (BP) bits to protect >>> the memory, and then set the SRWD bit to enable the grounded WP input and >>> prevent changing the BP bits. >> >> Again, this is bogus. The SRWD bit is non-volatile so just resetting the flash clears the bit and the BP bits can be changed. This does not magically turn the flash into a ROM. Grounded or not, the WP# is NOT respected until the SRWD bit is set; what you've described is yet another reason to default the SRWD to set. > > SRWD is a non-volatile bit: default at power-up will be the setting prior to > power-down. Ah, right you are! :) By grounding the WP# pin, you are effectively turning the SRWD bit into an OTP bit. Note that what you described above also requires setting the BPNV bit, otherwise protection is irrevocably reset to "unprotected" at reset. The BPNV bit cannot be set from Linux so whoever is using the "grounded pin" method must be programming the board in-factory from another system than Linux. Therefore it's OK for Linux to default SRWD to asserted, irregardless of the state of WP#. /Jonas > > Cheers, > ta > >> >> With the BPNV bit (see patch 3 that I posted in the v2 series), at least the flash can be made to start up write-protected; however, there is nothing preventing modification of the PROTection bits until SRWD is set AND WP# is asserted (through grounding or otherwise). Linux currently defaults SRWD to cleared which is "unprotected", irregardless of the state of WP#. >> >>> >>> The grounded WP pin + SRWD bit method is an older, legacy approach to the >>> problem. We don't know how many users actually ground the WP pin in this manner, >>> but there are probably some users out there who do. >> >> If they do so, they are fooling themselves... or they are running a patched kernel! :) >> >> /Jonas >> >> >> >> >>> >>> Cheers, >>> ta >>> >>>> >>>> /Jonas >>>> >>>>> >>>>> Jonas, Yong, what do you think? >>>>> >>>>> Cheers, >>>>> ta >>>>> >>>>>> >>>>>> Thanks, >>>>>> Yong >>>>>> >>>>>> -----Original Message----- >>>>>> From: Tudor.Ambarus@microchip.com <Tudor.Ambarus@microchip.com> >>>>>> Sent: Tuesday, March 12, 2019 5:30 AM >>>>>> To: Yong Qin <Yong.Qin@cypress.com>; jonas@norrbonn.se; James Tomasetta <James.Tomasetta@cypress.com> >>>>>> Cc: linux-mtd@lists.infradead.org; bbrezillon@kernel.org; richard@nod.at; marek.vasut@gmail.com; computersforpeace@gmail.com; dwmw2@infradead.org >>>>>> Subject: Re: [PATCH v2 1/3] mtd: spi-nor: always respect write-protect input >>>>>> >>>>>> Hi, Yong, >>>>>> >>>>>> Thank you for the explanation. There are still few things to clarify. >>>>>> >>>>>> On 03/11/2019 10:14 PM, Yong Qin wrote: >>>>>>> SRWD bit (along with WP#) provides a way to protect Status and Configuration Registers from been modified unintendedly or by a malicious actor. >>>>>>> >>>>>>> By default, SRWD bit is 0, which means no protection on registers alternations. Registers can be modified easily by WRR command. (this is most of the application use cases). >>>>>>> >>>>>>> If set SRWD bit to 1, then when WP# is driven low during WRR command, WRR command will be ignored and Registers can't be modified. This provides a way to protect Registers, meanwhile still reserve the capability to modify Registers when necessary by driving WP# to high during WRR command. >>>>>> >>>>>> Does the SRWD bit protect the Status and Configuration Register bits even when in Quad Mode? WP# function is not available in Quad mode. How can one release this protection when in Quad Mode and SRWD set to 1? >>>>>> >>>>>> If SRWD bit is ignored in Quad Mode, then why didn't Cypress enable Status and Configuration Register bits protection by default? I.e., remove SRWD bit from SR1, make BIT(7) a NOP, and consider the Status and Configuration Register bits protection enabled by default when not in Quad Mode. >>>>>> >>>>>> Cheers, >>>>>> ta >>>>>> >>>>>> This message and any attachments may contain confidential information from Cypress or its subsidiaries. If it has been received in error, please advise the sender and immediately delete this message. >>>>>> >> >> ______________________________________________________ >> Linux MTD discussion mailing list >> http://lists.infradead.org/mailman/listinfo/linux-mtd/
Hi Jonas, Tudor, I think Tudor described is a reasonable use case scenario. BPNV bit default value is 0, which means BP0-2 bits are default Non-Volatile. In this case, BPNV bit does not need to be set. After the flash chip mounted on PCB board with WP# grounded, all need to do are: Program data into flash --> Set BP0-2 to 1 --> Set SRWD to 1, then both SRWD bit and BP0-2 bits are locked-down, unless disconnecting WP# from grounding. Reset will not change SRWD bit and BP0-2 bits since they all are Non-Volatile. Best Regards, Yong -----Original Message----- From: Jonas Bonn <jonas@norrbonn.se> Sent: Wednesday, March 20, 2019 3:40 AM To: Tudor.Ambarus@microchip.com; Yong Qin <Yong.Qin@cypress.com>; James Tomasetta <James.Tomasetta@cypress.com> Cc: bbrezillon@kernel.org; richard@nod.at; marek.vasut@gmail.com; linux-mtd@lists.infradead.org; computersforpeace@gmail.com; dwmw2@infradead.org Subject: Re: [PATCH v2 1/3] mtd: spi-nor: always respect write-protect input On 20/03/2019 08:33, Tudor.Ambarus@microchip.com wrote: > > > On 03/20/2019 09:06 AM, Jonas Bonn wrote: >> External E-Mail >> >> >> >> On 20/03/2019 07:33, Tudor.Ambarus@microchip.com wrote: >>> Jonas, >>> >>> On 03/19/2019 09:13 AM, Jonas Bonn wrote: >>>> >>>> On 19/03/2019 07:59, Tudor.Ambarus@microchip.com wrote: >>>>> Jonas, Yong, >>>>> >>>>> On 03/12/2019 09:27 PM, Yong Qin wrote: >>>>>> Hi Tudor, >>>>>> >>>>>> Good question. >>>>>> >>>>>> The WP# function is not available when the Quad mode is enabled (CR[1]=1). The WP# function is replaced by IO2 for input and output during Quad mode. With that said, if CR[1] is set to 1 (Quad mode enabled), the registers will not be protected. >>>>>> >>>>>> Technically default SRWD bit can be set as either 0 or 1. However, as most customers (applications) don't use this feature to protect registers, the default SRWD bit set to 0 might be a better choice, and reserve the option to change to 1 for the applications do need this feature. >>>>> >>>>> I think I found the reason why SRWD bit is configurable, and >>>>> disabled by >>>>> default: => to allow the installation of the flash in a system >>>>> with a grounded WP# pin while still enabling write to the BP bits. >>>> >>>> I think this is bogus. Why would you ground the SRWD pin? That's a design error. >>> >>> I've talked with Microchip hardware team, we have this feature on sst26 flashes too. >>> >>> Grounding WP would not necessarily be a design error. The intention >>> is that some users might choose to populate the Flash chip onto >>> their PCB, program the memory in-circuit, and then lock down the write protection to use the chip like a ROM. >>> Grounding WP allows them to do this. Since SRWD is set to '0' from >>> the factory, so users can program the memory, set the block >>> protection (BP) bits to protect the memory, and then set the SRWD >>> bit to enable the grounded WP input and prevent changing the BP bits. >> >> Again, this is bogus. The SRWD bit is non-volatile so just resetting the flash clears the bit and the BP bits can be changed. This does not magically turn the flash into a ROM. Grounded or not, the WP# is NOT respected until the SRWD bit is set; what you've described is yet another reason to default the SRWD to set. > > SRWD is a non-volatile bit: default at power-up will be the setting > prior to power-down. Ah, right you are! :) By grounding the WP# pin, you are effectively turning the SRWD bit into an OTP bit. Note that what you described above also requires setting the BPNV bit, otherwise protection is irrevocably reset to "unprotected" at reset. The BPNV bit cannot be set from Linux so whoever is using the "grounded pin" method must be programming the board in-factory from another system than Linux. Therefore it's OK for Linux to default SRWD to asserted, irregardless of the state of WP#. /Jonas > > Cheers, > ta > >> >> With the BPNV bit (see patch 3 that I posted in the v2 series), at least the flash can be made to start up write-protected; however, there is nothing preventing modification of the PROTection bits until SRWD is set AND WP# is asserted (through grounding or otherwise). Linux currently defaults SRWD to cleared which is "unprotected", irregardless of the state of WP#. >> >>> >>> The grounded WP pin + SRWD bit method is an older, legacy approach >>> to the problem. We don't know how many users actually ground the WP >>> pin in this manner, but there are probably some users out there who do. >> >> If they do so, they are fooling themselves... or they are running a >> patched kernel! :) >> >> /Jonas >> >> >> >> >>> >>> Cheers, >>> ta >>> >>>> >>>> /Jonas >>>> >>>>> >>>>> Jonas, Yong, what do you think? >>>>> >>>>> Cheers, >>>>> ta >>>>> >>>>>> >>>>>> Thanks, >>>>>> Yong >>>>>> >>>>>> -----Original Message----- >>>>>> From: Tudor.Ambarus@microchip.com <Tudor.Ambarus@microchip.com> >>>>>> Sent: Tuesday, March 12, 2019 5:30 AM >>>>>> To: Yong Qin <Yong.Qin@cypress.com>; jonas@norrbonn.se; James >>>>>> Tomasetta <James.Tomasetta@cypress.com> >>>>>> Cc: linux-mtd@lists.infradead.org; bbrezillon@kernel.org; >>>>>> richard@nod.at; marek.vasut@gmail.com; >>>>>> computersforpeace@gmail.com; dwmw2@infradead.org >>>>>> Subject: Re: [PATCH v2 1/3] mtd: spi-nor: always respect >>>>>> write-protect input >>>>>> >>>>>> Hi, Yong, >>>>>> >>>>>> Thank you for the explanation. There are still few things to clarify. >>>>>> >>>>>> On 03/11/2019 10:14 PM, Yong Qin wrote: >>>>>>> SRWD bit (along with WP#) provides a way to protect Status and Configuration Registers from been modified unintendedly or by a malicious actor. >>>>>>> >>>>>>> By default, SRWD bit is 0, which means no protection on registers alternations. Registers can be modified easily by WRR command. (this is most of the application use cases). >>>>>>> >>>>>>> If set SRWD bit to 1, then when WP# is driven low during WRR command, WRR command will be ignored and Registers can't be modified. This provides a way to protect Registers, meanwhile still reserve the capability to modify Registers when necessary by driving WP# to high during WRR command. >>>>>> >>>>>> Does the SRWD bit protect the Status and Configuration Register bits even when in Quad Mode? WP# function is not available in Quad mode. How can one release this protection when in Quad Mode and SRWD set to 1? >>>>>> >>>>>> If SRWD bit is ignored in Quad Mode, then why didn't Cypress enable Status and Configuration Register bits protection by default? I.e., remove SRWD bit from SR1, make BIT(7) a NOP, and consider the Status and Configuration Register bits protection enabled by default when not in Quad Mode. >>>>>> >>>>>> Cheers, >>>>>> ta >>>>>> >>>>>> This message and any attachments may contain confidential information from Cypress or its subsidiaries. If it has been received in error, please advise the sender and immediately delete this message. >>>>>> >> >> ______________________________________________________ >> Linux MTD discussion mailing list >> http://lists.infradead.org/mailman/listinfo/linux-mtd/ This message and any attachments may contain confidential information from Cypress or its subsidiaries. If it has been received in error, please advise the sender and immediately delete this message.
On 20/03/2019 19:56, Yong Qin wrote: > Hi Jonas, Tudor, > > I think Tudor described is a reasonable use case scenario. > > BPNV bit default value is 0, which means BP0-2 bits are default Non-Volatile. In this case, BPNV bit does not need to be set. After the flash chip mounted on PCB board with WP# grounded, all need to do are: Program data into flash --> Set BP0-2 to 1 --> Set SRWD to 1, then both SRWD bit and BP0-2 bits are locked-down, unless disconnecting WP# from grounding. Reset will not change SRWD bit and BP0-2 bits since they all are Non-Volatile. OK, my own use case is causing me to get confused over the volatility of these bits. The use case I have is the following: a) The flash must be write-protected at power on b) The flash may be made writable by an authorized actor So the idea is to do this: i) Set BPNV to 1 so that BP0-2 get reset to 1 at power on ii) WP# is asserted at power on (effectively grounded) iii) Authorized actor invokes action to de-assert WP# iv) BP0-2 bits can now be modified freely and flash is writable v) Authorized actor may invoke action to assert WP# but if that doesn't happen and the system gets restarted at least the flash will not be writable when it is restarted Control of the BP bits in Linux is via stm_lock and stm_unlock. These functions also set and clear SRWD. This breaks the above use case because when the device is unlocked, SRWD is cleared, and then device is not protected after a power cycle. Suggestions? Can we separate the manipulation of SRWD from the manipulation of the BP bits without breaking existing users? /Jonas > > Best Regards, > Yong > > -----Original Message----- > From: Jonas Bonn <jonas@norrbonn.se> > Sent: Wednesday, March 20, 2019 3:40 AM > To: Tudor.Ambarus@microchip.com; Yong Qin <Yong.Qin@cypress.com>; James Tomasetta <James.Tomasetta@cypress.com> > Cc: bbrezillon@kernel.org; richard@nod.at; marek.vasut@gmail.com; linux-mtd@lists.infradead.org; computersforpeace@gmail.com; dwmw2@infradead.org > Subject: Re: [PATCH v2 1/3] mtd: spi-nor: always respect write-protect input > > > > On 20/03/2019 08:33, Tudor.Ambarus@microchip.com wrote: >> >> >> On 03/20/2019 09:06 AM, Jonas Bonn wrote: >>> External E-Mail >>> >>> >>> >>> On 20/03/2019 07:33, Tudor.Ambarus@microchip.com wrote: >>>> Jonas, >>>> >>>> On 03/19/2019 09:13 AM, Jonas Bonn wrote: >>>>> >>>>> On 19/03/2019 07:59, Tudor.Ambarus@microchip.com wrote: >>>>>> Jonas, Yong, >>>>>> >>>>>> On 03/12/2019 09:27 PM, Yong Qin wrote: >>>>>>> Hi Tudor, >>>>>>> >>>>>>> Good question. >>>>>>> >>>>>>> The WP# function is not available when the Quad mode is enabled (CR[1]=1). The WP# function is replaced by IO2 for input and output during Quad mode. With that said, if CR[1] is set to 1 (Quad mode enabled), the registers will not be protected. >>>>>>> >>>>>>> Technically default SRWD bit can be set as either 0 or 1. However, as most customers (applications) don't use this feature to protect registers, the default SRWD bit set to 0 might be a better choice, and reserve the option to change to 1 for the applications do need this feature. >>>>>> >>>>>> I think I found the reason why SRWD bit is configurable, and >>>>>> disabled by >>>>>> default: => to allow the installation of the flash in a system >>>>>> with a grounded WP# pin while still enabling write to the BP bits. >>>>> >>>>> I think this is bogus. Why would you ground the SRWD pin? That's a design error. >>>> >>>> I've talked with Microchip hardware team, we have this feature on sst26 flashes too. >>>> >>>> Grounding WP would not necessarily be a design error. The intention >>>> is that some users might choose to populate the Flash chip onto >>>> their PCB, program the memory in-circuit, and then lock down the write protection to use the chip like a ROM. >>>> Grounding WP allows them to do this. Since SRWD is set to '0' from >>>> the factory, so users can program the memory, set the block >>>> protection (BP) bits to protect the memory, and then set the SRWD >>>> bit to enable the grounded WP input and prevent changing the BP bits. >>> >>> Again, this is bogus. The SRWD bit is non-volatile so just resetting the flash clears the bit and the BP bits can be changed. This does not magically turn the flash into a ROM. Grounded or not, the WP# is NOT respected until the SRWD bit is set; what you've described is yet another reason to default the SRWD to set. >> >> SRWD is a non-volatile bit: default at power-up will be the setting >> prior to power-down. > > Ah, right you are! :) > > By grounding the WP# pin, you are effectively turning the SRWD bit into an OTP bit. Note that what you described above also requires setting the BPNV bit, otherwise protection is irrevocably reset to "unprotected" > at reset. The BPNV bit cannot be set from Linux so whoever is using the "grounded pin" method must be programming the board in-factory from another system than Linux. Therefore it's OK for Linux to default SRWD to asserted, irregardless of the state of WP#. > > /Jonas > > > >> >> Cheers, >> ta >> >>> >>> With the BPNV bit (see patch 3 that I posted in the v2 series), at least the flash can be made to start up write-protected; however, there is nothing preventing modification of the PROTection bits until SRWD is set AND WP# is asserted (through grounding or otherwise). Linux currently defaults SRWD to cleared which is "unprotected", irregardless of the state of WP#. >>> >>>> >>>> The grounded WP pin + SRWD bit method is an older, legacy approach >>>> to the problem. We don't know how many users actually ground the WP >>>> pin in this manner, but there are probably some users out there who do. >>> >>> If they do so, they are fooling themselves... or they are running a >>> patched kernel! :) >>> >>> /Jonas >>> >>> >>> >>> >>>> >>>> Cheers, >>>> ta >>>> >>>>> >>>>> /Jonas >>>>> >>>>>> >>>>>> Jonas, Yong, what do you think? >>>>>> >>>>>> Cheers, >>>>>> ta >>>>>> >>>>>>> >>>>>>> Thanks, >>>>>>> Yong >>>>>>> >>>>>>> -----Original Message----- >>>>>>> From: Tudor.Ambarus@microchip.com <Tudor.Ambarus@microchip.com> >>>>>>> Sent: Tuesday, March 12, 2019 5:30 AM >>>>>>> To: Yong Qin <Yong.Qin@cypress.com>; jonas@norrbonn.se; James >>>>>>> Tomasetta <James.Tomasetta@cypress.com> >>>>>>> Cc: linux-mtd@lists.infradead.org; bbrezillon@kernel.org; >>>>>>> richard@nod.at; marek.vasut@gmail.com; >>>>>>> computersforpeace@gmail.com; dwmw2@infradead.org >>>>>>> Subject: Re: [PATCH v2 1/3] mtd: spi-nor: always respect >>>>>>> write-protect input >>>>>>> >>>>>>> Hi, Yong, >>>>>>> >>>>>>> Thank you for the explanation. There are still few things to clarify. >>>>>>> >>>>>>> On 03/11/2019 10:14 PM, Yong Qin wrote: >>>>>>>> SRWD bit (along with WP#) provides a way to protect Status and Configuration Registers from been modified unintendedly or by a malicious actor. >>>>>>>> >>>>>>>> By default, SRWD bit is 0, which means no protection on registers alternations. Registers can be modified easily by WRR command. (this is most of the application use cases). >>>>>>>> >>>>>>>> If set SRWD bit to 1, then when WP# is driven low during WRR command, WRR command will be ignored and Registers can't be modified. This provides a way to protect Registers, meanwhile still reserve the capability to modify Registers when necessary by driving WP# to high during WRR command. >>>>>>> >>>>>>> Does the SRWD bit protect the Status and Configuration Register bits even when in Quad Mode? WP# function is not available in Quad mode. How can one release this protection when in Quad Mode and SRWD set to 1? >>>>>>> >>>>>>> If SRWD bit is ignored in Quad Mode, then why didn't Cypress enable Status and Configuration Register bits protection by default? I.e., remove SRWD bit from SR1, make BIT(7) a NOP, and consider the Status and Configuration Register bits protection enabled by default when not in Quad Mode. >>>>>>> >>>>>>> Cheers, >>>>>>> ta >>>>>>> >>>>>>> This message and any attachments may contain confidential information from Cypress or its subsidiaries. If it has been received in error, please advise the sender and immediately delete this message. >>>>>>> >>> >>> ______________________________________________________ >>> Linux MTD discussion mailing list >>> http://lists.infradead.org/mailman/listinfo/linux-mtd/ > > This message and any attachments may contain confidential information from Cypress or its subsidiaries. If it has been received in error, please advise the sender and immediately delete this message. >
Hi, Jonas, On 03/20/2019 11:05 PM, Jonas Bonn wrote: > External E-Mail > > > > On 20/03/2019 19:56, Yong Qin wrote: >> Hi Jonas, Tudor, >> >> I think Tudor described is a reasonable use case scenario. >> >> BPNV bit default value is 0, which means BP0-2 bits are default Non-Volatile. In this case, BPNV bit does not need to be set. After the flash chip mounted on PCB board with WP# grounded, all need to do are: Program data into flash --> Set BP0-2 to 1 --> Set SRWD to 1, then both SRWD bit and BP0-2 bits are locked-down, unless disconnecting WP# from grounding. Reset will not change SRWD bit and BP0-2 bits since they all are Non-Volatile. > > OK, my own use case is causing me to get confused over the volatility of these bits. > > The use case I have is the following: > a) The flash must be write-protected at power on > b) The flash may be made writable by an authorized actor Can you control who is authorized or not? Just for the fun of conversation, there are flashes that come write-protected by default after a power-on reset cycle (see sst26vf064B). Winbound (ex. W25Q128FV) and Macronix (ex. MX25U12835F) have this feature too. > > So the idea is to do this: > i) Set BPNV to 1 so that BP0-2 get reset to 1 at power on You would also want to set SRWD to 1, to lock the state of SRWD and BP0-2 bits. Setting BPNV to 1 will make the BP0-2 protection bits volatile with default value of 111 after power on or reset. > ii) WP# is asserted at power on (effectively grounded) If SRWD was previously set to 1, SRWD and BP0-2 are protected, indeed. > iii) Authorized actor invokes action to de-assert WP# SRWD and BP0-2 are now writable. > iv) BP0-2 bits can now be modified freely and flash is writable OK > v) Authorized actor may invoke action to assert WP# but if that doesn't happen and the system gets restarted at least the flash will not be writable when it is restarted I see your problem. SRWD is not OTP, and when cleared will not protect the state of the BP0-2 bits and implicitly will not protect the flash after a power-on reset cycle. This is way you want to always set SRWD to 1 in software. > > Control of the BP bits in Linux is via stm_lock and stm_unlock. These functions also set and clear SRWD. This breaks the above use case because when the device is unlocked, SRWD is cleared, and then device is not protected after a power cycle. > > Suggestions? Can we separate the manipulation of SRWD from the manipulation of the BP bits without breaking existing users? > Probably not, or at least I don't have a solution for now. Have you read what other data protection mechanisms offers Cypress? Cheers, ta > /Jonas > >> >> Best Regards, >> Yong >> >> -----Original Message----- >> From: Jonas Bonn <jonas@norrbonn.se> >> Sent: Wednesday, March 20, 2019 3:40 AM >> To: Tudor.Ambarus@microchip.com; Yong Qin <Yong.Qin@cypress.com>; James Tomasetta <James.Tomasetta@cypress.com> >> Cc: bbrezillon@kernel.org; richard@nod.at; marek.vasut@gmail.com; linux-mtd@lists.infradead.org; computersforpeace@gmail.com; dwmw2@infradead.org >> Subject: Re: [PATCH v2 1/3] mtd: spi-nor: always respect write-protect input >> >> >> >> On 20/03/2019 08:33, Tudor.Ambarus@microchip.com wrote: >>> >>> >>> On 03/20/2019 09:06 AM, Jonas Bonn wrote: >>>> External E-Mail >>>> >>>> >>>> >>>> On 20/03/2019 07:33, Tudor.Ambarus@microchip.com wrote: >>>>> Jonas, >>>>> >>>>> On 03/19/2019 09:13 AM, Jonas Bonn wrote: >>>>>> >>>>>> On 19/03/2019 07:59, Tudor.Ambarus@microchip.com wrote: >>>>>>> Jonas, Yong, >>>>>>> >>>>>>> On 03/12/2019 09:27 PM, Yong Qin wrote: >>>>>>>> Hi Tudor, >>>>>>>> >>>>>>>> Good question. >>>>>>>> >>>>>>>> The WP# function is not available when the Quad mode is enabled (CR[1]=1). The WP# function is replaced by IO2 for input and output during Quad mode. With that said, if CR[1] is set to 1 (Quad mode enabled), the registers will not be protected. >>>>>>>> >>>>>>>> Technically default SRWD bit can be set as either 0 or 1. However, as most customers (applications) don't use this feature to protect registers, the default SRWD bit set to 0 might be a better choice, and reserve the option to change to 1 for the applications do need this feature. >>>>>>> >>>>>>> I think I found the reason why SRWD bit is configurable, and >>>>>>> disabled by >>>>>>> default: => to allow the installation of the flash in a system >>>>>>> with a grounded WP# pin while still enabling write to the BP bits. >>>>>> >>>>>> I think this is bogus. Why would you ground the SRWD pin? That's a design error. >>>>> >>>>> I've talked with Microchip hardware team, we have this feature on sst26 flashes too. >>>>> >>>>> Grounding WP would not necessarily be a design error. The intention >>>>> is that some users might choose to populate the Flash chip onto >>>>> their PCB, program the memory in-circuit, and then lock down the write protection to use the chip like a ROM. >>>>> Grounding WP allows them to do this. Since SRWD is set to '0' from >>>>> the factory, so users can program the memory, set the block >>>>> protection (BP) bits to protect the memory, and then set the SRWD >>>>> bit to enable the grounded WP input and prevent changing the BP bits. >>>> >>>> Again, this is bogus. The SRWD bit is non-volatile so just resetting the flash clears the bit and the BP bits can be changed. This does not magically turn the flash into a ROM. Grounded or not, the WP# is NOT respected until the SRWD bit is set; what you've described is yet another reason to default the SRWD to set. >>> >>> SRWD is a non-volatile bit: default at power-up will be the setting >>> prior to power-down. >> >> Ah, right you are! :) >> >> By grounding the WP# pin, you are effectively turning the SRWD bit into an OTP bit. Note that what you described above also requires setting the BPNV bit, otherwise protection is irrevocably reset to "unprotected" >> at reset. The BPNV bit cannot be set from Linux so whoever is using the "grounded pin" method must be programming the board in-factory from another system than Linux. Therefore it's OK for Linux to default SRWD to asserted, irregardless of the state of WP#. >> >> /Jonas >> >> >> >>> >>> Cheers, >>> ta >>> >>>> >>>> With the BPNV bit (see patch 3 that I posted in the v2 series), at least the flash can be made to start up write-protected; however, there is nothing preventing modification of the PROTection bits until SRWD is set AND WP# is asserted (through grounding or otherwise). Linux currently defaults SRWD to cleared which is "unprotected", irregardless of the state of WP#. >>>> >>>>> >>>>> The grounded WP pin + SRWD bit method is an older, legacy approach >>>>> to the problem. We don't know how many users actually ground the WP >>>>> pin in this manner, but there are probably some users out there who do. >>>> >>>> If they do so, they are fooling themselves... or they are running a >>>> patched kernel! :) >>>> >>>> /Jonas >>>> >>>> >>>> >>>> >>>>> >>>>> Cheers, >>>>> ta >>>>> >>>>>> >>>>>> /Jonas >>>>>> >>>>>>> >>>>>>> Jonas, Yong, what do you think? >>>>>>> >>>>>>> Cheers, >>>>>>> ta >>>>>>> >>>>>>>> >>>>>>>> Thanks, >>>>>>>> Yong >>>>>>>> >>>>>>>> -----Original Message----- >>>>>>>> From: Tudor.Ambarus@microchip.com <Tudor.Ambarus@microchip.com> >>>>>>>> Sent: Tuesday, March 12, 2019 5:30 AM >>>>>>>> To: Yong Qin <Yong.Qin@cypress.com>; jonas@norrbonn.se; James >>>>>>>> Tomasetta <James.Tomasetta@cypress.com> >>>>>>>> Cc: linux-mtd@lists.infradead.org; bbrezillon@kernel.org; >>>>>>>> richard@nod.at; marek.vasut@gmail.com; >>>>>>>> computersforpeace@gmail.com; dwmw2@infradead.org >>>>>>>> Subject: Re: [PATCH v2 1/3] mtd: spi-nor: always respect >>>>>>>> write-protect input >>>>>>>> >>>>>>>> Hi, Yong, >>>>>>>> >>>>>>>> Thank you for the explanation. There are still few things to clarify. >>>>>>>> >>>>>>>> On 03/11/2019 10:14 PM, Yong Qin wrote: >>>>>>>>> SRWD bit (along with WP#) provides a way to protect Status and Configuration Registers from been modified unintendedly or by a malicious actor. >>>>>>>>> >>>>>>>>> By default, SRWD bit is 0, which means no protection on registers alternations. Registers can be modified easily by WRR command. (this is most of the application use cases). >>>>>>>>> >>>>>>>>> If set SRWD bit to 1, then when WP# is driven low during WRR command, WRR command will be ignored and Registers can't be modified. This provides a way to protect Registers, meanwhile still reserve the capability to modify Registers when necessary by driving WP# to high during WRR command. >>>>>>>> >>>>>>>> Does the SRWD bit protect the Status and Configuration Register bits even when in Quad Mode? WP# function is not available in Quad mode. How can one release this protection when in Quad Mode and SRWD set to 1? >>>>>>>> >>>>>>>> If SRWD bit is ignored in Quad Mode, then why didn't Cypress enable Status and Configuration Register bits protection by default? I.e., remove SRWD bit from SR1, make BIT(7) a NOP, and consider the Status and Configuration Register bits protection enabled by default when not in Quad Mode. >>>>>>>> >>>>>>>> Cheers, >>>>>>>> ta >>>>>>>> >>>>>>>> This message and any attachments may contain confidential information from Cypress or its subsidiaries. If it has been received in error, please advise the sender and immediately delete this message. >>>>>>>> >>>> >>>> ______________________________________________________ >>>> Linux MTD discussion mailing list >>>> http://lists.infradead.org/mailman/listinfo/linux-mtd/ >> >> This message and any attachments may contain confidential information from Cypress or its subsidiaries. If it has been received in error, please advise the sender and immediately delete this message. >>
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c index 13a5055e5f3f..4c8ce2b90838 100644 --- a/drivers/mtd/spi-nor/spi-nor.c +++ b/drivers/mtd/spi-nor/spi-nor.c @@ -1231,9 +1231,6 @@ static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len) status_new = (status_old & ~mask & ~SR_TB) | val; - /* Disallow further writes if WP pin is asserted */ - status_new |= SR_SRWD; - if (!use_top) status_new |= SR_TB; @@ -1313,10 +1310,6 @@ static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len) status_new = (status_old & ~mask & ~SR_TB) | val; - /* Don't protect status register if we're fully unlocked */ - if (lock_len == 0) - status_new &= ~SR_SRWD; - if (!use_top) status_new |= SR_TB; @@ -3898,6 +3891,7 @@ static int spi_nor_setup(struct spi_nor *nor, static int spi_nor_init(struct spi_nor *nor) { int err; + int sr; /* * Atmel, SST, Intel/Numonyx, and others serial NOR tend to power up @@ -3933,7 +3927,14 @@ static int spi_nor_init(struct spi_nor *nor) set_4byte(nor, true); } - return 0; + /* Always respect the WP# (write-protect) input */ + sr = read_sr(nor); + if (sr < 0) { + dev_err(nor->dev, "error while reading status register\n"); + return -EINVAL; + } + sr |= SR_SRWD; + return write_sr_and_check(nor, sr, SR_SRWD); } /* mtd resume handler */
The status register bit SRWD (status register write disable) is described in many words in the datasheets but effectively boils down to: i) if set, respect WP# when trying to change protection bits; ii) if unset, ignore WP# when trying to change protection bits In short, the bit determines whether the WP# signal is honored or not. It's difficult to imagine the use-case where the WP# is connected and asserted but the user doesn't want to respect its setting. As such, this patch sets the SRWD bit unconditionally so that the WP# is _always_ respected; hardware that doesn't care about WP# normally won't even have it connected. Tested on a Cypress s25fl512s. With this patch, the WP# is always respected, irregardless of whether any flash protection bits are set. Signed-off-by: Jonas Bonn <jonas@norrbonn.se> CC: Marek Vasut <marek.vasut@gmail.com> CC: David Woodhouse <dwmw2@infradead.org> CC: Brian Norris <computersforpeace@gmail.com> CC: Boris Brezillon <bbrezillon@kernel.org> CC: Richard Weinberger <richard@nod.at> CC: linux-mtd@lists.infradead.org --- drivers/mtd/spi-nor/spi-nor.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)