Messages in this thread | | | Date | Mon, 07 Jun 2021 11:56:33 +0200 | From | Michael Walle <> | Subject | Re: [PATCH v5 3/5] mtd: spi-nor: otp: return -EROFS if region is read-only |
| |
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@microchip.com: >>>>> 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@walle.cc> >>>>>> Reviewed-by: Pratyush Yadav <p.yadav@ti.com> >>>>>> --- >>>>>> 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.
-michael
| |