On Thu, Mar 8, 2018 at 4:10 PM, Verma, Vishal L <vishal.l.ve...@intel.com> wrote: > > On Tue, 2018-03-06 at 16:46 -0700, Dave Jiang wrote: >> Adding generic and intel support function to allow check if update >> firmware >> is supported by the kernel. >> >> Signed-off-by: Dave Jiang <dave.ji...@intel.com> >> Reviewed-by: Dan Williams <dan.j.willi...@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(+) >> >> 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. >> + */ > > Since this is an is_supported type function, for completeness, > shouldn't we just check for all the related DSMs? I agree we will > probably never hit the case where say FW_GET_INFO is supported but > others aren't, but just adding in the other checks is probably better > than the possibility of running into a case where this passes but one > of the other functions isn't supported.
Some of them aren't required for example I think this gauntlet of checks is overkill, especially when we consider other vendor firmware update mechanisms that might not implement all of these... fw->store_size = ndctl_cmd_fw_info_get_storage_size(cmd); if (fw->store_size == UINT_MAX) return -ENXIO; fw->update_size = ndctl_cmd_fw_info_get_max_send_len(cmd); if (fw->update_size == UINT_MAX) return -ENXIO; fw->query_interval = ndctl_cmd_fw_info_get_query_interval(cmd); if (fw->query_interval == UINT_MAX) return -ENXIO; fw->max_query = ndctl_cmd_fw_info_get_max_query_time(cmd); if (fw->max_query == UINT_MAX) return -ENXIO; fw->run_version = ndctl_cmd_fw_info_get_run_version(cmd); if (fw->run_version == ULLONG_MAX) return -ENXIO; ...so yes, I think it would be could to expand the 'supported' checks, but only to the bare minimum that would allow a firmware update to complete. _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm