Re: [edk2-devel] [PATCH v3] RedfishPkg: RedfishDiscoverDxe: Fix issue if IPv4 installed after RestEx

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

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 <quic_llindhol@quicinc.com>
> > Sent: Thursday, November 9, 2023 11:12 PM
> > To: devel@edk2.groups.io; igork@ami.com
> > Cc: Chang, Abner <Abner.Chang@amd.com>; Nickle Wang
> > <nicklew@nvidia.com>; 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 <abner.chang@amd.com>
> > > Cc: Nickle Wang <nicklew@nvidia.com>
> > > Signed-off-by: Igor Kulchytskyy <igork@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.

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]
-=-=-=-=-=-=-=-=-=-=-=-