| Message ID | 152037997577.39785.391131497352047651.stgit@djiang5-desk3.ch.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.jiang@intel.com> > Reviewed-by: Dan Williams <dan.j.williams@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.. > + 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; > } >
On Thu, Mar 8, 2018 at 4:10 PM, Verma, Vishal L <vishal.l.verma@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.jiang@intel.com> >> Reviewed-by: Dan Williams <dan.j.williams@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.
On 03/08/2018 05:23 PM, Dan Williams wrote: > On Thu, Mar 8, 2018 at 4:10 PM, Verma, Vishal L > <vishal.l.verma@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.jiang@intel.com> >>> Reviewed-by: Dan Williams <dan.j.williams@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. > At least for the Intel firmware update, every one of the DSM calls are required to complete all the steps.
On Thu, Mar 8, 2018 at 4:27 PM, Dave Jiang <dave.jiang@intel.com> wrote: > > > On 03/08/2018 05:23 PM, Dan Williams wrote: >> On Thu, Mar 8, 2018 at 4:10 PM, Verma, Vishal L >> <vishal.l.verma@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.jiang@intel.com> >>>> Reviewed-by: Dan Williams <dan.j.williams@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. >> > > At least for the Intel firmware update, every one of the DSM calls are > required to complete all the steps. Ugh, I didn't realize. Whatever happened to the Robustness Principle, oh well.
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; }