lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Sun, 16 Aug 2020 16:22:17 +0000
From:   Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@...renesas.com>
To:     Geert Uytterhoeven <geert@...ux-m68k.org>,
        "Lad, Prabhakar" <prabhakar.csengg@...il.com>
CC:     Kazuhiro Fujita <kazuhiro.fujita.jg@...esas.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jslaby@...e.com>,
        "open list:SERIAL DRIVERS" <linux-serial@...r.kernel.org>,
        Linux-Renesas <linux-renesas-soc@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Hao Bui <hao.bui.yg@...esas.com>,
        KAZUMI HARADA <kazumi.harada.rh@...esas.com>,
        Sasha Levin <sashal@...nel.org>,
        Chris Brandt <Chris.Brandt@...esas.com>,
        Magnus Damm <magnus.damm@...il.com>,
        Linux-sh list <linux-sh@...r.kernel.org>,
        Rob Landley <rob@...dley.net>,
        John Paul Adrian Glaubitz <glaubitz@...sik.fu-berlin.de>
Subject: RE: [PATCH] serial: sh-sci: Make sure status register SCxSR is read
 in correct sequence

> -----Original Message-----
> From: Geert Uytterhoeven <geert@...ux-m68k.org>
> Sent: 14 August 2020 16:25
> To: Lad, Prabhakar <prabhakar.csengg@...il.com>
> Cc: Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@...renesas.com>; Kazuhiro Fujita <kazuhiro.fujita.jg@...esas.com>; Greg Kroah-
> Hartman <gregkh@...uxfoundation.org>; Jiri Slaby <jslaby@...e.com>; open list:SERIAL DRIVERS <linux-serial@...r.kernel.org>; Linux-
> Renesas <linux-renesas-soc@...r.kernel.org>; Linux Kernel Mailing List <linux-kernel@...r.kernel.org>; Hao Bui
> <hao.bui.yg@...esas.com>; KAZUMI HARADA <kazumi.harada.rh@...esas.com>; Sasha Levin <sashal@...nel.org>; Chris Brandt
> <Chris.Brandt@...esas.com>; Magnus Damm <magnus.damm@...il.com>; Linux-sh list <linux-sh@...r.kernel.org>; Rob Landley
> <rob@...dley.net>; John Paul Adrian Glaubitz <glaubitz@...sik.fu-berlin.de>
> Subject: Re: [PATCH] serial: sh-sci: Make sure status register SCxSR is read in correct sequence
>
> On Wed, Apr 15, 2020 at 2:36 PM Geert Uytterhoeven <geert@...ux-m68k.org> wrote:
> > On Thu, Apr 2, 2020 at 5:24 PM Geert Uytterhoeven <geert@...ux-m68k.org> wrote:
> > > On Thu, Apr 2, 2020 at 1:28 PM Lad, Prabhakar
> > > <prabhakar.csengg@...il.com> wrote:
> > > > On Wed, Apr 1, 2020 at 1:43 PM Geert Uytterhoeven <geert@...ux-m68k.org> wrote:
> > > > > On Tue, Mar 31, 2020 at 5:58 PM Prabhakar Mahadev Lad
> > > > > <prabhakar.mahadev-lad.rj@...renesas.com> wrote:
> > > > > > > -----Original Message-----
> > > > > > > From: Geert Uytterhoeven <geert@...ux-m68k.org>
> > > > > > > On Fri, Mar 27, 2020 at 7:17 PM Kazuhiro Fujita
> > > > > > > <kazuhiro.fujita.jg@...esas.com> wrote:
> > > > > > > > For SCIF and HSCIF interfaces the SCxSR register holds the status of
> > > > > > > > data that is to be read next from SCxRDR register, But where as for
> > > > > > > > SCIFA and SCIFB interfaces SCxSR register holds status of data that is
> > > > > > > > previously read from SCxRDR register.
> > > > > > > >
> > > > > > > > This patch makes sure the status register is read depending on the port
> > > > > > > > types so that errors are caught accordingly.
> > > > > > > >
> > > > > > > > Cc: <stable@...r.kernel.org>
> > > > > > > > Signed-off-by: Kazuhiro Fujita <kazuhiro.fujita.jg@...esas.com>
> > > > > > > > Signed-off-by: Hao Bui <hao.bui.yg@...esas.com>
> > > > > > > > Signed-off-by: KAZUMI HARADA <kazumi.harada.rh@...esas.com>
> > > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
> > > > >
> > > > > > > Nevertheless, this patch will need some testing on various hardware.
> > > > > > > Do you have a test case to verify the broken/fixed behavior?
> > > > > > >
> > > > > > Agreed, its been tested on RZ/G2x & RZ/G1x  by doing a loopback test, configure one interface as CS8 mode(8-bits data, No parity)
> and other as CS7 mode (7-bits data, 1-bit Parity) and parity errors should be detected.
> > > > >
> > > > > This can easily be tested on the console.  Basic testing can even be
> > > > > done with an unmodified kernel, as there is already a "parity error"
> > > > > notice message in the driver.
> > > > >
> > > > > Enable even parity on the console:
> > > > >
> > > > > $ stty evenp
> > > > >
> > > > > (use "oddp" for odd parity, and invert all below)
> > > > >
> > > > > Typing e.g. a single "p" should trigger a parity error.
> > > > > Typing "o" shouldn't.
> > > > > Without this patch, no parity error is detected on SCIF.
> > > > >
> > > > > Likewise, pasting a sequence of "p" characters should trigger a lot of
> > > > > parity errors, "o" shouldn't.
> > > > > Without this patch, parity errors are detected on SCIF, except for the
> > > > > first character.
> > > > >
> > > > > For more advanced testing, make the following change to the driver:
> > > > >
> > > > > - dev_notice(port->dev, "parity error\n");
> > > > > + dev_notice(port->dev, "parity error for char 0x%02x hweight %u\n",
> > > > > c, hweight8(c));
> > > > >
> > > > > Pasting an alternating sequence of "p" and "o" characters should trigger
> > > > > parity errors for the "p" characters.
> > > > > Without this patch, they are triggered for the "o" characters instead.
> > > > >
> > > > Thank you that makes life easier.
> > > >
> > > > > With this patch, the issues above are fixed on SCIF.
> > > > > This has been verified on:
> > > > >   1. SCIF on R-Car Gen 2,
> > > > >   2. SCIF on R-Car Gen3
> > > > >   3. SCIF on RZ/A1H,
> > > > >   4. SCIF on RZ/A2M.
> >
> > > If I disable DMA for HSCIF1 in the .dtsi, parity errors are detected
> > > as expected.
> >
> > So HSCIF on R-Car Gen3 is affected, and fixed by this patch.
> >
> > > Hence the driver does not support parity checking when DMA is enabled.
> > > I also think it's not easy to add support for that, if possible at all.
> > >
> > > > > I haven't tested it yet on:
> > > > >   1. SCIFB on SH/R-Mobile (needs wiring up),
> >
> > SCIFB on R-Mobile A1 is not affected, and works before/after, as expected.
> >
> > > > >   2. SCIFA, SCIFB, and HSCIF on R-Car Gen2 (needs wiring up),
> >
> > Same for SCIFA, SCIFB on R-Car M2-W.
> >
> > HSCIF on R-Car M2-W is affected, and fixed by this patch.
> >
> > This means the modern platforms we care about are handled fine, so
> > Tested-by: Geert Uytterhoeven <geert+renesas@...der.be>
> >
> > That leaves us with testing on a few legacy platforms...
> >
> > > > >   3. (H)SCIF on R-Car Gen1 (remote boards unaccessible at the moment),
> > > > >   4. SuperH (only remote Migo-R available, but unaccessible).
> > > > >
> > > > > I can test 1 and 2 (and perhaps 3 and 4) later, if needed.
> > > > probably testing this on SuperH is gonna be a pain due to lack of
> > > > hardware availability,
> > > > (it needs to be tested on 19 platforms)
> > > > how about #ifdef CONFIG_ARCH_RENESAS || CONFIG_H8300 and the fix ?
> > >
> > > I had a look at a few SuperH docs w.r.t. framing/parity error behavior:
> > >   - SCIF(A) on SH7724: similar to R-Car Gen2,
> > >   - H(SCIF) on SH7734: same as on R-Car Gen2,
> > >   - SCIF on SH7751: conflict between status register ("to be read next")
> > >     and flowchart ("read from").
>
> FTR, I gave it a try on the SH7751R-based I-O DATA USL-5P aka Landisk:
> SCIF is affected, and fixed by commit 3dc4db3662366306 ("serial: sh-sci:
> Make sure status register SCxSR is read in correct sequence").
>
Thank you Geert.

Cheers,
Prabhakar


Renesas Electronics Europe GmbH, Geschaeftsfuehrer/President: Carsten Jauch, Sitz der Gesellschaft/Registered office: Duesseldorf, Arcadiastrasse 10, 40472 Duesseldorf, Germany, Handelsregister/Commercial Register: Duesseldorf, HRB 3708 USt-IDNr./Tax identification no.: DE 119353406 WEEE-Reg.-Nr./WEEE reg. no.: DE 14978647

Powered by blists - more mailing lists