Hi Igor
please find my comments below.
On Tue, Oct 31, 2023 at 8:56 PM Igor Kulchytskyy via groups.io
<igork=ami....@groups.io> wrote:
>
> 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: Igor Kulchytskyy <ig...@ami.com>
> Signed-off-by: Igor Kulchytskyy <ig...@ami.com>
> ---
> RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c | 96 ++++++++++++++++++--
> 1 file changed, 89 insertions(+), 7 deletions(-)
>
> diff --git a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> index 23da3b968f..a88ad55938 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;
> + }
> +
I also have these two hunks in my pending list.
But I suggest to add ASSERT to GetTargetNetworkInterfaceInternal, just
because currently it is really impossible situation,
and mEfiRedfishDiscoverNetworkInterface was checked before in scope of
ValidateTargetNetworkInterface().
Also I wonder why check patch doesn't complain about missed spaces in
"IsListEmpty (&mEfiRedfishDiscoverNetworkInterface)" call for example.
> ThisNetworkInterface = (EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_INTERNAL
> *)GetFirstNode (&mEfiRedfishDiscoverNetworkInterface);
> while (TRUE) {
> if (ThisNetworkInterface->OpenDriverControllerHandle ==
> ControllerHandle) {
> @@ -476,6 +486,38 @@ CheckIsIpVersion6 (
> return FALSE;
> }
>
> +/**
> + This function returns the IP type supported by the Host Interface
> +
> + @retval IP Type
> + // Unknown=00h,
> + // Ipv4=01h,
> + // Ipv6=02h,
> +
> +**/
> +UINT8
STATIC ?
> +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 0;
In this driver REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_IP{4,6}
used and accessible.
so, 0 means REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_UNKNOWN
And these values actually checked in scope of BuildupNetworkInterface() below.
> + }
> + }
> + Status = RedfishGetHostInterfaceProtocolData (mSmbios, &DeviceDescriptor,
> &Data); // Search for SMBIOS type 42h
> + if (!EFI_ERROR (Status) && (Data != NULL) &&
> + (Data->HostIpAssignmentType == RedfishHostIpAssignmentStatic)) {
> + return Data->HostIpAddressFormat;
> + }
> + return 0;
Same here, 0 is a magic value for
REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_UNKNOWN
> +}
> +
> /**
> This function discover Redfish service through SMBIOS host interface.
>
> @@ -512,6 +554,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"));
Missed argument for %a format
Missed space "DEBUG (("
> + 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"));
Missed argument for %a format
Missed space "DEBUG (("
> + 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 +1153,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 +1193,16 @@ RedfishServiceGetNetworkInterface (
>
> ThisNetworkInterfaceIntn =
> (EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_INTERNAL *)GetFirstNode
> (&mEfiRedfishDiscoverNetworkInterface);
> while (TRUE) {
> + // If Get Subnet Info 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;
> @@ -1260,7 +1322,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 +1602,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 +1624,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++;
> }
It may be SuccessfulCount must have one indentation less, i.e in scope
of upper "if (!EFI_ERROR (Status)) {", not in scope of "if (EFI_ERROR
(Status)) {" in context.
> }
> }
> + 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 +1668,24 @@ BuildupNetworkInterface (
> EFI_REDFISH_DISCOVER_REST_EX_INSTANCE_INTERNAL *RestExInstance;
> EFI_TPL OldTpl;
> BOOLEAN
> NewNetworkInterfaceInstalled;
> + UINT8 IpType;
>
> NewNetworkInterfaceInstalled = FALSE;
> Index = 0;
> +
> +
> + // Get IP Type to filter out unnecessary network protocol if possible
> + IpType = GetHiIpProtocolType();
> +
> do {
> + // Check IP Type and skip an unnecessary network protocol if does not
> match
> + if (IpType != 0) {
IpType is invariant to this loop.
Eiher we may check it's value before the loop or we can just drop it,
because "&& IpType ==
REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_IP{4,6}"
is equal to "IpType != 0" Do I miss something?
> + 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)) {
> + Index++;
Ops. You can not do just { Index++;continue }
You need
{
Index++;
if (Index == (sizeof (gRequiredProtocol) / sizeof
(REDFISH_DISCOVER_REQUIRED_PROTOCOL))) {
break;
}
continue;
}
It's a terrible function. I have a patch, that improves this loop:
https://github.com/ghbaccount/edk2/commit/adc3218d3d96083c85cef6c396dde8ddb96530fb
> + continue;
> + }
> + }
> Status = gBS->OpenProtocol (
> // Already in list?
> ControllerHandle,
> --
> 2.37.1.windows.1
Regards,
Mike.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110458): https://edk2.groups.io/g/devel/message/110458
Mute This Topic: https://groups.io/mt/102303105/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
