| Message ID | 152036074146.9605.16255604270824668249.stgit@djiang5-desk3.ch.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Mar 6, 2018 at 10:25 AM, Dave Jiang <dave.jiang@intel.com> wrote: > Adding generic and intel support function to allow check if update firmware > is supported by the kernel. > > Signed-off-by: Dave Jiang <dave.jiang@intel.com> [..] > diff --git a/ndctl/update.c b/ndctl/update.c > index 0f0f0d81..b4ae1ddb 100644 > --- a/ndctl/update.c > +++ b/ndctl/update.c > @@ -449,11 +449,24 @@ static int get_ndctl_dimm(struct update_context *uctx, void *ctx) > { > struct ndctl_dimm *dimm; > struct ndctl_bus *bus; > + int rc; > > ndctl_bus_foreach(ctx, bus) > ndctl_dimm_foreach(bus, dimm) { > if (!util_dimm_filter(dimm, uctx->dimm_id)) > continue; > + rc = ndctl_dimm_fw_update_supported(dimm); > + switch (rc) { > + case -ENOTTY: > + error("DIMM firmware update not supported by ndctl."); > + return rc; > + case -EOPNOTSUPP: > + error("DIMM firmware update not supported by the kernel"); > + return rc; > + case -EIO: > + error("DIMM firmware update not supported by platform firmware."); I like that you separated the no ND_CMD_CALL case from the test_dimm_dsm(ND_INTEL_FW_GET_INFO) case, however this error message should be: "DIMM firmware update not supported by either platform firmware or the kernel" ...because it could be the case that the kernel has ND_CMD_CALL support but has not been updated for the given DSM mask. Other than that you can add: Reviewed-by: Dan Williams <dan.j.williams@intel.com>
On 03/06/2018 01:55 PM, Dan Williams wrote: > On Tue, Mar 6, 2018 at 10:25 AM, Dave Jiang <dave.jiang@intel.com> wrote: >> Adding generic and intel support function to allow check if update firmware >> is supported by the kernel. >> >> Signed-off-by: Dave Jiang <dave.jiang@intel.com> > [..] >> diff --git a/ndctl/update.c b/ndctl/update.c >> index 0f0f0d81..b4ae1ddb 100644 >> --- a/ndctl/update.c >> +++ b/ndctl/update.c >> @@ -449,11 +449,24 @@ static int get_ndctl_dimm(struct update_context *uctx, void *ctx) >> { >> struct ndctl_dimm *dimm; >> struct ndctl_bus *bus; >> + int rc; >> >> ndctl_bus_foreach(ctx, bus) >> ndctl_dimm_foreach(bus, dimm) { >> if (!util_dimm_filter(dimm, uctx->dimm_id)) >> continue; >> + rc = ndctl_dimm_fw_update_supported(dimm); >> + switch (rc) { >> + case -ENOTTY: >> + error("DIMM firmware update not supported by ndctl."); >> + return rc; >> + case -EOPNOTSUPP: >> + error("DIMM firmware update not supported by the kernel"); >> + return rc; >> + case -EIO: >> + error("DIMM firmware update not supported by platform firmware."); > > I like that you separated the no ND_CMD_CALL case from the > test_dimm_dsm(ND_INTEL_FW_GET_INFO) case, however this error message > should be: > > "DIMM firmware update not supported by either platform firmware > or the kernel" > > ...because it could be the case that the kernel has ND_CMD_CALL > support but has not been updated for the given DSM mask. > > Other than that you can add: > > Reviewed-by: Dan Williams <dan.j.williams@intel.com> > Thanks. I'll just change it in patch 2/2 instead of here.
diff --git a/ndctl/lib/firmware.c b/ndctl/lib/firmware.c index f6deec5d..277b5399 100644 --- a/ndctl/lib/firmware.c +++ b/ndctl/lib/firmware.c @@ -107,3 +107,14 @@ ndctl_cmd_fw_xlat_firmware_status(struct ndctl_cmd *cmd) else return FW_EUNKNOWN; } + +NDCTL_EXPORT int +ndctl_dimm_fw_update_supported(struct ndctl_dimm *dimm) +{ + struct ndctl_dimm_ops *ops = dimm->ops; + + if (ops && ops->fw_update_supported) + return ops->fw_update_supported(dimm); + else + return -ENOTTY; +} diff --git a/ndctl/lib/intel.c b/ndctl/lib/intel.c index cee5204c..a4f0af26 100644 --- a/ndctl/lib/intel.c +++ b/ndctl/lib/intel.c @@ -650,6 +650,29 @@ intel_dimm_cmd_new_lss(struct ndctl_dimm *dimm) return cmd; } +static int intel_dimm_fw_update_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; + } + + /* + * We only need to check FW_GET_INFO. If that isn't supported then + * the others aren't either. + */ + if (test_dimm_dsm(dimm, ND_INTEL_FW_GET_INFO) + == DIMM_DSM_UNSUPPORTED) { + dbg(ctx, "unsupported function: %d\n", + ND_INTEL_FW_GET_INFO); + return -EIO; + } + + return 0; +} + struct ndctl_dimm_ops * const intel_dimm_ops = &(struct ndctl_dimm_ops) { .cmd_desc = intel_cmd_desc, .new_smart = intel_dimm_cmd_new_smart, @@ -703,4 +726,5 @@ struct ndctl_dimm_ops * const intel_dimm_ops = &(struct ndctl_dimm_ops) { .fw_fquery_get_fw_rev = intel_cmd_fw_fquery_get_fw_rev, .fw_xlat_firmware_status = intel_cmd_fw_xlat_firmware_status, .new_ack_shutdown_count = intel_dimm_cmd_new_lss, + .fw_update_supported = intel_dimm_fw_update_supported, }; diff --git a/ndctl/lib/libndctl.sym b/ndctl/lib/libndctl.sym index af9b7d54..cc580f9c 100644 --- a/ndctl/lib/libndctl.sym +++ b/ndctl/lib/libndctl.sym @@ -344,4 +344,5 @@ global: ndctl_cmd_fw_fquery_get_fw_rev; ndctl_cmd_fw_xlat_firmware_status; ndctl_dimm_cmd_new_ack_shutdown_count; + ndctl_dimm_fw_update_supported; } LIBNDCTL_13; diff --git a/ndctl/lib/private.h b/ndctl/lib/private.h index 015eeb2d..1cad06b7 100644 --- a/ndctl/lib/private.h +++ b/ndctl/lib/private.h @@ -325,6 +325,7 @@ struct ndctl_dimm_ops { unsigned long long (*fw_fquery_get_fw_rev)(struct ndctl_cmd *); enum ND_FW_STATUS (*fw_xlat_firmware_status)(struct ndctl_cmd *); struct ndctl_cmd *(*new_ack_shutdown_count)(struct ndctl_dimm *); + int (*fw_update_supported)(struct ndctl_dimm *); }; struct ndctl_dimm_ops * const intel_dimm_ops; diff --git a/ndctl/libndctl.h b/ndctl/libndctl.h index 9db775ba..d223ea81 100644 --- a/ndctl/libndctl.h +++ b/ndctl/libndctl.h @@ -625,6 +625,7 @@ unsigned int ndctl_cmd_fw_start_get_context(struct ndctl_cmd *cmd); unsigned long long ndctl_cmd_fw_fquery_get_fw_rev(struct ndctl_cmd *cmd); enum ND_FW_STATUS ndctl_cmd_fw_xlat_firmware_status(struct ndctl_cmd *cmd); struct ndctl_cmd *ndctl_dimm_cmd_new_ack_shutdown_count(struct ndctl_dimm *dimm); +int ndctl_dimm_fw_update_supported(struct ndctl_dimm *dimm); #ifdef __cplusplus } /* extern "C" */ diff --git a/ndctl/update.c b/ndctl/update.c index 0f0f0d81..b4ae1ddb 100644 --- a/ndctl/update.c +++ b/ndctl/update.c @@ -449,11 +449,24 @@ static int get_ndctl_dimm(struct update_context *uctx, void *ctx) { struct ndctl_dimm *dimm; struct ndctl_bus *bus; + int rc; ndctl_bus_foreach(ctx, bus) ndctl_dimm_foreach(bus, dimm) { if (!util_dimm_filter(dimm, uctx->dimm_id)) continue; + rc = ndctl_dimm_fw_update_supported(dimm); + switch (rc) { + case -ENOTTY: + error("DIMM firmware update not supported by ndctl."); + return rc; + case -EOPNOTSUPP: + error("DIMM firmware update not supported by the kernel"); + return rc; + case -EIO: + error("DIMM firmware update not supported by platform firmware."); + return rc; + } uctx->dimm = dimm; return 0; }
Adding generic and intel support function to allow check if update firmware is supported by the kernel. Signed-off-by: Dave Jiang <dave.jiang@intel.com> --- ndctl/lib/firmware.c | 11 +++++++++++ ndctl/lib/intel.c | 24 ++++++++++++++++++++++++ ndctl/lib/libndctl.sym | 1 + ndctl/lib/private.h | 1 + ndctl/libndctl.h | 1 + ndctl/update.c | 13 +++++++++++++ 6 files changed, 51 insertions(+)