MdeModulePkg/SerialDxe: Inconsistent timeout processing in SerialRead

MdeModulePkg/SerialDxe: Inconsistent timeout processing in SerialRead

[Heyi Guo]

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

Re: MdeModulePkg/SerialDxe: Inconsistent timeout processing in SerialRead

[Heyi Guo]

+cc maintainers :)


在 7/17/2017 12:23 PM, Heyi Guo 写道:
> 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;
> }
>

Re: MdeModulePkg/SerialDxe: Inconsistent timeout processing in SerialRead

[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

Re: MdeModulePkg/SerialDxe: Inconsistent timeout processing in SerialRead

[Ni, Ruiyu]

Star,
Your change looks good.

Thanks/Ray

> -----Original Message-----
> From: Zeng, Star
> Sent: Tuesday, July 18, 2017 4:39 PM
> To: Heyi Guo <heyi.guo@linaro.org>; edk2-devel@lists.01.org; Ni, Ruiyu
> <ruiyu.ni@intel.com>; Laszlo Ersek (lersek@redhat.com)
> <lersek@redhat.com>
> Cc: Zeng, Star <star.zeng@intel.com>
> Subject: RE: [edk2] MdeModulePkg/SerialDxe: Inconsistent timeout
> processing in SerialRead
> 
> 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

Re: MdeModulePkg/SerialDxe: Inconsistent timeout processing in SerialRead

[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

Re: MdeModulePkg/SerialDxe: Inconsistent timeout processing in SerialRead

[Heyi Guo]

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
>

Re: MdeModulePkg/SerialDxe: Inconsistent timeout processing in SerialRead

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

Re: MdeModulePkg/SerialDxe: Inconsistent timeout processing in SerialRead

[Heyi Guo]

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