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?
The alternative would be to ensure the function does not return.
But I would not recommend that.
/
Leif
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
