Re: [PATCH V2 1/5] nvme-cli: Decode "Supported Events Bitmap" in PEL header

[Chaitanya Kulkarni <chaitanyak@nvidia.com>]

On 11/18/21 8:57 AM, wenxiong@linux.ibm.com wrote:
> External email: Use caution opening links or attachments
> 
> 
> From: Wen Xiong <wenxiong@linux.ibm.com>
> 
> "Supported Events Bitmap" in PEL header shows what events
> are supported in current nvme devices.
> 
> Persistent Event Log for device: nvme0n1
> Action for Persistent Event Log: 0
> ..
> ..
> ..
> Supported Events Bitmap:
>          Support SMART/Health Log Snapshot Event(0x1)

Support word is duplicate since we already have
"Supported Events Bitmap" in the header of this message.

>          Support Firmware Commit Event(0x2)
>          Support Timestamp Change Event(0x3)
>          Support Power-on or Reset Event(0x4)
>          Support NVM Subsystem Hardware Error Event(0x5)
>          Support Change Namespace Event(0x6)
>          Support Format NVM Start Event(0x7)
>          Support Format NVM Completion Event(0x8)
>          Support Sanitize Start Event(0x9)
>          Support Sanitize Completion Event(0xa)
>          Support Set Feature Event(0xb)
>          Support Set Telemetry CRT  Event(0xc)
>          Support Thermal Excursion Event(0xd)
> 
> Signed-off-by: Wen Xiong <wenxiong@linux.ibm.com>
> ---
>   nvme-print.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++------
>   nvme.c       |  4 ++--
>   2 files changed, 48 insertions(+), 8 deletions(-)
> 
> diff --git a/nvme-print.c b/nvme-print.c
> index 1556b52..1b419b7 100644
> --- a/nvme-print.c
> +++ b/nvme-print.c
> @@ -1053,6 +1053,26 @@ void nvme_show_predictable_latency_event_agg_log(
>          }
>   }
> 
> +const char *nvme_pel_event_to_string(int type)
> +{

type should be a type of enum nvme_persistent_event_types,
is there a reason its type is int ?

> +       switch (type) {
> +       case NVME_PEL_SMART_HEALTH_EVENT:       return "SMART/Health Log Snapshot Event(0x1)";
> +       case NVME_PEL_FW_COMMIT_EVENT:  return "Firmware Commit Event(0x2)";
> +       case NVME_PEL_TIMESTAMP_EVENT:  return "Timestamp Change Event(0x3)";
> +       case NVME_PEL_POWER_ON_RESET_EVENT:     return "Power-on or Reset Event(0x4)";
> +       case NVME_PEL_NSS_HW_ERROR_EVENT:       return "NVM Subsystem Hardware Error Event(0x5)";
> +       case NVME_PEL_CHANGE_NS_EVENT:  return "Change Namespace Event(0x6)";
> +       case NVME_PEL_FORMAT_START_EVENT:       return "Format NVM Start Event(0x7)";
> +       case NVME_PEL_FORMAT_COMPLETION_EVENT:  return "Format NVM Completion Event(0x8)";
> +       case NVME_PEL_SANITIZE_START_EVENT:     return "Sanitize Start Event(0x9)";
> +       case NVME_PEL_SANITIZE_COMPLETION_EVENT:        return "Sanitize Completion Event(0xa)";
> +       case NVME_PEL_SET_FEATURE_EVENT:        return "Set Feature Event(0xb)";
> +       case NVME_PEL_TELEMETRY_CRT:            return "Set Telemetry CRT  Event(0xc)";
> +       case NVME_PEL_THERMAL_EXCURSION_EVENT:  return "Thermal Excursion Event(0xd)";
> +       default:                        return NULL;
> +       }
> +}
> +
>   static const char *nvme_show_nss_hw_error(__u16 error_code)
>   {
>          switch (error_code) {
> @@ -1081,6 +1101,29 @@ static const char *nvme_show_nss_hw_error(__u16 error_code)
>          }
>   }
> 
> +void add_bitmap(int i, __u8 seb, struct json_object *root, int json_flag)
> +{
> +       char evt_str[50];
> +       char key[128];
> +

do we need to initialize above arrays to 0s ?

> +       for (int bit = 0; bit < 8; bit++) {
> +               if (nvme_pel_event_to_string(bit + i * 8)) {
> +                       if (json_flag == 1) {

If json_flag is going to take only two values then its type should be
bool.

> +                               sprintf(key, "bitmap_%x", (bit + i * 8));
> +                               if ((seb >> bit) & 0x1)
> +                                       snprintf(evt_str, sizeof(evt_str), "Support %s",
> +                                               nvme_pel_event_to_string(bit + i * 8));
> +                               json_object_add_value_string(root, key, evt_str);
> +                       } else {
> +                               if (nvme_pel_event_to_string(bit + i * 8))
> +                                       if ((seb >> bit) & 0x1)
> +                                               printf("        Support %s\n",
> +                                                       nvme_pel_event_to_string(bit + i * 8));
> +                       }
> +               }
> +       }
> +}
> +
>   void json_persistent_event_log(void *pevent_log_info, __u32 size)
>   {
>          struct json_object *root;
> @@ -1151,9 +1194,7 @@ void json_persistent_event_log(void *pevent_log_info, __u32 size)
>                  for (int i = 0; i < 32; i++) {
>                          if (pevent_log_head->seb[i] == 0)
>                                  continue;
> -                       sprintf(key, "bitmap_%d", i);
> -                       json_object_add_value_uint(root, key,
> -                               pevent_log_head->seb[i]);
> +                       add_bitmap(i, pevent_log_head->seb[i], root, 1);
>                  }
>          } else {
>                  printf("No log data can be shown with this log len at least " \
> @@ -1479,12 +1520,11 @@ void nvme_show_persistent_event_log(void *pevent_log_info,
>                          le32_to_cpu(pevent_log_head->rci));
>                  if (human)
>                          nvme_show_persistent_event_log_rci(pevent_log_head->rci);
> -               printf("Supported Events Bitmap: ");
> +               printf("Supported Events Bitmap: \n");
>                  for (int i = 0; i < 32; i++) {
>                          if (pevent_log_head->seb[i] == 0)
>                                  continue;
> -                       printf("  BitMap[%d] is 0x%x\n", i,
> -                               pevent_log_head->seb[i]);
> +                       add_bitmap(i, pevent_log_head->seb[i], NULL, 0);
>                  }
>          } else {
>                  printf("No log data can be shown with this log len at least " \
> diff --git a/nvme.c b/nvme.c
> index 6ebfa92..a68db6c 100644
> --- a/nvme.c
> +++ b/nvme.c
> @@ -4922,7 +4922,7 @@ static int dsm(int argc, char **argv, struct command *cmd, struct plugin *plugin
> 
>          nc = argconfig_parse_comma_sep_array(cfg.ctx_attrs, (int *)ctx_attrs, ARRAY_SIZE(ctx_attrs));
>          nb = argconfig_parse_comma_sep_array(cfg.blocks, (int *)nlbs, ARRAY_SIZE(nlbs));
> -       ns = argconfig_parse_comma_sep_array_long(cfg.slbas, slbas, ARRAY_SIZE(slbas));
> +       ns = argconfig_parse_comma_sep_array_long(cfg.slbas, (long long unsigned int *)slbas, ARRAY_SIZE(slbas));
>          nr = max(nc, max(nb, ns));
>          if (!nr || nr > 256) {
>                  fprintf(stderr, "No range definition provided\n");
> @@ -5067,7 +5067,7 @@ static int copy(int argc, char **argv, struct command *cmd, struct plugin *plugi
>                  }
>          }
> 
> -       nvme_init_copy_range(copy, nlbs, slbas, eilbrts, elbatms, elbats, nr);
> +       nvme_init_copy_range(copy, nlbs, (__u64 *)slbas, eilbrts, elbatms, elbats, nr);
> 
>          err = nvme_copy(fd, cfg.namespace_id, copy, cfg.sdlba, nr, cfg.prinfor,
>                          cfg.prinfow, cfg.dtype, cfg.dspec, cfg.format, cfg.lr,
> --
> 2.27.0
>