On 15 January 2016 at 18:32, Laszlo Ersek <ler...@redhat.com> wrote: > On 01/15/16 18:59, Ryan Harkin wrote: >> On 15 January 2016 at 17:39, Laszlo Ersek <ler...@redhat.com> wrote: >>> On 01/15/16 18:33, Ryan Harkin wrote: >>>> On 15 January 2016 at 17:05, Laszlo Ersek <ler...@redhat.com> wrote: > >>>>> Ryan, can you please test the following patch? >>>>> >>>>>> diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c >>>>>> b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c >>>>>> index 6fde3b2..e1a6527 100644 >>>>>> --- a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c >>>>>> +++ b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c >>>>>> @@ -806,7 +806,7 @@ TerminalDriverBindingStart ( >>>>>> Status = TerminalDevice->SerialIo->SetAttributes ( >>>>>> TerminalDevice->SerialIo, >>>>>> Mode->BaudRate, >>>>>> - Mode->ReceiveFifoDepth, >>>>>> + 0, // 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..ffdc8f2 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, // device's default FIFO depth. >>>>>> (UINT32) SerialInTimeOut, >>>>>> (EFI_PARITY_TYPE) (Mode->Parity), >>>>>> (UINT8) Mode->DataBits, >>>>> >>>> >>>> Yes, this works for me. >>> >>> Thanks for the testing (to Ard too)! >>> >>>> So what do you think the proper solution is? Is Ard's patch not what >>>> you were thinking? >>> >>> I think the UEFI spec is clear enough on EFI_SERIAL_IO_PROTOCOL that we >>> can derive what the right thing is: >>> >>> - SerialDxe needs a fix so that not only its ReceiveFifoDepth defaults >>> are correct (beause they are correct now!), but also the Timeout defaults. >>> >>> - TerminalDxe needs a fix too: it should recognize the defaults set by >>> SerialDxe (as per UEFI spec requirements), and set ReceiveFifoDepth >>> explicitly, with a SetAttributes() call. A good value for that looks >>> like zero (= device's default FIFO depth). I believe the value zero also >>> happens to restore the previous behavior, in practice -- it's just not >>> SerialDxe's responsibility. >>> >> >> The problem I have is that is someone decides that TerminalDxe needs >> to set the depth to 1 again, or some other value, the ARM platforms >> will break, because they clearly can set that value, but it isn't >> useful/usable. So perhaps PL011 needs to use the device's default >> value, irrespective of what TerminalDxe asks for? > > "irrespective" would be a violation of the UEFI spec (end to end);
What if the spec changes and the device cannot support the spec? Surely it has to do something sane? > the > Serial IO Protocol implementation must not set a larger queue depth than > what the client requests. If the client requests 1, the driver must not > set 16 (for example). > Even if it cannot support the value requested? > In my reading, the Serial IO Protocol spec is reasonably flexible, and > considers the case when a given serial port cannot provide the exact > queue depth requested. I think if the SerialDxe driver sticks to the > UEFI spec requirements, and the TerminalDxe driver also assumes nothing > more and nothing less (about the Serial IO Protocol) than specified, > then things should "just work". > > Your concern applies to all drivers and libraries under MdeModulePkg -- > some of the central modules have the potential to break any and all > platforms. I don't think we should try to work around that in > platform-specific libraries that the central modules delegate some work > to, especially if the UEFI spec goes into some detail on the relevant > behavior. > > Anyway, I wouldn't like to force my opinion on others -- I just thought > it was an interesting problem, with the solution coming more or less > directly from the spec. Admittedly, there are places where edk2 deviates > from the spec. At the moment I think it wouldn't be justified here. > To be fair, I don't know why I'm getting involved. I'd simply like to rebase to the tip of Tianocore and for it to actually work. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
