[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.maslen...@gmail.com>; devel@edk2.groups.io;
> ig...@ami.com
> Cc: Leif Lindholm <quic_llind...@quicinc.com>; Nickle Wang
> <nick...@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.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.
-=-=-=-=-=-=-=-=-=-=-=-
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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
