Re: MdeModulePkg/SerialDxe: Inconsistent timeout processing in SerialRead

[Heyi Guo <heyi.guo@linaro.org>]

No problem at all and thanks a lot for fixing the issue :)

Gary (Heyi Guo)


在 8/4/2017 4:05 PM, Zeng, Star 写道:
> Oh, sorry, I forgot it because of busying on other things. I will post it soon.
> Thanks for your reminder.
>
> Star
> -----Original Message-----
> From: Heyi Guo [mailto:heyi.guo@linaro.org]
> Sent: Friday, August 4, 2017 4:03 PM
> To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org; Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek (lersek@redhat.com) <lersek@redhat.com>
> Subject: Re: [edk2] MdeModulePkg/SerialDxe: Inconsistent timeout processing in SerialRead
>
> Hi Star,
>
> The patch works well on our platform. Would you merge it to EDK2 main stream or do you like me to post it to the mailing-list?
>
> Thanks and regards,
>
> Gary (Heyi Guo)
>
>
> 在 7/18/2017 6:59 PM, Heyi Guo 写道:
>> Hi Star,
>>
>> I think your proposed patch is fine.
>>
>> Thanks.
>>
>> Gary (Heyi Guo)
>>
>>
>> 在 7/18/2017 4:39 PM, Zeng, Star 写道:
>>> Gary,
>>> SerialDxe is a wrapper of SerialPortLib to produce serial io protocol.
>>> If let SerialDxe to handle the TimeOut, seemingly it can only check
>>> SerialPortPoll() and read data by SerialPortRead() one byte by one
>>> byte, for example like below.
>>> Do you have any proposed patch?
>>>
>>> Ray and Laszlo,
>>> Do you have any comments?
>>>
>>>
>>> ====================
>>> debf0d1b85e7a30defd29838abb20a44dd9ec69b
>>>    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..b05603d7f3b5 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);
>>> +      TimeOut += 10;
>>> +    }
>>> +    if (TimeOut >= mSerialIoMode.TimeOut) {
>>> +      break;
>>> +    }
>>> +    SerialPortRead (Buffer, 1);
>>> +    Count++;
>>> +    Buffer = (VOID *) ((UINT8 *) Buffer + 1);
>>>      }
>>>        if (Count != *BufferSize) {
>>> ====================
>>>
>>>
>>> Thanks,
>>> Star
>>> -----Original Message-----
>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
>>> Of Heyi Guo
>>> Sent: Monday, July 17, 2017 12:23 PM
>>> To: edk2-devel@lists.01.org
>>> Subject: [edk2] MdeModulePkg/SerialDxe: Inconsistent timeout
>>> processing in SerialRead
>>>
>>> Hi Folks,
>>>
>>> In SerialRead function in
>>> MdeModulePkg/Universal/SerialDxe/SerialIo.c,
>>> it seems 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.
>>>
>>> As SerialPortLib is a simple library implementation, I think it is
>>> better to improve SerialIoDxe driver instead of SerialPortLib.
>>>
>>> Please let me know your comments about this.
>>>
>>> Thanks and regards,
>>>
>>> Gary (Heyi Guo)
>>>
>>> EFI_STATUS
>>> EFIAPI
>>> SerialRead (
>>>      IN EFI_SERIAL_IO_PROTOCOL *This,
>>>      IN OUT UINTN              *BufferSize,
>>>      OUT VOID                  *Buffer
>>>      )
>>> {
>>>      UINTN Count;
>>>
>>>      Count = 0;
>>>
>>>      if (SerialPortPoll ()) {
>>>        Count = SerialPortRead (Buffer, *BufferSize);
>>>      }
>>>
>>>      if (Count != *BufferSize) {
>>>        *BufferSize = Count;
>>>        return EFI_TIMEOUT;
>>>      }
>>>
>>>      return EFI_SUCCESS;
>>> }
>>>
>>> _______________________________________________
>>> edk2-devel mailing list
>>> edk2-devel@lists.01.org
>>> https://lists.01.org/mailman/listinfo/edk2-devel