-----Original Message-----
From: Mike Maslenkin <mike.maslen...@gmail.com>
Sent: Wednesday, November 15, 2023 7:53 AM
To: devel@edk2.groups.io; ig...@ami.com
Cc: Leif Lindholm <quic_llind...@quicinc.com>; Chang, Abner
<abner.ch...@amd.com>; Nickle Wang <nick...@nvidia.com>
Subject: Re: [edk2-devel] [PATCH v5 2/2] RedfishPkg: RedfishDiscoverDxe:
Optimize the Redfish Discover flow
Caution: This message originated from an External Source. Use proper caution
when opening attachments, clicking links, or responding.
On Tue, Nov 14, 2023 at 9:57 PM Igor Kulchytskyy via groups.io
<igork=ami....@groups.io> wrote:
Hi Leif,
Please see my comments below.
Thank you,
Igor
-----Original Message-----
From: Leif Lindholm <quic_llind...@quicinc.com>
Sent: Tuesday, November 14, 2023 12:26 PM
To: devel@edk2.groups.io; Igor Kulchytskyy <ig...@ami.com>
Cc: Abner Chang <abner.ch...@amd.com>; Nickle Wang
<nick...@nvidia.com>
Subject: [EXTERNAL] Re: [edk2-devel] [PATCH v5 2/2] RedfishPkg:
RedfishDiscoverDxe: Optimize the Redfish Discover flow
**CAUTION: The e-mail below is from an external source. Please exercise
caution before opening attachments, clicking links, or following guidance.**
On 2023-11-14 14:28, Igor Kulchytskyy via groups.io wrote:
Filter 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>
Signed-off-by: Igor Kulchytskyy <ig...@ami.com>
---
RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c | 163
++++++++++++++------
RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverInternal.h | 6 +
2 files changed, 120 insertions(+), 49 deletions(-)
diff --git a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
index 0f622e05a9..ae83cd3c97 100644
--- a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
+++ b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
@@ -1601,10 +1681,22 @@ 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 (IS_TCP4_MATCH (IpType) || IS_TCP6_MATCH (IpType)) {
The logic of these macros is inverted compared to their names, though.
You want this test to read
if (!IS_TCP4_MATCH (IpType) && !IS_TCP6_MATCH (IpType)) {
+ continue;
+ }
+
Status = gBS->OpenProtocol (
// Already in list?
ControllerHandle,
diff --git a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverInternal.h
b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverInternal.h
index 01454acc1d..3093eea0d5 100644
--- a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverInternal.h
+++ b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverInternal.h
@@ -39,6 +39,12 @@
#define REDFISH_DISCOVER_VERSION 0x00010000
#define EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_TPL TPL_NOTIFY
+#define MAC_COMPARE(ThisNetworkInterface, TargetNetworkInterface)
(CompareMem ((VOID *)&ThisNetworkInterface->MacAddress,
&TargetNetworkInterface->MacAddress, ThisNetworkInterface-
HwAddressSize))
+#define VALID_TCP6(TargetNetworkInterface, ThisNetworkInterface)
(TargetNetworkInterface->IsIpv6 && (ThisNetworkInterface-
NetworkProtocolType == ProtocolTypeTcp6))
+#define VALID_TCP4(TargetNetworkInterface, ThisNetworkInterface)
(!TargetNetworkInterface->IsIpv6 && (ThisNetworkInterface-
NetworkProtocolType == ProtocolTypeTcp4))
+#define IS_TCP4_MATCH(IpType)
((gRequiredProtocol[Index].ProtocolType == ProtocolTypeTcp4) && (IpType !=
REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_IP4))
+#define IS_TCP6_MATCH(IpType)
((gRequiredProtocol[Index].ProtocolType == ProtocolTypeTcp6) && (IpType !=
REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_IP6))
And these macros to test for ==, not !=
Igor: First version tested "==", but we agreed that it may not work if we have
a wrong value of IpType.
Otherwise the code reads like it does the opposite of what it does.
(You could also keep the logic and call the macros IS_TCP#_MISMATCH, but
that feels a bit convoluted.)
Igor: I would prefer to go with IS_TCP#_MISMATCH names.
Regards,
Leif
Sorry, could I add my 2 cents?
For me all newly added defines looks bad, just because those
implicitly use reference to a global variable
plus local variable state (i.e current cycle index).
Could we rewrite code in a simple and straight forward manner, similar to:
if (IpType ==
REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_UNKNOWN) {
// The protocol type is not specified in SMBIOS table type 42h
return EFI_UNSUPPORTED;
}
for (Index = 0; Index < ListCount; Index++) {
if ((gRequiredProtocol[Index].ProtocolType == ProtocolTypeTcp4) &&
(IpType != REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_IP4)) {
continue;
}
if ((gRequiredProtocol[Index].ProtocolType == ProtocolTypeTcp6) &&
(IpType != REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_IP6)) {
continue;
}
<skip>
Regards,
Mike.
