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.amba...@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
