Skip to content

Console Variable fix for TestPointCheckLib#918

Open
shkrc wants to merge 1 commit intotianocore:masterfrom
shkrc:TpChkConVarFix
Open

Console Variable fix for TestPointCheckLib#918
shkrc wants to merge 1 commit intotianocore:masterfrom
shkrc:TpChkConVarFix

Conversation

@shkrc
Copy link
Contributor

@shkrc shkrc commented Dec 2, 2025

ConInDev, ConOutDev & ErrOutDev variables have list of device paths which have terminal console devices appended at the end. For only the user selected terminal type, the device path will have the necessary protocols installed at any point in time. TestPointCheckLib tries to discover these non existing terminal types and reports as an error. This patch fixes this by checking if the device path is a terminal type, and if so, then ignore the terminal node and only check for the existence of the parent path.

ConInDev, ConOutDev & ErrOutDev variables have list of device paths which have terminal console
devices appended at the end. For only the user selected terminal type, the device path will have
the necessary protocols installed at any point in time. TestPointCheckLib tries to discover these
non existing terminal types and reports as an error. This patch fixes this by checking if the
device path is a terminal type, and if so, then ignore the terminal node and only check for the existence
of the parent path.

Signed-off-by: Shankar C <shankar.c@amd.com>
@shkrc
Copy link
Contributor Author

shkrc commented Dec 2, 2025

@niruiyu requesting your reviews

Comment on lines +103 to +107
for (Index = 0; Index < ARRAY_SIZE (mTerminalGuids); Index++) {
if (CompareGuid (&((VENDOR_DEVICE_PATH *)Node)->Guid, mTerminalGuids[Index])) {
return TRUE;
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about returning TRUE without checking the Guid?
Then the logic would support any future extension of new terminal types.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @niruiyu , i guess we need to check for the guids, because we are ignoring the child nodes only in the cases where the devices path is of a terminal device, for other cases, we dont need to ignore the child nodes. Hence, I think its better to define the terminal types

Copy link
Member

@niruiyu niruiyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you check if the following logic works also?
I think it's simpler than the original logic.

BOOLEAN
IsDevicePathExist (
  IN EFI_DEVICE_PATH_PROTOCOL          *DevicePath
  )
{
  EFI_DEVICE_PATH_PROTOCOL *DevicePathInstance;
  EFI_DEVICE_PATH_PROTOCOL *RemainingDevicePath;
  EFI_DEVICE_PATH_PROTOCOL *RemainingNode;
  EFI_HANDLE               Handle;
  BOOLEAN                  Exist;

  Exist = FALSE;
  RemainingDevicePath = DevicePath;
  do {
    DevicePathInstance = GetNextDevicePathInstance (&RemainingDevicePath, &Size);
    //
    // RemainingDevicePath points to the next instance.
    // DevicePathInstance points to a copy of the current instance.
    //
    if (DevicePathInstance == NULL) {
      break;
    }
    Exist         = FALSE;
    RemainingNode = DevicePathInstance;
    Status = gBS->LocateDevicePath (
                    &gEfiDevicePathProtocolGuid,
                    &RemainingNode,
                    &Handle
                    );
    if (!EFI_ERROR (Status) && (IsDevicePathEnd (RemainingNode) || IsTerminalVendorNode (RemainingNode))) {
      //
      // A handle is found whose device path
      //   either equals to the DevicePathInstance (when IsDevicePathEnd() is TRUE)
      //   or equals to the DevicePathInstance - TerminalNode (when IsTerminalVendorNode() is TRUE)
      //
      Exist = TRUE;
    }
    FreePool (DevicePathInstance);
  } while (Exist && RemainingDevicePath!= NULL);

  return Exist;
}

@shkrc
Copy link
Contributor Author

shkrc commented Feb 27, 2026

Hi with the above code suggestion, the issue I see only IsDevicePathNodeExist() does the validation whether the device actually exists. In your suggestion
if (!EFI_ERROR (Status) && (IsDevicePathEnd (RemainingNode) || IsTerminalVendorNode (RemainingNode)
Lets assume there is a non-existing UART device that has a terminal child, then the above if condition would return true even when the device doesnt exist

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants