From: liaoweixiong <liaoweixiong@allwinnertech.com>
To: Miquel Raynal <mraynal@kernel.org>
Cc: Kees Cook <keescook@chromium.org>,
Anton Vorontsov <anton@enomsg.org>,
Colin Cross <ccross@android.com>, Tony Luck <tony.luck@intel.com>,
Jonathan Corbet <corbet@lwn.net>,
Richard Weinberger <richard@nod.at>,
Vignesh Raghavendra <vigneshr@ti.com>,
Mauro Carvalho Chehab <mchehab+samsung@kernel.org>,
"David S. Miller" <davem@davemloft.net>,
Rob Herring <robh@kernel.org>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Jonathan Cameron <Jonathan.Cameron@huawei.com>,
linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-mtd@lists.infradead.org
Subject: Re: [PATCH v1 11/11] mtd: new support oops logger based on pstore/blk
Date: Fri, 7 Feb 2020 18:30:37 +0800 [thread overview]
Message-ID: <05c21cb6-988b-63a7-60ff-c4e975f25817@allwinnertech.com> (raw)
In-Reply-To: <20200207094121.0ad702d0@xps13>
hi Miquel Raynal,
On 2020/2/7 下午4:41, Miquel Raynal wrote:
> Hi Liao,
>
> liaoweixiong <liaoweixiong@allwinnertech.com> wrote on Fri, 7 Feb 2020
> 12:13:08 +0800:
>
>> hi Miquel Raynal,
>>
>> On 2020/2/6 PM 11:45, Miquel Raynal wrote:
>>> Hi liao,
>>>
>>> liaoweixiong <liaoweixiong@allwinnertech.com> wrote on Thu, 6 Feb 2020
>>> 21:10:47 +0800:
>>>
>>>> hi Miquel Raynal,
>>>>
>>>> On 2020/1/23 AM 1:41, Miquel Raynal wrote:
>>>>> Hello,
>>>>>
>>>>> >>>>>>>> +/*
>>>>>>>>>> + * All zones will be read as pstore/blk will read zone one by one when do
>>>>>>>>>> + * recover.
>>>>>>>>>> + */
>>>>>>>>>> +static ssize_t mtdpstore_read(char *buf, size_t size, loff_t off)
>>>>>>>>>> +{
>>>>>>>>>> + struct mtdpstore_context *cxt = &oops_cxt;
>>>>>>>>>> + size_t retlen;
>>>>>>>>>> + int ret;
>>>>>>>>>> +
>>>>>>>>>> + if (mtdpstore_block_isbad(cxt, off))
>>>>>>>>>> + return -ENEXT;
>>>>>>>>>> +
>>>>>>>>>> + pr_debug("try to read off 0x%llx size %zu\n", off, size);
>>>>>>>>>> + ret = mtd_read(cxt->mtd, off, size, &retlen, (u_char *)buf);
>>>>>>>>>> + if ((ret < 0 && !mtd_is_bitflip(ret)) || size != retlen) {
>>>>>>>>>
>>>>>>>>> IIRC size != retlen does not mean it failed, but that you should
>>>>>>>>> continue reading after retlen bytes, no?
>>>>>>>>> >>
>>>>>>>> Yes, you are right. I will fix it. Thanks.
>>>>>>>> >>>>> Also, mtd_is_bitflip() does not mean that you are reading a false
>>>>>>>>> buffer, but that the data has been corrected as it contained bitflips.
>>>>>>>>> mtd_is_eccerr() however, would be meaningful.
>>>>>>>>> >>
>>>>>>>> Sure I know mtd_is_bitflip() does not mean failure, but I do not think
>>>>>>>> mtd_is_eccerr() should be here since the codes are ret < 0 and NOT
>>>>>>>> mtd_is_bitflip().
>>>>>>>
>>>>>>> Yes, just drop this check, only keep ret < 0.
>>>>>>> >>
>>>>>> If I don't get it wrong, it should not be dropped here. Like your words,
>>>>>> "mtd_is_bitflip() does not mean that you are reading a false buffer,
>>>>>> but that the data has been corrected as it contained bitflips.", the
>>>>>> data I get are valid even if mtd_is_bitflip() return true. It's correct
>>>>>> data and it's no need to go to handle error. To me, the codes
>>>>>> should be:
>>>>>> if (ret < 0 && !mit_is_bitflip())
>>>>>> [error handling]
>>>>>
>>>>> Please check the implementation of mtd_is_bitflip(). You'll probably
>>>>> figure out what I am saying.
>>>>>
>>>>> https://elixir.bootlin.com/linux/latest/source/include/linux/mtd/mtd.h#L585
>>>>> >>
>>>> How about the codes as follows:
>>>>
>>>> for (done = 0, retlen = 0; done < size; done += retlen) {
>>>> ret = mtd_read(..., &retlen, ...);
>>>> if (!ret)
>>>> continue;
>>>> /*
>>>> * do nothing if bitflip and ecc error occurs because whether
>>>> * it's bitflip or ECC error, just a small number of bits flip
>>>> * and the impact on log data is so small. The mtdpstore just
>>>> * hands over what it gets and user can judge whether the data
>>>> * is valid or not.
>>>> */
>>>> if (mtd_is_bitflip(ret)) {
>>>> dev_warn("bitflip at....");
>>>> continue;
>>
>>> I don't understand why do you check for bitflips. Bitflips have been
>>> corrected at this stage, you just get the information that there
>>> has been bitflips, but the data integrity is fine.
>>>
>>
>> Both of bitflip and eccerror are not real wrong in this
>> case. So we must check them.
>>
>>> I am not against ignoring ECC errors in this case though. I would
>>> propose:
>>>
>>> for (...) {
>>> if (ret < 0) {
>>> complain;
>>> return;
>>> }
>>>
>>
>> -117 (-EUCLEAN) means bitflip but be corrected.
>> -74 (-EBADMSG) means ecc error that uncorrectable
>> All of them are negative number that smaller than 0. If it just keeps
>> "ret < 0", it can never make a difference between bitflip/eccerror
>> and others.
>
> I forgot about these, your're right, so what about:
>
> static bool mtdpstore_is_io_error(int ret)
> {
> return ret < 0 && !mtd_is_eccerr(ret)> && !mtd_is_bitflip(ret);
> }
>
> for (...) {
> if (mtdpstore_is_io_error(ret))
> return ret;
>
> /*
> * Comment explaining why we still return valid data
> * despite ECC errors.
> */
> if (mtd_is_eccerr(ret))
> just-complain();
> }
>
> This snippet makes a difference between any "controller/bus
> timeout/error : data could not be retrieved" and "ECC error, maybe we
> can still read it and try to understand (risky, must be warned)".
>
That seems good to me. I will fix it later.
Thanks for your review.
>>
>>> if (mtd_is_eccerr())
>>> complain;
>>> }
>>>
>>>> } else if (mtd_is_eccerr(ret)) {
>>>> dev_warn("eccerr at....");
>>>> retlen = retlen == 0 ? size : retlen;
>>>> continue;
>>>> } else {
>>>> dev_err("read failure at...");
>>>> /* this zone is broken, try next one */
>>>> return -ENEXT;
>>>> }
>>>> }
>>>>
>>>
>>>
>>> Thanks,
>>> Miquèl
>>>
>
> Thanks,
> Miquèl
>
next prev parent reply other threads:[~2020-02-07 10:30 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-01-20 1:03 [PATCH v1 00/11] pstore: support crash log to block and mtd device WeiXiong Liao
2020-01-20 1:03 ` [PATCH v1 01/11] pstore/blk: new support logger for block devices WeiXiong Liao
2020-01-20 1:03 ` [PATCH v1 02/11] blkoops: add blkoops, a warpper for pstore/blk WeiXiong Liao
2020-01-20 1:03 ` [PATCH v1 03/11] pstore/blk: support pmsg recorder WeiXiong Liao
2020-01-20 1:03 ` [PATCH v1 04/11] pstore/blk: blkoops: support console recorder WeiXiong Liao
2020-01-20 1:03 ` [PATCH v1 05/11] pstore/blk: blkoops: support ftrace recorder WeiXiong Liao
2020-01-20 1:03 ` [PATCH v1 06/11] Documentation: pstore/blk: blkoops: create document for pstore_blk WeiXiong Liao
2020-01-21 4:13 ` Randy Dunlap
2020-01-21 5:23 ` liaoweixiong
2020-01-21 6:36 ` Randy Dunlap
2020-01-21 8:19 ` liaoweixiong
2020-01-21 15:34 ` Randy Dunlap
2020-01-22 15:01 ` liaoweixiong
2020-01-22 16:08 ` Randy Dunlap
2020-01-20 1:03 ` [PATCH v1 07/11] pstore/blk: skip broken zone for mtd device WeiXiong Liao
2020-01-20 1:03 ` [PATCH v1 08/11] blkoops: respect for device to pick recorders WeiXiong Liao
2020-01-20 1:03 ` [PATCH v1 09/11] pstore/blk: blkoops: support special removing jobs for dmesg WeiXiong Liao
2020-01-20 1:03 ` [PATCH v1 10/11] blkoops: add interface for dirver to get information of blkoops WeiXiong Liao
2020-01-20 1:03 ` [PATCH v1 11/11] mtd: new support oops logger based on pstore/blk WeiXiong Liao
2020-01-20 10:03 ` Miquel Raynal
2020-01-21 3:36 ` liaoweixiong
2020-01-21 8:48 ` Miquel Raynal
2020-01-22 17:22 ` liaoweixiong
2020-01-22 17:41 ` Miquel Raynal
2020-02-06 13:10 ` liaoweixiong
2020-02-06 15:45 ` Miquel Raynal
2020-02-07 4:13 ` liaoweixiong
2020-02-07 8:41 ` Miquel Raynal
2020-02-07 10:30 ` liaoweixiong [this message]
2020-01-23 4:24 ` Vignesh Raghavendra
2020-01-23 7:03 ` liaoweixiong
2020-02-06 9:13 ` [PATCH v1 00/11] pstore: support crash log to block and mtd device Kees Cook
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=05c21cb6-988b-63a7-60ff-c4e975f25817@allwinnertech.com \
--to=liaoweixiong@allwinnertech.com \
--cc=Jonathan.Cameron@huawei.com \
--cc=anton@enomsg.org \
--cc=ccross@android.com \
--cc=corbet@lwn.net \
--cc=davem@davemloft.net \
--cc=gregkh@linuxfoundation.org \
--cc=keescook@chromium.org \
--cc=linux-doc@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mtd@lists.infradead.org \
--cc=mchehab+samsung@kernel.org \
--cc=mraynal@kernel.org \
--cc=richard@nod.at \
--cc=robh@kernel.org \
--cc=tony.luck@intel.com \
--cc=vigneshr@ti.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).