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

[Stefan]

Hi Patrick,

On 04.09.2018 09:56, Patrick DELAUNAY wrote:
> Hi Stefan
> 
>> From: Stefan <sr at denx.de>
>> Sent: lundi 3 septembre 2018 16:03
>>
>> 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.
> 
> Yes, perhaps a larger code but no more  "pending"  ops call of serial driver.
> I only  directly use getc ops => that avoid one access to HW register I think.
> 
> Than can improve the execution time, but I agree that seems marginal in most the 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.
> 
> Unfortunately I have no platform with limited FIFO size, So I don't known how test it deeply.
> 
> And the impact depends also how is implemented the serial driver (gets and pending ops can use several HW register access and is depending on the access time to the IP).
> 
> But if you and Simon think that "optimization" is not needed, you can forget this proposal because I think also the gain should be very low.
> This proposal is only a reaction on the Simon comment (at least sub-optimal)

I would prefer not to change this code without any real need
(bug fix etc). As the current code has undergone many tests
and has proven stable - at least for me in all my test cases.
And I can't test any changes very easily, as the platform setup
will take a while for me.

Thanks,
Stefan