[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_llind...@quicinc.com>
> Sent: Wednesday, November 15, 2023 6:59 PM
> To: devel@edk2.groups.io; Chang, Abner <abner.ch...@amd.com>; Mike
> Maslenkin <mike.maslen...@gmail.com>; ig...@ami.com; Gao, Liming
> <gaolim...@byosoft.com.cn>; Kinney, Michael D
> <michael.d.kin...@intel.com>
> Cc: Nickle Wang <nick...@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.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 (#111255): https://edk2.groups.io/g/devel/message/111255
Mute This Topic: https://groups.io/mt/102602698/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
