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 4E5D2940F6E for ; Mon, 6 Nov 2023 02:29:45 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=VE81RftYfoGGkeFNafTj+toXH/Uk/NvM1HxlFc/Xmho=; c=relaxed/simple; d=groups.io; h=From:Message-Id:Mime-Version:Subject:Date:In-Reply-To:Cc:To:References:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type; s=20140610; t=1699237783; v=1; b=XMWRX9iEAVTvSx7ra2+wSeyeqhtl61NbW/E2+unjBMY4+9chEfIkcDWzcgpCoXvKksDIZR2R RIwUOY5mHCrtvkRXUpbvSa8/25QT1sFr+GCwPXrQgMo05uUCMPxsvJZn5ugmZg3puBd97LBvy8L yxSzxKdjNiT+HB/meeHQTKL8= X-Received: by 127.0.0.2 with SMTP id 9t0IYY7687511xaXFvv8B52G; Sun, 05 Nov 2023 18:29:43 -0800 X-Received: from mail-lj1-f169.google.com (mail-lj1-f169.google.com [209.85.208.169]) by mx.groups.io with SMTP id smtpd.web11.45685.1699237782673145928 for ; Sun, 05 Nov 2023 18:29:43 -0800 X-Received: by mail-lj1-f169.google.com with SMTP id 38308e7fff4ca-2c6b5841f61so43048711fa.0 for ; Sun, 05 Nov 2023 18:29:42 -0800 (PST) X-Gm-Message-State: o4qOFMiERZMF341tUt5Sj9wWx7686176AA= X-Google-Smtp-Source: AGHT+IGjY9XiK51mHNniiZ+B0gloKWBPROslnQg+A96iPVr1QR+GuEoPU6mNThD5X25U9aL2iSeTOA== X-Received: by 2002:a2e:bcc4:0:b0:2c4:fdfa:41c with SMTP id z4-20020a2ebcc4000000b002c4fdfa041cmr4141798ljp.0.1699237780254; Sun, 05 Nov 2023 18:29:40 -0800 (PST) X-Received: from smtpclient.apple ([185.9.78.43]) by smtp.gmail.com with ESMTPSA id t3-20020a2e9d03000000b002c506c89511sm1032134lji.37.2023.11.05.18.29.39 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Sun, 05 Nov 2023 18:29:39 -0800 (PST) From: "Mike Maslenkin" Message-Id: <7FCC331E-BAF1-426C-89BE-2D4ED70E5CE9@gmail.com> Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3696.100.31\)) Subject: Re: [edk2-devel] [PATCH] RedfishPkg: RedfishDiscoverDxe: Fix issue if IPv4 installed after RestEx Date: Mon, 6 Nov 2023 05:29:38 +0300 In-Reply-To: Cc: "devel@edk2.groups.io" , Abner Chang , Nickle Wang To: Igor Kulchytskyy References: <1698774975-14480-1-git-send-email-igork@ami.com> 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: multipart/alternative; boundary="Apple-Mail=_217B290A-8040-4BD7-8649-03BF44F9107F" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=XMWRX9iE; 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 --Apple-Mail=_217B290A-8040-4BD7-8649-03BF44F9107F Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 Hi Igor I want to return to my initial comments regarding this patch. Please, find my comments below. > On 1. 11. 2023., at 06:24, Igor Kulchytskyy wrote: >=20 > Hi Mike, > Thank you for review. > Please see my answers below the text. >=20 > -----Original Message----- > From: Mike Maslenkin > Sent: Tuesday, October 31, 2023 9:00 PM > To: devel@edk2.groups.io; Igor Kulchytskyy > Cc: Abner Chang ; Nickle Wang > Subject: [EXTERNAL] Re: [edk2-devel] [PATCH] RedfishPkg: RedfishDiscoverD= xe: Fix issue if IPv4 installed after RestEx >=20 >=20 > **CAUTION: The e-mail below is from an external source. Please exercise c= aution before opening attachments, clicking links, or following guidance.** >=20 > Hi Igor >=20 > please find my comments below. >=20 > On Tue, Oct 31, 2023 at 8:56=E2=80=AFPM Igor Kulchytskyy via groups.io > wrote: >>=20 >> 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. >>=20 >> 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(-) >>=20 >> diff --git a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c b/Redfis= hPkg/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= ; >>=20 >> + if (IsListEmpty(&mEfiRedfishDiscoverNetworkInterface)) { >> + return NULL; >> + } >> + >> ThisNetworkInterface =3D (EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_INTER= NAL *)GetFirstNode (&mEfiRedfishDiscoverNetworkInterface); >> while (TRUE) { >> - if (CompareMem ((VOID *)&ThisNetworkInterface->MacAddress, &TargetN= etworkInterface->MacAddress, ThisNetworkInterface->HwAddressSize) =3D=3D 0)= { >> + if (CompareMem ((VOID *)&ThisNetworkInterface->MacAddress, &TargetN= etworkInterface->MacAddress, ThisNetworkInterface->HwAddressSize) =3D=3D 0 = && >> + ((TargetNetworkInterface->IsIpv6 && ThisNetworkInterface->Netwo= rkProtocolType =3D=3D ProtocolTypeTcp6) || >> + (!TargetNetworkInterface->IsIpv6 && ThisNetworkInterface->N= etworkProtocolType =3D=3D ProtocolTypeTcp4))) { >> return ThisNetworkInterface; >> } >>=20 >> @@ -354,6 +360,10 @@ GetTargetNetworkInterfaceInternalByController ( >> { >> EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_INTERNAL *ThisNetworkInterface= ; >>=20 >> + if (IsListEmpty(&mEfiRedfishDiscoverNetworkInterface)) { >> + return NULL; >> + } >> + >=20 > 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(). >=20 > Igor: Agree. > I also just noticed that even if GetTargetNetworkInterfaceInternal functi= on may return NULL, the return value is not verified on NULL in RedfishServ= iceAcquireService function. > I think we should add this verification. >=20 >=20 > Also I wonder why check patch doesn't complain about missed spaces in > "IsListEmpty (&mEfiRedfishDiscoverNetworkInterface)" call for example. >=20 >> 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; >> } >>=20 >> +/** >> + This function returns the IP type supported by the Host Interface >> + >> + @retval IP Type >> + // Unknown=3D00h, >> + // Ipv4=3D01h, >> + // Ipv6=3D02h, >> + >> +**/ >> +UINT8 >=20 > STATIC ? > Igor: WHY? >=20 I remembered discussion about STATIC functions usage and why it isn't commo= n in edk2. AFAIR it was because of difficulties with source level debugging for the ca= se when it could be inlined. BTW I won't insist on this here. >> +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, (VOI= D **)&mSmbios); >> + if (EFI_ERROR (Status)) { >> + return 0; >=20 > 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() b= elow. > Igor: Thank you. Missed that macro. >=20 >> + } >> + } >> + Status =3D RedfishGetHostInterfaceProtocolData (mSmbios, &DeviceDescr= iptor, &Data); // Search for SMBIOS type 42h >> + if (!EFI_ERROR (Status) && (Data !=3D NULL) && >> + (Data->HostIpAssignmentType =3D=3D RedfishHostIpAssignmentStatic)= ) { >> + return Data->HostIpAddressFormat; >> + } >> + return 0; >=20 > Same here, 0 is a magic value for > REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_UNKNOWN >=20 >> +} >> + >> /** >> This function discover Redfish service through SMBIOS host interface. >>=20 >> @@ -512,6 +554,15 @@ DiscoverRedfishHostInterface ( >>=20 >> 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 Protocol= TypeTcp4 && Data->HostIpAddressFormat !=3D REDFISH_HOST_INTERFACE_HOST_IP_A= DDRESS_FORMAT_IP4) { // IPv4 case >> + DEBUG((DEBUG_ERROR, "%a: Network Interface is IPv4, but Host Inte= rface requires Ipv6\n")); >=20 > Missed argument for %a format > Missed space "DEBUG ((" >=20 > Igor: Inattentiveness. Thank you. >=20 >> + return EFI_UNSUPPORTED; >> + } >> + else if (Instance->NetworkInterface->NetworkProtocolType =3D=3D Pro= tocolTypeTcp6 && Data->HostIpAddressFormat !=3D REDFISH_HOST_INTERFACE_HOST= _IP_ADDRESS_FORMAT_IP6) { // IPv6 case >> + DEBUG((DEBUG_ERROR, "%a: Network Interface is IPv6, but Host Inte= rface requires IPv4\n")); >=20 > Missed argument for %a format > Missed space "DEBUG ((" >=20 > Igor: Inattentiveness. Thank you. >=20 >> + 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 ( >>=20 >> 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, &ThisNetwo= rkInterfaceIntn->Entry)) { >> + break; >> + } >> + ThisNetworkInterfaceIntn =3D (EFI_REDFISH_DISCOVER_NETWORK_INTERF= ACE_INTERNAL *)GetNextNode (&mEfiRedfishDiscoverNetworkInterface, &ThisNetw= orkInterfaceIntn->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, Im= ageHandle); >> + Status1 =3D NetworkInterfaceGetSubnetInfo (TargetNetworkInterface= Internal, ImageHandle); >> + if (EFI_ERROR(Status1)) { >> + DEBUG ((DEBUG_ERROR, "%a: Get subnet information fail.\n", __fu= nc__)); >> + FreePool(Instance); >> + continue; >> + } >> NewInstance =3D TRUE; >> } >>=20 >> @@ -1535,7 +1602,7 @@ TestForRequiredProtocols ( >> UINT32 *Id; >> UINTN Index; >> EFI_STATUS Status; >> - UINTN ListCount; >> + UINTN ListCount, SuccessfulCount =3D 0; >>=20 >> 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 = 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. >=20 > Igor: We need TestForRequiredProtocols return SUCCESS when all protocols = listed in gRequiredProtocol installed. > When this happened the BuildupNetworkInterface function would be called, = and we iterate through gRequiredProtocol array and build network interfaces= . > We also install DiscoveredProtocolGuid to prevent BuildupNetworkInterface= to be called after that. > If we have SuccessfulCount in scope of upper "if (!EFI_ERROR (Status)) {= ", then BuildupNetworkInterface will be called if something installed on th= at ControllerHandle, like DNS protocol, for example. Yes, I understand this... but it's still illogical, taking in account varia= ble name. I mean if (EFI_ERROR (Status)) {SuccessfulCount++}; Here we increase SuccessfulCount for the case "Cond1 && !Cond2", and retur= n EFI_SUCCESS only if it is true for all gRequiredProtocol entries, i.e 3 t= imes. Wouldn't it be better to rewrite this code to smth like the following: { UINT32 *Id; UINTN Index; EFI_STATUS Status; for (Index =3D 0; Index < ARRAY_SIZE (gRequiredProtocol); Index++) { Status =3D gBS->OpenProtocol ( ControllerHandle, gRequiredProtocol[Index].RequiredServiceBindingProtocol= Guid, NULL, This->DriverBindingHandle, ControllerHandle, EFI_OPEN_PROTOCOL_TEST_PROTOCOL ); if (EFI_ERROR (Status)) { return EFI_UNSUPPORTED; } Status =3D gBS->OpenProtocol ( ControllerHandle, gRequiredProtocol[Index].DiscoveredProtocolGuid, (VOID **)&Id, This->DriverBindingHandle, ControllerHandle, EFI_OPEN_PROTOCOL_GET_PROTOCOL ); if (!EFI_ERROR (Status)) { // Already installed return EFI_UNSUPPORTED; } } DEBUG ((DEBUG_MANAGEABILITY, "%a: all required protocols are found on thi= s controller handle: %p.\n", __func__, ControllerHandle)); return EFI_SUCCESS; } ? No unnecessary variables, less indentation + this function is called from R= edfishDiscoverDriverBindingSupported(), i.e very often, so we just reduce n= umber of useless loop iterations and OpenProtocol() calls. 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 (#110694): https://edk2.groups.io/g/devel/message/110694 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- --Apple-Mail=_217B290A-8040-4BD7-8649-03BF44F9107F Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8
Hi Igor

I want to return to= my initial comments regarding this patch.
Please, fin= d my comments below.


On 1. 11. 2023= ., at 06:24, Igor Kulchytskyy <igork@ami.com> wrote:

Hi Mike,
Thank you for revi= ew.
Please see my answers below the text.

-----Original Message-----
From: Mike Maslenkin <= ;mike.maslenkin@gmai= l.com>
Sent: Tuesday, October 31, 2023 9:00 PM
To: devel@edk2.gr= oups.io; Igor Kulchytskyy <igork@ami.com>
Cc: Abner Chang <abner.chang@amd.com>; Nickle Wang = <nicklew@nvidia.com= >
Subject: [EXTERNAL] Re: [edk2-devel] [PATCH] RedfishPkg:= RedfishDiscoverDxe: Fix issue if IPv4 installed after RestEx


**CAUTION: The e-mail below is from an exter= nal source. Please exercise caution before opening attachments, clicking li= nks, or following guidance.**

Hi Igor

please find my comments below.

On Tue, Oct 31, 2023 at 8:56=E2=80=AFPM Igor Kulchytskyy via groups.io
<igork=3Dami.com@groups.io= > wrote:

Supported function of the driver changed to wait for all newtwork
interface to be installed.
Filer out the network inte= rfaces which are not supported by
Redfish Host Interface.

Cc: Abner Chang <abner.chang@amd.com>
Cc: Nickle W= ang <nicklew@nvidia.com= >
Cc: Igor Kulchytskyy <igork@ami.com>
Signed-off-by: Igor Ku= lchytskyy <igork@ami.com= >
---
RedfishPkg/RedfishDiscoverDxe/Redfish= DiscoverDxe.c | 96 ++++++++++++++++++--
1 file changed, 89 i= nsertions(+), 7 deletions(-)

diff --git a/Redf= ishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c b/RedfishPkg/RedfishDiscover= Dxe/RedfishDiscoverDxe.c
index 23da3b968f..a88ad55938 100644<= br class=3D"">--- a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
+++ b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
@@ -322,9 +322,15 @@ GetTargetNetworkInterfaceInternal (
{
  EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_INTE= RNAL  *ThisNetworkInterface;

+  if (= IsListEmpty(&mEfiRedfishDiscoverNetworkInterface)) {
+ &n= bsp;  return NULL;
+  }
+
  ThisNetworkInterface =3D (EFI_REDFISH_DISCOVER_NETWORK= _INTERFACE_INTERNAL *)GetFirstNode (&mEfiRedfishDiscoverNetworkInterfac= e);
  while (TRUE) {
-   &= nbsp;if (CompareMem ((VOID *)&ThisNetworkInterface->MacAddress, &= ;TargetNetworkInterface->MacAddress, ThisNetworkInterface->HwAddressS= ize) =3D=3D 0) {
+    if (CompareMem ((VOID *)= &ThisNetworkInterface->MacAddress, &TargetNetworkInterface->M= acAddress, ThisNetworkInterface->HwAddressSize) =3D=3D 0 &&
+        ((TargetNetworkInterf= ace->IsIpv6 && ThisNetworkInterface->NetworkProtocolType =3D= =3D ProtocolTypeTcp6) ||
+      &nbs= p;     (!TargetNetworkInterface->IsIpv6 &&a= mp; ThisNetworkInterface->NetworkProtocolType =3D=3D ProtocolTypeTcp4)))= {
      return ThisNetworkInt= erface;
    }

@@ -354,6 +360,10 @@ GetTargetNetworkInterfaceInternalByController (
{
  EFI_REDFISH_DISCOVER_NETWORK_INTER= FACE_INTERNAL  *ThisNetworkInterface;

+ &= nbsp;if (IsListEmpty(&mEfiRedfishDiscoverNetworkInterface)) {
+    return NULL;
+  }
+

I also have these two hunks i= n my pending list.
But I suggest to add ASSERT to GetTargetNe= tworkInterfaceInternal, just
because currently it is really i= mpossible situation,
and mEfiRedfishDiscoverNetworkInterface = was checked before in scope of
ValidateTargetNetworkInterfac= e().

Igor: Agree.
I also just no= ticed that even if GetTargetNetworkInterfaceInternal function may return NU= LL, the return value is not verified on NULL in RedfishServiceAcquireServic= e function.
I think we should add this verification.


Also I wonder why check patch doesn't c= omplain about missed spaces in
"IsListEmpty (&mEfiRedfish= DiscoverNetworkInterface)" call for example.

<= blockquote type=3D"cite" class=3D"">   ThisNetworkInterface =3D (= EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_INTERNAL *)GetFirstNode (&mEfiRe= dfishDiscoverNetworkInterface);
  while (TRUE) {     if (ThisNetworkInterface->OpenDriv= erControllerHandle =3D=3D ControllerHandle) {
@@ -476,6 +486,= 38 @@ CheckIsIpVersion6 (
  return FALSE;
}

+/**
+  This funct= ion returns the  IP type supported by the Host Interface
+
+  @retval IP Type
+  //  Unk= nown=3D00h,
+  //  Ipv4=3D01h,
+ &nbs= p;//  Ipv6=3D02h,
+
+**/
+UI= NT8

STATIC ?
Igor: = WHY?


I remembered discussion about STATIC functions usage and w= hy it isn't common in edk2.
AFAIR it was because of difficulties = with source level debugging for the case when it could be inlined.
BTW I won't insist on this here.


=
+GetHiIpProtocolType()
+= {
+  EFI_STATUS       &nbs= p;            &= nbsp;Status;
+  REDFISH_OVER_IP_PROTOCOL_DATA  *Dat= a;
+  REDFISH_INTERFACE_DATA     &nb= sp;   *DeviceDescriptor;
+
+ &nb= sp;Data            &= nbsp;=3D NULL;
+  DeviceDescriptor =3D NULL;
+  if (mSmbios =3D=3D NULL) {
+    Sta= tus =3D gBS->LocateProtocol (&gEfiSmbiosProtocolGuid, NULL, (VOID **= )&mSmbios);
+    if (EFI_ERROR (Status)) {=
+      return 0;

In this driver REDFISH_HOST_INTERFACE_HOST_IP_ADDRE= SS_FORMAT_IP{4,6}
used and accessible.
so, 0 me= ans  REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_UNKNOWN
And these values actually checked in scope of BuildupNetworkInterface() = below.
Igor: Thank you. Missed that macro.

+    }
+  }
+  Status =3D RedfishGetHostInterface= ProtocolData (mSmbios, &DeviceDescriptor, &Data); // Search for SMB= IOS type 42h
+  if (!EFI_ERROR (Status) && (Data= !=3D NULL) &&
+      (Data-= >HostIpAssignmentType =3D=3D RedfishHostIpAssignmentStatic)) {
+      return Data->HostIpAddressFormat;<= br class=3D"">+  }
+  return 0;

Same here, 0 is a magic value for
REDF= ISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_UNKNOWN

+}
+
/**
  This function discover Redfish service thr= ough 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) && (DeviceDescr= iptor !=3D NULL)) {
+
+    if (I= nstance->NetworkInterface->NetworkProtocolType =3D=3D ProtocolTypeTcp= 4 && Data->HostIpAddressFormat !=3D REDFISH_HOST_INTERFACE_HOST_= IP_ADDRESS_FORMAT_IP4) { // IPv4 case
+    &nb= sp; DEBUG((DEBUG_ERROR, "%a: Network Interface is IPv4, but Host Inter= face requires Ipv6\n"));

Missed a= rgument for %a format
Missed space "DEBUG (("
<= br class=3D"">Igor: Inattentiveness. Thank you.

+      retu= rn EFI_UNSUPPORTED;
+    }
+ &nb= sp;  else if (Instance->NetworkInterface->NetworkProtocolTy= pe =3D=3D ProtocolTypeTcp6 && Data->HostIpAddressFormat !=3D RED= FISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_IP6) { // IPv6 case
+      DEBUG((DEBUG_ERROR, "%a: Network Interfac= e is IPv6, but Host Interface requires IPv4\n"));

Missed argument for %a format
Missed space "= DEBUG (("

Igor: Inattentiveness. Thank you.
+  &nb= sp;   return EFI_UNSUPPORTED;
+   &n= bsp;}
    //
  =   // Check if we can reach out Redfish service using this network= interface.
    // Check with MAC addres= s using Device Descriptor Data Device Type 04 and Type 05.
@@= -1102,6 +1153,7 @@ RedfishServiceGetNetworkInterface (
&nbs= p; OUT EFI_REDFISH_DISCOVER_NETWORK_INTERFACE  **NetworkIntfInsta= nces
  )
{
+  EF= I_STATUS            =             &nb= sp;            =   Status;
  EFI_REDFISH_DISCOVER_NETWORK= _INTERFACE_INTERNAL  *ThisNetworkInterfaceIntn;
 &= nbsp;EFI_REDFISH_DISCOVER_NETWORK_INTERFACE      &= nbsp;    *ThisNetworkInterface;
 &n= bsp;EFI_REDFISH_DISCOVER_REST_EX_INSTANCE_INTERNAL   *RestExInsta= nce;
@@ -1141,6 +1193,16 @@ RedfishServiceGetNetworkInterface= (

  ThisNetworkInterfaceIntn =3D (= EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_INTERNAL *)GetFirstNode (&mEfiRe= dfishDiscoverNetworkInterface);
  while (TRUE) {+    // If Get Subnet Info skip this interface<= br class=3D"">+    Status =3D NetworkInterfaceGetSubnetInfo = (ThisNetworkInterfaceIntn, ImageHandle); // Get subnet info
+=    if (EFI_ERROR(Status)) {
+   &nb= sp;  if (IsNodeAtEnd (&mEfiRedfishDiscoverNetworkInterface, &= amp;ThisNetworkInterfaceIntn->Entry)) {
+   &nbs= p;    break;
+     &n= bsp;}
+      ThisNetworkInterfaceInt= n =3D (EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_INTERNAL *)GetNextNode (&= mEfiRedfishDiscoverNetworkInterface, &ThisNetworkInterfaceIntn->Entr= y);
+      continue;
+=    }
+
    = ;ThisNetworkInterface->IsIpv6 =3D FALSE;
  &nbs= p; if (CheckIsIpVersion6 (ThisNetworkInterfaceIntn)) {
=       ThisNetworkInterface->IsIpv6 =3D TRU= E;
@@ -1260,7 +1322,12 @@ RedfishServiceAcquireService (
      // Get subnet information in= case subnet information is not set because
  &nbs= p;   // RedfishServiceGetNetworkInterfaces hasn't been calle= d yet.
      //
= -      NetworkInterfaceGetSubnetInfo (TargetNetwor= kInterfaceInternal, ImageHandle);
+     &= nbsp;Status1 =3D NetworkInterfaceGetSubnetInfo (TargetNetworkInterfaceInter= nal, ImageHandle);
+      if (EFI_ER= ROR(Status1)) {
+        D= EBUG ((DEBUG_ERROR, "%a: Get subnet information fail.\n", __func__));
+        FreePool(Instance);+        continue;
+      }
   &n= bsp;  NewInstance =3D TRUE;
   &nbs= p;}

@@ -1535,7 +1602,7 @@ TestForRequiredProto= cols (
  UINT32      *Id;=
  UINTN       Index= ;
  EFI_STATUS  Status;
-  = ;UINTN       ListCount;
+  = ;UINTN       ListCount, SuccessfulCount =3D 0= ;

  ListCount =3D (sizeof (gRequire= dProtocol) / sizeof (REDFISH_DISCOVER_REQUIRED_PROTOCOL));
&= nbsp; for (Index =3D 0; Index < ListCount; Index++) {
@@ -1557,13 +1624,14 @@ TestForRequiredProtocols (
 &n= bsp;            = ;        EFI_OPEN_PROTOCOL_GET_PROT= OCOL
         &= nbsp;           &nbs= p;);
      if (EFI_ERROR (Stat= us)) {
-        if (Index = =3D=3D ListCount - 1) {
-       = ;   DEBUG ((DEBUG_INFO, "%a: all required protocols are foun= d on this controller handle: %p.\n", __func__, ControllerHandle));
-          return EFI_SU= CCESS;
-        }
+        SuccessfulCount++;
      }
I= t 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.

Igor: We need TestForRequiredProtocols return SUCCESS when all protocols= listed in gRequiredProtocol installed.
When this happened th= e BuildupNetworkInterface function would be called, and we iterate through = gRequiredProtocol array and build network interfaces.
We also= install DiscoveredProtocolGuid to prevent BuildupNetworkInterface to be ca= lled after that.
If we have SuccessfulCount in scope of  = ;upper "if (!EFI_ERROR (Status)) {", then BuildupNetworkInterface will be c= alled if something installed on that ControllerHandle, like DNS protocol, f= or example.

Yes, I unders= tand this... but it's still illogical, taking in account variable name. I m= ean  if (EFI_ERROR (Status)) {SuccessfulCount++};

Here we increase SuccessfulCount for the case  "Cond1= && !Cond2", and return EFI_SUCCESS only if it is true for all gReq= uiredProtocol entries, i.e 3 times.

Wou= ldn't it be better to rewrite this code to smth like the following:

{
  UINT32=       *Id;
  UI= NTN       Index;
&nb= sp; EFI_STATUS  Status;

  = for (Index =3D 0; Index < AR= RAY_SIZE (gRequiredProtocol); Index++) {
    Status =3D gBS->OpenProtocol (
                =     ControllerHandle,
&nb= sp;                   gRequire= dProtocol[Index].RequiredServiceBindingProtocolGuid,
                &= nbsp;   NULL,
                   = This->DriverBindingHandle,
= &nbs= p;                   Controlle= rHandle,
       = ;             EFI_OPEN_PROTOCOL_TEST_PROTOCOL=
         =           );
    if (EFI_ERROR (Status)) {<= /div>
      re= turn EFI_UNSUPPORTED;
  &nb= sp; }

    Status =3D gBS-&= gt;OpenProtocol (
    &= nbsp;               ControllerHandle,
          &n= bsp;         gRequiredProtocol[Index].DiscoveredProtoco= lGuid,
        =             (VOID **)&Id,
              &= nbsp;     This->DriverBindingHandle,
                &nbs= p;   ControllerHandle,
  =                   EFI_OPEN_PRO= TOCOL_GET_PROTOCOL
    &= nbsp;               );
    if (!EFI_ERROR (= Status)) {
  &n= bsp;   // Already installed
      return EFI_UNSUPPORTED;=
    }
  }

  DEBUG ((DEBUG_MANAGEABILITY, "%a: all re= quired protocols are found on this controller handle: %p= .\n", __func__, ControllerHandle));
  = return EFI_SUCCESS;
}

?
No&nb= sp;unnecessary variables, less indentation + this function is called f= rom RedfishDiscoverDriverBindingSupported(), i.e very often,= so we just reduce number of useless loop iterations and&= nbsp;OpenProtocol() calls.

Regards,
= Mike.


_._,_._,_
Groups.io Links:

=20 You receive all messages sent to this group. =20 =20

View/Reply Online (#110694) | =20 | Mute= This Topic | New Topic
Your Subscriptio= n | Contact Group Owner | Unsubscribe [rebecca@openfw.io]

_._,_._,_
--Apple-Mail=_217B290A-8040-4BD7-8649-03BF44F9107F--