[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20230925104938.3f7b4284@xps-13>
Date: Mon, 25 Sep 2023 10:49:38 +0200
From: Miquel Raynal <miquel.raynal@...tlin.com>
To: ZhaoLong Wang <wangzhaolong1@...wei.com>
Cc: <richard@....at>, <vigneshr@...com>,
<linux-mtd@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
<chengzhihao1@...wei.com>, <yi.zhang@...wei.com>,
<yangerkun@...wei.com>
Subject: Re: [RFC] mtd: Fix error code loss in mtdchar_read() function.
Hello,
Richard, your advice is welcome here.
wangzhaolong1@...wei.com wrote on Sat, 23 Sep 2023 08:58:56 +0800:
> In the first while loop, if the mtd_read() function returns -EBADMSG
s/the// s/function//
,
> and 'retlen' returns 0, the loop break and the function returns value
s/and// remains to 0. The loop breaks and the function
returns 'total_retlen' which is 0 instead of the error code.
> 'total_retlen' is 0, not the error code.
Actually after looking at the code, I have no strong opinion
regarding whether we should return 0 or an error code in this case.
There is this comment right above, and I'm not sure it is still up to
date because I believe many drivers just don't provide the data upon
ECC error:
/* Nand returns -EBADMSG on ECC errors, but it returns
* the data. For our userspace tools it is important
* to dump areas with ECC errors!
* For kernel internal usage it also might return -EUCLEAN
* to signal the caller that a bitflip has occurred and has
* been corrected by the ECC algorithm.
* Userspace software which accesses NAND this way
* must be aware of the fact that it deals with NAND
*/
> This problem causes the user-space program to encounter EOF when it has
> not finished reading the mtd partion, and this also violates the read
> system call standard in POSIX.
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217939
> Signed-off-by: ZhaoLong Wang <wangzhaolong1@...wei.com>
> ---
> drivers/mtd/mtdchar.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
> index 8dc4f5c493fc..ba60dc6bef98 100644
> --- a/drivers/mtd/mtdchar.c
> +++ b/drivers/mtd/mtdchar.c
> @@ -211,7 +211,7 @@ static ssize_t mtdchar_read(struct file *file, char __user *buf, size_t count,
> }
>
> kfree(kbuf);
> - return total_retlen;
> + return total_retlen ? total_retlen : ret;
This is kind of wrong, if ret is 0 then you return ret while you should
return total_retlen. In practice it does not really matter, the result
is the same, but it makes it harder to understand the code IMHO.
> } /* mtdchar_read */
>
> static ssize_t mtdchar_write(struct file *file, const char __user *buf, size_t count,
Thanks,
Miquèl
Powered by blists - more mailing lists