Re: [PATCH] serial: sh-sci: Fix off-by-one error in FIFO threshold register setting

From: Geert Uytterhoeven <geert@linux-m68k.org>
To: Wolfram Sang <wsa+renesas@sang-engineering.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Ulrich Hecht <ulrich.hecht+renesas@gmail.com>,
	Linh Phung <linh.phung.jy@renesas.com>,
	"open list:SERIAL DRIVERS" <linux-serial@vger.kernel.org>,
	Linux-Renesas <linux-renesas-soc@vger.kernel.org>
Subject: Re: [PATCH] serial: sh-sci: Fix off-by-one error in FIFO threshold register setting
Date: Tue, 11 May 2021 11:03:14 +0200	[thread overview]
Message-ID: <CAMuHMdWsP=CV8CNvGcy-LRZ28x8pjLCTJnGiS937EcZpVOvKFQ@mail.gmail.com> (raw)
In-Reply-To: <20210511085538.GC1626@ninjato>

Hi Wolfram,

On Tue, May 11, 2021 at 10:55 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> On Mon, May 10, 2021 at 02:07:55PM +0200, Geert Uytterhoeven wrote:
> > The Receive FIFO Data Count Trigger field (RTRG[6:0]) in the Receive
> > FIFO Data Count Trigger Register (HSRTRGR) of HSCIF can only hold values
> > ranging from 0-127.  As the FIFO size is equal to 128 on HSCIF, the user
> > can write an out-of-range value, touching reserved bits.
> >
> > Fix this by limiting the trigger value to the FIFO size minus one.
> > Reverse the order of the checks, to avoid rx_trig becoming zero if the
> > FIFO size is one.
> >
> > Note that this change has no impact on other SCIF variants, as their
> > maximum supported trigger value is lower than the FIFO size anyway, and
> > the code below takes care of enforcing these limits.
> >
> > Reported-by: Linh Phung <linh.phung.jy@renesas.com>
> > Fixes: a380ed461f66d1b8 ("serial: sh-sci: implement FIFO threshold register setting")
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> > Compile-tested only.
> >
> > The BSP contains a different patch[1], which masks the value to write by
> > 0x7f.  This is IMHO incorrect, as it would set the trigger value to zero
> > when 128 is requested.
>
> Makes also sense to me to have the trigger at fifosize-1 to have one
> spare byte to handle latencies.

Exactly, that's why the maximum value supported by the HSCIF
hardware is 127, not 128.

> Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds