Re: [PATCH v8 05/10] nvdimm: Add IOCTL pass thru functions

On Mon, Mar 21, 2016 at 12:37 PM, Jerry Hoemann <jerry.hoemann(a)hpe.com&gt; wrote: > Add ioctl command ND_CMD_CALL_DSM to acpi_nfit_ctl and __nd_ioctl which > allow kernel to call a nvdimm's _DSM as a passthru without using the > marshaling code of the nd_cmd_desc. > > Signed-off-by: Jerry Hoemann <jerry.hoemann(a)hpe.com&gt; > --- > drivers/acpi/nfit.c | 141 ++++++++++++++++++++++++++++++++++++++++++++------- > drivers/nvdimm/bus.c | 43 +++++++++++++++- > 2 files changed, 166 insertions(+), 18 deletions(-) > > diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c > index d0f35e6..9511ab4 100644 > --- a/drivers/acpi/nfit.c > +++ b/drivers/acpi/nfit.c > @@ -56,6 +56,21 @@ struct nfit_table_prev { > struct list_head flushes; > }; > > +struct cmd_family_tbl { > + enum nfit_uuids key_uuid; /* Internal handle */ > + int key_type; /* Exported handle */ > + int rev; /* _DSM rev */ > + u64 mask; /* 0 bit excludes underlying func.*/ > +}; > + > +struct cmd_family_tbl nfit_cmd_family_tbl[] = { > + { NFIT_DEV_BUS, ND_TYPE_BUS, 1, ~0UL}, Per the comment on patch2 lets kill this for now. > + { NFIT_DEV_DIMM, ND_TYPE_DIMM_INTEL1, 1, ~0UL}, > + { NFIT_DEV_DIMM_N_HPE1, ND_TYPE_DIMM_N_HPE1, 1, ~0UL}, > + { NFIT_DEV_DIMM_N_HPE2, ND_TYPE_DIMM_N_HPE2, 1, ~0UL}, Why does the mask default to all supported? I assume this should be the known-valid mask for each type, i.e. 0x3fe for ND_TYPE_DIMM_INTEL1.

> > + > +/* > + * determine if the _DSM specified by UUID is supported and return > + * mask of supported functions in nd_cmd_mask. > + */ > + > +static int acpi_nfit_sup_func(acpi_handle handle, const u8 *uuid, > + int rev, unsigned long *nd_cmd_mask) > +{ > + int i; > + u64 mask = 0; > + union acpi_object *obj; > + > + obj = acpi_evaluate_dsm(handle, uuid, rev, 0, NULL); > + if (!obj) > + return 0; > + /* For compatibility, old BIOSes may return an integer */ > + if (obj->type == ACPI_TYPE_INTEGER) > + mask = obj->integer.value; > + else if (obj->type == ACPI_TYPE_BUFFER) > + for (i = 0; i < obj->buffer.length && i < 8; i++) > + mask |= (((u8)obj->buffer.pointer[i]) << (i * 8)); > + ACPI_FREE(obj); > + > + *nd_cmd_mask = mask; > + > + return !!mask; > +} > + > + > +static inline void > +to_nfit_uuid_msk(acpi_handle handle, struct cmd_family_tbl *tbl, > + u8 const **cmd_uuid, unsigned long *cmd_mask) > +{ > + unsigned long mask = 0; > + int i; > + > + for (i = 0; tbl[i].key_uuid >= 0 ; i++) { > + const u8 *uuid = to_nfit_uuid(tbl[i].key_uuid); > + int rev = tbl[i].rev; > + > + if (acpi_nfit_sup_func(handle, uuid, rev, &mask)) { > + *cmd_mask = mask & tbl[i].mask; > + *cmd_uuid = uuid; > + break; > + } > + } > +} > + > static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc, > struct nfit_mem *nfit_mem, u32 device_handle) > { > struct acpi_device *adev, *adev_dimm; > struct device *dev = acpi_desc->dev; > - const u8 *uuid = to_nfit_uuid(NFIT_DEV_DIMM); > - int i; > > nfit_mem->dsm_mask = acpi_desc->dimm_dsm_force_en; > adev = to_acpi_dev(acpi_desc); > @@ -939,9 +1045,8 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc, > return force_enable_dimms ? 0 : -ENODEV; > } > > - for (i = ND_CMD_SMART; i <= ND_CMD_VENDOR; i++) > - if (acpi_check_dsm(adev_dimm->handle, uuid, 1, 1ULL << i)) > - set_bit(i, &nfit_mem->dsm_mask); > + to_nfit_uuid_msk(adev_dimm->handle, nfit_cmd_family_tbl, > + &nfit_mem->dsm_uuid, &nfit_mem->dsm_mask); Why is this replacing acpi_check_dsm() with an open-coded implementation?