edk2-stable202311 Code freeze process violation Re: [edk2-devel] [PATCH v5 2/2] RedfishPkg: RedfishDiscoverDxe: Optimize the Redfish Discover flow

["Leif Lindholm" <quic_llindhol@quicinc.com>]

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 (#111254): https://edk2.groups.io/g/devel/message/111254
Mute This Topic: https://groups.io/mt/102602698/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-