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
