Am 2020-01-23 18:20, schrieb Vignesh Raghavendra:
On 1/22/2020 6:14 PM, Michael Walle wrote:
Hi Vignesh,
Am 2020-01-22 13:10, schrieb Vignesh Raghavendra:
On 22/01/20 12:23 am, Tudor.Ambarus@xxxxxxxxxxxxx wrote:
Hi, Michael, Vignesh,
On Sunday, January 12, 2020 12:50:57 AM EET Michael Walle wrote:
EXTERNAL EMAIL: Do not click links or open attachments unless you
know the
content is safe
[...]
[...]
Preferences or suggestions?
I would go with 2/ or 3/. Vignesh, what do you prefer and why?
I dont like option 1, because I am not convinced that this is a HW
description to be put in DT. IIUC, problem is more of what to do
with
locking configuration that is done before Linux comes up(either in
previous boot or by bootloader or POR default). Current code just
discards it and unlocks entire flash.
But this is not the main problem. It is rather the intention of the
user to actually want write protect the flash (for flashes who has
proper support for them, that is the ones which have non-volatile
bits).
Flashes with volatile bits are another subject. Here it might be
useful
to unlock them either at probe time or when we first write to them, so
the user doesn't need to know if its this kind of flash and he would
actually have to unlock the flash before writing. I've left the
behaviour for these flashes as it was before.
And yes, u-boot suffers from the same problem, eg. it unlocks the
whole
flash too. I guess they inherited the behaviour from linux. But I
wanted to start with linux first.
U-Boot only unlocks entire flash in case of Atmel or SST or Intel.
well for now.. until someone thinks its a good idea to follow linux
behaviour.
But proposal is not to touch those bits at probe time and leave this
upto userspace to handle.
No, my proposal was to divide the flashes into two categories. The
unlocking is only done on the flashes which have volatile locking
bits,
thus even when the new option is enabled it won't break access to
these
flashes.
Hmm, looks like before commit 3e0930f109e7 ("mtd: spi-nor: Rework the
disabling of block write protection") global unlock was being done only
for Atmel, SST and Intel flashes. So 3e0930f109e7 definitely changes
this behavior to unlock all flashes that have SPI_NOR_HAS_LOCK set (in
addition to Atmel,SST and Intel).
I think we should just revert to the behavior before 3e0930f109e7 so as
not to break userspace expectation of preserving non volatile BP
setting
across boots
Are we sure there are no Atmel and SST flashes that have non-volatile
BP
bits? If so, then I have no objection for this patch as this
effectively
reverts back to behavior before 3e0930f109e7.
I've gone through all the flashes which were supported (at that time),
all
which have non-volatile have the SPI_NOR_UNPROTECT flag. Also some ESMT
parts have non-volatile flags.
But this patch doesn't restore the state as of before 3e0930f109e7.
Before
this commit any Atmel, Intel and SST flashes were unlocked. With this
patch only the ones are unlocked which have volatile bits.
I'll send a v3 which introduces a new kernel config option with three
choices:
(1) Disable WP on any flashes (legacy behaviour)
(2) Disable WP on flashes w/ volatile protection bits
(3) Keep write protection as is
(1) is enabled by default. One should choose (2) though, because that is
the sane thing to do and (3) is for completeness and the special case if
anyone will actually keep the write protection on flashes with volatile
bits.
I still don't think a kernel parameter is a good idea because you may
miss it and end up with your flash being unprotected.
And i still hope that sometime, (2) may be the default..
-michael
Regards
Vignesh
Adding a Kconfig does not scale well for multi-platform builds. There
would not be a way to have protection enabled on one platform but
disabled on other. Does not scale for multiple flashes either
Option 3 sounds least bad among all. If module param can be designed
to
be a string then, we could control locking behavior to be per flash
using flash name.
What about both? A kconfig option which defines the default for the
kernel parameter? My fear is that once it is a kernel parameter it is
easy to forget (thus having the non-volatile bits, the flash is
completely unlocked again) and it is not very handy; for proper write
protection you'd always have to have this parameter.
btw. I don't see a need to have this option per flash, because once
the user actually enables it, he is aware that its for all of his
flashes. I haven't seen flashes which has non-volatile protection bits
_and_ are protected by default. There shouldn't be a noticable
difference for the user if the option when enabled.
-michael
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
