| 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 |
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 >
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 >>
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
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
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
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
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
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 --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) { /*
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(+)