Re: [edk2-devel] [PATCH v5 2/2] RedfishPkg: RedfishDiscoverDxe: Optimize the Redfish Discover flow

["Mike Maslenkin" <mike.maslenkin@gmail.com>]

On Tue, Nov 14, 2023 at 9:57 PM Igor Kulchytskyy via groups.io
<igork=ami.com@groups.io> wrote:
>
> Hi Leif,
> Please see my comments below.
> Thank you,
> Igor
>
>
> -----Original Message-----
> From: Leif Lindholm <quic_llindhol@quicinc.com>
> Sent: Tuesday, November 14, 2023 12:26 PM
> To: devel@edk2.groups.io; Igor Kulchytskyy <igork@ami.com>
> Cc: Abner Chang <abner.chang@amd.com>; Nickle Wang <nicklew@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.chang@amd.com>
> > Cc: Nickle Wang <nicklew@nvidia.com>
> > Signed-off-by: Igor Kulchytskyy <igork@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.


-=-=-=-=-=-=-=-=-=-=-=-
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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-