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

[Pankaj Bansal <pankaj.bansal@nxp.com>]

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