On Thu, Dec 11, 2014 at 07:48:18PM -0000, Thomas Gleixner wrote:
> From: Yinghai Lu <[email protected]>
>
> commit e22ce93870de ("x86, PCI, ACPI: Kill private function
> resource_to_addr() in arch/x86/pci/acpi.c") dropped the sanity checks
> for resources initialized via acpi_dev_resource_memory().
>
> It further dropped a check for address_length = 0 for the resources
> which are initialized via acpi_dev_resource_address_space().
>
> For translated addresses the range check operates on the non
> translated end address. Assign orig_end after adding the translation
> offset.
>
> This causes that invalid resources are not dropped and the range check
> for translated addresses fails.
>
> [ tglx: Rewrote changelog.
>
> Added the address_length check to the x86 code and not into
> acpi_dev_resource_address_space(). While the acpi function should
> check this, we want to have a proper check which sets the
> IORESOURCE_DISABLED bit as it does for IO resources.
>
> Removed the pointless memset of addr. acpi_resource_to_address64()
> either fails or fills it completely ]
>
> Fixes: commit e22ce93870de ("x86, PCI, ACPI: Kill private function
> resource_to_addr() in arch/x86/pci/acpi.c").
> Signed-off-by: Yinghai Lu <[email protected]>
> Link:
> http://lkml.kernel.org/r/cae9fiqwal1qu1ep-fqv784ucy+r-65az95vyiurefn0dntf...@mail.gmail.com
> Signed-off-by: Thomas Gleixner <[email protected]>
>
> ---
> arch/x86/pci/acpi.c | 63
> ++++++++++++++++++++++++++--------------------------
> 1 file changed, 32 insertions(+), 31 deletions(-)
>
> Index: tip/arch/x86/pci/acpi.c
> ===================================================================
> --- tip.orig/arch/x86/pci/acpi.c
> +++ tip/arch/x86/pci/acpi.c
> @@ -221,10 +221,9 @@ static void teardown_mcfg_map(struct pci
> static acpi_status count_resource(struct acpi_resource *acpi_res, void *data)
> {
> struct pci_root_info *info = data;
> - struct resource r = {
> - .flags = 0
> - };
> + struct resource r;
>
> + memset(&r, 0, sizeof(r));
> if (!acpi_dev_resource_memory(acpi_res, &r) &&
> !acpi_dev_resource_address_space(acpi_res, &r))
> return AE_OK;
> @@ -239,14 +238,13 @@ static acpi_status setup_resource(struct
> {
> struct pci_root_info *info = data;
> u64 translation_offset = 0;
> - struct resource r = {
> - .flags = 0
> - };
> + struct resource r;
> + u64 orig_end;
>
> + memset(&r, 0, sizeof(r));
> if (acpi_dev_resource_memory(acpi_res, &r)) {
> - r.flags &= IORESOURCE_MEM | IORESOURCE_IO;
> + r.flags &= IORESOURCE_MEM;
As you pointed out on IRC, the &= is kinda ass-backwards logically but
I don't understand the big picture yet to know what happens with this
resource when it gets copied into the root a bit further down:
...
info->res[info->res_num] = r;
and what a *cleared* IORESOURCE_MEM flag for a memory resource means...
> } else if (acpi_dev_resource_address_space(acpi_res, &r)) {
> - u64 orig_end;
> struct acpi_resource_address64 addr;
>
> r.flags &= IORESOURCE_MEM | IORESOURCE_IO;
> @@ -256,40 +254,43 @@ static acpi_status setup_resource(struct
> if (ACPI_FAILURE(acpi_resource_to_address64(acpi_res, &addr)))
> return AE_OK;
>
> + if (!addr.address_length)
> + return AE_OK;
> +
We have an
if (r.flags == 0)
test above this - might change it to
if (!r.flags)
for consistency.
> if (addr.resource_type == ACPI_MEMORY_RANGE &&
> addr.info.mem.caching == ACPI_PREFETCHABLE_MEMORY)
> r.flags |= IORESOURCE_PREFETCH;
>
> translation_offset = addr.translation_offset;
> - orig_end = r.end;
> r.start += translation_offset;
> r.end += translation_offset;
> -
> - /* Exclude non-addressable range or non-addressable portion of
> range */
> - r.end = min(r.end, iomem_resource.end);
> - if (r.end <= r.start) {
> - dev_info(&info->bridge->dev,
> - "host bridge window [%#llx-%#llx] (ignored, not
> CPU addressable)\n",
> - (unsigned long long)r.start, orig_end);
> - return AE_OK;
> - } else if (orig_end != r.end) {
> - dev_info(&info->bridge->dev,
> - "host bridge window [%#llx-%#llx]
> ([%#llx-%#llx] ignored, not CPU addressable)\n",
> - (unsigned long long)r.start, orig_end,
> - (unsigned long long)r.end + 1, orig_end);
> - }
> + } else {
> + return AE_OK;
> }
>
> - if (r.flags && resource_size(&r)) {
> - r.name = info->name;
> - info->res[info->res_num] = r;
> - info->res_offset[info->res_num] = translation_offset;
> - info->res_num++;
> - if (!pci_use_crs)
> - dev_printk(KERN_DEBUG, &info->bridge->dev,
> - "host bridge window %pR (ignored)\n", &r);
> + /* Exclude non-addressable range or non-addressable portion of range */
> + orig_end = r.end;
> + r.end = min(r.end, iomem_resource.end);
> + if (r.end <= r.start) {
> + dev_info(&info->bridge->dev,
> + "host bridge window [%#llx-%#llx] (ignored, not CPU
> addressable)\n",
> + (unsigned long long)r.start, orig_end);
> + return AE_OK;
> + } else if (orig_end != r.end) {
> + dev_info(&info->bridge->dev,
> + "host bridge window [%#llx-%#llx] ([%#llx-%#llx]
> ignored, not CPU addressable)\n",
> + (unsigned long long)r.start, orig_end,
> + (unsigned long long)r.end + 1, orig_end);
This hunk could be made a little more readable (diff ontop):
---
Index: b/arch/x86/pci/acpi.c
===================================================================
--- a/arch/x86/pci/acpi.c 2014-12-12 12:25:56.036682117 +0100
+++ b/arch/x86/pci/acpi.c 2014-12-12 12:25:11.892683404 +0100
@@ -270,26 +270,29 @@ static acpi_status setup_resource(struct
/* Exclude non-addressable range or non-addressable portion of range */
orig_end = r.end;
- r.end = min(r.end, iomem_resource.end);
+ r.end = min(r.end, iomem_resource.end);
+
if (r.end <= r.start) {
dev_info(&info->bridge->dev,
"host bridge window [%#llx-%#llx] (ignored, not CPU
addressable)\n",
(unsigned long long)r.start, orig_end);
return AE_OK;
- } else if (orig_end != r.end) {
+ }
+
+ if (orig_end != r.end) {
dev_info(&info->bridge->dev,
"host bridge window [%#llx-%#llx] ([%#llx-%#llx]
ignored, not CPU addressable)\n",
(unsigned long long)r.start, orig_end,
(unsigned long long)r.end + 1, orig_end);
}
+ if (!pci_use_crs)
+ dev_printk(KERN_DEBUG, &info->bridge->dev, "host bridge window
%pR (ignored)\n", &r);
+
r.name = info->name;
info->res[info->res_num] = r;
info->res_offset[info->res_num] = translation_offset;
info->res_num++;
- if (!pci_use_crs)
- dev_printk(KERN_DEBUG, &info->bridge->dev,
- "host bridge window %pR (ignored)\n", &r);
return AE_OK;
}
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
