[PATCH] Fix not able to change serial attributes

[PATCH] Fix not able to change serial attributes

[Pankaj Bansal]

Issue : When try to change serial attributes using sermode
command, the default values are set
Cause : The SerialReset command resets the attributes' values
to default
Fix : Serial Reset command should set the attributes which have
been changed by user after calling SerialSetAttributes.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
---
 MdeModulePkg/Universal/SerialDxe/SerialIo.c | 66 ++++++++---------------------
 1 file changed, 18 insertions(+), 48 deletions(-)

diff --git a/MdeModulePkg/Universal/SerialDxe/SerialIo.c b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
index 43d33db..dc7e13a 100644
--- a/MdeModulePkg/Universal/SerialDxe/SerialIo.c
+++ b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
@@ -220,7 +220,6 @@ SerialReset (
   )
 {
   EFI_STATUS    Status;
-  EFI_TPL       Tpl;
 
   Status = SerialPortInitialize ();
   if (EFI_ERROR (Status)) {
@@ -228,49 +227,17 @@ SerialReset (
   }
 
   //
-  // Set the Serial I/O mode and update the device path
-  //
-
-  Tpl = gBS->RaiseTPL (TPL_NOTIFY);
-
-  //
-  // Set the Serial I/O mode
-  //
-  This->Mode->ReceiveFifoDepth  = PcdGet16 (PcdUartDefaultReceiveFifoDepth);
-  This->Mode->Timeout           = 1000 * 1000;
-  This->Mode->BaudRate          = PcdGet64 (PcdUartDefaultBaudRate);
-  This->Mode->DataBits          = (UINT32) PcdGet8 (PcdUartDefaultDataBits);
-  This->Mode->Parity            = (UINT32) PcdGet8 (PcdUartDefaultParity);
-  This->Mode->StopBits          = (UINT32) PcdGet8 (PcdUartDefaultStopBits);
-
-  //
-  // Check if the device path has actually changed
-  //
-  if (mSerialDevicePath.Uart.BaudRate == This->Mode->BaudRate &&
-      mSerialDevicePath.Uart.DataBits == (UINT8) This->Mode->DataBits &&
-      mSerialDevicePath.Uart.Parity   == (UINT8) This->Mode->Parity &&
-      mSerialDevicePath.Uart.StopBits == (UINT8) This->Mode->StopBits
-     ) {
-    gBS->RestoreTPL (Tpl);
-    return EFI_SUCCESS;
-  }
-
-  //
-  // Update the device path
+  // Go set the current attributes
   //
-  mSerialDevicePath.Uart.BaudRate = This->Mode->BaudRate;
-  mSerialDevicePath.Uart.DataBits = (UINT8) This->Mode->DataBits;
-  mSerialDevicePath.Uart.Parity   = (UINT8) This->Mode->Parity;
-  mSerialDevicePath.Uart.StopBits = (UINT8) This->Mode->StopBits;
-
-  Status = gBS->ReinstallProtocolInterface (
-                  mSerialHandle,
-                  &gEfiDevicePathProtocolGuid,
-                  &mSerialDevicePath,
-                  &mSerialDevicePath
-                  );
-
-  gBS->RestoreTPL (Tpl);
+  Status = This->SetAttributes (
+                   This,
+                   This->Mode->BaudRate,
+                   This->Mode->ReceiveFifoDepth,
+                   This->Mode->Timeout,
+                   (EFI_PARITY_TYPE) This->Mode->Parity,
+                   (UINT8) This->Mode->DataBits,
+                   (EFI_STOP_BITS_TYPE) This->Mode->StopBits
+                   );
 
   return Status;
 }
@@ -513,11 +480,6 @@ SerialDxeInitialize (
 {
   EFI_STATUS            Status;
 
-  Status = SerialPortInitialize ();
-  if (EFI_ERROR (Status)) {
-    return Status;
-  }
-
   mSerialIoMode.BaudRate = PcdGet64 (PcdUartDefaultBaudRate);
   mSerialIoMode.DataBits = (UINT32) PcdGet8 (PcdUartDefaultDataBits);
   mSerialIoMode.Parity   = (UINT32) PcdGet8 (PcdUartDefaultParity);
@@ -529,6 +491,14 @@ SerialDxeInitialize (
   mSerialDevicePath.Uart.StopBits = PcdGet8 (PcdUartDefaultStopBits);
 
   //
+  // Issue a reset to initialize the COM port
+  //
+  Status = mSerialIoTemplate.Reset (&mSerialIoTemplate);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  //
   // Make a new handle with Serial IO protocol and its device path on it.
   //
   Status = gBS->InstallMultipleProtocolInterfaces (
-- 
2.7.4

Re: [PATCH] Fix not able to change serial attributes

[Zeng, Star]

Pankaj,

Thanks for the contribution.
Could you help update the title to be like MdeModulePkg SerialDxe: XXXXXXXXXX?

In fact, I agree the fix as it matches the code at MdeModulePkg/Bus/Pci/PciSioSerialDxe/SerialIo.c and IntelFrameworkModulePkg/Bus/Isa/IsaSerialDxe/Serial.c.
But I am curious about how you reproduce the issue by sermode command as I see sermode command only calls SerialIo->SetAttributes() but not SerialIo->Reset().


Ray, Laszlo,
Do you have any comment?



Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Pankaj Bansal
Sent: Friday, September 15, 2017 9:14 PM
To: edk2-devel@lists.01.org
Cc: Pankaj Bansal <pankaj.bansal@nxp.com>
Subject: [edk2] [PATCH] Fix not able to change serial attributes

Issue : When try to change serial attributes using sermode command, the default values are set Cause : The SerialReset command resets the attributes' values to default Fix : Serial Reset command should set the attributes which have been changed by user after calling SerialSetAttributes.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
---
 MdeModulePkg/Universal/SerialDxe/SerialIo.c | 66 ++++++++---------------------
 1 file changed, 18 insertions(+), 48 deletions(-)

diff --git a/MdeModulePkg/Universal/SerialDxe/SerialIo.c b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
index 43d33db..dc7e13a 100644
--- a/MdeModulePkg/Universal/SerialDxe/SerialIo.c
+++ b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
@@ -220,7 +220,6 @@ SerialReset (
   )
 {
   EFI_STATUS    Status;
-  EFI_TPL       Tpl;
 
   Status = SerialPortInitialize ();
   if (EFI_ERROR (Status)) {
@@ -228,49 +227,17 @@ SerialReset (
   }
 
   //
-  // Set the Serial I/O mode and update the device path
-  //
-
-  Tpl = gBS->RaiseTPL (TPL_NOTIFY);
-
-  //
-  // Set the Serial I/O mode
-  //
-  This->Mode->ReceiveFifoDepth  = PcdGet16 (PcdUartDefaultReceiveFifoDepth);
-  This->Mode->Timeout           = 1000 * 1000;
-  This->Mode->BaudRate          = PcdGet64 (PcdUartDefaultBaudRate);
-  This->Mode->DataBits          = (UINT32) PcdGet8 (PcdUartDefaultDataBits);
-  This->Mode->Parity            = (UINT32) PcdGet8 (PcdUartDefaultParity);
-  This->Mode->StopBits          = (UINT32) PcdGet8 (PcdUartDefaultStopBits);
-
-  //
-  // Check if the device path has actually changed
-  //
-  if (mSerialDevicePath.Uart.BaudRate == This->Mode->BaudRate &&
-      mSerialDevicePath.Uart.DataBits == (UINT8) This->Mode->DataBits &&
-      mSerialDevicePath.Uart.Parity   == (UINT8) This->Mode->Parity &&
-      mSerialDevicePath.Uart.StopBits == (UINT8) This->Mode->StopBits
-     ) {
-    gBS->RestoreTPL (Tpl);
-    return EFI_SUCCESS;
-  }
-
-  //
-  // Update the device path
+  // Go set the current attributes
   //
-  mSerialDevicePath.Uart.BaudRate = This->Mode->BaudRate;
-  mSerialDevicePath.Uart.DataBits = (UINT8) This->Mode->DataBits;
-  mSerialDevicePath.Uart.Parity   = (UINT8) This->Mode->Parity;
-  mSerialDevicePath.Uart.StopBits = (UINT8) This->Mode->StopBits;
-
-  Status = gBS->ReinstallProtocolInterface (
-                  mSerialHandle,
-                  &gEfiDevicePathProtocolGuid,
-                  &mSerialDevicePath,
-                  &mSerialDevicePath
-                  );
-
-  gBS->RestoreTPL (Tpl);
+  Status = This->SetAttributes (
+                   This,
+                   This->Mode->BaudRate,
+                   This->Mode->ReceiveFifoDepth,
+                   This->Mode->Timeout,
+                   (EFI_PARITY_TYPE) This->Mode->Parity,
+                   (UINT8) This->Mode->DataBits,
+                   (EFI_STOP_BITS_TYPE) This->Mode->StopBits
+                   );
 
   return Status;
 }
@@ -513,11 +480,6 @@ SerialDxeInitialize (  {
   EFI_STATUS            Status;
 
-  Status = SerialPortInitialize ();
-  if (EFI_ERROR (Status)) {
-    return Status;
-  }
-
   mSerialIoMode.BaudRate = PcdGet64 (PcdUartDefaultBaudRate);
   mSerialIoMode.DataBits = (UINT32) PcdGet8 (PcdUartDefaultDataBits);
   mSerialIoMode.Parity   = (UINT32) PcdGet8 (PcdUartDefaultParity);
@@ -529,6 +491,14 @@ SerialDxeInitialize (
   mSerialDevicePath.Uart.StopBits = PcdGet8 (PcdUartDefaultStopBits);
 
   //
+  // Issue a reset to initialize the COM port  //  Status = 
+ mSerialIoTemplate.Reset (&mSerialIoTemplate);  if (EFI_ERROR (Status)) 
+ {
+    return Status;
+  }
+
+  //
   // Make a new handle with Serial IO protocol and its device path on it.
   //
   Status = gBS->InstallMultipleProtocolInterfaces (
--
2.7.4

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH] Fix not able to change serial attributes

[Laszlo Ersek]

On 09/16/17 03:21, Zeng, Star wrote:
> Pankaj,
> 
> Thanks for the contribution.
> Could you help update the title to be like MdeModulePkg SerialDxe: XXXXXXXXXX?
> 
> In fact, I agree the fix as it matches the code at MdeModulePkg/Bus/Pci/PciSioSerialDxe/SerialIo.c and IntelFrameworkModulePkg/Bus/Isa/IsaSerialDxe/Serial.c.
> But I am curious about how you reproduce the issue by sermode command as I see sermode command only calls SerialIo->SetAttributes() but not SerialIo->Reset().
> 
> 
> Ray, Laszlo,
> Do you have any comment?

Thanks for the CC.

The UEFI spec is very superficial on EFI_SERIAL_IO_PROTOCOL.Reset(). It
only says, "The Reset() function resets the hardware of a serial
device." It doesn't define what state the device should return to after
resetting the hardware.

Under the generic description of EFI_SERIAL_IO_PROTOCOL, we have

"The default attributes for all UART-style serial device interfaces are:
115,200 baud, a 1 byte receive FIFO, a 1,000,000 microsecond timeout per
character, no parity, 8 data bits, and 1 stop bit."

Now, the PCDs that are currently used in the code may differ from the
above standard-mandated values, but that's the platform's
responsibility. The point is that the UEFI spec seems to require that
the device be returned to a predefined state, and the PCDs make that
possible. I don't think that the argument in the commit message, "Serial
Reset command should set the attributes which have been changed by user
after calling SerialSetAttributes.", is consistent with the UEFI spec's
intent.

However, I could easily be wrong about this, given especially that the
other two SerialIo implementations already follow the suggested practice.

I guess I'll have to stay neutral on this patch. Hopefully it won't
regress anything.

Thanks,
Laszlo


> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Pankaj Bansal
> Sent: Friday, September 15, 2017 9:14 PM
> To: edk2-devel@lists.01.org
> Cc: Pankaj Bansal <pankaj.bansal@nxp.com>
> Subject: [edk2] [PATCH] Fix not able to change serial attributes
> 
> Issue : When try to change serial attributes using sermode command, the default values are set Cause : The SerialReset command resets the attributes' values to default Fix : Serial Reset command should set the attributes which have been changed by user after calling SerialSetAttributes.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
> ---
>  MdeModulePkg/Universal/SerialDxe/SerialIo.c | 66 ++++++++---------------------
>  1 file changed, 18 insertions(+), 48 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/SerialDxe/SerialIo.c b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
> index 43d33db..dc7e13a 100644
> --- a/MdeModulePkg/Universal/SerialDxe/SerialIo.c
> +++ b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
> @@ -220,7 +220,6 @@ SerialReset (
>    )
>  {
>    EFI_STATUS    Status;
> -  EFI_TPL       Tpl;
>  
>    Status = SerialPortInitialize ();
>    if (EFI_ERROR (Status)) {
> @@ -228,49 +227,17 @@ SerialReset (
>    }
>  
>    //
> -  // Set the Serial I/O mode and update the device path
> -  //
> -
> -  Tpl = gBS->RaiseTPL (TPL_NOTIFY);
> -
> -  //
> -  // Set the Serial I/O mode
> -  //
> -  This->Mode->ReceiveFifoDepth  = PcdGet16 (PcdUartDefaultReceiveFifoDepth);
> -  This->Mode->Timeout           = 1000 * 1000;
> -  This->Mode->BaudRate          = PcdGet64 (PcdUartDefaultBaudRate);
> -  This->Mode->DataBits          = (UINT32) PcdGet8 (PcdUartDefaultDataBits);
> -  This->Mode->Parity            = (UINT32) PcdGet8 (PcdUartDefaultParity);
> -  This->Mode->StopBits          = (UINT32) PcdGet8 (PcdUartDefaultStopBits);
> -
> -  //
> -  // Check if the device path has actually changed
> -  //
> -  if (mSerialDevicePath.Uart.BaudRate == This->Mode->BaudRate &&
> -      mSerialDevicePath.Uart.DataBits == (UINT8) This->Mode->DataBits &&
> -      mSerialDevicePath.Uart.Parity   == (UINT8) This->Mode->Parity &&
> -      mSerialDevicePath.Uart.StopBits == (UINT8) This->Mode->StopBits
> -     ) {
> -    gBS->RestoreTPL (Tpl);
> -    return EFI_SUCCESS;
> -  }
> -
> -  //
> -  // Update the device path
> +  // Go set the current attributes
>    //
> -  mSerialDevicePath.Uart.BaudRate = This->Mode->BaudRate;
> -  mSerialDevicePath.Uart.DataBits = (UINT8) This->Mode->DataBits;
> -  mSerialDevicePath.Uart.Parity   = (UINT8) This->Mode->Parity;
> -  mSerialDevicePath.Uart.StopBits = (UINT8) This->Mode->StopBits;
> -
> -  Status = gBS->ReinstallProtocolInterface (
> -                  mSerialHandle,
> -                  &gEfiDevicePathProtocolGuid,
> -                  &mSerialDevicePath,
> -                  &mSerialDevicePath
> -                  );
> -
> -  gBS->RestoreTPL (Tpl);
> +  Status = This->SetAttributes (
> +                   This,
> +                   This->Mode->BaudRate,
> +                   This->Mode->ReceiveFifoDepth,
> +                   This->Mode->Timeout,
> +                   (EFI_PARITY_TYPE) This->Mode->Parity,
> +                   (UINT8) This->Mode->DataBits,
> +                   (EFI_STOP_BITS_TYPE) This->Mode->StopBits
> +                   );
>  
>    return Status;
>  }
> @@ -513,11 +480,6 @@ SerialDxeInitialize (  {
>    EFI_STATUS            Status;
>  
> -  Status = SerialPortInitialize ();
> -  if (EFI_ERROR (Status)) {
> -    return Status;
> -  }
> -
>    mSerialIoMode.BaudRate = PcdGet64 (PcdUartDefaultBaudRate);
>    mSerialIoMode.DataBits = (UINT32) PcdGet8 (PcdUartDefaultDataBits);
>    mSerialIoMode.Parity   = (UINT32) PcdGet8 (PcdUartDefaultParity);
> @@ -529,6 +491,14 @@ SerialDxeInitialize (
>    mSerialDevicePath.Uart.StopBits = PcdGet8 (PcdUartDefaultStopBits);
>  
>    //
> +  // Issue a reset to initialize the COM port  //  Status = 
> + mSerialIoTemplate.Reset (&mSerialIoTemplate);  if (EFI_ERROR (Status)) 
> + {
> +    return Status;
> +  }
> +
> +  //
>    // Make a new handle with Serial IO protocol and its device path on it.
>    //
>    Status = gBS->InstallMultipleProtocolInterfaces (
> --
> 2.7.4
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 

Re: [PATCH] Fix not able to change serial attributes

[Zeng, Star]

Thanks for your good comments. :)
Since there is no clear description for the behavior of Reset() :(, I prone to align the behavior with MdeModulePkg/Bus/Pci/PciSioSerialDxe/SerialIo.c and IntelFrameworkModulePkg/Bus/Isa/IsaSerialDxe/Serial.c, that means I agree the fix.


Laszlo and Gary, if you can help do some simple regression test with the patch, that will be better.


Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
Sent: Sunday, September 17, 2017 4:18 AM
To: Zeng, Star <star.zeng@intel.com>; Pankaj Bansal <pankaj.bansal@nxp.com>; edk2-devel@lists.01.org
Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
Subject: Re: [edk2] [PATCH] Fix not able to change serial attributes

On 09/16/17 03:21, Zeng, Star wrote:
> Pankaj,
> 
> Thanks for the contribution.
> Could you help update the title to be like MdeModulePkg SerialDxe: XXXXXXXXXX?
> 
> In fact, I agree the fix as it matches the code at MdeModulePkg/Bus/Pci/PciSioSerialDxe/SerialIo.c and IntelFrameworkModulePkg/Bus/Isa/IsaSerialDxe/Serial.c.
> But I am curious about how you reproduce the issue by sermode command as I see sermode command only calls SerialIo->SetAttributes() but not SerialIo->Reset().
> 
> 
> Ray, Laszlo,
> Do you have any comment?

Thanks for the CC.

The UEFI spec is very superficial on EFI_SERIAL_IO_PROTOCOL.Reset(). It only says, "The Reset() function resets the hardware of a serial device." It doesn't define what state the device should return to after resetting the hardware.

Under the generic description of EFI_SERIAL_IO_PROTOCOL, we have

"The default attributes for all UART-style serial device interfaces are:
115,200 baud, a 1 byte receive FIFO, a 1,000,000 microsecond timeout per character, no parity, 8 data bits, and 1 stop bit."

Now, the PCDs that are currently used in the code may differ from the above standard-mandated values, but that's the platform's responsibility. The point is that the UEFI spec seems to require that the device be returned to a predefined state, and the PCDs make that possible. I don't think that the argument in the commit message, "Serial Reset command should set the attributes which have been changed by user after calling SerialSetAttributes.", is consistent with the UEFI spec's intent.

However, I could easily be wrong about this, given especially that the other two SerialIo implementations already follow the suggested practice.

I guess I'll have to stay neutral on this patch. Hopefully it won't regress anything.

Thanks,
Laszlo


> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of 
> Pankaj Bansal
> Sent: Friday, September 15, 2017 9:14 PM
> To: edk2-devel@lists.01.org
> Cc: Pankaj Bansal <pankaj.bansal@nxp.com>
> Subject: [edk2] [PATCH] Fix not able to change serial attributes
> 
> Issue : When try to change serial attributes using sermode command, the default values are set Cause : The SerialReset command resets the attributes' values to default Fix : Serial Reset command should set the attributes which have been changed by user after calling SerialSetAttributes.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
> ---
>  MdeModulePkg/Universal/SerialDxe/SerialIo.c | 66 
> ++++++++---------------------
>  1 file changed, 18 insertions(+), 48 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/SerialDxe/SerialIo.c 
> b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
> index 43d33db..dc7e13a 100644
> --- a/MdeModulePkg/Universal/SerialDxe/SerialIo.c
> +++ b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
> @@ -220,7 +220,6 @@ SerialReset (
>    )
>  {
>    EFI_STATUS    Status;
> -  EFI_TPL       Tpl;
>  
>    Status = SerialPortInitialize ();
>    if (EFI_ERROR (Status)) {
> @@ -228,49 +227,17 @@ SerialReset (
>    }
>  
>    //
> -  // Set the Serial I/O mode and update the device path
> -  //
> -
> -  Tpl = gBS->RaiseTPL (TPL_NOTIFY);
> -
> -  //
> -  // Set the Serial I/O mode
> -  //
> -  This->Mode->ReceiveFifoDepth  = PcdGet16 (PcdUartDefaultReceiveFifoDepth);
> -  This->Mode->Timeout           = 1000 * 1000;
> -  This->Mode->BaudRate          = PcdGet64 (PcdUartDefaultBaudRate);
> -  This->Mode->DataBits          = (UINT32) PcdGet8 (PcdUartDefaultDataBits);
> -  This->Mode->Parity            = (UINT32) PcdGet8 (PcdUartDefaultParity);
> -  This->Mode->StopBits          = (UINT32) PcdGet8 (PcdUartDefaultStopBits);
> -
> -  //
> -  // Check if the device path has actually changed
> -  //
> -  if (mSerialDevicePath.Uart.BaudRate == This->Mode->BaudRate &&
> -      mSerialDevicePath.Uart.DataBits == (UINT8) This->Mode->DataBits &&
> -      mSerialDevicePath.Uart.Parity   == (UINT8) This->Mode->Parity &&
> -      mSerialDevicePath.Uart.StopBits == (UINT8) This->Mode->StopBits
> -     ) {
> -    gBS->RestoreTPL (Tpl);
> -    return EFI_SUCCESS;
> -  }
> -
> -  //
> -  // Update the device path
> +  // Go set the current attributes
>    //
> -  mSerialDevicePath.Uart.BaudRate = This->Mode->BaudRate;
> -  mSerialDevicePath.Uart.DataBits = (UINT8) This->Mode->DataBits;
> -  mSerialDevicePath.Uart.Parity   = (UINT8) This->Mode->Parity;
> -  mSerialDevicePath.Uart.StopBits = (UINT8) This->Mode->StopBits;
> -
> -  Status = gBS->ReinstallProtocolInterface (
> -                  mSerialHandle,
> -                  &gEfiDevicePathProtocolGuid,
> -                  &mSerialDevicePath,
> -                  &mSerialDevicePath
> -                  );
> -
> -  gBS->RestoreTPL (Tpl);
> +  Status = This->SetAttributes (
> +                   This,
> +                   This->Mode->BaudRate,
> +                   This->Mode->ReceiveFifoDepth,
> +                   This->Mode->Timeout,
> +                   (EFI_PARITY_TYPE) This->Mode->Parity,
> +                   (UINT8) This->Mode->DataBits,
> +                   (EFI_STOP_BITS_TYPE) This->Mode->StopBits
> +                   );
>  
>    return Status;
>  }
> @@ -513,11 +480,6 @@ SerialDxeInitialize (  {
>    EFI_STATUS            Status;
>  
> -  Status = SerialPortInitialize ();
> -  if (EFI_ERROR (Status)) {
> -    return Status;
> -  }
> -
>    mSerialIoMode.BaudRate = PcdGet64 (PcdUartDefaultBaudRate);
>    mSerialIoMode.DataBits = (UINT32) PcdGet8 (PcdUartDefaultDataBits);
>    mSerialIoMode.Parity   = (UINT32) PcdGet8 (PcdUartDefaultParity);
> @@ -529,6 +491,14 @@ SerialDxeInitialize (
>    mSerialDevicePath.Uart.StopBits = PcdGet8 (PcdUartDefaultStopBits);
>  
>    //
> +  // Issue a reset to initialize the COM port  //  Status = 
> + mSerialIoTemplate.Reset (&mSerialIoTemplate);  if (EFI_ERROR 
> + (Status)) {
> +    return Status;
> +  }
> +
> +  //
>    // Make a new handle with Serial IO protocol and its device path on it.
>    //
>    Status = gBS->InstallMultipleProtocolInterfaces (
> --
> 2.7.4
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH] Fix not able to change serial attributes

[Pankaj Bansal]

Hi Laszlo, Zeng

Thanks for patch review.

@Zeng: The sermode command calls SerialSetAttributes, which sets H/W attributes of Serial device.
After that the SerialIo protocol is reinstalled, which causes MdeModulePkg/Universal/Console/TerminalDxe
and MdeModulePkg/Universal/Console/ConPlatformDxe drivers' bindings to stop and then start.
This in turn calls SerialReset, which undoes changes of SerialSetAttributes.

Thanks & Regards,
Pankaj Bansal
-----Original Message-----
From: Zeng, Star [mailto:star.zeng@intel.com] 
Sent: Monday, September 18, 2017 3:52 PM
To: Laszlo Ersek <lersek@redhat.com>; Heyi Guo <heyi.guo@linaro.org>; Pankaj Bansal <pankaj.bansal@nxp.com>; edk2-devel@lists.01.org
Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: RE: [edk2] [PATCH] Fix not able to change serial attributes

Thanks for your good comments. :)
Since there is no clear description for the behavior of Reset() :(, I prone to align the behavior with MdeModulePkg/Bus/Pci/PciSioSerialDxe/SerialIo.c and IntelFrameworkModulePkg/Bus/Isa/IsaSerialDxe/Serial.c, that means I agree the fix.


Laszlo and Gary, if you can help do some simple regression test with the patch, that will be better.


Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
Sent: Sunday, September 17, 2017 4:18 AM
To: Zeng, Star <star.zeng@intel.com>; Pankaj Bansal <pankaj.bansal@nxp.com>; edk2-devel@lists.01.org
Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
Subject: Re: [edk2] [PATCH] Fix not able to change serial attributes

On 09/16/17 03:21, Zeng, Star wrote:
> Pankaj,
> 
> Thanks for the contribution.
> Could you help update the title to be like MdeModulePkg SerialDxe: XXXXXXXXXX?
> 
> In fact, I agree the fix as it matches the code at MdeModulePkg/Bus/Pci/PciSioSerialDxe/SerialIo.c and IntelFrameworkModulePkg/Bus/Isa/IsaSerialDxe/Serial.c.
> But I am curious about how you reproduce the issue by sermode command as I see sermode command only calls SerialIo->SetAttributes() but not SerialIo->Reset().
> 
> 
> Ray, Laszlo,
> Do you have any comment?

Thanks for the CC.

The UEFI spec is very superficial on EFI_SERIAL_IO_PROTOCOL.Reset(). It only says, "The Reset() function resets the hardware of a serial device." It doesn't define what state the device should return to after resetting the hardware.

Under the generic description of EFI_SERIAL_IO_PROTOCOL, we have

"The default attributes for all UART-style serial device interfaces are:
115,200 baud, a 1 byte receive FIFO, a 1,000,000 microsecond timeout per character, no parity, 8 data bits, and 1 stop bit."

Now, the PCDs that are currently used in the code may differ from the above standard-mandated values, but that's the platform's responsibility. The point is that the UEFI spec seems to require that the device be returned to a predefined state, and the PCDs make that possible. I don't think that the argument in the commit message, "Serial Reset command should set the attributes which have been changed by user after calling SerialSetAttributes.", is consistent with the UEFI spec's intent.

However, I could easily be wrong about this, given especially that the other two SerialIo implementations already follow the suggested practice.

I guess I'll have to stay neutral on this patch. Hopefully it won't regress anything.

Thanks,
Laszlo


> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of 
> Pankaj Bansal
> Sent: Friday, September 15, 2017 9:14 PM
> To: edk2-devel@lists.01.org
> Cc: Pankaj Bansal <pankaj.bansal@nxp.com>
> Subject: [edk2] [PATCH] Fix not able to change serial attributes
> 
> Issue : When try to change serial attributes using sermode command, the default values are set Cause : The SerialReset command resets the attributes' values to default Fix : Serial Reset command should set the attributes which have been changed by user after calling SerialSetAttributes.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
> ---
>  MdeModulePkg/Universal/SerialDxe/SerialIo.c | 66
> ++++++++---------------------
>  1 file changed, 18 insertions(+), 48 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/SerialDxe/SerialIo.c
> b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
> index 43d33db..dc7e13a 100644
> --- a/MdeModulePkg/Universal/SerialDxe/SerialIo.c
> +++ b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
> @@ -220,7 +220,6 @@ SerialReset (
>    )
>  {
>    EFI_STATUS    Status;
> -  EFI_TPL       Tpl;
>  
>    Status = SerialPortInitialize ();
>    if (EFI_ERROR (Status)) {
> @@ -228,49 +227,17 @@ SerialReset (
>    }
>  
>    //
> -  // Set the Serial I/O mode and update the device path
> -  //
> -
> -  Tpl = gBS->RaiseTPL (TPL_NOTIFY);
> -
> -  //
> -  // Set the Serial I/O mode
> -  //
> -  This->Mode->ReceiveFifoDepth  = PcdGet16 (PcdUartDefaultReceiveFifoDepth);
> -  This->Mode->Timeout           = 1000 * 1000;
> -  This->Mode->BaudRate          = PcdGet64 (PcdUartDefaultBaudRate);
> -  This->Mode->DataBits          = (UINT32) PcdGet8 (PcdUartDefaultDataBits);
> -  This->Mode->Parity            = (UINT32) PcdGet8 (PcdUartDefaultParity);
> -  This->Mode->StopBits          = (UINT32) PcdGet8 (PcdUartDefaultStopBits);
> -
> -  //
> -  // Check if the device path has actually changed
> -  //
> -  if (mSerialDevicePath.Uart.BaudRate == This->Mode->BaudRate &&
> -      mSerialDevicePath.Uart.DataBits == (UINT8) This->Mode->DataBits &&
> -      mSerialDevicePath.Uart.Parity   == (UINT8) This->Mode->Parity &&
> -      mSerialDevicePath.Uart.StopBits == (UINT8) This->Mode->StopBits
> -     ) {
> -    gBS->RestoreTPL (Tpl);
> -    return EFI_SUCCESS;
> -  }
> -
> -  //
> -  // Update the device path
> +  // Go set the current attributes
>    //
> -  mSerialDevicePath.Uart.BaudRate = This->Mode->BaudRate;
> -  mSerialDevicePath.Uart.DataBits = (UINT8) This->Mode->DataBits;
> -  mSerialDevicePath.Uart.Parity   = (UINT8) This->Mode->Parity;
> -  mSerialDevicePath.Uart.StopBits = (UINT8) This->Mode->StopBits;
> -
> -  Status = gBS->ReinstallProtocolInterface (
> -                  mSerialHandle,
> -                  &gEfiDevicePathProtocolGuid,
> -                  &mSerialDevicePath,
> -                  &mSerialDevicePath
> -                  );
> -
> -  gBS->RestoreTPL (Tpl);
> +  Status = This->SetAttributes (
> +                   This,
> +                   This->Mode->BaudRate,
> +                   This->Mode->ReceiveFifoDepth,
> +                   This->Mode->Timeout,
> +                   (EFI_PARITY_TYPE) This->Mode->Parity,
> +                   (UINT8) This->Mode->DataBits,
> +                   (EFI_STOP_BITS_TYPE) This->Mode->StopBits
> +                   );
>  
>    return Status;
>  }
> @@ -513,11 +480,6 @@ SerialDxeInitialize (  {
>    EFI_STATUS            Status;
>  
> -  Status = SerialPortInitialize ();
> -  if (EFI_ERROR (Status)) {
> -    return Status;
> -  }
> -
>    mSerialIoMode.BaudRate = PcdGet64 (PcdUartDefaultBaudRate);
>    mSerialIoMode.DataBits = (UINT32) PcdGet8 (PcdUartDefaultDataBits);
>    mSerialIoMode.Parity   = (UINT32) PcdGet8 (PcdUartDefaultParity);
> @@ -529,6 +491,14 @@ SerialDxeInitialize (
>    mSerialDevicePath.Uart.StopBits = PcdGet8 (PcdUartDefaultStopBits);
>  
>    //
> +  // Issue a reset to initialize the COM port  //  Status = 
> + mSerialIoTemplate.Reset (&mSerialIoTemplate);  if (EFI_ERROR
> + (Status)) {
> +    return Status;
> +  }
> +
> +  //
>    // Make a new handle with Serial IO protocol and its device path on it.
>    //
>    Status = gBS->InstallMultipleProtocolInterfaces (
> --
> 2.7.4
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH] Fix not able to change serial attributes

[Laszlo Ersek]

On 09/18/17 12:22, Zeng, Star wrote:
> Thanks for your good comments. :)
> Since there is no clear description for the behavior of Reset() :(, I prone to align the behavior with MdeModulePkg/Bus/Pci/PciSioSerialDxe/SerialIo.c and IntelFrameworkModulePkg/Bus/Isa/IsaSerialDxe/Serial.c, that means I agree the fix.
> 
> 
> Laszlo and Gary, if you can help do some simple regression test with the patch, that will be better.

Never used the SERMODE command before, so I didn't try it now either.

I tested the patch as follows: built ArmVirtQemu with it, and first
checked normal use of the serial terminal, then issued a
RECONNECT/CONNECT from the graphics console, and then verified if the
serial terminal was working OK again (UEFI shell and Setup utility). I
found no regression with this.

Regression-tested-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

Re: [PATCH] Fix not able to change serial attributes

[Zeng, Star]

Really appreciate the Regression-tested-by, I will pick up it when pushing the patch.


Thanks,
Star
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Monday, September 18, 2017 11:55 PM
To: Zeng, Star <star.zeng@intel.com>; Heyi Guo <heyi.guo@linaro.org>; Pankaj Bansal <pankaj.bansal@nxp.com>; edk2-devel@lists.01.org
Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
Subject: Re: [edk2] [PATCH] Fix not able to change serial attributes

On 09/18/17 12:22, Zeng, Star wrote:
> Thanks for your good comments. :)
> Since there is no clear description for the behavior of Reset() :(, I prone to align the behavior with MdeModulePkg/Bus/Pci/PciSioSerialDxe/SerialIo.c and IntelFrameworkModulePkg/Bus/Isa/IsaSerialDxe/Serial.c, that means I agree the fix.
> 
> 
> Laszlo and Gary, if you can help do some simple regression test with the patch, that will be better.

Never used the SERMODE command before, so I didn't try it now either.

I tested the patch as follows: built ArmVirtQemu with it, and first checked normal use of the serial terminal, then issued a RECONNECT/CONNECT from the graphics console, and then verified if the serial terminal was working OK again (UEFI shell and Setup utility). I found no regression with this.

Regression-tested-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

(No subject)

[Pankaj Bansal]

Hi Laszlo, Zeng

Thanks for patch review.

@Zeng: The sermode command calls SerialSetAttributes, which sets H/W attributes of Serial device.
After that the SerialIo protocol is reinstalled, which causes MdeModulePkg/Universal/Console/TerminalDxe
and MdeModulePkg/Universal/Console/ConPlatformDxe drivers' bindings to stop and then start.
This in turn calls SerialReset, which undoes changes of SerialSetAttributes.

[PATCH v2] MdeModulePkg/SerialDxe: Fix not able to change serial attributes

[Pankaj Bansal]

Issue : When try to change serial attributes using sermode
command, the default values are set
Cause : The SerialReset command resets the attributes' values
to default
Fix : Serial Reset command should set the attributes which have
been changed by user after calling SerialSetAttributes.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
---
Changes in v2:
   - Modified Patch description

 MdeModulePkg/Universal/SerialDxe/SerialIo.c | 66 ++++++++---------------------
 1 file changed, 18 insertions(+), 48 deletions(-)

diff --git a/MdeModulePkg/Universal/SerialDxe/SerialIo.c b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
index 43d33db..dc7e13a 100644
--- a/MdeModulePkg/Universal/SerialDxe/SerialIo.c
+++ b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
@@ -220,7 +220,6 @@ SerialReset (
   )
 {
   EFI_STATUS    Status;
-  EFI_TPL       Tpl;
 
   Status = SerialPortInitialize ();
   if (EFI_ERROR (Status)) {
@@ -228,49 +227,17 @@ SerialReset (
   }
 
   //
-  // Set the Serial I/O mode and update the device path
-  //
-
-  Tpl = gBS->RaiseTPL (TPL_NOTIFY);
-
-  //
-  // Set the Serial I/O mode
-  //
-  This->Mode->ReceiveFifoDepth  = PcdGet16 (PcdUartDefaultReceiveFifoDepth);
-  This->Mode->Timeout           = 1000 * 1000;
-  This->Mode->BaudRate          = PcdGet64 (PcdUartDefaultBaudRate);
-  This->Mode->DataBits          = (UINT32) PcdGet8 (PcdUartDefaultDataBits);
-  This->Mode->Parity            = (UINT32) PcdGet8 (PcdUartDefaultParity);
-  This->Mode->StopBits          = (UINT32) PcdGet8 (PcdUartDefaultStopBits);
-
-  //
-  // Check if the device path has actually changed
-  //
-  if (mSerialDevicePath.Uart.BaudRate == This->Mode->BaudRate &&
-      mSerialDevicePath.Uart.DataBits == (UINT8) This->Mode->DataBits &&
-      mSerialDevicePath.Uart.Parity   == (UINT8) This->Mode->Parity &&
-      mSerialDevicePath.Uart.StopBits == (UINT8) This->Mode->StopBits
-     ) {
-    gBS->RestoreTPL (Tpl);
-    return EFI_SUCCESS;
-  }
-
-  //
-  // Update the device path
+  // Go set the current attributes
   //
-  mSerialDevicePath.Uart.BaudRate = This->Mode->BaudRate;
-  mSerialDevicePath.Uart.DataBits = (UINT8) This->Mode->DataBits;
-  mSerialDevicePath.Uart.Parity   = (UINT8) This->Mode->Parity;
-  mSerialDevicePath.Uart.StopBits = (UINT8) This->Mode->StopBits;
-
-  Status = gBS->ReinstallProtocolInterface (
-                  mSerialHandle,
-                  &gEfiDevicePathProtocolGuid,
-                  &mSerialDevicePath,
-                  &mSerialDevicePath
-                  );
-
-  gBS->RestoreTPL (Tpl);
+  Status = This->SetAttributes (
+                   This,
+                   This->Mode->BaudRate,
+                   This->Mode->ReceiveFifoDepth,
+                   This->Mode->Timeout,
+                   (EFI_PARITY_TYPE) This->Mode->Parity,
+                   (UINT8) This->Mode->DataBits,
+                   (EFI_STOP_BITS_TYPE) This->Mode->StopBits
+                   );
 
   return Status;
 }
@@ -513,11 +480,6 @@ SerialDxeInitialize (
 {
   EFI_STATUS            Status;
 
-  Status = SerialPortInitialize ();
-  if (EFI_ERROR (Status)) {
-    return Status;
-  }
-
   mSerialIoMode.BaudRate = PcdGet64 (PcdUartDefaultBaudRate);
   mSerialIoMode.DataBits = (UINT32) PcdGet8 (PcdUartDefaultDataBits);
   mSerialIoMode.Parity   = (UINT32) PcdGet8 (PcdUartDefaultParity);
@@ -529,6 +491,14 @@ SerialDxeInitialize (
   mSerialDevicePath.Uart.StopBits = PcdGet8 (PcdUartDefaultStopBits);
 
   //
+  // Issue a reset to initialize the COM port
+  //
+  Status = mSerialIoTemplate.Reset (&mSerialIoTemplate);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  //
   // Make a new handle with Serial IO protocol and its device path on it.
   //
   Status = gBS->InstallMultipleProtocolInterfaces (
-- 
2.7.4

Re: [PATCH v2] MdeModulePkg/SerialDxe: Fix not able to change serial attributes

[Zeng, Star]

Pankaj,

Thanks for the update.

I raised a question in the V1 patch review, could you help kindly provide the feedback?
"how you reproduce the issue by sermode command as I see sermode command only calls SerialIo->SetAttributes() but not SerialIo->Reset()"


Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Pankaj Bansal
Sent: Monday, September 18, 2017 3:43 PM
To: edk2-devel@lists.01.org
Cc: Pankaj Bansal <pankaj.bansal@nxp.com>
Subject: [edk2] [PATCH v2] MdeModulePkg/SerialDxe: Fix not able to change serial attributes

Issue : When try to change serial attributes using sermode command, the default values are set Cause : The SerialReset command resets the attributes' values to default Fix : Serial Reset command should set the attributes which have been changed by user after calling SerialSetAttributes.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
---
Changes in v2:
   - Modified Patch description

 MdeModulePkg/Universal/SerialDxe/SerialIo.c | 66 ++++++++---------------------
 1 file changed, 18 insertions(+), 48 deletions(-)

diff --git a/MdeModulePkg/Universal/SerialDxe/SerialIo.c b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
index 43d33db..dc7e13a 100644
--- a/MdeModulePkg/Universal/SerialDxe/SerialIo.c
+++ b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
@@ -220,7 +220,6 @@ SerialReset (
   )
 {
   EFI_STATUS    Status;
-  EFI_TPL       Tpl;
 
   Status = SerialPortInitialize ();
   if (EFI_ERROR (Status)) {
@@ -228,49 +227,17 @@ SerialReset (
   }
 
   //
-  // Set the Serial I/O mode and update the device path
-  //
-
-  Tpl = gBS->RaiseTPL (TPL_NOTIFY);
-
-  //
-  // Set the Serial I/O mode
-  //
-  This->Mode->ReceiveFifoDepth  = PcdGet16 (PcdUartDefaultReceiveFifoDepth);
-  This->Mode->Timeout           = 1000 * 1000;
-  This->Mode->BaudRate          = PcdGet64 (PcdUartDefaultBaudRate);
-  This->Mode->DataBits          = (UINT32) PcdGet8 (PcdUartDefaultDataBits);
-  This->Mode->Parity            = (UINT32) PcdGet8 (PcdUartDefaultParity);
-  This->Mode->StopBits          = (UINT32) PcdGet8 (PcdUartDefaultStopBits);
-
-  //
-  // Check if the device path has actually changed
-  //
-  if (mSerialDevicePath.Uart.BaudRate == This->Mode->BaudRate &&
-      mSerialDevicePath.Uart.DataBits == (UINT8) This->Mode->DataBits &&
-      mSerialDevicePath.Uart.Parity   == (UINT8) This->Mode->Parity &&
-      mSerialDevicePath.Uart.StopBits == (UINT8) This->Mode->StopBits
-     ) {
-    gBS->RestoreTPL (Tpl);
-    return EFI_SUCCESS;
-  }
-
-  //
-  // Update the device path
+  // Go set the current attributes
   //
-  mSerialDevicePath.Uart.BaudRate = This->Mode->BaudRate;
-  mSerialDevicePath.Uart.DataBits = (UINT8) This->Mode->DataBits;
-  mSerialDevicePath.Uart.Parity   = (UINT8) This->Mode->Parity;
-  mSerialDevicePath.Uart.StopBits = (UINT8) This->Mode->StopBits;
-
-  Status = gBS->ReinstallProtocolInterface (
-                  mSerialHandle,
-                  &gEfiDevicePathProtocolGuid,
-                  &mSerialDevicePath,
-                  &mSerialDevicePath
-                  );
-
-  gBS->RestoreTPL (Tpl);
+  Status = This->SetAttributes (
+                   This,
+                   This->Mode->BaudRate,
+                   This->Mode->ReceiveFifoDepth,
+                   This->Mode->Timeout,
+                   (EFI_PARITY_TYPE) This->Mode->Parity,
+                   (UINT8) This->Mode->DataBits,
+                   (EFI_STOP_BITS_TYPE) This->Mode->StopBits
+                   );
 
   return Status;
 }
@@ -513,11 +480,6 @@ SerialDxeInitialize (  {
   EFI_STATUS            Status;
 
-  Status = SerialPortInitialize ();
-  if (EFI_ERROR (Status)) {
-    return Status;
-  }
-
   mSerialIoMode.BaudRate = PcdGet64 (PcdUartDefaultBaudRate);
   mSerialIoMode.DataBits = (UINT32) PcdGet8 (PcdUartDefaultDataBits);
   mSerialIoMode.Parity   = (UINT32) PcdGet8 (PcdUartDefaultParity);
@@ -529,6 +491,14 @@ SerialDxeInitialize (
   mSerialDevicePath.Uart.StopBits = PcdGet8 (PcdUartDefaultStopBits);
 
   //
+  // Issue a reset to initialize the COM port  //  Status = 
+ mSerialIoTemplate.Reset (&mSerialIoTemplate);  if (EFI_ERROR (Status)) 
+ {
+    return Status;
+  }
+
+  //
   // Make a new handle with Serial IO protocol and its device path on it.
   //
   Status = gBS->InstallMultipleProtocolInterfaces (
--
2.7.4

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH v2] MdeModulePkg/SerialDxe: Fix not able to change serial attributes

[Zeng, Star]

Pankaj,

Thanks for your explanation at https://lists.01.org/pipermail/edk2-devel/2017-September/014832.html about the execute flow to reproduce the issue.
How about also include the explanation to the commit log like below?

==========
Issue : When try to change serial attributes using sermode
command, the default values are set with the execute flow
as below.

The sermode command calls SerialSetAttributes, which sets H/W
attributes of Serial device. After that the SerialIo protocol is
reinstalled, which causes MdeModulePkg/Universal/Console/TerminalDxe
and MdeModulePkg/Universal/Console/ConPlatformDxe drivers' bindings
to stop and then start. This in turn calls SerialReset, which undoes
changes of SerialSetAttributes.

Cause : The SerialReset command resets the attributes' values
to default.
Fix : Serial Reset command should set the attributes which have
been changed by user after calling SerialSetAttributes.
==========

Another minor comment, how about update "COM port" to be "Serial Port" in the change?


If you agree with above comments, you do not need to send an update patch and
I will help push the patch with the updates I comment above and Reviewed-by: Star Zeng <star.zeng@intel.com>,
and of course Regression-tested-by: Laszlo Ersek <lersek@redhat.com>.


Thanks,
Star
-----Original Message-----
From: Zeng, Star 
Sent: Monday, September 18, 2017 6:12 PM
To: Pankaj Bansal <pankaj.bansal@nxp.com>; edk2-devel@lists.01.org
Cc: Zeng, Star <star.zeng@intel.com>
Subject: RE: [edk2] [PATCH v2] MdeModulePkg/SerialDxe: Fix not able to change serial attributes

Pankaj,

Thanks for the update.

I raised a question in the V1 patch review, could you help kindly provide the feedback?
"how you reproduce the issue by sermode command as I see sermode command only calls SerialIo->SetAttributes() but not SerialIo->Reset()"


Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Pankaj Bansal
Sent: Monday, September 18, 2017 3:43 PM
To: edk2-devel@lists.01.org
Cc: Pankaj Bansal <pankaj.bansal@nxp.com>
Subject: [edk2] [PATCH v2] MdeModulePkg/SerialDxe: Fix not able to change serial attributes

Issue : When try to change serial attributes using sermode command, the default values are set Cause : The SerialReset command resets the attributes' values to default Fix : Serial Reset command should set the attributes which have been changed by user after calling SerialSetAttributes.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
---
Changes in v2:
   - Modified Patch description

 MdeModulePkg/Universal/SerialDxe/SerialIo.c | 66 ++++++++---------------------
 1 file changed, 18 insertions(+), 48 deletions(-)

diff --git a/MdeModulePkg/Universal/SerialDxe/SerialIo.c b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
index 43d33db..dc7e13a 100644
--- a/MdeModulePkg/Universal/SerialDxe/SerialIo.c
+++ b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
@@ -220,7 +220,6 @@ SerialReset (
   )
 {
   EFI_STATUS    Status;
-  EFI_TPL       Tpl;
 
   Status = SerialPortInitialize ();
   if (EFI_ERROR (Status)) {
@@ -228,49 +227,17 @@ SerialReset (
   }
 
   //
-  // Set the Serial I/O mode and update the device path
-  //
-
-  Tpl = gBS->RaiseTPL (TPL_NOTIFY);
-
-  //
-  // Set the Serial I/O mode
-  //
-  This->Mode->ReceiveFifoDepth  = PcdGet16 (PcdUartDefaultReceiveFifoDepth);
-  This->Mode->Timeout           = 1000 * 1000;
-  This->Mode->BaudRate          = PcdGet64 (PcdUartDefaultBaudRate);
-  This->Mode->DataBits          = (UINT32) PcdGet8 (PcdUartDefaultDataBits);
-  This->Mode->Parity            = (UINT32) PcdGet8 (PcdUartDefaultParity);
-  This->Mode->StopBits          = (UINT32) PcdGet8 (PcdUartDefaultStopBits);
-
-  //
-  // Check if the device path has actually changed
-  //
-  if (mSerialDevicePath.Uart.BaudRate == This->Mode->BaudRate &&
-      mSerialDevicePath.Uart.DataBits == (UINT8) This->Mode->DataBits &&
-      mSerialDevicePath.Uart.Parity   == (UINT8) This->Mode->Parity &&
-      mSerialDevicePath.Uart.StopBits == (UINT8) This->Mode->StopBits
-     ) {
-    gBS->RestoreTPL (Tpl);
-    return EFI_SUCCESS;
-  }
-
-  //
-  // Update the device path
+  // Go set the current attributes
   //
-  mSerialDevicePath.Uart.BaudRate = This->Mode->BaudRate;
-  mSerialDevicePath.Uart.DataBits = (UINT8) This->Mode->DataBits;
-  mSerialDevicePath.Uart.Parity   = (UINT8) This->Mode->Parity;
-  mSerialDevicePath.Uart.StopBits = (UINT8) This->Mode->StopBits;
-
-  Status = gBS->ReinstallProtocolInterface (
-                  mSerialHandle,
-                  &gEfiDevicePathProtocolGuid,
-                  &mSerialDevicePath,
-                  &mSerialDevicePath
-                  );
-
-  gBS->RestoreTPL (Tpl);
+  Status = This->SetAttributes (
+                   This,
+                   This->Mode->BaudRate,
+                   This->Mode->ReceiveFifoDepth,
+                   This->Mode->Timeout,
+                   (EFI_PARITY_TYPE) This->Mode->Parity,
+                   (UINT8) This->Mode->DataBits,
+                   (EFI_STOP_BITS_TYPE) This->Mode->StopBits
+                   );
 
   return Status;
 }
@@ -513,11 +480,6 @@ SerialDxeInitialize (  {
   EFI_STATUS            Status;
 
-  Status = SerialPortInitialize ();
-  if (EFI_ERROR (Status)) {
-    return Status;
-  }
-
   mSerialIoMode.BaudRate = PcdGet64 (PcdUartDefaultBaudRate);
   mSerialIoMode.DataBits = (UINT32) PcdGet8 (PcdUartDefaultDataBits);
   mSerialIoMode.Parity   = (UINT32) PcdGet8 (PcdUartDefaultParity);
@@ -529,6 +491,14 @@ SerialDxeInitialize (
   mSerialDevicePath.Uart.StopBits = PcdGet8 (PcdUartDefaultStopBits);
 
   //
+  // Issue a reset to initialize the COM port  //  Status = 
+ mSerialIoTemplate.Reset (&mSerialIoTemplate);  if (EFI_ERROR (Status)) 
+ {
+    return Status;
+  }
+
+  //
   // Make a new handle with Serial IO protocol and its device path on it.
   //
   Status = gBS->InstallMultipleProtocolInterfaces (
--
2.7.4

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH v2] MdeModulePkg/SerialDxe: Fix not able to change serial attributes

[Pankaj Bansal]

Hi Zeng,

Thanks for suggestion.
I agree, that this looks nicer and gives more details.
Please push the patch with comment updates and signatures as you have mentioned.

Thanks & Regards,
Pankaj Bansal

-----Original Message-----
From: Zeng, Star [mailto:star.zeng@intel.com] 
Sent: Tuesday, September 19, 2017 8:30 AM
To: Pankaj Bansal <pankaj.bansal@nxp.com>; edk2-devel@lists.01.org
Cc: Zeng, Star <star.zeng@intel.com>; Laszlo Ersek (lersek@redhat.com) <lersek@redhat.com>
Subject: RE: [edk2] [PATCH v2] MdeModulePkg/SerialDxe: Fix not able to change serial attributes

Pankaj,

Thanks for your explanation at https://lists.01.org/pipermail/edk2-devel/2017-September/014832.html about the execute flow to reproduce the issue.
How about also include the explanation to the commit log like below?

==========
Issue : When try to change serial attributes using sermode command, the default values are set with the execute flow as below.

The sermode command calls SerialSetAttributes, which sets H/W attributes of Serial device. After that the SerialIo protocol is reinstalled, which causes MdeModulePkg/Universal/Console/TerminalDxe
and MdeModulePkg/Universal/Console/ConPlatformDxe drivers' bindings to stop and then start. This in turn calls SerialReset, which undoes changes of SerialSetAttributes.

Cause : The SerialReset command resets the attributes' values to default.
Fix : Serial Reset command should set the attributes which have been changed by user after calling SerialSetAttributes.
==========

Another minor comment, how about update "COM port" to be "Serial Port" in the change?


If you agree with above comments, you do not need to send an update patch and I will help push the patch with the updates I comment above and Reviewed-by: Star Zeng <star.zeng@intel.com>, and of course Regression-tested-by: Laszlo Ersek <lersek@redhat.com>.


Thanks,
Star
-----Original Message-----
From: Zeng, Star
Sent: Monday, September 18, 2017 6:12 PM
To: Pankaj Bansal <pankaj.bansal@nxp.com>; edk2-devel@lists.01.org
Cc: Zeng, Star <star.zeng@intel.com>
Subject: RE: [edk2] [PATCH v2] MdeModulePkg/SerialDxe: Fix not able to change serial attributes

Pankaj,

Thanks for the update.

I raised a question in the V1 patch review, could you help kindly provide the feedback?
"how you reproduce the issue by sermode command as I see sermode command only calls SerialIo->SetAttributes() but not SerialIo->Reset()"


Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Pankaj Bansal
Sent: Monday, September 18, 2017 3:43 PM
To: edk2-devel@lists.01.org
Cc: Pankaj Bansal <pankaj.bansal@nxp.com>
Subject: [edk2] [PATCH v2] MdeModulePkg/SerialDxe: Fix not able to change serial attributes

Issue : When try to change serial attributes using sermode command, the default values are set Cause : The SerialReset command resets the attributes' values to default Fix : Serial Reset command should set the attributes which have been changed by user after calling SerialSetAttributes.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
---
Changes in v2:
   - Modified Patch description

 MdeModulePkg/Universal/SerialDxe/SerialIo.c | 66 ++++++++---------------------
 1 file changed, 18 insertions(+), 48 deletions(-)

diff --git a/MdeModulePkg/Universal/SerialDxe/SerialIo.c b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
index 43d33db..dc7e13a 100644
--- a/MdeModulePkg/Universal/SerialDxe/SerialIo.c
+++ b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
@@ -220,7 +220,6 @@ SerialReset (
   )
 {
   EFI_STATUS    Status;
-  EFI_TPL       Tpl;
 
   Status = SerialPortInitialize ();
   if (EFI_ERROR (Status)) {
@@ -228,49 +227,17 @@ SerialReset (
   }
 
   //
-  // Set the Serial I/O mode and update the device path
-  //
-
-  Tpl = gBS->RaiseTPL (TPL_NOTIFY);
-
-  //
-  // Set the Serial I/O mode
-  //
-  This->Mode->ReceiveFifoDepth  = PcdGet16 (PcdUartDefaultReceiveFifoDepth);
-  This->Mode->Timeout           = 1000 * 1000;
-  This->Mode->BaudRate          = PcdGet64 (PcdUartDefaultBaudRate);
-  This->Mode->DataBits          = (UINT32) PcdGet8 (PcdUartDefaultDataBits);
-  This->Mode->Parity            = (UINT32) PcdGet8 (PcdUartDefaultParity);
-  This->Mode->StopBits          = (UINT32) PcdGet8 (PcdUartDefaultStopBits);
-
-  //
-  // Check if the device path has actually changed
-  //
-  if (mSerialDevicePath.Uart.BaudRate == This->Mode->BaudRate &&
-      mSerialDevicePath.Uart.DataBits == (UINT8) This->Mode->DataBits &&
-      mSerialDevicePath.Uart.Parity   == (UINT8) This->Mode->Parity &&
-      mSerialDevicePath.Uart.StopBits == (UINT8) This->Mode->StopBits
-     ) {
-    gBS->RestoreTPL (Tpl);
-    return EFI_SUCCESS;
-  }
-
-  //
-  // Update the device path
+  // Go set the current attributes
   //
-  mSerialDevicePath.Uart.BaudRate = This->Mode->BaudRate;
-  mSerialDevicePath.Uart.DataBits = (UINT8) This->Mode->DataBits;
-  mSerialDevicePath.Uart.Parity   = (UINT8) This->Mode->Parity;
-  mSerialDevicePath.Uart.StopBits = (UINT8) This->Mode->StopBits;
-
-  Status = gBS->ReinstallProtocolInterface (
-                  mSerialHandle,
-                  &gEfiDevicePathProtocolGuid,
-                  &mSerialDevicePath,
-                  &mSerialDevicePath
-                  );
-
-  gBS->RestoreTPL (Tpl);
+  Status = This->SetAttributes (
+                   This,
+                   This->Mode->BaudRate,
+                   This->Mode->ReceiveFifoDepth,
+                   This->Mode->Timeout,
+                   (EFI_PARITY_TYPE) This->Mode->Parity,
+                   (UINT8) This->Mode->DataBits,
+                   (EFI_STOP_BITS_TYPE) This->Mode->StopBits
+                   );
 
   return Status;
 }
@@ -513,11 +480,6 @@ SerialDxeInitialize (  {
   EFI_STATUS            Status;
 
-  Status = SerialPortInitialize ();
-  if (EFI_ERROR (Status)) {
-    return Status;
-  }
-
   mSerialIoMode.BaudRate = PcdGet64 (PcdUartDefaultBaudRate);
   mSerialIoMode.DataBits = (UINT32) PcdGet8 (PcdUartDefaultDataBits);
   mSerialIoMode.Parity   = (UINT32) PcdGet8 (PcdUartDefaultParity);
@@ -529,6 +491,14 @@ SerialDxeInitialize (
   mSerialDevicePath.Uart.StopBits = PcdGet8 (PcdUartDefaultStopBits);
 
   //
+  // Issue a reset to initialize the COM port  //  Status = 
+ mSerialIoTemplate.Reset (&mSerialIoTemplate);  if (EFI_ERROR (Status)) 
+ {
+    return Status;
+  }
+
+  //
   // Make a new handle with Serial IO protocol and its device path on it.
   //
   Status = gBS->InstallMultipleProtocolInterfaces (
--
2.7.4

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH v2] MdeModulePkg/SerialDxe: Fix not able to change serial attributes

[Zeng, Star]

Pushed at 91cc526b15ffbbbdec5a57906596f37e059f80be :)

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Pankaj Bansal
Sent: Tuesday, September 19, 2017 11:05 AM
To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
Cc: Laszlo Ersek (lersek@redhat.com) <lersek@redhat.com>
Subject: Re: [edk2] [PATCH v2] MdeModulePkg/SerialDxe: Fix not able to change serial attributes

Hi Zeng,

Thanks for suggestion.
I agree, that this looks nicer and gives more details.
Please push the patch with comment updates and signatures as you have mentioned.

Thanks & Regards,
Pankaj Bansal

-----Original Message-----
From: Zeng, Star [mailto:star.zeng@intel.com]
Sent: Tuesday, September 19, 2017 8:30 AM
To: Pankaj Bansal <pankaj.bansal@nxp.com>; edk2-devel@lists.01.org
Cc: Zeng, Star <star.zeng@intel.com>; Laszlo Ersek (lersek@redhat.com) <lersek@redhat.com>
Subject: RE: [edk2] [PATCH v2] MdeModulePkg/SerialDxe: Fix not able to change serial attributes

Pankaj,

Thanks for your explanation at https://lists.01.org/pipermail/edk2-devel/2017-September/014832.html about the execute flow to reproduce the issue.
How about also include the explanation to the commit log like below?

==========
Issue : When try to change serial attributes using sermode command, the default values are set with the execute flow as below.

The sermode command calls SerialSetAttributes, which sets H/W attributes of Serial device. After that the SerialIo protocol is reinstalled, which causes MdeModulePkg/Universal/Console/TerminalDxe
and MdeModulePkg/Universal/Console/ConPlatformDxe drivers' bindings to stop and then start. This in turn calls SerialReset, which undoes changes of SerialSetAttributes.

Cause : The SerialReset command resets the attributes' values to default.
Fix : Serial Reset command should set the attributes which have been changed by user after calling SerialSetAttributes.
==========

Another minor comment, how about update "COM port" to be "Serial Port" in the change?


If you agree with above comments, you do not need to send an update patch and I will help push the patch with the updates I comment above and Reviewed-by: Star Zeng <star.zeng@intel.com>, and of course Regression-tested-by: Laszlo Ersek <lersek@redhat.com>.


Thanks,
Star
-----Original Message-----
From: Zeng, Star
Sent: Monday, September 18, 2017 6:12 PM
To: Pankaj Bansal <pankaj.bansal@nxp.com>; edk2-devel@lists.01.org
Cc: Zeng, Star <star.zeng@intel.com>
Subject: RE: [edk2] [PATCH v2] MdeModulePkg/SerialDxe: Fix not able to change serial attributes

Pankaj,

Thanks for the update.

I raised a question in the V1 patch review, could you help kindly provide the feedback?
"how you reproduce the issue by sermode command as I see sermode command only calls SerialIo->SetAttributes() but not SerialIo->Reset()"


Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Pankaj Bansal
Sent: Monday, September 18, 2017 3:43 PM
To: edk2-devel@lists.01.org
Cc: Pankaj Bansal <pankaj.bansal@nxp.com>
Subject: [edk2] [PATCH v2] MdeModulePkg/SerialDxe: Fix not able to change serial attributes

Issue : When try to change serial attributes using sermode command, the default values are set Cause : The SerialReset command resets the attributes' values to default Fix : Serial Reset command should set the attributes which have been changed by user after calling SerialSetAttributes.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
---
Changes in v2:
   - Modified Patch description

 MdeModulePkg/Universal/SerialDxe/SerialIo.c | 66 ++++++++---------------------
 1 file changed, 18 insertions(+), 48 deletions(-)

diff --git a/MdeModulePkg/Universal/SerialDxe/SerialIo.c b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
index 43d33db..dc7e13a 100644
--- a/MdeModulePkg/Universal/SerialDxe/SerialIo.c
+++ b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
@@ -220,7 +220,6 @@ SerialReset (
   )
 {
   EFI_STATUS    Status;
-  EFI_TPL       Tpl;
 
   Status = SerialPortInitialize ();
   if (EFI_ERROR (Status)) {
@@ -228,49 +227,17 @@ SerialReset (
   }
 
   //
-  // Set the Serial I/O mode and update the device path
-  //
-
-  Tpl = gBS->RaiseTPL (TPL_NOTIFY);
-
-  //
-  // Set the Serial I/O mode
-  //
-  This->Mode->ReceiveFifoDepth  = PcdGet16 (PcdUartDefaultReceiveFifoDepth);
-  This->Mode->Timeout           = 1000 * 1000;
-  This->Mode->BaudRate          = PcdGet64 (PcdUartDefaultBaudRate);
-  This->Mode->DataBits          = (UINT32) PcdGet8 (PcdUartDefaultDataBits);
-  This->Mode->Parity            = (UINT32) PcdGet8 (PcdUartDefaultParity);
-  This->Mode->StopBits          = (UINT32) PcdGet8 (PcdUartDefaultStopBits);
-
-  //
-  // Check if the device path has actually changed
-  //
-  if (mSerialDevicePath.Uart.BaudRate == This->Mode->BaudRate &&
-      mSerialDevicePath.Uart.DataBits == (UINT8) This->Mode->DataBits &&
-      mSerialDevicePath.Uart.Parity   == (UINT8) This->Mode->Parity &&
-      mSerialDevicePath.Uart.StopBits == (UINT8) This->Mode->StopBits
-     ) {
-    gBS->RestoreTPL (Tpl);
-    return EFI_SUCCESS;
-  }
-
-  //
-  // Update the device path
+  // Go set the current attributes
   //
-  mSerialDevicePath.Uart.BaudRate = This->Mode->BaudRate;
-  mSerialDevicePath.Uart.DataBits = (UINT8) This->Mode->DataBits;
-  mSerialDevicePath.Uart.Parity   = (UINT8) This->Mode->Parity;
-  mSerialDevicePath.Uart.StopBits = (UINT8) This->Mode->StopBits;
-
-  Status = gBS->ReinstallProtocolInterface (
-                  mSerialHandle,
-                  &gEfiDevicePathProtocolGuid,
-                  &mSerialDevicePath,
-                  &mSerialDevicePath
-                  );
-
-  gBS->RestoreTPL (Tpl);
+  Status = This->SetAttributes (
+                   This,
+                   This->Mode->BaudRate,
+                   This->Mode->ReceiveFifoDepth,
+                   This->Mode->Timeout,
+                   (EFI_PARITY_TYPE) This->Mode->Parity,
+                   (UINT8) This->Mode->DataBits,
+                   (EFI_STOP_BITS_TYPE) This->Mode->StopBits
+                   );
 
   return Status;
 }
@@ -513,11 +480,6 @@ SerialDxeInitialize (  {
   EFI_STATUS            Status;
 
-  Status = SerialPortInitialize ();
-  if (EFI_ERROR (Status)) {
-    return Status;
-  }
-
   mSerialIoMode.BaudRate = PcdGet64 (PcdUartDefaultBaudRate);
   mSerialIoMode.DataBits = (UINT32) PcdGet8 (PcdUartDefaultDataBits);
   mSerialIoMode.Parity   = (UINT32) PcdGet8 (PcdUartDefaultParity);
@@ -529,6 +491,14 @@ SerialDxeInitialize (
   mSerialDevicePath.Uart.StopBits = PcdGet8 (PcdUartDefaultStopBits);
 
   //
+  // Issue a reset to initialize the COM port  //  Status = 
+ mSerialIoTemplate.Reset (&mSerialIoTemplate);  if (EFI_ERROR (Status)) 
+ {
+    return Status;
+  }
+
+  //
   // Make a new handle with Serial IO protocol and its device path on it.
   //
   Status = gBS->InstallMultipleProtocolInterfaces (
--
2.7.4

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel