[PATCH v2] mtd: spi-nor: Do not proceed with spi_nor_sr_unlock if WP

[Vignesh Raghavendra]

On 9/17/20 1:01 PM, Matija Glavinic Pecotic wrote:
> In case Status register 1:SR_SRWD was set (either by software or write
> protect pin), sr_unlock fails on spi_nor_write_sr_and_check in 8-bit
> mode due to fact that written value will not match read value.
> 
> Problem was observed with n25q128a11 device having protection enabled
> by W# pin. Failures lead to device probe failure.
> 
> Problem was uncovered by 82de6a6fb67e16 ("mtd: spi-nor: Fix the writing
> of the Status Register on micron flashes"). Commit selected 8-bit mode
> for micron devices. In 16-bit mode, CR content is verified, while in
> 8-bit SR is checked for sanity.
> 
> Signed-off-by: Matija Glavinic Pecotic <matija.glavinic-pecotic.ext at nokia.com>
> ---
> v2: Fix whitespaces added by mail client
> 
>  drivers/mtd/spi-nor/core.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 65eff4c..4c3acc0 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -1777,6 +1777,10 @@ static int spi_nor_sr_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
>  
>  	status_old = nor->bouncebuf[0];
>  
> +	/* If SR is write protected, we cannot unlock */
> +	if (status_old & SR_SRWD)
> +		return 0;
> +

With this, MEMUNLOCK ioctl would succeed with no errors but would fail
to modify lock bits when  SR_SRWD is set. This would give user a wrong
impression that region has been unlocked as there is no error code
that's being returned (thus breaks ABI).

Also, how would one unlock the flash once SR_SRWD is set? For example:

(1) hardware WP# is deasserted
(2) program flash
(3) Set block protection bits and SR_SRWD
(4) hardware WP# is asserted
(5) flash protection range can no longer be changed, until WP# is
    deasserted

Now to make flash writeable again:

(1) hardware WP# is deasserted
(2) Unlock the protection flash bits and SR_SRWD

But (2) would always fail due to above check, right?


Alternatively, spi_nor_init() can be modified to be non fatal when
spi_nor_unlock_all() returns non zero value (but retain the dev_dbg()).
Also add a comment inline.

>  	/* If nothing in our range is locked, we don't need to do anything */
>  	if (spi_nor_is_unlocked_sr(nor, ofs, len, status_old))
>  		return 0;
> 

Regards
Vignesh