[PATCH 0/2] PNPACPI, x86/PCI: handle _CRS windows like Windows does

[PATCH 0/2] PNPACPI, x86/PCI: handle _CRS windows like Windows does

[Bjorn Helgaas]

These patches simplify the way we handle Address Space descriptors and make
Linux do it more like Windows does.

The PNPACPI change is definitely .35 material, since I don't think anybody
uses this information via PNP yet.

My first thought was that the x86/PCI change is for .35, too, but it does
make a difference in some cases, so maybe it should go in .34.

Here's an example where it matters.  ACPI _CRS could have an MMIO Address Space
descriptor like this:

  _MIN 0xE0000000
  _MAX 0xFEBFFFFF
  _LEN 0x10000000,

which doesn't satisfy the spec requirement that:

  _MAX == _MIN + _LEN - 1,

but obviously we can't rely on spec compliance.  These are the ways we could
interpret it:

  [mem 0xe0000000-0xefffffff] current Linux way (end is _MIN + _LEN - 1)
  [mem 0xe0000000-0xfebfffff] new Linux way (end is _MAX)
  [mem 0xe0000000-0xfebfffff] Windows way (end is _MAX)

The risk is that BIOS might have placed a PCI device in the area that we used
to think was illegal, i.e., anywhere in [mem 0xf0000000-0xfebfffff], and Linux
will try to move the device unnecessarily, and that move might fail.

---

Bjorn Helgaas (2):
      PNPACPI: compute Address Space length rather than using _LEN
      x86/PCI: compute Address Space length rather than using _LEN

 arch/x86/pci/acpi.c            |   40 ++--------------------------------------
 drivers/pnp/pnpacpi/rsparser.c |   26 ++++----------------------
 2 files changed, 6 insertions(+), 60 deletions(-)

[PATCH 1/2] PNPACPI: compute Address Space length rather than using _LEN

[Bjorn Helgaas]

ACPI _CRS Address Space Descriptors have _MIN, _MAX, and _LEN.  Linux has
been computing Address Spaces as [_MIN to _MIN + _LEN - 1].  Based on the
tests in the bug reports below, Windows apparently uses [_MIN to _MAX].

Per spec (ACPI 4.0, Table 6-40), for _CRS fixed-size, fixed location
descriptors, "_LEN must be (_MAX - _MIN + 1)", and when that's true, it
doesn't matter which way we compute the end.  But of course, there are
BIOSes that don't follow this rule, and we're better off if Linux handles
those exceptions the same way as Windows.

This patch makes Linux use [_MIN to _MAX], as Windows seems to do.  This
effectively reverts 3162b6f0c5e and replaces it with simpler code.

    https://bugzilla.kernel.org/show_bug.cgi?id=14337 (round)
    https://bugzilla.kernel.org/show_bug.cgi?id=15480 (truncate)

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
---

 drivers/pnp/pnpacpi/rsparser.c |   26 ++++----------------------
 1 files changed, 4 insertions(+), 22 deletions(-)


diff --git a/drivers/pnp/pnpacpi/rsparser.c b/drivers/pnp/pnpacpi/rsparser.c
index 35bb44a..100e4d9 100644
--- a/drivers/pnp/pnpacpi/rsparser.c
+++ b/drivers/pnp/pnpacpi/rsparser.c
@@ -274,26 +274,6 @@ static void pnpacpi_parse_allocated_busresource(struct pnp_dev *dev,
 	pnp_add_bus_resource(dev, start, end);
 }
 
-static u64 addr_space_length(struct pnp_dev *dev, u64 min, u64 max, u64 len)
-{
-	u64 max_len;
-
-	max_len = max - min + 1;
-	if (len <= max_len)
-		return len;
-
-	/*
-	 * Per 6.4.3.5, _LEN cannot exceed _MAX - _MIN + 1, but some BIOSes
-	 * don't do this correctly, e.g.,
-	 * https://bugzilla.kernel.org/show_bug.cgi?id=15480
-	 */
-	dev_info(&dev->dev,
-	         "resource length %#llx doesn't fit in %#llx-%#llx, trimming\n",
-		 (unsigned long long) len, (unsigned long long) min,
-		 (unsigned long long) max);
-	return max_len;
-}
-
 static void pnpacpi_parse_allocated_address_space(struct pnp_dev *dev,
 						  struct acpi_resource *res)
 {
@@ -309,7 +289,8 @@ static void pnpacpi_parse_allocated_address_space(struct pnp_dev *dev,
 		return;
 	}
 
-	len = addr_space_length(dev, p->minimum, p->maximum, p->address_length);
+	/* Windows apparently computes length rather than using _LEN */
+	len = p->maximum - p->minimum + 1;
 	window = (p->producer_consumer == ACPI_PRODUCER) ? 1 : 0;
 
 	if (p->resource_type == ACPI_MEMORY_RANGE)
@@ -330,7 +311,8 @@ static void pnpacpi_parse_allocated_ext_address_space(struct pnp_dev *dev,
 	int window;
 	u64 len;
 
-	len = addr_space_length(dev, p->minimum, p->maximum, p->address_length);
+	/* Windows apparently computes length rather than using _LEN */
+	len = p->maximum - p->minimum + 1;
 	window = (p->producer_consumer == ACPI_PRODUCER) ? 1 : 0;
 
 	if (p->resource_type == ACPI_MEMORY_RANGE)

[PATCH 2/2] x86/PCI: compute Address Space length rather than using _LEN

[Bjorn Helgaas]

ACPI _CRS Address Space Descriptors have _MIN, _MAX, and _LEN.  Linux has
been computing Address Spaces as [_MIN to _MIN + _LEN - 1].  Based on the
tests in the bug reports below, Windows apparently uses [_MIN to _MAX].

Per spec (ACPI 4.0, Table 6-40), for _CRS fixed-size, fixed location
descriptors, "_LEN must be (_MAX - _MIN + 1)", and when that's true, it
doesn't matter which way we compute the end.  But of course, there are
BIOSes that don't follow this rule, and we're better off if Linux handles
those exceptions the same way as Windows.

This patch makes Linux use [_MIN to _MAX], as Windows seems to do.  This
effectively reverts d558b483d5 and 03db42adfe and replaces them with
simpler code.

    https://bugzilla.kernel.org/show_bug.cgi?id=14337 (round)
    https://bugzilla.kernel.org/show_bug.cgi?id=15480 (truncate)

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
---

 arch/x86/pci/acpi.c |   40 ++--------------------------------------
 1 files changed, 2 insertions(+), 38 deletions(-)


diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
index 44f83ce..31930fd 100644
--- a/arch/x86/pci/acpi.c
+++ b/arch/x86/pci/acpi.c
@@ -121,30 +121,6 @@ count_resource(struct acpi_resource *acpi_res, void *data)
 	return AE_OK;
 }
 
-static void
-align_resource(struct acpi_device *bridge, struct resource *res)
-{
-	int align = (res->flags & IORESOURCE_MEM) ? 16 : 4;
-
-	/*
-	 * Host bridge windows are not BARs, but the decoders on the PCI side
-	 * that claim this address space have starting alignment and length
-	 * constraints, so fix any obvious BIOS goofs.
-	 */
-	if (!IS_ALIGNED(res->start, align)) {
-		dev_printk(KERN_DEBUG, &bridge->dev,
-			   "host bridge window %pR invalid; "
-			   "aligning start to %d-byte boundary\n", res, align);
-		res->start &= ~(align - 1);
-	}
-	if (!IS_ALIGNED(res->end + 1, align)) {
-		dev_printk(KERN_DEBUG, &bridge->dev,
-			   "host bridge window %pR invalid; "
-			   "aligning end to %d-byte boundary\n", res, align);
-		res->end = ALIGN(res->end, align) - 1;
-	}
-}
-
 static acpi_status
 setup_resource(struct acpi_resource *acpi_res, void *data)
 {
@@ -154,7 +130,7 @@ setup_resource(struct acpi_resource *acpi_res, void *data)
 	acpi_status status;
 	unsigned long flags;
 	struct resource *root, *conflict;
-	u64 start, end, max_len;
+	u64 start, end;
 
 	status = resource_to_addr(acpi_res, &addr);
 	if (!ACPI_SUCCESS(status))
@@ -171,19 +147,8 @@ setup_resource(struct acpi_resource *acpi_res, void *data)
 	} else
 		return AE_OK;
 
-	max_len = addr.maximum - addr.minimum + 1;
-	if (addr.address_length > max_len) {
-		dev_printk(KERN_DEBUG, &info->bridge->dev,
-			   "host bridge window length %#llx doesn't fit in "
-			   "%#llx-%#llx, trimming\n",
-			   (unsigned long long) addr.address_length,
-			   (unsigned long long) addr.minimum,
-			   (unsigned long long) addr.maximum);
-		addr.address_length = max_len;
-	}
-
 	start = addr.minimum + addr.translation_offset;
-	end = start + addr.address_length - 1;
+	end = addr.maximum + addr.translation_offset;
 
 	res = &info->res[info->res_num];
 	res->name = info->name;
@@ -191,7 +156,6 @@ setup_resource(struct acpi_resource *acpi_res, void *data)
 	res->start = start;
 	res->end = end;
 	res->child = NULL;
-	align_resource(info->bridge, res);
 
 	if (!pci_use_crs) {
 		dev_printk(KERN_DEBUG, &info->bridge->dev,

Re: [PATCH 0/2] PNPACPI, x86/PCI: handle _CRS windows like Windows does

[Len Brown]

applied both to acpi-test tree.

you have pretty convincing evidence in the bug report that this is 
"corrrect" -- where "correct" = matches common industry practice.

I wonder if this should wait for .35 or not...

thanks,
Len Brown, Intel Open Source Technology Center

Re: [PATCH 0/2] PNPACPI, x86/PCI: handle _CRS windows like Windows does

[Bjorn Helgaas]

On Wednesday 28 April 2010 07:48:42 pm Len Brown wrote:
> applied both to acpi-test tree.
> 
> you have pretty convincing evidence in the bug report that this is 
> "corrrect" -- where "correct" = matches common industry practice.
> 
> I wonder if this should wait for .35 or not...

I chatted with Jesse about this yesterday, and we plan to put the
x86/PCI part in .34.  Actually, it looks like Linus has already
pulled it.

If you wanted to put the PNPACPI one in for .34, I wouldn't complain,
I guess, but I don't think we need it because I'm pretty sure nobody
actually relies on it.  You'd be able to see the difference in
/sys/bus/pnp/devices/*/resources, but as far as I know, that's the only
place we'd notice.   

Bjorn