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 <[email protected]> > Cc: Artem Bityutskiy <[email protected]> > Cc: Kyungmin Park <[email protected]> > Cc: Amul Kumar Saha <[email protected]> > --- > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
