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