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@xxxxxxxxxx>
Cc: Artem Bityutskiy <artem.bityutskiy@xxxxxxxxxxxxxxx>
Cc: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
Cc: Amul Kumar Saha <amul.saha@xxxxxxxxxxx>
---
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
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/