On Wed, Nov 1, 2023 at 6:24 AM Igor Kulchytskyy <ig...@ami.com> wrote:
>
> Hi Mike,
> Thank you for review.
> Please see my answers below the text.
>
> -----Original Message-----
> From: Mike Maslenkin <mike.maslen...@gmail.com>
> Sent: Tuesday, October 31, 2023 9:00 PM
> To: devel@edk2.groups.io; Igor Kulchytskyy <ig...@ami.com>
> Cc: Abner Chang <abner.ch...@amd.com>; Nickle Wang <nick...@nvidia.com>
> Subject: [EXTERNAL] Re: [edk2-devel] [PATCH] RedfishPkg: RedfishDiscoverDxe:
> Fix issue if IPv4 installed after RestEx
>
>
> **CAUTION: The e-mail below is from an external source. Please exercise
> caution before opening attachments, clicking links, or following guidance.**
>
> Hi Igor
>
> please find my comments below.
>
> On Tue, Oct 31, 2023 at 8:56 PM Igor Kulchytskyy via groups.io
> <igork=ami....@groups.io> wrote:
> >
> > Supported function of the driver changed to wait for all newtwork
> > interface to be installed.
> > Filer out the network interfaces which are not supported by
> > Redfish Host Interface.
> >
> > Cc: Abner Chang <abner.ch...@amd.com>
> > Cc: Nickle Wang <nick...@nvidia.com>
> > Cc: Igor Kulchytskyy <ig...@ami.com>
> > Signed-off-by: Igor Kulchytskyy <ig...@ami.com>
> > ---
> > RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c | 96
> > ++++++++++++++++++--
> > 1 file changed, 89 insertions(+), 7 deletions(-)
> >
> > diff --git a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> > b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> > index 23da3b968f..a88ad55938 100644
> > --- a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> > +++ b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> > @@ -322,9 +322,15 @@ 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))) {
> > return ThisNetworkInterface;
> > }
> >
> > @@ -354,6 +360,10 @@ GetTargetNetworkInterfaceInternalByController (
> > {
> > EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_INTERNAL *ThisNetworkInterface;
> >
> > + if (IsListEmpty(&mEfiRedfishDiscoverNetworkInterface)) {
> > + return NULL;
> > + }
> > +
>
> I also have these two hunks in my pending list.
> But I suggest to add ASSERT to GetTargetNetworkInterfaceInternal, just
> because currently it is really impossible situation,
> and mEfiRedfishDiscoverNetworkInterface was checked before in scope of
> ValidateTargetNetworkInterface().
>
> Igor: Agree.
> I also just noticed that even if GetTargetNetworkInterfaceInternal function
> may return NULL, the return value is not verified on NULL in
> RedfishServiceAcquireService function.
> I think we should add this verification.
Mike: Agreed. I missed that the check in ValidateTargetNetworkInterface is:
if (IsListEmpty (&mEfiRedfishDiscoverNetworkInterface) &&
(TargetNetworkInterface == NULL))
not a just a IsListEmpty.
And ValidateTargetNetworkInterface is called before
GetTargetNetworkInterfaceInternal()...
But for now don't we need to add additional check for the NULL value
returned from GetTargetNetworkInterfaceInternal() in a caller ?
In RedfishServiceAcquireService():
TargetNetworkInterfaceInternal = GetTargetNetworkInterfaceInternal
(TargetNetworkInterface);
The NULL value of TargetNetworkInterfaceInternal is propagated down.
So on some code branch it can not cause a problems (hopefully because
of same IsListEmpty checks), but it can on another.
>
>
> Also I wonder why check patch doesn't complain about missed spaces in
> "IsListEmpty (&mEfiRedfishDiscoverNetworkInterface)" call for example.
>
> > ThisNetworkInterface = (EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_INTERNAL
> > *)GetFirstNode (&mEfiRedfishDiscoverNetworkInterface);
> > while (TRUE) {
> > if (ThisNetworkInterface->OpenDriverControllerHandle ==
> > ControllerHandle) {
> > @@ -476,6 +486,38 @@ CheckIsIpVersion6 (
> > return FALSE;
> > }
> >
> > +/**
> > + This function returns the IP type supported by the Host Interface
> > +
> > + @retval IP Type
> > + // Unknown=00h,
> > + // Ipv4=01h,
> > + // Ipv6=02h,
> > +
> > +**/
> > +UINT8
>
> STATIC ?
> Igor: WHY?
Mike: Just because it is local function, No need to grep over
RedfishPkg to find all its implementations if any.
>
> > +GetHiIpProtocolType()
> > +{
> > + 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 0;
>
> In this driver REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_IP{4,6}
> used and accessible.
> so, 0 means REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_UNKNOWN
> And these values actually checked in scope of BuildupNetworkInterface() below.
> Igor: Thank you. Missed that macro.
>
> > + }
> > + }
> > + Status = RedfishGetHostInterfaceProtocolData (mSmbios,
> > &DeviceDescriptor, &Data); // Search for SMBIOS type 42h
> > + if (!EFI_ERROR (Status) && (Data != NULL) &&
> > + (Data->HostIpAssignmentType == RedfishHostIpAssignmentStatic)) {
> > + return Data->HostIpAddressFormat;
> > + }
> > + return 0;
>
> Same here, 0 is a magic value for
> REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_UNKNOWN
>
> > +}
> > +
> > /**
> > This function discover Redfish service through SMBIOS host interface.
> >
> > @@ -512,6 +554,15 @@ 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"));
>
> Missed argument for %a format
> Missed space "DEBUG (("
>
> Igor: Inattentiveness. Thank you.
>
> > + 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"));
>
> Missed argument for %a format
> Missed space "DEBUG (("
>
> Igor: Inattentiveness. Thank you.
>
> > + 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 +1153,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,6 +1193,16 @@ RedfishServiceGetNetworkInterface (
> >
> > ThisNetworkInterfaceIntn =
> > (EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_INTERNAL *)GetFirstNode
> > (&mEfiRedfishDiscoverNetworkInterface);
> > while (TRUE) {
> > + // If Get Subnet Info 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;
> > @@ -1260,7 +1322,12 @@ 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;
> > }
> >
> > @@ -1535,7 +1602,7 @@ TestForRequiredProtocols (
> > UINT32 *Id;
> > UINTN Index;
> > EFI_STATUS Status;
> > - UINTN ListCount;
> > + UINTN ListCount, SuccessfulCount = 0;
> >
> > ListCount = (sizeof (gRequiredProtocol) / sizeof
> > (REDFISH_DISCOVER_REQUIRED_PROTOCOL));
> > for (Index = 0; Index < ListCount; Index++) {
> > @@ -1557,13 +1624,14 @@ TestForRequiredProtocols (
> > 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;
> > - }
> > + SuccessfulCount++;
> > }
> It may be SuccessfulCount must have one indentation less, i.e in scope
> of upper "if (!EFI_ERROR (Status)) {", not in scope of "if (EFI_ERROR
> (Status)) {" in context.
>
> Igor: We need TestForRequiredProtocols return SUCCESS when all protocols
> listed in gRequiredProtocol installed.
> When this happened the BuildupNetworkInterface function would be called, and
> we iterate through gRequiredProtocol array and build network interfaces.
> We also install DiscoveredProtocolGuid to prevent BuildupNetworkInterface to
> be called after that.
> If we have SuccessfulCount in scope of upper "if (!EFI_ERROR (Status)) {",
> then BuildupNetworkInterface will be called if something installed on that
> ControllerHandle, like DNS protocol, for example.
>
> > }
> > }
> > + if (ListCount == SuccessfulCount) {
> > + DEBUG ((DEBUG_INFO, "%a: all required protocols are found on this
> > controller handle: %p.\n", __func__, ControllerHandle));
> > + return EFI_SUCCESS;
> > + }
> >
> > return EFI_UNSUPPORTED;
> > }
> > @@ -1600,10 +1668,24 @@ BuildupNetworkInterface (
> > EFI_REDFISH_DISCOVER_REST_EX_INSTANCE_INTERNAL *RestExInstance;
> > EFI_TPL OldTpl;
> > BOOLEAN
> > NewNetworkInterfaceInstalled;
> > + UINT8 IpType;
> >
> > NewNetworkInterfaceInstalled = FALSE;
> > Index = 0;
> > +
> > +
> > + // Get IP Type to filter out unnecessary network protocol if possible
> > + IpType = GetHiIpProtocolType();
> > +
> > do {
> > + // Check IP Type and skip an unnecessary network protocol if does not
> > match
> > + if (IpType != 0) {
>
> IpType is invariant to this loop.
> Eiher we may check it's value before the loop or we can just drop it,
> because "&& IpType ==
> REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_IP{4,6}"
> is equal to "IpType != 0" Do I miss something?
>
> Igor: You are right. Can be dropped.
>
> > + 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)) {
> > + Index++;
>
> Ops. You can not do just { Index++;continue }
> You need
> {
> Index++;
> if (Index == (sizeof (gRequiredProtocol) / sizeof
> (REDFISH_DISCOVER_REQUIRED_PROTOCOL))) {
> break;
> }
> continue;
> }
>
> Igor: continue statement causes the program control to pass to the
> conditional tests. So, Index will be verified in while check.
Mike: Please disregard my comment :)
Definitely yes. I messed things up a bit, when I saw that this complex
loop become a bit more complex.
It's not clear why original code contains those checks and explicit breaks.
So, I rewrote this do while, to a simple for() iterating on index <
ARRAY_SIZE() similar to loop in TestForRequiredProtocols()
>
> It's a terrible function. I have a patch, that improves this loop:
> https://github.com/ghbaccount/edk2/commit/adc3218d3d96083c85cef6c396dde8ddb96530fb
>
> > + continue;
> > + }
> > + }
> > Status = gBS->OpenProtocol (
> > // Already in list?
> > ControllerHandle,
> > --
> > 2.37.1.windows.1
>
> Regards,
> Mike.
> -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.
Regards,
Mike.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110472): https://edk2.groups.io/g/devel/message/110472
Mute This Topic: https://groups.io/mt/102303105/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
