From 75fac0a105082293671a887a60671b5a54c7a842 Mon Sep 17 00:00:00 2001 From: Maisam Arif Date: Mon, 9 Jun 2025 23:44:09 -0500 Subject: [PATCH] Fixed Parser Folder Checking * Adjusted help text * Adjusted --afid to run only with --cper-file * Fixed interface return error Signed-off-by: Maisam Arif Change-Id: I2b96f4515c85f3b9dd84ba5c2d819729a997141b [ROCm/amdsmi commit: ac63f410c2da267c1c9b0048b258966382914766] --- .../amdsmi_cli/amdsmi_cli_exceptions.py | 10 ++++-- projects/amdsmi/amdsmi_cli/amdsmi_commands.py | 17 ++++++--- projects/amdsmi/amdsmi_cli/amdsmi_helpers.py | 16 +++++++-- projects/amdsmi/amdsmi_cli/amdsmi_parser.py | 36 +++++++++++-------- .../amdsmi/py-interface/amdsmi_interface.py | 2 +- projects/amdsmi/src/amd_smi/amd_smi.cc | 4 +-- 6 files changed, 58 insertions(+), 27 deletions(-) diff --git a/projects/amdsmi/amdsmi_cli/amdsmi_cli_exceptions.py b/projects/amdsmi/amdsmi_cli/amdsmi_cli_exceptions.py index ee4e823cb5..18e0cf4f91 100644 --- a/projects/amdsmi/amdsmi_cli/amdsmi_cli_exceptions.py +++ b/projects/amdsmi/amdsmi_cli/amdsmi_cli_exceptions.py @@ -98,7 +98,7 @@ class AmdSmiException(Exception): class AmdSmiInvalidCommandException(AmdSmiException): - def __init__(self, command, outputformat: str): + def __init__(self, command, outputformat: str, message=None): super().__init__() self.value = -1 self.command = command @@ -106,6 +106,9 @@ class AmdSmiInvalidCommandException(AmdSmiException): common_message = f"Command '{self.command}' is invalid. Run '--help' for more info." + if message: + common_message = message + self.json_message["error"] = common_message self.json_message["code"] = self.value self.csv_message = f"error,code\n{common_message}, {self.value}" @@ -152,7 +155,7 @@ class AmdSmiDeviceNotFoundException(AmdSmiException): class AmdSmiInvalidFilePathException(AmdSmiException): - def __init__(self, command, outputformat: str): + def __init__(self, command, outputformat: str, message=None): super().__init__() self.value = -4 self.command = command @@ -160,6 +163,9 @@ class AmdSmiInvalidFilePathException(AmdSmiException): common_message = f"Path '{self.command}' cannot be found." + if message: + common_message = message + self.json_message["error"] = common_message self.json_message["code"] = self.value self.csv_message = f"error,code\n{common_message}, {self.value}" diff --git a/projects/amdsmi/amdsmi_cli/amdsmi_commands.py b/projects/amdsmi/amdsmi_cli/amdsmi_commands.py index bfb89fd810..2c356dd9bc 100644 --- a/projects/amdsmi/amdsmi_cli/amdsmi_commands.py +++ b/projects/amdsmi/amdsmi_cli/amdsmi_commands.py @@ -29,7 +29,7 @@ import threading import time from _version import __version__ -from amdsmi_cli_exceptions import AmdSmiInvalidParameterException, AmdSmiRequiredCommandException +from amdsmi_cli_exceptions import AmdSmiInvalidParameterException, AmdSmiRequiredCommandException, AmdSmiInvalidCommandException from amdsmi_helpers import AMDSMIHelpers from amdsmi_logger import AMDSMILogger from amdsmi import amdsmi_exception, amdsmi_interface @@ -6496,10 +6496,17 @@ class AMDSMICommands(): if args.gpu == None: args.gpu = self.device_handles - if args.afid and args.cper_file: - afids = self.helpers.pvtDumpAfids(args.cper_file) - print(' '.join(map(str, afids))) - return + if args.afid: + if args.cper_file: + afids = self.helpers.pvtDumpAfids(args.cper_file) + print(' '.join(map(str, afids))) + return + else: + command = " ".join(sys.argv[1:]) + message = f"Command '{command}' requires '--cper-file'. Run '--help' for more info." + raise AmdSmiInvalidCommandException(command, + self.logger.format, + message) if not self.group_check_printed: self.helpers.check_required_groups() diff --git a/projects/amdsmi/amdsmi_cli/amdsmi_helpers.py b/projects/amdsmi/amdsmi_cli/amdsmi_helpers.py index 5ea8ada5d7..3c4b2483b0 100755 --- a/projects/amdsmi/amdsmi_cli/amdsmi_helpers.py +++ b/projects/amdsmi/amdsmi_cli/amdsmi_helpers.py @@ -1312,8 +1312,20 @@ class AMDSMIHelpers(): # assume it's already bytes raw = raw_data self.binary_to_hexdump_string(raw) - afids, num_afids = amdsmi_interface.amdsmi_get_afids_from_cper(raw) - return afids + try: + afids, num_afids = amdsmi_interface.amdsmi_get_afids_from_cper(raw) + return afids + except amdsmi_exception.AmdSmiLibraryException as e: + if e.get_error_code() == amdsmi_interface.amdsmi_wrapper.AMDSMI_STATUS_INVAL: + raise ValueError("Invalid CPER file inputs") from e + elif e.get_error_code() == amdsmi_interface.amdsmi_wrapper.AMDSMI_STATUS_UNEXPECTED_SIZE: + raise ValueError("Invalid CPER file data size") from e + elif e.get_error_code() == amdsmi_interface.amdsmi_wrapper.AMDSMI_STATUS_UNEXPECTED_DATA: + raise ValueError("Unexpected data in CPER file") from e + elif e.get_error_code() == amdsmi_interface.amdsmi_wrapper.AMDSMI_STATUS_NOT_SUPPORTED: + raise NotImplementedError("AFID decoding not supported") from e + else: + raise ValueError("Unexpected Error getting afids from CPER file") from e def ras_cper(self, args, device_handle, logger, gpu_idx): # Parse severity mask dynamically from the --severity option. diff --git a/projects/amdsmi/amdsmi_cli/amdsmi_parser.py b/projects/amdsmi/amdsmi_cli/amdsmi_parser.py index eb4a9b5077..3986845590 100644 --- a/projects/amdsmi/amdsmi_cli/amdsmi_parser.py +++ b/projects/amdsmi/amdsmi_cli/amdsmi_parser.py @@ -256,7 +256,12 @@ class AMDSMIParser(argparse.ArgumentParser): # Checks the values def __call__(self, parser, args, values, option_string=None): path = Path(values) - path.mkdir(parents=True, exist_ok=True) + try: + path.mkdir(parents=True, exist_ok=True) + except OSError as e: + raise amdsmi_cli_exceptions.AmdSmiInvalidFilePathException(path, + CheckOutputFilePath.outputformat, + f"Unable to make '{path}' a folder.") if not path.exists(): raise amdsmi_cli_exceptions.AmdSmiInvalidFilePathException(path, CheckOutputFilePath.outputformat) elif path.is_dir(): @@ -1424,35 +1429,36 @@ class AMDSMIParser(argparse.ArgumentParser): ras_optionals_title = "RAS arguments" # Help text for RAS arguments - cper_help = "Trigger CPER data retrieval" - afid_help = "Generate an AFID (AMD Field ID) using CPER record, which is similar to XID." + cper_help = "Trigger current CPER data retrieval" + afid_help = "Generate an AFID (AMD Field ID) using a CPER record, which is similar to XID." severity_choices = ["nonfatal-uncorrected", "fatal", "nonfatal-corrected", "all"] severity_choices_str = ", ".join(severity_choices) severity_help = f"Set the SEVERITY filters from the following:\n {severity_choices_str}" - folder_help = "Folder to dump CPER report files" - file_limit_help = "Maximum number of CPER files in target folder\n Older files beyond limit will be deleted" - cper_file_help = "Full path of the cper record file to generate the AFID" - follow_help = "Continuously monitor for new entries" + folder_help = "Folder to dump current CPER report files" + file_limit_help = "Maximum number of current CPER files in target folder\n Older files beyond limit will be deleted" + cper_file_help = "Full path of a retrieved cper record file to generate the AFID" + follow_help = "Continuously monitor for new CPER entries" ras_parser = subparsers.add_parser("ras", help=ras_help, description=ras_description) ras_parser._optionals.title = ras_optionals_title ras_parser.formatter_class = lambda prog: AMDSMISubparserHelpFormatter(prog) ras_parser.set_defaults(func=func) - # Group arguments into cper and afid categories and make them mutually exclusive + # Create mutually exclusive command ras group (--cper or --afid) ras_exclusive_group = ras_parser.add_mutually_exclusive_group(required=True) - ras_exclusive_group.title = "RAS Exclusive Arguments" ras_exclusive_group.add_argument("--cper", action="store_true", help=cper_help) ras_exclusive_group.add_argument("--afid", action="store_true", help=afid_help) - # CPER Arguments - ras_parser.add_argument("--severity", type=str.lower, nargs='+', default=['all'], help=severity_help, choices=severity_choices, metavar='SEVERITY') - ras_parser.add_argument("--folder", type=str, action=self._check_folder_path(), default=False, help=folder_help) - ras_parser.add_argument("--file-limit", type=self._positive_int, action='store', default=1000, help=file_limit_help) - ras_parser.add_argument("--follow", action="store_true", default=False, help=follow_help) + # CPER Arguments remove defaults + cper_group = ras_parser.add_argument_group("CPER Arguments") + cper_group.add_argument("--severity", type=str.lower, nargs='+', default=['all'], help=severity_help, choices=severity_choices, metavar='SEVERITY') + cper_group.add_argument("--folder", type=str, action=self._check_folder_path(), help=folder_help) + cper_group.add_argument("--file-limit", type=self._positive_int, action='store', help=file_limit_help) + cper_group.add_argument("--follow", action="store_true", help=follow_help) # AFID Arguments - ras_parser.add_argument("--cper-file", action=self._check_cper_file_path(), metavar="CPER_FILE", help=cper_file_help) + afid_group = ras_parser.add_argument_group("AFID Arguments") + afid_group.add_argument("--cper-file", action=self._check_cper_file_path(), metavar="CPER_FILE", help=cper_file_help) # Add common modifiers and device selection arguments. self._add_device_arguments(ras_parser, required=False) diff --git a/projects/amdsmi/py-interface/amdsmi_interface.py b/projects/amdsmi/py-interface/amdsmi_interface.py index 0a891f9c2d..ba0b26453a 100644 --- a/projects/amdsmi/py-interface/amdsmi_interface.py +++ b/projects/amdsmi/py-interface/amdsmi_interface.py @@ -2560,7 +2560,7 @@ def amdsmi_get_afids_from_cper( ctypes.byref(num_afids_ct) ) if status != amdsmi_wrapper.AMDSMI_STATUS_SUCCESS: - raise AmdSmiLibraryException(f"get_afids failed: {status}") + raise AmdSmiLibraryException(status) # Collect exactly the decoded AFIDs count = num_afids_ct.value diff --git a/projects/amdsmi/src/amd_smi/amd_smi.cc b/projects/amdsmi/src/amd_smi/amd_smi.cc index b92d9704e1..410d4d26a4 100644 --- a/projects/amdsmi/src/amd_smi/amd_smi.cc +++ b/projects/amdsmi/src/amd_smi/amd_smi.cc @@ -4176,12 +4176,12 @@ amdsmi_status_t amdsmi_get_afids_from_cper( if(cper->record_length > buf_size) { ss << __PRETTY_FUNCTION__ << "\n:" << __LINE__ << "[AFIDS] cper buffer size " << std::dec << buf_size << " is smaller than cper record length " << std::dec << cper->record_length << "\n"; LOG_ERROR(ss); - return AMDSMI_STATUS_INVAL; + return AMDSMI_STATUS_UNEXPECTED_SIZE; } else if(strncmp(cper->signature, "CPER", 4) != 0) { ss << __PRETTY_FUNCTION__ << "\n:" << __LINE__ << "[AFIDS] cper buffer does not have the correct signature\n"; LOG_ERROR(ss); - return AMDSMI_STATUS_INVAL; + return AMDSMI_STATUS_UNEXPECTED_DATA; } uint32_t i = 0; for(int afid: cper_decode(cper)) {