4:44 a.m.
From: Ramesh Thomas <ramesh.thomas(a)intel.com>
Functions enabliing/disabling workqueues and devices had several issues
with status return and printing of error messages. Fix the return value
issues and refactor code to make error handling simpler.
Ramesh Thomas (7):
accel-config: Fix wrong return values in accfg_device_get_cmd_status
accel-config: Refactor accfg_device_get_cmd_status_str
accel-config: Add 0 = "Successful completion" cmd status array
accel-config: Refactor handling printing of device cmd status
accel-config: Print device command status only on errors
accel-config: Remove redundant check of wq state
accel-config: Fix and refactor status return and printing
accfg/enable.c | 125 ++++++++++++++-----------------------------
accfg/lib/libaccfg.c | 30 ++++-------
2 files changed, 50 insertions(+), 105 deletions(-)
--
2.26.2
4:44 a.m.
New subject: [PATCH v1 1/7] accel-config: Fix wrong return values in accfg_device_get_cmd_status
From: Ramesh Thomas <ramesh.thomas(a)intel.com>
Function was returning 0 in error cases and incorrectly relying on errno
for errors not related to strtol after which it is checked.
Signed-off-by: Ramesh Thomas <ramesh.thomas(a)intel.com>
---
accfg/lib/libaccfg.c | 17 +++++++----------
1 file changed, 7 insertions(+), 10 deletions(-)
diff --git a/accfg/lib/libaccfg.c b/accfg/lib/libaccfg.c
index ae862f8..92afca1 100644
--- a/accfg/lib/libaccfg.c
+++ b/accfg/lib/libaccfg.c
@@ -1434,6 +1434,7 @@ ACCFG_EXPORT int accfg_device_get_cmd_status(struct accfg_device
*device)
char *path;
int len;
char buf[SYSFS_ATTR_SIZE], *end_ptr;
+ int rc;
if (!device)
return -EINVAL;
@@ -1445,20 +1446,16 @@ 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 0;
+ return -ENOMEM;
}
- if (sysfs_read_attr(ctx, path, buf) < 0)
- return 0;
+ rc = sysfs_read_attr(ctx, path, buf);
+ if (rc < 0)
+ return rc;
status = strtol(buf, &end_ptr, 0);
- if ((errno == ERANGE && (status == LONG_MAX || status == LONG_MIN)) ||
- (errno != 0 && status == 0))
- return -ERANGE;
-
- /* Nothing was found */
- if (end_ptr == buf)
- return -ENXIO;
+ if (errno == ERANGE || end_ptr == buf)
+ return -EIO;
return (int)status;
}
--
2.26.2
4:44 a.m.
New subject: [PATCH v1 2/7] accel-config: Refactor accfg_device_get_cmd_status_str
From: Ramesh Thomas <ramesh.thomas(a)intel.com>
Simplify the checking of the array containing error messages. The array
is a sparse array of character pointers with NULL in empty elements.
After excluding the bounds, the remaining elements can be returned
directly. A NULL would be returned as expected if the indexing status
code is invalid.
Signed-off-by: Ramesh Thomas <ramesh.thomas(a)intel.com>
---
accfg/lib/libaccfg.c | 12 ++----------
1 file changed, 2 insertions(+), 10 deletions(-)
diff --git a/accfg/lib/libaccfg.c b/accfg/lib/libaccfg.c
index 92afca1..976d8cc 100644
--- a/accfg/lib/libaccfg.c
+++ b/accfg/lib/libaccfg.c
@@ -1463,20 +1463,12 @@ ACCFG_EXPORT int accfg_device_get_cmd_status(struct accfg_device
*device)
ACCFG_EXPORT const char * accfg_device_get_cmd_status_str(struct accfg_device *device)
{
int status;
- const char *stat_str;
status = accfg_device_get_cmd_status(device);
- if (status < 0)
+ if (status < 0 || status >= ACCFG_CMD_STATUS_MAX)
return NULL;
- if (status > ACCFG_CMD_STATUS_MAX)
- return NULL;
-
- stat_str = accfg_device_cmd_status[status];
- if (strlen(stat_str) == 0)
- return NULL;
-
- return stat_str;
+ return accfg_device_cmd_status[status];
}
/* Helper function to validate device type in the defined device array based on
--
2.26.2
4:44 a.m.
New subject: [PATCH v1 3/7] accel-config: Add 0 = "Successful completion" cmd status array
From: Ramesh Thomas <ramesh.thomas(a)intel.com>
Device command status messages did not include status code 0, which is a
valid code meaning "Successful completion".
Signed-off-by: Ramesh Thomas <ramesh.thomas(a)intel.com>
---
accfg/lib/libaccfg.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/accfg/lib/libaccfg.c b/accfg/lib/libaccfg.c
index 976d8cc..2abb4f1 100644
--- a/accfg/lib/libaccfg.c
+++ b/accfg/lib/libaccfg.c
@@ -54,6 +54,7 @@ enum {
};
const char *accfg_device_cmd_status[] = {
+ [0x0] = "Successful completion",
[0x1] = "Invalid command code",
[0x2] = "Invalid WQ index",
[0x3] = "Internal or platform hardware error",
--
2.26.2
4:44 a.m.
New subject: [PATCH v1 4/7] accel-config: Refactor handling printing of device cmd status
From: Ramesh Thomas <ramesh.thomas(a)intel.com>
Create a separate function for common code printing device command
status.
Signed-off-by: Ramesh Thomas <ramesh.thomas(a)intel.com>
---
accfg/enable.c | 22 ++++++++++++----------
1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/accfg/enable.c b/accfg/enable.c
index 34328ca..d3968e4 100644
--- a/accfg/enable.c
+++ b/accfg/enable.c
@@ -81,6 +81,15 @@ static int dev_action_switch(struct accfg_device *device,
}
}
+static void print_device_cmd_status(struct accfg_device *device)
+{
+ const char *status;
+
+ status = accfg_device_get_cmd_status_str(device);
+ if (status)
+ fprintf(stderr, "device command status: %s\n", status);
+}
+
static int device_action(int argc, const char **argv, const char *usage,
const struct option *options, enum dev_action action,
struct accfg_ctx *ctx)
@@ -134,14 +143,10 @@ static int device_action(int argc, const char **argv, const char
*usage,
else
fail++;
} else if (!fail) {
- const char *status;
-
fail_reason = rc;
fprintf(stderr, "failed in %s\n", argv[i]);
- status = accfg_device_get_cmd_status_str(device);
- if (status)
- fprintf(stderr, "device cmd err: %s.\n", status);
+ print_device_cmd_status(device);
}
}
@@ -297,13 +302,10 @@ static int wq_action(int argc, const char **argv, const char
*usage,
else
fail++;
} else if (!fail) {
- const char *status;
-
fail_reason = rc;
fprintf(stderr, "failed in %s\n", wq_name);
- status = accfg_device_get_cmd_status_str(device);
- if (status)
- fprintf(stderr, "device cmd err: %s.\n", status);
+
+ print_device_cmd_status(device);
}
}
}
--
2.26.2
4:44 a.m.
New subject: [PATCH v1 5/7] accel-config: Print device command status only on errors
From: Ramesh Thomas <ramesh.thomas(a)intel.com>
Skip printing "Success" message when there is no device command error
status recorded as this is called in the context of error conditions
returned by library.
Signed-off-by: Ramesh Thomas <ramesh.thomas(a)intel.com>
---
accfg/enable.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/accfg/enable.c b/accfg/enable.c
index d3968e4..8abbaca 100644
--- a/accfg/enable.c
+++ b/accfg/enable.c
@@ -85,9 +85,11 @@ static void print_device_cmd_status(struct accfg_device *device)
{
const char *status;
- status = accfg_device_get_cmd_status_str(device);
- if (status)
- fprintf(stderr, "device command status: %s\n", status);
+ if (accfg_device_get_cmd_status(device) > 0) {
+ status = accfg_device_get_cmd_status_str(device);
+ if (status)
+ fprintf(stderr, "device command status: %s\n", status);
+ }
}
static int device_action(int argc, const char **argv, const char *usage,
--
2.26.2
4:44 a.m.
New subject: [PATCH v1 6/7] accel-config: Remove redundant check of wq state
From: Ramesh Thomas <ramesh.thomas(a)intel.com>
Functions that enable and disable wqs already verify state post
operation and return error if does not match the requested state.
Checking here again when success is returned will miss incrementing the
fail count.
Signed-off-by: Ramesh Thomas <ramesh.thomas(a)intel.com>
---
accfg/enable.c | 15 +++------------
1 file changed, 3 insertions(+), 12 deletions(-)
diff --git a/accfg/enable.c b/accfg/enable.c
index 8abbaca..642e30a 100644
--- a/accfg/enable.c
+++ b/accfg/enable.c
@@ -247,7 +247,6 @@ static int wq_action(int argc, const char **argv, const char *usage,
unsigned long dev_id, wq_id;
int i, rc = 0, success = 0, fail = 0, fail_reason = 0;
const char *all = "all";
- enum accfg_wq_state state;
argc = parse_options(argc, argv, options, u, 0);
@@ -293,17 +292,9 @@ static int wq_action(int argc, const char **argv, const char *usage,
found++;
rc = wq_action_switch(wq, action, wq_name);
if (rc == 0) {
- /*
- * Double check if the state of the
- * wq matches with the enable/disable
- */
- state = accfg_wq_get_state(wq);
- if (((state == 1) && (action == WQ_ACTION_ENABLE))
- ||((state == 0) && (action == WQ_ACTION_DISABLE)))
- success++;
- else
- fail++;
- } else if (!fail) {
+ success++;
+ } else {
+ fail++;
fail_reason = rc;
fprintf(stderr, "failed in %s\n", wq_name);
--
2.26.2
4:44 a.m.
New subject: [PATCH v1 7/7] accel-config: Fix and refactor status return and printing
From: Ramesh Thomas <ramesh.thomas(a)intel.com>
Return values should be 0 = success and < 0 = error. Some places were
not return correct values. Simplify the return and status messages.
Signed-off-by: Ramesh Thomas <ramesh.thomas(a)intel.com>
---
accfg/enable.c | 88 ++++++++++++++------------------------------------
1 file changed, 24 insertions(+), 64 deletions(-)
diff --git a/accfg/enable.c b/accfg/enable.c
index 642e30a..9cb6794 100644
--- a/accfg/enable.c
+++ b/accfg/enable.c
@@ -100,7 +100,7 @@ static int device_action(int argc, const char **argv, const char
*usage,
usage,
NULL
};
- int i, rc = 0, success = 0, fail = 0, fail_reason = 0;
+ int i, rc = -EINVAL, success = 0;
const char *all = "all";
enum accfg_device_state state;
@@ -139,13 +139,15 @@ static int device_action(int argc, const char **argv, const char
*usage,
* matches with the enable/disable
*/
state = accfg_device_get_state(device);
- if (((state == 1) && (action == DEV_ACTION_ENABLE))
- ||((state == 0) && (action == DEV_ACTION_DISABLE)))
- success++;
- else
- fail++;
- } else if (!fail) {
- fail_reason = rc;
+ if (((state != ACCFG_DEVICE_ENABLED) &&
+ (action == DEV_ACTION_ENABLE)) ||
+ ((state != ACCFG_DEVICE_DISABLED) &&
+ (action == DEV_ACTION_DISABLE)))
+ rc = ENXIO;
+ }
+ if (rc == 0) {
+ success++;
+ } else {
fprintf(stderr, "failed in %s\n", argv[i]);
print_device_cmd_status(device);
@@ -157,34 +159,14 @@ static int device_action(int argc, const char **argv, const char
*usage,
argv[i]);
}
- if (success) {
- if (action == DEV_ACTION_ENABLE)
- fprintf(stderr, "successfully enabled %d device%s\n",
- success, success > 1 ? "s" : "");
- else
- fprintf(stderr, "successfully disabled %d device%s\n",
- success, success > 1 ? "s" : "");
- return success;
- }
-
- if (fail) {
- if (action == DEV_ACTION_ENABLE)
- fprintf(stderr, "failed to enable %d device%s\n", fail,
- fail > 1 ? "s" : "");
- else
- fprintf(stderr, "failed to disable %d device%s\n", fail,
- fail > 1 ? "s" : "");
+ fprintf(stderr, "%s %d device(s) out of %d\n",
+ action == DEV_ACTION_ENABLE ? "enabled" : "disabled",
+ success, argc);
- return fail;
- }
+ if (success)
+ return 0;
- if (fail_reason) {
- fprintf(stderr, "failed due to reason %s\n",
- strerror(abs(fail_reason)));
- return fail_reason;
- }
-
- return -ENXIO;
+ return rc;
}
static int action_disable_wq(struct accfg_wq *wq, const char *wq_name)
@@ -245,7 +227,7 @@ static int wq_action(int argc, const char **argv, const char *usage,
NULL
};
unsigned long dev_id, wq_id;
- int i, rc = 0, success = 0, fail = 0, fail_reason = 0;
+ int i, rc = -EINVAL, success = 0;
const char *all = "all";
argc = parse_options(argc, argv, options, u, 0);
@@ -281,7 +263,7 @@ static int wq_action(int argc, const char **argv, const char *usage,
rc = sprintf(wq_name, "wq%lu.%lu", dev_id, wq_id);
if (rc < 0)
- return errno;
+ return rc;
accfg_device_foreach(ctx, device) {
if (!util_device_filter(device, dev_name))
@@ -294,8 +276,6 @@ static int wq_action(int argc, const char **argv, const char *usage,
if (rc == 0) {
success++;
} else {
- fail++;
- fail_reason = rc;
fprintf(stderr, "failed in %s\n", wq_name);
print_device_cmd_status(device);
@@ -307,34 +287,14 @@ static int wq_action(int argc, const char **argv, const char
*usage,
fprintf(stderr, "no wq matches id: %s\n", wq_name);
}
- if (success) {
- if (action == WQ_ACTION_ENABLE)
- fprintf(stderr, "successfully enabled %d wq%s\n",
- success,
- success > 1 ? "s" : "");
- else
- fprintf(stderr, "successfully disabled %d wq%s\n",
- success,
- success > 1 ? "s" : "");
- return success;
- }
+ fprintf(stderr, "%s %d wq(s) out of %d\n",
+ action == WQ_ACTION_ENABLE ? "enabled" : "disabled",
+ success, argc);
- if (fail) {
- if (action == WQ_ACTION_ENABLE)
- fprintf(stderr, "failed to enable %d wq%s\n",
- fail, fail > 1 ? "s" : "");
- else
- fprintf(stderr, "failed to disable %d wq%s\n",
- fail, fail > 1 ? "s" : "");
- return fail;
- }
+ if (success)
+ return 0;
- if (fail_reason) {
- fprintf(stderr, "failed due to reason %s\n",
- strerror(abs(fail_reason)));
- return fail_reason;
- }
- return -ENXIO;
+ return rc;
}
int cmd_disable_device(int argc, const char **argv, void *ctx)
--
2.26.2
3:23 p.m.
On 3/2/2021 10:44 AM, ramesh.thomas(a)intel.com wrote:
From: Ramesh Thomas <ramesh.thomas(a)intel.com>
Functions enabliing/disabling workqueues and devices had several issues
with status return and printing of error messages. Fix the return value
issues and refactor code to make error handling simpler.
Ramesh Thomas (7):
accel-config: Fix wrong return values in accfg_device_get_cmd_status
accel-config: Refactor accfg_device_get_cmd_status_str
accel-config: Add 0 = "Successful completion" cmd status array
accel-config: Refactor handling printing of device cmd status
accel-config: Print device command status only on errors
accel-config: Remove redundant check of wq state
accel-config: Fix and refactor status return and printing
accfg/enable.c | 125 ++++++++++++++-----------------------------
accfg/lib/libaccfg.c | 30 ++++-------
2 files changed, 50 insertions(+), 105 deletions(-)
Reviewed-by: Dave Jiang
<dave.jiang(a)intel.com> for the series.
564
days inactive
565
days old
8 comments
2 participants
participants (2)
-
Dave Jiang -
ramesh.thomas＠intel.com