From: Dan Williams dan.j.williams@intel.com Sent: Monday, January 28, 2019 9:22 PM To: linux-nvdimm@lists.01.org Cc: stable@vger.kernel.org; Dexuan Cui decui@microsoft.com; linux-kernel@vger.kernel.org Subject: [PATCH] nfit: Fix nfit_intel_shutdown_status() command submission

The implementation is broken in all the ways the unit test did not touch:

1/ The local definition of in_buf and in_obj violated C99 initializer expectations for zeroing. By only initializing 2 out of the three struct members the compiler was free to zero-initialize the remaining entry even though the aliased location in the union was initialized.

2/ The implementation made assumptions about the state of the 'smart' payload after command execution that are satisfied by acpi_nfit_ctl(), but not acpi_evaluate_dsm().

3/ populate_shutdown_status() is skipped on Intel NVDIMMs due to the early return for skipping the common _LS{I,R,W} enabling.

4/ The input length should be zero.

This breakage was missed due to the unit test implementation only testing the case where nfit_intel_shutdown_status() returns a valid payload.

Much of this complexity would be saved if acpi_nfit_ctl() could be used, but that currently requires a 'struct nvdimm *' argument and one is not created until later in the init process. The health result is needed before the device is created because the payload gates whether the nmemX/nfit/dirty_shutdown property is visible in sysfs.

Cc: stable@vger.kernel.org Fixes: 0ead11181fe0 ("acpi, nfit: Collect shutdown status") Reported-by: Dexuan Cui decui@microsoft.com Signed-off-by: Dan Williams dan.j.williams@intel.com

---

drivers/acpi/nfit/core.c | 41 ++++++++++++++++++++++++----------------- 1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c index e18ade5d74e9..0a49c57334cc 100644 --- a/drivers/acpi/nfit/core.c +++ b/drivers/acpi/nfit/core.c @@ -1759,14 +1759,14 @@ static bool acpi_nvdimm_has_method(struct acpi_device *adev, char *method)

__weak void nfit_intel_shutdown_status(struct nfit_mem *nfit_mem) {

• struct device *dev = &nfit_mem->adev->dev; struct nd_intel_smart smart = { 0 }; union acpi_object in_buf = {

• .type = ACPI_TYPE_BUFFER,
  

• .buffer.pointer = (char *) &smart,
  

• .buffer.length = sizeof(smart),
  

• .buffer.type = ACPI_TYPE_BUFFER,
  

• .buffer.length = 0,
  

  }; union acpi_object in_obj = {

• .type = ACPI_TYPE_PACKAGE,
  

• .package.type = ACPI_TYPE_PACKAGE,
  

  .package.count = 1, .package.elements = &in_buf, };

@@ -1781,8 +1781,15 @@ __weak void nfit_intel_shutdown_status(struct nfit_mem *nfit_mem) return;

out_obj = acpi_evaluate_dsm(handle, guid, revid, func, &in_obj);

• if (!out_obj)

• if (!out_obj || out_obj->type != ACPI_TYPE_BUFFER

• 	|| out_obj->buffer.length < sizeof(smart)) {
  

• dev_dbg(dev->parent, "%s: failed to retrieve initial health\n",
  

• 		dev_name(dev));
  

• ACPI_FREE(out_obj);
  

  return;

• }

• memcpy(&smart, out_obj->buffer.pointer, sizeof(smart));

• ACPI_FREE(out_obj);

  if (smart.flags & ND_INTEL_SMART_SHUTDOWN_VALID) { if (smart.shutdown_state)

@@ -1793,7 +1800,6 @@ __weak void nfit_intel_shutdown_status(struct nfit_mem *nfit_mem) set_bit(NFIT_MEM_DIRTY_COUNT, &nfit_mem->flags); nfit_mem->dirty_shutdown = smart.shutdown_count; }

• ACPI_FREE(out_obj);

}

static void populate_shutdown_status(struct nfit_mem *nfit_mem) @@ -1915,18 +1921,19 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc, | 1 << ND_CMD_SET_CONFIG_DATA; if (family == NVDIMM_FAMILY_INTEL && (dsm_mask & label_mask) == label_mask)

• return 0;
  

• if (acpi_nvdimm_has_method(adev_dimm, "_LSI")

• 	&& acpi_nvdimm_has_method(adev_dimm, "_LSR")) {
  

• dev_dbg(dev, "%s: has _LSR\n", dev_name(&adev_dimm->dev));
  

• set_bit(NFIT_MEM_LSR, &nfit_mem->flags);
  

• }

• /* skip _LS{I,R,W} enabling */;
  

• else {

• if (acpi_nvdimm_has_method(adev_dimm, "_LSI")
  

• 		&& acpi_nvdimm_has_method(adev_dimm, "_LSR")) {
  

• 	dev_dbg(dev, "%s: has _LSR\n", dev_name(&adev_dimm->dev));
  

• 	set_bit(NFIT_MEM_LSR, &nfit_mem->flags);
  

• }
  
  

• if (test_bit(NFIT_MEM_LSR, &nfit_mem->flags)

• 	&& acpi_nvdimm_has_method(adev_dimm, "_LSW")) {
  

• dev_dbg(dev, "%s: has _LSW\n", dev_name(&adev_dimm->dev));
  

• set_bit(NFIT_MEM_LSW, &nfit_mem->flags);
  

• if (test_bit(NFIT_MEM_LSR, &nfit_mem->flags)
  

• 		&& acpi_nvdimm_has_method(adev_dimm, "_LSW")) {
  

• 	dev_dbg(dev, "%s: has _LSW\n", dev_name(&adev_dimm->dev));
  

• 	set_bit(NFIT_MEM_LSW, &nfit_mem->flags);
  

• }
  

  }

  populate_shutdown_status(nfit_mem);

Reviewed-by: Dexuan Cui decui@microsoft.com

Thanks for the fix, Dan!

-- Dexuan