8:21 p.m.
On Tue, Mar 15, 2016 at 03:08:14PM -0700, Dan Williams wrote:
On Tue, Mar 15, 2016 at 2:32 PM, Jerry Hoemann
<jerry.hoemann(a)hpe.com> wrote:
> Indicate to userspace that the generic 'dsm_call' capability is
> available for the given control device. Over time we want to deprecate
> the per-dsm function ioctl commands and direct everything through the
> generic envelope.
>
> Signed-off-by: Dan Williams <dan.j.williams(a)intel.com>
> Signed-off-by: Jerry Hoemann <jerry.hoemann(a)hpe.com>
> ---
> drivers/nvdimm/core.c | 3 +++
> drivers/nvdimm/dimm_devs.c | 8 +++++++-
> include/linux/libnvdimm.h | 1 +
> 3 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/nvdimm/core.c b/drivers/nvdimm/core.c
> index 79646d0..9b395e5 100644
> --- a/drivers/nvdimm/core.c
> +++ b/drivers/nvdimm/core.c
> @@ -253,6 +253,9 @@ static ssize_t commands_show(struct device *dev,
>
> for_each_set_bit(cmd, &nd_desc->dsm_mask, BITS_PER_LONG)
> len += sprintf(buf + len, "%s ",
nvdimm_bus_cmd_name(cmd));
> + if (nd_desc->call_dsm)
> + len += sprintf(buf + len, "%s ",
> + nvdimm_bus_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..89086d6 100644
> --- a/drivers/nvdimm/dimm_devs.c
> +++ b/drivers/nvdimm/dimm_devs.c
> @@ -274,14 +274,20 @@ 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);
> + struct nvdimm_bus_descriptor *nd_desc;
> 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));
> + nd_desc = nvdimm_bus->nd_desc;
> + if (nd_desc->call_dsm)
> + len += sprintf(buf + len, "%s ",
> + nvdimm_cmd_name(ND_CMD_CALL));
> len += sprintf(buf + len, "\n");
> return len;
> }
> diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
> index af31d1c..458db57 100644
> --- a/include/linux/libnvdimm.h
> +++ b/include/linux/libnvdimm.h
> @@ -68,6 +68,7 @@ struct nd_mapping {
>
> struct nvdimm_bus_descriptor {
> const struct attribute_group **attr_groups;
> + unsigned int call_dsm:1;
We don't need this at the bus level, only at the dimm level. I.e
replace dsm_mask with cmd_mask as all it refers to is the ioctls that
the dimm interface supports where command 10 is the generic
nd_cmd_call interface.
If you want to tell userspace which functions a dimm supports that can
be a true "dsm_mask" exported in sysfs under the nfit/ sub-directory.
For example:
/sys/bus/nd/devices/nmem0/commands
...shows the base/generic nd commands supported in text format, while new
/sys/bus/nd/devices/nmem0/nfit/dsm_mask
/sys/bus/nd/devices/nmem0/nfit/family
...shares the raw bit mask of ACPI _DSM command codes. I'd like to
pretend that a dimm will not support multiple families even though the
spec technically allows that. If we're forced to cross that bridge
down the road we can add a series of alt_familyX/ sub-directories to
detail the other supported families per-dimm.
The above touches upon multiple things, and I'm not sure I understand
all your comments.
1. nvdimm_bus_descriptor.dsm_mask has been around for quite a while.
I think you simply want me to rename this to cmd_mask and leave
it in nvdimm_bus_descriptor.
however, it is still a mask of acceptable dsm functions. we test
the pass thru nd_command against it.
2. nvdimm_bus_descriptor.call_dsm:1;
This was introduced in:
https://lists.01.org/pipermail/linux-nvdimm/2016-January/004052.html
This flag didn't really change the system behavior. So, I don't
think the sense of the tests on call_dsm in core.c/commands_show() and
dimm_devs.c/commands_show() is what you intended.
In the respective commands_show function, I think we only want to
call nvdimm_bus_cmd_name or nvdimm_cmd_name if and only if the underlying
DSM matches the intel example dsm. (i.e. nvdimm_bus_cmd_name and nvdimm_cmd_name
describes function names from the intel example dsm, not hpe's example.)
For root commands, there is currently no divergence, so we could leave
it as proposed, or drop it as you suggest above.
For the non-root commands, if we're using the intel dsm, we want to call
nvdimm_cmd_name. But if not, then the only command available is the pass
thru "call_dsm".
So, I'll move a flag to the nvdimm struct and set it only if we're
using NFIT_DEV_DIMM, not for NFIT_DEV_DIMM_N_HPE[1|2].
3. sysfs extensions
I had planned to do some type of extension for sysfs for passthru
after getting basic features agreed upon. But, i hadn't thought
it through deeply. As for:
/sys/bus/nd/devices/nmem0/nfit/dsm_mask
/sys/bus/nd/devices/nmem0/nfit/family
Don't we want:
/sys/bus/nd/devices/nmem0/dsm_mask
/sys/bus/nd/devices/nmem0/family
As the mask/family is associated with the dimm.
Do you consider this sysfs change as a requirement for accepting the
rest of the patch set?
Jerry
--
-----------------------------------------------------------------------------
Jerry Hoemann Software Engineer Hewlett Packard Enterprise
-----------------------------------------------------------------------------