2012-08-14 16:01:39

by Feng Tang

[permalink] [raw]
Subject: [PATCH] lpc_ich: Fix a 3.5 kernel regression for iTCO_wdt driver

There are many reports (including 2 of my machines) that iTCO_wdt watchdog
driver fails to be initialized in 3.5 kernel with error message like:

[ 5.265175] ACPI Warning: 0x00001060-0x0000107f SystemIO conflicts with Region \_SB_.PCI0.LPCB.TCOI 1 (20120320/utaddress-251)
[ 5.265192] ACPI: If an ACPI driver is available for this device, you should use it instead of the native driver
[ 5.265206] lpc_ich: Resource conflict(s) found affecting iTCO_wdt

The root cause the iTCO_wdt driver in 3.4 probes the HW IO resource from
LPC's PCI config space, while in 3.5 kernel it relies on lpc_ich driver
for the probe, which adds a new acpi_check_resource_conflict() check, and
give up the probe if there is any conflict with ACPI.

Fix it by removing all the checks for iTCO_wdt to keep the same behavior as
3.4 kernel.
https://bugzilla.kernel.org/show_bug.cgi?id=44991

Actually the same check could be removed for the gpio-ich in lpc_ich.c,
but I'm not sure if it will cause problems.

Signed-off-by: Feng Tang <[email protected]>
Cc: Aaron Sierra <[email protected]>
Cc: Wim Van Sebroeck <[email protected]>
Cc: Len Brown <[email protected]>
Cc: Bob Moore <[email protected]>
---
drivers/mfd/lpc_ich.c | 20 +-------------------
1 files changed, 1 insertions(+), 19 deletions(-)

diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
index 027cc8f..a05fdfc 100644
--- a/drivers/mfd/lpc_ich.c
+++ b/drivers/mfd/lpc_ich.c
@@ -765,7 +765,6 @@ static int __devinit lpc_ich_init_wdt(struct pci_dev *dev,
u32 base_addr_cfg;
u32 base_addr;
int ret;
- bool acpi_conflict = false;
struct resource *res;

/* Setup power management base register */
@@ -780,20 +779,11 @@ static int __devinit lpc_ich_init_wdt(struct pci_dev *dev,
res = wdt_io_res(ICH_RES_IO_TCO);
res->start = base_addr + ACPIBASE_TCO_OFF;
res->end = base_addr + ACPIBASE_TCO_END;
- ret = acpi_check_resource_conflict(res);
- if (ret) {
- acpi_conflict = true;
- goto wdt_done;
- }

res = wdt_io_res(ICH_RES_IO_SMI);
res->start = base_addr + ACPIBASE_SMI_OFF;
res->end = base_addr + ACPIBASE_SMI_END;
- ret = acpi_check_resource_conflict(res);
- if (ret) {
- acpi_conflict = true;
- goto wdt_done;
- }
+
lpc_ich_enable_acpi_space(dev);

/*
@@ -813,11 +803,6 @@ 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;
- ret = acpi_check_resource_conflict(res);
- if (ret) {
- acpi_conflict = true;
- goto wdt_done;
- }
}

lpc_ich_finalize_cell(&lpc_ich_cells[LPC_WDT], id);
@@ -825,9 +810,6 @@ static int __devinit lpc_ich_init_wdt(struct pci_dev *dev,
1, NULL, 0);

wdt_done:
- if (acpi_conflict)
- pr_warn("Resource conflict(s) found affecting %s\n",
- lpc_ich_cells[LPC_WDT].name);
return ret;
}

--
1.7.1


2012-08-22 19:55:21

by Wim Van Sebroeck

[permalink] [raw]
Subject: Re: [PATCH] lpc_ich: Fix a 3.5 kernel regression for iTCO_wdt driver

Hi All,

Cc-ing Samuel and Guenter also.

> There are many reports (including 2 of my machines) that iTCO_wdt watchdog
> driver fails to be initialized in 3.5 kernel with error message like:
>
> [ 5.265175] ACPI Warning: 0x00001060-0x0000107f SystemIO conflicts with Region \_SB_.PCI0.LPCB.TCOI 1 (20120320/utaddress-251)
> [ 5.265192] ACPI: If an ACPI driver is available for this device, you should use it instead of the native driver
> [ 5.265206] lpc_ich: Resource conflict(s) found affecting iTCO_wdt
>
> The root cause the iTCO_wdt driver in 3.4 probes the HW IO resource from
> LPC's PCI config space, while in 3.5 kernel it relies on lpc_ich driver
> for the probe, which adds a new acpi_check_resource_conflict() check, and
> give up the probe if there is any conflict with ACPI.
>
> Fix it by removing all the checks for iTCO_wdt to keep the same behavior as
> 3.4 kernel.
> https://bugzilla.kernel.org/show_bug.cgi?id=44991
>
> Actually the same check could be removed for the gpio-ich in lpc_ich.c,
> but I'm not sure if it will cause problems.
>
> Signed-off-by: Feng Tang <[email protected]>
> Cc: Aaron Sierra <[email protected]>
> Cc: Wim Van Sebroeck <[email protected]>
> Cc: Len Brown <[email protected]>
> Cc: Bob Moore <[email protected]>
> ---
> drivers/mfd/lpc_ich.c | 20 +-------------------
> 1 files changed, 1 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
> index 027cc8f..a05fdfc 100644
> --- a/drivers/mfd/lpc_ich.c
> +++ b/drivers/mfd/lpc_ich.c
> @@ -765,7 +765,6 @@ static int __devinit lpc_ich_init_wdt(struct pci_dev *dev,
> u32 base_addr_cfg;
> u32 base_addr;
> int ret;
> - bool acpi_conflict = false;
> struct resource *res;
>
> /* Setup power management base register */
> @@ -780,20 +779,11 @@ static int __devinit lpc_ich_init_wdt(struct pci_dev *dev,
> res = wdt_io_res(ICH_RES_IO_TCO);
> res->start = base_addr + ACPIBASE_TCO_OFF;
> res->end = base_addr + ACPIBASE_TCO_END;
> - ret = acpi_check_resource_conflict(res);
> - if (ret) {
> - acpi_conflict = true;
> - goto wdt_done;
> - }
>
> res = wdt_io_res(ICH_RES_IO_SMI);
> res->start = base_addr + ACPIBASE_SMI_OFF;
> res->end = base_addr + ACPIBASE_SMI_END;
> - ret = acpi_check_resource_conflict(res);
> - if (ret) {
> - acpi_conflict = true;
> - goto wdt_done;
> - }
> +
> lpc_ich_enable_acpi_space(dev);
>
> /*
> @@ -813,11 +803,6 @@ 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;
> - ret = acpi_check_resource_conflict(res);
> - if (ret) {
> - acpi_conflict = true;
> - goto wdt_done;
> - }
> }
>
> lpc_ich_finalize_cell(&lpc_ich_cells[LPC_WDT], id);
> @@ -825,9 +810,6 @@ static int __devinit lpc_ich_init_wdt(struct pci_dev *dev,
> 1, NULL, 0);
>
> wdt_done:
> - if (acpi_conflict)
> - pr_warn("Resource conflict(s) found affecting %s\n",
> - lpc_ich_cells[LPC_WDT].name);
> return ret;
> }
>

Hi Len,

Any idea why the acpi_check_resource_conflict() check gives a conflict?

Thanks in advance,
Wim.

2012-08-22 21:55:58

by Matthew Garrett

[permalink] [raw]
Subject: Re: [PATCH] lpc_ich: Fix a 3.5 kernel regression for iTCO_wdt driver

On Wed, Aug 22, 2012 at 09:55:12PM +0200, Wim Van Sebroeck wrote:

> Any idea why the acpi_check_resource_conflict() check gives a conflict?

Because the resource range is declared in ACPI and we assume that that
means the firmware wants to scribble on it. We'd need the output of
acpidump to work out whether that's safe or not.

--
Matthew Garrett | [email protected]

2012-08-23 05:14:35

by Feng Tang

[permalink] [raw]
Subject: Re: [PATCH] lpc_ich: Fix a 3.5 kernel regression for iTCO_wdt driver

On Wed, 22 Aug 2012 22:55:43 +0100
Matthew Garrett <[email protected]> wrote:

> On Wed, Aug 22, 2012 at 09:55:12PM +0200, Wim Van Sebroeck wrote:
>
> > Any idea why the acpi_check_resource_conflict() check gives a conflict?
>
> Because the resource range is declared in ACPI and we assume that that
> means the firmware wants to scribble on it. We'd need the output of
> acpidump to work out whether that's safe or not.

Good point, I checked the conflict for iTCO_wdt, the conflict exists on
almost all the machines I have.

According to ICH (7/8/9 etc)spec, the TCO watchdog has a 32 bytes long IO
space resource, and the bit 9 of TCO1_STS register is "DMISCI_STS", which
indicates whether a SCI happens, and will be cleared by writing 1
to it. Most of DSDT table will claim a TCO op region only for one bit:
"DMISCI_STS" , as some method may need to access that bit.

I think there is some risk, but it's quite safe as the DMISCI_STS bit has
nothing to do with TCO driver itself, and TCO driver never access it, also
this TCO driver has been there for years, and this resource conflict also
exists for many generations hardware.

Thanks,
Feng


2012-08-23 15:28:58

by Guenter Roeck

[permalink] [raw]
Subject: Re: lpc_ich: Fix a 3.5 kernel regression for iTCO_wdt driver

On Tue, Aug 14, 2012 at 03:56:12PM -0000, Feng Tang wrote:
> There are many reports (including 2 of my machines) that iTCO_wdt watchdog
> driver fails to be initialized in 3.5 kernel with error message like:
>
> [ 5.265175] ACPI Warning: 0x00001060-0x0000107f SystemIO conflicts with Region \_SB_.PCI0.LPCB.TCOI 1 (20120320/utaddress-251)
> [ 5.265192] ACPI: If an ACPI driver is available for this device, you should use it instead of the native driver
> [ 5.265206] lpc_ich: Resource conflict(s) found affecting iTCO_wdt
>
> The root cause the iTCO_wdt driver in 3.4 probes the HW IO resource from
> LPC's PCI config space, while in 3.5 kernel it relies on lpc_ich driver
> for the probe, which adds a new acpi_check_resource_conflict() check, and
> give up the probe if there is any conflict with ACPI.
>
> Fix it by removing all the checks for iTCO_wdt to keep the same behavior as
> 3.4 kernel.
> https://bugzilla.kernel.org/show_bug.cgi?id=44991
>
> Actually the same check could be removed for the gpio-ich in lpc_ich.c,
> but I'm not sure if it will cause problems.
>
> Signed-off-by: Feng Tang <[email protected]>
> Cc: Aaron Sierra <[email protected]>
> Cc: Wim Van Sebroeck <[email protected]>
> Cc: Len Brown <[email protected]>
> Cc: Bob Moore <[email protected]>
>
Kind of unfortunate to have the ACPI conflict, but I don't have an idea
for a better fix.

Acked-by: Guenter Roeck <[email protected]>

> ---
> drivers/mfd/lpc_ich.c | 20 +-------------------
> 1 files changed, 1 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
> index 027cc8f..a05fdfc 100644
> --- a/drivers/mfd/lpc_ich.c
> +++ b/drivers/mfd/lpc_ich.c
> @@ -765,7 +765,6 @@ static int __devinit lpc_ich_init_wdt(struct pci_dev *dev,
> u32 base_addr_cfg;
> u32 base_addr;
> int ret;
> - bool acpi_conflict = false;
> struct resource *res;
>
> /* Setup power management base register */
> @@ -780,20 +779,11 @@ static int __devinit lpc_ich_init_wdt(struct pci_dev *dev,
> res = wdt_io_res(ICH_RES_IO_TCO);
> res->start = base_addr + ACPIBASE_TCO_OFF;
> res->end = base_addr + ACPIBASE_TCO_END;
> - ret = acpi_check_resource_conflict(res);
> - if (ret) {
> - acpi_conflict = true;
> - goto wdt_done;
> - }
>
> res = wdt_io_res(ICH_RES_IO_SMI);
> res->start = base_addr + ACPIBASE_SMI_OFF;
> res->end = base_addr + ACPIBASE_SMI_END;
> - ret = acpi_check_resource_conflict(res);
> - if (ret) {
> - acpi_conflict = true;
> - goto wdt_done;
> - }
> +
> lpc_ich_enable_acpi_space(dev);
>
> /*
> @@ -813,11 +803,6 @@ 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;
> - ret = acpi_check_resource_conflict(res);
> - if (ret) {
> - acpi_conflict = true;
> - goto wdt_done;
> - }
> }
>
> lpc_ich_finalize_cell(&lpc_ich_cells[LPC_WDT], id);
> @@ -825,9 +810,6 @@ static int __devinit lpc_ich_init_wdt(struct pci_dev *dev,
> 1, NULL, 0);
>
> wdt_done:
> - if (acpi_conflict)
> - pr_warn("Resource conflict(s) found affecting %s\n",
> - lpc_ich_cells[LPC_WDT].name);
> return ret;
> }
>

2012-08-23 17:20:46

by Samuel Ortiz

[permalink] [raw]
Subject: Re: [PATCH] lpc_ich: Fix a 3.5 kernel regression for iTCO_wdt driver

Hi Feng,

On Thu, Aug 23, 2012 at 01:08:14PM +0800, Feng Tang wrote:
> On Wed, 22 Aug 2012 22:55:43 +0100
> Matthew Garrett <[email protected]> wrote:
>
> > On Wed, Aug 22, 2012 at 09:55:12PM +0200, Wim Van Sebroeck wrote:
> >
> > > Any idea why the acpi_check_resource_conflict() check gives a conflict?
> >
> > Because the resource range is declared in ACPI and we assume that that
> > means the firmware wants to scribble on it. We'd need the output of
> > acpidump to work out whether that's safe or not.
>
> Good point, I checked the conflict for iTCO_wdt, the conflict exists on
> almost all the machines I have.
>
> According to ICH (7/8/9 etc)spec, the TCO watchdog has a 32 bytes long IO
> space resource, and the bit 9 of TCO1_STS register is "DMISCI_STS", which
> indicates whether a SCI happens, and will be cleared by writing 1
> to it. Most of DSDT table will claim a TCO op region only for one bit:
> "DMISCI_STS" , as some method may need to access that bit.
>
> I think there is some risk, but it's quite safe as the DMISCI_STS bit has
> nothing to do with TCO driver itself, and TCO driver never access it, also
> this TCO driver has been there for years, and this resource conflict also
> exists for many generations hardware.
Makes sense to me.
I'm queueing this one to my for-linus branch, I'll send a pull request soon.

Cheers,
Samuel.

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