Add diagnostics for iomem resources

PATCH In review Peter Hurley 4 participants 16 messages Nov 8, 2012 – Dec 14, 2012

Subsystems (5)

x86 ACPI PCI Multifunction devices (MFD) +1 more subsystems

lore

Share & tools

lore

Peter Hurley

author

2012-11-08 02:56 UTC #0 lore raw

cover Add diagnostics for iomem resources

The first 3 patches add diagnostics to iomem resource management. The x86 patch adds a diagnostic for bogus ACPI resource definitions. Lastly, the issue addressed by the mfd driver patch emerged while testing the other patches in this series.

Peter Hurley (5):
kernel: Print resource ranges when expanding overlaps
kernel: Print resource conflicts for failed requests
kernel: Print warning when inserting resource [mem 0x00000000]
x86: acpi: Print warning for malformed host bridge resources
drivers: mfd: Fix resource request for [mem 0x00000000]

 arch/x86/pci/acpi.c   |  4 ++++
 drivers/mfd/lpc_ich.c |  3 +++
 kernel/resource.c     | 12 ++++++++++--
 3 files changed, 17 insertions(+), 2 deletions(-)

Signature · 1 line

1.7.12.3

Peter Hurley

author

2012-11-08 02:56 UTC #1 lore raw

PATCH 1/5 resources: Print resource ranges when expanding overlaps

Resource names can be too generic to distinguish which resources overlap; eg.,
kernel: Expanded resource reserved due to conflict with PCI Bus 0000:00
kernel: Expanded resource reserved due to conflict with PCI Bus 0000:00

Rather, print decoded resource info as well; eg.,
kernel: resource: Expanding reserved [mem 0xcfe5ec00-0xcfffffff]; overlaps PCI Bus 0000:00 [mem 0xcff00000-0xdfffffff]
kernel: resource: Expanding reserved [mem 0xfe000000-0xfeffffff]; overlaps PCI Bus 0000:00 [mem 0xf0000000-0xfe000000]

Cc Bjorn Helgaas <[email protected]>

Signed-off-by Peter Hurley <[email protected]>

---
 kernel/resource.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

kernel/resource.c +3 -2

index 73f35d4..461c2e0 100644
@@ -696,12 +696,13 @@ void insert_resource_expand_to_fit(struct resource *root, struct resource *new)
 			break;
 
 		/* Ok, expand resource to cover the conflict, then try again .. */
+		pr_warn("Expanding %s %pR; overlaps %s %pR\n",
+			new->name, new, conflict->name, conflict);
+
 		if (conflict->start < new->start)
 			new->start = conflict->start;
 		if (conflict->end > new->end)
 			new->end = conflict->end;
-
-		printk("Expanded resource %s due to conflict with %s\n", new->name, conflict->name);
 	}
 	write_unlock(&resource_lock);
 }

Signature · 1 line

1.7.12.3

Peter Hurley

author

2012-11-08 02:56 UTC #2 lore raw

PATCH 2/5 resources: Print resource conflicts for failed requests

In reply to Peter Hurley

When resource requests fail, the conflicting resource is not
always apparent. Emit diagnostic print when resource conflicts prevent
resource requests. For example,
kernel: resource: Requested i5k_amb [mem 0xfe000000-0xfe01ffff flags 0x80000000] conflicts with PCI Bus 0000:00 [mem 0xf0000000-0xfe000000 flags 0x200]

Cc Bjorn Helgaas <[email protected]>

Signed-off-by Peter Hurley <[email protected]>

---
 kernel/resource.c | 4 ++++
 1 file changed, 4 insertions(+)

kernel/resource.c +4 -0

index 461c2e0..268b5fa 100644
@@ -924,6 +924,10 @@ struct resource * __request_region(struct resource *parent,
 			write_lock(&resource_lock);
 			continue;
 		}
+
+		pr_debug("Requested %s %pr conflicts with %s %pr\n",
+			 res->name, res, conflict->name, conflict);
+
 		/* Uhhuh, that didn't work out.. */
 		kfree(res);
 		res = NULL;

Signature · 1 line

1.7.12.3

Peter Hurley

author

2012-11-08 02:56 UTC #3 lore raw

PATCH 3/5 resources: Print warning when inserting resource [mem 0x00000000]

In reply to Peter Hurley

Inserting iomem resource [0x0-0x0] is most likely a bug; at least print a warning message. Eg.,
resource: inserting NULL resource iTCO_wdt [mem 0x00000000]

Cc Bjorn Helgaas <[email protected]>

Signed-off-by Peter Hurley <[email protected]>

---
 kernel/resource.c | 3 +++
 1 file changed, 3 insertions(+)

kernel/resource.c +3 -0

index 268b5fa..ab758bb 100644
@@ -589,6 +589,9 @@ static struct resource * __insert_resource(struct resource *parent, struct resou
 {
 	struct resource *first, *next;
 
+	if (new->flags & IORESOURCE_MEM && !new->start && !new->end)
+		pr_warn("inserting NULL resource %s %pR", new->name, new);
+
 	for (;; parent = first) {
 		first = __request_resource(parent, new);
 		if (!first)

Signature · 1 line

1.7.12.3

Peter Hurley

author

2012-11-08 02:56 UTC #4 lore raw

PATCH 4/5 x86: acpi: Print warning for malformed host bridge resources

In reply to Peter Hurley

An incorrectly specified host bridge window may prevent other devices from claiming assigned resources. For example, this flawed _CRS resource descriptor from a Dell T5400:

     DWordMemory (ResourceProducer, PosDecode, MinFixed, MaxFixed, NonCacheable, ReadWrite,
     	 0x00000000,         // Granularity
         0xF0000000,         // Range Minimum
         0xFE000000,         // Range Maximum
         0x00000000,         // Translation Offset
         0x0E000000,         // Length
         ,, , AddressRangeMemory, TypeStatic)

prevents the adjacent device from claiming [mem 0xfe0000000-0xfe01ffff]

Sanity check that the resource at least conforms to a valid PCI BAR; if not, emit a diagnostic warning.

Cc Bjorn Helgaas <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc H. Peter Anvin <[email protected]>

Cc [email protected]

Signed-off-by Peter Hurley <[email protected]>

---
 arch/x86/pci/acpi.c | 4 ++++
 1 file changed, 4 insertions(+)

arch/x86/pci/acpi.c +4 -0

index 192397c..3468d16 100644
@@ -298,6 +298,10 @@ setup_resource(struct acpi_resource *acpi_res, void *data)
 			"host bridge window [%#llx-%#llx] "
 			"([%#llx-%#llx] ignored, not CPU addressable)\n", 
 			start, orig_end, end + 1, orig_end);
+	} else if (flags & IORESOURCE_MEM && (start & 0x0f || ~end & 0x0f)) {
+		dev_warn(&info->bridge->dev,
+			 "invalid host bridge window [%#llx-%#llx]\n",
+			 start, end);
 	}
 
 	res = &info->res[info->res_num];

Signature · 1 line

1.7.12.3

Bjorn Helgaas

maintainer

2012-11-10 21:52 UTC #5 lore raw

Re: [PATCH 4/5] x86: acpi: Print warning for malformed host bridge resources

Quoted 10 lines · Peter Hurley

An incorrectly specified host bridge window may prevent other devices from claiming assigned resources. For example, this flawed _CRS resource descriptor from a Dell T5400:
     DWordMemory (ResourceProducer, PosDecode, MinFixed, MaxFixed, NonCacheable, ReadWrite,
         0x00000000,         // Granularity
         0xF0000000,         // Range Minimum
         0xFE000000,         // Range Maximum
         0x00000000,         // Translation Offset
         0x0E000000,         // Length
         ,, , AddressRangeMemory, TypeStatic)

I think the problem here is that the Range Maximum should be 0xFDFFFFFF, not 0xFE000000, right?

Quoted 20 lines

prevents the adjacent device from claiming [mem 0xfe0000000-0xfe01ffff]
Sanity check that the resource at least conforms to a valid PCI BAR; if not, emit a diagnostic warning.
Cc Bjorn Helgaas <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc H. Peter Anvin <[email protected]>
Cc [email protected]
Signed-off-by Peter Hurley <[email protected]>
---
 arch/x86/pci/acpi.c | 4 ++++
 1 file changed, 4 insertions(+)
arch/x86/pci/acpi.c +0 -0
index 192397c..3468d16 100644

arch/x86/pci/acpi.c setup_resource(struct acpi_resource *acpi_res, void *data) · line 304

Quoted 7 lines

arch/x86/pci/acpi.c +4 -0

@@ -298,6 +298,10 @@ setup_resource(struct acpi_resource *acpi_res, void *data)
                        "host bridge window [%#llx-%#llx] "
                        "([%#llx-%#llx] ignored, not CPU addressable)\n",
                        start, orig_end, end + 1, orig_end);
+       } else if (flags & IORESOURCE_MEM && (start & 0x0f || ~end & 0x0f)) {
+               dev_warn(&info->bridge->dev,
+                        "invalid host bridge window [%#llx-%#llx]\n",
+                        start, end);

We didn't actually *fix* anything here, so I guess we're just pointing out the reason for a subsequent failure to claim the adjacent resource.

As far as I know, the spec doesn't actually require resources of ACPI devices to be non-overlapping. Windows accepts overlapping resources, and I think Linux probably should, too, but right now we trip over this.

In the meantime (until we figure out how to handle overlapping resources better), can we do something to actually fix this? Maybe we should truncate the end of the range to 0xFDFFFFFF like we do for non-addressable parts of the range?

Is there a bugzilla or a complete dmesg log to look at?

Bjorn

Quoted 6 lines

arch/x86/pci/acpi.c +0 -1

        }
 
        res = &info->res[info->res_num];
--

1.7.12.3

Peter Hurley

author

2012-11-11 14:49 UTC #6 lore raw

Re: [PATCH 4/5] x86: acpi: Print warning for malformed host bridge resources

Quoted 12 lines · Bjorn Helgaas

Quoted 10 lines · Peter Hurley
An incorrectly specified host bridge window may prevent other devices from claiming assigned resources. For example, this flawed _CRS resource descriptor from a Dell T5400:
     DWordMemory (ResourceProducer, PosDecode, MinFixed, MaxFixed, NonCacheable, ReadWrite,
         0x00000000,         // Granularity
         0xF0000000,         // Range Minimum
         0xFE000000,         // Range Maximum
         0x00000000,         // Translation Offset
         0x0E000000,         // Length
         ,, , AddressRangeMemory, TypeStatic)

Quoted 2 lines

I think the problem here is that the Range Maximum should be 0xFDFFFFFF, not 0xFE000000, right?

I presume so.

Quoted 8 lines

arch/x86/pci/acpi.c +0 -0

index 192397c..3468d16 100644
@@ -298,6 +298,10 @@ setup_resource(struct acpi_resource *acpi_res, void *data)
                        "host bridge window [%#llx-%#llx] "
                        "([%#llx-%#llx] ignored, not CPU addressable)\n",
                        start, orig_end, end + 1, orig_end);

Quoted 8 lines

arch/x86/pci/acpi.c setup_resource(struct acpi_resource *acpi_res, void *data) · line 304
Quoted 4 lines
arch/x86/pci/acpi.c +4 -0
+       } else if (flags & IORESOURCE_MEM && (start & 0x0f || ~end & 0x0f)) {
+               dev_warn(&info->bridge->dev,
+                        "invalid host bridge window [%#llx-%#llx]\n",
+                        start, end);
We didn't actually *fix* anything here, so I guess we're just pointing out the reason for a subsequent failure to claim the adjacent resource.

Correct. There is no fix; only a diagnostic warning.

The warning is also a 'red flag' that, on this machine, it might be better to boot the kernel with the "pci=nocrs" option.

Quoted 4 lines

As far as I know, the spec doesn't actually require resources of ACPI devices to be non-overlapping. Windows accepts overlapping resources, and I think Linux probably should, too, but right now we trip over this.

(note: I included a link below to the defect report which has the /proc/iomem, dmesg & dmidecode)

The situation is this:

The adjacent resources (northbridge & southbridge) are not defined by ACPI, but rather reserved with an e820 address descriptor from [0xfe000000-0xfeffffff], so strictly speaking there is no overlapping ACPI resource.

The e820 descriptor is bumped out to [0xf0000000-0xfeffffff] and the malformed host bridge window is reparented to it.

At this point in the boot, there is no resource conflict.

Later in the boot, the i5k_amb driver tries to map [0xfe000000-0xfe01ffff] which is the FB-DIMM AMB register window on the Intel 5400 MCH and is rejected. The request is rejected because the requested range does not map completely to a single parent and this is not allowed. (The i5k_amb driver exposes the FB-DIMM temperature sensors through sysfs).

There is no problem in Windows because no driver attempts to allocate [0xfe000000-0xfe01ffff]. However, I doubt the PNP Manager would allow another bus pdo to claim an overlapping resource with PCI bus 0. I suspect the offending device would yellow bang. (That would be an interesting experiment...)

Quoted 4 lines

In the meantime (until we figure out how to handle overlapping resources better), can we do something to actually fix this? Maybe we should truncate the end of the range to 0xFDFFFFFF like we do for non-addressable parts of the range?

Auto-fixing this seems problematic because it's essentially impossible to determine if the resource length or the resource end or both is wrong.

Quoted 1 line

Is there a bugzilla or a complete dmesg log to look at?

https://bugzilla.kernel.org/show_bug.cgi?id=50161

Regards,
Peter Hurley

Peter Hurley

author

2012-12-14 12:51 UTC #7 lore raw

Re: [PATCH 4/5] x86: acpi: Print warning for malformed host bridge resources

Ping?

Quoted 90 lines · Peter Hurley

Quoted 12 lines · Bjorn Helgaas
Quoted 10 lines · Peter Hurley
An incorrectly specified host bridge window may prevent other devices from claiming assigned resources. For example, this flawed _CRS resource descriptor from a Dell T5400:
     DWordMemory (ResourceProducer, PosDecode, MinFixed, MaxFixed, NonCacheable, ReadWrite,
         0x00000000,         // Granularity
         0xF0000000,         // Range Minimum
         0xFE000000,         // Range Maximum
         0x00000000,         // Translation Offset
         0x0E000000,         // Length
         ,, , AddressRangeMemory, TypeStatic)
Quoted 2 lines
I think the problem here is that the Range Maximum should be 0xFDFFFFFF, not 0xFE000000, right?
I presume so.
Quoted 8 lines
Quoted 8 lines
arch/x86/pci/acpi.c +0 -0
index 192397c..3468d16 100644
@@ -298,6 +298,10 @@ setup_resource(struct acpi_resource *acpi_res, void *data)
                        "host bridge window [%#llx-%#llx] "
                        "([%#llx-%#llx] ignored, not CPU addressable)\n",
                        start, orig_end, end + 1, orig_end);
Quoted 8 lines
arch/x86/pci/acpi.c setup_resource(struct acpi_resource *acpi_res, void *data) · line 304
Quoted 4 lines
arch/x86/pci/acpi.c +4 -0
+       } else if (flags & IORESOURCE_MEM && (start & 0x0f || ~end & 0x0f)) {
+               dev_warn(&info->bridge->dev,
+                        "invalid host bridge window [%#llx-%#llx]\n",
+                        start, end);
We didn't actually *fix* anything here, so I guess we're just pointing out the reason for a subsequent failure to claim the adjacent resource.
Correct. There is no fix; only a diagnostic warning.
The warning is also a 'red flag' that, on this machine, it might be better to boot the kernel with the "pci=nocrs" option.
Quoted 4 lines
As far as I know, the spec doesn't actually require resources of ACPI devices to be non-overlapping. Windows accepts overlapping resources, and I think Linux probably should, too, but right now we trip over this.
(note: I included a link below to the defect report which has the /proc/iomem, dmesg & dmidecode)
The situation is this:
The adjacent resources (northbridge & southbridge) are not defined by ACPI, but rather reserved with an e820 address descriptor from [0xfe000000-0xfeffffff], so strictly speaking there is no overlapping ACPI resource.
The e820 descriptor is bumped out to [0xf0000000-0xfeffffff] and the malformed host bridge window is reparented to it.
At this point in the boot, there is no resource conflict.
Later in the boot, the i5k_amb driver tries to map [0xfe000000-0xfe01ffff] which is the FB-DIMM AMB register window on the Intel 5400 MCH and is rejected. The request is rejected because the requested range does not map completely to a single parent and this is not allowed. (The i5k_amb driver exposes the FB-DIMM temperature sensors through sysfs).
There is no problem in Windows because no driver attempts to allocate [0xfe000000-0xfe01ffff]. However, I doubt the PNP Manager would allow another bus pdo to claim an overlapping resource with PCI bus 0. I suspect the offending device would yellow bang. (That would be an interesting experiment...)
Quoted 4 lines
In the meantime (until we figure out how to handle overlapping resources better), can we do something to actually fix this? Maybe we should truncate the end of the range to 0xFDFFFFFF like we do for non-addressable parts of the range?
Auto-fixing this seems problematic because it's essentially impossible to determine if the resource length or the resource end or both is wrong.
Quoted 1 line
Is there a bugzilla or a complete dmesg log to look at?
https://bugzilla.kernel.org/show_bug.cgi?id=50161
Regards,
Peter Hurley

Peter Hurley

author

2012-11-08 02:57 UTC #8 lore raw

PATCH 5/5 drivers: mfd: Fix resource request for [mem 0x00000000]

In reply to Peter Hurley

The older southbridges supported by the lpc_ich driver do not provide memory-mapped space of the root complex. The driver correctly avoids computing the iomem address in this case, yet submits a zeroed resource request anyway (via mfd_add_devices()).

Remove the iomem resource from the resource array submitted to the mfd core for the older southbridges.

Cc Aaron Sierra <[email protected]>

Cc Peter Tyser <[email protected]>

Cc Samuel Ortiz <[email protected]>

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/mfd/lpc_ich.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/mfd/lpc_ich.c +3 -0

index a22544f..a46095b 100644
@@ -842,6 +842,9 @@ static int __devinit lpc_ich_init_wdt(struct pci_dev *dev,
 		res = wdt_mem_res(ICH_RES_MEM_GCS);
 		res->start = base_addr + ACPIBASE_GCS_OFF;
 		res->end = base_addr + ACPIBASE_GCS_END;
+	} else {
+		/* So don't register iomem for TCO ver 1 */
+		--lpc_ich_cells[LPC_WDT].num_resources;
 	}
 
 	lpc_ich_finalize_cell(&lpc_ich_cells[LPC_WDT], id);

Signature · 1 line

1.7.12.3

Aaron Sierra

2012-11-08 17:54 UTC #9 lore raw

Re: [PATCH 5/5] drivers: mfd: Fix resource request for [mem 0x00000000]

Quoted 8 lines

The older southbridges supported by the lpc_ich driver do not provide memory-mapped space of the root complex. The driver correctly avoids computing the iomem address in this case, yet submits a zeroed resource request anyway (via mfd_add_devices()).
Remove the iomem resource from the resource array submitted to the mfd core for the older southbridges.

Peter, thanks for catching this.

drivers/mfd/lpc_ich.c static int __devinit lpc_ich_init_wdt(struct pci_dev *dev, · line 847

Quoted 3 lines

drivers/mfd/lpc_ich.c +3 -0

+	} else {
+		/* So don't register iomem for TCO ver 1 */
+		--lpc_ich_cells[LPC_WDT].num_resources;

My only complaint is that pre-decrementing num_resources isn't necessary and doesn't match the other cases where num_resources is decremented (post-decremented), like here:

        if (!base_addr) {
                dev_err(&dev->dev, "I/O space for ACPI uninitialized\n");
                lpc_ich_cells[LPC_GPIO].num_resources--;
                goto gpe0_done;
        }

-Aaron

Peter Hurley

author

2012-11-08 18:24 UTC #10 lore raw

Re: [PATCH 5/5] drivers: mfd: Fix resource request for [mem 0x00000000]

Quoted 19 lines · Aaron Sierra

Quoted 8 lines
The older southbridges supported by the lpc_ich driver do not provide memory-mapped space of the root complex. The driver correctly avoids computing the iomem address in this case, yet submits a zeroed resource request anyway (via mfd_add_devices()).
Remove the iomem resource from the resource array submitted to the mfd core for the older southbridges.
Peter, thanks for catching this.
Review
Quoted 3 lines
(quoted diff) +3 -0
+	} else {
+		/* So don't register iomem for TCO ver 1 */
+		--lpc_ich_cells[LPC_WDT].num_resources;
My only complaint is that pre-decrementing num_resources isn't necessary and doesn't match the other cases where num_resources is decremented (post-decremented), like here:

Quoted 5 lines

        if (!base_addr) {
                dev_err(&dev->dev, "I/O space for ACPI uninitialized\n");
                lpc_ich_cells[LPC_GPIO].num_resources--;
                goto gpe0_done;
        }

Sorry, C++ force-of-habit. How's this instead? (please note, I also retitled the patch to refer to lpc_ich specifically)

-- >8 --
Subject: [PATCH v2 5/5] mfd: lpc_ich: Fix resource request for [mem 0x00000000]

Remove the iomem resource from the resource array submitted to the mfd core for the older southbridges.

Cc Aaron Sierra <[email protected]>

Cc Peter Tyser <[email protected]>

Cc Samuel Ortiz <[email protected]>

Signed-off-by Peter Hurley <[email protected]>

---

v2: post-decrement to match existing style
retitle patch subject

 drivers/mfd/lpc_ich.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/mfd/lpc_ich.c +3 -0

index a22544f..f507c09 100644
@@ -842,6 +842,9 @@ static int __devinit lpc_ich_init_wdt(struct pci_dev *dev,
 		res = wdt_mem_res(ICH_RES_MEM_GCS);
 		res->start = base_addr + ACPIBASE_GCS_OFF;
 		res->end = base_addr + ACPIBASE_GCS_END;
+	} else {
+		/* So don't register iomem for TCO ver 1 */
+		lpc_ich_cells[LPC_WDT].num_resources--;
 	}
 
 	lpc_ich_finalize_cell(&lpc_ich_cells[LPC_WDT], id);

Signature · 1 line

1.8.0

Aaron Sierra

Acked-by

2012-11-08 18:58 UTC #11 lore raw

Re: [PATCH 5/5] drivers: mfd: Fix resource request for [mem 0x00000000]

Quoted 5 lines

v2: post-decrement to match existing style
retitle patch subject
 drivers/mfd/lpc_ich.c | 3 +++
 1 file changed, 3 insertions(+)

Acked-by Aaron Sierra <[email protected]>

You could make Samuel's job easier by sending a new e-mail with the latest patch and the correct subject in the e-mail's subject line. Since this patch doesn't really depend on any of the other four you submitted, a new message/subject like this seems appropriate to me:

[PATCH v3] mfd: lpc_ich: Fix resource request for [mem 0x00000000]

Peter Hurley

author

2012-11-09 11:55 UTC #12 lore raw

PATCH v3 1/1 mfd: lpc_ich: Fix resource request for [mem 0x00000000]

Remove the iomem resource from the resource array submitted to the mfd core for the older southbridges.

Acked-by Aaron Sierra <[email protected]>

Cc Peter Tyser <[email protected]>

Cc Samuel Ortiz <[email protected]>

Signed-off-by Peter Hurley <[email protected]>

---

v2: post-decrement to match existing style
retitle patch subject
v3: respin as standalone patch

 drivers/mfd/lpc_ich.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/mfd/lpc_ich.c +3 -0

index a22544f..f507c09 100644
@@ -842,6 +842,9 @@ static int __devinit lpc_ich_init_wdt(struct pci_dev *dev,
 		res = wdt_mem_res(ICH_RES_MEM_GCS);
 		res->start = base_addr + ACPIBASE_GCS_OFF;
 		res->end = base_addr + ACPIBASE_GCS_END;
+	} else {
+		/* So don't register iomem for TCO ver 1 */
+		lpc_ich_cells[LPC_WDT].num_resources--;
 	}
 
 	lpc_ich_finalize_cell(&lpc_ich_cells[LPC_WDT], id);

Signature · 1 line

1.8.0

Samuel Ortiz

2012-11-19 17:46 UTC #13 lore raw

Re: [PATCH v3] mfd: lpc_ich: Fix resource request for [mem 0x00000000]

Hi Peter,

Quoted 26 lines · Peter Hurley

The older southbridges supported by the lpc_ich driver do not provide memory-mapped space of the root complex. The driver correctly avoids computing the iomem address in this case, yet submits a zeroed resource request anyway (via mfd_add_devices()).
Remove the iomem resource from the resource array submitted to the mfd core for the older southbridges.
Acked-by Aaron Sierra <[email protected]>
Cc Peter Tyser <[email protected]>
Cc Samuel Ortiz <[email protected]>
Signed-off-by Peter Hurley <[email protected]>
---
v2: post-decrement to match existing style
retitle patch subject
v3: respin as standalone patch
 drivers/mfd/lpc_ich.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/mfd/lpc_ich.c +0 -0
index a22544f..f507c09 100644

drivers/mfd/lpc_ich.c static int __devinit lpc_ich_init_wdt(struct pci_dev *dev, · line 845

Quoted 4 lines

drivers/mfd/lpc_ich.c +1 -0

@@ -842,6 +842,9 @@ static int __devinit lpc_ich_init_wdt(struct pci_dev *dev,
 		res = wdt_mem_res(ICH_RES_MEM_GCS);
 		res->start = base_addr + ACPIBASE_GCS_OFF;
 		res->end = base_addr + ACPIBASE_GCS_END;
+	} else {

So I suppose there is no v3 for the iTCO ? If we're expecting all versions after 1 to have a memory mapped region, we should have something like:

drivers/mfd/lpc_ich.c +0 -0

@@ -830,7 +830,10 @@ static int __devinit lpc_ich_init_wdt(struct pci_dev

*dev,
* we have to read RCBA from PCI Config space 0xf0 and use
* it as base. GCS = RCBA + ICH6_GCS(0x3410). */
- if (lpc_chipset_info[id->driver_data].iTCO_version == 2) {
+ if (lpc_chipset_info[id->driver_data].iTCO_version == 1) {
+ /* Don't register iomem for TCO ver 1 */
+ lpc_ich_cells[LPC_WDT].num_resources--;

+       } else {
                pci_read_config_dword(dev, RCBABASE, &base_addr_cfg);
                base_addr = base_addr_cfg & 0xffffc000;
                if (!(base_addr_cfg & 1)) {

Cheers,
Samuel.

Signature · 2 lines

Intel Open Source Technology Centre
http://oss.intel.com/

Peter Hurley

author

2012-11-19 18:29 UTC #14 lore raw

Re: [PATCH v3] mfd: lpc_ich: Fix resource request for [mem 0x00000000]

Quoted 50 lines · Samuel Ortiz

Hi Peter,
Quoted 26 lines · Peter Hurley
The older southbridges supported by the lpc_ich driver do not provide memory-mapped space of the root complex. The driver correctly avoids computing the iomem address in this case, yet submits a zeroed resource request anyway (via mfd_add_devices()).
Remove the iomem resource from the resource array submitted to the mfd core for the older southbridges.
Acked-by Aaron Sierra <[email protected]>
Cc Peter Tyser <[email protected]>
Cc Samuel Ortiz <[email protected]>
Signed-off-by Peter Hurley <[email protected]>
---
v2: post-decrement to match existing style
retitle patch subject
v3: respin as standalone patch
 drivers/mfd/lpc_ich.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/mfd/lpc_ich.c +0 -0
index a22544f..f507c09 100644
drivers/mfd/lpc_ich.c static int __devinit lpc_ich_init_wdt(struct pci_dev *dev, · line 845
Quoted 4 lines
drivers/mfd/lpc_ich.c +1 -0
@@ -842,6 +842,9 @@ static int __devinit lpc_ich_init_wdt(struct pci_dev *dev,
 		res = wdt_mem_res(ICH_RES_MEM_GCS);
 		res->start = base_addr + ACPIBASE_GCS_OFF;
 		res->end = base_addr + ACPIBASE_GCS_END;
+	} else {
So I suppose there is no v3 for the iTCO ? If we're expecting all versions after 1 to have a memory mapped region, we should have something like:
drivers/mfd/lpc_ich.c +4 -1
@@ -830,7 +830,10 @@ static int __devinit lpc_ich_init_wdt(struct pci_dev
 *dev,
         * we have to read RCBA from PCI Config space 0xf0 and use
         * it as base. GCS = RCBA + ICH6_GCS(0x3410).
         */
-       if (lpc_chipset_info[id->driver_data].iTCO_version == 2) {
+       if (lpc_chipset_info[id->driver_data].iTCO_version == 1) {
+               /* Don't register iomem for TCO ver 1 */
+               lpc_ich_cells[LPC_WDT].num_resources--;
+       } else {
                pci_read_config_dword(dev, RCBABASE, &base_addr_cfg);
                base_addr = base_addr_cfg & 0xffffc000;

Quoted 1 line

                if (!(base_addr_cfg & 1)) {

Hi Samuel,

I have no objection to your version.

FWIW, the iTCO_version field is exclusively a driver construct used to differentiate southbridges that support memory-mapped I/O to the TCO registers from those that only support port-based I/O. IOW, there's no intrinsic meaning to the values and could be represented with a bool type instead.

Regards,
Peter Hurley

Samuel Ortiz

Applied

2012-11-21 16:33 UTC #15 lore raw

Re: [PATCH v3] mfd: lpc_ich: Fix resource request for [mem 0x00000000]

Hi Peter,

Quoted 55 lines · Peter Hurley

Quoted 50 lines · Samuel Ortiz
Hi Peter,
Quoted 26 lines · Peter Hurley
The older southbridges supported by the lpc_ich driver do not provide memory-mapped space of the root complex. The driver correctly avoids computing the iomem address in this case, yet submits a zeroed resource request anyway (via mfd_add_devices()).
Remove the iomem resource from the resource array submitted to the mfd core for the older southbridges.
Acked-by Aaron Sierra <[email protected]>
Cc Peter Tyser <[email protected]>
Cc Samuel Ortiz <[email protected]>
Signed-off-by Peter Hurley <[email protected]>
---
v2: post-decrement to match existing style
retitle patch subject
v3: respin as standalone patch
 drivers/mfd/lpc_ich.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/mfd/lpc_ich.c +0 -0
index a22544f..f507c09 100644
drivers/mfd/lpc_ich.c static int __devinit lpc_ich_init_wdt(struct pci_dev *dev, · line 845
Quoted 4 lines
drivers/mfd/lpc_ich.c +1 -0
@@ -842,6 +842,9 @@ static int __devinit lpc_ich_init_wdt(struct pci_dev *dev,
 		res = wdt_mem_res(ICH_RES_MEM_GCS);
 		res->start = base_addr + ACPIBASE_GCS_OFF;
 		res->end = base_addr + ACPIBASE_GCS_END;
+	} else {
So I suppose there is no v3 for the iTCO ? If we're expecting all versions after 1 to have a memory mapped region, we should have something like:
drivers/mfd/lpc_ich.c +4 -1
@@ -830,7 +830,10 @@ static int __devinit lpc_ich_init_wdt(struct pci_dev
 *dev,
         * we have to read RCBA from PCI Config space 0xf0 and use
         * it as base. GCS = RCBA + ICH6_GCS(0x3410).
         */
-       if (lpc_chipset_info[id->driver_data].iTCO_version == 2) {
+       if (lpc_chipset_info[id->driver_data].iTCO_version == 1) {
+               /* Don't register iomem for TCO ver 1 */
+               lpc_ich_cells[LPC_WDT].num_resources--;
+       } else {
                pci_read_config_dword(dev, RCBABASE, &base_addr_cfg);
                base_addr = base_addr_cfg & 0xffffc000;
Quoted 1 line
                if (!(base_addr_cfg & 1)) {
Hi Samuel,

Quoted 1 line

I have no objection to your version.

I applied and pushed it to my for-next branch.

Quoted 5 lines

FWIW, the iTCO_version field is exclusively a driver construct used to differentiate southbridges that support memory-mapped I/O to the TCO registers from those that only support port-based I/O. IOW, there's no intrinsic meaning to the values and could be represented with a bool type instead.

Right, that sounds like follow up patch material.

Cheers,
Samuel.

Signature · 2 lines

Intel Open Source Technology Centre
http://oss.intel.com/