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

[Michael Walle]

Am 2021-05-25 21:33, schrieb Pratyush Yadav:
> On 21/05/21 09:40PM, Michael Walle wrote:
> > 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.
>
> Ok.
>
> > Fixes: 069089acf88b ("mtd: spi-nor: add OTP support")
> > Signed-off-by: Michael Walle <michael@xxxxxxxx>
> > ---
> >  drivers/mtd/spi-nor/otp.c | 35 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 35 insertions(+)
> >
> > diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
> > index 3898ed67ba1c..b87f96593c13 100644
> > --- a/drivers/mtd/spi-nor/otp.c
> > +++ b/drivers/mtd/spi-nor/otp.c
> > @@ -249,6 +249,31 @@ 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;
>
> I was inclined to say that the loop conditional below would take care 
> of
> this but it can cause an underflow when ofs and len are both 0.

Correct. I didn't want to put an extra check to the caller, because
as you noticed, it is checked by the loop there later.

> > +
> > +	/*
> > +	 * 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);
> > +		if (locked)
> > +			return locked;
> > +	}
>
> Ok.

Btw I didn't know if I should put a comment here that this if () handles
both locked state and errors. But it seems you've already found out by
looking at the caller ;) I'm not sure if this is obvious, though.

> > +
> > +	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 +296,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;
>
> I wonder if we should have a dev_info() or dev_err() here. I think this
> warrants a dev_dbg() at least.

Are you sure? Reporting something to the user via an error code is
enough IMHO. I wouldn't want my syslog to be cluttered with messages
I already know. I mean the program tell me "hey, you aren't allowed
to write there". Why would the kernel still need to tell me that again?
Without any connection to the caller, I don't get much out of the kernel
message by looking at it alone, just that someone tried to write there.

So definetly no dev_info() or dev_err(). But IMHO no dev_dbg() either.
Tudor, Vingesh, any opinions?


> > +			goto out;
> > +		}
>
> So it returns -errno when the check for is_locked() fails and 1 or 0
> when it is locked or not. Ok.
>
> It would be nice if you add a dev_dbg or dev_err() or dev_info() above.
> Nonetheless,
>
> Reviewed-by: Pratyush Yadav <p.yadav@xxxxxx>

Thanks for reviewing!

-michael