| Message ID | 20180711211709.3007-1-vishal.l.verma@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jul 11, 2018 at 2:17 PM, Vishal Verma <vishal.l.verma@intel.com> wrote: > Add an API to check whether smart injection is supported, and add an > intel-dsm implementation for it. Use this check in the ndctl > inject-smart command. > > Reported-by: Leszek Lugin <leszek.lugin@intel.com> > Cc: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> [..] > +static int intel_dimm_smart_inject_supported(struct ndctl_dimm *dimm) > +{ > + struct ndctl_ctx *ctx = ndctl_dimm_get_ctx(dimm); > + > + if (!ndctl_dimm_is_cmd_supported(dimm, ND_CMD_CALL)) { > + dbg(ctx, "unsupported cmd: %d\n", ND_CMD_CALL); > + return -EOPNOTSUPP; > + } > + > + if (test_dimm_dsm(dimm, ND_INTEL_SMART_INJECT_MTEMP) == > + DIMM_DSM_UNSUPPORTED || > + test_dimm_dsm(dimm, ND_INTEL_SMART_INJECT_SPARE) == > + DIMM_DSM_UNSUPPORTED || > + test_dimm_dsm(dimm, ND_INTEL_SMART_INJECT_FATAL) == > + DIMM_DSM_UNSUPPORTED || > + test_dimm_dsm(dimm, ND_INTEL_SMART_INJECT_SHUTDOWN) == > + DIMM_DSM_UNSUPPORTED) { > + dbg(ctx, "smart injection functions unsupported\n"); > + return -EIO; Just a nit on readability. I find it jarring to split the arguments to "== "on 2 different lines, especially after Neil Brown made a similar observation on my code. The natural read cadence is to grok the "==" statement and then move to the next operator on the following line. Also, we've thankfully defined DIMM_DSM_UNSUPPORTED as zero, so it would be more readable to drop == and do the following with the || operators: if (!test_dimm_dsm(dimm, ND_INTEL_SMART_INJECT_MTEMP) || !test_dimm_dsm(dimm, ND_INTEL_SMART_INJECT_SPARE) || !test_dimm_dsm(dimm, ND_INTEL_SMART_INJECT_FATAL) || !test_dimm_dsm(dimm, ND_INTEL_SMART_INJECT_SHUTDOWN)) {
On Wed, 2018-07-11 at 14:38 -0700, Dan Williams wrote: > On Wed, Jul 11, 2018 at 2:17 PM, Vishal Verma <vishal.l.verma@intel.com> wrote: > > Add an API to check whether smart injection is supported, and add an > > intel-dsm implementation for it. Use this check in the ndctl > > inject-smart command. > > > > Reported-by: Leszek Lugin <leszek.lugin@intel.com> > > Cc: Dan Williams <dan.j.williams@intel.com> > > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> > > [..] > > +static int intel_dimm_smart_inject_supported(struct ndctl_dimm *dimm) > > +{ > > + struct ndctl_ctx *ctx = ndctl_dimm_get_ctx(dimm); > > + > > + if (!ndctl_dimm_is_cmd_supported(dimm, ND_CMD_CALL)) { > > + dbg(ctx, "unsupported cmd: %d\n", ND_CMD_CALL); > > + return -EOPNOTSUPP; > > + } > > + > > + if (test_dimm_dsm(dimm, ND_INTEL_SMART_INJECT_MTEMP) == > > + DIMM_DSM_UNSUPPORTED || > > + test_dimm_dsm(dimm, ND_INTEL_SMART_INJECT_SPARE) == > > + DIMM_DSM_UNSUPPORTED || > > + test_dimm_dsm(dimm, ND_INTEL_SMART_INJECT_FATAL) == > > + DIMM_DSM_UNSUPPORTED || > > + test_dimm_dsm(dimm, ND_INTEL_SMART_INJECT_SHUTDOWN) == > > + DIMM_DSM_UNSUPPORTED) { > > + dbg(ctx, "smart injection functions unsupported\n"); > > + return -EIO; > > Just a nit on readability. I find it jarring to split the arguments to > "== "on 2 different lines, especially after Neil Brown made a similar > observation on my code. The natural read cadence is to grok the "==" > statement and then move to the next operator on the following line. > Also, we've thankfully defined DIMM_DSM_UNSUPPORTED as zero, so it > would be more readable to drop == and do the following with the || > operators: > > if (!test_dimm_dsm(dimm, ND_INTEL_SMART_INJECT_MTEMP) > || !test_dimm_dsm(dimm, ND_INTEL_SMART_INJECT_SPARE) > || !test_dimm_dsm(dimm, ND_INTEL_SMART_INJECT_FATAL) > || !test_dimm_dsm(dimm, > ND_INTEL_SMART_INJECT_SHUTDOWN)) { Yes that looks much better. I'll send a v2.
On Wed, 2018-07-11 at 21:51 +0000, Verma, Vishal L wrote: > On Wed, 2018-07-11 at 14:38 -0700, Dan Williams wrote: > > On Wed, Jul 11, 2018 at 2:17 PM, Vishal Verma <vishal.l.verma@intel > > .com> wrote: > > > Add an API to check whether smart injection is supported, and add an > > > intel-dsm implementation for it. Use this check in the ndctl > > > inject-smart command. > > > > > > Reported-by: Leszek Lugin <leszek.lugin@intel.com> > > > Cc: Dan Williams <dan.j.williams@intel.com> > > > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> > > > > [..] > > > +static int intel_dimm_smart_inject_supported(struct ndctl_dimm *dimm) > > > +{ > > > + struct ndctl_ctx *ctx = ndctl_dimm_get_ctx(dimm); > > > + > > > + if (!ndctl_dimm_is_cmd_supported(dimm, ND_CMD_CALL)) { > > > + dbg(ctx, "unsupported cmd: %d\n", ND_CMD_CALL); > > > + return -EOPNOTSUPP; > > > + } > > > + > > > + if (test_dimm_dsm(dimm, ND_INTEL_SMART_INJECT_MTEMP) == > > > + DIMM_DSM_UNSUPPORTED || > > > + test_dimm_dsm(dimm, ND_INTEL_SMART_INJECT_SPARE) == > > > + DIMM_DSM_UNSUPPORTED || > > > + test_dimm_dsm(dimm, ND_INTEL_SMART_INJECT_FATAL) == > > > + DIMM_DSM_UNSUPPORTED || > > > + test_dimm_dsm(dimm, ND_INTEL_SMART_INJECT_SHUTDOWN) == > > > + DIMM_DSM_UNSUPPORTED) { > > > + dbg(ctx, "smart injection functions unsupported\n"); > > > + return -EIO; > > > > Just a nit on readability. I find it jarring to split the arguments to > > "== "on 2 different lines, especially after Neil Brown made a similar > > observation on my code. The natural read cadence is to grok the "==" > > statement and then move to the next operator on the following line. > > Also, we've thankfully defined DIMM_DSM_UNSUPPORTED as zero, so it > > would be more readable to drop == and do the following with the || > > operators: > > > > if (!test_dimm_dsm(dimm, ND_INTEL_SMART_INJECT_MTEMP) > > || !test_dimm_dsm(dimm, ND_INTEL_SMART_INJECT_SPARE) > > || !test_dimm_dsm(dimm, ND_INTEL_SMART_INJECT_FATAL) > > || !test_dimm_dsm(dimm, > > ND_INTEL_SMART_INJECT_SHUTDOWN)) { > > Yes that looks much better. I'll send a v2. I just realized, this check is not correct. INJECT_* are not separate commands, but flags to the same command - ND_INTEL_SMART_INJECT. So that is all we need to check for in the DSM mask. > _______________________________________________ > Linux-nvdimm mailing list > Linux-nvdimm@lists.01.org > https://lists.01.org/mailman/listinfo/linux-nvdimm
diff --git a/ndctl/inject-smart.c b/ndctl/inject-smart.c index 60de9fe..7793706 100644 --- a/ndctl/inject-smart.c +++ b/ndctl/inject-smart.c @@ -352,6 +352,22 @@ static int dimm_inject_smart(struct ndctl_dimm *dimm) struct json_object *jdimm; int rc; + rc = ndctl_dimm_smart_inject_supported(dimm); + switch (rc) { + case -ENOTTY: + error("%s: smart injection not supported by ndctl.", + ndctl_dimm_get_devname(dimm)); + return rc; + case -EOPNOTSUPP: + error("%s: smart injection not supported by the kernel", + ndctl_dimm_get_devname(dimm)); + return rc; + case -EIO: + error("%s: smart injection not supported by either platform firmware or the kernel.", + ndctl_dimm_get_devname(dimm)); + return rc; + } + if (sctx.op_mask & (1 << OP_SET)) { rc = smart_set_thresh(dimm); if (rc) diff --git a/ndctl/lib/intel.c b/ndctl/lib/intel.c index 49a4360..5a8ecba 100644 --- a/ndctl/lib/intel.c +++ b/ndctl/lib/intel.c @@ -362,6 +362,30 @@ static int intel_cmd_smart_inject_unsafe_shutdown(struct ndctl_cmd *cmd, return 0; } +static int intel_dimm_smart_inject_supported(struct ndctl_dimm *dimm) +{ + struct ndctl_ctx *ctx = ndctl_dimm_get_ctx(dimm); + + if (!ndctl_dimm_is_cmd_supported(dimm, ND_CMD_CALL)) { + dbg(ctx, "unsupported cmd: %d\n", ND_CMD_CALL); + return -EOPNOTSUPP; + } + + if (test_dimm_dsm(dimm, ND_INTEL_SMART_INJECT_MTEMP) == + DIMM_DSM_UNSUPPORTED || + test_dimm_dsm(dimm, ND_INTEL_SMART_INJECT_SPARE) == + DIMM_DSM_UNSUPPORTED || + test_dimm_dsm(dimm, ND_INTEL_SMART_INJECT_FATAL) == + DIMM_DSM_UNSUPPORTED || + test_dimm_dsm(dimm, ND_INTEL_SMART_INJECT_SHUTDOWN) == + DIMM_DSM_UNSUPPORTED) { + dbg(ctx, "smart injection functions unsupported\n"); + return -EIO; + } + + return 0; +} + static const char *intel_cmd_desc(int fn) { static const char *descs[] = { @@ -714,6 +738,7 @@ struct ndctl_dimm_ops * const intel_dimm_ops = &(struct ndctl_dimm_ops) { .smart_inject_spares = intel_cmd_smart_inject_spares, .smart_inject_fatal = intel_cmd_smart_inject_fatal, .smart_inject_unsafe_shutdown = intel_cmd_smart_inject_unsafe_shutdown, + .smart_inject_supported = intel_dimm_smart_inject_supported, .new_fw_get_info = intel_dimm_cmd_new_fw_get_info, .fw_info_get_storage_size = intel_cmd_fw_info_get_storage_size, .fw_info_get_max_send_len = intel_cmd_fw_info_get_max_send_len, diff --git a/ndctl/lib/libndctl.sym b/ndctl/lib/libndctl.sym index e939993..8932ef6 100644 --- a/ndctl/lib/libndctl.sym +++ b/ndctl/lib/libndctl.sym @@ -367,3 +367,8 @@ global: ndctl_namespace_uninject_error2; ndctl_cmd_ars_stat_get_flag_overflow; } LIBNDCTL_15; + +LIBNDCTL_17 { +global: + ndctl_dimm_smart_inject_supported; +} LIBNDCTL_16; diff --git a/ndctl/lib/private.h b/ndctl/lib/private.h index b756b74..a94f894 100644 --- a/ndctl/lib/private.h +++ b/ndctl/lib/private.h @@ -311,6 +311,7 @@ struct ndctl_dimm_ops { int (*smart_inject_spares)(struct ndctl_cmd *, bool, unsigned int); int (*smart_inject_fatal)(struct ndctl_cmd *, bool); int (*smart_inject_unsafe_shutdown)(struct ndctl_cmd *, bool); + int (*smart_inject_supported)(struct ndctl_dimm *); struct ndctl_cmd *(*new_fw_get_info)(struct ndctl_dimm *); unsigned int (*fw_info_get_storage_size)(struct ndctl_cmd *); unsigned int (*fw_info_get_max_send_len)(struct ndctl_cmd *); diff --git a/ndctl/lib/smart.c b/ndctl/lib/smart.c index 0455252..7ba46d1 100644 --- a/ndctl/lib/smart.c +++ b/ndctl/lib/smart.c @@ -172,3 +172,13 @@ ndctl_dimm_cmd_new_ack_shutdown_count(struct ndctl_dimm *dimm) else return NULL; } + +NDCTL_EXPORT int ndctl_dimm_smart_inject_supported(struct ndctl_dimm *dimm) +{ + struct ndctl_dimm_ops *ops = dimm->ops; + + if (ops && ops->smart_inject_supported) + return ops->smart_inject_supported(dimm); + else + return -ENOTTY; +} diff --git a/ndctl/libndctl.h b/ndctl/libndctl.h index 9270bae..8a96c84 100644 --- a/ndctl/libndctl.h +++ b/ndctl/libndctl.h @@ -279,6 +279,7 @@ int ndctl_cmd_smart_inject_spares(struct ndctl_cmd *cmd, bool enable, unsigned int spares); int ndctl_cmd_smart_inject_fatal(struct ndctl_cmd *cmd, bool enable); int ndctl_cmd_smart_inject_unsafe_shutdown(struct ndctl_cmd *cmd, bool enable); +int ndctl_dimm_smart_inject_supported(struct ndctl_dimm *dimm); struct ndctl_cmd *ndctl_dimm_cmd_new_vendor_specific(struct ndctl_dimm *dimm, unsigned int opcode, size_t input_size, size_t output_size);
Add an API to check whether smart injection is supported, and add an intel-dsm implementation for it. Use this check in the ndctl inject-smart command. Reported-by: Leszek Lugin <leszek.lugin@intel.com> Cc: Dan Williams <dan.j.williams@intel.com> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> --- ndctl/inject-smart.c | 16 ++++++++++++++++ ndctl/lib/intel.c | 25 +++++++++++++++++++++++++ ndctl/lib/libndctl.sym | 5 +++++ ndctl/lib/private.h | 1 + ndctl/lib/smart.c | 10 ++++++++++ ndctl/libndctl.h | 1 + 6 files changed, 58 insertions(+)