[PATCH v2 RESEND 2/2] mtd: Fix the behavior of otp write if there is not enough room for data
Christian Riesch
christian.riesch at omicron.at
Thu Mar 6 03:57:52 EST 2014
Hi Brian,
--On March 06, 2014 00:49 -0800 Brian Norris <computersforpeace at 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 at 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 at omicron.at>
>> >> Cc: Artem Bityutskiy <artem.bityutskiy at linux.intel.com>
>> >> Cc: Kyungmin Park <kyungmin.park at samsung.com>
>> >> Cc: Amul Kumar Saha <amul.saha at 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/
More information about the linux-mtd
mailing list