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

[Michael Walle <michael@walle.cc>]

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@walle.cc>
>> ---
>>  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@ti.com>

Thanks for reviewing!

-michael