The NVDIMM code in the kernel supports an IOCTL interface to user
space based upon the Intel Example DSM:
http://pmem.io/documents/NVDIMM_DSM_Interface_Example.pdf
This interface cannot be used by other NVDIMM DSMs that support
incompatible functions.
This patch set adds a generic "passthru" IOCTL interface which
is not tied to a particular DSM.
A new IOCTL type "P" is added for the pass thru call.
The new data structure ndn_pkg serves as a wrapper for the passthru
calls. This wrapper supplies the data that the kernel needs to
make the _DSM call.
Unlike the definitions of the _DSM functions themselves, the ndn_pkg
provides the calling information (input/output sizes) in an uniform
manner making the kernel marshaling of the arguments straight
forward.
This shifts the marshaling burden from the kernel to the user
space application while still permitting the kernel to internally
calling _DSM functions.
To make the resultant kernel code easier to understand the existing
functions acpi_nfit_ctl and __nd_ioctl were renamed to .*_intel to
denote calling mechanism as in 4.2 tailored to the Intel Example DSM.
New functions acpi_nfit_ctl_passthru and __nd_ioctl_passthru were
created to supply the pass thru interface.
These changes are based upon the 4.3 kernel.
Jerry Hoemann (4):
nvdimm: Add wrapper for IOCTL pass thru.
nvdimm: Add IOCTL pass thru
nvdimm: Add IOCTL pass thru
nvdimm: rename functions that aren't IOCTL passthru
drivers/acpi/nfit.c | 91 ++++++++++++++++++++++++++++++++--
drivers/nvdimm/bus.c | 118 +++++++++++++++++++++++++++++++++++++++++----
drivers/nvdimm/dimm_devs.c | 6 +--
include/linux/libnvdimm.h | 3 +-
include/uapi/linux/ndctl.h | 20 +++++++-
5 files changed, 220 insertions(+), 18 deletions(-)
--
1.7.11.3
Add IOCTL type 'P' to denote NVDIMM_TYPE_PASSTHRU.
Add struct ndn_pkg which the pass thru IOCTL interfaces uses.
ndn_pkg serves as a wrapper for the data being passed to the
underlying DSM and specifies siz data in a uniform manner allowing
the kernel to call the DSM without knowing specifics of the DSM.
Signed-off-by: Jerry Hoemann <[email protected]>
---
include/uapi/linux/ndctl.h | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h
index 5b4a4be..1c81a99 100644
--- a/include/uapi/linux/ndctl.h
+++ b/include/uapi/linux/ndctl.h
@@ -15,6 +15,9 @@
#include <linux/types.h>
+#define NVDIMM_TYPE_INTEL 'N'
+#define NVDIMM_TYPE_PASSTHRU 'P'
+
struct nd_cmd_smart {
__u32 status;
__u8 data[128];
@@ -148,7 +151,8 @@ static inline const char *nvdimm_cmd_name(unsigned cmd)
return "unknown";
}
-#define ND_IOCTL 'N'
+#define ND_IOCTL NVDIMM_TYPE_INTEL
+
#define ND_IOCTL_SMART _IOWR(ND_IOCTL, ND_CMD_SMART,\
struct nd_cmd_smart)
@@ -204,4 +208,18 @@ enum ars_masks {
ARS_STATUS_MASK = 0x0000FFFF,
ARS_EXT_STATUS_SHIFT = 16,
};
+
+
+struct ndn_pkg {
+ struct {
+ __u8 dsm_uuid[16];
+ __u32 dsm_in; /* size of _DSM input */
+ __u32 dsm_out; /* size of user buffer */
+ __u32 dsm_rev; /* revision of dsm call */
+ __u32 res[8]; /* reserved must be zero */
+ __u32 dsm_size; /* size _DSM would write */
+ } h;
+ unsigned char buf[];
+} __packed;
+
#endif /* __NDCTL_H__ */
--
1.7.11.3
Add internal data structure for ndctl_passthru call.
Signed-off-by: Jerry Hoemann <[email protected]>
---
include/linux/libnvdimm.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
index 3f021dc..01117e1 100644
--- a/include/linux/libnvdimm.h
+++ b/include/linux/libnvdimm.h
@@ -72,6 +72,7 @@ struct nvdimm_bus_descriptor {
unsigned long dsm_mask;
char *provider_name;
ndctl_fn ndctl;
+ ndctl_fn ndctl_passthru;
};
struct nd_cmd_desc {
--
1.7.11.3
Add functions acpi_nfit_ctl_passthru and __nd_ioctl_passthru which allow
kernel to call a nvdimm's _DSM as a passthru without the marshaling code
of the nd_cmd_desc.
Signed-off-by: Jerry Hoemann <[email protected]>
---
drivers/acpi/nfit.c | 85 +++++++++++++++++++++++++++++++++++++++++
drivers/nvdimm/bus.c | 105 +++++++++++++++++++++++++++++++++++++++++++++++++--
2 files changed, 186 insertions(+), 4 deletions(-)
diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
index c1b8d03..a6b458a 100644
--- a/drivers/acpi/nfit.c
+++ b/drivers/acpi/nfit.c
@@ -190,6 +190,90 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc,
return rc;
}
+
+static int acpi_nfit_ctl_passthru(struct nvdimm_bus_descriptor *nd_desc,
+ struct nvdimm *nvdimm, unsigned int cmd, void *buf,
+ unsigned int buf_len)
+{
+ struct acpi_nfit_desc *acpi_desc = to_acpi_nfit_desc(nd_desc);
+ union acpi_object in_obj, in_buf, *out_obj;
+ struct device *dev = acpi_desc->dev;
+ const char *dimm_name;
+ acpi_handle handle;
+ const u8 *uuid;
+ int rc = 0;
+ int rev = 0;
+
+ struct ndn_pkg *pkg = buf;
+
+ if (nvdimm) {
+ struct nfit_mem *nfit_mem = nvdimm_provider_data(nvdimm);
+ struct acpi_device *adev = nfit_mem->adev;
+
+ if (!adev)
+ return -ENOTTY;
+ dimm_name = nvdimm_name(nvdimm);
+ handle = adev->handle;
+ } else {
+ struct acpi_device *adev = to_acpi_dev(acpi_desc);
+
+ handle = adev->handle;
+ dimm_name = "bus";
+ }
+ uuid = pkg->h.dsm_uuid;
+ rev = pkg->h.dsm_rev ? pkg->h.dsm_rev : 1;
+
+ in_obj.type = ACPI_TYPE_PACKAGE;
+ in_obj.package.count = 1;
+ in_obj.package.elements = &in_buf;
+ in_buf.type = ACPI_TYPE_BUFFER;
+ in_buf.buffer.pointer = (void *) &pkg->buf;
+
+ in_buf.buffer.length = pkg->h.dsm_in;
+
+ if (IS_ENABLED(CONFIG_ACPI_NFIT_DEBUG)) {
+ dev_dbg(dev, "%s:%s cmd: %d input length: %d\n", __func__,
+ dimm_name, cmd, in_buf.buffer.length);
+ print_hex_dump_debug("cmd: ", DUMP_PREFIX_OFFSET, 4,
+ 4, in_buf.buffer.pointer, min_t(u32, 128,
+ in_buf.buffer.length), true);
+ }
+
+ out_obj = acpi_evaluate_dsm(handle, uuid, rev, cmd, &in_obj);
+ if (!out_obj) {
+ dev_dbg(dev, "%s:%s _DSM failed cmd: %d\n", __func__, dimm_name,
+ cmd);
+ return -EINVAL;
+ }
+
+ if (out_obj->package.type != ACPI_TYPE_BUFFER) {
+ dev_dbg(dev, "%s:%s unexpected output object type cmd: %d type: %d\n",
+ __func__, dimm_name, cmd, out_obj->type);
+ rc = -EINVAL;
+ goto out;
+ }
+
+ if (IS_ENABLED(CONFIG_ACPI_NFIT_DEBUG)) {
+ dev_dbg(dev, "%s:%s cmd: %d output length: %d\n", __func__,
+ dimm_name, cmd, out_obj->buffer.length);
+ print_hex_dump_debug("cmd: ", DUMP_PREFIX_OFFSET, 4,
+ 4, out_obj->buffer.pointer, min_t(u32, 128,
+ out_obj->buffer.length), true);
+ }
+
+ pkg->h.dsm_size = out_obj->buffer.length;
+ memcpy(pkg->buf + pkg->h.dsm_in,
+ out_obj->buffer.pointer,
+ min(pkg->h.dsm_size, pkg->h.dsm_out));
+
+
+ out:
+ ACPI_FREE(out_obj);
+
+ return rc;
+}
+
+
static const char *spa_type_name(u16 type)
{
static const char *to_name[] = {
@@ -1614,6 +1698,7 @@ static int acpi_nfit_add(struct acpi_device *adev)
nd_desc = &acpi_desc->nd_desc;
nd_desc->provider_name = "ACPI.NFIT";
nd_desc->ndctl = acpi_nfit_ctl;
+ nd_desc->ndctl_passthru = acpi_nfit_ctl_passthru;
nd_desc->attr_groups = acpi_nfit_attribute_groups;
acpi_desc->nvdimm_bus = nvdimm_bus_register(dev, nd_desc);
diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
index 7e2c43f..cfb10eb 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -599,18 +599,103 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
return rc;
}
+
+static int __nd_ioctl_passthru(struct nvdimm_bus *nvdimm_bus,
+ struct nvdimm *nvdimm, int read_only, unsigned
+ int ioctl_cmd, unsigned long arg)
+{
+ struct nvdimm_bus_descriptor *nd_desc = nvdimm_bus->nd_desc;
+ size_t buf_len = 0, in_len = 0, out_len = 0;
+ unsigned int cmd = _IOC_NR(ioctl_cmd);
+ unsigned int size = _IOC_SIZE(ioctl_cmd);
+ void __user *p = (void __user *) arg;
+ struct device *dev = &nvdimm_bus->dev;
+ const char *dimm_name = "";
+ void *buf = NULL;
+ int i, rc;
+ struct ndn_pkg pkg;
+
+ if (nvdimm)
+ dimm_name = dev_name(&nvdimm->dev);
+ else
+ dimm_name = "bus";
+
+ if (copy_from_user(&pkg, p, sizeof(pkg))) {
+ rc = -EFAULT;
+ goto out;
+ }
+
+ for (i = 0; i < ARRAY_SIZE(pkg.h.res); i++)
+ if (pkg.h.res[i])
+ return -EINVAL;
+
+ /* Caller must tell us size of input to _DSM. */
+ /* This may be bigger that the fixed portion of the pakcage */
+ in_len = pkg.h.dsm_in;
+ out_len = pkg.h.dsm_out;
+ buf_len = sizeof(pkg.h) + in_len + out_len;
+
+
+ dev_dbg(dev, "%s:%s cmd: %d, size: %d, in: %zu, out: %zu len: %zu\n",
+ __func__,
+ dimm_name, cmd, size,
+ in_len, out_len, buf_len);
+
+ if (buf_len > ND_IOCTL_MAX_BUFLEN) {
+ dev_dbg(dev, "%s:%s cmd: %d buf_len: %zu > %d\n", __func__,
+ dimm_name, cmd, buf_len,
+ ND_IOCTL_MAX_BUFLEN);
+ return -EINVAL;
+ }
+
+ buf = vmalloc(buf_len);
+ if (!buf)
+ return -ENOMEM;
+
+ if (copy_from_user(buf, p, buf_len)) {
+ rc = -EFAULT;
+ goto out;
+ }
+
+ nvdimm_bus_lock(&nvdimm_bus->dev);
+ rc = nd_cmd_clear_to_send(nvdimm, cmd);
+ if (rc)
+ goto out_unlock;
+
+ rc = nd_desc->ndctl_passthru(nd_desc, nvdimm, cmd, buf, buf_len);
+ if (rc < 0)
+ goto out_unlock;
+ if (copy_to_user(p, buf, buf_len))
+ rc = -EFAULT;
+ out_unlock:
+ nvdimm_bus_unlock(&nvdimm_bus->dev);
+ out:
+ vfree(buf);
+ return rc;
+}
+
static long nd_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
{
long id = (long) file->private_data;
int rc = -ENXIO, read_only;
struct nvdimm_bus *nvdimm_bus;
+ unsigned int type = _IOC_TYPE(cmd);
read_only = (O_RDWR != (file->f_flags & O_ACCMODE));
mutex_lock(&nvdimm_bus_list_mutex);
list_for_each_entry(nvdimm_bus, &nvdimm_bus_list, list) {
- if (nvdimm_bus->id == id) {
+ if (nvdimm_bus->id != id)
+ continue;
+
+ switch (type) {
+ case NVDIMM_TYPE_INTEL:
rc = __nd_ioctl(nvdimm_bus, NULL, read_only, cmd, arg);
break;
+ case NVDIMM_TYPE_PASSTHRU:
+ rc = __nd_ioctl_passthru(nvdimm_bus, NULL, 0, cmd, arg);
+ break;
+ default:
+ rc = -ENOTTY;
}
}
mutex_unlock(&nvdimm_bus_list_mutex);
@@ -633,10 +718,11 @@ static int match_dimm(struct device *dev, void *data)
static long nvdimm_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
{
- int rc = -ENXIO, read_only;
+ int rc = -ENXIO, ro;
struct nvdimm_bus *nvdimm_bus;
+ unsigned int type = _IOC_TYPE(cmd);
- read_only = (O_RDWR != (file->f_flags & O_ACCMODE));
+ ro = (O_RDWR != (file->f_flags & O_ACCMODE));
mutex_lock(&nvdimm_bus_list_mutex);
list_for_each_entry(nvdimm_bus, &nvdimm_bus_list, list) {
struct device *dev = device_find_child(&nvdimm_bus->dev,
@@ -647,7 +733,18 @@ static long nvdimm_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
continue;
nvdimm = to_nvdimm(dev);
- rc = __nd_ioctl(nvdimm_bus, nvdimm, read_only, cmd, arg);
+
+ switch (type) {
+ case NVDIMM_TYPE_INTEL:
+ rc = __nd_ioctl(nvdimm_bus, nvdimm, ro, cmd, arg);
+ break;
+ case NVDIMM_TYPE_PASSTHRU:
+ rc = __nd_ioctl_passthru(nvdimm_bus, nvdimm, ro, cmd, arg);
+ break;
+ default:
+ rc = -ENOTTY;
+ }
+
put_device(dev);
break;
}
--
1.7.11.3
rename functions acpi_nfit_ctl, __nd_ioctl, ndctl, to *_intel to denote
that the functions implement the Intel example _DSM.
Signed-off-by: Jerry Hoemann <[email protected]>
---
drivers/acpi/nfit.c | 6 +++---
drivers/nvdimm/bus.c | 15 ++++++++-------
drivers/nvdimm/dimm_devs.c | 6 +++---
include/linux/libnvdimm.h | 2 +-
4 files changed, 15 insertions(+), 14 deletions(-)
diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
index a6b458a..f85200d 100644
--- a/drivers/acpi/nfit.c
+++ b/drivers/acpi/nfit.c
@@ -62,7 +62,7 @@ static struct acpi_device *to_acpi_dev(struct acpi_nfit_desc *acpi_desc)
return to_acpi_device(acpi_desc->dev);
}
-static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc,
+static int acpi_nfit_ctl_intel(struct nvdimm_bus_descriptor *nd_desc,
struct nvdimm *nvdimm, unsigned int cmd, void *buf,
unsigned int buf_len)
{
@@ -1352,7 +1352,7 @@ static int acpi_nfit_blk_get_flags(struct nvdimm_bus_descriptor *nd_desc,
int rc;
memset(&flags, 0, sizeof(flags));
- rc = nd_desc->ndctl(nd_desc, nvdimm, ND_CMD_DIMM_FLAGS, &flags,
+ rc = nd_desc->ndctl_intel(nd_desc, nvdimm, ND_CMD_DIMM_FLAGS, &flags,
sizeof(flags));
if (rc >= 0 && flags.status == 0)
@@ -1697,7 +1697,7 @@ static int acpi_nfit_add(struct acpi_device *adev)
acpi_desc->blk_do_io = acpi_nfit_blk_region_do_io;
nd_desc = &acpi_desc->nd_desc;
nd_desc->provider_name = "ACPI.NFIT";
- nd_desc->ndctl = acpi_nfit_ctl;
+ nd_desc->ndctl_intel = acpi_nfit_ctl_intel;
nd_desc->ndctl_passthru = acpi_nfit_ctl_passthru;
nd_desc->attr_groups = acpi_nfit_attribute_groups;
diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
index cfb10eb..4587d30 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -479,8 +479,9 @@ static int nd_cmd_clear_to_send(struct nvdimm *nvdimm, unsigned int cmd)
return 0;
}
-static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
- int read_only, unsigned int ioctl_cmd, unsigned long arg)
+static int __nd_ioctl_intel(struct nvdimm_bus *nvdimm_bus,
+ struct nvdimm *nvdimm, int read_only, unsigned int ioctl_cmd,
+ unsigned long arg)
{
struct nvdimm_bus_descriptor *nd_desc = nvdimm_bus->nd_desc;
size_t buf_len = 0, in_len = 0, out_len = 0;
@@ -587,7 +588,7 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
if (rc)
goto out_unlock;
- rc = nd_desc->ndctl(nd_desc, nvdimm, cmd, buf, buf_len);
+ rc = nd_desc->ndctl_intel(nd_desc, nvdimm, cmd, buf, buf_len);
if (rc < 0)
goto out_unlock;
if (copy_to_user(p, buf, buf_len))
@@ -677,11 +678,11 @@ static int __nd_ioctl_passthru(struct nvdimm_bus *nvdimm_bus,
static long nd_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
{
long id = (long) file->private_data;
- int rc = -ENXIO, read_only;
+ int rc = -ENXIO, ro;
struct nvdimm_bus *nvdimm_bus;
unsigned int type = _IOC_TYPE(cmd);
- read_only = (O_RDWR != (file->f_flags & O_ACCMODE));
+ ro = (O_RDWR != (file->f_flags & O_ACCMODE));
mutex_lock(&nvdimm_bus_list_mutex);
list_for_each_entry(nvdimm_bus, &nvdimm_bus_list, list) {
if (nvdimm_bus->id != id)
@@ -689,7 +690,7 @@ static long nd_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
switch (type) {
case NVDIMM_TYPE_INTEL:
- rc = __nd_ioctl(nvdimm_bus, NULL, read_only, cmd, arg);
+ rc = __nd_ioctl_intel(nvdimm_bus, NULL, ro, cmd, arg);
break;
case NVDIMM_TYPE_PASSTHRU:
rc = __nd_ioctl_passthru(nvdimm_bus, NULL, 0, cmd, arg);
@@ -736,7 +737,7 @@ static long nvdimm_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
switch (type) {
case NVDIMM_TYPE_INTEL:
- rc = __nd_ioctl(nvdimm_bus, nvdimm, ro, cmd, arg);
+ rc = __nd_ioctl_intel(nvdimm_bus, nvdimm, ro, cmd, arg);
break;
case NVDIMM_TYPE_PASSTHRU:
rc = __nd_ioctl_passthru(nvdimm_bus, nvdimm, ro, cmd, arg);
diff --git a/drivers/nvdimm/dimm_devs.c b/drivers/nvdimm/dimm_devs.c
index 651b8d1..9c0cc23 100644
--- a/drivers/nvdimm/dimm_devs.c
+++ b/drivers/nvdimm/dimm_devs.c
@@ -74,7 +74,7 @@ int nvdimm_init_nsarea(struct nvdimm_drvdata *ndd)
memset(cmd, 0, sizeof(*cmd));
nd_desc = nvdimm_bus->nd_desc;
- return nd_desc->ndctl(nd_desc, to_nvdimm(ndd->dev),
+ return nd_desc->ndctl_intel(nd_desc, to_nvdimm(ndd->dev),
ND_CMD_GET_CONFIG_SIZE, cmd, sizeof(*cmd));
}
@@ -118,7 +118,7 @@ int nvdimm_init_config_data(struct nvdimm_drvdata *ndd)
offset += cmd->in_length) {
cmd->in_length = min(config_size, max_cmd_size);
cmd->in_offset = offset;
- rc = nd_desc->ndctl(nd_desc, to_nvdimm(ndd->dev),
+ rc = nd_desc->ndctl_intel(nd_desc, to_nvdimm(ndd->dev),
ND_CMD_GET_CONFIG_DATA, cmd,
cmd->in_length + sizeof(*cmd));
if (rc || cmd->status) {
@@ -170,7 +170,7 @@ int nvdimm_set_config_data(struct nvdimm_drvdata *ndd, size_t offset,
cmd_size = sizeof(*cmd) + cmd->in_length + sizeof(u32);
status = ((void *) cmd) + cmd_size - sizeof(u32);
- rc = nd_desc->ndctl(nd_desc, to_nvdimm(ndd->dev),
+ rc = nd_desc->ndctl_intel(nd_desc, to_nvdimm(ndd->dev),
ND_CMD_SET_CONFIG_DATA, cmd, cmd_size);
if (rc || *status) {
rc = rc ? rc : -ENXIO;
diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
index 01117e1..594a772 100644
--- a/include/linux/libnvdimm.h
+++ b/include/linux/libnvdimm.h
@@ -71,7 +71,7 @@ struct nvdimm_bus_descriptor {
const struct attribute_group **attr_groups;
unsigned long dsm_mask;
char *provider_name;
- ndctl_fn ndctl;
+ ndctl_fn ndctl_intel;
ndctl_fn ndctl_passthru;
};
--
1.7.11.3
> + if (IS_ENABLED(CONFIG_ACPI_NFIT_DEBUG)) {
> + dev_dbg(dev, "%s:%s cmd: %d input length: %d\n", __func__,
> + dimm_name, cmd, in_buf.buffer.length);
> + print_hex_dump_debug("cmd: ", DUMP_PREFIX_OFFSET, 4,
> + 4, in_buf.buffer.pointer, min_t(u32, 128,
> + in_buf.buffer.length), true);
> + }
Maybe move this code to a helper function? There are 4 almost
identical blocks now in acpi_nfit_ctl_passthru and
acpi_nfit_ctl_intel.
> + for (i = 0; i < ARRAY_SIZE(pkg.h.res); i++)
> + if (pkg.h.res[i])
> + return -EINVAL;
I'd rename "res" to "reserved" for clarity.
> + /* This may be bigger that the fixed portion of the pakcage */
s/that/than/
s/pakcage/package/
> + switch (type) {
> + case NVDIMM_TYPE_INTEL:
> + rc = __nd_ioctl(nvdimm_bus, nvdimm, ro, cmd, arg);
> + break;
> + case NVDIMM_TYPE_PASSTHRU:
> + rc = __nd_ioctl_passthru(nvdimm_bus, nvdimm, ro, cmd, arg);
> + break;
> + default:
> + rc = -ENOTTY;
> + }
The same comment. Identical code in nd_ioctl and nvdimm_ioctl.
Perhaps move to a helper function?
Thanks,
Dmitry
Dmitry,
thanks for you review. Questions in-line.
On Sat, Nov 07, 2015 at 05:02:36PM +0300, Dmitry Krivenok wrote:
> > + if (IS_ENABLED(CONFIG_ACPI_NFIT_DEBUG)) {
> > + dev_dbg(dev, "%s:%s cmd: %d input length: %d\n", __func__,
> > + dimm_name, cmd, in_buf.buffer.length);
> > + print_hex_dump_debug("cmd: ", DUMP_PREFIX_OFFSET, 4,
> > + 4, in_buf.buffer.pointer, min_t(u32, 128,
> > + in_buf.buffer.length), true);
> > + }
>
> Maybe move this code to a helper function? There are 4 almost
> identical blocks now in acpi_nfit_ctl_passthru and
> acpi_nfit_ctl_intel.
Is your concern readibility or size of generated code (or both?)
I'll look to consolidating the debug printing in next version as additional patch.
>
> > + for (i = 0; i < ARRAY_SIZE(pkg.h.res); i++)
> > + if (pkg.h.res[i])
> > + return -EINVAL;
>
> I'd rename "res" to "reserved" for clarity.
Will do.
>
> > + /* This may be bigger that the fixed portion of the pakcage */
>
> s/that/than/
> s/pakcage/package/
Will do.
>
> > + switch (type) {
> > + case NVDIMM_TYPE_INTEL:
> > + rc = __nd_ioctl(nvdimm_bus, nvdimm, ro, cmd, arg);
> > + break;
> > + case NVDIMM_TYPE_PASSTHRU:
> > + rc = __nd_ioctl_passthru(nvdimm_bus, nvdimm, ro, cmd, arg);
> > + break;
> > + default:
> > + rc = -ENOTTY;
> > + }
>
> The same comment. Identical code in nd_ioctl and nvdimm_ioctl.
> Perhaps move to a helper function?
If we had a longer list, I would definitely say yes. Not so sure with
just two types. I'll take a look for the next version.
--
-----------------------------------------------------------------------------
Jerry Hoemann Software Engineer Hewlett-Packard Enterprise
3404 E Harmony Rd. MS 36 phone: (970) 898-1022
Ft. Collins, CO 80528 FAX: (970) 898-0707
email: [email protected]
-----------------------------------------------------------------------------
> Is your concern readibility or size of generated code (or both?)
>
> I'll look to consolidating the debug printing in next version as additional patch.
Just a minor style comment, not critical.
> If we had a longer list, I would definitely say yes. Not so sure with
> just two types. I'll take a look for the next version.
The same, just a style comment.
> list_for_each_entry(nvdimm_bus, &nvdimm_bus_list, list) {
> - if (nvdimm_bus->id == id) {
> + if (nvdimm_bus->id != id)
I noticed another minor issue. You have switched from "==" to "!="
here, but you didn't add "break" after ioctl is handled for the found
bus.
Thanks,
Dmitry
Jerry Hoemann <[email protected]> writes:
> The NVDIMM code in the kernel supports an IOCTL interface to user
> space based upon the Intel Example DSM:
>
> http://pmem.io/documents/NVDIMM_DSM_Interface_Example.pdf
>
> This interface cannot be used by other NVDIMM DSMs that support
> incompatible functions.
>
> This patch set adds a generic "passthru" IOCTL interface which
> is not tied to a particular DSM.
>
> A new IOCTL type "P" is added for the pass thru call.
>
> The new data structure ndn_pkg serves as a wrapper for the passthru
> calls. This wrapper supplies the data that the kernel needs to
> make the _DSM call.
What does 'ndn' stand for? If it stands for NVDIMM-N, then I think
that's too narrow a scope. Anyway, it helps readability if you call out
what abbreviations mean, especially when it's non-obvious.
> Jerry Hoemann (4):
> nvdimm: Add wrapper for IOCTL pass thru.
> nvdimm: Add IOCTL pass thru
> nvdimm: Add IOCTL pass thru
You should really give each patch a different subject.
Cheers,
Jeff
Jerry Hoemann <[email protected]> writes:
> @@ -633,10 +718,11 @@ static int match_dimm(struct device *dev, void *data)
>
> static long nvdimm_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> {
> - int rc = -ENXIO, read_only;
> + int rc = -ENXIO, ro;
> struct nvdimm_bus *nvdimm_bus;
> + unsigned int type = _IOC_TYPE(cmd);
>
> - read_only = (O_RDWR != (file->f_flags & O_ACCMODE));
> + ro = (O_RDWR != (file->f_flags & O_ACCMODE));
I'm still reviewing the rest of this, but this is bugging me. The
existing check for read_only looks pretty fishy to me. O_WRONLY is a
thing (even though it's probably not a supportable mode for this ioctl).
Why not just check for O_RDONLY?
Cheers,
Jeff
Jerry Hoemann <[email protected]> writes:
> Add IOCTL type 'P' to denote NVDIMM_TYPE_PASSTHRU.
Can't you just make passthrough a separate command? If you actually add
the ioctl definition for passthrough (which you didn't do for some
reason?), it looks odd:
#define ND_IOCTL_PASSTHRU _IOWR(NVDIMM_TYPE_PASSTHRU,, ND_CMD_PASSTHRU, \
struct ndn_package)
Care to comment on why you chose a different type instead of specifying
a new command?
> +struct ndn_pkg {
> + struct {
> + __u8 dsm_uuid[16];
> + __u32 dsm_in; /* size of _DSM input */
> + __u32 dsm_out; /* size of user buffer */
> + __u32 dsm_rev; /* revision of dsm call */
> + __u32 res[8]; /* reserved must be zero */
> + __u32 dsm_size; /* size _DSM would write */
> + } h;
> + unsigned char buf[];
Please change that to:
__u8 *buf;
since acpi_object.buffer.pointer is a u8 *.
Note that the size of this structure will be different for 32 vs. 64
bit, but I don't think it matters since offsets won't change (the
pointer is at the end of the structure).
On Tue, Nov 10, 2015 at 9:51 AM, Jeff Moyer <[email protected]> wrote:
> Jerry Hoemann <[email protected]> writes:
>
>> Add IOCTL type 'P' to denote NVDIMM_TYPE_PASSTHRU.
>
> Can't you just make passthrough a separate command? If you actually add
> the ioctl definition for passthrough (which you didn't do for some
> reason?), it looks odd:
>
> #define ND_IOCTL_PASSTHRU _IOWR(NVDIMM_TYPE_PASSTHRU,, ND_CMD_PASSTHRU, \
> struct ndn_package)
>
> Care to comment on why you chose a different type instead of specifying
> a new command?
+1 for making this just a new command number without a new top-level
number space.
Jerry Hoemann <[email protected]> writes:
> Add internal data structure for ndctl_passthru call.
>
> Signed-off-by: Jerry Hoemann <[email protected]>
> ---
> include/linux/libnvdimm.h | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
> index 3f021dc..01117e1 100644
> --- a/include/linux/libnvdimm.h
> +++ b/include/linux/libnvdimm.h
> @@ -72,6 +72,7 @@ struct nvdimm_bus_descriptor {
> unsigned long dsm_mask;
> char *provider_name;
> ndctl_fn ndctl;
> + ndctl_fn ndctl_passthru;
I don't think this is necessary. Vector off inside of __nd_ioctl. That
especially makes sense if you do switch to passthrough as a command
instead of a type, but it can work either way.
-Jeff
> -----Original Message-----
> From: Linux-nvdimm [mailto:[email protected]] On Behalf Of
> Hoemann, Jerry
> Sent: Friday, November 6, 2015 4:27 PM
> Subject: [PATCH 1/4] nvdimm: Add wrapper for IOCTL pass thru.
...
> diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h
...
> +struct ndn_pkg {
> + struct {
> + __u8 dsm_uuid[16];
> + __u32 dsm_in; /* size of _DSM input */
> + __u32 dsm_out; /* size of user buffer */
> + __u32 dsm_rev; /* revision of dsm call */
> + __u32 res[8]; /* reserved must be zero */
> + __u32 dsm_size; /* size _DSM would write */
> + } h;
> + unsigned char buf[];
> +} __packed;
Given that the _DSM arguments are defined as:
* Arg0 UUID: Buffer of 16 bytes
* Arg1 Revision ID: Integer (8 bytes)
* Arg2 Function Index: Integer (8 bytes)
* Arg3 Package: function-specific
1. The __u32 for dsm_rev is not big enough to express all
possible 8 byte Revision IDs.
2. The unsigned int cmd (carried outside this structure)
is not big enough on all platforms (e.g., 32-bit) to
express all possible Function Indexes.
3. The Revision ID and Function Index values passed to
the _DSM are defined as little-endian. Are they
intended to use native endianness or be little-endian
in this structure?
---
Robert Elliott, HPE Persistent Memory
On Tue, Nov 10, 2015 at 12:51:59PM -0500, Jeff Moyer wrote:
> Jerry Hoemann <[email protected]> writes:
>
> > Add IOCTL type 'P' to denote NVDIMM_TYPE_PASSTHRU.
>
> Can't you just make passthrough a separate command? If you actually add
There are multiple conflicting NVDIMM _DSM running around, they
are "device specific". So, we should plan in general and not just
for the example DSM that Intel added support for. These DSM have
over lapping and incompatible function ids.
The Intel example is an example, not standard. They are free to
change it at will. So, we can't be certain there won't be a
conflict some time in the future if we try to use their number space.
I'm trying to create a generic pass thru that any vendors can use. Putting
this in the Intel function number space doesn't make a lot of sense to me.
> the ioctl definition for passthrough (which you didn't do for some
> reason?), it looks odd:
The definition for the IOCTLs are in a user space application.
These aren't required in the kernel as the kernel is only a
pass thru.
As the DSM I'm working with isn't yet finalized, I've been told that
i can't share the user space portion yet.
>
> #define ND_IOCTL_PASSTHRU _IOWR(NVDIMM_TYPE_PASSTHRU,, ND_CMD_PASSTHRU, \
> struct ndn_package)
>
> Care to comment on why you chose a different type instead of specifying
> a new command?
>
> > +struct ndn_pkg {
> > + struct {
> > + __u8 dsm_uuid[16];
> > + __u32 dsm_in; /* size of _DSM input */
> > + __u32 dsm_out; /* size of user buffer */
> > + __u32 dsm_rev; /* revision of dsm call */
> > + __u32 res[8]; /* reserved must be zero */
> > + __u32 dsm_size; /* size _DSM would write */
> > + } h;
> > + unsigned char buf[];
>
> Please change that to:
> __u8 *buf;
> since acpi_object.buffer.pointer is a u8 *.
buf isn't being passed to acpi_evaluate_dsm. its just being used for pointer offset
in acpi_nfit_ctl_passthru. The "payload" that will be passed to acpi_evaluate_dsm
follows.
>
> Note that the size of this structure will be different for 32 vs. 64
> bit, but I don't think it matters since offsets won't change (the
> pointer is at the end of the structure).
I assume you mean size of struct changes if I use the pointer as
substitute for the zero sized array? or are you saying that the
packed attribute doesn't affect the layout of the anonymous struct?
--
-----------------------------------------------------------------------------
Jerry Hoemann Software Engineer Hewlett-Packard Enterprise
3404 E Harmony Rd. MS 36 phone: (970) 898-1022
Ft. Collins, CO 80528 FAX: (970) 898-0707
email: [email protected]
-----------------------------------------------------------------------------
Jerry Hoemann <[email protected]> writes:
> On Tue, Nov 10, 2015 at 12:51:59PM -0500, Jeff Moyer wrote:
>> Jerry Hoemann <[email protected]> writes:
>>
>> > Add IOCTL type 'P' to denote NVDIMM_TYPE_PASSTHRU.
>>
>> Can't you just make passthrough a separate command? If you actually add
>
> There are multiple conflicting NVDIMM _DSM running around, they
> are "device specific". So, we should plan in general and not just
> for the example DSM that Intel added support for. These DSM have
> over lapping and incompatible function ids.
>
> The Intel example is an example, not standard. They are free to
> change it at will. So, we can't be certain there won't be a
> conflict some time in the future if we try to use their number space.
>
> I'm trying to create a generic pass thru that any vendors can use. Putting
> this in the Intel function number space doesn't make a lot of sense to me.
OK, I see your point.
>> the ioctl definition for passthrough (which you didn't do for some
>> reason?), it looks odd:
>
> The definition for the IOCTLs are in a user space application.
> These aren't required in the kernel as the kernel is only a
> pass thru.
OK, I don't see the harm in including it in the kernel headers, but I'm
not going to insist on it.
> As the DSM I'm working with isn't yet finalized, I've been told that
> i can't share the user space portion yet.
That's OK, I don't think providing the userspace code is necessary for
this patch set to make progress. (I didn't actually ask for it, to be
clear.)
>> #define ND_IOCTL_PASSTHRU _IOWR(NVDIMM_TYPE_PASSTHRU,, ND_CMD_PASSTHRU, \
>> struct ndn_package)
>>
>> Care to comment on why you chose a different type instead of specifying
>> a new command?
>>
>> > +struct ndn_pkg {
>> > + struct {
>> > + __u8 dsm_uuid[16];
>> > + __u32 dsm_in; /* size of _DSM input */
>> > + __u32 dsm_out; /* size of user buffer */
>> > + __u32 dsm_rev; /* revision of dsm call */
>> > + __u32 res[8]; /* reserved must be zero */
>> > + __u32 dsm_size; /* size _DSM would write */
>> > + } h;
>> > + unsigned char buf[];
>>
>> Please change that to:
>> __u8 *buf;
>> since acpi_object.buffer.pointer is a u8 *.
>
> buf isn't being passed to acpi_evaluate_dsm. its just being used for pointer offset
> in acpi_nfit_ctl_passthru. The "payload" that will be passed to acpi_evaluate_dsm
> follows.
+ in_buf.buffer.pointer = (void *) &pkg->buf;
I see. I misread that, because you didn't actually make buf a zero
length array (see the structure definition quoted above). I guess you
meant to write this:
unsigned char buf[0];
Cheers,
Jeff
On Tue, Nov 10, 2015 at 11:49 AM, Jerry Hoemann <[email protected]> wrote:
> On Tue, Nov 10, 2015 at 12:51:59PM -0500, Jeff Moyer wrote:
>> Jerry Hoemann <[email protected]> writes:
>>
>> > Add IOCTL type 'P' to denote NVDIMM_TYPE_PASSTHRU.
>>
>> Can't you just make passthrough a separate command? If you actually add
>
> There are multiple conflicting NVDIMM _DSM running around, they
> are "device specific". So, we should plan in general and not just
> for the example DSM that Intel added support for. These DSM have
> over lapping and incompatible function ids.
>
> The Intel example is an example, not standard. They are free to
> change it at will. So, we can't be certain there won't be a
> conflict some time in the future if we try to use their number space.
>
> I'm trying to create a generic pass thru that any vendors can use. Putting
> this in the Intel function number space doesn't make a lot of sense to me.
It isn't the "Intel" function number space. The fact that they
currently align is just a happy accident. The kernel is free to break
the 1:1 ioctl number to DSM function number relationship, and I think
it would make the implementation cleaner in this case.
On 11/10/2015 3:27 PM, Dan Williams wrote:
> On Tue, Nov 10, 2015 at 11:49 AM, Jerry Hoemann <[email protected]> wrote:
>> On Tue, Nov 10, 2015 at 12:51:59PM -0500, Jeff Moyer wrote:
>>> Jerry Hoemann <[email protected]> writes:
>>>
>>>> Add IOCTL type 'P' to denote NVDIMM_TYPE_PASSTHRU.
>>>
>>> Can't you just make passthrough a separate command? If you actually add
>>
>> There are multiple conflicting NVDIMM _DSM running around, they
>> are "device specific". So, we should plan in general and not just
>> for the example DSM that Intel added support for. These DSM have
>> over lapping and incompatible function ids.
>>
>> The Intel example is an example, not standard. They are free to
>> change it at will. So, we can't be certain there won't be a
>> conflict some time in the future if we try to use their number space.
>>
>> I'm trying to create a generic pass thru that any vendors can use. Putting
>> this in the Intel function number space doesn't make a lot of sense to me.
>
> It isn't the "Intel" function number space. The fact that they
> currently align is just a happy accident.
It's not really a happy accident. Your commit message says it
was derived from the Intel spec 'for convenience', which I think is convenient
for anything that implements that spec.
We've discussed ways of supporting different command sets with you
and determined that this pass-through mechanism was a good approach
because it allows multiple different command sets to be support in
a generic way. Blending the two flavors (generic pass through and explicit
function definitions) is confusing to me.
> The kernel is free to break
> the 1:1 ioctl number to DSM function number relationship, and I think
> it would make the implementation cleaner in this case.
To me it's less clean and even for your own example spec, less
convenient if Intel ever updates that spec.
-- ljk
> _______________________________________________
> Linux-nvdimm mailing list
> [email protected]
> https://lists.01.org/mailman/listinfo/linux-nvdimm
>
On Tue, Nov 10, 2015 at 12:04:22PM -0700, Elliott, Robert (Persistent Memory) wrote:
> > -----Original Message-----
> > From: Linux-nvdimm [mailto:[email protected]] On Behalf Of
> > Hoemann, Jerry
> > Sent: Friday, November 6, 2015 4:27 PM
> > Subject: [PATCH 1/4] nvdimm: Add wrapper for IOCTL pass thru.
> ...
> > diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h
> ...
> > +struct ndn_pkg {
> > + struct {
> > + __u8 dsm_uuid[16];
> > + __u32 dsm_in; /* size of _DSM input */
> > + __u32 dsm_out; /* size of user buffer */
> > + __u32 dsm_rev; /* revision of dsm call */
> > + __u32 res[8]; /* reserved must be zero */
> > + __u32 dsm_size; /* size _DSM would write */
> > + } h;
> > + unsigned char buf[];
> > +} __packed;
>
> Given that the _DSM arguments are defined as:
> * Arg0 UUID: Buffer of 16 bytes
> * Arg1 Revision ID: Integer (8 bytes)
> * Arg2 Function Index: Integer (8 bytes)
> * Arg3 Package: function-specific
>
> 1. The __u32 for dsm_rev is not big enough to express all
> possible 8 byte Revision IDs.
>
> 2. The unsigned int cmd (carried outside this structure)
> is not big enough on all platforms (e.g., 32-bit) to
> express all possible Function Indexes.
>
> 3. The Revision ID and Function Index values passed to
> the _DSM are defined as little-endian. Are they
> intended to use native endianness or be little-endian
> in this structure?
>
Thanks, Robert. I will look at these for version 2.
Jerry
--
-----------------------------------------------------------------------------
Jerry Hoemann Software Engineer Hewlett-Packard Enterprise
3404 E Harmony Rd. MS 36 phone: (970) 898-1022
Ft. Collins, CO 80528 FAX: (970) 898-0707
email: [email protected]
-----------------------------------------------------------------------------
On Tue, Nov 10, 2015 at 11:24:47AM -0500, Jeff Moyer wrote:
> Jerry Hoemann <[email protected]> writes:
>
> > @@ -633,10 +718,11 @@ static int match_dimm(struct device *dev, void *data)
> >
> > static long nvdimm_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> > {
> > - int rc = -ENXIO, read_only;
> > + int rc = -ENXIO, ro;
> > struct nvdimm_bus *nvdimm_bus;
> > + unsigned int type = _IOC_TYPE(cmd);
> >
> > - read_only = (O_RDWR != (file->f_flags & O_ACCMODE));
> > + ro = (O_RDWR != (file->f_flags & O_ACCMODE));
>
> I'm still reviewing the rest of this, but this is bugging me. The
> existing check for read_only looks pretty fishy to me. O_WRONLY is a
> thing (even though it's probably not a supportable mode for this ioctl).
> Why not just check for O_RDONLY?
Good question. I'll look into changing for version 2.
I suspect you would like something more like:
ro = ((file->f_flags & O_ACCMODE) == O_RDONLY);
>
> Cheers,
> Jeff
--
-----------------------------------------------------------------------------
Jerry Hoemann Software Engineer Hewlett-Packard Enterprise
3404 E Harmony Rd. MS 36 phone: (970) 898-1022
Ft. Collins, CO 80528 FAX: (970) 898-0707
email: [email protected]
-----------------------------------------------------------------------------
On Tue, Nov 10, 2015 at 10:33:29AM -0500, Jeff Moyer wrote:
> Jerry Hoemann <[email protected]> writes:
>
> > The NVDIMM code in the kernel supports an IOCTL interface to user
> > space based upon the Intel Example DSM:
> >
> > http://pmem.io/documents/NVDIMM_DSM_Interface_Example.pdf
> >
> > This interface cannot be used by other NVDIMM DSMs that support
> > incompatible functions.
> >
> > This patch set adds a generic "passthru" IOCTL interface which
> > is not tied to a particular DSM.
> >
> > A new IOCTL type "P" is added for the pass thru call.
> >
> > The new data structure ndn_pkg serves as a wrapper for the passthru
> > calls. This wrapper supplies the data that the kernel needs to
> > make the _DSM call.
>
> What does 'ndn' stand for? If it stands for NVDIMM-N, then I think
Yes, hold over from earlier, less generic version.
> that's too narrow a scope. Anyway, it helps readability if you call out
> what abbreviations mean, especially when it's non-obvious.
Will fix in version 2.
>
> > Jerry Hoemann (4):
> > nvdimm: Add wrapper for IOCTL pass thru.
> > nvdimm: Add IOCTL pass thru
> > nvdimm: Add IOCTL pass thru
>
> You should really give each patch a different subject.
Will do.
>
> Cheers,
> Jeff
--
-----------------------------------------------------------------------------
Jerry Hoemann Software Engineer Hewlett-Packard Enterprise
3404 E Harmony Rd. MS 36 phone: (970) 898-1022
Ft. Collins, CO 80528 FAX: (970) 898-0707
email: [email protected]
-----------------------------------------------------------------------------
Jerry Hoemann <[email protected]> writes:
> On Tue, Nov 10, 2015 at 11:24:47AM -0500, Jeff Moyer wrote:
>> Jerry Hoemann <[email protected]> writes:
>>
>> > @@ -633,10 +718,11 @@ static int match_dimm(struct device *dev, void *data)
>> >
>> > static long nvdimm_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>> > {
>> > - int rc = -ENXIO, read_only;
>> > + int rc = -ENXIO, ro;
>> > struct nvdimm_bus *nvdimm_bus;
>> > + unsigned int type = _IOC_TYPE(cmd);
>> >
>> > - read_only = (O_RDWR != (file->f_flags & O_ACCMODE));
>> > + ro = (O_RDWR != (file->f_flags & O_ACCMODE));
>>
>> I'm still reviewing the rest of this, but this is bugging me. The
>> existing check for read_only looks pretty fishy to me. O_WRONLY is a
>> thing (even though it's probably not a supportable mode for this ioctl).
>> Why not just check for O_RDONLY?
>
>
> Good question. I'll look into changing for version 2.
> I suspect you would like something more like:
>
> ro = ((file->f_flags & O_ACCMODE) == O_RDONLY);
Yeah. I'd make that a separate patch, and put it first in the series
since it's a cleanup than can be applied to older kernels if necessary.
Thanks,
Jeff
Jerry Hoemann <[email protected]> writes:
> + uuid = pkg->h.dsm_uuid;
> + rev = pkg->h.dsm_rev ? pkg->h.dsm_rev : 1;
Shouldn't revision id be required?
Cheers,
Jeff
On Tue, Nov 10, 2015 at 01:05:20PM -0500, Jeff Moyer wrote:
> Jerry Hoemann <[email protected]> writes:
>
> > Add internal data structure for ndctl_passthru call.
> >
> > Signed-off-by: Jerry Hoemann <[email protected]>
> > ---
> > include/linux/libnvdimm.h | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
> > index 3f021dc..01117e1 100644
> > --- a/include/linux/libnvdimm.h
> > +++ b/include/linux/libnvdimm.h
> > @@ -72,6 +72,7 @@ struct nvdimm_bus_descriptor {
> > unsigned long dsm_mask;
> > char *provider_name;
> > ndctl_fn ndctl;
> > + ndctl_fn ndctl_passthru;
>
> I don't think this is necessary. Vector off inside of __nd_ioctl. That
> especially makes sense if you do switch to passthrough as a command
> instead of a type, but it can work either way.
>
In an earlier version, I added a "type" argument to ndctl_fn and switched
internally based upon that. I just came to the conclusion that I'd rather
have two separate acpi_nfit_ctl functions than one trying to do both sets
of argument marshaling. This is quite different both internally and
to the caller.
So, I thought it would be less confusing to the next engineer, and that
this was a good logical separation point.
--
-----------------------------------------------------------------------------
Jerry Hoemann Software Engineer Hewlett-Packard Enterprise
3404 E Harmony Rd. MS 36 phone: (970) 898-1022
Ft. Collins, CO 80528 FAX: (970) 898-0707
email: [email protected]
-----------------------------------------------------------------------------
On Tue, Nov 10, 2015 at 04:45:16PM -0500, Jeff Moyer wrote:
> Jerry Hoemann <[email protected]> writes:
>
> > On Tue, Nov 10, 2015 at 11:24:47AM -0500, Jeff Moyer wrote:
> >> Jerry Hoemann <[email protected]> writes:
> >>
> >> > @@ -633,10 +718,11 @@ static int match_dimm(struct device *dev, void *data)
> >> >
> >> > static long nvdimm_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> >> > {
> >> > - int rc = -ENXIO, read_only;
> >> > + int rc = -ENXIO, ro;
> >> > struct nvdimm_bus *nvdimm_bus;
> >> > + unsigned int type = _IOC_TYPE(cmd);
> >> >
> >> > - read_only = (O_RDWR != (file->f_flags & O_ACCMODE));
> >> > + ro = (O_RDWR != (file->f_flags & O_ACCMODE));
> >>
> >> I'm still reviewing the rest of this, but this is bugging me. The
> >> existing check for read_only looks pretty fishy to me. O_WRONLY is a
> >> thing (even though it's probably not a supportable mode for this ioctl).
> >> Why not just check for O_RDONLY?
> >
> >
> > Good question. I'll look into changing for version 2.
> > I suspect you would like something more like:
> >
> > ro = ((file->f_flags & O_ACCMODE) == O_RDONLY);
>
> Yeah. I'd make that a separate patch, and put it first in the series
> since it's a cleanup than can be applied to older kernels if necessary.
>
Will do.
--
-----------------------------------------------------------------------------
Jerry Hoemann Software Engineer Hewlett-Packard Enterprise
3404 E Harmony Rd. MS 36 phone: (970) 898-1022
Ft. Collins, CO 80528 FAX: (970) 898-0707
email: [email protected]
-----------------------------------------------------------------------------
On Tue, Nov 10, 2015 at 12:53 PM, Linda Knippers <[email protected]> wrote:
> On 11/10/2015 3:27 PM, Dan Williams wrote:
>>
>> On Tue, Nov 10, 2015 at 11:49 AM, Jerry Hoemann <[email protected]>
>> wrote:
>>>
>>> On Tue, Nov 10, 2015 at 12:51:59PM -0500, Jeff Moyer wrote:
>>>>
>>>> Jerry Hoemann <[email protected]> writes:
>>>>
>>>>> Add IOCTL type 'P' to denote NVDIMM_TYPE_PASSTHRU.
>>>>
>>>>
>>>> Can't you just make passthrough a separate command? If you actually add
>>>
>>>
>>> There are multiple conflicting NVDIMM _DSM running around, they
>>> are "device specific". So, we should plan in general and not just
>>> for the example DSM that Intel added support for. These DSM have
>>> over lapping and incompatible function ids.
>>>
>>> The Intel example is an example, not standard. They are free to
>>> change it at will. So, we can't be certain there won't be a
>>> conflict some time in the future if we try to use their number space.
>>>
>>> I'm trying to create a generic pass thru that any vendors can use.
>>> Putting
>>> this in the Intel function number space doesn't make a lot of sense to
>>> me.
>>
>>
>> It isn't the "Intel" function number space. The fact that they
>> currently align is just a happy accident.
>
>
> It's not really a happy accident. Your commit message says it
> was derived from the Intel spec 'for convenience', which I think is
> convenient
> for anything that implements that spec.
Right, and now its no longer convenient to keep things one to one.
> We've discussed ways of supporting different command sets with you
> and determined that this pass-through mechanism was a good approach
> because it allows multiple different command sets to be support in
> a generic way. Blending the two flavors (generic pass through and explicit
> function definitions) is confusing to me.
>
>> The kernel is free to break
>> the 1:1 ioctl number to DSM function number relationship, and I think
>> it would make the implementation cleaner in this case.
>
>
> To me it's less clean and even for your own example spec, less
> convenient if Intel ever updates that spec.
If that spec is ever updated any new commands will be implemented with
this new generic envelope as the marshaling mechanism. I'd also look
to convert the existing commands into this new envelope and deprecate
the existing per-DSM-function number approach. Finally I don't look
at this as purely "passthru" as the kernel will want to crack open the
input payload for commands that it cares about with kernel relevant
side effects, like namespace label updates.
On Tue, Nov 10, 2015 at 03:26:38PM -0500, Jeff Moyer wrote:
> >
> > The definition for the IOCTLs are in a user space application.
> > These aren't required in the kernel as the kernel is only a
> > pass thru.
>
> OK, I don't see the harm in including it in the kernel headers, but I'm
> not going to insist on it.
>
The IOCTL are defined in terms of the data structures representing
the dsm functions. Since I'm not supposed to share the definitions
of the DSM at this point, I can't share the IOCTL definitions.
When this restriction is lifted, I would be interested in pushing
these definitions to the appropriate header file.
> > As the DSM I'm working with isn't yet finalized, I've been told that
> > i can't share the user space portion yet.
>
> That's OK, I don't think providing the userspace code is necessary for
> this patch set to make progress. (I didn't actually ask for it, to be
> clear.)
Understood. But it is sometimes nice to have a concrete example(s)
of an interfaces usage.
...
> >> > +struct ndn_pkg {
> >> > + struct {
> >> > + __u8 dsm_uuid[16];
> >> > + __u32 dsm_in; /* size of _DSM input */
> >> > + __u32 dsm_out; /* size of user buffer */
> >> > + __u32 dsm_rev; /* revision of dsm call */
> >> > + __u32 res[8]; /* reserved must be zero */
> >> > + __u32 dsm_size; /* size _DSM would write */
> >> > + } h;
> >> > + unsigned char buf[];
> >>
> >> Please change that to:
> >> __u8 *buf;
> >> since acpi_object.buffer.pointer is a u8 *.
> >
> > buf isn't being passed to acpi_evaluate_dsm. its just being used for pointer offset
> > in acpi_nfit_ctl_passthru. The "payload" that will be passed to acpi_evaluate_dsm
> > follows.
>
> + in_buf.buffer.pointer = (void *) &pkg->buf;
>
> I see. I misread that, because you didn't actually make buf a zero
> length array (see the structure definition quoted above). I guess you
> meant to write this:
>
> unsigned char buf[0];
>
The ndn_pkg.buf struct uses a flexible array definition. This is in C99.
An explicit zero length array is a gcc extension that has been around much
longer. They behave in a similar fashion, but aren't identical. In my
limited use they behave the same.
--
-----------------------------------------------------------------------------
Jerry Hoemann Software Engineer Hewlett-Packard Enterprise
-----------------------------------------------------------------------------
On Tue, Nov 10, 2015 at 4:44 PM, Jerry Hoemann <[email protected]> wrote:
> On Tue, Nov 10, 2015 at 03:26:38PM -0500, Jeff Moyer wrote:
[..]
>> I see. I misread that, because you didn't actually make buf a zero
>> length array (see the structure definition quoted above). I guess you
>> meant to write this:
>>
>> unsigned char buf[0];
>>
>
> The ndn_pkg.buf struct uses a flexible array definition. This is in C99.
> An explicit zero length array is a gcc extension that has been around much
> longer. They behave in a similar fashion, but aren't identical. In my
> limited use they behave the same.
"buf[0]" is more idiomatic for Linux. I know I expressed concern
about compiler compatibility for ACPICA, but this path does not have
ACPICA interactions.
On Tue, Nov 10, 2015 at 04:54:28PM -0500, Jeff Moyer wrote:
> Jerry Hoemann <[email protected]> writes:
>
> > + uuid = pkg->h.dsm_uuid;
> > + rev = pkg->h.dsm_rev ? pkg->h.dsm_rev : 1;
>
> Shouldn't revision id be required?
>
Yes it should be. Good catch.
I was testing use of reclaiming previously unused members of
the structure. I should have removed this line.
And this reminds me that I should also add a test that the reserved fields
of ndn_pkg are actually zero.
I'll address both in version 2.
thanks
--
-----------------------------------------------------------------------------
Jerry Hoemann Software Engineer Hewlett-Packard Enterprise
-----------------------------------------------------------------------------
Jerry Hoemann <[email protected]> writes:
> On Tue, Nov 10, 2015 at 01:05:20PM -0500, Jeff Moyer wrote:
>> Jerry Hoemann <[email protected]> writes:
>>
>> > Add internal data structure for ndctl_passthru call.
>> >
>> > Signed-off-by: Jerry Hoemann <[email protected]>
>> > ---
>> > include/linux/libnvdimm.h | 1 +
>> > 1 file changed, 1 insertion(+)
>> >
>> > diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
>> > index 3f021dc..01117e1 100644
>> > --- a/include/linux/libnvdimm.h
>> > +++ b/include/linux/libnvdimm.h
>> > @@ -72,6 +72,7 @@ struct nvdimm_bus_descriptor {
>> > unsigned long dsm_mask;
>> > char *provider_name;
>> > ndctl_fn ndctl;
>> > + ndctl_fn ndctl_passthru;
>>
>> I don't think this is necessary. Vector off inside of __nd_ioctl. That
>> especially makes sense if you do switch to passthrough as a command
>> instead of a type, but it can work either way.
>>
>
> In an earlier version, I added a "type" argument to ndctl_fn and switched
> internally based upon that. I just came to the conclusion that I'd rather
> have two separate acpi_nfit_ctl functions than one trying to do both sets
> of argument marshaling. This is quite different both internally and
> to the caller.
>
> So, I thought it would be less confusing to the next engineer, and that
> this was a good logical separation point.
I'll leave this up to Dan. To me, it doesn't make sense to add a new
ioctl function for every new type of ioctl that get's added (assuming
more types will follow).
Cheers,
Jeff
Jerry Hoemann <[email protected]> writes:
> The ndn_pkg.buf struct uses a flexible array definition. This is in C99.
> An explicit zero length array is a gcc extension that has been around much
> longer. They behave in a similar fashion, but aren't identical. In my
> limited use they behave the same.
I could swear I've been bitten by this before, but you are correct.
Thanks,
Jeff
On Tue, Nov 10, 2015 at 06:05:15PM +0300, Dmitry Krivenok wrote:
>
> > list_for_each_entry(nvdimm_bus, &nvdimm_bus_list, list) {
> > - if (nvdimm_bus->id == id) {
> > + if (nvdimm_bus->id != id)
>
> I noticed another minor issue. You have switched from "==" to "!="
> here, but you didn't add "break" after ioctl is handled for the found
> bus.
>
I added the continue.
the code is going through a list and wants to only do action when it
matches on id. but, we still want to go through entire list.
--
-----------------------------------------------------------------------------
Jerry Hoemann Software Engineer Hewlett-Packard Enterprise
-----------------------------------------------------------------------------
> but, we still want to go through entire list.
Shouldn't you break the loop immediately after you found the bus and sent ioctl?
Maybe I'm missing something, but I see no reason to continue iterating
after the bus was found (even though you don't do anything and just
compare IDs and "continue").
On Thu, Nov 12, 2015 at 12:52:47AM +0300, Dmitry Krivenok wrote:
> > but, we still want to go through entire list.
>
> Shouldn't you break the loop immediately after you found the bus and sent ioctl?
> Maybe I'm missing something, but I see no reason to continue iterating
> after the bus was found (even though you don't do anything and just
> compare IDs and "continue").
okay, i understand now what you're saying. i'll address in version 2.
--
-----------------------------------------------------------------------------
Jerry Hoemann Software Engineer Hewlett-Packard Enterprise
-----------------------------------------------------------------------------