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 <[email protected]> > --- > 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; > +}
