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

[Stefan]

Hi Patrick,

On 03.09.2018 15:35, Patrick DELAUNAY wrote:
> 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....

I really hope so. I did test this implementation quite heavily at
that time.

> 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.

I'm not 100% sure, if this new implementation is "better". Lets
compare the code:

Current version:
static int _serial_tstc(struct udevice *dev)
{
	struct serial_dev_priv *upriv = dev_get_uclass_priv(dev);

	/* 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;
	}

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

New version:
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;
}

The new version has more code and will most likely produce
a larger binary. You are removing the calls to the __foo()
functions - making the call chain a bit smaller. But this
will only affect the performance which is most likely negligible
in this case.

I any case, I don't object against any "optimizations" here.
But please make sure to test any changes very thorough - please
also on platforms with limited RX FIFO sizes.

Thanks,
Stefan