On Tue, Nov 14, 2023 at 9:57 PM Igor Kulchytskyy via groups.io
<[email protected]> wrote:
>
> Hi Leif,
> Please see my comments below.
> Thank you,
> Igor
>
>
> -----Original Message-----
> From: Leif Lindholm <[email protected]>
> Sent: Tuesday, November 14, 2023 12:26 PM
> To: [email protected]; Igor Kulchytskyy <[email protected]>
> Cc: Abner Chang <[email protected]>; Nickle Wang <[email protected]>
> 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 <[email protected]>
> > Cc: Nickle Wang <[email protected]>
> > Signed-off-by: Igor Kulchytskyy <[email protected]>
> > ---
> > 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.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111220): https://edk2.groups.io/g/devel/message/111220
Mute This Topic: https://groups.io/mt/102584140/21656
Group Owner: [email protected]
Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]]
-=-=-=-=-=-=-=-=-=-=-=-
