Re: [PATCH v7 2/7] mtd: spi-nor: ignore errors in spi_nor_unlock_all()

[Tudor.Ambarus]

On 12/3/20 1:00 AM, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Just try to unlock the whole SPI-NOR flash array. Don't abort the
> probing in case of an error. Justifications:
>  (1) For some boards, this just works because
>      spi_nor_write_16bit_sr_and_check() is broken and just checks the
>      second half of the 16bit. Once that will be fixed, SPI probe will
>      fail for boards which has hardware-write protected SPI-NOR flashes.
>  (2) Until now, hardware write-protection was the only viable solution
>      to use the block protection bits. This is because this very
>      function spi_nor_unlock_all() will be called unconditionally on
>      every linux boot. Therefore, this bits only makes sense in
>      combination with the hardware write-protection. If we would fail
>      the SPI probe on an error in spi_nor_unlock_all() we'd break
>      virtually all users of the block protection bits.
>  (3) We should try hard to keep the MTD working even if the flash might
>      not be writable/erasable.
> 
> Fixes: 3e0930f109e7 ("mtd: spi-nor: Rework the disabling of block write protection")
> Signed-off-by: Michael Walle <michael@xxxxxxxx>

Reviewed-by: Tudor Ambarus <tudor.ambarus@xxxxxxxxxxxxx>

> ---
> changes since v6:
>  - new patch
> 
>  drivers/mtd/spi-nor/core.c | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 5bee7c8da4dc..013198abe929 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -3121,20 +3121,27 @@ static int spi_nor_quad_enable(struct spi_nor *nor)
>  }
> 
>  /**
> - * spi_nor_unlock_all() - Unlocks the entire flash memory array.
> + * spi_nor_try_unlock_all() - Tries to unlock the entire flash memory array.
>   * @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.
> + *
> + * Unprotecting the entire flash array will fail for boards which are hardware
> + * write-protected. Thus any errors are ignored.
>   */
> -static int spi_nor_unlock_all(struct spi_nor *nor)
> +static void spi_nor_try_unlock_all(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;
> +       if (!(nor->flags & SNOR_F_HAS_LOCK))
> +               return;
> +
> +       ret = spi_nor_unlock(&nor->mtd, 0, nor->params->size);
> +       if (ret)
> +               dev_dbg(nor->dev, "Failed to unlock the entire flash memory array\n");
>  }
> 
>  static int spi_nor_init(struct spi_nor *nor)
> @@ -3153,11 +3160,7 @@ 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;
> -       }
> +       spi_nor_try_unlock_all(nor);
> 
>         if (nor->addr_width == 4 &&
>             nor->read_proto != SNOR_PROTO_8_8_8_DTR &&
> --
> 2.20.1
>