On Tue, Oct 31, 2017 at 02:42:01PM -0700, Dan Williams wrote: > On Tue, Oct 31, 2017 at 2:27 PM, Jerry Hoemann <jerry.hoemann@xxxxxxx> wrote: > > On Mon, Oct 30, 2017 at 01:47:08PM -0700, Dan Williams wrote: > >> Per v1.6 of the NVDIMM_FAMILY_INTEL command set [1] some of the new > >> commands require rev-id 2. In addition to enabling ND_CMD_CALL for these > >> new function numbers, add a lookup table for revision-ids by family > >> and function number. > >> > >> [1]: http://pmem.io/documents/NVDIMM_DSM_Interface-V1.6.pdf > >> > >> Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx> > >> --- > >> drivers/acpi/nfit/core.c | 45 ++++++++++++++++++++++++++++++++++++++++----- > >> drivers/acpi/nfit/nfit.h | 26 +++++++++++++++++++++++++- > >> 2 files changed, 65 insertions(+), 6 deletions(-) > >> > >> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c > >> index 444832b372ec..1b2c613a9d8b 100644 > >> --- a/drivers/acpi/nfit/core.c > >> +++ b/drivers/acpi/nfit/core.c > >> @@ -370,6 +370,32 @@ static union acpi_object *acpi_label_info(acpi_handle handle) > >> return pkg_to_buf(buf.pointer); > >> } > >> > >> +static u8 nfit_dsm_revid(unsigned family, unsigned func) > >> +{ > >> + static const u8 revid_table[NVDIMM_FAMILY_MAX][32] = { > >> + [NVDIMM_FAMILY_INTEL] = { > >> + [NVDIMM_INTEL_GET_MODES] = 2, > >> + [NVDIMM_INTEL_GET_FWINFO] = 2, > >> + [NVDIMM_INTEL_START_FWUPDATE] = 2, > >> + [NVDIMM_INTEL_SEND_FWUPDATE] = 2, > >> + [NVDIMM_INTEL_FINISH_FWUPDATE] = 2, > >> + [NVDIMM_INTEL_QUERY_FWUPDATE] = 2, > >> + [NVDIMM_INTEL_SET_THRESHOLD] = 2, > >> + [NVDIMM_INTEL_INJECT_ERROR] = 2, > >> + }, > >> + }; > >> + u8 id; > >> + > >> + if (family > NVDIMM_FAMILY_MAX) > >> + return 0; > >> + if (func > 31) > >> + return 0; > >> + id = revid_table[family][func]; > >> + if (id == 0) > >> + return 1; /* default */ > >> + return id; > >> +} > >> + > > > > > > Elsewhere we find NVDIMM_FAMILY_MAX == NVDIMM_FAMILY_MSFT. > > Code is off by 1 in the dimension the array revid_table. > > Nice catch, will fix. > > At least the compiler will complain if we use c99 style initialization > for that NVDIMM_FAMILY_MAX index (error: array index in initializer > exceeds array bounds). > > > This approach only ever calls revision ID 1 of a function that > > was initially defined in revision ID 1 of a spec. > > > > Is it required that FW returns same data for a given function if > > called with different revision IDs? I.e. could firmware implement > > a function N such that it returns different information if called > > with revision ID 2 versus being called with revision ID 1? > > E.g. a field that is reserved in revision 1 could have a definition > > in revision 2. My reading of the ACPI spec is this is acceptable. > > Yes, this can happen. However, for the kernel implementation it can > decide to move to rev-id2 at its leisure since ACPI mandates that > rev-id1 implementations stick around, and if the function number was > reserved in rev-id1 the kernel will already not support it. Unfortunately, we can't do that. The kernel will still need to support firmware that only knows about Rev ID 1. Example, HPE currently supports customers w/ NVDIMMS with DSM that only responds to rev id 1. Say in the future we ship a new generation of NVDIMMs that extends the DSM health function. We could make that additional info available only for rev id 2. If the table above were extended to call health function with rev id 2 so that we get the additional info for the new nvdimm, that call will stop working on what will then be legacy systems that only know rev id 1. > > > > > Not sure if this makes a difference w/ the Intel FW or not, but > > some of the structures returns were modified from the earlier > > spec. I have concerns that this approach can be correctly > > extended to other vendors. > > > [..] > >> +#define NVDIMM_FAMILY_MAX NVDIMM_FAMILY_MSFT > > > > > > Suggest this be defined immediately after where NVDIMM_FAMILY_MSFT > > is defined. That way if a new family is ever defined, its more obvious > > that NVDIMM_FAMILY_MAX needs to be redefined as well. > > These are currently defined in the userspace api header and that value > is not relevant to userspace since the kernel is free to support more > families than NVDIMM_FAMILY_MAX would imply. Also, if we as an > industry continue to add NVDIMM families something is broken with the > standardization process. > -- > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html -- ----------------------------------------------------------------------------- Jerry Hoemann Software Engineer Hewlett Packard Enterprise ----------------------------------------------------------------------------- -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
