All of lore.kernel.org
 help / color / mirror / Atom feed
From: Michael Walle <michael@walle.cc>
To: Tudor.Ambarus@microchip.com
Cc: linux-mtd@lists.infradead.org, linux-kernel@vger.kernel.org,
	miquel.raynal@bootlin.com, richard@nod.at, vigneshr@ti.com,
	boris.brezillon@collabora.com
Subject: Re: [PATCH v6 5/5] mtd: spi-nor: keep lock bits if they are non-volatile
Date: Wed, 02 Dec 2020 12:25:49 +0100	[thread overview]
Message-ID: <fe04f234584c2f459e865955b0d09303@walle.cc> (raw)
In-Reply-To: <8e0a6a20-2779-9397-eedf-02518b4a0e5a@microchip.com>

Am 2020-12-02 12:10, schrieb Tudor.Ambarus@microchip.com:
> On 11/30/20 4:38 PM, Michael Walle wrote:
[..]
>>>> +        * indicated by SNOR_F_WP_IS_VOLATILE.
>>>> +        */
>>>> +       if (IS_ENABLED(CONFIG_MTD_SPI_NOR_WP_DISABLE) ||
>>>> +           (IS_ENABLED(CONFIG_MTD_SPI_NOR_WP_DISABLE_ON_VOLATILE) 
>>>> &&
>>>> +            nor->flags & SNOR_F_WP_IS_VOLATILE)) {
>>>> +               err = spi_nor_unlock_all(nor);
>>>> +               if (err) {
>>>> +                       dev_err(nor->dev, "Failed to unlock the 
>>>> entire
>>>> flash memory array\n");
>>> 
>>> dev_dbg for low level info
>> 
>> Is this low level info or an actual error? Which raises the question:
>> should spi_nor_unlock_all() in case SWRD couldn't be cleared and thus
>> should all the spi_nor_init fail of this? Or should it rather be a
> 
> yes, it should, because the flash will not work as expected/requested.

One counterargument: take our sl28 board, it has a hardware 
write-protected
SPI flash. It actually works right now because the write_sr_and_check()
doesn't work as intended and doesn't check what is written. So if you'd
fix that (and these changes would be backported to the stable trees), 
you'd
basically break spi-nor on these boards. And this _must_ be the case for
all boards which are actually using (hard- or sofware) write-protection.
That is the only way write-protection makes sense prior to this patch
series. Because linux will happily unlock every flash on startup. 
Therefore,
the hardware write protection is the only measure against this.

-michael

WARNING: multiple messages have this Message-ID (diff)
From: Michael Walle <michael@walle.cc>
To: Tudor.Ambarus@microchip.com
Cc: vigneshr@ti.com, richard@nod.at, linux-kernel@vger.kernel.org,
	boris.brezillon@collabora.com, linux-mtd@lists.infradead.org,
	miquel.raynal@bootlin.com
Subject: Re: [PATCH v6 5/5] mtd: spi-nor: keep lock bits if they are non-volatile
Date: Wed, 02 Dec 2020 12:25:49 +0100	[thread overview]
Message-ID: <fe04f234584c2f459e865955b0d09303@walle.cc> (raw)
In-Reply-To: <8e0a6a20-2779-9397-eedf-02518b4a0e5a@microchip.com>

Am 2020-12-02 12:10, schrieb Tudor.Ambarus@microchip.com:
> On 11/30/20 4:38 PM, Michael Walle wrote:
[..]
>>>> +        * indicated by SNOR_F_WP_IS_VOLATILE.
>>>> +        */
>>>> +       if (IS_ENABLED(CONFIG_MTD_SPI_NOR_WP_DISABLE) ||
>>>> +           (IS_ENABLED(CONFIG_MTD_SPI_NOR_WP_DISABLE_ON_VOLATILE) 
>>>> &&
>>>> +            nor->flags & SNOR_F_WP_IS_VOLATILE)) {
>>>> +               err = spi_nor_unlock_all(nor);
>>>> +               if (err) {
>>>> +                       dev_err(nor->dev, "Failed to unlock the 
>>>> entire
>>>> flash memory array\n");
>>> 
>>> dev_dbg for low level info
>> 
>> Is this low level info or an actual error? Which raises the question:
>> should spi_nor_unlock_all() in case SWRD couldn't be cleared and thus
>> should all the spi_nor_init fail of this? Or should it rather be a
> 
> yes, it should, because the flash will not work as expected/requested.

One counterargument: take our sl28 board, it has a hardware 
write-protected
SPI flash. It actually works right now because the write_sr_and_check()
doesn't work as intended and doesn't check what is written. So if you'd
fix that (and these changes would be backported to the stable trees), 
you'd
basically break spi-nor on these boards. And this _must_ be the case for
all boards which are actually using (hard- or sofware) write-protection.
That is the only way write-protection makes sense prior to this patch
series. Because linux will happily unlock every flash on startup. 
Therefore,
the hardware write protection is the only measure against this.

-michael

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

  reply	other threads:[~2020-12-02 11:26 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-26 20:26 [PATCH v6 0/5] mtd: spi-nor: keep lock bits if they are non-volatile Michael Walle
2020-11-26 20:26 ` Michael Walle
2020-11-26 20:26 ` [PATCH v6 1/5] mtd: spi-nor: atmel: remove global protection flag Michael Walle
2020-11-26 20:26   ` Michael Walle
2020-11-26 20:26 ` [PATCH v6 2/5] mtd: spi-nor: sst: " Michael Walle
2020-11-26 20:26   ` Michael Walle
2020-11-26 20:26 ` [PATCH v6 3/5] mtd: spi-nor: intel: " Michael Walle
2020-11-26 20:26   ` Michael Walle
2020-11-27  9:07   ` Tudor.Ambarus
2020-11-27  9:07     ` Tudor.Ambarus
2020-11-26 20:26 ` [PATCH v6 4/5] mtd: spi-nor: atmel: Fix unlock_all() for AT25FS010/040 Michael Walle
2020-11-26 20:26   ` Michael Walle
2020-11-28  8:25   ` Tudor.Ambarus
2020-11-28  8:25     ` Tudor.Ambarus
2020-11-30 14:16     ` Michael Walle
2020-11-30 14:16       ` Michael Walle
2020-12-02 10:32       ` Tudor.Ambarus
2020-12-02 10:32         ` Tudor.Ambarus
2020-11-26 20:26 ` [PATCH v6 5/5] mtd: spi-nor: keep lock bits if they are non-volatile Michael Walle
2020-11-26 20:26   ` Michael Walle
2020-11-28 10:17   ` Tudor.Ambarus
2020-11-28 10:17     ` Tudor.Ambarus
2020-11-30 14:38     ` Michael Walle
2020-11-30 14:38       ` Michael Walle
2020-12-02 11:10       ` Tudor.Ambarus
2020-12-02 11:10         ` Tudor.Ambarus
2020-12-02 11:25         ` Michael Walle [this message]
2020-12-02 11:25           ` Michael Walle
2020-12-02 14:09           ` Tudor.Ambarus
2020-12-02 14:09             ` Tudor.Ambarus

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=fe04f234584c2f459e865955b0d09303@walle.cc \
    --to=michael@walle.cc \
    --cc=Tudor.Ambarus@microchip.com \
    --cc=boris.brezillon@collabora.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mtd@lists.infradead.org \
    --cc=miquel.raynal@bootlin.com \
    --cc=richard@nod.at \
    --cc=vigneshr@ti.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.