On 07/25/18 14:09, Dong, Eric wrote: > Hi Laszlo, > > >> -----Original Message----- >> From: Laszlo Ersek [mailto:ler...@redhat.com] >> Sent: Wednesday, July 25, 2018 7:47 PM >> To: Dong, Eric <eric.d...@intel.com>; edk2-devel@lists.01.org >> Cc: Ni, Ruiyu <ruiyu...@intel.com> >> Subject: Re: [edk2] [Patch v3 2/3] UefiCpuPkg/MpInitLib: Remove StartCount >> and volatile definition. >> >> Hi Eric, >> >> On 07/25/18 09:50, Eric Dong wrote: >>> The StartCount is duplicated with RunningCount, replace it with >>> RunningCount. Also the volatile for RunningCount is not needed. >> >> after staring at this patch for a long time, I think it is correct. >> However, I suggest that we improve the commit message, because this patch >> actually does three things: >> >> (1) It removes "volatile" from RunningCount. >> >> [This is OK, because only the BSP modifies it.] >> >> (2) [This is the tricky part.] >> >> When we detect a timeout in CheckAllAPs(), and collect the list of failed >> CPUs, >> the size of the list is derived from the following difference, before the >> patch: >> >> StartCount - FinishedCount >> >> where "StartCount" is set by the BSP at startup, and FinishedCount is >> incremented by the APs themselves. >> > > I think FinishedCount used here is a typo. What the logic from the code > context here is want to get the failed Aps and return them. So it should use > RunningCount here, right? Also the FinishedCount may be bigger than > StartCount if many Aps has been disabled. Right?
I agree FinishedCount looks questionable / out of place in the original code. Thanks Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
