Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965104AbbDUWGA (ORCPT ); Tue, 21 Apr 2015 18:06:00 -0400 Received: from mail-wg0-f46.google.com ([74.125.82.46]:36643 "EHLO mail-wg0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S964844AbbDUWF5 (ORCPT ); Tue, 21 Apr 2015 18:05:57 -0400 MIME-Version: 1.0 In-Reply-To: <1429651258.17259.60.camel@misato.fc.hp.com> References: <20150418013256.25237.96403.stgit@dwillia2-desk3.amr.corp.intel.com> <20150418013557.25237.81354.stgit@dwillia2-desk3.amr.corp.intel.com> <1429651258.17259.60.camel@misato.fc.hp.com> Date: Tue, 21 Apr 2015 15:05:56 -0700 Message-ID: Subject: Re: [Linux-nvdimm] [PATCH 08/21] nd: ndctl.h, the nd ioctl abi From: Dan Williams To: Toshi Kani Cc: "linux-nvdimm@lists.01.org" , linux-acpi@vger.kernel.org, "Rafael J. Wysocki" , Robert Moore , "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 15673 Lines: 404 On Tue, Apr 21, 2015 at 2:20 PM, Toshi Kani wrote: > On Fri, 2015-04-17 at 21:35 -0400, Dan Williams wrote: >> Most configuration of the nd-subsystem is done via nd-sysfs. However, >> the NFIT specification defines a small set of messages that can be >> passed to the subsystem via platform-firmware-defined methods. The >> command set (as of the current version of the NFIT-DSM spec) is: >> >> NFIT_CMD_SMART: media health and diagnostics >> NFIT_CMD_GET_CONFIG_SIZE: size of the label space >> NFIT_CMD_GET_CONFIG_DATA: read label >> NFIT_CMD_SET_CONFIG_DATA: write label >> NFIT_CMD_VENDOR: vendor-specific command passthrough >> NFIT_CMD_ARS_CAP: report address-range-scrubbing capabilities >> NFIT_CMD_START_ARS: initiate scrubbing >> NFIT_CMD_QUERY_ARS: report on scrubbing state >> NFIT_CMD_SMART_THRESHOLD: configure alarm thresholds for smart events >> >> Most of the commands target a specific dimm. However, the >> address-range-scrubbing commands target the entire NFIT-bus / platform. >> The 'commands' attribute of an nd-bus, or an nd-dimm enumerate the >> supported commands for that object. >> >> Cc: >> Cc: Robert Moore >> Cc: Rafael J. Wysocki >> Reported-by: Nicholas Moulin >> Signed-off-by: Dan Williams >> --- >> drivers/block/nd/Kconfig | 11 + >> drivers/block/nd/acpi.c | 333 +++++++++++++++++++++++++++++++++++++++++ >> drivers/block/nd/bus.c | 230 ++++++++++++++++++++++++++++ >> drivers/block/nd/core.c | 17 ++ >> drivers/block/nd/dimm_devs.c | 69 ++++++++ >> drivers/block/nd/nd-private.h | 11 + >> drivers/block/nd/nd.h | 21 +++ >> drivers/block/nd/test/nfit.c | 89 +++++++++++ >> include/uapi/linux/Kbuild | 1 >> include/uapi/linux/ndctl.h | 178 ++++++++++++++++++++++ >> 10 files changed, 950 insertions(+), 10 deletions(-) >> create mode 100644 drivers/block/nd/nd.h >> create mode 100644 include/uapi/linux/ndctl.h >> >> diff --git a/drivers/block/nd/Kconfig b/drivers/block/nd/Kconfig >> index 0106b3807202..6c15d10bf4e0 100644 >> --- a/drivers/block/nd/Kconfig >> +++ b/drivers/block/nd/Kconfig >> @@ -42,6 +42,17 @@ config NFIT_ACPI >> enables the core to craft ACPI._DSM messages for platform/dimm >> configuration. >> >> +config NFIT_ACPI_DEBUG >> + bool "NFIT ACPI: Turn on extra debugging" >> + depends on NFIT_ACPI >> + depends on DYNAMIC_DEBUG >> + default n >> + help >> + Enabling this option causes the nd_acpi driver to dump the >> + input and output buffers of _DSM operations on the ACPI0012 >> + device, which can be very verbose. Leave it disabled unless >> + you are debugging a hardware / firmware issue. >> + >> config NFIT_TEST >> tristate "NFIT TEST: Manufactured NFIT for interface testing" >> depends on DMA_CMA >> diff --git a/drivers/block/nd/acpi.c b/drivers/block/nd/acpi.c >> index 48db723d7a90..073ff28fdbfe 100644 >> --- a/drivers/block/nd/acpi.c >> +++ b/drivers/block/nd/acpi.c >> @@ -13,8 +13,10 @@ >> #include >> #include >> #include >> +#include >> #include >> #include "nfit.h" >> +#include "nd.h" >> >> enum { >> NFIT_ACPI_NOTIFY_TABLE = 0x80, >> @@ -26,20 +28,330 @@ struct acpi_nfit { >> struct nd_bus *nd_bus; >> }; >> >> +static struct acpi_nfit *to_acpi_nfit(struct nfit_bus_descriptor *nfit_desc) >> +{ >> + return container_of(nfit_desc, struct acpi_nfit, nfit_desc); >> +} >> + >> +#define NFIT_ACPI_MAX_ELEM 4 >> +struct nfit_cmd_desc { >> + int in_num; >> + int out_num; >> + u32 in_sizes[NFIT_ACPI_MAX_ELEM]; >> + int out_sizes[NFIT_ACPI_MAX_ELEM]; >> +}; >> + >> +static const struct nfit_cmd_desc nfit_dimm_descs[] = { >> + [NFIT_CMD_IMPLEMENTED] = { }, >> + [NFIT_CMD_SMART] = { >> + .out_num = 2, >> + .out_sizes = { 4, 8, }, >> + }, >> + [NFIT_CMD_SMART_THRESHOLD] = { >> + .out_num = 2, >> + .out_sizes = { 4, 8, }, >> + }, >> + [NFIT_CMD_DIMM_FLAGS] = { >> + .out_num = 2, >> + .out_sizes = { 4, 4 }, >> + }, >> + [NFIT_CMD_GET_CONFIG_SIZE] = { >> + .out_num = 3, >> + .out_sizes = { 4, 4, 4, }, >> + }, >> + [NFIT_CMD_GET_CONFIG_DATA] = { >> + .in_num = 2, >> + .in_sizes = { 4, 4, }, >> + .out_num = 2, >> + .out_sizes = { 4, UINT_MAX, }, >> + }, >> + [NFIT_CMD_SET_CONFIG_DATA] = { >> + .in_num = 3, >> + .in_sizes = { 4, 4, UINT_MAX, }, >> + .out_num = 1, >> + .out_sizes = { 4, }, >> + }, >> + [NFIT_CMD_VENDOR] = { >> + .in_num = 3, >> + .in_sizes = { 4, 4, UINT_MAX, }, >> + .out_num = 3, >> + .out_sizes = { 4, 4, UINT_MAX, }, >> + }, >> +}; >> + >> +static const struct nfit_cmd_desc nfit_acpi_descs[] = { >> + [NFIT_CMD_IMPLEMENTED] = { }, >> + [NFIT_CMD_ARS_CAP] = { >> + .in_num = 2, >> + .in_sizes = { 8, 8, }, >> + .out_num = 2, >> + .out_sizes = { 4, 4, }, >> + }, >> + [NFIT_CMD_ARS_START] = { >> + .in_num = 4, >> + .in_sizes = { 8, 8, 2, 6, }, >> + .out_num = 1, >> + .out_sizes = { 4, }, >> + }, >> + [NFIT_CMD_ARS_QUERY] = { >> + .out_num = 2, >> + .out_sizes = { 4, UINT_MAX, }, >> + }, >> +}; >> + >> +static u32 to_cmd_in_size(struct nd_dimm *nd_dimm, int cmd, >> + const struct nfit_cmd_desc *desc, int idx, void *buf) >> +{ >> + if (idx >= desc->in_num) >> + return UINT_MAX; >> + >> + if (desc->in_sizes[idx] < UINT_MAX) >> + return desc->in_sizes[idx]; >> + >> + if (nd_dimm && cmd == NFIT_CMD_SET_CONFIG_DATA && idx == 2) { >> + struct nfit_cmd_set_config_hdr *hdr = buf; >> + >> + return hdr->in_length; >> + } else if (nd_dimm && cmd == NFIT_CMD_VENDOR && idx == 2) { >> + struct nfit_cmd_vendor_hdr *hdr = buf; >> + >> + return hdr->in_length; >> + } >> + >> + return UINT_MAX; >> +} >> + >> +static u32 to_cmd_out_size(struct nd_dimm *nd_dimm, int cmd, >> + const struct nfit_cmd_desc *desc, int idx, >> + void *buf, u32 out_length, u32 offset) >> +{ >> + if (idx >= desc->out_num) >> + return UINT_MAX; >> + >> + if (desc->out_sizes[idx] < UINT_MAX) >> + return desc->out_sizes[idx]; >> + >> + if (offset >= out_length) >> + return UINT_MAX; >> + >> + if (nd_dimm && cmd == NFIT_CMD_GET_CONFIG_DATA && idx == 1) >> + return out_length - offset; >> + else if (nd_dimm && cmd == NFIT_CMD_VENDOR && idx == 2) >> + return out_length - offset; >> + else if (!nd_dimm && cmd == NFIT_CMD_ARS_QUERY && idx == 1) >> + return out_length - offset; >> + >> + return UINT_MAX; >> +} >> + >> +static u8 nd_acpi_uuids[2][16]; /* initialized at nd_acpi_init */ >> + >> +static u8 *nd_acpi_bus_uuid(void) >> +{ >> + return nd_acpi_uuids[0]; >> +} >> + >> +static u8 *nd_acpi_dimm_uuid(void) >> +{ >> + return nd_acpi_uuids[1]; >> +} >> + >> static int nd_acpi_ctl(struct nfit_bus_descriptor *nfit_desc, >> struct nd_dimm *nd_dimm, unsigned int cmd, void *buf, >> unsigned int buf_len) >> { >> - return -ENOTTY; >> + struct acpi_nfit *nfit = to_acpi_nfit(nfit_desc); >> + union acpi_object in_obj, in_buf, *out_obj; >> + const struct nfit_cmd_desc *desc = NULL; >> + struct device *dev = &nfit->dev->dev; >> + const char *cmd_name, *dimm_name; >> + unsigned long dsm_mask; >> + acpi_handle handle; >> + u32 offset; >> + int rc, i; >> + u8 *uuid; >> + >> + if (nd_dimm) { >> + struct acpi_device *adev = nd_dimm_get_pdata(nd_dimm); >> + >> + if (cmd < ARRAY_SIZE(nfit_dimm_descs)) >> + desc = &nfit_dimm_descs[cmd]; >> + cmd_name = nfit_dimm_cmd_name(cmd); >> + dsm_mask = nd_dimm_get_dsm_mask(nd_dimm); >> + handle = adev->handle; >> + uuid = nd_acpi_dimm_uuid(); >> + dimm_name = dev_name(&adev->dev); >> + } else { >> + if (cmd < ARRAY_SIZE(nfit_acpi_descs)) >> + desc = &nfit_acpi_descs[cmd]; >> + cmd_name = nfit_bus_cmd_name(cmd); >> + dsm_mask = nfit_desc->dsm_mask; >> + handle = nfit->dev->handle; >> + uuid = nd_acpi_bus_uuid(); >> + dimm_name = "bus"; >> + } >> + >> + if (!desc || (cmd && (desc->out_num + desc->in_num == 0))) >> + return -ENOTTY; >> + >> + if (!test_bit(cmd, &dsm_mask)) >> + return -ENOTTY; >> + >> + 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 = buf; >> + in_buf.buffer.length = 0; >> + >> + /* double check that the nfit_acpi_cmd_descs table is self consistent */ >> + if (desc->in_num > NFIT_ACPI_MAX_ELEM) { >> + WARN_ON_ONCE(1); >> + return -ENXIO; >> + } >> + >> + for (i = 0; i < desc->in_num; i++) { >> + u32 in_size; >> + >> + in_size = to_cmd_in_size(nd_dimm, cmd, desc, i, buf); >> + if (in_size == UINT_MAX) { >> + dev_err(dev, "%s:%s unknown input size cmd: %s field: %d\n", >> + __func__, dimm_name, cmd_name, i); >> + return -ENXIO; >> + } >> + in_buf.buffer.length += in_size; >> + if (in_buf.buffer.length > buf_len) { >> + dev_err(dev, "%s:%s input underrun cmd: %s field: %d\n", >> + __func__, dimm_name, cmd_name, i); >> + return -ENXIO; >> + } >> + } >> + >> + dev_dbg(dev, "%s:%s cmd: %s input length: %d\n", __func__, dimm_name, >> + cmd_name, in_buf.buffer.length); >> + if (IS_ENABLED(CONFIG_NFIT_ACPI_DEBUG)) >> + print_hex_dump_debug(cmd_name, 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, 1, cmd, &in_obj); >> + if (!out_obj) { >> + dev_dbg(dev, "%s:%s _DSM failed cmd: %s\n", __func__, dimm_name, >> + cmd_name); >> + return -EINVAL; >> + } >> + >> + if (out_obj->package.type != ACPI_TYPE_BUFFER) { >> + dev_dbg(dev, "%s:%s unexpected output object type cmd: %s type: %d\n", >> + __func__, dimm_name, cmd_name, out_obj->type); >> + rc = -EINVAL; >> + goto out; >> + } >> + >> + dev_dbg(dev, "%s:%s cmd: %s output length: %d\n", __func__, dimm_name, >> + cmd_name, out_obj->buffer.length); >> + if (IS_ENABLED(CONFIG_NFIT_ACPI_DEBUG)) >> + print_hex_dump_debug(cmd_name, DUMP_PREFIX_OFFSET, 4, >> + 4, out_obj->buffer.pointer, min_t(u32, 128, >> + out_obj->buffer.length), true); >> + >> + for (i = 0, offset = 0; i < desc->out_num; i++) { >> + u32 out_size = to_cmd_out_size(nd_dimm, cmd, desc, i, buf, >> + out_obj->buffer.length, offset); >> + >> + if (out_size == UINT_MAX) { >> + dev_dbg(dev, "%s:%s unknown output size cmd: %s field: %d\n", >> + __func__, dimm_name, cmd_name, i); >> + break; >> + } >> + >> + if (offset + out_size > out_obj->buffer.length) { >> + dev_dbg(dev, "%s:%s output object underflow cmd: %s field: %d\n", >> + __func__, dimm_name, cmd_name, i); >> + break; >> + } >> + >> + if (in_buf.buffer.length + offset + out_size > buf_len) { >> + dev_dbg(dev, "%s:%s output overrun cmd: %s field: %d\n", >> + __func__, dimm_name, cmd_name, i); >> + rc = -ENXIO; >> + goto out; >> + } >> + memcpy(buf + in_buf.buffer.length + offset, >> + out_obj->buffer.pointer + offset, out_size); >> + offset += out_size; >> + } >> + if (offset + in_buf.buffer.length < buf_len) { >> + if (i >= 1) { >> + /* >> + * status valid, return the number of bytes left >> + * unfilled in the output buffer >> + */ >> + rc = buf_len - offset - in_buf.buffer.length; >> + } else { >> + dev_err(dev, "%s:%s underrun cmd: %s buf_len: %d out_len: %d\n", >> + __func__, dimm_name, cmd_name, buf_len, offset); >> + rc = -ENXIO; >> + } >> + } else >> + rc = 0; >> + >> + out: >> + ACPI_FREE(out_obj); >> + >> + return rc; >> +} >> + >> +static int nd_acpi_add_dimm(struct nfit_bus_descriptor *nfit_desc, >> + struct nd_dimm *nd_dimm) >> +{ >> + struct acpi_nfit *nfit = to_acpi_nfit(nfit_desc); >> + u32 nfit_handle = to_nfit_handle(nd_dimm); >> + struct device *dev = &nfit->dev->dev; >> + struct acpi_device *acpi_dimm; >> + unsigned long dsm_mask = 0; >> + u8 *uuid = nd_acpi_dimm_uuid(); >> + unsigned long long sta; >> + int i, rc = -ENODEV; >> + acpi_status status; >> + >> + acpi_dimm = acpi_find_child_device(nfit->dev, nfit_handle, false); >> + if (!acpi_dimm) { >> + dev_err(dev, "no ACPI.NFIT device with _ADR %#x, disabling...\n", >> + nfit_handle); >> + return -ENODEV; >> + } >> + >> + status = acpi_evaluate_integer(acpi_dimm->handle, "_STA", NULL, &sta); >> + if (status == AE_NOT_FOUND) >> + dev_err(dev, "%s missing _STA, disabling...\n", >> + dev_name(&acpi_dimm->dev)); > > I do not think it is correct to set a DIMM _ADR object disabled when it > has no _STA. ACPI 6.0 spec states the followings: > > - Section 6.3.7 _STA, "If a device object describes a device that is > not on an enumerable bus and the device object does not have an _STA > object, then OSPM assumes that the device is present, enabled, shown in > the UI, and functioning." Ok, I'll take a look. [..] > So, in this case, it should set the DIMM object enabled or look up the > NFIT table to check the presence. At this point we've already determined that a dimm device is present because nd_acpi_add_dimm() is called for each dimm found in the NFIT. Does that count as "enumerable" and require an _STA? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/