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_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.
That means it should be in the same set, not in the same patch.
> > > 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.
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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
