diff mbox series

[v5,3/5] mtd: spi-nor: otp: return -EROFS if region is read-only

Message ID 20210604100252.9975-4-michael@walle.cc
State Superseded
Delegated to: Vignesh R
Headers show
Series mtd: spi-nor: otp: 4 byte mode fix and erase support | expand

Commit Message

Michael Walle June 4, 2021, 10:02 a.m. UTC
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(+)

Comments

Tudor Ambarus June 4, 2021, 1:07 p.m. UTC | #1
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:

Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>

> +       /*
> +        * If any of the affected OTP regions are locked the entire range is
> +        * considered locked.
> +        */
> +       for (region = spi_nor_otp_offset_to_region(nor, ofs);
> +            region <= spi_nor_otp_offset_to_region(nor, ofs + len - 1);
> +            region++) {
> +               locked = ops->is_locked(nor, region);
> +               /* take the branch it is locked or in case of an error */
> +               if (locked)
> +                       return locked;
> +       }
> +
> +       return 0;
> +}
> +
>  static int spi_nor_mtd_otp_read_write(struct mtd_info *mtd, loff_t ofs,
>                                       size_t total_len, size_t *retlen,
>                                       const u8 *buf, bool is_write)
> @@ -271,6 +297,16 @@ static int spi_nor_mtd_otp_read_write(struct mtd_info *mtd, loff_t ofs,
>         /* don't access beyond the end */
>         total_len = min_t(size_t, total_len, spi_nor_otp_size(nor) - ofs);
> 
> +       if (is_write) {
> +               ret = spi_nor_mtd_otp_range_is_locked(nor, ofs, total_len);
> +               if (ret < 0) {
> +                       goto out;
> +               } else if (ret) {
> +                       ret = -EROFS;
> +                       goto out;
> +               }
> +       }
> +
>         *retlen = 0;
>         while (total_len) {
>                 /*
> --
> 2.20.1
>
Michael Walle June 4, 2021, 1:15 p.m. UTC | #2
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.

> 
> Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> 
>> +       /*
>> +        * If any of the affected OTP regions are locked the entire 
>> range is
>> +        * considered locked.
>> +        */
>> +       for (region = spi_nor_otp_offset_to_region(nor, ofs);
>> +            region <= spi_nor_otp_offset_to_region(nor, ofs + len - 
>> 1);
>> +            region++) {
>> +               locked = ops->is_locked(nor, region);
>> +               /* take the branch it is locked or in case of an error 
>> */
>> +               if (locked)
>> +                       return locked;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>>  static int spi_nor_mtd_otp_read_write(struct mtd_info *mtd, loff_t 
>> ofs,
>>                                       size_t total_len, size_t 
>> *retlen,
>>                                       const u8 *buf, bool is_write)
>> @@ -271,6 +297,16 @@ static int spi_nor_mtd_otp_read_write(struct 
>> mtd_info *mtd, loff_t ofs,
>>         /* don't access beyond the end */
>>         total_len = min_t(size_t, total_len, spi_nor_otp_size(nor) - 
>> ofs);
>> 
>> +       if (is_write) {
>> +               ret = spi_nor_mtd_otp_range_is_locked(nor, ofs, 
>> total_len);
>> +               if (ret < 0) {
>> +                       goto out;
>> +               } else if (ret) {
>> +                       ret = -EROFS;
>> +                       goto out;
>> +               }
>> +       }
>> +
>>         *retlen = 0;
>>         while (total_len) {
>>                 /*
>> --
>> 2.20.1
>>
Vignesh Raghavendra June 7, 2021, 5:46 a.m. UTC | #3
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?

Regards
Vignesh
Michael Walle June 7, 2021, 6:08 a.m. UTC | #4
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.

But sure, adding the same Fixes: tag, I can swap those two.

-michael
Vignesh Raghavendra June 7, 2021, 6:47 a.m. UTC | #5
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.

> But sure, adding the same Fixes: tag, I can swap those two.
> 
> -michael
Michael Walle June 7, 2021, 9:56 a.m. UTC | #6
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
Vignesh Raghavendra June 7, 2021, 10:30 a.m. UTC | #7
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@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.
> 

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.

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.

But I am fine to live with this temporary ugliness if Tudor agrees.


Regards
Vignesh
Michael Walle June 7, 2021, 10:45 a.m. UTC | #8
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@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.
>> 
> 
> 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
diff mbox series

Patch

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;
+
+	/*
+	 * If any of the affected OTP regions are locked the entire range is
+	 * considered locked.
+	 */
+	for (region = spi_nor_otp_offset_to_region(nor, ofs);
+	     region <= spi_nor_otp_offset_to_region(nor, ofs + len - 1);
+	     region++) {
+		locked = ops->is_locked(nor, region);
+		/* take the branch it is locked or in case of an error */
+		if (locked)
+			return locked;
+	}
+
+	return 0;
+}
+
 static int spi_nor_mtd_otp_read_write(struct mtd_info *mtd, loff_t ofs,
 				      size_t total_len, size_t *retlen,
 				      const u8 *buf, bool is_write)
@@ -271,6 +297,16 @@  static int spi_nor_mtd_otp_read_write(struct mtd_info *mtd, loff_t ofs,
 	/* don't access beyond the end */
 	total_len = min_t(size_t, total_len, spi_nor_otp_size(nor) - ofs);
 
+	if (is_write) {
+		ret = spi_nor_mtd_otp_range_is_locked(nor, ofs, total_len);
+		if (ret < 0) {
+			goto out;
+		} else if (ret) {
+			ret = -EROFS;
+			goto out;
+		}
+	}
+
 	*retlen = 0;
 	while (total_len) {
 		/*