From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 78C68D80A13 for ; Wed, 15 Nov 2023 10:56:08 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=/NyJRQGGBAeqKAu4ZFZ3QIXEX7fSmceECIhS3so9GGg=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:User-Agent:Subject:To:CC:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1700045766; v=1; b=hq67x1wAJnWoxLozeA6uyo3dldBDCyuO9S2wq2LCl9uUXEwTalxHiYe2iH0YQOisvNCnixnW 8IksHc/5RygHQQ75dwL3NhP24mY5XcFnwXmI73jDe7iC5WjoIYvasCN8GBapg3QOg/mFGbr3IE1 kRINPizFCRXvLwkg7yeXE3IQ= X-Received: by 127.0.0.2 with SMTP id Gz1iYY7687511xQhcloybBZ8; Wed, 15 Nov 2023 02:56:06 -0800 X-Received: from mx0a-0031df01.pphosted.com (mx0a-0031df01.pphosted.com [205.220.168.131]) by mx.groups.io with SMTP id smtpd.web11.10513.1700045766419259633 for ; Wed, 15 Nov 2023 02:56:06 -0800 X-Received: from pps.filterd (m0279865.ppops.net [127.0.0.1]) by mx0a-0031df01.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 3AF8OZPd020708; Wed, 15 Nov 2023 10:55:56 GMT X-Received: from nasanppmta05.qualcomm.com (i-global254.qualcomm.com [199.106.103.254]) by mx0a-0031df01.pphosted.com (PPS) with ESMTPS id 3ucrb2gjgm-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 15 Nov 2023 10:55:56 +0000 X-Received: from nasanex01c.na.qualcomm.com (nasanex01c.na.qualcomm.com [10.45.79.139]) by NASANPPMTA05.qualcomm.com (8.17.1.5/8.17.1.5) with ESMTPS id 3AFAttVp006654 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 15 Nov 2023 10:55:55 GMT X-Received: from [10.251.43.200] (10.80.80.8) by nasanex01c.na.qualcomm.com (10.45.79.139) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.1118.39; Wed, 15 Nov 2023 02:55:53 -0800 Message-ID: <1a48eddc-4047-498e-ab7f-3d388ad2ace6@quicinc.com> Date: Wed, 15 Nov 2023 10:55:50 +0000 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [edk2-devel] [PATCH v5 2/2] RedfishPkg: RedfishDiscoverDxe: Optimize the Redfish Discover flow To: , , Mike Maslenkin , "igork@ami.com" , "Gao, Liming" , "Kinney, Michael D" CC: Nickle Wang References: <20231114142815.1604-1-igork@ami.com> <3e8c08dc-a665-41f1-8c51-d4e2743bdac1@quicinc.com> From: "Leif Lindholm" In-Reply-To: X-Originating-IP: [10.80.80.8] X-ClientProxiedBy: nasanex01b.na.qualcomm.com (10.46.141.250) To nasanex01c.na.qualcomm.com (10.45.79.139) X-QCInternal: smtphost X-Proofpoint-GUID: 1oC4jBZWlaRwftrP5_69hXLOQ4wStxyv X-Proofpoint-ORIG-GUID: 1oC4jBZWlaRwftrP5_69hXLOQ4wStxyv X-MIME-Autoconverted: from 8bit to quoted-printable by mx0a-0031df01.pphosted.com id 3AF8OZPd020708 Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,quic_llindhol@quicinc.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: hSggvHE3d1Pkw9KBtT9FupLVx7686176AA= Content-Language: en-GB Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=hq67x1wA; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=quicinc.com (policy=none) On 2023-11-15 01:19, Chang, Abner via groups.io wrote: > [AMD Official Use Only - General] >=20 > Hi Mike and Leif, > Thanks for your comments on this change. As we are rushing to get this ch= ange to be pulled in stable release 202312 this week, I will just merge thi= s code to master branch and let the discussing keeps going. > I think there is no functionality difference base on your suggestions, bu= t it's about the coding practice and readability. Readability is not optional. If the fix is important enough, we need to delay the stable tag, not=20 ignore the process. / Leif >=20 > Hi Igor, > Could you please resend the V6 after stable tag is released if Mike and L= eif's comment is reasonable to you? >=20 > Thanks > Abner >=20 >> -----Original Message----- >> From: Mike Maslenkin >> Sent: Wednesday, November 15, 2023 7:53 AM >> To: devel@edk2.groups.io; igork@ami.com >> Cc: Leif Lindholm ; Chang, Abner >> ; Nickle Wang >> 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 cau= tion >> when opening attachments, clicking links, or responding. >> >> >> On Tue, Nov 14, 2023 at 9:57=E2=80=AFPM Igor Kulchytskyy via groups.io >> wrote: >>> >>> Hi Leif, >>> Please see my comments below. >>> Thank you, >>> Igor >>> >>> >>> -----Original Message----- >>> From: Leif Lindholm >>> Sent: Tuesday, November 14, 2023 12:26 PM >>> To: devel@edk2.groups.io; Igor Kulchytskyy >>> Cc: Abner Chang ; Nickle Wang >> >>> 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 guidanc= e.** >>> >>> 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 >>>> Cc: Nickle Wang >>>> Signed-off-by: Igor Kulchytskyy >>>> --- >>>> 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 NewNetworkInterf= aceInstalled; >>>> + UINT8 IpType; >>>> + UINTN ListCount; >>>> >>>> + ListCount =3D (sizeof (gRequiredProtocol) / size= of >> (REDFISH_DISCOVER_REQUIRED_PROTOCOL)); >>>> NewNetworkInterfaceInstalled =3D FALSE; >>>> Index =3D 0; >>>> - do { >>>> + >>>> + // Get IP Type to filter out unnecessary network protocol if possib= le >>>> + IpType =3D GetHiIpProtocolType (); >>>> + >>>> + for (Index =3D 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 =3D 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 =3D=3D ProtocolTypeTcp6)) >>>> +#define VALID_TCP4(TargetNetworkInterface, ThisNetworkInterface) >> (!TargetNetworkInterface->IsIpv6 && (ThisNetworkInterface- >>> NetworkProtocolType =3D=3D ProtocolTypeTcp4)) >>>> +#define IS_TCP4_MATCH(IpType) >> ((gRequiredProtocol[Index].ProtocolType =3D=3D ProtocolTypeTcp4) && (IpT= ype !=3D >> REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_IP4)) >>>> +#define IS_TCP6_MATCH(IpType) >> ((gRequiredProtocol[Index].ProtocolType =3D=3D ProtocolTypeTcp6) && (IpT= ype !=3D >> REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_IP6)) >>> >>> And these macros to test for =3D=3D, not !=3D >>> >>> >>> Igor: First version tested "=3D=3D", 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, bu= t >>> 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 t= o: >> >> if (IpType =3D=3D >> REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_UNKNOWN) { >> // The protocol type is not specified in SMBIOS table type 42h >> return EFI_UNSUPPORTED; >> } >> >> for (Index =3D 0; Index < ListCount; Index++) { >> if ((gRequiredProtocol[Index].ProtocolType =3D=3D ProtocolTypeTcp4) &= & >> (IpType !=3D REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_IP4)) { >> continue; >> } >> if ((gRequiredProtocol[Index].ProtocolType =3D=3D ProtocolTypeTcp6) &= & >> (IpType !=3D REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_IP6)) { >> continue; >> } >> >> >> Regards, >> Mike. >=20 >=20 >=20 >=20 >=20 -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#111253): https://edk2.groups.io/g/devel/message/111253 Mute This Topic: https://groups.io/mt/102584140/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/19134562= 12/xyzzy [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-