[patch 0/4] x86: Fix the ACPI resource handling and range checking

[patch 0/4] x86: Fix the ACPI resource handling and range checking

[Thomas Gleixner]

Yinghai noticed that Jiangs changes broke the resource range checks. I
picked up his patch and addressed a few other things:

   - Ignoring the IORESOURCE_DISABLED flag
   - Range checking of IO resources against iomem_resource

There is more cleanup work to do in that area, but thats for later.

Please review and test ride it, so we can get x86/apic out of blocked
state.

Thanks,

	tglx
---
 arch/x86/pci/acpi.c   |  111 +++++++++++++++++++++++++++-----------------------
 drivers/acpi/ioapic.c |   38 +++++++++--------
 2 files changed, 83 insertions(+), 66 deletions(-)

[patch 1/4] x86, pci, acpi: Redo sanity checks for root brigde probing

[Thomas Gleixner]

[-- Attachment #1: fix_x86_pci_acpi.patch --]
[-- Type: text/plain, Size: 4745 bytes --]

From: Yinghai Lu <yinghai@kernel.org>

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 <yinghai@kernel.org>
Link: http://lkml.kernel.org/r/CAE9FiQWaL1Qu1eP-fQV784ucy+r-65AZ95VyiUrefn0dNtfLDw@mail.gmail.com
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

---
 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;
 	} 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;
+
 		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);
 	}
 
+	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;
 }
 

Re: [patch 1/4] x86, pci, acpi: Redo sanity checks for root brigde probing

[Borislav Petkov]

On Thu, Dec 11, 2014 at 07:48:18PM -0000, Thomas Gleixner wrote:
> From: Yinghai Lu <yinghai@kernel.org>
> 
> 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 <yinghai@kernel.org>
> Link: http://lkml.kernel.org/r/CAE9FiQWaL1Qu1eP-fQV784ucy+r-65AZ95VyiUrefn0dNtfLDw@mail.gmail.com
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> 
> ---
>  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.
--

[patch 2/4] x86: pci: acpi: Respect ioresource flags

[Thomas Gleixner]

[-- Attachment #1: x86-pci-respect-ioresource-flags.patch --]
[-- Type: text/plain, Size: 2756 bytes --]

setup_res() blindly clear all resource->flags except IO and MEM. So if
a resource is marked disabled by the acpi code, setup_res() will use
it nevertheless.

Preserve the flags and add proper checks to setup_res() and
__release_pci_root_info(). The latter is simplified while at it.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/pci/acpi.c |   35 ++++++++++++++++-------------------
 1 file changed, 16 insertions(+), 19 deletions(-)

Index: tip/arch/x86/pci/acpi.c
===================================================================
--- tip.orig/arch/x86/pci/acpi.c
+++ tip/arch/x86/pci/acpi.c
@@ -218,6 +218,12 @@ static void teardown_mcfg_map(struct pci
 }
 #endif
 
+static bool is_valid_resource(struct resource *res)
+{
+	return (res->flags & (IORESOURCE_MEM | IORESOURCE_IO)) &&
+		!(res->flags & IORESOURCE_DISABLED);
+}
+
 static acpi_status count_resource(struct acpi_resource *acpi_res, void *data)
 {
 	struct pci_root_info *info = data;
@@ -228,7 +234,7 @@ static acpi_status count_resource(struct
 	    !acpi_dev_resource_address_space(acpi_res, &r))
 		return AE_OK;
 
-	if ((r.flags & (IORESOURCE_IO | IORESOURCE_MEM)) && resource_size(&r))
+	if (is_valid_resource(&r))
 		info->res_num++;
 
 	return AE_OK;
@@ -242,13 +248,10 @@ static acpi_status setup_resource(struct
 	u64 orig_end;
 
 	memset(&r, 0, sizeof(r));
-	if (acpi_dev_resource_memory(acpi_res, &r)) {
-		r.flags &= IORESOURCE_MEM;
-	} else if (acpi_dev_resource_address_space(acpi_res, &r)) {
+	if (!acpi_dev_resource_memory(acpi_res, &r)) {
 		struct acpi_resource_address64 addr;
 
-		r.flags &= IORESOURCE_MEM | IORESOURCE_IO;
-		if (r.flags == 0)
+		if (!acpi_dev_resource_address_space(acpi_res, &r))
 			return AE_OK;
 
 		if (ACPI_FAILURE(acpi_resource_to_address64(acpi_res, &addr)))
@@ -264,10 +267,11 @@ static acpi_status setup_resource(struct
 		translation_offset = addr.translation_offset;
 		r.start += translation_offset;
 		r.end += translation_offset;
-	} else {
-		return AE_OK;
 	}
 
+	if (!is_valid_resource(&r))
+		return AE_OK;
+
 	/* Exclude non-addressable range or non-addressable portion of range */
 	orig_end = r.end;
 	r.end = min(r.end, iomem_resource.end);
@@ -367,19 +371,12 @@ static void free_pci_root_info_res(struc
 
 static void __release_pci_root_info(struct pci_root_info *info)
 {
+	struct resource *res = info->res;
 	int i;
-	struct resource *res;
-
-	for (i = 0; i < info->res_num; i++) {
-		res = &info->res[i];
-
-		if (!res->parent)
-			continue;
-
-		if (!(res->flags & (IORESOURCE_MEM | IORESOURCE_IO)))
-			continue;
 
-		release_resource(res);
+	for (i = 0; i < info->res_num; i++, res++) {
+		if (res->parent && is_valid_resource(res))
+			release_resource(res);
 	}
 
 	free_pci_root_info_res(info);

Re: [patch 2/4] x86: pci: acpi: Respect ioresource flags

[Borislav Petkov]

On Thu, Dec 11, 2014 at 07:48:20PM -0000, Thomas Gleixner wrote:
> setup_res() blindly clear all resource->flags except IO and MEM. So if
> a resource is marked disabled by the acpi code, setup_res() will use
> it nevertheless.
> 
> Preserve the flags and add proper checks to setup_res() and
> __release_pci_root_info(). The latter is simplified while at it.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  arch/x86/pci/acpi.c |   35 ++++++++++++++++-------------------
>  1 file changed, 16 insertions(+), 19 deletions(-)
> 
> Index: tip/arch/x86/pci/acpi.c
> ===================================================================
> --- tip.orig/arch/x86/pci/acpi.c
> +++ tip/arch/x86/pci/acpi.c
> @@ -218,6 +218,12 @@ static void teardown_mcfg_map(struct pci
>  }
>  #endif
>  
> +static bool is_valid_resource(struct resource *res)
> +{
> +	return (res->flags & (IORESOURCE_MEM | IORESOURCE_IO)) &&
> +		!(res->flags & IORESOURCE_DISABLED);
> +}
> +
>  static acpi_status count_resource(struct acpi_resource *acpi_res, void *data)
>  {
>  	struct pci_root_info *info = data;
> @@ -228,7 +234,7 @@ static acpi_status count_resource(struct
>  	    !acpi_dev_resource_address_space(acpi_res, &r))
>  		return AE_OK;
>  
> -	if ((r.flags & (IORESOURCE_IO | IORESOURCE_MEM)) && resource_size(&r))
> +	if (is_valid_resource(&r))
>  		info->res_num++;
>  
>  	return AE_OK;

Ok, this makes definitely more sense in getting rid of the
*clearing* of the flags which is intuitively backwards to what it should
do.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

[patch 3/4] x86: pci: acpi: Fix the range check for IO resources

[Thomas Gleixner]

[-- Attachment #1: x86-pci-check-proper-range.patch --]
[-- Type: text/plain, Size: 2874 bytes --]

The range check in setup_res() checks the IO range against
iomem_resource. That's just wrong.

Move the check into a separate function and use ioport_resource for IO
ranges.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/pci/acpi.c |   51 +++++++++++++++++++++++++++++++++------------------
 1 file changed, 33 insertions(+), 18 deletions(-)

Index: tip/arch/x86/pci/acpi.c
===================================================================
--- tip.orig/arch/x86/pci/acpi.c
+++ tip/arch/x86/pci/acpi.c
@@ -240,12 +240,41 @@ static acpi_status count_resource(struct
 	return AE_OK;
 }
 
+static bool valid_resource_range(struct resource *r, struct device *dev)
+{
+	struct resource *ior;
+	u64 end;
+
+	ior = (r->flags & IORESOURCE_IO) ? &ioport_resource : &iomem_resource;
+
+	/*
+	 * Exclude non-addressable range or non-addressable portion of
+	 * range. We only check end because ior->start is always 0.
+	 */
+	end = r->end;
+	r->end = min(r->end, ior->end);
+	if (r->end <= r->start) {
+		dev_info(dev, "host bridge %s window [%#llx-%#llx] invalid range\n",
+			 r->flags & IORESOURCE_IO ? "io" : "mem",
+			 (unsigned long long)r->start, end);
+		return false;
+	}
+
+	if (r->end == end)
+		return true;
+
+	dev_info(dev, "host bridge %s window [%#llx-%#llx] truncated to [%#llx-%#llx]\n",
+		 r->flags & IORESOURCE_IO ? "io" : "mem",
+		 (unsigned long long)r->start, end,
+		 (unsigned long long)r->start, (unsigned long long)r->end);
+	return true;
+}
+
 static acpi_status setup_resource(struct acpi_resource *acpi_res, void *data)
 {
 	struct pci_root_info *info = data;
 	u64 translation_offset = 0;
 	struct resource r;
-	u64 orig_end;
 
 	memset(&r, 0, sizeof(r));
 	if (!acpi_dev_resource_memory(acpi_res, &r)) {
@@ -269,31 +298,17 @@ static acpi_status setup_resource(struct
 		r.end += translation_offset;
 	}
 
-	if (!is_valid_resource(&r))
+	if (!is_valid_resource(&r) ||
+	    !valid_resource_range(&r, &info->bridge->dev))
 		return AE_OK;
 
-	/* 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);
-	}
-
 	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);
+			"host bridge window %pR ignored\n", &r);
 
 	return AE_OK;
 }

Re: [patch 3/4] x86: pci: acpi: Fix the range check for IO resources

[Borislav Petkov]

On Thu, Dec 11, 2014 at 07:48:22PM -0000, Thomas Gleixner wrote:
> The range check in setup_res() checks the IO range against
> iomem_resource. That's just wrong.
> 
> Move the check into a separate function and use ioport_resource for IO
> ranges.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  arch/x86/pci/acpi.c |   51 +++++++++++++++++++++++++++++++++------------------
>  1 file changed, 33 insertions(+), 18 deletions(-)
> 
> Index: tip/arch/x86/pci/acpi.c
> ===================================================================
> --- tip.orig/arch/x86/pci/acpi.c
> +++ tip/arch/x86/pci/acpi.c
> @@ -240,12 +240,41 @@ static acpi_status count_resource(struct
>  	return AE_OK;
>  }
>  
> +static bool valid_resource_range(struct resource *r, struct device *dev)
> +{
> +	struct resource *ior;
> +	u64 end;
> +
> +	ior = (r->flags & IORESOURCE_IO) ? &ioport_resource : &iomem_resource;
> +
> +	/*
> +	 * Exclude non-addressable range or non-addressable portion of
> +	 * range. We only check end because ior->start is always 0.
> +	 */
> +	end = r->end;
> +	r->end = min(r->end, ior->end);
> +	if (r->end <= r->start) {
> +		dev_info(dev, "host bridge %s window [%#llx-%#llx] invalid range\n",
> +			 r->flags & IORESOURCE_IO ? "io" : "mem",

I think you can simplify this and below even further as the check
Iagainst ORESOURCE_IO is done already when selecting the ior thing (diff
Iontop):

---
Index: b/arch/x86/pci/acpi.c
===================================================================
--- a/arch/x86/pci/acpi.c	2014-12-12 15:54:40.856317113 +0100
+++ b/arch/x86/pci/acpi.c	2014-12-12 15:53:16.832319561 +0100
@@ -255,7 +255,7 @@ static bool valid_resource_range(struct
 	r->end = min(r->end, ior->end);
 	if (r->end <= r->start) {
 		dev_info(dev, "host bridge %s window [%#llx-%#llx] invalid range\n",
-			 r->flags & IORESOURCE_IO ? "io" : "mem",
+			 ior->name,
 			 (unsigned long long)r->start, end);
 		return false;
 	}
@@ -264,7 +264,7 @@ static bool valid_resource_range(struct
 		return true;
 
 	dev_info(dev, "host bridge %s window [%#llx-%#llx] truncated to [%#llx-%#llx]\n",
-		 r->flags & IORESOURCE_IO ? "io" : "mem",
+		 ior->name,
 		 (unsigned long long)r->start, end,
 		 (unsigned long long)r->start, (unsigned long long)r->end);
 	return true;

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

[patch 4/4] acpi: ioapic: Respect the resource flags

[Thomas Gleixner]

[-- Attachment #1: acpi-ioapoc-respect-resource-flags.patch --]
[-- Type: text/plain, Size: 2387 bytes --]

The acpi parser can set the DISABLED flag on a resource. In
setup_res() we clear all flags except MEM, so we ignore the DISABLED
flag. Add proper checks and preserve the flags.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/acpi/ioapic.c |   38 +++++++++++++++++++++-----------------
 1 file changed, 21 insertions(+), 17 deletions(-)

Index: tip/drivers/acpi/ioapic.c
===================================================================
--- tip.orig/drivers/acpi/ioapic.c
+++ tip/drivers/acpi/ioapic.c
@@ -40,29 +40,33 @@ struct acpi_pci_ioapic {
 static LIST_HEAD(ioapic_list);
 static DEFINE_MUTEX(ioapic_list_lock);
 
+static inline bool is_valid_mem_resource(struct resource *res)
+{
+	return !(res->flags & IORESOURCE_DISABLED) &&
+		(res->flags & IORESOURCE_MEM);
+}
+
 static acpi_status setup_res(struct acpi_resource *acpi_res, void *data)
 {
+	struct acpi_resource_address64 addr;
 	struct resource *res = data;
 
 	memset(res, 0, sizeof(*res));
-	if (acpi_dev_resource_memory(acpi_res, res)) {
-		res->flags &= IORESOURCE_MEM;
-		if (res->flags)
-			return AE_OK;
-	} else if (acpi_dev_resource_address_space(acpi_res, res)) {
-		struct acpi_resource_address64 addr;
+	if (acpi_dev_resource_memory(acpi_res, res))
+		return AE_OK;
 
-		res->flags &= IORESOURCE_MEM;
-		if (res->flags &&
-		    ACPI_SUCCESS(acpi_resource_to_address64(acpi_res, &addr)) &&
-		    addr.info.mem.caching != ACPI_PREFETCHABLE_MEMORY) {
-			res->start += addr.translation_offset;
-			res->end += addr.translation_offset;
-			return AE_OK;
-		}
+	if (!acpi_dev_resource_address_space(acpi_res, res) ||
+	    !is_valid_mem_resource(res))
+		return AE_OK;
+	/*
+	 * FIXME: This lacks a proper comment, why the resource
+	 * address needs to be translated.
+	 */
+	if (ACPI_SUCCESS(acpi_resource_to_address64(acpi_res, &addr)) &&
+	    addr.info.mem.caching != ACPI_PREFETCHABLE_MEMORY) {
+		res->start += addr.translation_offset;
+		res->end += addr.translation_offset;
 	}
-	res->flags = 0;
-
 	return AE_OK;
 }
 
@@ -150,7 +154,7 @@ static acpi_status handle_ioapic_add(acp
 
 		res = &ioapic->res;
 		acpi_walk_resources(handle, METHOD_NAME__CRS, setup_res, res);
-		if (res->flags == IORESOURCE_UNSET) {
+		if (!is_valid_mem_resource(res)) {
 			acpi_handle_warn(handle, "failed to get resource\n");
 			goto exit_free;
 		} else if (request_resource(&iomem_resource, res)) {

Re: [patch 4/4] acpi: ioapic: Respect the resource flags

[Yinghai Lu]

[-- Attachment #1: Type: text/plain, Size: 3720 bytes --]

On Thu, Dec 11, 2014 at 11:48 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> The acpi parser can set the DISABLED flag on a resource. In
> setup_res() we clear all flags except MEM, so we ignore the DISABLED
> flag. Add proper checks and preserve the flags.
>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  drivers/acpi/ioapic.c |   38 +++++++++++++++++++++-----------------
>  1 file changed, 21 insertions(+), 17 deletions(-)
>
> Index: tip/drivers/acpi/ioapic.c
> ===================================================================
> --- tip.orig/drivers/acpi/ioapic.c
> +++ tip/drivers/acpi/ioapic.c
> @@ -40,29 +40,33 @@ struct acpi_pci_ioapic {
>  static LIST_HEAD(ioapic_list);
>  static DEFINE_MUTEX(ioapic_list_lock);
>
> +static inline bool is_valid_mem_resource(struct resource *res)
> +{
> +       return !(res->flags & IORESOURCE_DISABLED) &&
> +               (res->flags & IORESOURCE_MEM);
> +}
> +
>  static acpi_status setup_res(struct acpi_resource *acpi_res, void *data)
>  {
> +       struct acpi_resource_address64 addr;
>         struct resource *res = data;
>
>         memset(res, 0, sizeof(*res));
> -       if (acpi_dev_resource_memory(acpi_res, res)) {
> -               res->flags &= IORESOURCE_MEM;
> -               if (res->flags)
> -                       return AE_OK;
> -       } else if (acpi_dev_resource_address_space(acpi_res, res)) {
> -               struct acpi_resource_address64 addr;
> +       if (acpi_dev_resource_memory(acpi_res, res))
> +               return AE_OK;
>
> -               res->flags &= IORESOURCE_MEM;
> -               if (res->flags &&
> -                   ACPI_SUCCESS(acpi_resource_to_address64(acpi_res, &addr)) &&
> -                   addr.info.mem.caching != ACPI_PREFETCHABLE_MEMORY) {
> -                       res->start += addr.translation_offset;
> -                       res->end += addr.translation_offset;
> -                       return AE_OK;
> -               }
> +       if (!acpi_dev_resource_address_space(acpi_res, res) ||
> +           !is_valid_mem_resource(res))
> +               return AE_OK;
> +       /*
> +        * FIXME: This lacks a proper comment, why the resource
> +        * address needs to be translated.
> +        */
> +       if (ACPI_SUCCESS(acpi_resource_to_address64(acpi_res, &addr)) &&
> +           addr.info.mem.caching != ACPI_PREFETCHABLE_MEMORY) {
> +               res->start += addr.translation_offset;
> +               res->end += addr.translation_offset;
>         }
> -       res->flags = 0;
> -
>         return AE_OK;
>  }
>
> @@ -150,7 +154,7 @@ static acpi_status handle_ioapic_add(acp
>
>                 res = &ioapic->res;
>                 acpi_walk_resources(handle, METHOD_NAME__CRS, setup_res, res);
> -               if (res->flags == IORESOURCE_UNSET) {
> +               if (!is_valid_mem_resource(res)) {
>                         acpi_handle_warn(handle, "failed to get resource\n");
>                         goto exit_free;
>                 } else if (request_resource(&iomem_resource, res)) {
>
>

There is minor problem about mem pref handling, original code will ignore them.

with this patch will let it follow through.

should change is_valid_mem_resource to is_valid_mem_nonpref_resource()...

+static inline bool is_valid_mem_nonpref_resource(struct resource *res)
 {
        return !(res->flags & IORESOURCE_DISABLED) &&
-               (res->flags & IORESOURCE_MEM);
+               (res->flags & IORESOURCE_MEM) &&
+               !(res->flags & IORESOURCE_PREFETCH);
 }


other than that for whole patch set:
Acked-by: Yinghai Lu <yinghai@kernel.org>

Also please check attached following patch that will move code to acpi core.

Thanks

Yinghai

[-- Attachment #2: x86_pci_acpi_5.patch --]
[-- Type: text/x-patch, Size: 7448 bytes --]

Subject: [PATCH] x86, PCI, ACPI: offset resource in acpi core

Also pass back via offset for pci root bus resource probing.

and set IORESOURCE_PREFETCH...

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 arch/x86/pci/acpi.c            |   29 ++++++-----------------------
 drivers/acpi/ioapic.c          |   22 ++++++----------------
 drivers/acpi/resource.c        |   16 +++++++++++-----
 drivers/pnp/pnpacpi/rsparser.c |    2 +-
 include/linux/acpi.h           |    2 +-
 5 files changed, 25 insertions(+), 46 deletions(-)

Index: linux-2.6/arch/x86/pci/acpi.c
===================================================================
--- linux-2.6.orig/arch/x86/pci/acpi.c
+++ linux-2.6/arch/x86/pci/acpi.c
@@ -231,7 +231,7 @@ static acpi_status count_resource(struct
 
 	memset(&r, 0, sizeof(r));
 	if (!acpi_dev_resource_memory(acpi_res, &r) &&
-	    !acpi_dev_resource_address_space(acpi_res, &r))
+	    !acpi_dev_resource_address_space(acpi_res, &r, NULL))
 		return AE_OK;
 
 	if (is_valid_resource(&r))
@@ -273,30 +273,13 @@ static bool valid_resource_range(struct
 static acpi_status setup_resource(struct acpi_resource *acpi_res, void *data)
 {
 	struct pci_root_info *info = data;
-	u64 translation_offset = 0;
+	u64 offset = 0;
 	struct resource r;
 
 	memset(&r, 0, sizeof(r));
-	if (!acpi_dev_resource_memory(acpi_res, &r)) {
-		struct acpi_resource_address64 addr;
-
-		if (!acpi_dev_resource_address_space(acpi_res, &r))
-			return AE_OK;
-
-		if (ACPI_FAILURE(acpi_resource_to_address64(acpi_res, &addr)))
-			return AE_OK;
-
-		if (!addr.address_length)
-			return AE_OK;
-
-		if (addr.resource_type == ACPI_MEMORY_RANGE &&
-		    addr.info.mem.caching == ACPI_PREFETCHABLE_MEMORY)
-			r.flags |= IORESOURCE_PREFETCH;
-
-		translation_offset = addr.translation_offset;
-		r.start += translation_offset;
-		r.end += translation_offset;
-	}
+	if (!acpi_dev_resource_memory(acpi_res, &r) &&
+	    !acpi_dev_resource_address_space(acpi_res, &r, &offset))
+		return AE_OK;
 
 	if (!is_valid_resource(&r) ||
 	    !valid_resource_range(&r, &info->bridge->dev))
@@ -304,7 +287,7 @@ static acpi_status setup_resource(struct
 
 	r.name = info->name;
 	info->res[info->res_num] = r;
-	info->res_offset[info->res_num] = translation_offset;
+	info->res_offset[info->res_num] = offset;
 	info->res_num++;
 	if (!pci_use_crs)
 		dev_printk(KERN_DEBUG, &info->bridge->dev,
Index: linux-2.6/drivers/acpi/ioapic.c
===================================================================
--- linux-2.6.orig/drivers/acpi/ioapic.c
+++ linux-2.6/drivers/acpi/ioapic.c
@@ -40,33 +40,23 @@ struct acpi_pci_ioapic {
 static LIST_HEAD(ioapic_list);
 static DEFINE_MUTEX(ioapic_list_lock);
 
-static inline bool is_valid_mem_resource(struct resource *res)
+static inline bool is_valid_mem_nonpref_resource(struct resource *res)
 {
 	return !(res->flags & IORESOURCE_DISABLED) &&
-		(res->flags & IORESOURCE_MEM);
+		(res->flags & IORESOURCE_MEM) &&
+		!(res->flags & IORESOURCE_PREFETCH);
 }
 
 static acpi_status setup_res(struct acpi_resource *acpi_res, void *data)
 {
-	struct acpi_resource_address64 addr;
 	struct resource *res = data;
 
 	memset(res, 0, sizeof(*res));
 	if (acpi_dev_resource_memory(acpi_res, res))
 		return AE_OK;
 
-	if (!acpi_dev_resource_address_space(acpi_res, res) ||
-	    !is_valid_mem_resource(res))
-		return AE_OK;
-	/*
-	 * FIXME: This lacks a proper comment, why the resource
-	 * address needs to be translated.
-	 */
-	if (ACPI_SUCCESS(acpi_resource_to_address64(acpi_res, &addr)) &&
-	    addr.info.mem.caching != ACPI_PREFETCHABLE_MEMORY) {
-		res->start += addr.translation_offset;
-		res->end += addr.translation_offset;
-	}
+	acpi_dev_resource_address_space(acpi_res, res, NULL);
+
 	return AE_OK;
 }
 
@@ -154,7 +144,7 @@ static acpi_status handle_ioapic_add(acp
 
 		res = &ioapic->res;
 		acpi_walk_resources(handle, METHOD_NAME__CRS, setup_res, res);
-		if (!is_valid_mem_resource(res)) {
+		if (!is_valid_mem_nonpref_resource(res)) {
 			acpi_handle_warn(handle, "failed to get resource\n");
 			goto exit_free;
 		} else if (request_resource(&iomem_resource, res)) {
Index: linux-2.6/drivers/acpi/resource.c
===================================================================
--- linux-2.6.orig/drivers/acpi/resource.c
+++ linux-2.6/drivers/acpi/resource.c
@@ -175,13 +175,14 @@ EXPORT_SYMBOL_GPL(acpi_dev_resource_io);
  * acpi_dev_resource_address_space - Extract ACPI address space information.
  * @ares: Input ACPI resource object.
  * @res: Output generic resource object.
+ * @offset: Output for translation_offset.
  *
  * Check if the given ACPI resource object represents an address space resource
  * and if that's the case, use the information in it to populate the generic
  * resource object pointed to by @res.
  */
 bool acpi_dev_resource_address_space(struct acpi_resource *ares,
-				     struct resource *res)
+				     struct resource *res, u64 *offset)
 {
 	acpi_status status;
 	struct acpi_resource_address64 addr;
@@ -199,12 +200,14 @@ bool acpi_dev_resource_address_space(str
 	}
 
 	status = acpi_resource_to_address64(ares, &addr);
-	if (ACPI_FAILURE(status))
+	if (ACPI_FAILURE(status) || !addr.address_length)
 		return false;
 
-	res->start = addr.minimum;
-	res->end = addr.maximum;
+	res->start = addr.minimum + addr.translation_offset;
+	res->end = addr.maximum + addr.translation_offset;
 	window = addr.producer_consumer == ACPI_PRODUCER;
+	if (offset)
+		*offset = addr.translation_offset;
 
 	switch(addr.resource_type) {
 	case ACPI_MEMORY_RANGE:
@@ -212,6 +215,9 @@ bool acpi_dev_resource_address_space(str
 		res->flags = acpi_dev_memresource_flags(len,
 						addr.info.mem.write_protect,
 						window);
+		if (addr.info.mem.caching == ACPI_PREFETCHABLE_MEMORY)
+			res->flags |= IORESOURCE_PREFETCH;
+
 		break;
 	case ACPI_IO_RANGE:
 		io_decode = addr.granularity == 0xfff ?
@@ -471,7 +477,7 @@ static acpi_status acpi_dev_process_reso
 
 	if (acpi_dev_resource_memory(ares, &r)
 	    || acpi_dev_resource_io(ares, &r)
-	    || acpi_dev_resource_address_space(ares, &r)
+	    || acpi_dev_resource_address_space(ares, &r, NULL)
 	    || acpi_dev_resource_ext_address_space(ares, &r))
 		return acpi_dev_new_resource_entry(&r, c);
 
Index: linux-2.6/drivers/pnp/pnpacpi/rsparser.c
===================================================================
--- linux-2.6.orig/drivers/pnp/pnpacpi/rsparser.c
+++ linux-2.6/drivers/pnp/pnpacpi/rsparser.c
@@ -183,7 +183,7 @@ static acpi_status pnpacpi_allocated_res
 	struct resource r = {0};
 	int i, flags;
 
-	if (acpi_dev_resource_address_space(res, &r)
+	if (acpi_dev_resource_address_space(res, &r, NULL)
 	    || acpi_dev_resource_ext_address_space(res, &r)) {
 		pnp_add_resource(dev, &r);
 		return AE_OK;
Index: linux-2.6/include/linux/acpi.h
===================================================================
--- linux-2.6.orig/include/linux/acpi.h
+++ linux-2.6/include/linux/acpi.h
@@ -292,7 +292,7 @@ extern int pnpacpi_disabled;
 bool acpi_dev_resource_memory(struct acpi_resource *ares, struct resource *res);
 bool acpi_dev_resource_io(struct acpi_resource *ares, struct resource *res);
 bool acpi_dev_resource_address_space(struct acpi_resource *ares,
-				     struct resource *res);
+				     struct resource *res, u64 *offset);
 bool acpi_dev_resource_ext_address_space(struct acpi_resource *ares,
 					 struct resource *res);
 unsigned long acpi_dev_irq_flags(u8 triggering, u8 polarity, u8 shareable);

Re: [patch 4/4] acpi: ioapic: Respect the resource flags

[Thomas Gleixner]

On Thu, 11 Dec 2014, Yinghai Lu wrote:
> On Thu, Dec 11, 2014 at 11:48 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > +static inline bool is_valid_mem_resource(struct resource *res)
> > +{
> > +       return !(res->flags & IORESOURCE_DISABLED) &&
> > +               (res->flags & IORESOURCE_MEM);
> > +}
> > +
> There is minor problem about mem pref handling, original code will ignore them.

Bah. I missed that in that well documented function...

> with this patch will let it follow through.
> 
> should change is_valid_mem_resource to is_valid_mem_nonpref_resource()...
> 
> +static inline bool is_valid_mem_nonpref_resource(struct resource *res)
>  {
>         return !(res->flags & IORESOURCE_DISABLED) &&
> -               (res->flags & IORESOURCE_MEM);
> +               (res->flags & IORESOURCE_MEM) &&
> +               !(res->flags & IORESOURCE_PREFETCH);
>  }

Unfortunately that does not help, because nothing sets the
IORESOURCE_PREFETCH flag. Will fix it proper.

I still have no explanation why the translation offset needs to be
applied here.

Thanks,

	tglx

Re: [patch 4/4] acpi: ioapic: Respect the resource flags

[Yinghai Lu]

On Thu, Dec 11, 2014 at 11:53 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
>
> I still have no explanation why the translation offset needs to be
> applied here.

That should be copied from pci root bus resource setup.

But looks like only ia64 use those translation_offset?

Thanks

Yinghai

Re: [patch 4/4] acpi: ioapic: Respect the resource flags

[Jiang Liu]

On 2014/12/12 15:53, Thomas Gleixner wrote:
> On Thu, 11 Dec 2014, Yinghai Lu wrote:
>> On Thu, Dec 11, 2014 at 11:48 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
>>> +static inline bool is_valid_mem_resource(struct resource *res)
>>> +{
>>> +       return !(res->flags & IORESOURCE_DISABLED) &&
>>> +               (res->flags & IORESOURCE_MEM);
>>> +}
>>> +
>> There is minor problem about mem pref handling, original code will ignore them.
> 
> Bah. I missed that in that well documented function...
> 
>> with this patch will let it follow through.
>>
>> should change is_valid_mem_resource to is_valid_mem_nonpref_resource()...
>>
>> +static inline bool is_valid_mem_nonpref_resource(struct resource *res)
>>  {
>>         return !(res->flags & IORESOURCE_DISABLED) &&
>> -               (res->flags & IORESOURCE_MEM);
>> +               (res->flags & IORESOURCE_MEM) &&
>> +               !(res->flags & IORESOURCE_PREFETCH);
>>  }
> 
> Unfortunately that does not help, because nothing sets the
> IORESOURCE_PREFETCH flag. Will fix it proper.
> 
> I still have no explanation why the translation offset needs to be
> applied here.
Hi Thomas,
	I have read related section in ACPI spec, seems the addition
of translation_offset is redundant here.

Quotation from ACPI spec 5a, 6.4.3.5.1
For bridges that translate addresses across the bridge, this is the
offset that must be added to the address on the secondary side to
obtain the address on the primary side. Non-bridge devices must list
0 for all Address Translation offset bits.

Quotation from ACPI spec 5, 9.17 I/O APIC Device:
It must also contain a _CRS object that reports the base address of the
I/O APIC device. The _CRS object is required to contain only one
resource, a memory resource pointing to the I/O APIC register base.

IO APIC is not a bridge, so translation_offset should always be zero.
Thanks!
Gerry

> 
> Thanks,
> 
> 	tglx
> 

Re: [patch 4/4] acpi: ioapic: Respect the resource flags

[Thomas Gleixner]

On Fri, 12 Dec 2014, Jiang Liu wrote:
> On 2014/12/12 15:53, Thomas Gleixner wrote:
> > On Thu, 11 Dec 2014, Yinghai Lu wrote:
> >> On Thu, Dec 11, 2014 at 11:48 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> >>> +static inline bool is_valid_mem_resource(struct resource *res)
> >>> +{
> >>> +       return !(res->flags & IORESOURCE_DISABLED) &&
> >>> +               (res->flags & IORESOURCE_MEM);
> >>> +}
> >>> +
> >> There is minor problem about mem pref handling, original code will ignore them.
> > 
> > Bah. I missed that in that well documented function...
> > 
> >> with this patch will let it follow through.
> >>
> >> should change is_valid_mem_resource to is_valid_mem_nonpref_resource()...
> >>
> >> +static inline bool is_valid_mem_nonpref_resource(struct resource *res)
> >>  {
> >>         return !(res->flags & IORESOURCE_DISABLED) &&
> >> -               (res->flags & IORESOURCE_MEM);
> >> +               (res->flags & IORESOURCE_MEM) &&
> >> +               !(res->flags & IORESOURCE_PREFETCH);
> >>  }
> > 
> > Unfortunately that does not help, because nothing sets the
> > IORESOURCE_PREFETCH flag. Will fix it proper.
> > 
> > I still have no explanation why the translation offset needs to be
> > applied here.
> Hi Thomas,
> 	I have read related section in ACPI spec, seems the addition
> of translation_offset is redundant here.
> 
> Quotation from ACPI spec 5a, 6.4.3.5.1
> For bridges that translate addresses across the bridge, this is the
> offset that must be added to the address on the secondary side to
> obtain the address on the primary side. Non-bridge devices must list
> 0 for all Address Translation offset bits.
> 
> Quotation from ACPI spec 5, 9.17 I/O APIC Device:
> It must also contain a _CRS object that reports the base address of the
> I/O APIC device. The _CRS object is required to contain only one
> resource, a memory resource pointing to the I/O APIC register base.
> 
> IO APIC is not a bridge, so translation_offset should always be zero.

Right and we really need a proper interface for this on the acpi side,
so we can avoid all that dance in the usage sites.

Thanks,

	tglx

Re: [patch 4/4] acpi: ioapic: Respect the resource flags

[Jiang Liu]

Hi Thomas,
	Should I keep the development history or start from scratch
for this ACPI resource patch set?
Thanks!
Gerry

On 2014/12/12 19:43, Thomas Gleixner wrote:
> On Fri, 12 Dec 2014, Jiang Liu wrote:
>> On 2014/12/12 15:53, Thomas Gleixner wrote:
>>> On Thu, 11 Dec 2014, Yinghai Lu wrote:
>>>> On Thu, Dec 11, 2014 at 11:48 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
>>>>> +static inline bool is_valid_mem_resource(struct resource *res)
>>>>> +{
>>>>> +       return !(res->flags & IORESOURCE_DISABLED) &&
>>>>> +               (res->flags & IORESOURCE_MEM);
>>>>> +}
>>>>> +
>>>> There is minor problem about mem pref handling, original code will ignore them.
>>>
>>> Bah. I missed that in that well documented function...
>>>
>>>> with this patch will let it follow through.
>>>>
>>>> should change is_valid_mem_resource to is_valid_mem_nonpref_resource()...
>>>>
>>>> +static inline bool is_valid_mem_nonpref_resource(struct resource *res)
>>>>  {
>>>>         return !(res->flags & IORESOURCE_DISABLED) &&
>>>> -               (res->flags & IORESOURCE_MEM);
>>>> +               (res->flags & IORESOURCE_MEM) &&
>>>> +               !(res->flags & IORESOURCE_PREFETCH);
>>>>  }
>>>
>>> Unfortunately that does not help, because nothing sets the
>>> IORESOURCE_PREFETCH flag. Will fix it proper.
>>>
>>> I still have no explanation why the translation offset needs to be
>>> applied here.
>> Hi Thomas,
>> 	I have read related section in ACPI spec, seems the addition
>> of translation_offset is redundant here.
>>
>> Quotation from ACPI spec 5a, 6.4.3.5.1
>> For bridges that translate addresses across the bridge, this is the
>> offset that must be added to the address on the secondary side to
>> obtain the address on the primary side. Non-bridge devices must list
>> 0 for all Address Translation offset bits.
>>
>> Quotation from ACPI spec 5, 9.17 I/O APIC Device:
>> It must also contain a _CRS object that reports the base address of the
>> I/O APIC device. The _CRS object is required to contain only one
>> resource, a memory resource pointing to the I/O APIC register base.
>>
>> IO APIC is not a bridge, so translation_offset should always be zero.
> 
> Right and we really need a proper interface for this on the acpi side,
> so we can avoid all that dance in the usage sites.
> 
> Thanks,
> 
> 	tglx
> --
> 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/
> 

Re: [patch 4/4] acpi: ioapic: Respect the resource flags

[Thomas Gleixner]

On Wed, 17 Dec 2014, Jiang Liu wrote:

> Hi Thomas,
> 	Should I keep the development history or start from scratch
> for this ACPI resource patch set?

We better start from scratch.

Re: [patch 4/4] acpi: ioapic: Respect the resource flags

[Thomas Gleixner]

On Thu, 11 Dec 2014, Yinghai Lu wrote:
> On Thu, Dec 11, 2014 at 11:48 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> Also please check attached following patch that will move code to acpi core.

Fun. I came up with more or less the same patch (properly split into
seperate patches) yesterday morning, but decided to keep it out of the
series because it looked ugly.

We really should have something like this:

struct translated_rescource {
       struct resource 	    res;
       u64    	       	    offset;
};

and a proper acpi function for it. Adding the offset could be done
there as well. That will simplify the magic in x86/pci/acpi as you
only have to deal with a single storage place.

Thanks,

	tglx