On 01/15/16 18:25, Carsey, Jaben wrote: > >> -----Original Message----- >> From: edk2-devel [mailto:[email protected]] On Behalf Of >> Laszlo Ersek >> Sent: Friday, January 15, 2016 9:22 AM >> To: Ryan Harkin <[email protected]>; Ard Biesheuvel >> <[email protected]> >> Cc: [email protected]; Zeng, Star <[email protected]>; Leif >> Lindholm <[email protected]> >> Subject: Re: [edk2] [PATCH] MdeModulePkg/SerialDxe: fix porting error from >> EmbeddedPkg >> >> On 01/15/16 17:07, Ryan Harkin wrote: >>> On 15 January 2016 at 15:42, Ard Biesheuvel <[email protected]> >> wrote: >>>> SerialDxe was migrated to MdeModulePkg from EmbeddedPkg, and all >>>> users of the latter were moved to the former. However, the new >>>> version is not quite identical to the original, in ways that break >>>> ARM platforms that use the PL011 driver. >>>> >>> >>> The original patch should never have passed review. >> >> I agree it's unfortunate that the patch silently changed behavior in the >> course of the conversion. Stuff like that is very hard to catch in >> review; in my analysis that I posted a few minutes ago, I actually tried >> to diff the two drivers against each other, and gave up. I had to review >> both files in separate windows at the same time. >> >> Ard is right -- extreme discipline is required when converting things. >> If the programmer notices a bug or non-conformance in the course of the >> conversion, he should make a note somewhere, for later, but the >> conversion *must* preserve the original bugs! Bugfixing is a separate >> step. It can occur after conversion, as a separate patch. It can even >> occur *before* the conversion (and then the corrected code will be >> converted.) >> > Agree. > > Isn't there a SVN->Move operation (and a GIT version) that should have been > used and would inherently have maintained any and all behaviors? Then if > there are name changes required for PCDs or the like they can be changed > after the move in the patch series? > > We researched better move methods than "add then delete" after the move of > libraries from ShellPkg to MdeModulePkg. Another major benefit was continuity > of file history...
I usually follow the steps below, if I can: - First create a copy of the module that I'm going to change. In this patch files are only copied, and the only thing changed is the FILE_GUID in the .INF file. This patch can be then formatted with the --find-copies-harder flag passed to git-format-patch. That will create an extremely concise patch; it will point out what comes whence, and generate only the relative differences (= the FILE_GUID). - In the next patch I implement minimal changes that allow the copy to be built and used in place of the original. (With equivalent behavior.) - Do the conversion (still with equivalent behavior). - Fix bugs found. The first patch (formatted with --find-copies-harder) should eliminate the bulk of the cruft. Thanks Laszlo > > -Jaben > >>> >>> >>>> In SerialReset(), the serial port is reset to its default values, >>>> but the defaults used by the new version for ReceiveFifoDepth and >>>> Timeout deviate from the original values. So put them back. >>>> >>>> Contributed-under: TianoCore Contribution Agreement 1.0 >>>> Signed-off-by: Ard Biesheuvel <[email protected]> >>> Tested-by: Ryan Harkin <[email protected]> >> >> Can you please test my proposal as well? >> >> Thanks >> Laszlo >> >> >>> >>>> --- >>>> MdeModulePkg/Universal/SerialDxe/SerialIo.c | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/MdeModulePkg/Universal/SerialDxe/SerialIo.c >> b/MdeModulePkg/Universal/SerialDxe/SerialIo.c >>>> index de928d1719e9..9e9db28ce5cc 100644 >>>> --- a/MdeModulePkg/Universal/SerialDxe/SerialIo.c >>>> +++ b/MdeModulePkg/Universal/SerialDxe/SerialIo.c >>>> @@ -233,8 +233,8 @@ SerialReset ( >>>> // >>>> // Set the Serial I/O mode >>>> // >>>> - This->Mode->ReceiveFifoDepth = 1; >>>> - This->Mode->Timeout = 0; >>>> + This->Mode->ReceiveFifoDepth = 0; >>>> + This->Mode->Timeout = 1000000; >>>> This->Mode->BaudRate = PcdGet64 (PcdUartDefaultBaudRate); >>>> This->Mode->DataBits = (UINT32) PcdGet8 >>>> (PcdUartDefaultDataBits); >>>> This->Mode->Parity = (UINT32) PcdGet8 (PcdUartDefaultParity); >>>> -- >>>> 2.5.0 >>>> >>> _______________________________________________ >>> edk2-devel mailing list >>> [email protected] >>> https://lists.01.org/mailman/listinfo/edk2-devel >>> >> >> _______________________________________________ >> edk2-devel mailing list >> [email protected] >> https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
