On Mon, Jul 24, 2023 at 10:07:47PM +0000, Verma, Vishal L wrote: > Hi Jehoon, > > Thanks for adding this. A few minor comments below, otherwise these > look good. >
Hi, Vishal. Thank you for comments. I agree with all of them, especially the use of strcmp. I missed the awkward case you mentioned. I'll send v2 patch soon with applying those comments. Jehoon > 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; > > +} >
