[PATCH] mtd: spi-nor: Clear Winbond SR3 WPS bit on boot

[Marek Vasut]

On 3/5/24 10:41 PM, Michael Walle wrote:
> On Tue Mar 5, 2024 at 7:54 PM CET, Marek Vasut wrote:
>> On 3/5/24 5:55 PM, Michael Walle wrote:
>>
>> [...]
>>
>>>>>>>> Clearing this SR3 WPS bit fixes that problem, both in U-Boot and in
>>>>>>>> Linux, since Linux that is booted afterward then gets a device that has
>>>>>>>> locking scheme configured in a way that Linux expects and can operate.
>>>>>>>>
>>>>>>>> Better yet, if some old LTS version of the Linux kernel is in use, it
>>>>>>>> will also work correctly, because this patch will configure the SPI NOR
>>>>>>>> locking scheme to what Linux expects it to be, before the SPI NOR is
>>>>>>>> handed over to that old kernel.
>>>>>>>
>>>>>>> Agreed, but it should *not* be done automatically and nor
>>>>>>> unconditionally. Please don't just overwrite any locking bits just
>>>>>>> because there is one flash which have it set.
>>>>>>
>>>>>> The unlock code is not triggered unconditionally, it is protected by
>>>>>>
>>>>>> if (IS_ENABLED(CONFIG_SPI_FLASH_UNLOCK_ALL)...
>>>>>>
>>>>>> Kconfig option, so this can be turned on/off in board config already.
>>>>>
>>>>> Ahh, OK then :)
>>>>>
>>>>> But keep in mind that enabling this option is foobar and I've gone
>>>>> lengths to eliminate it in linux. And actually made that option in
>>>>> u-boot.
>>>>>
>>>>> See linux commit 31ad3eff093c ("mtd: spi-nor: keep lock bits if they
>>>>> are non-volatile").
>>>>
>>>> So, how shall we proceed here?
>>>
>>> Regarding this patch, I think it's fine. It will unlock the whole
>>> flash as advertised.
>>
>> This change won't unlock the flash, it will switch to the supported
>> locking scheme, one which can then be used to unlock the flash.
> 
> I can't follow you here. The code is added right below the
> write_sr(0), which will clear all the BP bits, thus unlocking
> everything if WPS=0. Therefore, no locking will be enabled anymore
> afterwards. What did I miss?

Ah, I think you didn't miss anything. Toggling the SR3 WPS does not 
clear any lock bits (BP or page ones), it only selects the locking 
scheme. The SR0 write does clear lock bits (BP ones) using the standard 
locking scheme.

>>> But u-boot should really consider making that a "default n" option.
>>> And most likely adding =y to every existing defconfig out there so
>>> that at least new boards will benefit from it.
>>
>> Yes, I agree with that.
>>
>>>> The way I see this, if Linux ever implements this scheme, Linux can set
>>>> the SR3 WPS bit as needed, it does not matter which way bootloader sets
>>>> the bit as the protection bits are not cleared when the bit is cleared,
>>>> they seem to be stored elsewhere.
>>>
>>> On each reboot you'd wear out that cell with two erases/writes.
>>> That's another reason why that whole unlocking thing is foobar for
>>> non-volatile bits. For me, non-volatile bits are for provisioning,
>>> so during a normal boot they should not be touched at all. Just
>>> during board manufacturing or because the user actively want to
>>> protect something.
>>
>> That is what happens here, the write to clear the bit is triggered only
>> if the bit is set , so OK .
>>
>> And if Linux wants to use the new locking scheme, then the bootloader
>> should ideally be updated to match, i.e. SPI_FLASH_UNLOCK_ALL=n etc, so
>> that is also OK .
> 
> I'd argue if one wants to use the locking at all, you have to set
> UNLOCK_ALL=n. Otherwise, the bootloader might come alone and just
> clear your locking bits again. Clearing the WPS bit there is just
> one more thing which IMHO doesn't make much sense.

On the other hand, if UNLOCK_ALL=y is supposed to work and reset 
locking, then the SR3 WPS bit has to be configured to select the 
standard SPI NOR locking scheme, so the locking can actually be reset 
using that scheme.

>> In other words, there should be no writes into the non-volatile bits,
>> unless U-Boot and Linux disagree on the locking scheme,
> 
> Agreed.
> 
>> in which case
>> writes are unavoidable (if you want unlock to work in both projects).
> 
> But this should only happen if the user want to change the locking
> at all. u-boot should not just do "oh this bit is set, I'm clearing
> it" during SPI flash probe. Again, I'm not caring much if this is
> guarded by the UNLOCK_ALL=y, because u-boot is already doing "oh the
> BP bits are set, lets clear em".

It is guarded, yes.

>>> If you clear this bit during the unlock all command, all the locking
>>> bits are cleared anyway. Or do you mean, the individual bits are
>>> still retained?
>>
>> The lock bits themselves are retained, this SR3 WPS only selects which
>> of those bits take effect, whether the SR ones (standard locking scheme)
>> or the per-page ones (winbond custom locking scheme).
> 
> Ok. So switching back to WPS=1 might come with some suprises :)

Yes, the bad kind.