[U-Boot] [PATCH 2/4] serial: protect access to serial rx buffer

[Patrick DELAUNAY]

Hi Simon and Stefan,

Sorry for my late answer (I just come back from my summer holiday)

> From: sjg at google.com <sjg at google.com> On Behalf Of Simon Glass
> Sent: mercredi 8 août 2018 11:56
> 
> On 3 August 2018 at 05:38, Patrick Delaunay <patrick.delaunay at st.com> wrote:
> > Add test to avoid access to rx buffer when this buffer is empty.
> > In this case directly call getc() function to avoid issue when tstc()
> > is not called.
> >
> > Signed-off-by: Patrick Delaunay <patrick.delaunay at st.com>
> > ---
> >
> >  drivers/serial/serial-uclass.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/serial/serial-uclass.c
> > b/drivers/serial/serial-uclass.c index 321d23e..4121a37 100644
> > --- a/drivers/serial/serial-uclass.c
> > +++ b/drivers/serial/serial-uclass.c
> > @@ -228,6 +228,9 @@ static int _serial_getc(struct udevice *dev)
> >         struct serial_dev_priv *upriv = dev_get_uclass_priv(dev);
> >         char val;
> >
> > +       if (upriv->rd_ptr == upriv->wr_ptr)
> > +               return __serial_getc(dev);
> > +
> >         val = upriv->buf[upriv->rd_ptr++];
> >         upriv->rd_ptr %= CONFIG_SERIAL_RX_BUFFER_SIZE;
> >
> > --
> > 2.7.4
> >
> 
> Reviewed-by: Simon Glass <sjg at chromium.org>
> 
> BTW I think the code in _serial_tstc() is wrong, or at least sub-optimal:
> 
> /* Read all available chars into the RX buffer */ while (__serial_tstc(dev)) {
>    upriv->buf[upriv->wr_ptr++] = __serial_getc(dev);
>    upriv->wr_ptr %= CONFIG_SERIAL_RX_BUFFER_SIZE; }
> 
> It should call serial_getc() until it returns -EAGAIN. There should be no need to
> call __serial_tstc() first,

This part had be added by Stefan Roese in SHA1 3ca7a06afb7cbc867b7861a8b9aada74e5bbb559

But I check again the code, and I think the code is correct.... but I agree it is not optimal.

we can directly ops->getc(dev) and no more __serial_getc() or __serial_tstc() :
the behavior don't change but the access to serial driver is reduced. 
When no char is available, ops->getc witll return -EAGAIN

static int _serial_tstc(struct udevice *dev)
{
	struct dm_serial_ops *ops = serial_get_ops(dev);
	struct serial_dev_priv *upriv = dev_get_uclass_priv(dev); 
	int err;

	/* Read all available chars into the RX buffer */
	while(1) {
		err = ops->getc(dev);
		if (err < 0)
			break;
		upriv->buf[upriv->wr_ptr++] = err;
		upriv->wr_ptr %= CONFIG_SERIAL_RX_BUFFER_SIZE;
	}

	return upriv->rd_ptr != upriv->wr_ptr ? 1 : 0;
}

If you are OK I will push a other patchset for these otpimisation.

> 
> Regards,
> Simon

Regards
Patrick