Hi Abner, please find my comments below.
On Thu, Nov 23, 2023 at 9:47 AM <abner.ch...@amd.com> wrote: > > From: Abner Chang <abner.ch...@amd.com> > > Update BMC USB NIC searching algorithm for IPv4 only. > > Signed-off-by: Abner Chang <abner.ch...@amd.com> > Cc: Nickle Wang <nick...@nvidia.com> > Cc: Igor Kulchytskyy <ig...@ami.com> > Cc: Mike Maslenkin <mike.maslen...@gmail.com> > --- > .../PlatformHostInterfaceBmcUsbNicLib.c | 188 ++++++++++++------ > 1 file changed, 128 insertions(+), 60 deletions(-) > > diff --git > a/RedfishPkg/Library/PlatformHostInterfaceBmcUsbNicLib/PlatformHostInterfaceBmcUsbNicLib.c > > b/RedfishPkg/Library/PlatformHostInterfaceBmcUsbNicLib/PlatformHostInterfaceBmcUsbNicLib.c > index 95900579118..e5bf70cfd58 100644 > --- > a/RedfishPkg/Library/PlatformHostInterfaceBmcUsbNicLib/PlatformHostInterfaceBmcUsbNicLib.c > +++ > b/RedfishPkg/Library/PlatformHostInterfaceBmcUsbNicLib/PlatformHostInterfaceBmcUsbNicLib.c > @@ -368,7 +368,9 @@ RetrievedBmcUsbNicInfo ( > )); > CopyMem ((VOID *)&ThisInstance->RedfishIpAddressIpv4, (VOID > *)&DestIpAddress->IpAddress, sizeof (DestIpAddress->IpAddress)); > // > - // According to UEFI spec, the IP address at BMC USB NIC host end is > the IP address at BMC end minus 1. > + // According to the design spec: > + // > https://github.com/tianocore/edk2/tree/master/RedfishPkg#platform-with-bmc-and-the-bmc-exposed-usb-network-device > + // The IP address at BMC USB NIC host end is the IP address at BMC end > minus 1. > // > CopyMem ((VOID *)&ThisInstance->HostIpAddressIpv4, (VOID > *)&DestIpAddress->IpAddress, sizeof (DestIpAddress->IpAddress)); > ThisInstance->HostIpAddressIpv4[sizeof > (ThisInstance->HostIpAddressIpv4) - 1] -= 1; > @@ -729,8 +731,10 @@ HostInterfaceIpmiCheckMacAddress ( > > // > // According to design spec in Readme file under RedfishPkg. > - // Compare the first five MAC address and > - // the 6th MAC address. > + // > https://github.com/tianocore/edk2/tree/master/RedfishPkg#platform-with-bmc-and-the-bmc-exposed-usb-network-device > + // Compare the first five elements of MAC address and the 6th element > of MAC address. > + // The 6th element of MAC address must be the 6th element of > + // IPMI channel MAC address minus 1. > // > if ((IpmiLanMacAddressSize != UsbNicInfo->MacAddressSize) || > (CompareMem ( > @@ -738,8 +742,8 @@ HostInterfaceIpmiCheckMacAddress ( > (VOID *)&IpmiLanChannelMacAddress.Addr, > IpmiLanMacAddressSize - 1 > ) != 0) || > - (IpmiLanChannelMacAddress.Addr[IpmiLanMacAddressSize - 1] != > - *(UsbNicInfo->MacAddress + IpmiLanMacAddressSize - 1) - 1) > + ((IpmiLanChannelMacAddress.Addr[IpmiLanMacAddressSize - 1] - 1) != > + *(UsbNicInfo->MacAddress + IpmiLanMacAddressSize - 1)) > ) > { > DEBUG ((DEBUG_REDFISH_HOST_INTERFACE, " MAC address is not > matched.\n")); > @@ -962,6 +966,49 @@ UsbNicSearchUsbIo ( > return EFI_NOT_FOUND; > } > > +/** > + This function identifies if the USB NIC has MAC address and internet > + protocol device path installed. (Only support IPv4) > + > + @param[in] UsbDevicePath USB device path. > + > + @retval EFI_SUCCESS Yes, this is IPv4 SNP handle > + @retval EFI_NOT_FOUND No, this is not IPv4 SNP handle > + > +**/ > +EFI_STATUS > +IdentifyNetworkMessageDevicePath ( > + IN EFI_DEVICE_PATH_PROTOCOL *UsbDevicePath > + ) > +{ > + EFI_DEVICE_PATH_PROTOCOL *DevicePath; > + > + DevicePath = UsbDevicePath; > + while (TRUE) { > + DevicePath = NextDevicePathNode (DevicePath); > + if (IsDevicePathEnd (DevicePath)) { > + DEBUG ((DEBUG_REDFISH_HOST_INTERFACE, "MAC address device path is not > found on this handle.\n")); > + break; > + } > + > + if ((DevicePath->Type == MESSAGING_DEVICE_PATH) && (DevicePath->SubType > == MSG_MAC_ADDR_DP)) { > + DevicePath = NextDevicePathNode (DevicePath); // Advance to next > device path protocol. > + if (IsDevicePathEnd (DevicePath)) { > + DEBUG ((DEBUG_REDFISH_HOST_INTERFACE, "IPv4 device path is not found > on this handle.\n")); > + break; > + } > + > + if ((DevicePath->Type == MESSAGING_DEVICE_PATH) && > (DevicePath->SubType == MSG_IPv4_DP)) { > + return EFI_SUCCESS; > + } > + > + break; > + } > + } > + > + return EFI_NOT_FOUND; > +} > + > /** > This function identifies if the USB NIC is exposed by BMC as > the host-BMC channel. > @@ -1025,7 +1072,7 @@ IdentifyUsbNicBmcChannel ( > (VOID *)&Snp->Mode->CurrentAddress, > BmcUsbNic->MacAddressSize > ); > - DEBUG ((DEBUG_REDFISH_HOST_INTERFACE, " MAC address (in size %d) for > this SNP instance:\n ", BmcUsbNic->MacAddressSize)); > + DEBUG ((DEBUG_REDFISH_HOST_INTERFACE, " MAC address (in size %d) for > this SNP instance:\n", BmcUsbNic->MacAddressSize)); > for (Index = 0; Index < BmcUsbNic->MacAddressSize; Index++) { > DEBUG ((DEBUG_REDFISH_HOST_INTERFACE, "%02x ", *(BmcUsbNic->MacAddress + > Index))); > } > @@ -1068,7 +1115,8 @@ CheckBmcUsbNicOnHandles ( > UINTN Index; > EFI_STATUS Status; > EFI_DEVICE_PATH_PROTOCOL *DevicePath; > - BOOLEAN GotOneUsbNIc; > + BOOLEAN GotBmcUsbNic; > + CHAR16 *DevicePathStr; > > if ((HandleNumer == 0) || (HandleBuffer == NULL)) { > return EFI_INVALID_PARAMETER; > @@ -1076,26 +1124,37 @@ CheckBmcUsbNicOnHandles ( > > DEBUG ((DEBUG_MANAGEABILITY, "%a: Entry, #%d SNP handle\n", __func__, > HandleNumer)); > > - GotOneUsbNIc = FALSE; > + GotBmcUsbNic = FALSE; > for (Index = 0; Index < HandleNumer; Index++) { > + DEBUG ((DEBUG_MANAGEABILITY, " Locate device path on handle > 0x%08x\n", *(HandleBuffer + Index))); > Status = gBS->HandleProtocol ( > *(HandleBuffer + Index), > &gEfiDevicePathProtocolGuid, > (VOID **)&DevicePath > ); > if (EFI_ERROR (Status)) { > - DEBUG ((DEBUG_ERROR, " Failed to locate SNP on %d handle.\n", > Index)); > + DEBUG ((DEBUG_ERROR, " Failed to locate device path on %d > handle.\n", __func__, Index)); There is no format for __func__ argument. > continue; > } > > + DevicePathStr = ConvertDevicePathToText (DevicePath, FALSE, FALSE); > + if (DevicePathStr != NULL) { > + DEBUG ((DEBUG_MANAGEABILITY, " Device path: %s\n", DevicePathStr)); > + FreePool (DevicePathStr); > + } > + > // Check if this is an BMC exposed USB NIC device. > while (TRUE) { > if ((DevicePath->Type == MESSAGING_DEVICE_PATH) && > (DevicePath->SubType == MSG_USB_DP)) { > - Status = IdentifyUsbNicBmcChannel (*(HandleBuffer + Index), > DevicePath); > + Status = IdentifyNetworkMessageDevicePath (DevicePath); > if (!EFI_ERROR (Status)) { > - GotOneUsbNIc = TRUE; > - break; > + Status = IdentifyUsbNicBmcChannel (*(HandleBuffer + Index), > DevicePath); Not related to this patch, but while you are changing this could you fix memory allocation issue in IdentifyUsbNicBmcChannel()? The problem string is: BmcUsbNic->MacAddress = AllocateZeroPool (sizeof (BmcUsbNic->MacAddressSize)); Obviously sizeof() must be removed. Also, AllocateZeroPool is not required because of the following CopyMem (BmcUsbNic->MacAddress, &Snp->Mode->CurrentAddress, BmcUsbNic->MacAddressSize); -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#111706): https://edk2.groups.io/g/devel/message/111706 Mute This Topic: https://groups.io/mt/102763117/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
