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 9C9599418B3 for ; Fri, 10 Nov 2023 13:53:46 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=S7zEkDFta37egErclweEM23TupjONZW51gd3PIsk8to=; c=relaxed/simple; d=groups.io; h=Date:From:To:CC:Subject:Message-ID:References:MIME-Version:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type:Content-Disposition; s=20140610; t=1699624425; v=1; b=kUK0g6cRJJVmA4OxMOopaaQaIzNRAeeQrYXKixFawqsbAfgKPpsy0hNvqG3xtUHrtNDM4rH6 L+cHyqBnTpFFBsT3J+v1TY0QYpVt7Et0YjpIrIrpBsYL1xyh7DYLZwplHWN+5NZapPYk8b6HXf7 T4r+IdGeEQg2WM7V+F+Mnpok= X-Received: by 127.0.0.2 with SMTP id L5j5YY7687511xFQ53IlB6Qt; Fri, 10 Nov 2023 05:53:45 -0800 X-Received: from mx0b-0031df01.pphosted.com (mx0b-0031df01.pphosted.com [205.220.180.131]) by mx.groups.io with SMTP id smtpd.web11.27808.1699624424507106682 for ; Fri, 10 Nov 2023 05:53:44 -0800 X-Received: from pps.filterd (m0279870.ppops.net [127.0.0.1]) by mx0a-0031df01.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 3AADdA4W000414; Fri, 10 Nov 2023 13:53:35 GMT X-Received: from nasanppmta01.qualcomm.com (i-global254.qualcomm.com [199.106.103.254]) by mx0a-0031df01.pphosted.com (PPS) with ESMTPS id 3u9g4n0u14-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 10 Nov 2023 13:53:34 +0000 X-Received: from nasanex01c.na.qualcomm.com (nasanex01c.na.qualcomm.com [10.45.79.139]) by NASANPPMTA01.qualcomm.com (8.17.1.5/8.17.1.5) with ESMTPS id 3AADrXcG008649 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 10 Nov 2023 13:53:33 GMT X-Received: from qc-i7.hemma.eciton.net (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; Fri, 10 Nov 2023 05:53:31 -0800 Date: Fri, 10 Nov 2023 13:53:28 +0000 From: "Leif Lindholm" To: "Chang, Abner" CC: "devel@edk2.groups.io" , "igork@ami.com" , Nickle Wang , "gaoliming@byosoft.com.cn" Subject: Re: [edk2-devel] [PATCH v3] RedfishPkg: RedfishDiscoverDxe: Fix issue if IPv4 installed after RestEx Message-ID: References: <20231107120605.2035-1-igork@ami.com> <1371cdaf-9a6a-4e63-8265-2ad0d6e62fb2@quicinc.com> MIME-Version: 1.0 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-ORIG-GUID: FQXxgV5hB213lS8HARanV9hqp4ees9Uk X-Proofpoint-GUID: FQXxgV5hB213lS8HARanV9hqp4ees9Uk 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: rEhPfpF27sVXCGGirE8imP5rx7686176AA= Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=kUK0g6cR; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=quicinc.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io On Fri, Nov 10, 2023 at 00:39:53 +0000, Chang, Abner wrote: > [AMD Official Use Only - General] > > Thanks Leif, some responses are given in line. > As we would like to have this be part of edk2-stable202312, we > prefer letting this change gets in stable release first and address > the comment in another patch afterward. > > Hi Igor, > Could you please check my comment and help to send another patch after edk2-stable202312? > > Thanks > Abner > > > > -----Original Message----- > > From: Leif Lindholm > > Sent: Thursday, November 9, 2023 11:12 PM > > To: devel@edk2.groups.io; igork@ami.com > > Cc: Chang, Abner ; Nickle Wang > > ; gaoliming@byosoft.com.cn > > Subject: Re: [edk2-devel] [PATCH v3] RedfishPkg: RedfishDiscoverDxe: Fix > > issue if IPv4 installed after RestEx > > > > Caution: This message originated from an External Source. Use proper caution > > when opening attachments, clicking links, or responding. > > > > > > On 2023-11-07 12:06, Igor Kulchytskyy via groups.io wrote: > > > Supported function of the driver changed to wait for all newtwork > > > > Typo: > > newtwork -> > > network > > > > > interface to be installed. > > > Filer out the network interfaces which are not supported by > > > > Filer -> > > Filter > > > > > Redfish Host Interface. > > > > These sound like two separate changes? > This is for the same issue. That means it should be in the same set, not in the same patch. > > > Cc: Abner Chang > > > Cc: Nickle Wang > > > Signed-off-by: Igor Kulchytskyy > > > --- > > > RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c | 165 > > ++++++++++++++------ > > > 1 file changed, 117 insertions(+), 48 deletions(-) > > > > > > diff --git a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c > > b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c > > > index 23da3b968f..85e47843e4 100644 > > > --- a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c > > > +++ b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c > > > @@ -322,9 +322,16 @@ GetTargetNetworkInterfaceInternal ( > > > { > > > EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_INTERNAL > > *ThisNetworkInterface; > > > > > > + if (IsListEmpty (&mEfiRedfishDiscoverNetworkInterface)) { > > > + return NULL; > > > + } > > > + > > > ThisNetworkInterface = > > (EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_INTERNAL *)GetFirstNode > > (&mEfiRedfishDiscoverNetworkInterface); > > > while (TRUE) { > > > - if (CompareMem ((VOID *)&ThisNetworkInterface->MacAddress, > > &TargetNetworkInterface->MacAddress, ThisNetworkInterface- > > >HwAddressSize) == 0) { > > > + if ((CompareMem ((VOID *)&ThisNetworkInterface->MacAddress, > > &TargetNetworkInterface->MacAddress, ThisNetworkInterface- > > >HwAddressSize) == 0) && > > > + ((TargetNetworkInterface->IsIpv6 && (ThisNetworkInterface- > > >NetworkProtocolType == ProtocolTypeTcp6)) || > > > + (!TargetNetworkInterface->IsIpv6 && (ThisNetworkInterface- > > >NetworkProtocolType == ProtocolTypeTcp4)))) > > > + { > > > > This could really benefit from some helper macros. > > e.g. if the test could look like > > Hi Igor, > Could we consider this suggestion after stable release? > > > > > if ((MAC_COMPARE (ThisNetworkInterface, TargetNetworkInterface) == 0) && > > (VALID_TCP6 (TargetNetworkInterface, ThisNetworkInterface) || > > VALID_TCP4 (TargetNetworkInterface, ThisNetworkInterface))) { > > > > > return ThisNetworkInterface; > > > } > > > > > > @@ -354,6 +361,10 @@ GetTargetNetworkInterfaceInternalByController ( > > > { > > > EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_INTERNAL > > *ThisNetworkInterface; > > > > > > + if (IsListEmpty (&mEfiRedfishDiscoverNetworkInterface)) { > > > + return NULL; > > > + } > > > + > > > ThisNetworkInterface = > > (EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_INTERNAL *)GetFirstNode > > (&mEfiRedfishDiscoverNetworkInterface); > > > while (TRUE) { > > > if (ThisNetworkInterface->OpenDriverControllerHandle == > > ControllerHandle) { > > > @@ -476,6 +487,42 @@ CheckIsIpVersion6 ( > > > return FALSE; > > > } > > > > > > +/** > > > + This function returns the IP type supported by the Host Interface. > > > + > > > + @retval 00h is Unknown > > > + 01h is Ipv4 > > > + 02h is Ipv6 > > > + > > > +**/ > > > +UINT8 > > > > If this is just a local helper function, we probably want it STATIC. > However, it is recommended that not using STATIC for local function > for the source level debug purpose in 5.4.2.2.2 in edk2 C coding > standard spec. Yeah, we need to fix that doc. I have no idea what decade that comment applied to, but I bet the year number started with a 1. > > > +GetHiIpProtocolType ( > > > + VOID > > > + ) > > > +{ > > > + EFI_STATUS Status; > > > + REDFISH_OVER_IP_PROTOCOL_DATA *Data; > > > + REDFISH_INTERFACE_DATA *DeviceDescriptor; > > > + > > > + Data = NULL; > > > + DeviceDescriptor = NULL; > > > + if (mSmbios == NULL) { > > > + Status = gBS->LocateProtocol (&gEfiSmbiosProtocolGuid, NULL, (VOID > > **)&mSmbios); > > > + if (EFI_ERROR (Status)) { > > > + return > > REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_UNKNOWN; > > > + } > > > + } > > > + > > > + Status = RedfishGetHostInterfaceProtocolData (mSmbios, > > &DeviceDescriptor, &Data); // Search for SMBIOS type 42h > > > + if (!EFI_ERROR (Status) && (Data != NULL) && > > > + (Data->HostIpAssignmentType == RedfishHostIpAssignmentStatic)) > > > + { > > > + return Data->HostIpAddressFormat; > > > + } > > > + > > > + return > > REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_UNKNOWN; > > > +} > > > + > > > /** > > > This function discover Redfish service through SMBIOS host interface. > > > > > > @@ -512,6 +559,18 @@ DiscoverRedfishHostInterface ( > > > > > > Status = RedfishGetHostInterfaceProtocolData (mSmbios, > > &DeviceDescriptor, &Data); // Search for SMBIOS type 42h > > > if (!EFI_ERROR (Status) && (Data != NULL) && (DeviceDescriptor != NULL)) > > { > > > + if ((Instance->NetworkInterface->NetworkProtocolType == > > ProtocolTypeTcp4) && > > > + (Data->HostIpAddressFormat != > > REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_IP4)) // IPv4 case > > > + { > > > + DEBUG ((DEBUG_ERROR, "%a: Network Interface is IPv4, but Host > > Interface requires Ipv6\n", __func__)); > > > + return EFI_UNSUPPORTED; > > > + } else if ((Instance->NetworkInterface->NetworkProtocolType == > > ProtocolTypeTcp6) && > > > + (Data->HostIpAddressFormat != > > REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_IP6)) // IPv6 case > > > + { > > > + DEBUG ((DEBUG_ERROR, "%a: Network Interface is IPv6, but Host > > Interface requires IPv4\n", __func__)); > > > + return EFI_UNSUPPORTED; > > > + } > > > + > > > // > > > // Check if we can reach out Redfish service using this network interface. > > > // Check with MAC address using Device Descriptor Data Device Type 04 > > and Type 05. > > > @@ -1102,6 +1161,7 @@ RedfishServiceGetNetworkInterface ( > > > OUT EFI_REDFISH_DISCOVER_NETWORK_INTERFACE > > **NetworkIntfInstances > > > ) > > > { > > > + EFI_STATUS Status; > > > EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_INTERNAL > > *ThisNetworkInterfaceIntn; > > > EFI_REDFISH_DISCOVER_NETWORK_INTERFACE > > *ThisNetworkInterface; > > > EFI_REDFISH_DISCOVER_REST_EX_INSTANCE_INTERNAL > > *RestExInstance; > > > @@ -1141,13 +1201,23 @@ RedfishServiceGetNetworkInterface ( > > > > > > ThisNetworkInterfaceIntn = > > (EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_INTERNAL *)GetFirstNode > > (&mEfiRedfishDiscoverNetworkInterface); > > > while (TRUE) { > > > + // If Get Subnet Info failed then skip this interface > > > + Status = NetworkInterfaceGetSubnetInfo (ThisNetworkInterfaceIntn, > > ImageHandle); // Get subnet info > > > + if (EFI_ERROR (Status)) { > > > + if (IsNodeAtEnd (&mEfiRedfishDiscoverNetworkInterface, > > &ThisNetworkInterfaceIntn->Entry)) { > > > + break; > > > + } > > > + > > > + ThisNetworkInterfaceIntn = > > (EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_INTERNAL *)GetNextNode > > (&mEfiRedfishDiscoverNetworkInterface, &ThisNetworkInterfaceIntn->Entry); > > > + continue; > > > + } > > > + > > > ThisNetworkInterface->IsIpv6 = FALSE; > > > if (CheckIsIpVersion6 (ThisNetworkInterfaceIntn)) { > > > ThisNetworkInterface->IsIpv6 = TRUE; > > > } > > > > > > CopyMem ((VOID *)&ThisNetworkInterface->MacAddress, > > &ThisNetworkInterfaceIntn->MacAddress, ThisNetworkInterfaceIntn- > > >HwAddressSize); > > > - NetworkInterfaceGetSubnetInfo (ThisNetworkInterfaceIntn, > > ImageHandle); // Get subnet info. > > > if (!ThisNetworkInterface->IsIpv6) { > > > IP4_COPY_ADDRESS (&ThisNetworkInterface->SubnetId.v4, > > &ThisNetworkInterfaceIntn->SubnetAddr.v4); // IPv4 subnet information. > > > } else { > > > @@ -1230,7 +1300,12 @@ RedfishServiceAcquireService ( > > > > > > if (TargetNetworkInterface != NULL) { > > > TargetNetworkInterfaceInternal = GetTargetNetworkInterfaceInternal > > (TargetNetworkInterface); > > > - NumNetworkInterfaces = 1; > > > + if (TargetNetworkInterfaceInternal == NULL) { > > > + DEBUG ((DEBUG_ERROR, "%a:No network interface on platform.\n", > > __func__)); > > > + return EFI_UNSUPPORTED; > > > + } > > > + > > > + NumNetworkInterfaces = 1; > > > } else { > > > TargetNetworkInterfaceInternal = > > (EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_INTERNAL *)GetFirstNode > > (&mEfiRedfishDiscoverNetworkInterface); > > > NumNetworkInterfaces = NumberOfNetworkInterface (); > > > @@ -1260,7 +1335,13 @@ RedfishServiceAcquireService ( > > > // Get subnet information in case subnet information is not set because > > > // RedfishServiceGetNetworkInterfaces hasn't been called yet. > > > // > > > - NetworkInterfaceGetSubnetInfo (TargetNetworkInterfaceInternal, > > ImageHandle); > > > + Status1 = NetworkInterfaceGetSubnetInfo > > (TargetNetworkInterfaceInternal, ImageHandle); > > > + if (EFI_ERROR (Status1)) { > > > + DEBUG ((DEBUG_ERROR, "%a: Get subnet information fail.\n", > > __func__)); > > > + FreePool (Instance); > > > + continue; > > > + } > > > + > > > NewInstance = TRUE; > > > } > > > > > > @@ -1547,25 +1628,26 @@ TestForRequiredProtocols ( > > > ControllerHandle, > > > EFI_OPEN_PROTOCOL_TEST_PROTOCOL > > > ); > > > + if (EFI_ERROR (Status)) { > > > + return EFI_UNSUPPORTED; > > > + } > > > + > > > + Status = gBS->OpenProtocol ( > > > + ControllerHandle, > > > + gRequiredProtocol[Index].DiscoveredProtocolGuid, > > > + (VOID **)&Id, > > > + This->DriverBindingHandle, > > > + ControllerHandle, > > > + EFI_OPEN_PROTOCOL_GET_PROTOCOL > > > + ); > > > if (!EFI_ERROR (Status)) { > > > - Status = gBS->OpenProtocol ( > > > - ControllerHandle, > > > - gRequiredProtocol[Index].DiscoveredProtocolGuid, > > > - (VOID **)&Id, > > > - This->DriverBindingHandle, > > > - ControllerHandle, > > > - EFI_OPEN_PROTOCOL_GET_PROTOCOL > > > - ); > > > - if (EFI_ERROR (Status)) { > > > - if (Index == ListCount - 1) { > > > - DEBUG ((DEBUG_INFO, "%a: all required protocols are found on this > > controller handle: %p.\n", __func__, ControllerHandle)); > > > - return EFI_SUCCESS; > > > - } > > > - } > > > + // Already installed > > > + return EFI_UNSUPPORTED; > > > } > > > } > > > > > > - return EFI_UNSUPPORTED; > > > + DEBUG ((DEBUG_MANAGEABILITY, "%a: all required protocols are found > > on this controller handle: %p.\n", __func__, ControllerHandle)); > > > + return EFI_SUCCESS; > > > } > > > > > > /** > > > @@ -1600,10 +1682,24 @@ 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 (((gRequiredProtocol[Index].ProtocolType == ProtocolTypeTcp4) && > > (IpType == REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_IP6)) || > > > + ((gRequiredProtocol[Index].ProtocolType == ProtocolTypeTcp6) && > > (IpType == REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_IP4))) > > > > Similarly, this becomes a bit of a handful. > Igor, could we consider using MACRO for this as well? > > > > > But it also feels like the logic is incorrect? > > I.e. it won't filter out entries where IpType == > > REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_UNKNOWN, or > > invalid? > > > > Shouldn't the test be > > > > ((gRequiredProtocol[Index].ProtocolType == ProtocolTypeTcp4) && > > (IpType != REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_IP4)) > > > > and > > > > ((gRequiredProtocol[Index].ProtocolType == ProtocolTypeTcp6) && > > (IpType != REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_IP6))) > > I think above is the error condition, thus there is a "continue" to skip this entry. Yes, but it's checking whether the error is one of two possible error conditions instead of checking for whether the inputs are valid. That's a bug. / Leif > Thanks > Abner > > > > > Either way, those tests could probably also do with being turned into > > macros to improve readability. > > > > / > > Leif -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#111037): https://edk2.groups.io/g/devel/message/111037 Mute This Topic: https://groups.io/mt/102441003/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-