| Message ID | 9f04df3b80c68dd6406cfee5ce11a2cd8ed33b64.1460936121.git.jerry.hoemann@hpe.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sonntag, 17. April 2016 17:38:47 CEST Jerry Hoemann wrote: > The pass thru calls return command mask. Previously, bit zero > wasn't part of command mask, but as it is now, this left commands_show > displaying "unknown" for function zero. Add an ioctl interface > to return command mask. > > Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com> > --- Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
On Sun, Apr 17, 2016 at 4:38 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote: > The pass thru calls return command mask. Previously, bit zero > wasn't part of command mask, but as it is now, this left commands_show > displaying "unknown" for function zero. Add an ioctl interface > to return command mask. > > Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com> > --- > drivers/nvdimm/bus.c | 10 ++++++++-- > include/uapi/linux/ndctl.h | 9 +++++++++ > 2 files changed, 17 insertions(+), 2 deletions(-) Let's not add yet another ioctl for this... just add a 'dsm_mask' attribute to the nfit_mem device in acpi_nfit_dimm_attributes.
On Tue, Apr 19, 2016 at 10:45:48AM -0700, Dan Williams wrote: > On Sun, Apr 17, 2016 at 4:38 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote: > > The pass thru calls return command mask. Previously, bit zero > > wasn't part of command mask, but as it is now, this left commands_show > > displaying "unknown" for function zero. Add an ioctl interface > > to return command mask. > > > > Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com> > > --- > > drivers/nvdimm/bus.c | 10 ++++++++-- > > include/uapi/linux/ndctl.h | 9 +++++++++ > > 2 files changed, 17 insertions(+), 2 deletions(-) > > Let's not add yet another ioctl for this... just add a 'dsm_mask' > attribute to the nfit_mem device in acpi_nfit_dimm_attributes. I don't understand this comment. I can change just static const char * const names[] = { + [ND_CMD_IMPLEMENTED] = "cmd_mask", [ND_CMD_ARS_CAP] = "ars_cap", [ND_CMD_ARS_START] = "ars_start", [ND_CMD_ARS_STATUS] = "ars_status", @@ -150,6 +155,7 @@ static inline const char *nvdimm_bus_cmd_name(unsigned cmd) static inline const char *nvdimm_cmd_name(unsigned cmd) { static const char * const names[] = { + [ND_CMD_IMPLEMENTED] = "cmd_mask", And have same effect w/o adding the full ioctl support. But I don't see how modifying acpi_nfit_dimm_attributes affects what commands show will display. Are you asking me to also add a new attribute to sysfs to display the command mask?
On Wed, Apr 20, 2016 at 10:41 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote: > On Tue, Apr 19, 2016 at 10:45:48AM -0700, Dan Williams wrote: >> On Sun, Apr 17, 2016 at 4:38 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote: >> > The pass thru calls return command mask. Previously, bit zero >> > wasn't part of command mask, but as it is now, this left commands_show >> > displaying "unknown" for function zero. Add an ioctl interface >> > to return command mask. >> > >> > Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com> >> > --- >> > drivers/nvdimm/bus.c | 10 ++++++++-- >> > include/uapi/linux/ndctl.h | 9 +++++++++ >> > 2 files changed, 17 insertions(+), 2 deletions(-) >> >> Let's not add yet another ioctl for this... just add a 'dsm_mask' >> attribute to the nfit_mem device in acpi_nfit_dimm_attributes. > > I don't understand this comment. So, I'm trying to find the change that made bit zero part of the command mask, I think that is the source of the confusion. Why do we need to ever set bit zero in the cmd_mask? > I can change just > static const char * const names[] = { > + [ND_CMD_IMPLEMENTED] = "cmd_mask", > [ND_CMD_ARS_CAP] = "ars_cap", > [ND_CMD_ARS_START] = "ars_start", > [ND_CMD_ARS_STATUS] = "ars_status", > @@ -150,6 +155,7 @@ static inline const char *nvdimm_bus_cmd_name(unsigned cmd) > static inline const char *nvdimm_cmd_name(unsigned cmd) > { > static const char * const names[] = { > + [ND_CMD_IMPLEMENTED] = "cmd_mask", > > And have same effect w/o adding the full ioctl support. ...but there's no ND_IOCTL_IMPLEMENTED, so why would userspace ever care to know the name of a command it can't issue? > But I don't see how modifying acpi_nfit_dimm_attributes affects what > commands show will display. It doesn't, but I was wondering if you were trying to add support for discovering the dsm_mask via an ioctl. > Are you asking me to also add a new attribute to sysfs to display the > command mask? Yes, I think it would be useful to communicate the *dsm_mask* in sysfs, but the cmd_mask can stay as is without adding an ND_CMD_IMPLEMENTED entry.
On Thu, Apr 21, 2016 at 04:52:17AM -0700, Dan Williams wrote: > On Wed, Apr 20, 2016 at 10:41 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote: > > On Tue, Apr 19, 2016 at 10:45:48AM -0700, Dan Williams wrote: > >> On Sun, Apr 17, 2016 at 4:38 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote: > >> > The pass thru calls return command mask. Previously, bit zero > >> > wasn't part of command mask, but as it is now, this left commands_show > >> > displaying "unknown" for function zero. Add an ioctl interface > >> > to return command mask. > >> > > >> > Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com> > >> > --- > >> > drivers/nvdimm/bus.c | 10 ++++++++-- > >> > include/uapi/linux/ndctl.h | 9 +++++++++ > >> > 2 files changed, 17 insertions(+), 2 deletions(-) > >> > >> Let's not add yet another ioctl for this... just add a 'dsm_mask' > >> attribute to the nfit_mem device in acpi_nfit_dimm_attributes. > > > > I don't understand this comment. > > So, I'm trying to find the change that made bit zero part of the > command mask, I think that is the source of the confusion. Why do we > need to ever set bit zero in the cmd_mask? > In patch set [RFC v9 1/5] nvdimm: Add IOCTL pass thru functions 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); + const u8 *uuid; int i; nfit_mem->dsm_mask = acpi_desc->dimm_dsm_force_en; @@ -939,7 +1018,12 @@ 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 (!determine_uuid(adev_dimm->handle, nfit_cmd_family_tbl, + &nfit_mem->dsm_uuid)) + return force_enable_dimms ? 0 : -ENODEV; + + uuid = nfit_mem->dsm_uuid; + for (i = 0; i <= ND_MAX_CMD; i++) if (acpi_check_dsm(adev_dimm->handle, uuid, 1, 1ULL << i)) set_bit(i, &nfit_mem->dsm_mask); > > I can change just > > static const char * const names[] = { > > + [ND_CMD_IMPLEMENTED] = "cmd_mask", > > [ND_CMD_ARS_CAP] = "ars_cap", > > [ND_CMD_ARS_START] = "ars_start", > > [ND_CMD_ARS_STATUS] = "ars_status", > > @@ -150,6 +155,7 @@ static inline const char *nvdimm_bus_cmd_name(unsigned cmd) > > static inline const char *nvdimm_cmd_name(unsigned cmd) > > { > > static const char * const names[] = { > > + [ND_CMD_IMPLEMENTED] = "cmd_mask", > > > > And have same effect w/o adding the full ioctl support. > > ...but there's no ND_IOCTL_IMPLEMENTED, so why would userspace ever > care to know the name of a command it can't issue? I added 0 to dsm mask so a user applications could know which dsm function are actually implemented. The architecture defines the function in the DSM, but a particular implementation may or may not implement all functions of a dsm. The dsm mask tells which function the FW implements. And yes, we have different SKUs and they don't all implement all/same functions. A side effect of having "0" in the dsm_mask is that when commands_show executes: for_each_set_bit(cmd, &nvdimm->cmd_mask, BITS_PER_LONG) len += sprintf(buf + len, "%s ", nvdimm_cmd_name(cmd)); len += sprintf(buf + len, "\n"); The value of names[ND_CMD_IMPLEMENTED] is printed. Currently that value is "unknown" which I didn't think looked correct. Hence, the change. I my other patch, I am constructing cmd_mask as either (1<<ND_CMD)|dsm_mask -or- (1<<ND_CMD) depending upon UUID. Now construction of cmd_mask is subject of an earlier patch set. > > > But I don't see how modifying acpi_nfit_dimm_attributes affects what > > commands show will display. > > It doesn't, but I was wondering if you were trying to add support for > discovering the dsm_mask via an ioctl. > > > Are you asking me to also add a new attribute to sysfs to display the > > command mask? > > Yes, I think it would be useful to communicate the *dsm_mask* in > sysfs, but the cmd_mask can stay as is without adding an > ND_CMD_IMPLEMENTED entry. I think this is a good idea and am completely happy to do it, but want to defer this until after we get basic functionality completed.
On Thu, Apr 21, 2016 at 9:52 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote: > On Thu, Apr 21, 2016 at 04:52:17AM -0700, Dan Williams wrote: >> On Wed, Apr 20, 2016 at 10:41 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote: >> > On Tue, Apr 19, 2016 at 10:45:48AM -0700, Dan Williams wrote: >> >> On Sun, Apr 17, 2016 at 4:38 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote: >> >> > The pass thru calls return command mask. Previously, bit zero >> >> > wasn't part of command mask, but as it is now, this left commands_show >> >> > displaying "unknown" for function zero. Add an ioctl interface >> >> > to return command mask. >> >> > >> >> > Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com> >> >> > --- >> >> > drivers/nvdimm/bus.c | 10 ++++++++-- >> >> > include/uapi/linux/ndctl.h | 9 +++++++++ >> >> > 2 files changed, 17 insertions(+), 2 deletions(-) >> >> >> >> Let's not add yet another ioctl for this... just add a 'dsm_mask' >> >> attribute to the nfit_mem device in acpi_nfit_dimm_attributes. >> > >> > I don't understand this comment. >> >> So, I'm trying to find the change that made bit zero part of the >> command mask, I think that is the source of the confusion. Why do we >> need to ever set bit zero in the cmd_mask? >> > > In patch set > [RFC v9 1/5] nvdimm: Add IOCTL pass thru functions Thanks for copying this here, I had overlooked this aspect of the patch. > > > 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); > + const u8 *uuid; > int i; > > nfit_mem->dsm_mask = acpi_desc->dimm_dsm_force_en; > @@ -939,7 +1018,12 @@ 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 (!determine_uuid(adev_dimm->handle, nfit_cmd_family_tbl, > + &nfit_mem->dsm_uuid)) > + return force_enable_dimms ? 0 : -ENODEV; > + > + uuid = nfit_mem->dsm_uuid; > + for (i = 0; i <= ND_MAX_CMD; i++) > if (acpi_check_dsm(adev_dimm->handle, uuid, 1, 1ULL << i)) > set_bit(i, &nfit_mem->dsm_mask); > This is confusing the ND_CMD function number space with DSMs. We already know the possible commands that may be supported because they are specified per-family. This goes back to my other request to limit the kernel to only handling known valid function numbers, i.e. don't probe for known invalid functions. For_example: unsigned long famliy_mask = nfit_cmd_family_tbl[family_id].family_mask; for_each_set_bit(i, &family_mask, BITS_PER_LONG) if (acpi_check_dsm(adev_dimm->handle, uuid, 1, 1ULL << i)) set_bit(i, &nfit_mem->dsm_mask); Where, for example, family_mask is statically initialized to 0x1ff in the Intel case. >> > I can change just >> > static const char * const names[] = { >> > + [ND_CMD_IMPLEMENTED] = "cmd_mask", >> > [ND_CMD_ARS_CAP] = "ars_cap", >> > [ND_CMD_ARS_START] = "ars_start", >> > [ND_CMD_ARS_STATUS] = "ars_status", >> > @@ -150,6 +155,7 @@ static inline const char *nvdimm_bus_cmd_name(unsigned cmd) >> > static inline const char *nvdimm_cmd_name(unsigned cmd) >> > { >> > static const char * const names[] = { >> > + [ND_CMD_IMPLEMENTED] = "cmd_mask", >> > >> > And have same effect w/o adding the full ioctl support. >> >> ...but there's no ND_IOCTL_IMPLEMENTED, so why would userspace ever >> care to know the name of a command it can't issue? > > > I added 0 to dsm mask so a user applications could know which dsm > function are actually implemented. Right, I don't want to add an ioctl for discovering dsm_mask. [..] >> Yes, I think it would be useful to communicate the *dsm_mask* in >> sysfs, but the cmd_mask can stay as is without adding an >> ND_CMD_IMPLEMENTED entry. > > > I think this is a good idea and am completely happy to do it, but > want to defer this until after we get basic functionality completed. > Why wait, it's a trivial amount of code. Replace the above changes with something like the below (untested), or just let me know and I'll append a patch like this to the end of your series. @@ -802,6 +802,16 @@ static ssize_t handle_show(struct device *dev, } static DEVICE_ATTR_RO(handle); +static ssize_t dsm_mask_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct nvdimm *nvdimm = to_nvdimm(dev); + struct nfit_mem *nfit_mem = nvdimm_provider_data(nvdimm); + + return sprintf(buf, "%#lx\n", nfit_mem->dsm_mask); +} +static DEVICE_ATTR_RO(dsm_mask); + static ssize_t phys_id_show(struct device *dev, struct device_attribute *attr, char *buf) { @@ -879,6 +889,7 @@ static struct attribute *acpi_nfit_dimm_attributes[] = { &dev_attr_serial.attr, &dev_attr_rev_id.attr, &dev_attr_flags.attr, + &dev_attr_dsm_mask.attr, NULL, };
On Thu, Apr 21, 2016 at 11:18:40AM -0700, Dan Williams wrote: > On Thu, Apr 21, 2016 at 9:52 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote: > > On Thu, Apr 21, 2016 at 04:52:17AM -0700, Dan Williams wrote: > >> On Wed, Apr 20, 2016 at 10:41 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote: > >> > On Tue, Apr 19, 2016 at 10:45:48AM -0700, Dan Williams wrote: > >> >> On Sun, Apr 17, 2016 at 4:38 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote: > > } > > > > - for (i = ND_CMD_SMART; i <= ND_CMD_VENDOR; i++) > > + if (!determine_uuid(adev_dimm->handle, nfit_cmd_family_tbl, > > + &nfit_mem->dsm_uuid)) > > + return force_enable_dimms ? 0 : -ENODEV; > > + > > + uuid = nfit_mem->dsm_uuid; > > + for (i = 0; i <= ND_MAX_CMD; i++) > > if (acpi_check_dsm(adev_dimm->handle, uuid, 1, 1ULL << i)) > > set_bit(i, &nfit_mem->dsm_mask); > > > > This is confusing the ND_CMD function number space with DSMs. We Yes, should have called it something like ND_MAX_DSM_FUN_IDX. I am looking at basing it off of bits in specified bits like below. > already know the possible commands that may be supported because they > are specified per-family. This goes back to my other request to limit > the kernel to only handling known valid function numbers, i.e. don't > probe for known invalid functions. > > For_example: > > unsigned long family_mask = nfit_cmd_family_tbl[family_id].family_mask; > > for_each_set_bit(i, &family_mask, BITS_PER_LONG) > if (acpi_check_dsm(adev_dimm->handle, uuid, 1, 1ULL << i)) > set_bit(i, &nfit_mem->dsm_mask); > > Where, for example, family_mask is statically initialized to 0x1ff in > the Intel case. > > >> > I can change just > >> > static const char * const names[] = { > >> > + [ND_CMD_IMPLEMENTED] = "cmd_mask", > >> > [ND_CMD_ARS_CAP] = "ars_cap", > >> > [ND_CMD_ARS_START] = "ars_start", > >> > [ND_CMD_ARS_STATUS] = "ars_status", > >> > @@ -150,6 +155,7 @@ static inline const char *nvdimm_bus_cmd_name(unsigned cmd) > >> > static inline const char *nvdimm_cmd_name(unsigned cmd) > >> > { > >> > static const char * const names[] = { > >> > + [ND_CMD_IMPLEMENTED] = "cmd_mask", > >> > > >> > And have same effect w/o adding the full ioctl support. > >> > >> ...but there's no ND_IOCTL_IMPLEMENTED, so why would userspace ever > >> care to know the name of a command it can't issue? > > > > > > I added 0 to dsm mask so a user applications could know which dsm > > function are actually implemented. > > Right, I don't want to add an ioctl for discovering dsm_mask. I can interpret your statement in multiple ways. We need to be able to call the query command (function 0) in pass thru to support our firmware. Now, if you're just saying you don't want to add +#define ND_IOCTL_CMD_MASK _IOWR(ND_IOCTL, ND_CMD_IMPLEMENTED,\ + struct nd_cmd_mask) and other associated changes in ndctl.h, that is an entirely different and I can rework that patch.
On Fri, Apr 22, 2016 at 4:15 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote: > On Thu, Apr 21, 2016 at 11:18:40AM -0700, Dan Williams wrote: [..] >> Right, I don't want to add an ioctl for discovering dsm_mask. > > I can interpret your statement in multiple ways. > > We need to be able to call the query command (function 0) in pass thru > to support our firmware. > > Now, if you're just saying you don't want to add > > > +#define ND_IOCTL_CMD_MASK _IOWR(ND_IOCTL, ND_CMD_IMPLEMENTED,\ > + struct nd_cmd_mask) > > > and other associated changes in ndctl.h, that is an entirely different > and I can rework that patch. So, I'm saying I don't want ND_CMD_CALL to be used to issue the "is dsm function implemented?" check. Especially because the kernel may want to limit the mask for one reason or another, the raw data returned from the DSM may not be accurate. It also just comes across as inefficient to me when the kernel has already probed this information and is maintaining it internally. Lastly ioctls are terrible and having one less piece of information that is conveyed via an ioctl in favor of a sysfs attribute is a win.
diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c index b133301..047c364 100644 --- a/drivers/nvdimm/bus.c +++ b/drivers/nvdimm/bus.c @@ -404,7 +404,10 @@ void nvdimm_bus_destroy_ndctl(struct nvdimm_bus *nvdimm_bus) } static const struct nd_cmd_desc __nd_cmd_dimm_descs[] = { - [ND_CMD_IMPLEMENTED] = { }, + [ND_CMD_IMPLEMENTED] = { + .out_num = 1, + .out_sizes = { 8, }, + }, [ND_CMD_SMART] = { .out_num = 2, .out_sizes = { 4, 128, }, @@ -456,7 +459,10 @@ const struct nd_cmd_desc *nd_cmd_dimm_desc(int cmd) EXPORT_SYMBOL_GPL(nd_cmd_dimm_desc); static const struct nd_cmd_desc __nd_cmd_bus_descs[] = { - [ND_CMD_IMPLEMENTED] = { }, + [ND_CMD_IMPLEMENTED] = { + .out_num = 1, + .out_sizes = { 1, }, + }, [ND_CMD_ARS_CAP] = { .in_num = 2, .in_sizes = { 8, 8, }, diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h index 9214af7..8c086b9 100644 --- a/include/uapi/linux/ndctl.h +++ b/include/uapi/linux/ndctl.h @@ -20,6 +20,10 @@ struct nd_cmd_smart { __u8 data[128]; } __packed; +struct nd_cmd_mask { + __u8 data[8]; +} __packed; + struct nd_cmd_smart_threshold { __u32 status; __u8 data[8]; @@ -136,6 +140,7 @@ enum { static inline const char *nvdimm_bus_cmd_name(unsigned cmd) { static const char * const names[] = { + [ND_CMD_IMPLEMENTED] = "cmd_mask", [ND_CMD_ARS_CAP] = "ars_cap", [ND_CMD_ARS_START] = "ars_start", [ND_CMD_ARS_STATUS] = "ars_status", @@ -150,6 +155,7 @@ static inline const char *nvdimm_bus_cmd_name(unsigned cmd) static inline const char *nvdimm_cmd_name(unsigned cmd) { static const char * const names[] = { + [ND_CMD_IMPLEMENTED] = "cmd_mask", [ND_CMD_SMART] = "smart", [ND_CMD_SMART_THRESHOLD] = "smart_thresh", [ND_CMD_DIMM_FLAGS] = "flags", @@ -169,6 +175,9 @@ static inline const char *nvdimm_cmd_name(unsigned cmd) #define ND_IOCTL 'N' +#define ND_IOCTL_CMD_MASK _IOWR(ND_IOCTL, ND_CMD_IMPLEMENTED,\ + struct nd_cmd_mask) + #define ND_IOCTL_SMART _IOWR(ND_IOCTL, ND_CMD_SMART,\ struct nd_cmd_smart)
The pass thru calls return command mask. Previously, bit zero wasn't part of command mask, but as it is now, this left commands_show displaying "unknown" for function zero. Add an ioctl interface to return command mask. Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com> --- drivers/nvdimm/bus.c | 10 ++++++++-- include/uapi/linux/ndctl.h | 9 +++++++++ 2 files changed, 17 insertions(+), 2 deletions(-)