在 8/14/2018 11:48 PM, Leif Lindholm 写道: > On Tue, Aug 14, 2018 at 10:38:14AM +0800, Ming wrote: >> >> >> 在 8/9/2018 6:19 PM, Leif Lindholm 写道: >>> On Thu, Aug 09, 2018 at 02:16:49PM +0800, Ming wrote: >>>>>> +UINT16 crc_tab[256] = { >>>>> >>>>> CrcTable. >>>> >>>> Modify it in v2. >>>> >>>>> Hmm. >>>>> This might be useful to add to a core library/driver/protocol. We >>>>> don't appear to have it yet. This is another note to the universe. >>>> >>>> Do you suggest to move the CRC16 function to >>>> MdePkg/Library/BaseLib/CheckSum.c ? >>>> I think it's not enouth time to do this before Linaro 18.08 maybe. >>> >>> Not needed for 18.08, but I would like to see the improvement made >>> afterwards. >>> >>>>>> +EFI_STATUS >>>>>> +EFIAPI >>>>>> +OemGetMac ( >>>>>> + IN OUT EFI_MAC_ADDRESS *Mac, >>>>>> + IN UINTN Port >>>>>> + ) >>>>>> +{ >>>>>> + EFI_STATUS Status; >>>>>> + >>>>>> + if (NULL == Mac) { >>>>> >>>>> No jeopardy tests. Turn the comparison around to the logical order. >>>>> >>>>>> + DEBUG ((DEBUG_ERROR, "[%a]:[%dL] Mac buffer is null!\n", >>>>>> + __FUNCTION__, __LINE__)); >>>>>> + return EFI_INVALID_PARAMETER; >>>>>> + } >>>>>> + >>>>>> + Status = OemGetMacE2prom (Port, Mac->Addr); >>>>>> + if ((EFI_ERROR (Status))) { >>>>> >>>>> Parantheses around EFI_ERROR are not needed. >>>>> >>>>>> + DEBUG ((DEBUG_ERROR, >>>>>> + "[%a]:[%dL] Cannot get MAC from EEPROM, Status: %r; using default >>>>>> MAC.\n", >>>>>> + __FUNCTION__, __LINE__, Status)); >>>>>> + >>>>>> + Mac->Addr[0] = 0x00; >>>>>> + Mac->Addr[1] = 0x18; >>>>>> + Mac->Addr[2] = 0x82; >>>>>> + Mac->Addr[3] = 0x2F; >>>>>> + Mac->Addr[4] = 0x02; >>>>>> + Mac->Addr[5] = Port; >>>>> >>>>> I'm not super happy about this. This would wreak havoc on any real >>>>> network. >>>>> Arguably, a server platform should just fail hard at this point. >>>>> I would certainly appreciate a Pcd making that an option. >>>>> >>>>> Otherwise, some sort of proper scheme would need to be implemented: >>>>> using the 'locally administered range' of MAC addresses, and ensuring >>>>> addresses are only allocated after checking for possible duplicates on >>>>> the network. >>>> >>>> Do you suggest we should return EFI_NOT_FOUND here? >>> >>> Yes please. >>> It would be good if we could have some (common) code to handle the >>> fluke situation where you end up without your own MAC address. >>> (So that the node can boot up and report that it is broken.) >>> But it needs to be done in a reliable way, and that's too big a task >>> for 18.08. >> >> I found some modules which invoke OemGetMac() don't judge the Status of >> OemGetMac, so it may cause some issue now if changing to EFI_NOT_FOUND. >> How about change it while we handle the fluke situation after 18.08 ? > > We cannot release 18.08 with known bugs. > And not checking return value is a bug. > > I presume you mean that these calling functions are inside HwPkg?
Yes. All D06 board will burn a Mac to eeprom before delivery and there is a command (SetMac) to write a Mac. For handling the fluke situation, we think there are several ways: 1 Initialize Mac to 0xFF; Kernel seems will create a random Mac while the Mac is 0xFF. 2 Make a Mac from ArmReadCntPct() and gTR->GetTime(); 3 Make a locally administered Mac from ArmReadCntPct() and gTR->GetTime(); The 2nd is the way our product project use to handle the fluke situation. What is your suggestion? > > The alternative would be to ensure the function does not return. > But I would not recommend that. > > / > Leif > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel