Re: [PATCH v2] mtd: spi-nor: keep lock bits if they are non-volatile
From: Michael Walle
Date: Wed Jan 22 2020 - 07:44:48 EST
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
[...]
I see three choices:
1/ dt prop which gives a per flash granularity. The prop is related
to
hw
protection and there might be some chances to get this accepted,
maybe
it is
worth to involve Rob. But I tend to share Vignesh's opinion, this
would
configure the flash and not describe it.
Still my preferred way. but also see below. But I wouldn't say it
Try to convince Rob.
configures the
flash but describe that the user want to use the write protection.
2/ kconfig option, the behavior would be enforced on all the
flashes.
It would
be similar to what we have with CONFIG_MTD_SPI_NOR_USE_4K_SECTORS. I
did a
patch to address this some time ago:
https://patchwork.ozlabs.org/patch/
1133278/
Mhh. If we would combine this with this patch that would be at least
a
step into
the right direction. At least a distro could enable that kernel
option
without
breaking old boards/flashes. Because as outlined about you need that
for
flashes
in category (2). Or you'd have to do a flash_unlock every time you
want
to write
to it. But that would be really a backwards incompatible change.. ;)
3/ module param, the behavior would be enforced on all the flashes.
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.
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.
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