Re: [PATCH v5 3/5] mtd: spi-nor: otp: return -EROFS if region is read-only
From: Michael Walle
Date: Mon Jun 07 2021 - 06:45:52 EST
Am 2021-06-07 12:30, schrieb Vignesh Raghavendra:
On 6/7/21 3:26 PM, Michael Walle wrote:
Am 2021-06-07 08:47, schrieb Vignesh Raghavendra:
On 6/7/21 11:38 AM, Michael Walle wrote:
Am 2021-06-07 07:46, schrieb Vignesh Raghavendra:
On 6/4/21 6:45 PM, Michael Walle wrote:
Am 2021-06-04 15:07, schrieb Tudor.Ambarus@xxxxxxxxxxxxx:
On 6/4/21 1:02 PM, Michael Walle wrote:
EXTERNAL EMAIL: Do not click links or open attachments unless
you
know the content is safe
SPI NOR flashes will just ignore program commands if the OTP
region is
locked. Thus, a user might not notice that the intended write
didn't end
up in the flash. Return -EROFS to the user in this case. From
what
I can
tell, chips/cfi_cmdset_0001.c also return this error code.
One could optimize spi_nor_mtd_otp_range_is_locked() to read the
status
register only once and not for every OTP region, but for that we
would
need some more invasive changes. Given that this is
one-time-programmable memory and the normal access mode is
reading, we
just live with the small overhead.
Fixes: 069089acf88b ("mtd: spi-nor: add OTP support")
Signed-off-by: Michael Walle <michael@xxxxxxxx>
Reviewed-by: Pratyush Yadav <p.yadav@xxxxxx>
---
drivers/mtd/spi-nor/otp.c | 36
++++++++++++++++++++++++++++++++++++
1 file changed, 36 insertions(+)
diff --git a/drivers/mtd/spi-nor/otp.c
b/drivers/mtd/spi-nor/otp.c
index 3898ed67ba1c..063f8fb68649 100644
--- a/drivers/mtd/spi-nor/otp.c
+++ b/drivers/mtd/spi-nor/otp.c
@@ -249,6 +249,32 @@ static int spi_nor_mtd_otp_info(struct
mtd_info
*mtd, size_t len,
return ret;
}
+static int spi_nor_mtd_otp_range_is_locked(struct spi_nor *nor,
loff_t ofs,
+ size_t len)
+{
+ const struct spi_nor_otp_ops *ops =
nor->params->otp.ops;
+ unsigned int region;
+ int locked;
+
+ if (!len)
+ return 0;
+
You won't need this if you put patch 4/5 before this one. With
this:
This patch will get backported to the stable kernels. Patch 4 on
the
other hand does not.
I don't see why 4/5 cannot be marked for backport too as it makes
3/5
much cleaner?
What kind of problem does 4/5 fix? I can't see how that patch would
apply to any rule in Documentation/process/stable-kernel-rules.rst.
Looking further, I don't see the need for 4/5 to be a separate patch.
Patch 4/5 is simplifying spi_nor_mtd_otp_range_is_locked() by
ensuring
'len' passed is never 0 which can be done in 3/5 when introducing
spi_nor_mtd_otp_range_is_locked().
So why not squashed it into 3/5.
Because, strictly speaking, it is not part of that particular fix
and IMHO violates "It must fix only one thing". But if you're fine
with that, I can squash the two.
TBH I find it kinda funny to bend the rules, just to get rid of
these three lines of code or the ugliness that they will be removed
in the following patch.
This is still fixing only one thing "Indicating OTP writes are ignored
when region is locked" (ie spi_nor_mtd_otp_range_is_locked() check).
But, spi_nor_mtd_otp_range_is_locked() (as in 3/5) can be simplified if
'len != 0' is checked prior to calling the function. That's what 4/5
does which I believe can be squashed here.
Correct, but it also skip the lock/unlock as well as the "*retlen = 0",
which isn't needed to just fix the error, IMHO. I've already asked Tudor
before adding this patch, but I guess I didn't make it clear enough that
one would be for the backporting and one for for-next.
I just don't like code being refactored for the purpose of being able
to
be backported. It feels weird to have a piece of code being added in
one
commit, and then being deleted the very next commit.
So strictly speaking 4/5 has to come before 3/5.
Fair enough. And I tend to agree. But see my reasoning above why I did
it that way.
But I am fine to live with this temporary ugliness if Tudor agrees.
No need ;) I'll squash it. It might even be better, if the two versions
doesn't diverge that much.
-michael