This change looks good to me. Just minor comments: > DEBUG ((DEBUG_INFO, "%a: all required protocols are found on this controller > handle: %p.\n", __func__, ControllerHandle));
Could we use DEBUG_MANAGEABILITY to replace DEBUG_INFO in Redfish driver? This gives us the way to show manageability information only while debugging. And it looks like we don't run uncrustify to this patch. May I know if it is possible to create a PR on edk2 repository and edk2 CI can test it? Edk2 CI helps me to catch some formatting issue that I am not aware of. Thanks, Nickle > -----Original Message----- > From: Igor Kulchytskyy <ig...@ami.com> > Sent: Thursday, November 2, 2023 4:18 AM > To: devel@edk2.groups.io > Cc: Abner Chang <abner.ch...@amd.com>; Nickle Wang <nick...@nvidia.com>; > Mike Maslenkin <mike.maslen...@gmail.com> > Subject: [PATCH v2] RedfishPkg: RedfishDiscoverDxe: Fix issue if IPv4 > installed > after RestEx > > External email: Use caution opening links or attachments > > > Supported function of the driver changed to wait for all newtwork > interface to be installed. > Filer out the network interfaces which are not supported by > Redfish Host Interface. > > Cc: Abner Chang <abner.ch...@amd.com> > Cc: Nickle Wang <nick...@nvidia.com> > Cc: Mike Maslenkin <mike.maslen...@gmail.com> > Signed-off-by: Igor Kulchytskyy <ig...@ami.com> > --- > RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c | 131 ++++++++++++++--- > --- > 1 file changed, 95 insertions(+), 36 deletions(-) > > diff --git a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c > b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c > index 23da3b968f..c67b8acf12 100644 > --- a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c > +++ b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c > @@ -322,9 +322,15 @@ GetTargetNetworkInterfaceInternal ( > { > EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_INTERNAL > *ThisNetworkInterface; > > + if (IsListEmpty (&mEfiRedfishDiscoverNetworkInterface)) { > + return NULL; > + } > + > ThisNetworkInterface = > (EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_INTERNAL *)GetFirstNode > (&mEfiRedfishDiscoverNetworkInterface); > while (TRUE) { > - if (CompareMem ((VOID *)&ThisNetworkInterface->MacAddress, > &TargetNetworkInterface->MacAddress, ThisNetworkInterface->HwAddressSize) > == 0) { > + if (CompareMem ((VOID *)&ThisNetworkInterface->MacAddress, > &TargetNetworkInterface->MacAddress, ThisNetworkInterface->HwAddressSize) > == 0 && > + ((TargetNetworkInterface->IsIpv6 && ThisNetworkInterface- > >NetworkProtocolType == ProtocolTypeTcp6) || > + (!TargetNetworkInterface->IsIpv6 && ThisNetworkInterface- > >NetworkProtocolType == ProtocolTypeTcp4))) { > return ThisNetworkInterface; > } > > @@ -354,6 +360,10 @@ GetTargetNetworkInterfaceInternalByController ( > { > EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_INTERNAL > *ThisNetworkInterface; > > + if (IsListEmpty (&mEfiRedfishDiscoverNetworkInterface)) { > + return NULL; > + } > + > ThisNetworkInterface = > (EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_INTERNAL *)GetFirstNode > (&mEfiRedfishDiscoverNetworkInterface); > while (TRUE) { > if (ThisNetworkInterface->OpenDriverControllerHandle == > ControllerHandle) { > @@ -476,6 +486,39 @@ CheckIsIpVersion6 ( > return FALSE; > } > > +/** > + This function returns the IP type supported by the Host Interface > + > + @retval IP Type > + // Unknown=00h, > + // Ipv4=01h, > + // Ipv6=02h, > + > +**/ > +STATIC > +UINT8 > +GetHiIpProtocolType() > +{ > + EFI_STATUS Status; > + REDFISH_OVER_IP_PROTOCOL_DATA *Data; > + REDFISH_INTERFACE_DATA *DeviceDescriptor; > + > + Data = NULL; > + DeviceDescriptor = NULL; > + if (mSmbios == NULL) { > + Status = gBS->LocateProtocol (&gEfiSmbiosProtocolGuid, NULL, (VOID > **)&mSmbios); > + if (EFI_ERROR (Status)) { > + return > REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_UNKNOWN; > + } > + } > + Status = RedfishGetHostInterfaceProtocolData (mSmbios, &DeviceDescriptor, > &Data); // Search for SMBIOS type 42h > + if (!EFI_ERROR (Status) && (Data != NULL) && > + (Data->HostIpAssignmentType == RedfishHostIpAssignmentStatic)) { > + return Data->HostIpAddressFormat; > + } > + return REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_UNKNOWN; > +} > + > /** > This function discover Redfish service through SMBIOS host interface. > > @@ -512,6 +555,15 @@ DiscoverRedfishHostInterface ( > > Status = RedfishGetHostInterfaceProtocolData (mSmbios, &DeviceDescriptor, > &Data); // Search for SMBIOS type 42h > if (!EFI_ERROR (Status) && (Data != NULL) && (DeviceDescriptor != NULL)) { > + > + if (Instance->NetworkInterface->NetworkProtocolType == ProtocolTypeTcp4 > && Data->HostIpAddressFormat != > REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_IP4) { // IPv4 case > + DEBUG ((DEBUG_ERROR, "%a: Network Interface is IPv4, but Host Interface > requires Ipv6\n", __func__)); > + return EFI_UNSUPPORTED; > + } > + else if (Instance->NetworkInterface->NetworkProtocolType == > ProtocolTypeTcp6 && Data->HostIpAddressFormat != > REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_IP6) { // IPv6 case > + DEBUG ((DEBUG_ERROR, "%a: Network Interface is IPv6, but Host Interface > requires IPv4\n", __func__)); > + return EFI_UNSUPPORTED; > + } > // > // Check if we can reach out Redfish service using this network > interface. > // Check with MAC address using Device Descriptor Data Device Type 04 and > Type 05. > @@ -1102,6 +1154,7 @@ RedfishServiceGetNetworkInterface ( > OUT EFI_REDFISH_DISCOVER_NETWORK_INTERFACE **NetworkIntfInstances > ) > { > + EFI_STATUS Status; > EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_INTERNAL > *ThisNetworkInterfaceIntn; > EFI_REDFISH_DISCOVER_NETWORK_INTERFACE *ThisNetworkInterface; > EFI_REDFISH_DISCOVER_REST_EX_INSTANCE_INTERNAL *RestExInstance; > @@ -1141,6 +1194,16 @@ RedfishServiceGetNetworkInterface ( > > ThisNetworkInterfaceIntn = > (EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_INTERNAL *)GetFirstNode > (&mEfiRedfishDiscoverNetworkInterface); > while (TRUE) { > + // If Get Subnet Info failed then skip this interface > + Status = NetworkInterfaceGetSubnetInfo (ThisNetworkInterfaceIntn, > ImageHandle); // Get subnet info > + if (EFI_ERROR(Status)) { > + if (IsNodeAtEnd (&mEfiRedfishDiscoverNetworkInterface, > &ThisNetworkInterfaceIntn->Entry)) { > + break; > + } > + ThisNetworkInterfaceIntn = > (EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_INTERNAL *)GetNextNode > (&mEfiRedfishDiscoverNetworkInterface, &ThisNetworkInterfaceIntn->Entry); > + continue; > + } > + > ThisNetworkInterface->IsIpv6 = FALSE; > if (CheckIsIpVersion6 (ThisNetworkInterfaceIntn)) { > ThisNetworkInterface->IsIpv6 = TRUE; > @@ -1230,6 +1293,10 @@ RedfishServiceAcquireService ( > > if (TargetNetworkInterface != NULL) { > TargetNetworkInterfaceInternal = GetTargetNetworkInterfaceInternal > (TargetNetworkInterface); > + if (TargetNetworkInterfaceInternal == NULL) { > + DEBUG ((DEBUG_ERROR, "%a:No network interface on platform.\n", > __func__)); > + return EFI_UNSUPPORTED; > + } > NumNetworkInterfaces = 1; > } else { > TargetNetworkInterfaceInternal = > (EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_INTERNAL *)GetFirstNode > (&mEfiRedfishDiscoverNetworkInterface); > @@ -1260,7 +1327,12 @@ RedfishServiceAcquireService ( > // Get subnet information in case subnet information is not set because > // RedfishServiceGetNetworkInterfaces hasn't been called yet. > // > - NetworkInterfaceGetSubnetInfo (TargetNetworkInterfaceInternal, > ImageHandle); > + Status1 = NetworkInterfaceGetSubnetInfo > (TargetNetworkInterfaceInternal, > ImageHandle); > + if (EFI_ERROR(Status1)) { > + DEBUG ((DEBUG_ERROR, "%a: Get subnet information fail.\n", > __func__)); > + FreePool(Instance); > + continue; > + } > NewInstance = TRUE; > } > > @@ -1535,7 +1607,7 @@ TestForRequiredProtocols ( > UINT32 *Id; > UINTN Index; > EFI_STATUS Status; > - UINTN ListCount; > + UINTN ListCount, SuccessfulCount = 0; > > ListCount = (sizeof (gRequiredProtocol) / sizeof > (REDFISH_DISCOVER_REQUIRED_PROTOCOL)); > for (Index = 0; Index < ListCount; Index++) { > @@ -1557,13 +1629,14 @@ TestForRequiredProtocols ( > EFI_OPEN_PROTOCOL_GET_PROTOCOL > ); > if (EFI_ERROR (Status)) { > - if (Index == ListCount - 1) { > - DEBUG ((DEBUG_INFO, "%a: all required protocols are found on this > controller handle: %p.\n", __func__, ControllerHandle)); > - return EFI_SUCCESS; > - } > + SuccessfulCount++; > } > } > } > + if (ListCount == SuccessfulCount) { > + DEBUG ((DEBUG_INFO, "%a: all required protocols are found on this > controller > handle: %p.\n", __func__, ControllerHandle)); > + return EFI_SUCCESS; > + } > > return EFI_UNSUPPORTED; > } > @@ -1600,10 +1673,23 @@ BuildupNetworkInterface ( > EFI_REDFISH_DISCOVER_REST_EX_INSTANCE_INTERNAL *RestExInstance; > EFI_TPL OldTpl; > BOOLEAN > NewNetworkInterfaceInstalled; > + UINT8 IpType; > + UINTN ListCount; > > + ListCount = (sizeof (gRequiredProtocol) / sizeof > (REDFISH_DISCOVER_REQUIRED_PROTOCOL)); > NewNetworkInterfaceInstalled = FALSE; > Index = 0; > - do { > + > + > + // Get IP Type to filter out unnecessary network protocol if possible > + IpType = GetHiIpProtocolType(); > + > + for (Index = 0; Index < ListCount; Index++) { > + // Check IP Type and skip an unnecessary network protocol if does not > match > + if ((gRequiredProtocol[Index].ProtocolType == ProtocolTypeTcp4 && IpType > == > REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_IP6) || > + (gRequiredProtocol[Index].ProtocolType == ProtocolTypeTcp6 && IpType > == > REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_IP4)) { > + continue; > + } > Status = gBS->OpenProtocol ( > // Already in list? > ControllerHandle, > @@ -1614,11 +1700,6 @@ BuildupNetworkInterface ( > EFI_OPEN_PROTOCOL_GET_PROTOCOL > ); > if (!EFI_ERROR (Status)) { > - Index++; > - if (Index == (sizeof (gRequiredProtocol) / sizeof > (REDFISH_DISCOVER_REQUIRED_PROTOCOL))) { > - break; > - } > - > continue; > } > > @@ -1631,11 +1712,6 @@ BuildupNetworkInterface ( > EFI_OPEN_PROTOCOL_GET_PROTOCOL > ); > if (EFI_ERROR (Status)) { > - Index++; > - if (Index == (sizeof (gRequiredProtocol) / sizeof > (REDFISH_DISCOVER_REQUIRED_PROTOCOL))) { > - break; > - } > - > continue; > } > > @@ -1694,11 +1770,6 @@ BuildupNetworkInterface ( > ProtocolDiscoverIdPtr > ); > if (EFI_ERROR (Status)) { > - Index++; > - if (Index == (sizeof (gRequiredProtocol) / sizeof > (REDFISH_DISCOVER_REQUIRED_PROTOCOL))) { > - break; > - } > - > continue; > } > > @@ -1755,25 +1826,13 @@ BuildupNetworkInterface ( > } > } else { > DEBUG ((DEBUG_MANAGEABILITY, "%a: Not REST EX, continue with > next\n", __func__)); > - Index++; > - if (Index == (sizeof (gRequiredProtocol) / sizeof > (REDFISH_DISCOVER_REQUIRED_PROTOCOL))) { > - break; > - } > - > continue; > } > } > > return Status; > - } else { > - Index++; > - if (Index == (sizeof (gRequiredProtocol) / sizeof > (REDFISH_DISCOVER_REQUIRED_PROTOCOL))) { > - break; > - } > - > - continue; > } > - } while (Index < (sizeof (gRequiredProtocol) / sizeof > (REDFISH_DISCOVER_REQUIRED_PROTOCOL))); > + } > > return EFI_DEVICE_ERROR; > } > -- > 2.37.1.windows.1 > -The information contained in this message may be confidential and proprietary > to American Megatrends (AMI). This communication is intended to be read only > by > the individual or entity to whom it is addressed or by their designee. If the > reader > of this message is not the intended recipient, you are on notice that any > distribution of this message, in any form, is strictly prohibited. Please > promptly > notify the sender by reply e-mail or by telephone at 770-246-8600, and then > delete or destroy all copies of the transmission. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110498): https://edk2.groups.io/g/devel/message/110498 Mute This Topic: https://groups.io/mt/102328322/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-