[PATCH v2] sp5100_tco: Remove code that may cause a boot failure

[PATCH v2] sp5100_tco: Remove code that may cause a boot failure

[Takahisa Tanaka]

The critical problem which can't boot OS until the power is completely
cut off found on PC with SB700 chipset. This patch fix the problem, but,
this patch prevents the sp5100_tco driver from using watchdog timer
function of chipset on PC with SP5100 or SB7x0 chipset.

Re-programming the MMIO address registers for the watchdog timer must
have generated the problem. However, I don't know root cause so far.
So, I decided to remove the concerned codes.

Note that this patch is dependent on the commit adbdcc03 and a6e26b13.

Link: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1116835
Link: https://lkml.org/lkml/2013/2/14/271

Signed-off-by: Takahisa Tanaka <mc74hc00@gmail.com>
---
v1 -> v2
 -The patch was split into the bug fix and the typo fix.
---
 drivers/watchdog/sp5100_tco.c | 126 ++----------------------------------------
 1 file changed, 6 insertions(+), 120 deletions(-)

diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
index e3b8f75..0e9d8c4 100644
--- a/drivers/watchdog/sp5100_tco.c
+++ b/drivers/watchdog/sp5100_tco.c
@@ -40,13 +40,12 @@
 #include "sp5100_tco.h"
 
 /* Module and version information */
-#define TCO_VERSION "0.03"
+#define TCO_VERSION "0.05"
 #define TCO_MODULE_NAME "SP5100 TCO timer"
 #define TCO_DRIVER_NAME   TCO_MODULE_NAME ", v" TCO_VERSION
 
 /* internal variables */
 static u32 tcobase_phys;
-static u32 resbase_phys;
 static u32 tco_wdt_fired;
 static void __iomem *tcobase;
 static unsigned int pm_iobase;
@@ -54,10 +53,6 @@ static DEFINE_SPINLOCK(tco_lock);	/* Guards the hardware */
 static unsigned long timer_alive;
 static char tco_expect_close;
 static struct pci_dev *sp5100_tco_pci;
-static struct resource wdt_res = {
-	.name = "Watchdog Timer",
-	.flags = IORESOURCE_MEM,
-};
 
 /* the watchdog platform device */
 static struct platform_device *sp5100_tco_platform_device;
@@ -75,12 +70,6 @@ module_param(nowayout, bool, 0);
 MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started."
 		" (default=" __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
 
-static unsigned int force_addr;
-module_param(force_addr, uint, 0);
-MODULE_PARM_DESC(force_addr, "Force the use of specified MMIO address."
-		" ONLY USE THIS PARAMETER IF YOU REALLY KNOW"
-		" WHAT YOU ARE DOING (default=none)");
-
 /*
  * Some TCO specific functions
  */
@@ -176,39 +165,6 @@ static void tco_timer_enable(void)
 	}
 }
 
-static void tco_timer_disable(void)
-{
-	int val;
-
-	if (sp5100_tco_pci->revision >= 0x40) {
-		/* For SB800 or later */
-		/* Enable watchdog decode bit and Disable watchdog timer */
-		outb(SB800_PM_WATCHDOG_CONTROL, SB800_IO_PM_INDEX_REG);
-		val = inb(SB800_IO_PM_DATA_REG);
-		val |= SB800_PCI_WATCHDOG_DECODE_EN;
-		val |= SB800_PM_WATCHDOG_DISABLE;
-		outb(val, SB800_IO_PM_DATA_REG);
-	} else {
-		/* For SP5100 or SB7x0 */
-		/* Enable watchdog decode bit */
-		pci_read_config_dword(sp5100_tco_pci,
-				      SP5100_PCI_WATCHDOG_MISC_REG,
-				      &val);
-
-		val |= SP5100_PCI_WATCHDOG_DECODE_EN;
-
-		pci_write_config_dword(sp5100_tco_pci,
-				       SP5100_PCI_WATCHDOG_MISC_REG,
-				       val);
-
-		/* Disable Watchdog timer */
-		outb(SP5100_PM_WATCHDOG_CONTROL, SP5100_IO_PM_INDEX_REG);
-		val = inb(SP5100_IO_PM_DATA_REG);
-		val |= SP5100_PM_WATCHDOG_DISABLE;
-		outb(val, SP5100_IO_PM_DATA_REG);
-	}
-}
-
 /*
  *	/dev/watchdog handling
  */
@@ -361,7 +317,7 @@ static unsigned char sp5100_tco_setupdevice(void)
 {
 	struct pci_dev *dev = NULL;
 	const char *dev_name = NULL;
-	u32 val, tmp_val;
+	u32 val;
 	u32 index_reg, data_reg, base_addr;
 
 	/* Match the PCI device */
@@ -459,63 +415,8 @@ static unsigned char sp5100_tco_setupdevice(void)
 	} else
 		pr_debug("SBResource_MMIO is disabled(0x%04x)\n", val);
 
-	/*
-	 * Lastly re-programming the watchdog timer MMIO address,
-	 * This method is a last resort...
-	 *
-	 * Before re-programming, to ensure that the watchdog timer
-	 * is disabled, disable the watchdog timer.
-	 */
-	tco_timer_disable();
-
-	if (force_addr) {
-		/*
-		 * Force the use of watchdog timer MMIO address, and aligned to
-		 * 8byte boundary.
-		 */
-		force_addr &= ~0x7;
-		val = force_addr;
-
-		pr_info("Force the use of 0x%04x as MMIO address\n", val);
-	} else {
-		/*
-		 * Get empty slot into the resource tree for watchdog timer.
-		 */
-		if (allocate_resource(&iomem_resource,
-				      &wdt_res,
-				      SP5100_WDT_MEM_MAP_SIZE,
-				      0xf0000000,
-				      0xfffffff8,
-				      0x8,
-				      NULL,
-				      NULL)) {
-			pr_err("MMIO allocation failed\n");
-			goto unreg_region;
-		}
-
-		val = resbase_phys = wdt_res.start;
-		pr_debug("Got 0x%04x from resource tree\n", val);
-	}
-
-	/* Restore to the low three bits */
-	outb(base_addr+0, index_reg);
-	tmp_val = val | (inb(data_reg) & 0x7);
-
-	/* Re-programming the watchdog timer base address */
-	outb(base_addr+0, index_reg);
-	outb((tmp_val >>  0) & 0xff, data_reg);
-	outb(base_addr+1, index_reg);
-	outb((tmp_val >>  8) & 0xff, data_reg);
-	outb(base_addr+2, index_reg);
-	outb((tmp_val >> 16) & 0xff, data_reg);
-	outb(base_addr+3, index_reg);
-	outb((tmp_val >> 24) & 0xff, data_reg);
-
-	if (!request_mem_region_exclusive(val, SP5100_WDT_MEM_MAP_SIZE,
-								   dev_name)) {
-		pr_err("MMIO address 0x%04x already in use\n", val);
-		goto unreg_resource;
-	}
+	pr_notice("failed to find MMIO address, giving up.\n");
+	goto  unreg_region;
 
 setup_wdt:
 	tcobase_phys = val;
@@ -555,9 +456,6 @@ setup_wdt:
 
 unreg_mem_region:
 	release_mem_region(tcobase_phys, SP5100_WDT_MEM_MAP_SIZE);
-unreg_resource:
-	if (resbase_phys)
-		release_resource(&wdt_res);
 unreg_region:
 	release_region(pm_iobase, SP5100_PM_IOPORTS_SIZE);
 exit:
@@ -567,7 +465,6 @@ exit:
 static int sp5100_tco_init(struct platform_device *dev)
 {
 	int ret;
-	char addr_str[16];
 
 	/*
 	 * Check whether or not the hardware watchdog is there. If found, then
@@ -599,23 +496,14 @@ static int sp5100_tco_init(struct platform_device *dev)
 	clear_bit(0, &timer_alive);
 
 	/* Show module parameters */
-	if (force_addr == tcobase_phys)
-		/* The force_addr is vaild */
-		sprintf(addr_str, "0x%04x", force_addr);
-	else
-		strcpy(addr_str, "none");
-
-	pr_info("initialized (0x%p). heartbeat=%d sec (nowayout=%d, "
-		"force_addr=%s)\n",
-		tcobase, heartbeat, nowayout, addr_str);
+	pr_info("initialized (0x%p). heartbeat=%d sec (nowayout=%d)\n",
+		tcobase, heartbeat, nowayout);
 
 	return 0;
 
 exit:
 	iounmap(tcobase);
 	release_mem_region(tcobase_phys, SP5100_WDT_MEM_MAP_SIZE);
-	if (resbase_phys)
-		release_resource(&wdt_res);
 	release_region(pm_iobase, SP5100_PM_IOPORTS_SIZE);
 	return ret;
 }
@@ -630,8 +518,6 @@ static void sp5100_tco_cleanup(void)
 	misc_deregister(&sp5100_tco_miscdev);
 	iounmap(tcobase);
 	release_mem_region(tcobase_phys, SP5100_WDT_MEM_MAP_SIZE);
-	if (resbase_phys)
-		release_resource(&wdt_res);
 	release_region(pm_iobase, SP5100_PM_IOPORTS_SIZE);
 }
 
-- 
1.8.1.4

Re: [PATCH v2] sp5100_tco: Remove code that may cause a boot failure

[Paul Menzel]

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

Dear Takahisa,


thank you for your patch.


Am Sonntag, den 03.03.2013, 14:52 +0900 schrieb Takahisa Tanaka:
> The critical problem which can't boot OS until the power is completely
> cut off found on PC with SB700 chipset.

As commented in my first reply – at least I think – please add the
mainboard model in here too.

Also add the commit (summary and hash) introducing this problem.

> This patch fix the problem, but,

• fix*es*

> this patch prevents the sp5100_tco driver from using watchdog timer
> function of chipset on PC with SP5100 or SB7x0 chipset.

I still do not understand it, as your patches were for SB8x0 support,
right?

> Re-programming the MMIO address registers for the watchdog timer must
> have generated the problem. However, I don't know root cause so far.
> So, I decided to remove the concerned codes.
> 
> Note that this patch is dependent on the commit adbdcc03 and a6e26b13.

Please add the commit summaries, as a lot of people cannot memorize
commit hashes.

Is this patch exactly reverting these two patches?

> Link: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1116835
> Link: https://lkml.org/lkml/2013/2/14/271
> 
> Signed-off-by: Takahisa Tanaka <mc74hc00@gmail.com>

As Linux 3.8 has been released, please add

CC: stable@vger.kernel.org

> ---
> v1 -> v2
>  -The patch was split into the bug fix and the typo fix.

Thanks. Good idea.

> ---
>  drivers/watchdog/sp5100_tco.c | 126 ++----------------------------------------
>  1 file changed, 6 insertions(+), 120 deletions(-)

[…]


Thanks,

Paul

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v2] sp5100_tco: Remove code that may cause a boot failure

[Tanaka Takahisa]

Hi Paul,

Thank you for your advice!
The patch reflecting what was pointed out is posted later.

2013/3/3 Paul Menzel <paulepanter@users.sourceforge.net>:
> I still do not understand it, as your patches were for SB8x0 support,
> right?

There are two methods of accessing the watchdog registers.

 1. Re-programming a resource address obtained by allocate_resource()
to chipset.
 2. Use the direct memory-mapped IO access.

The method 1 can be used by all the chipsets (SP5100, SB7x0, SB8x0 or
later). However, experience shows that only PC with the SB8x0 (or
later) chipsets can use the method 2.

This patch removes the method 1, because the critical problem was found.
That's why the watchdog timer was able to be used on SP5100 and SB7x0
chipsets until now.


> Is this patch exactly reverting these two patches?

Yes. This patch was made on the linux-next brunch (next-20130301).

This patch is dependent on the following patches. So, This patch isn't
applicable to Linux 3.8 kernel which doesn't apply the following patches.

  commit adbdcc030be5f7bb54d229c04efdd0c6b7ed3ab7
      watchdog: sp5100_tco: Write back the original value to reserved
bits, instead of zero

  commit a6e26b1369b34760ca5e1f16039c9018bf68ce7e
      watchdog: sp5100_tco: Fix wrong indirect I/O access for getting
value of reserved bits


> As Linux 3.8 has been released, please add

The above-mentioned two patches may be soon applied to stable kernel (3.8.n).

https://lkml.org/lkml/2013/3/1/546

I thought that I should submit this patch to the stable after these
two patches are merged into stable 3.8.n.


Regards,
Takahisa

Re: [PATCH v2] sp5100_tco: Remove code that may cause a boot failure

[Tanaka Takahisa]

Hi Wim,

I found out the commit of this patch in the linux-next. Thank you for
improving the commit log.
I am sorry for all the trouble I have caused you and Paul.

https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=84a9a694633692d54c8b969664ce8fd115fa09f9


Best regards,
Takahisa