Re: [ndctl PATCH 2/2] cxl: add 'set-alert-config' command to cxl tool

["Verma, Vishal L" <vishal.l.verma@intel.com>]

Hi Jehoon,

Thanks for adding this. A few minor comments below, otherwise these
look good.

On Tue, 2023-07-11 at 16:10 +0900, Jehoon Park wrote:
> Add a new command: 'set-alert-config', which configures device's warning alert
> 
>  usage: cxl set-alert-config <mem0> [<mem1>..<memN>] [<options>]
> 
>     -v, --verbose         turn on debug
>     -S, --serial          use serial numbers to id memdevs
>     -L, --life-used-threshold <threshold>
>                           threshold value for life used warning alert
>         --life-used-alert <'on' or 'off'>
>                           enable or disable life used warning alert
>     -O, --over-temperature-threshold <threshold>
>                           threshold value for device over temperature warning alert
>         --over-temperature-alert <'on' or 'off'>
>                           enable or disable device over temperature warning alert
>     -U, --under-temperature-threshold <threshold>
>                           threshold value for device under temperature warning alert
>         --under-temperature-alert <'on' or 'off'>
>                           enable or disable device under temperature warning alert
>     -V, --volatile-mem-err-threshold <threshold>
>                           threshold value for corrected volatile mem error warning alert
>         --volatile-mem-err-alert <'on' or 'off'>
>                           enable or disable corrected volatile mem error warning alert
>     -P, --pmem-err-threshold <threshold>
>                           threshold value for corrected pmem error warning alert
>         --pmem-err-alert <'on' or 'off'>
>                           enable or disable corrected pmem error warning alert

No need to include the full usage text in the commit message - this is
available in the man page. Just mention and describe what functionality
is being added.

> 
> Signed-off-by: Jehoon Park <jehoon.park@samsung.com>
> ---
>  Documentation/cxl/cxl-set-alert-config.txt |  96 +++++++++
>  Documentation/cxl/meson.build              |   1 +
>  cxl/builtin.h                              |   1 +
>  cxl/cxl.c                                  |   1 +
>  cxl/memdev.c                               | 219 ++++++++++++++++++++-
>  5 files changed, 317 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/cxl/cxl-set-alert-config.txt
> 
> diff --git a/Documentation/cxl/cxl-set-alert-config.txt b/Documentation/cxl/cxl-set-alert-config.txt
> new file mode 100644
> index 0000000..a291c09
> --- /dev/null
> +++ b/Documentation/cxl/cxl-set-alert-config.txt
> @@ -0,0 +1,96 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +cxl-set-alert-config(1)
> +=======================
> +
> +NAME
> +----
> +cxl-set-alert-config - set the warning alert threshold on a CXL memdev
> +
> +SYNOPSIS
> +--------
> +[verse]
> +'cxl set-alert-config <mem0> [<mem1>..<memN>] [<options>]'
> +
> +DESCRIPTION
> +-----------
> +CXL device raises an alert when its health status is changed. Critical alert
> +shall automatically be configured by the device after a device reset.
> +If supported, programmable warning thresholds also be initialized to vendor
> +recommended defaults, then could be configured by the host.

s/host/user/ ?

> 
<snip>
>  
> +static int validate_alert_threshold(enum cxl_setalert_event event,
> +                                   int threshold)
> +{
> +       if (event == CXL_SETALERT_LIFE_USED) {
> +               if (threshold < 0 || threshold > 100) {
> +                       log_err(&ml, "Invalid life used threshold: %d\n",
> +                               threshold);
> +                       return -EINVAL;
> +               }
> +       } else if (event == CXL_SETALERT_OVER_TEMP ||
> +                  event == CXL_SETALERT_UNDER_TEMP) {
> +               if (threshold < SHRT_MIN || threshold > SHRT_MAX) {
> +                       log_err(&ml,
> +                               "Invalid device temperature threshold: %d\n",
> +                               threshold);
> +                       return -EINVAL;
> +               }
> +       } else {
> +               if (threshold < 0 || threshold > USHRT_MAX) {
> +                       log_err(&ml,
> +                               "Invalid corrected mem error threshold: %d\n",
> +                               threshold);
> +                       return -EINVAL;
> +               }
> +       }
> +       return 0;
> +}
> +
> +#define alert_param_set_threshold(arg, alert_event)                           \
> +{                                                                             \
> +       if (!param.arg##_alert) {                                             \
> +               if (param.arg##_threshold) {                                  \
> +                       log_err(&ml, "Action not specified\n");               \
> +                       return -EINVAL;                                       \
> +               }                                                             \
> +       } else if (strncmp(param.arg##_alert, "on", 2) == 0) {                \

I see that ndctl-inject-smart also does strncmp, but I'm wondering if
we should be a little more strict and use strcmp instead.

The option parser won't give us strings that are not nul-terminated, so
it should be safe, and it will avoid something awkward like
"--some-alert=onward".

Ideally we probably want a helper similar to the kernel's kstrtobool(),
which would handle all of {on,true,1,t} and different capitalization as
well, but that can be a follow on patch.

> +               if (param.arg##_threshold) {                                  \
> +                       char *endptr;                                         \
> +                       alertctx.arg##_threshold =                            \
> +                               strtol(param.arg##_threshold, &endptr, 10);   \
> +                       if (endptr[0] != '\0') {                              \
> +                               log_err(&ml, "Invalid threshold: %s\n",       \
> +                                       param.arg##_threshold);               \
> +                               return -EINVAL;                               \
> +                       }                                                     \
> +                       rc = validate_alert_threshold(                        \
> +                               alert_event, alertctx.arg##_threshold);       \
> +                       if (rc != 0)                                          \
> +                               return rc;                                    \
> +                       alertctx.valid_alert_actions |= 1 << alert_event;     \
> +                       alertctx.enable_alert_actions |= 1 << alert_event;    \
> +               } else {                                                      \
> +                       log_err(&ml, "Threshold not specified\n");            \
> +                       return -EINVAL;                                       \
> +               }                                                             \
> +       } else if (strncmp(param.arg##_alert, "off", 3) == 0) {               \
> +               if (!param.arg##_threshold) {                                 \
> +                       alertctx.valid_alert_actions |= 1 << alert_event;     \
> +                       alertctx.enable_alert_actions &= ~(1 << alert_event); \
> +               } else {                                                      \
> +                       log_err(&ml, "Disable not require threshold\n");      \
> +                       return -EINVAL;                                       \
> +               }                                                             \
> +       } else {                                                              \
> +               log_err(&ml, "Invalid action: %s\n", param.arg##_alert);      \
> +               return -EINVAL;                                               \
> +       }                                                                     \
> +}
> +
> 

<snip>

> +int cmd_set_alert_config(int argc, const char **argv, struct cxl_ctx *ctx)
> +{
> +       int count = memdev_action(
> +               argc, argv, ctx, action_set_alert_config, set_alert_options,
> +               "cxl set-alert-config <mem0> [<mem1>..<memN>] [<options>]");
> +       log_info(&ml, "set alert configuration %d mem%s\n",

Maybe "set alert configuration for %d ..."

> +                count >= 0 ? count : 0, count > 1 ? "s" : "");
> +
> +       return count >= 0 ? 0 : EXIT_FAILURE;
> +}