Skip to contentSkip to conversation

fix NVDIMM hotplug changes

PATCH In review Linda Knippers 3 participants 11 messages Nov 21, 2015 – Nov 24, 2015

Subsystems (2)
ACPI libnvdimm: non-volatile memory device
lore
Share & tools
lore

Linda Knippers

author
#0 lore raw

cover fix NVDIMM hotplug changes

Since commit 209851649dc4f7900a6bfe1de5e2640ab2c7d931 ("acpi: nfit: Add support for hot-add"), we no longer see NVDIMM devices on our NVDIMM-N systems. The NFIT/_FIT processing at initialization gets a table from _FIT but thinks it is malformed and exits, leaving us with no NFIT at all.

This series fixes the problem caused by a difference in ACPI spec interpretation regarding the layout of the _FIT vs. NFIT tables. In addition to changing the code, we are proposing wording changes to the ACPI spec.

While testing the notify code path, I discovered that the way the old and new tables were compared didn't take into account that there is some variation in table length. I also determined that the check for whether the NFIT merge was successful was backwards.

I couldn't find a generic way to invoke the notify function so my testing was done with a change to the the revision_show() function that calls acpi_nfit_notify() any time the sysfs nfit/revision file is read. That change is not included here.

These patches were tested on a system with NVDIMM-N hardware so changes related to NFIT tables not provided on a NVDIMM-N system have only been compile-tested.

Linda Knippers (3):
nfit: Account for table size length variation
nfit: Fix the check for a successful NFIT merge
nfit: Adjust for different _FIT and NFIT headers

 drivers/acpi/nfit.c | 83 +++++++++++++++++++++++++++++++++++++++++------------
 drivers/acpi/nfit.h |  3 +-
 2 files changed, 67 insertions(+), 19 deletions(-)
Signature · 1 line
1.8.3.1
author
#1 lore raw

PATCH 1/3 nfit: Account for table size length variation

The size of NFIT tables don't necessarily match the size of the data structures that we use for them. For example, the NVDIMM Control Region Structure table is shorter for a device with no block control windows than for a device with block control windows. Other tables, such as Flush Hint Address Structure and the Interleave Structure are variable length by definition.

Account for the size difference when comparing table entries by using the actual table size from the table header if it's less than the structure size.

Signed-off-by Linda Knippers <[email protected]>
---
 drivers/acpi/nfit.c | 36 ++++++++++++++++++++++++++++++------
 1 file changed, 30 insertions(+), 6 deletions(-)
drivers/acpi/nfit.c +30 -6
index f7dab53..710092a 100644
@@ -235,9 +235,13 @@ static bool add_spa(struct acpi_nfit_desc *acpi_desc,
 {
 	struct device *dev = acpi_desc->dev;
 	struct nfit_spa *nfit_spa;
+	size_t length = sizeof(*spa);
+
+	if (length > spa->header.length)
+		length = spa->header.length;
 
 	list_for_each_entry(nfit_spa, &prev->spas, list) {
-		if (memcmp(nfit_spa->spa, spa, sizeof(*spa)) == 0) {
+		if (memcmp(nfit_spa->spa, spa, length) == 0) {
 			list_move_tail(&nfit_spa->list, &acpi_desc->spas);
 			return true;
 		}

@@ -261,9 +265,13 @@ static bool add_memdev(struct acpi_nfit_desc *acpi_desc,
 {
 	struct device *dev = acpi_desc->dev;
 	struct nfit_memdev *nfit_memdev;
+	size_t length = sizeof(*memdev);
+
+	if (length > memdev->header.length)
+		length = memdev->header.length;
 
 	list_for_each_entry(nfit_memdev, &prev->memdevs, list)
-		if (memcmp(nfit_memdev->memdev, memdev, sizeof(*memdev)) == 0) {
+		if (memcmp(nfit_memdev->memdev, memdev, length) == 0) {
 			list_move_tail(&nfit_memdev->list, &acpi_desc->memdevs);
 			return true;
 		}

@@ -286,9 +294,13 @@ static bool add_dcr(struct acpi_nfit_desc *acpi_desc,
 {
 	struct device *dev = acpi_desc->dev;
 	struct nfit_dcr *nfit_dcr;
+	size_t length = sizeof(*dcr);
+
+	if (length > dcr->header.length)
+		length = dcr->header.length;
 
 	list_for_each_entry(nfit_dcr, &prev->dcrs, list)
-		if (memcmp(nfit_dcr->dcr, dcr, sizeof(*dcr)) == 0) {
+		if (memcmp(nfit_dcr->dcr, dcr, length) == 0) {
 			list_move_tail(&nfit_dcr->list, &acpi_desc->dcrs);
 			return true;
 		}

@@ -310,9 +322,13 @@ static bool add_bdw(struct acpi_nfit_desc *acpi_desc,
 {
 	struct device *dev = acpi_desc->dev;
 	struct nfit_bdw *nfit_bdw;
+	size_t length = sizeof(*bdw);
+
+	if (length > bdw->header.length)
+		length = bdw->header.length;
 
 	list_for_each_entry(nfit_bdw, &prev->bdws, list)
-		if (memcmp(nfit_bdw->bdw, bdw, sizeof(*bdw)) == 0) {
+		if (memcmp(nfit_bdw->bdw, bdw, length) == 0) {
 			list_move_tail(&nfit_bdw->list, &acpi_desc->bdws);
 			return true;
 		}

@@ -334,9 +350,13 @@ static bool add_idt(struct acpi_nfit_desc *acpi_desc,
 {
 	struct device *dev = acpi_desc->dev;
 	struct nfit_idt *nfit_idt;
+	size_t length = sizeof(*idt);
+
+	if (length > idt->header.length)
+		length = idt->header.length;
 
 	list_for_each_entry(nfit_idt, &prev->idts, list)
-		if (memcmp(nfit_idt->idt, idt, sizeof(*idt)) == 0) {
+		if (memcmp(nfit_idt->idt, idt, length) == 0) {
 			list_move_tail(&nfit_idt->list, &acpi_desc->idts);
 			return true;
 		}

@@ -358,9 +378,13 @@ static bool add_flush(struct acpi_nfit_desc *acpi_desc,
 {
 	struct device *dev = acpi_desc->dev;
 	struct nfit_flush *nfit_flush;
+	size_t length = sizeof(*flush);
+
+	if (length > flush->header.length)
+		length = flush->header.length;
 
 	list_for_each_entry(nfit_flush, &prev->flushes, list)
-		if (memcmp(nfit_flush->flush, flush, sizeof(*flush)) == 0) {
+		if (memcmp(nfit_flush->flush, flush, length) == 0) {
 			list_move_tail(&nfit_flush->list, &acpi_desc->flushes);
 			return true;
 		}
Signature · 1 line
1.8.3.1
maintainer
#2 lore raw

Re: [PATCH 1/3] nfit: Account for table size length variation

Quoted 11 lines · Linda Knippers

The size of NFIT tables don't necessarily match the size of the data structures that we use for them.  For example, the NVDIMM Control Region Structure table is shorter for a device with no block control windows than for a device with block control windows. Other tables, such as Flush Hint Address Structure and the Interleave Structure are variable length by definition.

Account for the size difference when comparing table entries by using the actual table size from the table header if it's less than the structure size.

Agreed about the variable length tables - Flush Hint and Interleave. But for the others, this makes it possible for a buggy bios implementation to have a *really* small table - one that doesn't even have enough fields to work - to pass the add_tables stage where it might have failed previously.

I feel there may be need for an ACPI clarification for this specifying whether if certain fields are irrelevant, they can be excluded entirely.

For example, the DCR wording is:
Number of Block Control Windows must match the corresponding number of Block Data Windows. Fields that follow this field are valid only if the number of Block Control Windows is non-zero.

This leads me to believe that those fields should be 'present' but ignored in the case of zero block windows.

If we make add_tables process only header.length and accept the shortened table, there is nothing to tell future code that the structure that piece of memory is casted to is a truncated one.

Thoughts?

-Vishal

author
#3 lore raw

Re: [PATCH 1/3] nfit: Account for table size length variation

Quoted 22 lines · Vishal L
Quoted 11 lines · Linda Knippers

The size of NFIT tables don't necessarily match the size of the data structures that we use for them. For example, the NVDIMM Control Region Structure table is shorter for a device with no block control windows than for a device with block control windows. Other tables, such as Flush Hint Address Structure and the Interleave Structure are variable length by definition.

Account for the size difference when comparing table entries by using the actual table size from the table header if it's less than the structure size.

Agreed about the variable length tables - Flush Hint and Interleave. But for the others, this makes it possible for a buggy bios implementation to have a *really* small table - one that doesn't even have enough fields to work - to pass the add_tables stage where it might have failed previously.

I feel there may be need for an ACPI clarification for this specifying whether if certain fields are irrelevant, they can be excluded entirely.

Quoted 8 lines

For example, the DCR wording is:
Number of Block Control Windows must match the corresponding number of Block Data Windows. Fields that follow this field are valid only if the number of Block Control Windows is non-zero.

This leads me to believe that those fields should be 'present' but ignored in the case of zero block windows.

Actually, the spec is pretty clear in this case. If you look at the length definition for that table (5-133) it says:

Length in bytes for entire structure.
The length of this structure is either 32 bytes or 80 bytes. The length of the structure can be 32 bytes only if the Number of Block Control Windows field has a value of 0.

The structure is 80 bytes but it is legal to have a 32-byte table. We hit a similar problem with the original NFIT code. We could explicitly check for a size of 32 but we didn't before.

Quoted 5 lines

If we make add_tables process only header.length and accept the shortened table, there is nothing to tell future code that the structure that piece of memory is casted to is a truncated one.

Thoughts?

If we want to be more paranoid about buggy FW when we're comparing old and new tables, we could compare based on the length of the old and new tables since we have both pieces of information. That would let you catch the case where a table size changes during a hotplug event or whenever the _FIT is processed. Since you were comparing based on structure size instead of header length, I didn't change that.

-- ljk

Quoted 3 lines

-Vishal

#4 lore raw

Re: [PATCH 1/3] nfit: Account for table size length variation

On Tue, Nov 24, 2015 at 8:24 AM, Linda Knippers <[email protected]> wrote:
[..]

Quoted 6 lines

If we want to be more paranoid about buggy FW when we're comparing old and new tables, we could compare based on the length of the old and new tables since we have both pieces of information. That would let you catch the case where a table size changes during a hotplug event or whenever the _FIT is processed. Since you were comparing based on structure size instead of header length, I didn't change that.

Comparing that the size didn't change sounds like a good incremental change to tag on later.

maintainer
#5 lore raw

Re: [PATCH 1/3] nfit: Account for table size length variation

In reply to Linda Knippers
Quoted 4 lines · Linda Knippers


Actually, the spec is pretty clear in this case.  If you look at the length definition for that table (5-133) it says:

Quoted 8 lines

Length in bytes for entire structure.
The length of this structure is either 32 bytes or 80 bytes. The length of the structure can be 32 bytes only if the Number of Block Control Windows field has a value of 0.

The structure is 80 bytes but it is legal to have a 32-byte table. We hit a similar problem with the original NFIT code.  We could explicitly check for a size of 32 but we didn't before.

Thanks, I missed that. No objections from me any more :)

Quoted 8 lines
Quoted 7 lines

If we make add_tables process only header.length and accept the shortened table, there is nothing to tell future code that the structure
that piece of memory is casted to is a truncated one.

Thoughts?

Quoted 10 lines

If we want to be more paranoid about buggy FW when we're comparing old and new tables, we could compare based on the length of the old and new
tables since we have both pieces of information.  That would let you catch
the case where a table size changes during a hotplug event or whenever the
_FIT is processed.  Since you were comparing based on structure size instead
of header length, I didn't change that.

Agreed this can be incremental work.

Quoted 6 lines

-- ljk

Quoted 3 lines

-Vishal

author
#6 lore raw

PATCH 2/3 nfit: Fix the check for a successful NFIT merge

In reply to Linda Knippers
Signed-off-by Linda Knippers <[email protected]>
---
 drivers/acpi/nfit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/acpi/nfit.c +1 -1
index 710092a..c140194d 100644
@@ -1834,7 +1834,7 @@ static void acpi_nfit_notify(struct acpi_device *adev, u32 event)
 	nfit_saved = acpi_desc->nfit;
 	acpi_desc->nfit = (struct acpi_table_nfit *)buf.pointer;
 	ret = acpi_nfit_init(acpi_desc, buf.length);
-	if (!ret) {
+	if (ret) {
 		/* Merge failed, restore old nfit, and exit */
 		acpi_desc->nfit = nfit_saved;
 		dev_err(dev, "failed to merge updated NFIT\n");
Signature · 1 line
1.8.3.1
author
#7 lore raw

PATCH 3/3 nfit: Adjust for different _FIT and NFIT headers

In reply to Linda Knippers

When support for _FIT was added, the code presumed that the data returned by the _FIT method is identical to the NFIT table, which starts with an acpi_table_header. However, the _FIT is defined to return a data in the format of a series of NFIT type structure entries and as a method, has an acpi_object header rather tahn an acpi_table_header.

To address the differences, explicitly save the acpi_table_header from the NFIT, since it is accessible through /sys, and change the nfit pointer in the acpi_desc structure to point to the table entries rather than the headers.

Reported-by Jeff Moyer ([email protected]>
Signed-off-by Linda Knippers <[email protected]>
---
 drivers/acpi/nfit.c | 47 +++++++++++++++++++++++++++++++++++------------
 drivers/acpi/nfit.h |  3 ++-
 2 files changed, 37 insertions(+), 13 deletions(-)
drivers/acpi/nfit.c +35 -12
index c140194d..b1fae13 100644
@@ -679,7 +679,7 @@ static ssize_t revision_show(struct device *dev,
 	struct nvdimm_bus_descriptor *nd_desc = to_nd_desc(nvdimm_bus);
 	struct acpi_nfit_desc *acpi_desc = to_acpi_desc(nd_desc);
 
-	return sprintf(buf, "%d\n", acpi_desc->nfit->header.revision);
+	return sprintf(buf, "%d\n", acpi_desc->acpi_header.revision);
 }
 static DEVICE_ATTR_RO(revision);
 

@@ -1676,7 +1676,6 @@ int acpi_nfit_init(struct acpi_nfit_desc *acpi_desc, acpi_size sz)
 
 	data = (u8 *) acpi_desc->nfit;
 	end = data + sz;
-	data += sizeof(struct acpi_table_nfit);
 	while (!IS_ERR_OR_NULL(data))
 		data = add_table(acpi_desc, &prev, data, end);
 

@@ -1772,13 +1771,29 @@ static int acpi_nfit_add(struct acpi_device *adev)
 		return PTR_ERR(acpi_desc);
 	}
 
-	acpi_desc->nfit = (struct acpi_table_nfit *) tbl;
+	/*
+	 * Save the acpi header for later and then skip it,
+	 * making nfit point to the first nfit table header.
+	 */
+	acpi_desc->acpi_header = *tbl;
+	acpi_desc->nfit = (void *) tbl + sizeof(struct acpi_table_nfit);
+	sz -= sizeof(struct acpi_table_nfit);
 
 	/* Evaluate _FIT and override with that if present */
 	status = acpi_evaluate_object(adev->handle, "_FIT", NULL, &buf);
 	if (ACPI_SUCCESS(status) && buf.length > 0) {
-		acpi_desc->nfit = (struct acpi_table_nfit *)buf.pointer;
-		sz = buf.length;
+		union acpi_object *obj;
+		/*
+		 * Adjust for the acpi_object header of the _FIT
+		 */
+		obj = buf.pointer;
+		if (obj->type == ACPI_TYPE_BUFFER) {
+			acpi_desc->nfit =
+				(struct acpi_nfit_header *)obj->buffer.pointer;
+			sz = obj->buffer.length;
+		} else
+			dev_dbg(dev, "%s invalid type %d, ignoring _FIT\n",
+				 __func__, (int) obj->type);
 	}
 
 	rc = acpi_nfit_init(acpi_desc, sz);

@@ -1801,7 +1816,8 @@ static void acpi_nfit_notify(struct acpi_device *adev, u32 event)
 {
 	struct acpi_nfit_desc *acpi_desc = dev_get_drvdata(&adev->dev);
 	struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL };
-	struct acpi_table_nfit *nfit_saved;
+	struct acpi_nfit_header *nfit_saved;
+	union acpi_object *obj;
 	struct device *dev = &adev->dev;
 	acpi_status status;
 	int ret;

@@ -1832,12 +1848,19 @@ static void acpi_nfit_notify(struct acpi_device *adev, u32 event)
 	}
 
 	nfit_saved = acpi_desc->nfit;
-	acpi_desc->nfit = (struct acpi_table_nfit *)buf.pointer;
-	ret = acpi_nfit_init(acpi_desc, buf.length);
-	if (ret) {
-		/* Merge failed, restore old nfit, and exit */
-		acpi_desc->nfit = nfit_saved;
-		dev_err(dev, "failed to merge updated NFIT\n");
+	obj = buf.pointer;
+	if (obj->type == ACPI_TYPE_BUFFER) {
+		acpi_desc->nfit =
+			(struct acpi_nfit_header *)obj->buffer.pointer;
+		ret = acpi_nfit_init(acpi_desc, obj->buffer.length);
+		if (ret) {
+			/* Merge failed, restore old nfit, and exit */
+			acpi_desc->nfit = nfit_saved;
+			dev_err(dev, "failed to merge updated NFIT\n");
+		}
+	} else {
+		/* Bad _FIT, restore old nfit */
+		dev_err(dev, "Invalid _FIT\n");
 	}
 	kfree(buf.pointer);
drivers/acpi/nfit.h +2 -1
index 2ea5c07..3d549a3 100644
@@ -96,7 +96,8 @@ struct nfit_mem {
 
 struct acpi_nfit_desc {
 	struct nvdimm_bus_descriptor nd_desc;
-	struct acpi_table_nfit *nfit;
+	struct acpi_table_header acpi_header;
+	struct acpi_nfit_header *nfit;
 	struct mutex spa_map_mutex;
 	struct mutex init_mutex;
 	struct list_head spa_maps;
Signature · 1 line
1.8.3.1
maintainer Reviewed-by on v1 3/3
#8 lore raw

Re: [PATCH 3/3] nfit: Adjust for different _FIT and NFIT headers

Quoted 12 lines · Linda Knippers

When support for _FIT was added, the code presumed that the data returned by the _FIT method is identical to the NFIT table, which starts with an acpi_table_header.  However, the _FIT is defined to return a data in the format of a series of NFIT type structure entries and as a method, has an acpi_object header rather tahn an acpi_table_header.

To address the differences, explicitly save the acpi_table_header from the NFIT, since it is accessible through /sys, and change the nfit pointer in the acpi_desc structure to point to the table entries rather than the headers.

Quoted 8 lines
Reported-by Jeff Moyer ([email protected]>
Signed-off-by Linda Knippers <[email protected]>

---
 drivers/acpi/nfit.c | 47 +++++++++++++++++++++++++++++++++++---------
---
 drivers/acpi/nfit.h |  3 ++-

 2 files changed, 37 insertions(+), 13 deletions(-)

This patch looks good, but the fact that we're doing the header processing earlier (before acpi_nfit_init) means that the nfit supplied by the nfit_test framework (tools/testing/nvdimm/test) can no longer have a header. The following patch fixes this.

Apart from that, and the open question for patch 1, for the series,

Reviewed-by Vishal Verma <[email protected]>

8<-----

From da2fd5730bc5125c72101a6b55b85fce97663e40 Mon Sep 17 00:00:00 2001
From: Vishal Verma <[email protected]>
Date: Mon, 23 Nov 2015 15:41:00 -0700
Subject: [PATCH] nfit_test: Fix nfit generation for test framework

The hotplug fixes move header parsing to happen earlier, and the way nfit_test calls into the nfit initialization code, a header is no longer needed. Remove the header from the table generated by nfit_test entirely.

Signed-off-by Vishal Verma <[email protected]>

---
 tools/testing/nvdimm/test/nfit.c | 46 ++++++++++++----------------------------

 1 file changed, 14 insertions(+), 32 deletions(-)
tools/testing/nvdimm/test/nfit.c +0 -0
index 40ab447..3f92d7f 100644
@@ -420,8 +420,7 @@ static struct nfit_test_resource *nfit_test_lookup(resource_size_t addr)


 static int nfit_test0_alloc(struct nfit_test *t)
 {
- size_t nfit_size = sizeof(struct acpi_table_nfit)
- + sizeof(struct acpi_nfit_system_address) * NUM_SPA
+ size_t nfit_size = sizeof(struct acpi_nfit_system_address) * NUM_SPA
  + sizeof(struct acpi_nfit_memory_map) * NUM_MEM
  + sizeof(struct acpi_nfit_control_region) * NUM_DCR
  + sizeof(struct acpi_nfit_data_region) * NUM_BDW @@ -471,8 +470,7 @@ static int nfit_test0_alloc(struct nfit_test *t)
 static int nfit_test1_alloc(struct nfit_test *t)
 {
- size_t nfit_size = sizeof(struct acpi_table_nfit)
- + sizeof(struct acpi_nfit_system_address)
+ size_t nfit_size = sizeof(struct acpi_nfit_system_address)
  + sizeof(struct acpi_nfit_memory_map)
  + sizeof(struct acpi_nfit_control_region);

@@ -488,18 +486,6 @@ static int nfit_test1_alloc(struct nfit_test *t) return 0;
 }

-static void nfit_test_init_header(struct acpi_table_nfit *nfit, size_t size) -{
- memcpy(nfit->header.signature, ACPI_SIG_NFIT, 4);
- nfit->header.length = size;
- nfit->header.revision = 1;
- memcpy(nfit->header.oem_id, "LIBND", 6);
- memcpy(nfit->header.oem_table_id, "TEST", 5);
- nfit->header.oem_revision = 1;
- memcpy(nfit->header.asl_compiler_id, "TST", 4);
- nfit->header.asl_compiler_revision = 1;
-}
-
 static void nfit_test0_setup(struct nfit_test *t)
 {
  struct nvdimm_bus_descriptor *nd_desc;
@@ -513,14 +499,12 @@ static void nfit_test0_setup(struct nfit_test *t) struct acpi_nfit_flush_address *flush;
  unsigned int offset;

- nfit_test_init_header(nfit_buf, size);
-
  /*
   * spa0 (interleave first half of dimm0 and dimm1, note storage
   * does not actually alias the related block-data-window
   * regions)
   */
- spa = nfit_buf + sizeof(struct acpi_table_nfit);
+ spa = nfit_buf;
  spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS; spa->header.length = sizeof(*spa); memcpy(spa->range_guid, to_nfit_uuid(NFIT_SPA_PM), 16); @@ -533,7 +517,7 @@ static void nfit_test0_setup(struct nfit_test *t)
   * does not actually alias the related block-data-window
   * regions)
   */
- spa = nfit_buf + sizeof(struct acpi_table_nfit) + sizeof(*spa);
+ spa = nfit_buf + sizeof(*spa);
  spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS; spa->header.length = sizeof(*spa); memcpy(spa->range_guid, to_nfit_uuid(NFIT_SPA_PM), 16); @@ -542,7 +526,7 @@ static void nfit_test0_setup(struct nfit_test *t) spa->length = SPA1_SIZE;

  /* spa2 (dcr0) dimm0 */
- spa = nfit_buf + sizeof(struct acpi_table_nfit) + sizeof(*spa) * 2;
+ spa = nfit_buf + sizeof(*spa) * 2;
  spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS; spa->header.length = sizeof(*spa); memcpy(spa->range_guid, to_nfit_uuid(NFIT_SPA_DCR), 16); @@ -551,7 +535,7 @@ static void nfit_test0_setup(struct nfit_test *t) spa->length = DCR_SIZE;

  /* spa3 (dcr1) dimm1 */
- spa = nfit_buf + sizeof(struct acpi_table_nfit) + sizeof(*spa) * 3;
+ spa = nfit_buf + sizeof(*spa) * 3;
  spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS; spa->header.length = sizeof(*spa); memcpy(spa->range_guid, to_nfit_uuid(NFIT_SPA_DCR), 16); @@ -560,7 +544,7 @@ static void nfit_test0_setup(struct nfit_test *t) spa->length = DCR_SIZE;

  /* spa4 (dcr2) dimm2 */
- spa = nfit_buf + sizeof(struct acpi_table_nfit) + sizeof(*spa) * 4;
+ spa = nfit_buf + sizeof(*spa) * 4;
  spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS; spa->header.length = sizeof(*spa); memcpy(spa->range_guid, to_nfit_uuid(NFIT_SPA_DCR), 16); @@ -569,7 +553,7 @@ static void nfit_test0_setup(struct nfit_test *t) spa->length = DCR_SIZE;

  /* spa5 (dcr3) dimm3 */
- spa = nfit_buf + sizeof(struct acpi_table_nfit) + sizeof(*spa) * 5;
+ spa = nfit_buf + sizeof(*spa) * 5;
  spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS; spa->header.length = sizeof(*spa); memcpy(spa->range_guid, to_nfit_uuid(NFIT_SPA_DCR), 16); @@ -578,7 +562,7 @@ static void nfit_test0_setup(struct nfit_test *t) spa->length = DCR_SIZE;

  /* spa6 (bdw for dcr0) dimm0 */
- spa = nfit_buf + sizeof(struct acpi_table_nfit) + sizeof(*spa) * 6;
+ spa = nfit_buf + sizeof(*spa) * 6;
  spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS; spa->header.length = sizeof(*spa); memcpy(spa->range_guid, to_nfit_uuid(NFIT_SPA_BDW), 16); @@ -587,7 +571,7 @@ static void nfit_test0_setup(struct nfit_test *t) spa->length = DIMM_SIZE;

  /* spa7 (bdw for dcr1) dimm1 */
- spa = nfit_buf + sizeof(struct acpi_table_nfit) + sizeof(*spa) * 7;
+ spa = nfit_buf + sizeof(*spa) * 7;
  spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS; spa->header.length = sizeof(*spa); memcpy(spa->range_guid, to_nfit_uuid(NFIT_SPA_BDW), 16); @@ -596,7 +580,7 @@ static void nfit_test0_setup(struct nfit_test *t) spa->length = DIMM_SIZE;

  /* spa8 (bdw for dcr2) dimm2 */
- spa = nfit_buf + sizeof(struct acpi_table_nfit) + sizeof(*spa) * 8;
+ spa = nfit_buf + sizeof(*spa) * 8;
  spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS; spa->header.length = sizeof(*spa); memcpy(spa->range_guid, to_nfit_uuid(NFIT_SPA_BDW), 16); @@ -605,7 +589,7 @@ static void nfit_test0_setup(struct nfit_test *t) spa->length = DIMM_SIZE;

  /* spa9 (bdw for dcr3) dimm3 */
- spa = nfit_buf + sizeof(struct acpi_table_nfit) + sizeof(*spa) * 9;
+ spa = nfit_buf + sizeof(*spa) * 9;
  spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS; spa->header.length = sizeof(*spa); memcpy(spa->range_guid, to_nfit_uuid(NFIT_SPA_BDW), 16); @@ -613,7 +597,7 @@ static void nfit_test0_setup(struct nfit_test *t) spa->address = t->dimm_dma[3];
  spa->length = DIMM_SIZE;

- offset = sizeof(struct acpi_table_nfit) + sizeof(*spa) * 10;
+ offset = sizeof(*spa) * 10;
  /* mem-region0 (spa0, dimm0) */
  memdev = nfit_buf + offset;
  memdev->header.type = ACPI_NFIT_TYPE_MEMORY_MAP;
@@ -1106,9 +1090,7 @@ static void nfit_test1_setup(struct nfit_test *t) struct acpi_nfit_control_region *dcr;
  struct acpi_nfit_system_address *spa;

- nfit_test_init_header(nfit_buf, size);
-
- offset = sizeof(struct acpi_table_nfit);
+ offset = 0;
  /* spa0 (flat range with no bdw aliasing) */
  spa = nfit_buf + offset;
  spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS;

Signature · 1 line
2.5.0
author
#9 lore raw

Re: [PATCH 3/3] nfit: Adjust for different _FIT and NFIT headers

Quoted 22 lines · Vishal L
Quoted 12 lines · Linda Knippers

When support for _FIT was added, the code presumed that the data returned by the _FIT method is identical to the NFIT table, which starts with an acpi_table_header. However, the _FIT is defined to return a data in the format of a series of NFIT type structure entries and as a method, has an acpi_object header rather tahn an acpi_table_header.

To address the differences, explicitly save the acpi_table_header from the NFIT, since it is accessible through /sys, and change the nfit pointer in the acpi_desc structure to point to the table entries rather than the headers.

Quoted 8 lines
Reported-by Jeff Moyer ([email protected]>
Signed-off-by Linda Knippers <[email protected]>
---
  drivers/acpi/nfit.c | 47 +++++++++++++++++++++++++++++++++++---------
---
  drivers/acpi/nfit.h |  3 ++-
  2 files changed, 37 insertions(+), 13 deletions(-)
Quoted 8 lines

This patch looks good, but the fact that we're doing the header processing earlier (before acpi_nfit_init) means that the nfit supplied by the nfit_test framework (tools/testing/nvdimm/test) can no longer have a header. The following patch fixes this.

Apart from that, and the open question for patch 1, for the series,

Reviewed-by Vishal Verma <[email protected]>

Thanks Vishal.

I hope I've addressed your question on patch 1. I think patch 1 works as well as the original code did so if you have concerns about making the code more bullet proof in the face of buggy firmware, perhaps we can address that separately from this series.

I would really like to see this series go in soon because since the hotplug code went in I can't run an upstream kernel on my hardware.

-- ljk

Quoted 181 lines

8<-----

From da2fd5730bc5125c72101a6b55b85fce97663e40 Mon Sep 17 00:00:00 2001
From: Vishal Verma <[email protected]>
Date: Mon, 23 Nov 2015 15:41:00 -0700
Subject: [PATCH] nfit_test: Fix nfit generation for test framework

The hotplug fixes move header parsing to happen earlier, and the way nfit_test calls into the nfit initialization code, a header is no longer needed. Remove the header from the table generated by nfit_test entirely.

Signed-off-by Vishal Verma <[email protected]>
---
  tools/testing/nvdimm/test/nfit.c | 46 ++++++++++++----------------------------
  1 file changed, 14 insertions(+), 32 deletions(-)
tools/testing/nvdimm/test/nfit.c +14 -32
index 40ab447..3f92d7f 100644
@@ -420,8 +420,7 @@ static struct nfit_test_resource *nfit_test_lookup(resource_size_t addr)
 
  static int nfit_test0_alloc(struct nfit_test *t)
  {
-	size_t nfit_size = sizeof(struct acpi_table_nfit)
-			+ sizeof(struct acpi_nfit_system_address) * NUM_SPA
+	size_t nfit_size = sizeof(struct acpi_nfit_system_address) * NUM_SPA
  			+ sizeof(struct acpi_nfit_memory_map) * NUM_MEM
  			+ sizeof(struct acpi_nfit_control_region) * NUM_DCR
  			+ sizeof(struct acpi_nfit_data_region) * NUM_BDW

@@ -471,8 +470,7 @@ static int nfit_test0_alloc(struct nfit_test *t)
 
  static int nfit_test1_alloc(struct nfit_test *t)
  {
-	size_t nfit_size = sizeof(struct acpi_table_nfit)
-		+ sizeof(struct acpi_nfit_system_address)
+	size_t nfit_size = sizeof(struct acpi_nfit_system_address)
  		+ sizeof(struct acpi_nfit_memory_map)
  		+ sizeof(struct acpi_nfit_control_region);
 

@@ -488,18 +486,6 @@ static int nfit_test1_alloc(struct nfit_test *t)
  	return 0;
  }
 
-static void nfit_test_init_header(struct acpi_table_nfit *nfit, size_t size)
-{
-	memcpy(nfit->header.signature, ACPI_SIG_NFIT, 4);
-	nfit->header.length = size;
-	nfit->header.revision = 1;
-	memcpy(nfit->header.oem_id, "LIBND", 6);
-	memcpy(nfit->header.oem_table_id, "TEST", 5);
-	nfit->header.oem_revision = 1;
-	memcpy(nfit->header.asl_compiler_id, "TST", 4);
-	nfit->header.asl_compiler_revision = 1;
-}
-
  static void nfit_test0_setup(struct nfit_test *t)
  {
  	struct nvdimm_bus_descriptor *nd_desc;

@@ -513,14 +499,12 @@ static void nfit_test0_setup(struct nfit_test *t)
  	struct acpi_nfit_flush_address *flush;
  	unsigned int offset;
 
-	nfit_test_init_header(nfit_buf, size);
-
  	/*
  	 * spa0 (interleave first half of dimm0 and dimm1, note storage
  	 * does not actually alias the related block-data-window
  	 * regions)
  	 */
-	spa = nfit_buf + sizeof(struct acpi_table_nfit);
+	spa = nfit_buf;
  	spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS;
  	spa->header.length = sizeof(*spa);
  	memcpy(spa->range_guid, to_nfit_uuid(NFIT_SPA_PM), 16);

@@ -533,7 +517,7 @@ static void nfit_test0_setup(struct nfit_test *t)
  	 * does not actually alias the related block-data-window
  	 * regions)
  	 */
-	spa = nfit_buf + sizeof(struct acpi_table_nfit) + sizeof(*spa);
+	spa = nfit_buf + sizeof(*spa);
  	spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS;
  	spa->header.length = sizeof(*spa);
  	memcpy(spa->range_guid, to_nfit_uuid(NFIT_SPA_PM), 16);

@@ -542,7 +526,7 @@ static void nfit_test0_setup(struct nfit_test *t)
  	spa->length = SPA1_SIZE;
 
  	/* spa2 (dcr0) dimm0 */
-	spa = nfit_buf + sizeof(struct acpi_table_nfit) + sizeof(*spa) * 2;
+	spa = nfit_buf + sizeof(*spa) * 2;
  	spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS;
  	spa->header.length = sizeof(*spa);
  	memcpy(spa->range_guid, to_nfit_uuid(NFIT_SPA_DCR), 16);

@@ -551,7 +535,7 @@ static void nfit_test0_setup(struct nfit_test *t)
  	spa->length = DCR_SIZE;
 
  	/* spa3 (dcr1) dimm1 */
-	spa = nfit_buf + sizeof(struct acpi_table_nfit) + sizeof(*spa) * 3;
+	spa = nfit_buf + sizeof(*spa) * 3;
  	spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS;
  	spa->header.length = sizeof(*spa);
  	memcpy(spa->range_guid, to_nfit_uuid(NFIT_SPA_DCR), 16);

@@ -560,7 +544,7 @@ static void nfit_test0_setup(struct nfit_test *t)
  	spa->length = DCR_SIZE;
 
  	/* spa4 (dcr2) dimm2 */
-	spa = nfit_buf + sizeof(struct acpi_table_nfit) + sizeof(*spa) * 4;
+	spa = nfit_buf + sizeof(*spa) * 4;
  	spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS;
  	spa->header.length = sizeof(*spa);
  	memcpy(spa->range_guid, to_nfit_uuid(NFIT_SPA_DCR), 16);

@@ -569,7 +553,7 @@ static void nfit_test0_setup(struct nfit_test *t)
  	spa->length = DCR_SIZE;
 
  	/* spa5 (dcr3) dimm3 */
-	spa = nfit_buf + sizeof(struct acpi_table_nfit) + sizeof(*spa) * 5;
+	spa = nfit_buf + sizeof(*spa) * 5;
  	spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS;
  	spa->header.length = sizeof(*spa);
  	memcpy(spa->range_guid, to_nfit_uuid(NFIT_SPA_DCR), 16);

@@ -578,7 +562,7 @@ static void nfit_test0_setup(struct nfit_test *t)
  	spa->length = DCR_SIZE;
 
  	/* spa6 (bdw for dcr0) dimm0 */
-	spa = nfit_buf + sizeof(struct acpi_table_nfit) + sizeof(*spa) * 6;
+	spa = nfit_buf + sizeof(*spa) * 6;
  	spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS;
  	spa->header.length = sizeof(*spa);
  	memcpy(spa->range_guid, to_nfit_uuid(NFIT_SPA_BDW), 16);

@@ -587,7 +571,7 @@ static void nfit_test0_setup(struct nfit_test *t)
  	spa->length = DIMM_SIZE;
 
  	/* spa7 (bdw for dcr1) dimm1 */
-	spa = nfit_buf + sizeof(struct acpi_table_nfit) + sizeof(*spa) * 7;
+	spa = nfit_buf + sizeof(*spa) * 7;
  	spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS;
  	spa->header.length = sizeof(*spa);
  	memcpy(spa->range_guid, to_nfit_uuid(NFIT_SPA_BDW), 16);

@@ -596,7 +580,7 @@ static void nfit_test0_setup(struct nfit_test *t)
  	spa->length = DIMM_SIZE;
 
  	/* spa8 (bdw for dcr2) dimm2 */
-	spa = nfit_buf + sizeof(struct acpi_table_nfit) + sizeof(*spa) * 8;
+	spa = nfit_buf + sizeof(*spa) * 8;
  	spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS;
  	spa->header.length = sizeof(*spa);
  	memcpy(spa->range_guid, to_nfit_uuid(NFIT_SPA_BDW), 16);

@@ -605,7 +589,7 @@ static void nfit_test0_setup(struct nfit_test *t)
  	spa->length = DIMM_SIZE;
 
  	/* spa9 (bdw for dcr3) dimm3 */
-	spa = nfit_buf + sizeof(struct acpi_table_nfit) + sizeof(*spa) * 9;
+	spa = nfit_buf + sizeof(*spa) * 9;
  	spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS;
  	spa->header.length = sizeof(*spa);
  	memcpy(spa->range_guid, to_nfit_uuid(NFIT_SPA_BDW), 16);

@@ -613,7 +597,7 @@ static void nfit_test0_setup(struct nfit_test *t)
  	spa->address = t->dimm_dma[3];
  	spa->length = DIMM_SIZE;
 
-	offset = sizeof(struct acpi_table_nfit) + sizeof(*spa) * 10;
+	offset = sizeof(*spa) * 10;
  	/* mem-region0 (spa0, dimm0) */
  	memdev = nfit_buf + offset;
  	memdev->header.type = ACPI_NFIT_TYPE_MEMORY_MAP;

@@ -1106,9 +1090,7 @@ static void nfit_test1_setup(struct nfit_test *t)
  	struct acpi_nfit_control_region *dcr;
  	struct acpi_nfit_system_address *spa;
 
-	nfit_test_init_header(nfit_buf, size);
-
-	offset = sizeof(struct acpi_table_nfit);
+	offset = 0;
  	/* spa0 (flat range with no bdw aliasing) */
  	spa = nfit_buf + offset;
  	spa->header.type = ACPI_NFIT_TYPE_SYSTEM_ADDRESS;
maintainer
#10 lore raw

Re: [PATCH 3/3] nfit: Adjust for different _FIT and NFIT headers

Quoted 4 lines · Linda Knippers

I would really like to see this series go in soon because since the hotplug code went in I can't run an upstream kernel on my hardware.

Agreed - I'm guessing Dan will queue this for the next -rc pull request. Thanks for the fixes!