[PATCH v2 RESEND 1/2] mtd: Add a retlen parameter to _get_{fact, user}_prot_info

[PATCH v2 RESEND 1/2] mtd: Add a retlen parameter to _get_{fact, user}_prot_info

[Christian Riesch]

Signed-off-by: Christian Riesch <christian.riesch@omicron.at>
Cc: Artem Bityutskiy <Artem.Bityutskiy@linux.intel.com>
Cc: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/chips/cfi_cmdset_0001.c |   31 +++++++++++++------------------
 drivers/mtd/devices/mtd_dataflash.c |    7 ++++---
 drivers/mtd/mtdchar.c               |   11 ++++++-----
 drivers/mtd/mtdcore.c               |   12 ++++++------
 drivers/mtd/mtdpart.c               |   14 ++++++++------
 drivers/mtd/onenand/onenand_base.c  |   30 ++++++++++++------------------
 include/linux/mtd/mtd.h             |   16 ++++++++--------
 7 files changed, 57 insertions(+), 64 deletions(-)

diff --git a/drivers/mtd/chips/cfi_cmdset_0001.c b/drivers/mtd/chips/cfi_cmdset_0001.c
index 7751443..7aa581f 100644
--- a/drivers/mtd/chips/cfi_cmdset_0001.c
+++ b/drivers/mtd/chips/cfi_cmdset_0001.c
@@ -69,10 +69,10 @@ static int cfi_intelext_read_fact_prot_reg (struct mtd_info *, loff_t, size_t, s
 static int cfi_intelext_read_user_prot_reg (struct mtd_info *, loff_t, size_t, size_t *, u_char *);
 static int cfi_intelext_write_user_prot_reg (struct mtd_info *, loff_t, size_t, size_t *, u_char *);
 static int cfi_intelext_lock_user_prot_reg (struct mtd_info *, loff_t, size_t);
-static int cfi_intelext_get_fact_prot_info (struct mtd_info *,
-					    struct otp_info *, size_t);
-static int cfi_intelext_get_user_prot_info (struct mtd_info *,
-					    struct otp_info *, size_t);
+static int cfi_intelext_get_fact_prot_info(struct mtd_info *, size_t,
+					   size_t *, struct otp_info *);
+static int cfi_intelext_get_user_prot_info(struct mtd_info *, size_t,
+					   size_t *, struct otp_info *);
 #endif
 static int cfi_intelext_suspend (struct mtd_info *);
 static void cfi_intelext_resume (struct mtd_info *);
@@ -2399,24 +2399,19 @@ static int cfi_intelext_lock_user_prot_reg(struct mtd_info *mtd,
 				     NULL, do_otp_lock, 1);
 }
 
-static int cfi_intelext_get_fact_prot_info(struct mtd_info *mtd,
-					   struct otp_info *buf, size_t len)
-{
-	size_t retlen;
-	int ret;
+static int cfi_intelext_get_fact_prot_info(struct mtd_info *mtd, size_t len,
+					   size_t *retlen, struct otp_info *buf)
 
-	ret = cfi_intelext_otp_walk(mtd, 0, len, &retlen, (u_char *)buf, NULL, 0);
-	return ret ? : retlen;
+{
+	return cfi_intelext_otp_walk(mtd, 0, len, retlen, (u_char *)buf,
+				     NULL, 0);
 }
 
-static int cfi_intelext_get_user_prot_info(struct mtd_info *mtd,
-					   struct otp_info *buf, size_t len)
+static int cfi_intelext_get_user_prot_info(struct mtd_info *mtd, size_t len,
+					   size_t *retlen, struct otp_info *buf)
 {
-	size_t retlen;
-	int ret;
-
-	ret = cfi_intelext_otp_walk(mtd, 0, len, &retlen, (u_char *)buf, NULL, 1);
-	return ret ? : retlen;
+	return cfi_intelext_otp_walk(mtd, 0, len, retlen, (u_char *)buf,
+				     NULL, 1);
 }
 
 #endif
diff --git a/drivers/mtd/devices/mtd_dataflash.c b/drivers/mtd/devices/mtd_dataflash.c
index 28779b6..09c69ce 100644
--- a/drivers/mtd/devices/mtd_dataflash.c
+++ b/drivers/mtd/devices/mtd_dataflash.c
@@ -442,8 +442,8 @@ static int dataflash_write(struct mtd_info *mtd, loff_t to, size_t len,
 
 #ifdef CONFIG_MTD_DATAFLASH_OTP
 
-static int dataflash_get_otp_info(struct mtd_info *mtd,
-		struct otp_info *info, size_t len)
+static int dataflash_get_otp_info(struct mtd_info *mtd, size_t len,
+				  size_t *retlen, struct otp_info *info)
 {
 	/* Report both blocks as identical:  bytes 0..64, locked.
 	 * Unless the user block changed from all-ones, we can't
@@ -452,7 +452,8 @@ static int dataflash_get_otp_info(struct mtd_info *mtd,
 	info->start = 0;
 	info->length = 64;
 	info->locked = 1;
-	return sizeof(*info);
+	*retlen = sizeof(*info);
+	return 0;
 }
 
 static ssize_t otp_read(struct spi_device *spi, unsigned base,
diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
index 684bfa3..0edb0ca 100644
--- a/drivers/mtd/mtdchar.c
+++ b/drivers/mtd/mtdchar.c
@@ -888,25 +888,26 @@ static int mtdchar_ioctl(struct file *file, u_int cmd, u_long arg)
 	case OTPGETREGIONINFO:
 	{
 		struct otp_info *buf = kmalloc(4096, GFP_KERNEL);
+		size_t retlen;
 		if (!buf)
 			return -ENOMEM;
 		switch (mfi->mode) {
 		case MTD_FILE_MODE_OTP_FACTORY:
-			ret = mtd_get_fact_prot_info(mtd, buf, 4096);
+			ret = mtd_get_fact_prot_info(mtd, 4096, &retlen, buf);
 			break;
 		case MTD_FILE_MODE_OTP_USER:
-			ret = mtd_get_user_prot_info(mtd, buf, 4096);
+			ret = mtd_get_user_prot_info(mtd, 4096, &retlen, buf);
 			break;
 		default:
 			ret = -EINVAL;
 			break;
 		}
-		if (ret >= 0) {
+		if (!ret) {
 			if (cmd == OTPGETREGIONCOUNT) {
-				int nbr = ret / sizeof(struct otp_info);
+				int nbr = retlen / sizeof(struct otp_info);
 				ret = copy_to_user(argp, &nbr, sizeof(int));
 			} else
-				ret = copy_to_user(argp, buf, ret);
+				ret = copy_to_user(argp, buf, retlen);
 			if (ret)
 				ret = -EFAULT;
 		}
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 048c823..d0c3d41 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -877,14 +877,14 @@ EXPORT_SYMBOL_GPL(mtd_read_oob);
  * devices. The user data is one time programmable but the factory data is read
  * only.
  */
-int mtd_get_fact_prot_info(struct mtd_info *mtd, struct otp_info *buf,
-			   size_t len)
+int mtd_get_fact_prot_info(struct mtd_info *mtd, size_t len, size_t *retlen,
+			   struct otp_info *buf)
 {
 	if (!mtd->_get_fact_prot_info)
 		return -EOPNOTSUPP;
 	if (!len)
 		return 0;
-	return mtd->_get_fact_prot_info(mtd, buf, len);
+	return mtd->_get_fact_prot_info(mtd, len, retlen, buf);
 }
 EXPORT_SYMBOL_GPL(mtd_get_fact_prot_info);
 
@@ -900,14 +900,14 @@ int mtd_read_fact_prot_reg(struct mtd_info *mtd, loff_t from, size_t len,
 }
 EXPORT_SYMBOL_GPL(mtd_read_fact_prot_reg);
 
-int mtd_get_user_prot_info(struct mtd_info *mtd, struct otp_info *buf,
-			   size_t len)
+int mtd_get_user_prot_info(struct mtd_info *mtd, size_t len, size_t *retlen,
+			   struct otp_info *buf)
 {
 	if (!mtd->_get_user_prot_info)
 		return -EOPNOTSUPP;
 	if (!len)
 		return 0;
-	return mtd->_get_user_prot_info(mtd, buf, len);
+	return mtd->_get_user_prot_info(mtd, len, retlen, buf);
 }
 EXPORT_SYMBOL_GPL(mtd_get_user_prot_info);
 
diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index 3014933..57d8cf7 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -150,11 +150,12 @@ static int part_read_user_prot_reg(struct mtd_info *mtd, loff_t from,
 						 retlen, buf);
 }
 
-static int part_get_user_prot_info(struct mtd_info *mtd,
-		struct otp_info *buf, size_t len)
+static int part_get_user_prot_info(struct mtd_info *mtd, size_t len,
+				   size_t *retlen, struct otp_info *buf)
 {
 	struct mtd_part *part = PART(mtd);
-	return part->master->_get_user_prot_info(part->master, buf, len);
+	return part->master->_get_user_prot_info(part->master, len, retlen,
+						 buf);
 }
 
 static int part_read_fact_prot_reg(struct mtd_info *mtd, loff_t from,
@@ -165,11 +166,12 @@ static int part_read_fact_prot_reg(struct mtd_info *mtd, loff_t from,
 						 retlen, buf);
 }
 
-static int part_get_fact_prot_info(struct mtd_info *mtd, struct otp_info *buf,
-		size_t len)
+static int part_get_fact_prot_info(struct mtd_info *mtd, size_t len,
+				   size_t *retlen, struct otp_info *buf)
 {
 	struct mtd_part *part = PART(mtd);
-	return part->master->_get_fact_prot_info(part->master, buf, len);
+	return part->master->_get_fact_prot_info(part->master, len, retlen,
+						 buf);
 }
 
 static int part_write(struct mtd_info *mtd, loff_t to, size_t len,
diff --git a/drivers/mtd/onenand/onenand_base.c b/drivers/mtd/onenand/onenand_base.c
index b3f41f2..419c538 100644
--- a/drivers/mtd/onenand/onenand_base.c
+++ b/drivers/mtd/onenand/onenand_base.c
@@ -3242,20 +3242,17 @@ static int onenand_otp_walk(struct mtd_info *mtd, loff_t from, size_t len,
 /**
  * onenand_get_fact_prot_info - [MTD Interface] Read factory OTP info
  * @param mtd		MTD device structure
- * @param buf		the databuffer to put/get data
  * @param len		number of bytes to read
+ * @param retlen	pointer to variable to store the number of read bytes
+ * @param buf		the databuffer to put/get data
  *
  * Read factory OTP info.
  */
-static int onenand_get_fact_prot_info(struct mtd_info *mtd,
-			struct otp_info *buf, size_t len)
+static int onenand_get_fact_prot_info(struct mtd_info *mtd, size_t len,
+				      size_t *retlen, struct otp_info *buf)
 {
-	size_t retlen;
-	int ret;
-
-	ret = onenand_otp_walk(mtd, 0, len, &retlen, (u_char *) buf, NULL, MTD_OTP_FACTORY);
-
-	return ret ? : retlen;
+	return onenand_otp_walk(mtd, 0, len, retlen, (u_char *) buf, NULL,
+				MTD_OTP_FACTORY);
 }
 
 /**
@@ -3277,20 +3274,17 @@ static int onenand_read_fact_prot_reg(struct mtd_info *mtd, loff_t from,
 /**
  * onenand_get_user_prot_info - [MTD Interface] Read user OTP info
  * @param mtd		MTD device structure
- * @param buf		the databuffer to put/get data
+ * @param retlen	pointer to variable to store the number of read bytes
  * @param len		number of bytes to read
+ * @param buf		the databuffer to put/get data
  *
  * Read user OTP info.
  */
-static int onenand_get_user_prot_info(struct mtd_info *mtd,
-			struct otp_info *buf, size_t len)
+static int onenand_get_user_prot_info(struct mtd_info *mtd, size_t len,
+				      size_t *retlen, struct otp_info *buf)
 {
-	size_t retlen;
-	int ret;
-
-	ret = onenand_otp_walk(mtd, 0, len, &retlen, (u_char *) buf, NULL, MTD_OTP_USER);
-
-	return ret ? : retlen;
+	return onenand_otp_walk(mtd, 0, len, retlen, (u_char *) buf, NULL,
+				MTD_OTP_USER);
 }
 
 /**
diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index a5cf4e8..af35068 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -204,12 +204,12 @@ struct mtd_info {
 			  struct mtd_oob_ops *ops);
 	int (*_write_oob) (struct mtd_info *mtd, loff_t to,
 			   struct mtd_oob_ops *ops);
-	int (*_get_fact_prot_info) (struct mtd_info *mtd, struct otp_info *buf,
-				    size_t len);
+	int (*_get_fact_prot_info) (struct mtd_info *mtd, size_t len,
+				    size_t *retlen, struct otp_info *buf);
 	int (*_read_fact_prot_reg) (struct mtd_info *mtd, loff_t from,
 				    size_t len, size_t *retlen, u_char *buf);
-	int (*_get_user_prot_info) (struct mtd_info *mtd, struct otp_info *buf,
-				    size_t len);
+	int (*_get_user_prot_info) (struct mtd_info *mtd, size_t len,
+				    size_t *retlen, struct otp_info *buf);
 	int (*_read_user_prot_reg) (struct mtd_info *mtd, loff_t from,
 				    size_t len, size_t *retlen, u_char *buf);
 	int (*_write_user_prot_reg) (struct mtd_info *mtd, loff_t to,
@@ -278,12 +278,12 @@ static inline int mtd_write_oob(struct mtd_info *mtd, loff_t to,
 	return mtd->_write_oob(mtd, to, ops);
 }
 
-int mtd_get_fact_prot_info(struct mtd_info *mtd, struct otp_info *buf,
-			   size_t len);
+int mtd_get_fact_prot_info(struct mtd_info *mtd, size_t len, size_t *retlen,
+			   struct otp_info *buf);
 int mtd_read_fact_prot_reg(struct mtd_info *mtd, loff_t from, size_t len,
 			   size_t *retlen, u_char *buf);
-int mtd_get_user_prot_info(struct mtd_info *mtd, struct otp_info *buf,
-			   size_t len);
+int mtd_get_user_prot_info(struct mtd_info *mtd, size_t len, size_t *retlen,
+			   struct otp_info *buf);
 int mtd_read_user_prot_reg(struct mtd_info *mtd, loff_t from, size_t len,
 			   size_t *retlen, u_char *buf);
 int mtd_write_user_prot_reg(struct mtd_info *mtd, loff_t to, size_t len,
-- 
1.7.9.5

Re: [PATCH v2 RESEND 1/2] mtd: Add a retlen parameter to _get_{fact,user}_prot_info

[Brian Norris]

On Tue, Jan 28, 2014 at 09:29:44AM +0100, Christian Riesch wrote:
> Signed-off-by: Christian Riesch <christian.riesch@omicron.at>
> Cc: Artem Bityutskiy <Artem.Bityutskiy@linux.intel.com>
> Cc: Brian Norris <computersforpeace@gmail.com>
> ---
>  drivers/mtd/chips/cfi_cmdset_0001.c |   31 +++++++++++++------------------
>  drivers/mtd/devices/mtd_dataflash.c |    7 ++++---
>  drivers/mtd/mtdchar.c               |   11 ++++++-----
>  drivers/mtd/mtdcore.c               |   12 ++++++------
>  drivers/mtd/mtdpart.c               |   14 ++++++++------
>  drivers/mtd/onenand/onenand_base.c  |   30 ++++++++++++------------------
>  include/linux/mtd/mtd.h             |   16 ++++++++--------
>  7 files changed, 57 insertions(+), 64 deletions(-)

Pushed patch 1 to l2-mtd.git. I may not push patch 2 yet; let me know if
you see a problem with that.

Thanks,
Brian

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

[Christian Riesch]

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 */
+	if (!ret && len && !(*retlen))
+		return -ENOSPC;
+
+	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;
 
-	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;
+		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;
+		}
 		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;
+
+	return ret;
 }
 
 /**
-- 
1.7.9.5

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

[Brian Norris]

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):

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.)

> +		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

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

[Brian Norris]

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

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

[Christian Riesch]

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/

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

[Brian Norris]

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

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

[Christian Riesch]

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/