[PATCH 0/5] Add diagnostics for iomem resources

[PATCH 0/5] Add diagnostics for iomem resources

[Peter Hurley]

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(-)

-- 
1.7.12.3

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

[Peter Hurley]

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 <bhelgaas@google.com>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 kernel/resource.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel/resource.c b/kernel/resource.c
index 73f35d4..461c2e0 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -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);
 }
-- 
1.7.12.3

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

[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 <bhelgaas@google.com>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 kernel/resource.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/kernel/resource.c b/kernel/resource.c
index 461c2e0..268b5fa 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -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;
-- 
1.7.12.3

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

[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 <bhelgaas@google.com>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 kernel/resource.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/resource.c b/kernel/resource.c
index 268b5fa..ab758bb 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -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)
-- 
1.7.12.3

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

[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 <bhelgaas@google.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: x86@kernel.org
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 arch/x86/pci/acpi.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
index 192397c..3468d16 100644
--- a/arch/x86/pci/acpi.c
+++ b/arch/x86/pci/acpi.c
@@ -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];
-- 
1.7.12.3

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

[Bjorn Helgaas]

On Wed, Nov 7, 2012 at 7:55 PM, Peter Hurley <peter@hurleysoftware.com> wrote:
> 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?

> 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 <bhelgaas@google.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: H. Peter Anvin <hpa@zytor.com>
> Cc: x86@kernel.org
> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
> ---
>  arch/x86/pci/acpi.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
> index 192397c..3468d16 100644
> --- a/arch/x86/pci/acpi.c
> +++ b/arch/x86/pci/acpi.c
> @@ -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

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

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

[Peter Hurley]

On Sat, 2012-11-10 at 14:52 -0700, Bjorn Helgaas wrote:
> On Wed, Nov 7, 2012 at 7:55 PM, Peter Hurley <peter@hurleysoftware.com> wrote:
> > 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?

I presume so.

> > diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
> > index 192397c..3468d16 100644
> > --- a/arch/x86/pci/acpi.c
> > +++ b/arch/x86/pci/acpi.c
> > @@ -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.

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.

> 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...)

> 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.

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

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

Regards,
Peter Hurley

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

[Peter Hurley]

Ping?

On Sun, 2012-11-11 at 09:49 -0500, Peter Hurley wrote:
> On Sat, 2012-11-10 at 14:52 -0700, Bjorn Helgaas wrote:
> > On Wed, Nov 7, 2012 at 7:55 PM, Peter Hurley <peter@hurleysoftware.com> wrote:
> > > 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?
> 
> I presume so.
> 
> > > diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
> > > index 192397c..3468d16 100644
> > > --- a/arch/x86/pci/acpi.c
> > > +++ b/arch/x86/pci/acpi.c
> > > @@ -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.
> 
> 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.
> 
> > 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...)
> 
> > 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.
> 
> > Is there a bugzilla or a complete dmesg log to look at?
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=50161
> 
> Regards,
> Peter Hurley
> 
> 

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

[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 <asierra@xes-inc.com>
Cc: Peter Tyser <ptyser@xes-inc.com>
Cc: Samuel Ortiz <sameo@linux.intel.com>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/mfd/lpc_ich.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
index a22544f..a46095b 100644
--- a/drivers/mfd/lpc_ich.c
+++ b/drivers/mfd/lpc_ich.c
@@ -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);
-- 
1.7.12.3

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

[Aaron Sierra]

> 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.

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

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

[Peter Hurley]

On Thu, 2012-11-08 at 11:04 -0600, Aaron Sierra wrote:
> > 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.
> 
> > +	} 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;
>         }

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]

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 <asierra@xes-inc.com>
Cc: Peter Tyser <ptyser@xes-inc.com>
Cc: Samuel Ortiz <sameo@linux.intel.com>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---

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

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

diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
index a22544f..f507c09 100644
--- a/drivers/mfd/lpc_ich.c
+++ b/drivers/mfd/lpc_ich.c
@@ -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);
-- 
1.8.0

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

[Aaron Sierra]

> 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 <asierra@xes-inc.com>

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]

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

[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 <asierra@xes-inc.com>
Cc: Peter Tyser <ptyser@xes-inc.com>
Cc: Samuel Ortiz <sameo@linux.intel.com>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---

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(+)

diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
index a22544f..f507c09 100644
--- a/drivers/mfd/lpc_ich.c
+++ b/drivers/mfd/lpc_ich.c
@@ -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);
-- 
1.8.0

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

[Samuel Ortiz]

Hi Peter,

On Fri, Nov 09, 2012 at 06:55:29AM -0500, Peter Hurley wrote:
> 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 <asierra@xes-inc.com>
> Cc: Peter Tyser <ptyser@xes-inc.com>
> Cc: Samuel Ortiz <sameo@linux.intel.com>
> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
> ---
> 
> 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(+)
> 
> diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
> index a22544f..f507c09 100644
> --- a/drivers/mfd/lpc_ich.c
> +++ b/drivers/mfd/lpc_ich.c
> @@ -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:

--- a/drivers/mfd/lpc_ich.c
+++ b/drivers/mfd/lpc_ich.c
@@ -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.

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

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

[Peter Hurley]

On Mon, 2012-11-19 at 18:46 +0100, Samuel Ortiz wrote:
> Hi Peter,
> 
> On Fri, Nov 09, 2012 at 06:55:29AM -0500, Peter Hurley wrote:
> > 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 <asierra@xes-inc.com>
> > Cc: Peter Tyser <ptyser@xes-inc.com>
> > Cc: Samuel Ortiz <sameo@linux.intel.com>
> > Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
> > ---
> > 
> > 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(+)
> > 
> > diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
> > index a22544f..f507c09 100644
> > --- a/drivers/mfd/lpc_ich.c
> > +++ b/drivers/mfd/lpc_ich.c
> > @@ -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:
> 
> --- a/drivers/mfd/lpc_ich.c
> +++ b/drivers/mfd/lpc_ich.c
> @@ -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)) {

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

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

[Samuel Ortiz]

Hi Peter,

On Mon, Nov 19, 2012 at 01:28:53PM -0500, Peter Hurley wrote:
> On Mon, 2012-11-19 at 18:46 +0100, Samuel Ortiz wrote:
> > Hi Peter,
> > 
> > On Fri, Nov 09, 2012 at 06:55:29AM -0500, Peter Hurley wrote:
> > > 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 <asierra@xes-inc.com>
> > > Cc: Peter Tyser <ptyser@xes-inc.com>
> > > Cc: Samuel Ortiz <sameo@linux.intel.com>
> > > Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
> > > ---
> > > 
> > > 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(+)
> > > 
> > > diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
> > > index a22544f..f507c09 100644
> > > --- a/drivers/mfd/lpc_ich.c
> > > +++ b/drivers/mfd/lpc_ich.c
> > > @@ -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:
> > 
> > --- a/drivers/mfd/lpc_ich.c
> > +++ b/drivers/mfd/lpc_ich.c
> > @@ -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)) {
> 
> Hi Samuel,
> 
> I have no objection to your version.
I applied and pushed it to my for-next branch.

 
> 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.

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