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

[Marek Vasut]

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.

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

In other words, there should be no writes into the non-volatile bits, 
unless U-Boot and Linux disagree on the locking scheme, in which case 
writes are unavoidable (if you want unlock to work in both projects).

> 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).

>> But, if Linux starts to use SR3 WPS, then U-Boot should be updated to
>> match, i.e. both projects should agree on the locking scheme, so that
>> there won't be a situation where on the same device, one project uses
>> one scheme, the other project uses another scheme. I think this would
>> technically work, but it would be horrible from the user perspective.
>> And if that happens, both projects should then be updated in lockstep
>> and this UNLOCK_ALL should be disabled for such a setup then.
> 
> If you don't touch it, I don't think you have a problem here. u-boot
> and linux can support different schemes, as long as the user is
> aware of that. I.e. if they want to lock a region and the flash is
> configured in a mode which isn't supported in u-boot (or linux) it
> should be rejected. There might of course be a command to switch the
> scheme if someone want to do so.

That is why I wrote that it is technically possible, but probably not a 
good idea because it is inconsistent and therefore error prone.