[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 <quic_llind...@quicinc.com>
> Sent: Thursday, November 9, 2023 11:12 PM
> To: devel@edk2.groups.io; ig...@ami.com
> Cc: Chang, Abner <abner.ch...@amd.com>; Nickle Wang
> <nick...@nvidia.com>; gaolim...@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.
>
> > 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 | 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.
>
> > +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.
Thanks
Abner
>
> Either way, those tests could probably also do with being turned into
> macros to improve readability.
>
> /
> Leif
>
> > + {
> > + continue;
> > + }
> > +
> > Status = gBS->OpenProtocol (
> > // Already in list?
> > ControllerHandle,
> > @@ -1614,11 +1710,6 @@ BuildupNetworkInterface (
> > EFI_OPEN_PROTOCOL_GET_PROTOCOL
> > );
> > if (!EFI_ERROR (Status)) {
> > - Index++;
> > - if (Index == (sizeof (gRequiredProtocol) / sizeof
> (REDFISH_DISCOVER_REQUIRED_PROTOCOL))) {
> > - break;
> > - }
> > -
> > continue;
> > }
> >
> > @@ -1631,11 +1722,6 @@ BuildupNetworkInterface (
> > EFI_OPEN_PROTOCOL_GET_PROTOCOL
> > );
> > if (EFI_ERROR (Status)) {
> > - Index++;
> > - if (Index == (sizeof (gRequiredProtocol) / sizeof
> (REDFISH_DISCOVER_REQUIRED_PROTOCOL))) {
> > - break;
> > - }
> > -
> > continue;
> > }
> >
> > @@ -1694,11 +1780,6 @@ BuildupNetworkInterface (
> > ProtocolDiscoverIdPtr
> > );
> > if (EFI_ERROR (Status)) {
> > - Index++;
> > - if (Index == (sizeof (gRequiredProtocol) / sizeof
> (REDFISH_DISCOVER_REQUIRED_PROTOCOL))) {
> > - break;
> > - }
> > -
> > continue;
> > }
> >
> > @@ -1755,25 +1836,13 @@ BuildupNetworkInterface (
> > }
> > } else {
> > DEBUG ((DEBUG_MANAGEABILITY, "%a: Not REST EX, continue with
> next\n", __func__));
> > - Index++;
> > - if (Index == (sizeof (gRequiredProtocol) / sizeof
> (REDFISH_DISCOVER_REQUIRED_PROTOCOL))) {
> > - break;
> > - }
> > -
> > continue;
> > }
> > }
> >
> > return Status;
> > - } else {
> > - Index++;
> > - if (Index == (sizeof (gRequiredProtocol) / sizeof
> (REDFISH_DISCOVER_REQUIRED_PROTOCOL))) {
> > - break;
> > - }
> > -
> > - continue;
> > }
> > - } while (Index < (sizeof (gRequiredProtocol) / sizeof
> (REDFISH_DISCOVER_REQUIRED_PROTOCOL)));
> > + }
> >
> > return EFI_DEVICE_ERROR;
> > }
> > --
> > 2.37.1.windows.1
> > -The information contained in this message may be confidential and
> proprietary to American Megatrends (AMI). This communication is intended
> to be read only by the individual or entity to whom it is addressed or by
> their
> designee. If the reader of this message is not the intended recipient, you are
> on notice that any distribution of this message, in any form, is strictly
> prohibited. Please promptly notify the sender by reply e-mail or by telephone
> at 770-246-8600, and then delete or destroy all copies of the transmission.
> >
> >
> >
> >
> >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111003): https://edk2.groups.io/g/devel/message/111003
Mute This Topic: https://groups.io/mt/102441003/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
