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

Thu Sep 6 08:09:20 UTC 2018

Hi Stefan,

> From: Stefan <sr at denx.de>
> Sent: mardi 4 septembre 2018 14:08
> 
> 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.

Ok thanks for the feedback, so I forget my "optimization" proposal.
But the fist patch to protect  the serial rx buffer access is needed 
(the initial patch reviewed by Simon).

> 
> Thanks,
> Stefan

Regards 
Patrick