Re: [PATCH v6 5/5] mtd: spi-nor: keep lock bits if they are non-volatile

[<Tudor.Ambarus@microchip.com>]

On 12/2/20 1:25 PM, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 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.
> 

I see. If WP# is asserted, spi_nor_unlock_all() would fail and would stop
the execution. One can avoid the fail by choosing MTD_SPI_NOR_SWP_KEEP,
but that would not be backward compatible. Having in mind that in the
past the unlock all was just a zero written to SR, without checking if
the write was successful, I would now say that your proposal with the
soft error if fair. Even if writes and erases will not work in case WP#
is asserted, we would at least let users do reads. The writes and erases
problems would be caught when enabling the debug traces. So please go
ahead and print just a dev_dbg when spi_nor_unlock_all() fails, without
stopping the execution.

Cheers,
ta