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.