[Accel-config] Re: [PATCH 1/2] accel-config: Add APIs to get details of last error and associated devices

On Mon, Jul 12, 2021 at 01:58:45PM -0700, Dave Jiang wrote:
> 
> On 7/12/2021 1:40 PM, Thomas, Ramesh wrote:
> > On Mon, Jul 12, 2021 at 01:26:52PM -0700, Dave Jiang wrote:
> >> On 7/9/2021 10:47 AM, ramesh.thomas(a)intel.com wrote:
> >>> From: Ramesh Thomas <ramesh.thomas(a)intel.com>
> >>>
> >>> Capture a snapshot of error condition containing cmd_status and the
> >>> associated devices. Provide APIs to return the captured details. Handles
> >>> both hw and sw error codes stored in sysfs cmd_status field of devices.
> >>> Following APIs are added:
> >>> - accfg_ctx_get_last_error;
> >>> - accfg_ctx_get_last_error_str;
> >>> - accfg_ctx_get_last_error_device;
> >>> - accfg_ctx_get_last_error_wq;
> >>> - accfg_ctx_get_last_error_group;
> >>> - accfg_ctx_get_last_error_engine;
> >>>
> >>> Signed-off-by: Ramesh Thomas <ramesh.thomas(a)intel.com>
> >>> ---
> >>>    accfg/lib/libaccel-config.sym |  10 +++
> >>>    accfg/lib/libaccfg.c          | 156 ++++++++++++++++++++++++++++++----
> >>>    accfg/lib/private.h           |   9 ++
> >>>    accfg/libaccel_config.h       |   9 +-
> >>>    4 files changed, 164 insertions(+), 20 deletions(-)
> >>>
> >>> diff --git a/accfg/lib/libaccel-config.sym b/accfg/lib/libaccel-config.sym
> >>> index aecc98b..679c597 100644
> >>> --- a/accfg/lib/libaccel-config.sym
> >>> +++ b/accfg/lib/libaccel-config.sym
> >>> @@ -149,3 +149,13 @@ LIBACCFG_9 {
> >>>    global:
> >>>      accfg_device_get_op_cap;
> >>>    } LIBACCFG_8;
> >>> +
> >>> +LIBACCFG_10 {
> >>> +global:
> >>> +   accfg_ctx_get_last_error;
> >>> +   accfg_ctx_get_last_error_str;
> >>> +   accfg_ctx_get_last_error_device;
> >>> +   accfg_ctx_get_last_error_wq;
> >>> +   accfg_ctx_get_last_error_group;
> >>> +   accfg_ctx_get_last_error_engine;
> >>> +} LIBACCFG_9;
> >>> diff --git a/accfg/lib/libaccfg.c b/accfg/lib/libaccfg.c
> >>> index f53751e..00314b0 100644
> >>> --- a/accfg/lib/libaccfg.c
> >>> +++ b/accfg/lib/libaccfg.c
> >>> @@ -70,6 +70,7 @@ ACCFG_EXPORT char *accfg_mdev_basenames[] = {
> >>>
> >>>    enum {
> >>>      ACCFG_CMD_STATUS_MAX = 0x45,
> >>> +   ACCFG_CMD_STATUS_ERROR = 0x80010000,
> >>>    };
> >>>
> >>>    const char *accfg_device_cmd_status[] = {
> >>> @@ -105,9 +106,43 @@ const char *accfg_device_cmd_status[] = {
> >>>      [0x42]  = "No interrupt handle available",
> >>>      [0x43]  = "No interrupt handles associated with the index",
> >>>      [0x44]  = "No revoked handles associted with the index",
> >>> -   [ACCFG_CMD_STATUS_MAX]  = "",
> >>> +   [ACCFG_CMD_STATUS_MAX]  = "Unknown error",
> >>>    };
> >>>
> >>> +const char *accfg_sw_cmd_status[] = {
> >>> +   [0x01] = "Error reading cmd_status",
> >>> +   [0x02] = "DMA device registration error",
> >>> +   [0x03] = "wq error - group not set",
> >>> +   [0x04] = "wq error - name not set",
> >>> +   [0x05] = "wq error - no SVM support and needed",
> >>> +   [0x06] = "wq error - threshold not set",
> >>> +   [0x07] = "wq portal mapping error",
> >>> +   [0x08] = "wq resource allocation error",
> >>> +   [0x09] = "wq percpu counter allocation error",
> >>> +   [0x0a] = "wq DMA channel registration error",
> >>> +   [0x0b] = "wq char dev registration error",
> >>> +   [0x0c] = "wq error - no shared wq support",
> >>> +   [0x0d] = "wq error - no wqs configured",
> >>> +   [0x0e] = "wq error - size not set",
> >>> +};
> >>> +
> >>> +static void save_last_error(struct accfg_device *device, struct accfg_wq *wq,
> >>> +           struct accfg_group *group, struct accfg_engine *engine)
> >>> +{
> >>> +   struct accfg_ctx *ctx = device->ctx;
> >>> +
> >>> +   if (!ctx->error_ctx)
> >>> +           ctx->error_ctx = calloc(1, sizeof(struct accfg_error_ctx));
> >>> +
> >>> +   if (ctx->error_ctx) {
> >>> +           ctx->error_ctx->cmd_status = accfg_device_get_cmd_status(device);
> >>> +           ctx->error_ctx->device = device;
> >>> +           ctx->error_ctx->wq = wq;
> >>> +           ctx->error_ctx->group = group;
> >>> +           ctx->error_ctx->engine = engine;
> >>> +   }
> >>> +}
> >>> +
> >>>    static inline bool is_mdev_registered(struct accfg_device *device)
> >>>    {
> >>>      return device->mdev_path && !access(device->mdev_path, R_OK);
> >>> @@ -251,6 +286,7 @@ static void free_context(struct accfg_ctx *ctx)
> >>>
> >>>      list_for_each_safe(&ctx->devices, device, _b, list)
> >>>              free_device(device, &ctx->devices);
> >>> +   free(ctx->error_ctx);
> >>>      free(ctx);
> >>>    }
> >>>
> >>> @@ -1082,6 +1118,7 @@ ACCFG_EXPORT int accfg_create_mdev(struct accfg_device *device,
> >>>      rc = sysfs_write_attr(ctx, mdev_path, uuid_str);
> >>>      if (rc < 0) {
> >>>              err(ctx, "create mdev failed %d\n", rc);
> >>> +           save_last_error(device, NULL, NULL, NULL);
> >>>              goto create_err;
> >>>      }
> >>>
> >>> @@ -1105,8 +1142,10 @@ static int accfg_device_mdev_remove(struct accfg_device *device,
> >>>      uuid_unparse(mdev->uuid, uuid_str);
> >>>      sprintf(mdev_path, "%s/%s/remove", device->mdev_path, uuid_str);
> >>>      rc = sysfs_write_attr(ctx, mdev_path, "1");
> >>> -   if (rc < 0)
> >>> +   if (rc < 0) {
> >>> +           save_last_error(device, NULL, NULL, NULL);
> >>>              return rc;
> >>> +   }
> >>>
> >>>      list_del(&mdev->list);
> >>>      free(mdev);
> >>> @@ -1355,6 +1394,7 @@ ACCFG_EXPORT enum accfg_device_state accfg_device_get_state(
> >>>              err(ctx, "%s: sysfs_read_attr failed '%s': %s\n",
> >>>                              __func__, device->device_path,
> >>>                              strerror(errno));
> >>> +           save_last_error(device, NULL, NULL, NULL);
> >>>              return ACCFG_DEVICE_UNKNOWN;
> >>>      }
> >>>
> >>> @@ -1414,6 +1454,7 @@ ACCFG_EXPORT int accfg_device_get_clients(struct accfg_device
> *device)
> >>>      if (sysfs_read_attr(ctx, path, buf) < 0) {
> >>>              err(ctx, "%s: retrieve clients failed '%s': %s\n",
> >>>                              __func__, path, strerror(errno));
> >>> +           save_last_error(device, NULL, NULL, NULL);
> >>>              return -errno;
> >>>      }
> >>>
> >>> @@ -1448,6 +1489,7 @@ ACCFG_EXPORT int accfg_device_set_token_limit(struct accfg_device
> >> *dev, int val)
> >>>      if (sysfs_write_attr(ctx, path, buf) < 0) {
> >>>              err(ctx, "%s: write failed: %s\n",
> >>>                              accfg_device_get_devname(dev), strerror(errno));
> >>> +           save_last_error(dev, NULL, NULL, NULL);
> >>>              return -errno;
> >>>      }
> >>>
> >>> @@ -1476,8 +1518,10 @@ ACCFG_EXPORT int accfg_device_is_active(struct accfg_device
> *device)
> >>>              return 0;
> >>>      }
> >>>
> >>> -   if (sysfs_read_attr(ctx, path, buf) < 0)
> >>> +   if (sysfs_read_attr(ctx, path, buf) < 0) {
> >>> +           save_last_error(device, NULL, NULL, NULL);
> >>>              return 0;
> >>> +   }
> >>>
> >>>      if (strcmp(buf, "enabled") == 0)
> >>>              return 1;
> >>> @@ -1485,17 +1529,16 @@ ACCFG_EXPORT int accfg_device_is_active(struct accfg_device
> *device)
> >>>      return 0;
> >>>    }
> >>>
> >>> -ACCFG_EXPORT int accfg_device_get_cmd_status(struct accfg_device *device)
> >>> +ACCFG_EXPORT unsigned int accfg_device_get_cmd_status(struct accfg_device *device)
> >>>    {
> >>>      struct accfg_ctx *ctx;
> >>>      long status;
> >>>      char *path;
> >>>      int len;
> >>>      char buf[SYSFS_ATTR_SIZE], *end_ptr;
> >>> -   int rc;
> >>>
> >>>      if (!device)
> >>> -           return -EINVAL;
> >>> +           return ACCFG_CMD_STATUS_ERROR;
> >>>
> >>>      ctx = accfg_device_get_ctx(device);
> >>>      path = device->device_buf;
> >>> @@ -1504,31 +1547,96 @@ ACCFG_EXPORT int accfg_device_get_cmd_status(struct accfg_device
> >> *device)
> >>>      if (snprintf(path, len, "%s/cmd_status", device->device_path) >= len) {
> >>>              err(ctx, "%s: buffer too small!\n",
> >>>                              accfg_device_get_devname(device));
> >>> -           return -ENOMEM;
> >>> +           return ACCFG_CMD_STATUS_ERROR;
> >>>      }
> >>>
> >>> -   rc = sysfs_read_attr(ctx, path, buf);
> >>> -   if (rc < 0)
> >>> -           return rc;
> >>> +   if (sysfs_read_attr(ctx, path, buf))
> >>> +           return ACCFG_CMD_STATUS_ERROR;
> >>>
> >>>      status = strtol(buf, &end_ptr, 0);
> >>>      if (errno == ERANGE || end_ptr == buf)
> >>> -           return -EIO;
> >>> +           return ACCFG_CMD_STATUS_ERROR;
> >>>
> >>> -   return (int)status;
> >>> +   return (unsigned int)status;
> >>>    }
> >>>
> >>> -ACCFG_EXPORT const char * accfg_device_get_cmd_status_str(struct accfg_device *device)
> >>> +static const char *get_cmd_status_str(unsigned int status)
> >>>    {
> >>> -   int status;
> >>> +   unsigned int sw_status;
> >>>
> >>> -   status = accfg_device_get_cmd_status(device);
> >>> -   if (status < 0 || status >= ACCFG_CMD_STATUS_MAX)
> >>> -           return NULL;
> >>> +   if (status & 0x80000000) {
> >>> +           sw_status = (status & ~0x80000000) >> 16;
> >>
> >> Magic number. Do I need to add a mask define to the header?
> > Or add a comment stating device error codes are in the lower 16 bits and
> > sw error codes are in the higher bits with the highest bit set.
> 
> Looks like I already defined the mask in the user header:
> 
> IDXD_SCMD_SOFTERR_MASK
> 
> I've also added a shift define. Just fetch djiang5/idxd-upstream-next again.
> 

I will use that and send an update.