On 01/21/16 03:20, Ni, Ruiyu wrote: > Laszlo, > I may not agree with your change to the Terminal driver. > Firstly it's not a good code practice to not preserve the other fields when > changing > the time out value. People may forget that the FIFO depth was changed in > Terminal > driver after a long enough period.
I don't understand this argument. People may forget anything at all; that's why they should look at the code. Do you mean that the receive FIFO depth parameter should be changed in a different / separate SetAttributes() call, in the BindingStart() function of the terminal driver? I.e., independently of the Timeout setting? > Secondly your change resets the FIFO depth but > it's possible that the device may not in the default FIFO depth which may > bring > functionality issue. I think I disagree. The serial IO protocol is not a service binding-style protocol, so when you open it with the BY_DRIVER attribute, and another agent already has it open with the BY_DRIVER attribute, EFI_ACCESS_DENIED is returned. This means in practice that TerminalDxe gets exclusive access to the underlying Serial IO protocol, and it can configure it whichever way it sees suitable. (Same as when a BlockIO or other driver sits atop of a PciIo instance -- we don't care what state the underlying PciIo instance might be at that point; we'll just set it up the way we need it for implementing BlockIo on top.) > I have offline talked with Star and have understood the intention of the > change. > How about we create a new PCD PcdUartDefaultReceiveFifoDepth and change the > SerialDxe > driver to use the PCD value in its entrypoint and Reset() function? So that > any platform can > override the default PCD in platform DSC. Sure, technically that could work. But if you look at the UEFI specification, the SerialIO protocol instance, produced by SerialDxe with such a PCD override, would not conform to the spec. (Similarly to the case when you change the already existing PCDs from their default values -- for example, if you change the baud rate PCD from 115200, then the resultant SerialIO protocol instance will *also* not conform.) > > Through this, we also limit the change to SerialDxe driver. After all > Terminal driver can start > above any SerialIo instance, not just the one created by the > MdeModulePkg/Universal/SerialDxe. Yes, I fully agree that TerminalDxe should be able to work with any SerialIO protocol instance, regardless of what UART driver produced it. However, the consequence that I draw from this requirement is different from yours. My argument goes, if TerminalDxe cannot (and it should not) assume anything particular about the underlying SerialIO protocol instance, then what it *should* assume, is *only* what the UEFI spec dictates about the SerialIO protocol defaults. Those defaults include "receive FIFO depth = 1", at least right after Reset(). Since TerminalDxe knows for sure that "receive FIFO depth = 1" will not be appropriate for parsing escape sequences, it can be argued that TerminalDxe simply cannot avoid setting the receive FIFO depth to something more suitable than the spec-mandated default. Anyway, I don't insist. The only reason I chimed in was that the UEFI spec explicitly regulates this question, as far as I could see, so we should try to conform in the reference implementation. But, whatever works. Feel free to revert my patches, or restore those bits as part of another solution. Thanks Laszlo > > Regards, > Ray > > -----Original Message----- > From: edk2-devel [mailto:[email protected]] On Behalf Of Laszlo > Ersek > Sent: Tuesday, January 19, 2016 9:54 PM > To: [email protected] > Cc: Ryan Harkin <[email protected]>; Zeng, Star <[email protected]>; > Leif Lindholm <[email protected]>; Ard Biesheuvel > <[email protected]> > Subject: [edk2] [PATCH 3/3] MdeModulePkg: TerminalDxe: select the UART's > default receive FIFO depth > > The Serial IO protocol instances provided by SerialDxe and consumed by > TerminalDxe come with a Mode.ReceiveFifoDepth=1 default setting, as > required by UEFI 2.5. > > Although TerminalDxe calls EFI_SERIAL_IO_PROTOCOL.SetAttributes() in the > TerminalDriverBindingStart() and TerminalConInTimerHandler() functions, it > only does so to change the Mode.Timeout member. Other members of Mode, > including Mode.ReceiveFifoDepth, are preserved. > > On some platforms this causes the UART that underlies TerminalDxe not to > have enough room for bursts of scan codes, which translates to broken > parsing of escape sequences, e.g. cursor movement keys. > > According to the UEFI spec, passing ReceiveFifoDepth=0 to > EFI_SERIAL_IO_PROTOCOL.SetAttributes() "will use the device's default FIFO > depth". While TerminalDxe could try to configure a receive FIFO depth that > matches the longest escape sequence it wishes to parse, in practice the > device-specific default FIFO depth -- which may well differ from the > spec-mandated SerialIo->Mode.ReceiveFifoDepth=1 default -- seems to work. > Hence let's just set that. > > This issue was exposed by SVN r18971 / git commit 921e987b2b > ("ArmPlatformPkg: Use SerialDxe in MdeModulePkg instead of EmbeddedPkg"). > In that conversion, MdeModulePkg's SerialDxe started to initialize > Mode.ReceiveFifoDepth to 1 (in conformance with the spec), unlike the > prior, non-conformant initialization to 0 in EmbeddedPkg's SerialDxe. > > Since TerminalDxe would never change ReceiveFifoDepth from the new default > value 1, and the ArmPlatformPkg/Drivers/PL011Uart library instance, > underlying SerialDxe through SerialPortLib, would obey it too, they would > collectively effect a receive queue depth of 1, rather than the default 16 > or 32. This broke cursor keys on the ARM FVP and Juno platforms. > > It is the client of EFI_SERIAL_IO_PROTOCOL that is responsible for > modifying the attributes, if the defaults are not appropriate, hence this > patch modifies TerminalDxe. > > Cc: Ard Biesheuvel <[email protected]> > Cc: Ryan Harkin <[email protected]> > Cc: Leif Lindholm <[email protected]> > Cc: Star Zeng <[email protected]> > Reported-by: Ryan Harkin <[email protected]> > Reference: http://thread.gmane.org/gmane.comp.bios.edk2.devel/4779/focus=6553 > Reference: http://thread.gmane.org/gmane.comp.bios.edk2.devel/6594 > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Laszlo Ersek <[email protected]> > --- > MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c | 2 +- > MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c > b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c > index f6cee4e..8238a93 100644 > --- a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c > +++ b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c > @@ -841,7 +841,7 @@ TerminalDriverBindingStart ( > Status = TerminalDevice->SerialIo->SetAttributes ( > TerminalDevice->SerialIo, > Mode->BaudRate, > - Mode->ReceiveFifoDepth, > + 0, // the device's default FIFO depth > (UINT32) SerialInTimeOut, > (EFI_PARITY_TYPE) (Mode->Parity), > (UINT8) Mode->DataBits, > diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c > b/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c > index 3be877b..2215df6 100644 > --- a/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c > +++ b/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c > @@ -547,7 +547,7 @@ TerminalConInTimerHandler ( > Status = SerialIo->SetAttributes ( > SerialIo, > Mode->BaudRate, > - Mode->ReceiveFifoDepth, > + 0, // the device's default FIFO depth > (UINT32) SerialInTimeOut, > (EFI_PARITY_TYPE) (Mode->Parity), > (UINT8) Mode->DataBits, > _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
