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

[Michael Walle]

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.

-michael