Re: [PATCH] MdeModulePkg SerialDxe: Process timeout consistently in SerialRead

["Ni, Ruiyu" <ruiyu.ni@intel.com>]

I thought unit of Stall is 100ns. Then no issues now.

Reviewed-by: Ruiyu Ni <Ruiyu.ni@intel.com>

Thanks/Ray

> -----Original Message-----
> From: Zeng, Star
> Sent: Friday, August 4, 2017 5:25 PM
> To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
> Cc: Heyi Guo <heyi.guo@linaro.org>; Laszlo Ersek <lersek@redhat.com>;
> Zeng, Star <star.zeng@intel.com>
> Subject: RE: [PATCH] MdeModulePkg SerialDxe: Process timeout consistently
> in SerialRead
> 
> Thanks for the comments.
> 
> EFI_TIMER_PERIOD_MICROSECONDS is used for timer event according to its
> definition, and its unit is 100ns.
> But the unit of mSerialIoMode.Timeout and gBS->Stall() is 1us.
> 
> +1 may cause more polling of SerialPortPoll(). How about keeping using
> ++10? :)
> 
> 
> Thanks,
> Star
> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Friday, August 4, 2017 5:13 PM
> To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
> Cc: Heyi Guo <heyi.guo@linaro.org>; Laszlo Ersek <lersek@redhat.com>
> Subject: RE: [PATCH] MdeModulePkg SerialDxe: Process timeout consistently
> in SerialRead
> 
> Star,
> 3 minor comments below.
> 
> Thanks/Ray
> 
> > -----Original Message-----
> > From: Zeng, Star
> > Sent: Friday, August 4, 2017 4:29 PM
> > To: edk2-devel@lists.01.org
> > Cc: Zeng, Star <star.zeng@intel.com>; Heyi Guo <heyi.guo@linaro.org>;
> > Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
> > Subject: [PATCH] MdeModulePkg SerialDxe: Process timeout consistently
> > in SerialRead
> >
> > https://lists.01.org/pipermail/edk2-devel/2017-July/012385.html
> > reported the timeout processing in SerialRead is not consistent.
> >
> > Since SerialPortPoll only checks the status of serial port and returns
> > immediately, and SerialPortRead does not really implement a time out
> > mechanism and will always wait for enough input, it will cause below results:
> > 1. If there is no serial input at all, this interface will return
> > timeout immediately without any waiting; 2. If there is A characters
> > in serial port FIFO, and caller requires
> > A+1 characters, it will wait until a new input is coming and timeout
> > will not really occur.
> >
> > This patch is to update SerialRead() to check SerialPortPoll() and
> > read data through SerialPortRead() one byte by one byte, and check
> > timeout against mSerialIoMode.Timeout if no input.
> >
> > Cc: Heyi Guo <heyi.guo@linaro.org>
> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Star Zeng <star.zeng@intel.com>
> > ---
> >  MdeModulePkg/Universal/SerialDxe/SerialIo.c | 18 ++++++++++++++++--
> >  1 file changed, 16 insertions(+), 2 deletions(-)
> >
> > diff --git a/MdeModulePkg/Universal/SerialDxe/SerialIo.c
> > b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
> > index d2383e56dd8f..43d33dba0c2a 100644
> > --- a/MdeModulePkg/Universal/SerialDxe/SerialIo.c
> > +++ b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
> > @@ -465,11 +465,25 @@ SerialRead (
> >    )
> >  {
> >    UINTN Count;
> > +  UINTN TimeOut;
> >
> >    Count = 0;
> >
> > -  if (SerialPortPoll ()) {
> > -    Count = SerialPortRead (Buffer, *BufferSize);
> > +  while (Count < *BufferSize) {
> > +    TimeOut = 0;
> > +    while (TimeOut < mSerialIoMode.Timeout) {
> > +      if (SerialPortPoll ()) {
> > +        break;
> > +      }
> > +      gBS->Stall (10);
> 1. can you use EFI_TIMER_PERIOD_MICROSECONDS(1)?
> 
> > +      TimeOut += 10;
> 2. TImeOut++?
> 
> > +    }
> > +    if (TimeOut >= mSerialIoMode.Timeout) {
> 3. if (TimeOut == ...) { ?
> 
> > +      break;
> > +    }
> > +    SerialPortRead (Buffer, 1);
> > +    Count++;
> > +    Buffer = (VOID *) ((UINT8 *) Buffer + 1);
> >    }
> >
> >    if (Count != *BufferSize) {
> > --
> > 2.7.0.windows.1