From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 978A3740037 for ; Wed, 1 Nov 2023 01:00:50 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=9LpwLlQX63yT6k5qglIL/uRgIKX6v/8m+zwtgm+ZfmE=; c=relaxed/simple; d=groups.io; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject:To:Cc:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type:Content-Transfer-Encoding; s=20140610; t=1698800449; v=1; b=M0sWlcUQ8FZIOu1wA9Uvc+pQG2ffKtp+sdvFKyTjqdmIKrtwSvJ8ESwddH3oIt1L6DnzC1kF 3+7fHVCyMCASocVY3FMDuWWzB+zi5LuYC4cYGCZGTHDdZWFM5rgIv6px20EVT4ExU6E0CZv2cFC T/iMU1qScaekmmvweFaWJALc= X-Received: by 127.0.0.2 with SMTP id EeCoYY7687511xiy53n2O52K; Tue, 31 Oct 2023 18:00:49 -0700 X-Received: from mail-yw1-f175.google.com (mail-yw1-f175.google.com [209.85.128.175]) by mx.groups.io with SMTP id smtpd.web10.12139.1698800448611313319 for ; Tue, 31 Oct 2023 18:00:48 -0700 X-Received: by mail-yw1-f175.google.com with SMTP id 00721157ae682-5a8ada42c2aso63440037b3.3 for ; Tue, 31 Oct 2023 18:00:48 -0700 (PDT) X-Gm-Message-State: PjMqkM94tqzMe9mTm8qcasakx7686176AA= X-Google-Smtp-Source: AGHT+IFPE+Vx6FhQ/uoa96ZsloYSeaXKGCGJtvKLnM7pm8fHYnT9s3SO4VPLAxlcMQyeBW0uHMZm8+QQnfH+ev9jkhM= X-Received: by 2002:a81:8d52:0:b0:5ad:c699:f21d with SMTP id w18-20020a818d52000000b005adc699f21dmr13838452ywj.32.1698800447609; Tue, 31 Oct 2023 18:00:47 -0700 (PDT) MIME-Version: 1.0 References: <1698774975-14480-1-git-send-email-igork@ami.com> In-Reply-To: <1698774975-14480-1-git-send-email-igork@ami.com> From: "Mike Maslenkin" Date: Wed, 1 Nov 2023 04:00:11 +0300 Message-ID: Subject: Re: [edk2-devel] [PATCH] RedfishPkg: RedfishDiscoverDxe: Fix issue if IPv4 installed after RestEx To: devel@edk2.groups.io, igork@ami.com Cc: Abner Chang , Nickle Wang Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,mike.maslenkin@gmail.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=M0sWlcUQ; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=gmail.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io Hi Igor please find my comments below. On Tue, Oct 31, 2023 at 8:56=E2=80=AFPM Igor Kulchytskyy via 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 > Cc: Nickle Wang > Cc: Igor Kulchytskyy > Signed-off-by: Igor Kulchytskyy > --- > RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c | 96 ++++++++++++++++= ++-- > 1 file changed, 89 insertions(+), 7 deletions(-) > > diff --git a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c b/Redfish= Pkg/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 =3D (EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_INTER= NAL *)GetFirstNode (&mEfiRedfishDiscoverNetworkInterface); > while (TRUE) { > - if (CompareMem ((VOID *)&ThisNetworkInterface->MacAddress, &TargetNe= tworkInterface->MacAddress, ThisNetworkInterface->HwAddressSize) =3D=3D 0) = { > + if (CompareMem ((VOID *)&ThisNetworkInterface->MacAddress, &TargetNe= tworkInterface->MacAddress, ThisNetworkInterface->HwAddressSize) =3D=3D 0 &= & > + ((TargetNetworkInterface->IsIpv6 && ThisNetworkInterface->Networ= kProtocolType =3D=3D ProtocolTypeTcp6) || > + (!TargetNetworkInterface->IsIpv6 && ThisNetworkInterface->Ne= tworkProtocolType =3D=3D 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 =3D (EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_INTER= NAL *)GetFirstNode (&mEfiRedfishDiscoverNetworkInterface); > while (TRUE) { > if (ThisNetworkInterface->OpenDriverControllerHandle =3D=3D Controll= erHandle) { > @@ -476,6 +486,38 @@ CheckIsIpVersion6 ( > return FALSE; > } > > +/** > + This function returns the IP type supported by the Host Interface > + > + @retval IP Type > + // Unknown=3D00h, > + // Ipv4=3D01h, > + // Ipv6=3D02h, > + > +**/ > +UINT8 STATIC ? > +GetHiIpProtocolType() > +{ > + EFI_STATUS Status; > + REDFISH_OVER_IP_PROTOCOL_DATA *Data; > + REDFISH_INTERFACE_DATA *DeviceDescriptor; > + > + Data =3D NULL; > + DeviceDescriptor =3D NULL; > + if (mSmbios =3D=3D NULL) { > + Status =3D 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() bel= ow. > + } > + } > + Status =3D RedfishGetHostInterfaceProtocolData (mSmbios, &DeviceDescri= ptor, &Data); // Search for SMBIOS type 42h > + if (!EFI_ERROR (Status) && (Data !=3D NULL) && > + (Data->HostIpAssignmentType =3D=3D 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 =3D RedfishGetHostInterfaceProtocolData (mSmbios, &DeviceDescri= ptor, &Data); // Search for SMBIOS type 42h > if (!EFI_ERROR (Status) && (Data !=3D NULL) && (DeviceDescriptor !=3D = NULL)) { > + > + if (Instance->NetworkInterface->NetworkProtocolType =3D=3D ProtocolT= ypeTcp4 && Data->HostIpAddressFormat !=3D REDFISH_HOST_INTERFACE_HOST_IP_AD= DRESS_FORMAT_IP4) { // IPv4 case > + DEBUG((DEBUG_ERROR, "%a: Network Interface is IPv4, but Host Inter= face requires Ipv6\n")); Missed argument for %a format Missed space "DEBUG ((" > + return EFI_UNSUPPORTED; > + } > + else if (Instance->NetworkInterface->NetworkProtocolType =3D=3D Prot= ocolTypeTcp6 && Data->HostIpAddressFormat !=3D REDFISH_HOST_INTERFACE_HOST_= IP_ADDRESS_FORMAT_IP6) { // IPv6 case > + DEBUG((DEBUG_ERROR, "%a: Network Interface is IPv6, but Host Inter= face 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 inte= rface. > // Check with MAC address using Device Descriptor Data Device Type 0= 4 and Type 05. > @@ -1102,6 +1153,7 @@ RedfishServiceGetNetworkInterface ( > OUT EFI_REDFISH_DISCOVER_NETWORK_INTERFACE **NetworkIntfInstances > ) > { > + EFI_STATUS Status; > EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_INTERNAL *ThisNetworkInterface= Intn; > EFI_REDFISH_DISCOVER_NETWORK_INTERFACE *ThisNetworkInterface= ; > EFI_REDFISH_DISCOVER_REST_EX_INSTANCE_INTERNAL *RestExInstance; > @@ -1141,6 +1193,16 @@ RedfishServiceGetNetworkInterface ( > > ThisNetworkInterfaceIntn =3D (EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_I= NTERNAL *)GetFirstNode (&mEfiRedfishDiscoverNetworkInterface); > while (TRUE) { > + // If Get Subnet Info skip this interface > + Status =3D NetworkInterfaceGetSubnetInfo (ThisNetworkInterfaceIntn, = ImageHandle); // Get subnet info > + if (EFI_ERROR(Status)) { > + if (IsNodeAtEnd (&mEfiRedfishDiscoverNetworkInterface, &ThisNetwor= kInterfaceIntn->Entry)) { > + break; > + } > + ThisNetworkInterfaceIntn =3D (EFI_REDFISH_DISCOVER_NETWORK_INTERFA= CE_INTERNAL *)GetNextNode (&mEfiRedfishDiscoverNetworkInterface, &ThisNetwo= rkInterfaceIntn->Entry); > + continue; > + } > + > ThisNetworkInterface->IsIpv6 =3D FALSE; > if (CheckIsIpVersion6 (ThisNetworkInterfaceIntn)) { > ThisNetworkInterface->IsIpv6 =3D TRUE; > @@ -1260,7 +1322,12 @@ RedfishServiceAcquireService ( > // Get subnet information in case subnet information is not set be= cause > // RedfishServiceGetNetworkInterfaces hasn't been called yet. > // > - NetworkInterfaceGetSubnetInfo (TargetNetworkInterfaceInternal, Ima= geHandle); > + Status1 =3D NetworkInterfaceGetSubnetInfo (TargetNetworkInterfaceI= nternal, ImageHandle); > + if (EFI_ERROR(Status1)) { > + DEBUG ((DEBUG_ERROR, "%a: Get subnet information fail.\n", __fun= c__)); > + FreePool(Instance); > + continue; > + } > NewInstance =3D TRUE; > } > > @@ -1535,7 +1602,7 @@ TestForRequiredProtocols ( > UINT32 *Id; > UINTN Index; > EFI_STATUS Status; > - UINTN ListCount; > + UINTN ListCount, SuccessfulCount =3D 0; > > ListCount =3D (sizeof (gRequiredProtocol) / sizeof (REDFISH_DISCOVER_R= EQUIRED_PROTOCOL)); > for (Index =3D 0; Index < ListCount; Index++) { > @@ -1557,13 +1624,14 @@ TestForRequiredProtocols ( > EFI_OPEN_PROTOCOL_GET_PROTOCOL > ); > if (EFI_ERROR (Status)) { > - if (Index =3D=3D ListCount - 1) { > - DEBUG ((DEBUG_INFO, "%a: all required protocols are found on t= his 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 =3D=3D SuccessfulCount) { > + DEBUG ((DEBUG_INFO, "%a: all required protocols are found on this co= ntroller 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 NewNetworkInterfaceIn= stalled; > + UINT8 IpType; > > NewNetworkInterfaceInstalled =3D FALSE; > Index =3D 0; > + > + > + // Get IP Type to filter out unnecessary network protocol if possible > + IpType =3D GetHiIpProtocolType(); > + > do { > + // Check IP Type and skip an unnecessary network protocol if does no= t match > + if (IpType !=3D 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 =3D=3D REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_IP{4,6}" is equal to "IpType !=3D 0" Do I miss something? > + if ((gRequiredProtocol[Index].ProtocolType =3D=3D ProtocolTypeTcp4= && IpType =3D=3D REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_IP6) || > + (gRequiredProtocol[Index].ProtocolType =3D=3D ProtocolTypeTcp6= && IpType =3D=3D REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_IP4)) { > + Index++; Ops. You can not do just { Index++;continue } You need { Index++; if (Index =3D=3D (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/adc3218d3d96083c85cef6c396dde8ddb= 96530fb > + continue; > + } > + } > Status =3D gBS->OpenProtocol ( > // Already in list? > ControllerHandle, > -- > 2.37.1.windows.1 Regards, Mike. -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- 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/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-