Re: edk2-stable202311 Code freeze process violation 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]

Hi Leif,
As we requested Liming to wait for this change last week, he accepted to wait for the PR. But you are right, suppose I shouldn't be allowed to merge the change during code freeze. Maybe only certain people have privilege to merge the code during code freeze. If I still can merge the code then the mechanism may be broken. I am fine if you would like to revert these commits.

Regards,
Abner

> -----Original Message-----
> From: Leif Lindholm <quic_llindhol@quicinc.com>
> Sent: Wednesday, November 15, 2023 6:59 PM
> To: devel@edk2.groups.io; Chang, Abner <Abner.Chang@amd.com>; Mike
> Maslenkin <mike.maslenkin@gmail.com>; igork@ami.com; Gao, Liming
> <gaoliming@byosoft.com.cn>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Cc: Nickle Wang <nicklew@nvidia.com>
> Subject: edk2-stable202311 Code freeze process violation 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 2023-11-15 03:55, Chang, Abner via groups.io wrote:
> > [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.
>
> I was under the impression merging was disabled for everyone except Mike
> and Liming during code freeze specifically to avoid this situation.
> Apparently, that isn't working.
>
> Regardless, this is a violation of the stable tag process.
> Liming: can you please revert these commits?
>
> Regards,
>
> Leif
>
> > 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 (#111255): https://edk2.groups.io/g/devel/message/111255
Mute This Topic: https://groups.io/mt/102602698/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-