[AMD Official Use Only - General] Mike, your comments will be fixed in V3 9/9. Thanks for catching this.
Abner > -----Original Message----- > From: Mike Maslenkin <mike.maslen...@gmail.com> > Sent: Monday, November 27, 2023 3:38 AM > To: Chang, Abner <abner.ch...@amd.com> > Cc: devel@edk2.groups.io; Nickle Wang <nick...@nvidia.com>; Igor > Kulchytskyy <ig...@ami.com> > Subject: Re: [PATCH V2 1/8] RedfishPkg/BmcUsbNicLib: Update BMC USB NIC > searching algorithm > > Caution: This message originated from an External Source. Use proper caution > when opening attachments, clicking links, or responding. > > > 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/PlatformHostInter > faceBmcUsbNicLib.c > b/RedfishPkg/Library/PlatformHostInterfaceBmcUsbNicLib/PlatformHostInter > faceBmcUsbNicLib.c > > index 95900579118..e5bf70cfd58 100644 > > --- > a/RedfishPkg/Library/PlatformHostInterfaceBmcUsbNicLib/PlatformHostInter > faceBmcUsbNicLib.c > > +++ > b/RedfishPkg/Library/PlatformHostInterfaceBmcUsbNicLib/PlatformHostInter > faceBmcUsbNicLib.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 (#111713): https://edk2.groups.io/g/devel/message/111713 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] -=-=-=-=-=-=-=-=-=-=-=-
