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

[Leif Lindholm <leif.lindholm@linaro.org>]

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.

/
    Leif