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

["gaoliming via groups.io" <gaoliming=byosoft.com.cn@groups.io>]

Mike:
  Yes. We should set Read access in code freeze phase.

Abner:
  In hard code freeze phase, the critical issue is still allowed to be merged after it got approval from Stewards. Can you resend the patch with edk2-stable202311 title to get maintainer review and Stewards approval? For the merged code, I agree Lefi to revert it first. 

Leif:
  This is my mistake. I should set the correct access in the code freeze phase. For this patch, I suggest Abner goes through the critical patch review process if this patch needs to catch this stable tag. 

Thanks
Liming
> -----邮件原件-----
> 发件人: Kinney, Michael D <michael.d.kinney@intel.com>
> 发送时间: 2023年11月16日 1:29
> 收件人: Chang, Abner <Abner.Chang@amd.com>; Leif Lindholm
> <quic_llindhol@quicinc.com>; devel@edk2.groups.io; Mike Maslenkin
> <mike.maslenkin@gmail.com>; igork@ami.com; Gao, Liming
> <gaoliming@byosoft.com.cn>
> 抄送: Nickle Wang <nicklew@nvidia.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> 主题: RE: edk2-stable202311 Code freeze process violation Re: [edk2-devel]
> [PATCH v5 2/2] RedfishPkg: RedfishDiscoverDxe: Optimize the Redfish Discover
> flow
> 
> Hello,
> 
> Looks like the process for permissions needs to be adjusted during soft/hard
> freeze.
> 
> Liming reduced EDK II Maintainers team to "Triage" for edk2 repo.
> 
> But from this documentation lists Triage as allowing add/remove labels.
> 
> https://docs.github.com/en/organizations/managing-user-access-to-your-org
> anizations-repositories/managing-repository-roles/repository-roles-for-an-org
> anization
> 
> Looks like reducing to EDK II Maintainer team to "Read" is the right setting for
> soft/hard freeze.
> 
> Mike
> 
> 
> > -----Original Message-----
> > From: Chang, Abner <Abner.Chang@amd.com>
> > Sent: Wednesday, November 15, 2023 3:55 AM
> > To: Leif Lindholm <quic_llindhol@quicinc.com>; devel@edk2.groups.io;
> > 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: RE: edk2-stable202311 Code freeze process violation Re:
> > [edk2-devel] [PATCH v5 2/2] RedfishPkg: RedfishDiscoverDxe: Optimize
> > the Redfish Discover flow
> >
> > [AMD Official Use Only - General]
> >
> > Ok, Liming as we are going to revert that two commits. Is that
> > possible to wait for Igor sending out another patch to address Leif's
> > comment? This may delay the stable release a little bit.
> > As without this patch, users may encounter the problem when sending
> > request to Redfish service on the platform which have multiple NIC
> > installed,
> >
> > Let us know, thanks!
> > Abner
> >
> > > -----Original Message-----
> > > From: Chang, Abner
> > > Sent: Wednesday, November 15, 2023 7:07 PM
> > > To: Leif Lindholm <quic_llindhol@quicinc.com>; devel@edk2.groups.io;
> > 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: RE: edk2-stable202311 Code freeze process violation Re:
> > [edk2-
> > > devel] [PATCH v5 2/2] RedfishPkg: RedfishDiscoverDxe: Optimize the
> > Redfish
> > > Discover flow
> > >
> > > 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 (#111295): https://edk2.groups.io/g/devel/message/111295
Mute This Topic: https://groups.io/mt/102618505/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-