Re: [PATCH edk2-platforms v1 22/38] Platform/Hisilicon/D06: Add OemNicLib

[Ming <ming.huang@linaro.org>]

在 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
>