Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 7 Feb 2020 09:41:21 +0100
From:   Miquel Raynal <mraynal@kernel.org>
To:     liaoweixiong <liaoweixiong@allwinnertech.com>
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
Message-ID: <20200207094121.0ad702d0@xps13>
In-Reply-To: <6a1b50f4-320f-43d1-50e3-b0a2c3c7fb96@allwinnertech.com>
References: <1579482233-2672-1-git-send-email-liaoweixiong@allwinnertech.com>
        <1579482233-2672-12-git-send-email-liaoweixiong@allwinnertech.com>
        <20200120110306.32e53fd8@xps13>
        <27226590-379c-8784-f461-f5d701015611@allwinnertech.com>
        <20200121094802.61f8cb4d@xps13>
        <2c6000b1-ae25-564b-911a-2879e9c244b2@allwinnertech.com>
        <20200122184114.125b42c8@xps13>
        <e135f947-226f-8dd0-b328-fb87c5064914@allwinnertech.com>
        <20200206164559.59c5eb6a@xps13>
        <6a1b50f4-320f-43d1-50e3-b0a2c3c7fb96@allwinnertech.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Liao,

liaoweixiong <liaoweixiong@allwinnertech.com> wrote on Fri, 7 Feb 2020
12:13:08 +0800:

> hi Miquel Raynal,
>=20
> On 2020/2/6 PM 11:45, Miquel Raynal wrote:
> > Hi liao,
> >=20
> > liaoweixiong <liaoweixiong@allwinnertech.com> wrote on Thu, 6 Feb 2020
> > 21:10:47 +0800:
> >  =20
> >> hi Miquel Raynal,
> >>
> >> On 2020/1/23 AM 1:41, Miquel Raynal wrote: =20
> >>> Hello,
> >>> =20
> >>>    >>>>>>>> +/* =20
> >>>>>>>> + * All zones will be read as pstore/blk will read zone one by o=
ne when do
> >>>>>>>> + * recover.
> >>>>>>>> + */
> >>>>>>>> +static ssize_t mtdpstore_read(char *buf, size_t size, loff_t of=
f)
> >>>>>>>> +{
> >>>>>>>> +	struct mtdpstore_context *cxt =3D &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 =3D mtd_read(cxt->mtd, off, size, &retlen, (u_char *)buf);
> >>>>>>>> +	if ((ret < 0 && !mtd_is_bitflip(ret)) || size !=3D retlen)  { =
=20
> >>>>>>>
> >>>>>>> IIRC size !=3D retlen does not mean it failed, but that you should
> >>>>>>> continue reading after retlen bytes, no? =20
> >>>>>>>      >> =20
> >>>>>> Yes, you are right. I will fix it. Thanks. =20
> >>>>>>    >>>>> Also, mtd_is_bitflip() does not mean that you are reading=
 a false =20
> >>>>>>> buffer, but that the data has been corrected as it contained bitf=
lips.
> >>>>>>> mtd_is_eccerr() however, would be meaningful. =20
> >>>>>>>      >> =20
> >>>>>> Sure I know mtd_is_bitflip() does not mean failure, but I do not t=
hink
> >>>>>> mtd_is_eccerr() should be here since the codes are ret < 0 and NOT
> >>>>>> mtd_is_bitflip(). =20
> >>>>>
> >>>>> Yes, just drop this check, only keep ret < 0. =20
> >>>>>     >> =20
> >>>> If I don't get it wrong, it should not	 be dropped here. Like your w=
ords,
> >>>> "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 corr=
ect
> >>>> 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] =20
> >>>
> >>> 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 =20
> >>>    >> =20
> >> How about the codes as follows:
> >>
> >> for (done =3D 0, retlen =3D 0; done < size; done +=3D retlen) {
> >> 	ret =3D 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; =20
>=20
> > 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.
> >  =20
>=20
> Both of bitflip and eccerror are not real wrong in this
> case. So we must check them.
>=20
> > I am not against ignoring ECC errors in this case though. I would
> > propose:
> >=20
> > 	for (...) {
> > 		if (ret < 0) {
> > 			complain;
> > 			return;
> > 		}
> >  =20
>=20
> -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)".

>=20
> > 		if (mtd_is_eccerr())
> > 			complain;
> > 	}
> > 		 =20
> >> 	} else if (mtd_is_eccerr(ret)) {
> >> 		dev_warn("eccerr at....");
> >> 		retlen =3D retlen =3D=3D 0 ? size : retlen;
> >> 		continue;
> >> 	} else {
> >> 		dev_err("read failure at...");
> >> 		/* this zone is broken, try next one */
> >> 		return -ENEXT;
> >> 	}
> >> }
> >> =20
> >=20
> >=20
> > Thanks,
> > Miqu=C3=A8l
> >  =20

Thanks,
Miqu=C3=A8l