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

[Michael Walle]

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?

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

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

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

-michael

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