Patchwork [v2,RESEND,2/2] mtd: Fix the behavior of otp write if there is not enough room for data

login
register
mail settings
Submitter Brian Norris
Date March 5, 2014, 7:20 a.m.
Message ID <20140305072010.GL13420@norris-Latitude-E6410>
Download mbox | patch
Permalink /patch/326605/
State New
Headers show

Comments

Brian Norris - March 5, 2014, 7:20 a.m.
Hi Christian,

A few comments below.

On Tue, Jan 28, 2014 at 09:29:45AM +0100, Christian Riesch wrote:
> An OTP write shall write as much data as possible to the OTP memory
> and return the number of bytes that have actually been written.
> If no data could be written at all due to lack of OTP memory,
> return -ENOSPC.
> 
> Signed-off-by: Christian Riesch <christian.riesch@omicron.at>
> Cc: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> Cc: Amul Kumar Saha <amul.saha@samsung.com>
> ---
>  drivers/mtd/chips/cfi_cmdset_0001.c |   13 +++++++++++--
>  drivers/mtd/devices/mtd_dataflash.c |   13 +++++--------
>  drivers/mtd/mtdchar.c               |    7 +++++++
>  drivers/mtd/onenand/onenand_base.c  |   10 +++++++++-
>  4 files changed, 32 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/mtd/chips/cfi_cmdset_0001.c b/drivers/mtd/chips/cfi_cmdset_0001.c
> index 7aa581f..cf423a6 100644
> --- a/drivers/mtd/chips/cfi_cmdset_0001.c
> +++ b/drivers/mtd/chips/cfi_cmdset_0001.c
> @@ -2387,8 +2387,17 @@ static int cfi_intelext_write_user_prot_reg(struct mtd_info *mtd, loff_t from,
>  					    size_t len, size_t *retlen,
>  					     u_char *buf)
>  {
> -	return cfi_intelext_otp_walk(mtd, from, len, retlen,
> -				     buf, do_otp_write, 1);
> +	int ret;
> +
> +	ret = cfi_intelext_otp_walk(mtd, from, len, retlen,
> +				    buf, do_otp_write, 1);
> +
> +	/* if no data could be written due to lack of OTP memory,
> +	   return ENOSPC */

/*
 * Can you use this style of mult-line comments please?
 * It's in Documentation/CodingStyle
 */

> +	if (!ret && len && !(*retlen))
> +		return -ENOSPC;

Couldn't (shouldn't) this check be pushed to the common
mtd_write_user_prot_reg() helper in mtdcore.c? And once you do that, you
will see that cfi_intelext_write_user_prot_reg() (and other
mtd->_write_user_prot_reg() implementations) will never be called with
len == 0. So this just becomes (in mtdcore.c):


> +
> +	return ret;
>  }
>  
>  static int cfi_intelext_lock_user_prot_reg(struct mtd_info *mtd,
> diff --git a/drivers/mtd/devices/mtd_dataflash.c b/drivers/mtd/devices/mtd_dataflash.c
> index 09c69ce..5236d85 100644
> --- a/drivers/mtd/devices/mtd_dataflash.c
> +++ b/drivers/mtd/devices/mtd_dataflash.c
> @@ -545,14 +545,11 @@ static int dataflash_write_user_otp(struct mtd_info *mtd,
>  	struct dataflash	*priv = mtd->priv;
>  	int			status;
>  

I'm not sure I quite follow the logic for the following hunk. I think it
deserves some more explanation, either in your commit or in a comment.
As it stands, you're deleting a comment and potentially changing the
return code behavior subtly.

> -	if (len > 64)
> -		return -EINVAL;
> -
> -	/* Strictly speaking, we *could* truncate the write ... but
> -	 * let's not do that for the only write that's ever possible.
> -	 */
> -	if ((from + len) > 64)
> -		return -EINVAL;
> +	if ((from + len) > 64) {
> +		len = 64 - from;

Why are you reassigning len? Are you trying to undo the comment above,
so that you *can* truncate the write? (It looks like there are other
implmentations which will truncate the write and return -ENOSPC, FWIW.)

> +		if (len <= 0)
> +			return -ENOSPC;
> +	}
>  
>  	/* OUT: OP_WRITE_SECURITY, 3 zeroes, 64 data-or-zero bytes
>  	 * IN:  ignore all
> diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
> index 0edb0ca..db99031 100644
> --- a/drivers/mtd/mtdchar.c
> +++ b/drivers/mtd/mtdchar.c
> @@ -323,6 +323,13 @@ static ssize_t mtdchar_write(struct file *file, const char __user *buf, size_t c
>  		default:
>  			ret = mtd_write(mtd, *ppos, len, &retlen, kbuf);
>  		}
> +		/* return -ENOSPC only if no data was written */
> +		if ((ret == -ENOSPC) && (total_retlen)) {
> +			ret = 0;
> +			retlen = 0;
> +			/* drop the remaining data */
> +			count = 0;

This block can just be a 'break' statement, no?

> +		}

I'm a bit wary of changing the behavior of non-OTP writes. At a minimum,
the patch description needs to acknowledge that this affects more than
just OTP writes. But after a cursory review of mtd->_write()
implementations, it looks like there's no driver which could be
returning -ENOSPC already, so this change is probably OK.

>  		if (!ret) {
>  			*ppos += retlen;
>  			total_retlen += retlen;
> diff --git a/drivers/mtd/onenand/onenand_base.c b/drivers/mtd/onenand/onenand_base.c
> index 419c538..6c49a6f 100644
> --- a/drivers/mtd/onenand/onenand_base.c
> +++ b/drivers/mtd/onenand/onenand_base.c
> @@ -3316,7 +3316,15 @@ static int onenand_read_user_prot_reg(struct mtd_info *mtd, loff_t from,
>  static int onenand_write_user_prot_reg(struct mtd_info *mtd, loff_t from,
>  			size_t len, size_t *retlen, u_char *buf)
>  {
> -	return onenand_otp_walk(mtd, from, len, retlen, buf, do_otp_write, MTD_OTP_USER);
> +	int ret;
> +	ret = onenand_otp_walk(mtd, from, len, retlen, buf, do_otp_write, MTD_OTP_USER);
> +
> +	/* if no data could be written due to lack of OTP memory,
> +	   return ENOSPC */
> +	if (!ret && len && !(*retlen))
> +		return -ENOSPC;

Same comments from cfi_intelext_write_user_prot_reg(), so I think this
change can be dropped. (And again, 'len' never will be 0.)

> +
> +	return ret;
>  }
>  
>  /**

Brian
Brian Norris - March 5, 2014, 7:35 a.m.
A few more things...

On Tue, Mar 04, 2014 at 11:20:10PM -0800, Brian Norris wrote:
> On Tue, Jan 28, 2014 at 09:29:45AM +0100, Christian Riesch wrote:
> > An OTP write shall write as much data as possible to the OTP memory
> > and return the number of bytes that have actually been written.
> > If no data could be written at all due to lack of OTP memory,
> > return -ENOSPC.
[snip]
> > diff --git a/drivers/mtd/chips/cfi_cmdset_0001.c b/drivers/mtd/chips/cfi_cmdset_0001.c
> > index 7aa581f..cf423a6 100644
> > --- a/drivers/mtd/chips/cfi_cmdset_0001.c
> > +++ b/drivers/mtd/chips/cfi_cmdset_0001.c
> > @@ -2387,8 +2387,17 @@ static int cfi_intelext_write_user_prot_reg(struct mtd_info *mtd, loff_t from,
> >  					    size_t len, size_t *retlen,
> >  					     u_char *buf)
> >  {
> > -	return cfi_intelext_otp_walk(mtd, from, len, retlen,
> > -				     buf, do_otp_write, 1);
> > +	int ret;
> > +
> > +	ret = cfi_intelext_otp_walk(mtd, from, len, retlen,
> > +				    buf, do_otp_write, 1);
> > +
> > +	/* if no data could be written due to lack of OTP memory,
> > +	   return ENOSPC */
> 
> /*
>  * Can you use this style of mult-line comments please?
>  * It's in Documentation/CodingStyle
>  */
> 
> > +	if (!ret && len && !(*retlen))
> > +		return -ENOSPC;
> 
> Couldn't (shouldn't) this check be pushed to the common
> mtd_write_user_prot_reg() helper in mtdcore.c? And once you do that, you
> will see that cfi_intelext_write_user_prot_reg() (and other
> mtd->_write_user_prot_reg() implementations) will never be called with
> len == 0. So this just becomes (in mtdcore.c):
> 
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 0a7d77e65335..ee6730748f7e 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -909,11 +909,16 @@ EXPORT_SYMBOL_GPL(mtd_read_fact_prot_reg);
>  int mtd_get_user_prot_info(struct mtd_info *mtd, size_t len, size_t *retlen,
>  			   struct otp_info *buf)
>  {
> +	int ret;
> +
>  	if (!mtd->_get_user_prot_info)
>  		return -EOPNOTSUPP;
>  	if (!len)
>  		return 0;
> -	return mtd->_get_user_prot_info(mtd, len, retlen, buf);
> +	ret = mtd->_get_user_prot_info(mtd, len, retlen, buf);
> +	if (ret)
> +		return ret;
> +	return !(*retlen) ? -ENOSPC: 0;
>  }
>  EXPORT_SYMBOL_GPL(mtd_get_user_prot_info);

Sorry, I patched the wrong function here! Please use your brain and
apply this to the OTP write function :)

> > +
> > +	return ret;
> >  }
> >  
> >  static int cfi_intelext_lock_user_prot_reg(struct mtd_info *mtd,
> > diff --git a/drivers/mtd/devices/mtd_dataflash.c b/drivers/mtd/devices/mtd_dataflash.c
> > index 09c69ce..5236d85 100644
> > --- a/drivers/mtd/devices/mtd_dataflash.c
> > +++ b/drivers/mtd/devices/mtd_dataflash.c
> > @@ -545,14 +545,11 @@ static int dataflash_write_user_otp(struct mtd_info *mtd,
> >  	struct dataflash	*priv = mtd->priv;
> >  	int			status;
> >  
> 
> I'm not sure I quite follow the logic for the following hunk. I think it
> deserves some more explanation, either in your commit or in a comment.
> As it stands, you're deleting a comment and potentially changing the
> return code behavior subtly.
> 
> > -	if (len > 64)
> > -		return -EINVAL;
> > -
> > -	/* Strictly speaking, we *could* truncate the write ... but
> > -	 * let's not do that for the only write that's ever possible.
> > -	 */
> > -	if ((from + len) > 64)
> > -		return -EINVAL;
> > +	if ((from + len) > 64) {
> > +		len = 64 - from;
> 
> Why are you reassigning len? Are you trying to undo the comment above,
> so that you *can* truncate the write? (It looks like there are other
> implmentations which will truncate the write and return -ENOSPC, FWIW.)
> 
> > +		if (len <= 0)
> > +			return -ENOSPC;
> > +	}

I looked a bit more at [1] and it looks like you're actually trying to
straighten out some inconsistencies (hence the "harmonizing" in
$subject). I think this warrants:

(1) A little more in the commit message. You describe the new policy,
    but you should also note *how* this is changing existing
    implementations.

(2) A comment next to mtd_write_user_prot_reg() to describe the new
    harmony.

> >  
> >  	/* OUT: OP_WRITE_SECURITY, 3 zeroes, 64 data-or-zero bytes
> >  	 * IN:  ignore all

[1] http://patchwork.ozlabs.org/patch/239897/

Brian
Christian Riesch - March 5, 2014, 8:50 a.m.
Hi Brian,
Thank you very much for your comments on the patch!

--On March 04, 2014 23:20 -0800 Brian Norris <computersforpeace@gmail.com> 
wrote:

> Hi Christian,
>
> A few comments below.
>
> On Tue, Jan 28, 2014 at 09:29:45AM +0100, Christian Riesch wrote:
>> An OTP write shall write as much data as possible to the OTP memory
>> and return the number of bytes that have actually been written.
>> If no data could be written at all due to lack of OTP memory,
>> return -ENOSPC.
>>
>> Signed-off-by: Christian Riesch <christian.riesch@omicron.at>
>> Cc: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
>> Cc: Kyungmin Park <kyungmin.park@samsung.com>
>> Cc: Amul Kumar Saha <amul.saha@samsung.com>
>> ---
>>  drivers/mtd/chips/cfi_cmdset_0001.c |   13 +++++++++++--
>>  drivers/mtd/devices/mtd_dataflash.c |   13 +++++--------
>>  drivers/mtd/mtdchar.c               |    7 +++++++
>>  drivers/mtd/onenand/onenand_base.c  |   10 +++++++++-
>>  4 files changed, 32 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/mtd/chips/cfi_cmdset_0001.c
>> b/drivers/mtd/chips/cfi_cmdset_0001.c index 7aa581f..cf423a6 100644
>> --- a/drivers/mtd/chips/cfi_cmdset_0001.c
>> +++ b/drivers/mtd/chips/cfi_cmdset_0001.c
>> @@ -2387,8 +2387,17 @@ static int
>> cfi_intelext_write_user_prot_reg(struct mtd_info *mtd, loff_t from,
>>  					    size_t len, size_t *retlen,
>>  					     u_char *buf)
>>  {
>> -	return cfi_intelext_otp_walk(mtd, from, len, retlen,
>> -				     buf, do_otp_write, 1);
>> +	int ret;
>> +
>> +	ret = cfi_intelext_otp_walk(mtd, from, len, retlen,
>> +				    buf, do_otp_write, 1);
>> +
>> +	/* if no data could be written due to lack of OTP memory,
>> +	   return ENOSPC */
>
> /*
>  * Can you use this style of mult-line comments please?
>  * It's in Documentation/CodingStyle
>  */
>

Ok, I will change that.

>> +	if (!ret && len && !(*retlen))
>> +		return -ENOSPC;
>
> Couldn't (shouldn't) this check be pushed to the common
> mtd_write_user_prot_reg() helper in mtdcore.c?

Yes, I don't see why this wouldn't work. But I thought the code would be 
easier to understand if we return the correct error code as soon as the 
error is detected, not using some additional logic in some other function. 
What do you think?

> And once you do that, you
> will see that cfi_intelext_write_user_prot_reg() (and other
> mtd->_write_user_prot_reg() implementations) will never be called with
> len == 0. So this just becomes (in mtdcore.c):
>
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 0a7d77e65335..ee6730748f7e 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -909,11 +909,16 @@ EXPORT_SYMBOL_GPL(mtd_read_fact_prot_reg);
>  int mtd_get_user_prot_info(struct mtd_info *mtd, size_t len, size_t
> *retlen,  			   struct otp_info *buf)
>  {
> +	int ret;
> +
>  	if (!mtd->_get_user_prot_info)
>  		return -EOPNOTSUPP;
>  	if (!len)
>  		return 0;
> -	return mtd->_get_user_prot_info(mtd, len, retlen, buf);
> +	ret = mtd->_get_user_prot_info(mtd, len, retlen, buf);
> +	if (ret)
> +		return ret;
> +	return !(*retlen) ? -ENOSPC: 0;
>  }
>  EXPORT_SYMBOL_GPL(mtd_get_user_prot_info);
>
>
>> +
>> +	return ret;
>>  }
>>
>>  static int cfi_intelext_lock_user_prot_reg(struct mtd_info *mtd,
>> diff --git a/drivers/mtd/devices/mtd_dataflash.c
>> b/drivers/mtd/devices/mtd_dataflash.c index 09c69ce..5236d85 100644
>> --- a/drivers/mtd/devices/mtd_dataflash.c
>> +++ b/drivers/mtd/devices/mtd_dataflash.c
>> @@ -545,14 +545,11 @@ static int dataflash_write_user_otp(struct
>> mtd_info *mtd, struct dataflash	*priv = mtd->priv;
>>  	int			status;
>>
>
> I'm not sure I quite follow the logic for the following hunk. I think it
> deserves some more explanation, either in your commit or in a comment.
> As it stands, you're deleting a comment and potentially changing the
> return code behavior subtly.
>
>> -	if (len > 64)
>> -		return -EINVAL;
>> -
>> -	/* Strictly speaking, we *could* truncate the write ... but
>> -	 * let's not do that for the only write that's ever possible.
>> -	 */
>> -	if ((from + len) > 64)
>> -		return -EINVAL;
>> +	if ((from + len) > 64) {
>> +		len = 64 - from;
>
> Why are you reassigning len? Are you trying to undo the comment above,
> so that you *can* truncate the write? (It looks like there are other
> implmentations which will truncate the write and return -ENOSPC, FWIW.)

Currently we have two kind of implementations: We have implementations like 
this one which will refuse to write any data if the write requests more 
data to be written than space is available. And we have implementations 
like cfi_intelext_write_user_prot_reg that will truncate the write and 
write as much data that is possible (and return the number of bytes that 
actually have been written, -ENOSPC shall only be returned if no data could 
be written at all).

For a harmonization one of the implementations and their behavior must be 
changed. I chose to change it to "write as much as possible/truncate the 
write" since this is how a write should behave 
(http://pubs.opengroup.org/onlinepubs/9699919799/functions/write.html). And 
yes, this is why I try to undo the comment.

But if you are afraid that this will break things for current users of the 
functions, I would keep the old behavior. What do you think?

>
>> +		if (len <= 0)
>> +			return -ENOSPC;
>> +	}
>>
>>  	/* OUT: OP_WRITE_SECURITY, 3 zeroes, 64 data-or-zero bytes
>>  	 * IN:  ignore all
>> diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
>> index 0edb0ca..db99031 100644
>> --- a/drivers/mtd/mtdchar.c
>> +++ b/drivers/mtd/mtdchar.c
>> @@ -323,6 +323,13 @@ static ssize_t mtdchar_write(struct file *file,
>> const char __user *buf, size_t c default:
>>  			ret = mtd_write(mtd, *ppos, len, &retlen, kbuf);
>>  		}
>> +		/* return -ENOSPC only if no data was written */
>> +		if ((ret == -ENOSPC) && (total_retlen)) {
>> +			ret = 0;
>> +			retlen = 0;
>> +			/* drop the remaining data */
>> +			count = 0;
>
> This block can just be a 'break' statement, no?

No. mtdchar_write may split the write into several calls of mtd_write, 
mtd_write_user_prot_reg... It will call mtd_write, mtd_write_user_prot_reg 
as long as there is data to be written. If the write hits the boundary of 
the memory, the last call of mtd_write_user_prot_reg will return -ENOSPC. 
If this was the only call of mtd_write_user_prot_reg (so no data could be 
written at all), returning -ENOSPC to the user is fine. However, if data 
has been written before, we must not return -ENOSPC, we must return the 
number of bytes that have actually been written.

So at least it must be

if ((ret == -ENOSPC) && (total_retlen)) {
	ret = 0;
	break;
}

Which one do you prefer?

>
>> +		}
>
> I'm a bit wary of changing the behavior of non-OTP writes. At a minimum,
> the patch description needs to acknowledge that this affects more than
> just OTP writes. But after a cursory review of mtd->_write()
> implementations, it looks like there's no driver which could be
> returning -ENOSPC already, so this change is probably OK.

The behavior of non-OTP writes is not changed at all. At the begin of 
mtdchar_write, a check against mtd->size is done, and the write is 
truncated. Therefore, non-OTP writes will never hit the end of memory in 
the write function.

Regards, Christian

>
>>  		if (!ret) {
>>  			*ppos += retlen;
>>  			total_retlen += retlen;
>> diff --git a/drivers/mtd/onenand/onenand_base.c
>> b/drivers/mtd/onenand/onenand_base.c index 419c538..6c49a6f 100644
>> --- a/drivers/mtd/onenand/onenand_base.c
>> +++ b/drivers/mtd/onenand/onenand_base.c
>> @@ -3316,7 +3316,15 @@ static int onenand_read_user_prot_reg(struct
>> mtd_info *mtd, loff_t from, static int
>>  onenand_write_user_prot_reg(struct mtd_info *mtd, loff_t from, size_t
>>  			len, size_t *retlen, u_char *buf)
>>  {
>> -	return onenand_otp_walk(mtd, from, len, retlen, buf, do_otp_write,
>> MTD_OTP_USER); +	int ret;
>> +	ret = onenand_otp_walk(mtd, from, len, retlen, buf, do_otp_write,
>> MTD_OTP_USER); +
>> +	/* if no data could be written due to lack of OTP memory,
>> +	   return ENOSPC */
>> +	if (!ret && len && !(*retlen))
>> +		return -ENOSPC;
>
> Same comments from cfi_intelext_write_user_prot_reg(), so I think this
> change can be dropped. (And again, 'len' never will be 0.)
>
>> +
>> +	return ret;
>>  }
>>
>>  /**
>
> Brian
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
Brian Norris - March 6, 2014, 8:49 a.m.
Hi,

On Wed, Mar 05, 2014 at 09:50:35AM +0100, Christian Riesch wrote:
> On March 04, 2014 23:20 -0800 Brian Norris <computersforpeace@gmail.com> wrote:
> >On Tue, Jan 28, 2014 at 09:29:45AM +0100, Christian Riesch wrote:
> >>An OTP write shall write as much data as possible to the OTP memory
> >>and return the number of bytes that have actually been written.
> >>If no data could be written at all due to lack of OTP memory,
> >>return -ENOSPC.
> >>
> >>Signed-off-by: Christian Riesch <christian.riesch@omicron.at>
> >>Cc: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> >>Cc: Kyungmin Park <kyungmin.park@samsung.com>
> >>Cc: Amul Kumar Saha <amul.saha@samsung.com>
> >>---
> >> drivers/mtd/chips/cfi_cmdset_0001.c |   13 +++++++++++--
> >> drivers/mtd/devices/mtd_dataflash.c |   13 +++++--------
> >> drivers/mtd/mtdchar.c               |    7 +++++++
> >> drivers/mtd/onenand/onenand_base.c  |   10 +++++++++-
> >> 4 files changed, 32 insertions(+), 11 deletions(-)
> >>
> >>diff --git a/drivers/mtd/chips/cfi_cmdset_0001.c
> >>b/drivers/mtd/chips/cfi_cmdset_0001.c index 7aa581f..cf423a6 100644
> >>--- a/drivers/mtd/chips/cfi_cmdset_0001.c
> >>+++ b/drivers/mtd/chips/cfi_cmdset_0001.c
> >>@@ -2387,8 +2387,17 @@ static int
> >>cfi_intelext_write_user_prot_reg(struct mtd_info *mtd, loff_t from,
> >> 					    size_t len, size_t *retlen,
> >> 					     u_char *buf)
> >> {
> >>-	return cfi_intelext_otp_walk(mtd, from, len, retlen,
> >>-				     buf, do_otp_write, 1);
> >>+	int ret;
> >>+
> >>+	ret = cfi_intelext_otp_walk(mtd, from, len, retlen,
> >>+				    buf, do_otp_write, 1);
> >>+
> >>+	/* if no data could be written due to lack of OTP memory,
> >>+	   return ENOSPC */
> >
> >/*
> > * Can you use this style of mult-line comments please?
> > * It's in Documentation/CodingStyle
> > */
> >
> 
> Ok, I will change that.
> 
> >>+	if (!ret && len && !(*retlen))
> >>+		return -ENOSPC;
> >
> >Couldn't (shouldn't) this check be pushed to the common
> >mtd_write_user_prot_reg() helper in mtdcore.c?
> 
> Yes, I don't see why this wouldn't work. But I thought the code
> would be easier to understand if we return the correct error code as
> soon as the error is detected, not using some additional logic in
> some other function. What do you think?

No, this is the purpose of the mtd_xxx() wrappers for the mtd->_xxx
implementations. That way we don't have to inconsistently implement the
same checks in every driver. This caught a few bugs for
mtd_{read,write}() when we unified the bounds checking, I think.

> >And once you do that, you
> >will see that cfi_intelext_write_user_prot_reg() (and other
> >mtd->_write_user_prot_reg() implementations) will never be called with
> >len == 0. So this just becomes (in mtdcore.c):
> >
> >diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> >index 0a7d77e65335..ee6730748f7e 100644
> >--- a/drivers/mtd/mtdcore.c
> >+++ b/drivers/mtd/mtdcore.c
> >@@ -909,11 +909,16 @@ EXPORT_SYMBOL_GPL(mtd_read_fact_prot_reg);
> > int mtd_get_user_prot_info(struct mtd_info *mtd, size_t len, size_t
> >*retlen,  			   struct otp_info *buf)
> > {
> >+	int ret;
> >+
> > 	if (!mtd->_get_user_prot_info)
> > 		return -EOPNOTSUPP;
> > 	if (!len)
> > 		return 0;
> >-	return mtd->_get_user_prot_info(mtd, len, retlen, buf);
> >+	ret = mtd->_get_user_prot_info(mtd, len, retlen, buf);
> >+	if (ret)
> >+		return ret;
> >+	return !(*retlen) ? -ENOSPC: 0;
> > }
> > EXPORT_SYMBOL_GPL(mtd_get_user_prot_info);
> >
> >
> >>+
> >>+	return ret;
> >> }
> >>
> >> static int cfi_intelext_lock_user_prot_reg(struct mtd_info *mtd,
> >>diff --git a/drivers/mtd/devices/mtd_dataflash.c
> >>b/drivers/mtd/devices/mtd_dataflash.c index 09c69ce..5236d85 100644
> >>--- a/drivers/mtd/devices/mtd_dataflash.c
> >>+++ b/drivers/mtd/devices/mtd_dataflash.c
> >>@@ -545,14 +545,11 @@ static int dataflash_write_user_otp(struct
> >>mtd_info *mtd, struct dataflash	*priv = mtd->priv;
> >> 	int			status;
> >>
> >
> >I'm not sure I quite follow the logic for the following hunk. I think it
> >deserves some more explanation, either in your commit or in a comment.
> >As it stands, you're deleting a comment and potentially changing the
> >return code behavior subtly.
> >
> >>-	if (len > 64)
> >>-		return -EINVAL;
> >>-
> >>-	/* Strictly speaking, we *could* truncate the write ... but
> >>-	 * let's not do that for the only write that's ever possible.
> >>-	 */
> >>-	if ((from + len) > 64)
> >>-		return -EINVAL;
> >>+	if ((from + len) > 64) {
> >>+		len = 64 - from;
> >
> >Why are you reassigning len? Are you trying to undo the comment above,
> >so that you *can* truncate the write? (It looks like there are other
> >implmentations which will truncate the write and return -ENOSPC, FWIW.)
> 
> Currently we have two kind of implementations: We have
> implementations like this one which will refuse to write any data if
> the write requests more data to be written than space is available.
> And we have implementations like cfi_intelext_write_user_prot_reg
> that will truncate the write and write as much data that is possible
> (and return the number of bytes that actually have been written,
> -ENOSPC shall only be returned if no data could be written at all).
> 
> For a harmonization one of the implementations and their behavior
> must be changed. I chose to change it to "write as much as
> possible/truncate the write" since this is how a write should behave (http://pubs.opengroup.org/onlinepubs/9699919799/functions/write.html).
> And yes, this is why I try to undo the comment.

OK, that makes sense. Then I think you should add a comment here in
dataflash_write_user_otp() to say that you *are* truncating (or possibly
rearrange the logic?), as it's not 100% clear what you're trying to do
here. And add a small blurb to note this in the commit description. Some
version of the above two paragraphs would make a nice
addition/replacement to the patch description.

> But if you are afraid that this will break things for current users
> of the functions, I would keep the old behavior. What do you think?

No, I believe your semantics make sense, and it's not a major breakage.

> >
> >>+		if (len <= 0)
> >>+			return -ENOSPC;
> >>+	}
> >>
> >> 	/* OUT: OP_WRITE_SECURITY, 3 zeroes, 64 data-or-zero bytes
> >> 	 * IN:  ignore all
> >>diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
> >>index 0edb0ca..db99031 100644
> >>--- a/drivers/mtd/mtdchar.c
> >>+++ b/drivers/mtd/mtdchar.c
> >>@@ -323,6 +323,13 @@ static ssize_t mtdchar_write(struct file *file,
> >>const char __user *buf, size_t c default:
> >> 			ret = mtd_write(mtd, *ppos, len, &retlen, kbuf);
> >> 		}
> >>+		/* return -ENOSPC only if no data was written */
> >>+		if ((ret == -ENOSPC) && (total_retlen)) {
> >>+			ret = 0;
> >>+			retlen = 0;
> >>+			/* drop the remaining data */
> >>+			count = 0;
> >
> >This block can just be a 'break' statement, no?
> 
> No. mtdchar_write may split the write into several calls of
> mtd_write, mtd_write_user_prot_reg... It will call mtd_write,
> mtd_write_user_prot_reg as long as there is data to be written. If
> the write hits the boundary of the memory, the last call of
> mtd_write_user_prot_reg will return -ENOSPC. If this was the only
> call of mtd_write_user_prot_reg (so no data could be written at
> all), returning -ENOSPC to the user is fine. However, if data has
> been written before, we must not return -ENOSPC, we must return the
> number of bytes that have actually been written.
> 
> So at least it must be
> 
> if ((ret == -ENOSPC) && (total_retlen)) {
> 	ret = 0;

^^^ this line will have no effect, since 'ret' is not used outside the
while loop.

> 	break;
> }
> 
> Which one do you prefer?

I prefer the following :) It's fewer lines and more straightforward, I
think.

if ((ret == -ENOSPC) && total_retlen)
	break;

> >
> >>+		}
> >
> >I'm a bit wary of changing the behavior of non-OTP writes. At a minimum,
> >the patch description needs to acknowledge that this affects more than
> >just OTP writes. But after a cursory review of mtd->_write()
> >implementations, it looks like there's no driver which could be
> >returning -ENOSPC already, so this change is probably OK.
> 
> The behavior of non-OTP writes is not changed at all. At the begin
> of mtdchar_write, a check against mtd->size is done, and the write
> is truncated. Therefore, non-OTP writes will never hit the end of
> memory in the write function.

Right, and actually mtd_write() never gives -ENOSPC; it returns -EINVAL.
So this is fine.

Brian
Christian Riesch - March 6, 2014, 8:57 a.m.
Hi Brian,

--On March 06, 2014 00:49 -0800 Brian Norris <computersforpeace@gmail.com> 
wrote:

> Hi,
>
> On Wed, Mar 05, 2014 at 09:50:35AM +0100, Christian Riesch wrote:
>> On March 04, 2014 23:20 -0800 Brian Norris <computersforpeace@gmail.com>
>> wrote:
>> > On Tue, Jan 28, 2014 at 09:29:45AM +0100, Christian Riesch wrote:
>> >> An OTP write shall write as much data as possible to the OTP memory
>> >> and return the number of bytes that have actually been written.
>> >> If no data could be written at all due to lack of OTP memory,
>> >> return -ENOSPC.
>> >>
>> >> Signed-off-by: Christian Riesch <christian.riesch@omicron.at>
>> >> Cc: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
>> >> Cc: Kyungmin Park <kyungmin.park@samsung.com>
>> >> Cc: Amul Kumar Saha <amul.saha@samsung.com>
>> >> ---
>> >> drivers/mtd/chips/cfi_cmdset_0001.c |   13 +++++++++++--
>> >> drivers/mtd/devices/mtd_dataflash.c |   13 +++++--------
>> >> drivers/mtd/mtdchar.c               |    7 +++++++
>> >> drivers/mtd/onenand/onenand_base.c  |   10 +++++++++-
>> >> 4 files changed, 32 insertions(+), 11 deletions(-)
>> >>
>> >> diff --git a/drivers/mtd/chips/cfi_cmdset_0001.c
>> >> b/drivers/mtd/chips/cfi_cmdset_0001.c index 7aa581f..cf423a6 100644
>> >> --- a/drivers/mtd/chips/cfi_cmdset_0001.c
>> >> +++ b/drivers/mtd/chips/cfi_cmdset_0001.c
>> >> @@ -2387,8 +2387,17 @@ static int
>> >> cfi_intelext_write_user_prot_reg(struct mtd_info *mtd, loff_t from,
>> >> 					    size_t len, size_t *retlen,
>> >> 					     u_char *buf)
>> >> {
>> >> -	return cfi_intelext_otp_walk(mtd, from, len, retlen,
>> >> -				     buf, do_otp_write, 1);
>> >> +	int ret;
>> >> +
>> >> +	ret = cfi_intelext_otp_walk(mtd, from, len, retlen,
>> >> +				    buf, do_otp_write, 1);
>> >> +
>> >> +	/* if no data could be written due to lack of OTP memory,
>> >> +	   return ENOSPC */
>> >
>> > /*
>> > * Can you use this style of mult-line comments please?
>> > * It's in Documentation/CodingStyle
>> > */
>> >
>>
>> Ok, I will change that.
>>
>> >> +	if (!ret && len && !(*retlen))
>> >> +		return -ENOSPC;
>> >
>> > Couldn't (shouldn't) this check be pushed to the common
>> > mtd_write_user_prot_reg() helper in mtdcore.c?
>>
>> Yes, I don't see why this wouldn't work. But I thought the code
>> would be easier to understand if we return the correct error code as
>> soon as the error is detected, not using some additional logic in
>> some other function. What do you think?
>
> No, this is the purpose of the mtd_xxx() wrappers for the mtd->_xxx
> implementations. That way we don't have to inconsistently implement the
> same checks in every driver. This caught a few bugs for
> mtd_{read,write}() when we unified the bounds checking, I think.

Ok, I will change that.

>> > And once you do that, you
>> > will see that cfi_intelext_write_user_prot_reg() (and other
>> > mtd->_write_user_prot_reg() implementations) will never be called with
>> > len == 0. So this just becomes (in mtdcore.c):
>> >
>> > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
>> > index 0a7d77e65335..ee6730748f7e 100644
>> > --- a/drivers/mtd/mtdcore.c
>> > +++ b/drivers/mtd/mtdcore.c
>> > @@ -909,11 +909,16 @@ EXPORT_SYMBOL_GPL(mtd_read_fact_prot_reg);
>> > int mtd_get_user_prot_info(struct mtd_info *mtd, size_t len, size_t
>> > *retlen,  			   struct otp_info *buf)
>> > {
>> > +	int ret;
>> > +
>> > 	if (!mtd->_get_user_prot_info)
>> > 		return -EOPNOTSUPP;
>> > 	if (!len)
>> > 		return 0;
>> > -	return mtd->_get_user_prot_info(mtd, len, retlen, buf);
>> > +	ret = mtd->_get_user_prot_info(mtd, len, retlen, buf);
>> > +	if (ret)
>> > +		return ret;
>> > +	return !(*retlen) ? -ENOSPC: 0;
>> > }
>> > EXPORT_SYMBOL_GPL(mtd_get_user_prot_info);
>> >
>> >
>> >> +
>> >> +	return ret;
>> >> }
>> >>
>> >> static int cfi_intelext_lock_user_prot_reg(struct mtd_info *mtd,
>> >> diff --git a/drivers/mtd/devices/mtd_dataflash.c
>> >> b/drivers/mtd/devices/mtd_dataflash.c index 09c69ce..5236d85 100644
>> >> --- a/drivers/mtd/devices/mtd_dataflash.c
>> >> +++ b/drivers/mtd/devices/mtd_dataflash.c
>> >> @@ -545,14 +545,11 @@ static int dataflash_write_user_otp(struct
>> >> mtd_info *mtd, struct dataflash	*priv = mtd->priv;
>> >> 	int			status;
>> >>
>> >
>> > I'm not sure I quite follow the logic for the following hunk. I think
>> > it deserves some more explanation, either in your commit or in a
>> > comment. As it stands, you're deleting a comment and potentially
>> > changing the return code behavior subtly.
>> >
>> >> -	if (len > 64)
>> >> -		return -EINVAL;
>> >> -
>> >> -	/* Strictly speaking, we *could* truncate the write ... but
>> >> -	 * let's not do that for the only write that's ever possible.
>> >> -	 */
>> >> -	if ((from + len) > 64)
>> >> -		return -EINVAL;
>> >> +	if ((from + len) > 64) {
>> >> +		len = 64 - from;
>> >
>> > Why are you reassigning len? Are you trying to undo the comment above,
>> > so that you *can* truncate the write? (It looks like there are other
>> > implmentations which will truncate the write and return -ENOSPC, FWIW.)
>>
>> Currently we have two kind of implementations: We have
>> implementations like this one which will refuse to write any data if
>> the write requests more data to be written than space is available.
>> And we have implementations like cfi_intelext_write_user_prot_reg
>> that will truncate the write and write as much data that is possible
>> (and return the number of bytes that actually have been written,
>> -ENOSPC shall only be returned if no data could be written at all).
>>
>> For a harmonization one of the implementations and their behavior
>> must be changed. I chose to change it to "write as much as
>> possible/truncate the write" since this is how a write should behave
>> (http://pubs.opengroup.org/onlinepubs/9699919799/functions/write.html).
>> And yes, this is why I try to undo the comment.
>
> OK, that makes sense. Then I think you should add a comment here in
> dataflash_write_user_otp() to say that you *are* truncating (or possibly
> rearrange the logic?), as it's not 100% clear what you're trying to do
> here. And add a small blurb to note this in the commit description. Some
> version of the above two paragraphs would make a nice
> addition/replacement to the patch description.

Ok, I will add a comment.

>> But if you are afraid that this will break things for current users
>> of the functions, I would keep the old behavior. What do you think?
>
> No, I believe your semantics make sense, and it's not a major breakage.
>

Ok.

>> >
>> >> +		if (len <= 0)
>> >> +			return -ENOSPC;
>> >> +	}
>> >>
>> >> 	/* OUT: OP_WRITE_SECURITY, 3 zeroes, 64 data-or-zero bytes
>> >> 	 * IN:  ignore all
>> >> diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
>> >> index 0edb0ca..db99031 100644
>> >> --- a/drivers/mtd/mtdchar.c
>> >> +++ b/drivers/mtd/mtdchar.c
>> >> @@ -323,6 +323,13 @@ static ssize_t mtdchar_write(struct file *file,
>> >> const char __user *buf, size_t c default:
>> >> 			ret = mtd_write(mtd, *ppos, len, &retlen, kbuf);
>> >> 		}
>> >> +		/* return -ENOSPC only if no data was written */
>> >> +		if ((ret == -ENOSPC) && (total_retlen)) {
>> >> +			ret = 0;
>> >> +			retlen = 0;
>> >> +			/* drop the remaining data */
>> >> +			count = 0;
>> >
>> > This block can just be a 'break' statement, no?
>>
>> No. mtdchar_write may split the write into several calls of
>> mtd_write, mtd_write_user_prot_reg... It will call mtd_write,
>> mtd_write_user_prot_reg as long as there is data to be written. If
>> the write hits the boundary of the memory, the last call of
>> mtd_write_user_prot_reg will return -ENOSPC. If this was the only
>> call of mtd_write_user_prot_reg (so no data could be written at
>> all), returning -ENOSPC to the user is fine. However, if data has
>> been written before, we must not return -ENOSPC, we must return the
>> number of bytes that have actually been written.
>>
>> So at least it must be
>>
>> if ((ret == -ENOSPC) && (total_retlen)) {
>> 	ret = 0;
>
> ^^^ this line will have no effect, since 'ret' is not used outside the
> while loop.
>

You are right, sorry for that.

>> 	break;
>> }
>>
>> Which one do you prefer?
>
> I prefer the following :) It's fewer lines and more straightforward, I
> think.
>
> if ((ret == -ENOSPC) && total_retlen)
> 	break;
>

Ok, I'll change that.
Christian

>> >
>> >> +		}
>> >
>> > I'm a bit wary of changing the behavior of non-OTP writes. At a
>> > minimum, the patch description needs to acknowledge that this affects
>> > more than just OTP writes. But after a cursory review of mtd->_write()
>> > implementations, it looks like there's no driver which could be
>> > returning -ENOSPC already, so this change is probably OK.
>>
>> The behavior of non-OTP writes is not changed at all. At the begin
>> of mtdchar_write, a check against mtd->size is done, and the write
>> is truncated. Therefore, non-OTP writes will never hit the end of
>> memory in the write function.
>
> Right, and actually mtd_write() never gives -ENOSPC; it returns -EINVAL.
> So this is fine.
>
> Brian
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

Patch

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 0a7d77e65335..ee6730748f7e 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -909,11 +909,16 @@  EXPORT_SYMBOL_GPL(mtd_read_fact_prot_reg);
 int mtd_get_user_prot_info(struct mtd_info *mtd, size_t len, size_t *retlen,
 			   struct otp_info *buf)
 {
+	int ret;
+
 	if (!mtd->_get_user_prot_info)
 		return -EOPNOTSUPP;
 	if (!len)
 		return 0;
-	return mtd->_get_user_prot_info(mtd, len, retlen, buf);
+	ret = mtd->_get_user_prot_info(mtd, len, retlen, buf);
+	if (ret)
+		return ret;
+	return !(*retlen) ? -ENOSPC: 0;
 }
 EXPORT_SYMBOL_GPL(mtd_get_user_prot_info);