Re: [RFC PATCH 2/3] mtd: spi-nor: Introduce MTD_SPI_NOR_ALLOW_STATEFUL_MODES

[Vignesh Raghavendra]

On 9/29/20 3:29 PM, Tudor Ambarus wrote:
> Some users may teach their bootloaders to discover and recover a
> flash even when left in a statefull mode (a X-X-X I/O mode that is
> configured via a non-volatile bit).
> 
> Provide a way for those users to enter in stateful modes. A reset
> or a crash will leave the flash in full I/O mode and if the bootloader
> does not know how to recover, the SPI NOR boot will be broken.
> 
> Flashes that will enable stateful modes will be accepted only if a
> hook to recover from the stateful mode is provided in the kernel.
> With this, even if a user will break its SPI NOR boot, it'll be able
> to recover the flash at the kernel level (on those systems that have
> at least another boot media). Both the Kconfig and the acceptance
> restriction are needed, so that we don't end up completely hopeless
> and look at a flash for which there is no software to discover and
> recover the flash. Even if we can recover the flash from a stateful
> mode in kernel, entering the stateful mode is still dangerous if one's
> bootloader can't handle it. We need a way to pass the responsibility
> to the user and let him decide conciously about the risks of allowing
> stateful modes.
> 

Recovering from non-volatile (NV) stateful mode would mean unsetting
some NV bit. Doing this for every boot would mean NV bit will wear out
quite quickly which is a concern. And sometimes NV Octal Enable bits are
OTP only (Some Macronix flashes).

NV bits are not to be fiddled with too often. One would set stateful
mode in NV way only if entire system is capable of handling this
somehow. But, IMHO, since SPI NOR core currently does not support
flashes that boot in Quad or Octal mode  at the moment, SPI NOR core
should not bother providing a hook for manipulating NV IO mode settings.

So my recommendation is to not support writing to non volatile IO modes
bits unless absolutely necessary.

Regards
Vignesh

> Signed-off-by: Tudor Ambarus <tudor.ambarus@xxxxxxxxxxxxx>
> ---
>  drivers/mtd/spi-nor/Kconfig | 10 ++++++++++
>  drivers/mtd/spi-nor/core.c  |  2 ++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
> index ffc4b380f2b1..ab62457559b2 100644
> --- a/drivers/mtd/spi-nor/Kconfig
> +++ b/drivers/mtd/spi-nor/Kconfig
> @@ -24,6 +24,16 @@ config MTD_SPI_NOR_USE_4K_SECTORS
>  	  Please note that some tools/drivers/filesystems may not work with
>  	  4096 B erase size (e.g. UBIFS requires 15 KiB as a minimum).
>  
> +config MTD_SPI_NOR_ALLOW_STATEFUL_MODES
> +	bool "Allow stateful modes (DANGEROUS)"
> +	help
> +	  Allow the flash to enter in full I/O mode via a non-volatile bit.
> +	  A reset or a crash will leave the flash in the full I/O mode and if
> +	  the bootloader does not know how to recover, the SPI NOR boot will be
> +	  broken.
> +
> +	  Say N, unless you absolutely know what you are doing.
> +
>  source "drivers/mtd/spi-nor/controllers/Kconfig"
>  
>  endif # MTD_SPI_NOR
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index c149b318e2e8..e89c3ea9a736 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -3089,8 +3089,10 @@ static int spi_nor_octal_dtr_enable(struct spi_nor *nor, bool enable)
>  	      nor->write_proto == SNOR_PROTO_8_8_8_DTR))
>  		return 0;
>  
> +#ifndef CONFIG_MTD_SPI_NOR_ALLOW_STATEFUL_MODES
>  	if (!(nor->flags & SNOR_F_IO_MODE_EN_VOLATILE))
>  		return 0;
> +#endif
>  
>  	ret = nor->params->octal_dtr_enable(nor, enable);
>  	if (ret)
>