| Message ID | a016b07e1705c6b37ae9aa4d0a1896e6b7eb6f75.1458583367.git.jerry.hoemann@hpe.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Mar 21, 2016 at 12:37 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote: > NVDIMM DSMs whose functions map to the Intel Example DSM, can > be called with the ND_IOCTL_.* ioctl interface. Those that > don't map, can only be called with the pass thru command. > > Save this indication in struct nvdimm to be passed to > the sysfs interface. > I'm having trouble understanding the intent of cmd_ioctl. I don't want a flag for "supports Intel Example DSM", I want a mask of ND commands the bus provider knows how to handle in its ->ndctl() entry point for the given dimm. The bus provider is free to provide a translation of ND command to the bus specific command. It's just an arbitrary coincidence that the current list of ND commands matches the Intel ACPI _DSM command format.
On Mon, Apr 11, 2016 at 02:43:39PM -0700, Dan Williams wrote: > On Mon, Mar 21, 2016 at 12:37 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote: > > NVDIMM DSMs whose functions map to the Intel Example DSM, can > > be called with the ND_IOCTL_.* ioctl interface. Those that > > don't map, can only be called with the pass thru command. > > > > Save this indication in struct nvdimm to be passed to > > the sysfs interface. > > > > I'm having trouble understanding the intent of cmd_ioctl. I don't > want a flag for "supports Intel Example DSM", I want a mask of ND > commands the bus provider knows how to handle in its ->ndctl() entry > point for the given dimm. The bus provider is free to provide a > translation of ND command to the bus specific command. It's just an > arbitrary coincidence that the current list of ND commands matches the > Intel ACPI _DSM command format. This is my extension of your earlier RFC patch, so I may have misunderstood your original intent. The commands_show functions prints the listing of commands for that device. There is one version for root, and one version for non-root. For root commands, both dsm spec match on names and semantics, so no code works for both nvdimms types. But for non-root commands the names/semantics don't match. So, this is an attempt to show that on an NVDIMM-N, only the pass thru command is supported.
On Mon, Apr 11, 2016 at 4:58 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote: > On Mon, Apr 11, 2016 at 02:43:39PM -0700, Dan Williams wrote: >> On Mon, Mar 21, 2016 at 12:37 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote: >> > NVDIMM DSMs whose functions map to the Intel Example DSM, can >> > be called with the ND_IOCTL_.* ioctl interface. Those that >> > don't map, can only be called with the pass thru command. >> > >> > Save this indication in struct nvdimm to be passed to >> > the sysfs interface. >> > >> >> I'm having trouble understanding the intent of cmd_ioctl. I don't >> want a flag for "supports Intel Example DSM", I want a mask of ND >> commands the bus provider knows how to handle in its ->ndctl() entry >> point for the given dimm. The bus provider is free to provide a >> translation of ND command to the bus specific command. It's just an >> arbitrary coincidence that the current list of ND commands matches the >> Intel ACPI _DSM command format. > > This is my extension of your earlier RFC patch, so I may > have misunderstood your original intent. > > The commands_show functions prints the listing of commands for that device. > There is one version for root, and one version for non-root. > > For root commands, both dsm spec match on names and semantics, so no > code works for both nvdimms types. > > But for non-root commands the names/semantics don't match. So, this > is an attempt to show that on an NVDIMM-N, only the pass thru command > is supported. Ah, ok. Rather than telling the core about a dsm_mask and a cmd_ioctl at nvdimm_create() time, it should just be passing the mask of generic ND commands that the dimm supports. For NVDIMM-N that mask will just be (1 << 10) it's up to the nfit driver to manage any ND command to ACPI _DSM command number translation.
On Mon, Apr 11, 2016 at 05:30:25PM -0700, Dan Williams wrote: > On Mon, Apr 11, 2016 at 4:58 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote: > > On Mon, Apr 11, 2016 at 02:43:39PM -0700, Dan Williams wrote: > >> On Mon, Mar 21, 2016 at 12:37 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote: > >> > NVDIMM DSMs whose functions map to the Intel Example DSM, can > >> > be called with the ND_IOCTL_.* ioctl interface. Those that > >> > don't map, can only be called with the pass thru command. > >> > > >> > Save this indication in struct nvdimm to be passed to > >> > the sysfs interface. > >> > > >> > >> I'm having trouble understanding the intent of cmd_ioctl. I don't > >> want a flag for "supports Intel Example DSM", I want a mask of ND > >> commands the bus provider knows how to handle in its ->ndctl() entry > >> point for the given dimm. The bus provider is free to provide a > >> translation of ND command to the bus specific command. It's just an > >> arbitrary coincidence that the current list of ND commands matches the > >> Intel ACPI _DSM command format. > > > > This is my extension of your earlier RFC patch, so I may > > have misunderstood your original intent. > > > > The commands_show functions prints the listing of commands for that device. > > There is one version for root, and one version for non-root. > > > > For root commands, both dsm spec match on names and semantics, so no > > code works for both nvdimms types. > > > > But for non-root commands the names/semantics don't match. So, this > > is an attempt to show that on an NVDIMM-N, only the pass thru command > > is supported. > > Ah, ok. Rather than telling the core about a dsm_mask and a cmd_ioctl > at nvdimm_create() time, it should just be passing the mask of generic > ND commands that the dimm supports. For NVDIMM-N that mask will just > be (1 << 10) it's up to the nfit driver to manage any ND command to > ACPI _DSM command number translation. dsm_mask is used in both acpi_nfit_ctl and __nd_ioctl to filter calls the functions might make. I think dsm_mask and cmd_ioctl need to be kept separate, or drop use of filtering by dsm_mask in these two functions. Or another way of controlling what commands_show produces?
On Tue, Apr 12, 2016 at 2:41 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote: > On Mon, Apr 11, 2016 at 05:30:25PM -0700, Dan Williams wrote: >> On Mon, Apr 11, 2016 at 4:58 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote: >> > On Mon, Apr 11, 2016 at 02:43:39PM -0700, Dan Williams wrote: >> >> On Mon, Mar 21, 2016 at 12:37 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote: >> >> > NVDIMM DSMs whose functions map to the Intel Example DSM, can >> >> > be called with the ND_IOCTL_.* ioctl interface. Those that >> >> > don't map, can only be called with the pass thru command. >> >> > >> >> > Save this indication in struct nvdimm to be passed to >> >> > the sysfs interface. >> >> > >> >> >> >> I'm having trouble understanding the intent of cmd_ioctl. I don't >> >> want a flag for "supports Intel Example DSM", I want a mask of ND >> >> commands the bus provider knows how to handle in its ->ndctl() entry >> >> point for the given dimm. The bus provider is free to provide a >> >> translation of ND command to the bus specific command. It's just an >> >> arbitrary coincidence that the current list of ND commands matches the >> >> Intel ACPI _DSM command format. >> > >> > This is my extension of your earlier RFC patch, so I may >> > have misunderstood your original intent. >> > >> > The commands_show functions prints the listing of commands for that device. >> > There is one version for root, and one version for non-root. >> > >> > For root commands, both dsm spec match on names and semantics, so no >> > code works for both nvdimms types. >> > >> > But for non-root commands the names/semantics don't match. So, this >> > is an attempt to show that on an NVDIMM-N, only the pass thru command >> > is supported. >> >> Ah, ok. Rather than telling the core about a dsm_mask and a cmd_ioctl >> at nvdimm_create() time, it should just be passing the mask of generic >> ND commands that the dimm supports. For NVDIMM-N that mask will just >> be (1 << 10) it's up to the nfit driver to manage any ND command to >> ACPI _DSM command number translation. > > dsm_mask is used in both acpi_nfit_ctl and __nd_ioctl to filter calls > the functions might make. I think dsm_mask and cmd_ioctl need to be > kept separate, or drop use of filtering by dsm_mask in these two > functions. > > Or another way of controlling what commands_show produces? I was thinking that the dsm_mask cease being a pointer in struct nvdimm and instead be a cmd_mask (of ND function numbers). struct nfit_mem would maintain the dsm_mask. acpi_nfit_ctl is charged with translating from the nd command to the acpi dsm. This "just works" for the existing sysfs interface, and we can export the dsm_mask in the nfit-specific attributes of the nme device.
On Tue, Apr 12, 2016 at 03:05:47PM -0700, Dan Williams wrote: > On Tue, Apr 12, 2016 at 2:41 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote: > > On Mon, Apr 11, 2016 at 05:30:25PM -0700, Dan Williams wrote: > >> On Mon, Apr 11, 2016 at 4:58 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote: > >> > On Mon, Apr 11, 2016 at 02:43:39PM -0700, Dan Williams wrote: > >> >> On Mon, Mar 21, 2016 at 12:37 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote: > >> >> > NVDIMM DSMs whose functions map to the Intel Example DSM, can > >> >> > be called with the ND_IOCTL_.* ioctl interface. Those that > >> >> > don't map, can only be called with the pass thru command. > >> >> > > >> >> > Save this indication in struct nvdimm to be passed to > >> >> > the sysfs interface. > >> >> > > >> >> > >> >> I'm having trouble understanding the intent of cmd_ioctl. I don't > >> >> want a flag for "supports Intel Example DSM", I want a mask of ND > >> >> commands the bus provider knows how to handle in its ->ndctl() entry > >> >> point for the given dimm. The bus provider is free to provide a > >> >> translation of ND command to the bus specific command. It's just an > >> >> arbitrary coincidence that the current list of ND commands matches the > >> >> Intel ACPI _DSM command format. > >> > > >> > This is my extension of your earlier RFC patch, so I may > >> > have misunderstood your original intent. > >> > > >> > The commands_show functions prints the listing of commands for that device. > >> > There is one version for root, and one version for non-root. > >> > > >> > For root commands, both dsm spec match on names and semantics, so no > >> > code works for both nvdimms types. > >> > > >> > But for non-root commands the names/semantics don't match. So, this > >> > is an attempt to show that on an NVDIMM-N, only the pass thru command > >> > is supported. > >> > >> Ah, ok. Rather than telling the core about a dsm_mask and a cmd_ioctl > >> at nvdimm_create() time, it should just be passing the mask of generic > >> ND commands that the dimm supports. For NVDIMM-N that mask will just > >> be (1 << 10) it's up to the nfit driver to manage any ND command to > >> ACPI _DSM command number translation. > > > > dsm_mask is used in both acpi_nfit_ctl and __nd_ioctl to filter calls > > the functions might make. I think dsm_mask and cmd_ioctl need to be > > kept separate, or drop use of filtering by dsm_mask in these two > > functions. > > > > Or another way of controlling what commands_show produces? > > I was thinking that the dsm_mask cease being a pointer in struct > nvdimm and instead be a cmd_mask (of ND function numbers). struct > nfit_mem would maintain the dsm_mask. acpi_nfit_ctl is charged with > translating from the nd command to the acpi dsm. This "just works" > for the existing sysfs interface, and we can export the dsm_mask in > the nfit-specific attributes of the nme device. Not sure I understand. Are you suggesting that struct nfit_mem have both a dsm_mask and a cmd_mask and when nvdimm_create is called we pass just cmd_mask and assign it to new field u64 cmd_mask (deleting the old pointer to dsm_mask?) For NVDIMM-N, the cmd_mask would just be (1<<ND_CMD_CALL) and for the pre-existent nvdimm it would be (1<<ND_CMD_CALL)|dsm_mask This would have acpi_nfit_ctl testing against cmd_mask and __nd_ioctl testing against dsm_mask. I don't know how nvdimm_create would get the nd_cmd_mask if its not determined in acpi_nfit_add_dimm when we determine uuid......
On Thu, Apr 14, 2016 at 3:58 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote: > On Tue, Apr 12, 2016 at 03:05:47PM -0700, Dan Williams wrote: [..] >> I was thinking that the dsm_mask cease being a pointer in struct >> nvdimm and instead be a cmd_mask (of ND function numbers). struct >> nfit_mem would maintain the dsm_mask. acpi_nfit_ctl is charged with >> translating from the nd command to the acpi dsm. This "just works" >> for the existing sysfs interface, and we can export the dsm_mask in >> the nfit-specific attributes of the nme device. > > Not sure I understand. > > Are you suggesting that struct nfit_mem have both a dsm_mask and a cmd_mask > and when nvdimm_create is called we pass just cmd_mask and assign it to > new field u64 cmd_mask (deleting the old pointer to dsm_mask?) Yes. > For NVDIMM-N, the cmd_mask would just be (1<<ND_CMD_CALL) and for the > pre-existent nvdimm it would be (1<<ND_CMD_CALL)|dsm_mask > Yes. > This would have acpi_nfit_ctl testing against cmd_mask and __nd_ioctl > testing against dsm_mask. __nd_ioctl would only test against cmd_mask. acpi_nfit_ctl would check if it can translate the cmd to a DSM. In the case of ND_CMD_SMART..ND_CMD_VENDOR it translate to the corresponding DSM (currently a nop for Intel DIMMs and not applicable to HP DIMMs). In the case of ND_CMD_CALL it will check the nfit_mem's dsm_mask to see if the DSM is supported and passes it through. > I don't know how nvdimm_create would get the nd_cmd_mask if its > not determined in acpi_nfit_add_dimm when we determine uuid...... nfit_mem will have a cmd_mask and dsm_mask both initialized at acpi_nfit_add_dimm() time. At nvdimm_create we just pass nfit_mem->cmd_mask.
diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c index 8d933b3..a35b939 100644 --- a/drivers/acpi/nfit.c +++ b/drivers/acpi/nfit.c @@ -61,14 +61,15 @@ struct cmd_family_tbl { int key_type; /* Exported handle */ int rev; /* _DSM rev */ u64 mask; /* 0 bit excludes underlying func.*/ + int cmd_ioctl; /* supports full cmd ioctl interface */ }; struct cmd_family_tbl nfit_cmd_family_tbl[] = { - { NFIT_DEV_BUS, ND_TYPE_BUS, 1, ~0UL}, - { 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}, - { -1, -1, -1, 0}, + { NFIT_DEV_BUS, ND_TYPE_BUS, 1, ~0UL, 1}, + { NFIT_DEV_DIMM, ND_TYPE_DIMM_INTEL1, 1, ~0UL, 1}, + { NFIT_DEV_DIMM_N_HPE1, ND_TYPE_DIMM_N_HPE1, 1, ~0UL, 0}, + { NFIT_DEV_DIMM_N_HPE2, ND_TYPE_DIMM_N_HPE2, 1, ~0UL, 0}, + { -1, -1, -1, -1, 0}, }; static u8 nfit_uuid[NFIT_UUID_MAX][16]; @@ -264,8 +265,8 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, in_buf.buffer.length = call_dsm->nd_size_in; uuid = family_to_uuid(call_dsm->nd_family); if (!uuid) { - dev_dbg(dev, "%s:%s unsupported uuid\n", dimm_name, - cmd_name); + dev_dbg(dev, "%s unsupported cmd family: %lld\n", + dimm_name, call_dsm->nd_family); return -EINVAL; } } @@ -1009,7 +1010,8 @@ static int acpi_nfit_sup_func(acpi_handle handle, const u8 *uuid, static inline void to_nfit_uuid_msk(acpi_handle handle, struct cmd_family_tbl *tbl, - u8 const **cmd_uuid, unsigned long *cmd_mask) + u8 const **cmd_uuid, unsigned long *cmd_mask, + int *cmd_ioctl) { unsigned long mask = 0; int i; @@ -1021,6 +1023,7 @@ to_nfit_uuid_msk(acpi_handle handle, struct cmd_family_tbl *tbl, if (acpi_nfit_sup_func(handle, uuid, rev, &mask)) { *cmd_mask = mask & tbl[i].mask; *cmd_uuid = uuid; + *cmd_ioctl = tbl[i].cmd_ioctl; break; } } @@ -1046,7 +1049,8 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc, } to_nfit_uuid_msk(adev_dimm->handle, nfit_cmd_family_tbl, - &nfit_mem->dsm_uuid, &nfit_mem->dsm_mask); + &nfit_mem->dsm_uuid, &nfit_mem->dsm_mask, + &nfit_mem->cmd_ioctl); return 0; } @@ -1083,7 +1087,8 @@ static int acpi_nfit_register_dimms(struct acpi_nfit_desc *acpi_desc) nvdimm = nvdimm_create(acpi_desc->nvdimm_bus, nfit_mem, acpi_nfit_dimm_attribute_groups, - flags, &nfit_mem->dsm_mask); + flags, &nfit_mem->dsm_mask, + nfit_mem->cmd_ioctl); if (!nvdimm) return -ENOMEM; diff --git a/drivers/nvdimm/core.c b/drivers/nvdimm/core.c index ce80767..24da865 100644 --- a/drivers/nvdimm/core.c +++ b/drivers/nvdimm/core.c @@ -253,6 +253,7 @@ static ssize_t commands_show(struct device *dev, for_each_set_bit(cmd, &nd_desc->cmd_mask, BITS_PER_LONG) len += sprintf(buf + len, "%s ", nvdimm_bus_cmd_name(cmd)); + len += sprintf(buf + len, "%s ", nvdimm_cmd_name(ND_CMD_CALL)); len += sprintf(buf + len, "\n"); return len; } diff --git a/drivers/nvdimm/dimm_devs.c b/drivers/nvdimm/dimm_devs.c index c56f882..65614b2 100644 --- a/drivers/nvdimm/dimm_devs.c +++ b/drivers/nvdimm/dimm_devs.c @@ -274,14 +274,17 @@ EXPORT_SYMBOL_GPL(nvdimm_provider_data); static ssize_t commands_show(struct device *dev, struct device_attribute *attr, char *buf) { + struct nvdimm_bus *nvdimm_bus = walk_to_nvdimm_bus(dev); struct nvdimm *nvdimm = to_nvdimm(dev); int cmd, len = 0; - if (!nvdimm->dsm_mask) + if (!nvdimm->dsm_mask || !nvdimm_bus) return sprintf(buf, "\n"); - for_each_set_bit(cmd, nvdimm->dsm_mask, BITS_PER_LONG) - len += sprintf(buf + len, "%s ", nvdimm_cmd_name(cmd)); + if (nvdimm->cmd_ioctl) + for_each_set_bit(cmd, nvdimm->dsm_mask, BITS_PER_LONG) + len += sprintf(buf + len, "%s ", nvdimm_cmd_name(cmd)); + len += sprintf(buf + len, "%s ", nvdimm_cmd_name(ND_CMD_CALL)); len += sprintf(buf + len, "\n"); return len; } @@ -340,7 +343,7 @@ EXPORT_SYMBOL_GPL(nvdimm_attribute_group); struct nvdimm *nvdimm_create(struct nvdimm_bus *nvdimm_bus, void *provider_data, const struct attribute_group **groups, unsigned long flags, - unsigned long *dsm_mask) + unsigned long *dsm_mask, int cmd_ioctl) { struct nvdimm *nvdimm = kzalloc(sizeof(*nvdimm), GFP_KERNEL); struct device *dev; @@ -356,6 +359,7 @@ struct nvdimm *nvdimm_create(struct nvdimm_bus *nvdimm_bus, void *provider_data, nvdimm->provider_data = provider_data; nvdimm->flags = flags; nvdimm->dsm_mask = dsm_mask; + nvdimm->cmd_ioctl = cmd_ioctl; atomic_set(&nvdimm->busy, 0); dev = &nvdimm->dev; dev_set_name(dev, "nmem%d", nvdimm->id); diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h index 67a1ba0..154986a 100644 --- a/include/linux/libnvdimm.h +++ b/include/linux/libnvdimm.h @@ -133,7 +133,7 @@ const char *nvdimm_name(struct nvdimm *nvdimm); void *nvdimm_provider_data(struct nvdimm *nvdimm); struct nvdimm *nvdimm_create(struct nvdimm_bus *nvdimm_bus, void *provider_data, const struct attribute_group **groups, unsigned long flags, - unsigned long *dsm_mask); + unsigned long *dsm_mask, int cmd_ioctl); const struct nd_cmd_desc *nd_cmd_dimm_desc(int cmd); const struct nd_cmd_desc *nd_cmd_bus_desc(int cmd); u32 nd_cmd_in_size(struct nvdimm *nvdimm, int cmd,
NVDIMM DSMs whose functions map to the Intel Example DSM, can be called with the ND_IOCTL_.* ioctl interface. Those that don't map, can only be called with the pass thru command. Save this indication in struct nvdimm to be passed to the sysfs interface. Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com> --- drivers/acpi/nfit.c | 25 +++++++++++++++---------- drivers/nvdimm/core.c | 1 + drivers/nvdimm/dimm_devs.c | 12 ++++++++---- include/linux/libnvdimm.h | 2 +- 4 files changed, 25 insertions(+), 15 deletions(-)