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

["Chang, Abner via groups.io" <abner.chang=amd.com@groups.io>]

[AMD Official Use Only - General]

Just let you know I just merged this change. Igor can help to follow up the suggestions given by Leif and Mike.

Thanks
Abner

> -----Original Message-----
> From: Chang, Abner
> Sent: Wednesday, November 15, 2023 9:20 AM
> To: Mike Maslenkin <mike.maslenkin@gmail.com>; devel@edk2.groups.io;
> igork@ami.com
> Cc: Leif Lindholm <quic_llindhol@quicinc.com>; Nickle Wang
> <nicklew@nvidia.com>
> Subject: RE: [edk2-devel] [PATCH v5 2/2] RedfishPkg: RedfishDiscoverDxe:
> Optimize the Redfish Discover flow
>
> Hi Mike and Leif,
> Thanks for your comments on this change. As we are rushing to get this
> change to be pulled in stable release 202312 this week, I will just merge this
> code to master branch and let the discussing keeps going.
> I think there is no functionality difference base on your suggestions, but it's
> about the coding practice and readability.
>
> Hi Igor,
> Could you please resend the V6 after stable tag is released if Mike and Leif's
> comment is reasonable to you?
>
> Thanks
> Abner
>
> > -----Original Message-----
> > From: Mike Maslenkin <mike.maslenkin@gmail.com>
> > Sent: Wednesday, November 15, 2023 7:53 AM
> > To: devel@edk2.groups.io; igork@ami.com
> > Cc: Leif Lindholm <quic_llindhol@quicinc.com>; Chang, Abner
> > <Abner.Chang@amd.com>; Nickle Wang <nicklew@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.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 (#111233): https://edk2.groups.io/g/devel/message/111233
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]
-=-=-=-=-=-=-=-=-=-=-=-