[PATCH] watchdog: sp5100_tco: add reversible legacy MMIO relocation

[PATCH] watchdog: sp5100_tco: add reversible legacy MMIO relocation

[Christoph Berliner]

Older SP5100/SB7x0 systems may provide a watchdog MMIO base that
overlaps another reserved resource. On the tested system firmware
programs the watchdog at 0xfec000f0, which falls inside the IOAPIC
resource and causes sp5100_tco to fail probing with -EBUSY.

A relocation mechanism for this class of conflict was introduced by
commit 740fbddf5c3f ("watchdog: sp5100_tco: Add SB8x0 chipset support")
for Linux 3.8.

It was deliberately removed shortly afterwards by commit 18e4321276fc
("watchdog: sp5100_tco: Remove code that may cause a boot failure")
after an SB700 machine failed to load BIOS after running a kernel
containing the relocation path until power was completely removed.

The exact cause of that failure is not documented. One relevant
difference is that the old relocation path reprogrammed the watchdog
base but did not restore the firmware-programmed PM register state when
the driver was removed.

Add a reversible relocation path for the legacy SP5100 register layout.
Locate the top-level firmware-reserved MMIO resource containing the
conflicting address and search it for a free, naturally aligned 8-byte
child resource.

Use request_resource() for conflict checking instead of
allocate_resource(), since x86 dynamic resource allocation excludes
E820-reserved address ranges.

Before reprogramming the watchdog, save the original PM control and base
registers. Restore them on probe failure or device removal and release
the relocated resource.

On the tested system the complete lifecycle is:

  firmware:  0xfec000f0, PM control 0x03
  relocated: 0xfec00400, PM control 0x06
  restored:  0xfec000f0, PM control 0x03

The relocated resource is registered correctly in /proc/iomem,
the watchdog registers successfully as /dev/watchdog0, and unloading
the module restores both the original firmware address and control
register.

Tested on Linux 7.1.8 with an AMD SBx00 SMBus controller
(PCI 1002:4385, revision 0x3c). The sp5100_tco.c driver in current
mainline is identical to the tested 7.1.8 version.

Signed-off-by: Christoph Berliner <caberliner@gmail.com>
---
Hi Wim, Guenter,

A note on context: I am a physician, not a software developer. This
watchdog issue has annoyed me for quite some time on an older machine
that I still use. Current AI-assisted programming tools finally gave me
a practical way to investigate it in enough depth to trace the firmware
configuration, historical driver behavior and kernel resource handling,
and to build and test a possible fix on the actual hardware.

While investigating the history I found the 2013 SB700 regression that
led to removal of the old relocation path. I do not have the machine
affected by that regression and therefore cannot establish its exact
root cause. The reversible restoration of the firmware-programmed PM
state in this patch is intended to address one important difference
from the old implementation.

I consider this a hardware-tested proposal rather than the work of an
experienced kernel developer. I would particularly appreciate review
of whether restoring the original PM state sufficiently addresses the
risk that led to commit 18e4321276fc ("watchdog: sp5100_tco: Remove
code that may cause a boot failure").

 drivers/watchdog/sp5100_tco.c | 255 +++++++++++++++++++++++++++++++++-
 1 file changed, 251 insertions(+), 4 deletions(-)

diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
index 7e99c3b1f367..d21339d07942 100644
--- a/drivers/watchdog/sp5100_tco.c
+++ b/drivers/watchdog/sp5100_tco.c
@@ -229,6 +229,231 @@ static void tco_timer_enable(struct sp5100_tco *tco)
 	}
 }
 
+static u32 sp5100_tco_prepare_base(struct sp5100_tco *tco,
+				   u32 mmio_addr,
+				   u32 alt_mmio_addr,
+				   const char *dev_name,
+				   bool report_error);
+
+/*
+ * Legacy SP5100/SB7x0 fallback.
+ *
+ * Some firmware uses 0xfec000f0 for the watchdog MMIO window.
+ * This may overlap the IOAPIC resource. Historical Linux versions
+ * relocated the watchdog to a free 8-byte MMIO window and
+ * reprogrammed PM registers 0x6c..0x6f.
+ */
+static void sp5100_tco_write_pm_reg8(u8 index, u8 val)
+{
+	outb(index, SP5100_IO_PM_INDEX_REG);
+	outb(val, SP5100_IO_PM_DATA_REG);
+}
+
+struct sp5100_tco_relocation {
+	struct resource res;
+	u8 saved_control;
+	u8 saved_base[4];
+	bool programmed;
+};
+
+static void sp5100_tco_release_relocation(void *data)
+{
+	struct sp5100_tco_relocation *reloc = data;
+	int i;
+
+	if (reloc->programmed &&
+	    request_muxed_region(SP5100_IO_PM_INDEX_REG,
+				 SP5100_PM_IOPORTS_SIZE,
+				 "sp5100_tco restore")) {
+		/*
+		 * Disable decoding while restoring the original base.
+		 */
+		sp5100_tco_update_pm_reg8(SP5100_PM_WATCHDOG_CONTROL,
+					  0xff,
+					  SP5100_PM_WATCHDOG_DISABLE);
+
+		for (i = 0; i < 4; i++)
+			sp5100_tco_write_pm_reg8(SP5100_PM_WATCHDOG_BASE + i,
+						 reloc->saved_base[i]);
+
+		sp5100_tco_write_pm_reg8(SP5100_PM_WATCHDOG_CONTROL,
+					 reloc->saved_control);
+
+		release_region(SP5100_IO_PM_INDEX_REG,
+			       SP5100_PM_IOPORTS_SIZE);
+	}
+
+	if (reloc->res.parent)
+		release_resource(&reloc->res);
+}
+
+/*
+ * Find the top-level firmware resource containing the conflicting
+ * watchdog address.  The watchdog can then be allocated as a sibling
+ * of resources such as the IOAPIC rather than as a child of them.
+ */
+static struct resource *
+sp5100_tco_find_parent_resource(resource_size_t addr)
+{
+	struct resource *res;
+
+	for (res = iomem_resource.child; res; res = res->sibling) {
+		if (addr >= res->start && addr <= res->end)
+			return res;
+	}
+
+	return NULL;
+}
+
+/*
+ * Older SP5100/SB7x0 firmware may place the watchdog MMIO window inside
+ * another reserved resource, historically most notably the IOAPIC area.
+ *
+ * The old Linux driver could relocate the watchdog when its firmware
+ * supplied address was unusable.  Modern resource trees may expose one
+ * large firmware-reserved parent with individual resources below it.
+ *
+ * Allocate an unused, naturally aligned watchdog-sized child resource
+ * inside that parent and reprogram the SP5100 watchdog base registers.
+ */
+static int sp5100_tco_reprogram_base(struct sp5100_tco *tco,
+				     u32 conflict_addr,
+				     const char *dev_name)
+{
+	struct device *dev = tco->wdd.parent;
+	struct resource *parent;
+	struct sp5100_tco_relocation *reloc;
+	struct resource *res;
+	resource_size_t candidate;
+	resource_size_t max_addr;
+	u32 mmio_addr;
+	u8 base0_reserved;
+	int ret;
+	int i;
+
+	parent = sp5100_tco_find_parent_resource(conflict_addr);
+	if (!parent) {
+		dev_err(dev,
+			"No parent MMIO resource contains conflicting watchdog address 0x%08x\n",
+			conflict_addr);
+		return -ENODEV;
+	}
+
+	/*
+	 * The SP5100 watchdog base is a 32-bit physical address.
+	 */
+	if (parent->start > U32_MAX)
+		return -ERANGE;
+
+	max_addr = min_t(resource_size_t, parent->end, U32_MAX);
+
+	reloc = devm_kzalloc(dev, sizeof(*reloc), GFP_KERNEL);
+	if (!reloc)
+		return -ENOMEM;
+
+	res = &reloc->res;
+	res->name = dev_name;
+	res->flags = IORESOURCE_MEM;
+
+	/*
+	 * Do not use allocate_resource() here.
+	 *
+	 * On x86, dynamic resource allocation excludes E820-reserved
+	 * address ranges. Legacy SP5100 firmware may deliberately place
+	 * the watchdog inside such a reserved parent resource.
+	 *
+	 * Try naturally aligned watchdog-sized slots inside the existing
+	 * parent. request_resource() performs the actual conflict check
+	 * while holding the resource lock.
+	 */
+	candidate = ALIGN(parent->start, SP5100_WDT_MEM_MAP_SIZE);
+
+	for (;;) {
+		if (candidate > max_addr ||
+		    max_addr - candidate + 1 < SP5100_WDT_MEM_MAP_SIZE) {
+			dev_err(dev,
+				"No free watchdog MMIO slot found inside %pR\n",
+				parent);
+			return -EBUSY;
+		}
+
+		res->start = candidate;
+		res->end = candidate + SP5100_WDT_MEM_MAP_SIZE - 1;
+
+		ret = request_resource(parent, res);
+		if (!ret)
+			break;
+
+		if (ret != -EBUSY)
+			return ret;
+
+		candidate += SP5100_WDT_MEM_MAP_SIZE;
+	}
+
+	/*
+	 * Save the firmware-provided watchdog state before changing it.
+	 * The devm cleanup action restores it on probe failure or device
+	 * removal and releases the relocated resource.
+	 */
+	reloc->saved_control =
+		sp5100_tco_read_pm_reg8(SP5100_PM_WATCHDOG_CONTROL);
+
+	for (i = 0; i < 4; i++)
+		reloc->saved_base[i] =
+			sp5100_tco_read_pm_reg8(SP5100_PM_WATCHDOG_BASE + i);
+
+	ret = devm_add_action_or_reset(dev,
+				       sp5100_tco_release_relocation,
+				       reloc);
+	if (ret)
+		return ret;
+
+	mmio_addr = (u32)res->start;
+
+	/*
+	 * Disable the watchdog before changing its decode address.
+	 */
+	sp5100_tco_update_pm_reg8(SP5100_PM_WATCHDOG_CONTROL,
+				  0xff,
+				  SP5100_PM_WATCHDOG_DISABLE);
+
+	/*
+	 * Preserve the reserved low three bits of BASE0 while
+	 * reprogramming the aligned watchdog address.
+	 */
+	base0_reserved = reloc->saved_base[0] & 0x07;
+
+	for (i = 0; i < 4; i++) {
+		u8 val = (mmio_addr >> (8 * i)) & 0xff;
+
+		if (!i)
+			val |= base0_reserved;
+
+		sp5100_tco_write_pm_reg8(SP5100_PM_WATCHDOG_BASE + i, val);
+	}
+
+	reloc->programmed = true;
+
+	dev_info(dev,
+		 "Relocated legacy SP5100 watchdog MMIO from 0x%08x to 0x%08x\n",
+		 conflict_addr, mmio_addr);
+
+	tco->tcobase = devm_ioremap(dev, mmio_addr,
+				    SP5100_WDT_MEM_MAP_SIZE);
+	if (!tco->tcobase) {
+		dev_err(dev,
+			"Relocated watchdog MMIO address 0x%08x failed mapping\n",
+			mmio_addr);
+		return -ENOMEM;
+	}
+
+	dev_info(dev,
+		 "Using relocated 0x%08x for watchdog MMIO address\n",
+		 mmio_addr);
+
+	return 0;
+}
+
 static u32 sp5100_tco_read_pm_reg32(u8 index)
 {
 	u32 val = 0;
@@ -256,7 +481,8 @@ static u32 sp5100_tco_request_region(struct device *dev,
 static u32 sp5100_tco_prepare_base(struct sp5100_tco *tco,
 				   u32 mmio_addr,
 				   u32 alt_mmio_addr,
-				   const char *dev_name)
+				   const char *dev_name,
+				   bool report_error)
 {
 	struct device *dev = tco->wdd.parent;
 
@@ -273,7 +499,9 @@ static u32 sp5100_tco_prepare_base(struct sp5100_tco *tco,
 		mmio_addr = sp5100_tco_request_region(dev, alt_mmio_addr, dev_name);
 
 	if (!mmio_addr) {
-		dev_err(dev, "Failed to reserve MMIO or alternate MMIO region\n");
+		if (report_error)
+			dev_err(dev,
+				"Failed to reserve MMIO or alternate MMIO region\n");
 		return -EBUSY;
 	}
 
@@ -405,7 +633,7 @@ static int sp5100_tco_setupdevice_mmio(struct device *dev,
 		alt_mmio_addr = EFCH_PM_ACPI_MMIO_ADDR +
 			EFCH_PM_ACPI_MMIO_WDT_OFFSET;
 
-	ret = sp5100_tco_prepare_base(tco, mmio_addr, alt_mmio_addr, dev_name);
+	ret = sp5100_tco_prepare_base(tco, mmio_addr, alt_mmio_addr, dev_name, true);
 	if (!ret) {
 		tco_timer_enable_mmio(addr);
 		ret = sp5100_tco_timer_init(tco);
@@ -428,6 +656,8 @@ static int sp5100_tco_setupdevice(struct device *dev,
 	const char *dev_name;
 	u32 mmio_addr = 0, val;
 	u32 alt_mmio_addr = 0;
+	u32 conflict_addr;
+	bool report_error;
 	int ret;
 
 	if (tco->tco_reg_layout == efch_mmio)
@@ -491,7 +721,24 @@ static int sp5100_tco_setupdevice(struct device *dev,
 		return -ENODEV;
 	}
 
-	ret = sp5100_tco_prepare_base(tco, mmio_addr, alt_mmio_addr, dev_name);
+	report_error = tco->tco_reg_layout != sp5100;
+	ret = sp5100_tco_prepare_base(tco, mmio_addr, alt_mmio_addr, dev_name, report_error);
+
+	/*
+	 * Legacy SP5100/SB7x0 firmware commonly uses 0xfec000f0,
+	 * which can overlap the IOAPIC resource. If neither firmware
+	 * MMIO location can be reserved, restore the historical
+	 * relocation fallback.
+	 */
+	if (ret == -EBUSY && tco->tco_reg_layout == sp5100) {
+		conflict_addr = mmio_addr ? mmio_addr : alt_mmio_addr;
+		ret = sp5100_tco_reprogram_base(tco, conflict_addr, dev_name);
+		if (ret)
+			dev_err(dev,
+				"Failed to reserve or relocate watchdog MMIO region: %d\n",
+				ret);
+	}
+
 	if (!ret) {
 		/* Setup the watchdog timer */
 		tco_timer_enable(tco);
-- 
2.55.0

Re: [PATCH] watchdog: sp5100_tco: add reversible legacy MMIO relocation

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:

New issues:
- [High] Lockless traversal of the global `iomem_resource` tree can lead to a Time-of-Check to Time-of-Use (TOCTOU) Use-After-Free (UAF) and system crash.
- [High] Algorithmic Denial of Service (O(N) boot lockup) due to a step-by-8-bytes brute-force resource search loop.
- [High] The watchdog relocation logic can allocate memory inside leaf devices such as the IOAPIC, causing hardware conflicts and immediate system hangs.
- [High] Intermediate MMIO base addresses are left actively decoding during byte-by-byte register writes, risking hardware hangs.
- [High] The patch fails to restore the hardware PM registers on system shutdown/reboot, risking the same BIOS boot failure it aims to prevent.
- [Medium] The relocated watchdog MMIO resource is allocated without the `IORESOURCE_BUSY` flag.

Pre-existing issues:
- [High] Incorrect release of a muxed memory region leaves concurrent waiters sleeping forever.
- [High] The probe function leaks the muxed IO port region lock on an error path.
--

--- Patch [1]: [PATCH] watchdog: sp5100_tco: add reversible legacy MMIO relocation ---
commit acc706a989fc9058e6e8b5ba34a48d8247e973fd
Author: Christoph Berliner <caberliner@gmail.com>
Subject: watchdog: sp5100_tco: add reversible legacy MMIO relocation

This patch adds a reversible relocation path for legacy SP5100 watchdog
registers when they conflict with reserved resources. It attempts to locate an
unreserved child resource inside the conflicting parent, reprogram the
watchdog base address, and uses devres to restore the original state upon
failure or removal.

> diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
> index 2bd3dc25cb030..aafa73bd3be12 100644
> --- a/drivers/watchdog/sp5100_tco.c
> +++ b/drivers/watchdog/sp5100_tco.c
[ ... ]
> +/*
> + * Find the top-level firmware resource containing the conflicting
> + * watchdog address.  The watchdog can then be allocated as a sibling
> + * of resources such as the IOAPIC rather than as a child of them.
> + */
> +static struct resource *
> +sp5100_tco_find_parent_resource(resource_size_t addr)
> +{
> +	struct resource *res;
> +
> +	for (res = iomem_resource.child; res; res = res->sibling) {

[Severity: High]
Is it safe to traverse the iomem_resource.child tree without holding
read_lock(&resource_lock)?

Lockless traversal can race with concurrent resource modifications, potentially
returning an unpinned pointer that could lead to a use-after-free when passed
to request_resource() later.

[Severity: High]
Could this top-level traversal return a leaf device (such as the IOAPIC)
instead of a container?

If it returns a leaf device, request_resource() won't detect any child
conflicts and will succeed, allocating watchdog memory that perfectly overlaps
the IOAPIC. This might cause immediate hardware hangs due to interrupt
starvation.

> +		if (addr >= res->start && addr <= res->end)
> +			return res;
> +	}
> +
> +	return NULL;
> +}
[ ... ]
> +static int sp5100_tco_reprogram_base(struct sp5100_tco *tco,
> +				     u32 conflict_addr,
> +				     const char *dev_name)
> +{
[ ... ]
> +	reloc = devm_kzalloc(dev, sizeof(*reloc), GFP_KERNEL);
> +	if (!reloc)
> +		return -ENOMEM;
> +
> +	res = &reloc->res;
> +	res->name = dev_name;
> +	res->flags = IORESOURCE_MEM;

[Severity: Medium]
Does this resource initialization omit the IORESOURCE_BUSY flag?

Without it, the kernel treats this as a non-leaf container rather than an
exclusively owned region. If another driver calls request_mem_region() for an
overlapping address space, it might incorrectly succeed by carving out a child
resource.

> +	candidate = ALIGN(parent->start, SP5100_WDT_MEM_MAP_SIZE);
> +
> +	for (;;) {
> +		if (candidate > max_addr ||
> +		    max_addr - candidate + 1 < SP5100_WDT_MEM_MAP_SIZE) {
> +			dev_err(dev,
> +				"No free watchdog MMIO slot found inside %pR\n",
> +				parent);
> +			return -EBUSY;
> +		}
> +
> +		res->start = candidate;
> +		res->end = candidate + SP5100_WDT_MEM_MAP_SIZE - 1;
> +
> +		ret = request_resource(parent, res);
> +		if (!ret)
> +			break;
> +
> +		if (ret != -EBUSY)
> +			return ret;
> +
> +		candidate += SP5100_WDT_MEM_MAP_SIZE;
> +	}

[Severity: High]
Does this loop excessively delay boot on large, occupied parent resources?

Blindly incrementing the candidate address by 8 bytes
(SP5100_WDT_MEM_MAP_SIZE) on each request_resource() failure, instead of using
the conflicting resource's end address to skip occupied regions, could result
in millions of loop iterations and acquisitions of the global resource_lock.

> +	reloc->saved_control =
> +		sp5100_tco_read_pm_reg8(SP5100_PM_WATCHDOG_CONTROL);
> +
> +	for (i = 0; i < 4; i++)
> +		reloc->saved_base[i] =
> +			sp5100_tco_read_pm_reg8(SP5100_PM_WATCHDOG_BASE + i);
> +
> +	ret = devm_add_action_or_reset(dev,
> +				       sp5100_tco_release_relocation,
> +				       reloc);

[Severity: High]
Is devm_add_action_or_reset() sufficient for restoring the hardware PM
registers on system shutdown or reboot?

Devres callbacks fire on device unbind or probe failure, but not during
shutdown. Could this leave the MMIO base actively relocated on a warm reboot,
potentially causing the exact BIOS boot failures this patch intends to prevent?

> +	if (ret)
> +		return ret;
> +
> +	mmio_addr = (u32)res->start;
> +
> +	/*
> +	 * Disable the watchdog before changing its decode address.
> +	 */
> +	sp5100_tco_update_pm_reg8(SP5100_PM_WATCHDOG_CONTROL,
> +				  0xff,
> +				  SP5100_PM_WATCHDOG_DISABLE);
> +
> +	/*
> +	 * Preserve the reserved low three bits of BASE0 while
> +	 * reprogramming the aligned watchdog address.
> +	 */
> +	base0_reserved = reloc->saved_base[0] & 0x07;
> +
> +	for (i = 0; i < 4; i++) {
> +		u8 val = (mmio_addr >> (8 * i)) & 0xff;
> +
> +		if (!i)
> +			val |= base0_reserved;
> +
> +		sp5100_tco_write_pm_reg8(SP5100_PM_WATCHDOG_BASE + i, val);
> +	}

[Severity: High]
Are intermediate MMIO base addresses actively decoding during these
byte-by-byte register writes?

For the SP5100/SB7x0 layout, setting SP5100_PM_WATCHDOG_DISABLE disables the
timer, but decoding is controlled by SP5100_PCI_WATCHDOG_DECODE_EN which
appears to remain active. As the base address shifts during the loop, could
DMA or CPU accesses intercept these intermediate addresses and cause hardware
hangs?

[ ... ]
> @@ -404,7 +632,7 @@ static int sp5100_tco_setupdevice_mmio(struct device *dev,
>  		alt_mmio_addr = EFCH_PM_ACPI_MMIO_ADDR +
>  			EFCH_PM_ACPI_MMIO_WDT_OFFSET;
>  
> -	ret = sp5100_tco_prepare_base(tco, mmio_addr, alt_mmio_addr, dev_name);
> +	ret = sp5100_tco_prepare_base(tco, mmio_addr, alt_mmio_addr, dev_name, true);

[Severity: High]
This is a pre-existing issue, but further down in sp5100_tco_setupdevice_mmio()
on the teardown/error path, release_resource(res) is called instead of
release_mem_region(res).

Since the memory was requested with request_mem_region_muxed(), using
release_resource() skips the wake_up(&muxed_resource_wait) call. Could this
leave concurrent waiters sleeping forever?

[ ... ]
> @@ -490,7 +720,24 @@ static int sp5100_tco_setupdevice(struct device *dev,
>  		return -ENODEV;
>  	}

[Severity: High]
This is a pre-existing issue, but the above return -ENODEV occurs when the
switch statement hits the default branch for an unsupported tco_reg_layout.

This exits the probe function without releasing the muxed IO port region
acquired earlier via request_muxed_region(SP5100_IO_PM_INDEX_REG). Does this
lock leak indefinitely starve other drivers requiring the muxed region?

> -	ret = sp5100_tco_prepare_base(tco, mmio_addr, alt_mmio_addr, dev_name);
> +	report_error = tco->tco_reg_layout != sp5100;
> +	ret = sp5100_tco_prepare_base(tco, mmio_addr, alt_mmio_addr, dev_name, report_error);
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260820222541.16494-1-caberliner@gmail.com?part=1

[PATCH v2] watchdog: sp5100_tco: add reversible legacy MMIO relocation

[Christoph Berliner]

Older SP5100/SB7x0 systems may provide a watchdog MMIO base that
overlaps another reserved resource. On the tested system firmware
programs the watchdog at 0xfec000f0, which falls inside the IOAPIC
resource and causes sp5100_tco to fail probing with -EBUSY.

A relocation mechanism for this class of conflict was introduced by
commit 740fbddf5c3f ("watchdog: sp5100_tco: Add SB8x0 chipset support")
for Linux 3.8.

It was deliberately removed shortly afterwards by commit 18e4321276fc
("watchdog: sp5100_tco: Remove code that may cause a boot failure")
after an SB700 machine failed to load BIOS after running a kernel
containing the relocation path until power was completely removed.

The exact cause of that failure is not documented. One relevant
difference is that the old relocation path reprogrammed the watchdog
base but did not restore the firmware-programmed PM register state when
the driver was removed.

Add a reversible relocation path for the legacy SP5100 register layout.

When the firmware-provided watchdog address cannot be reserved, search a
bounded 4 KiB range above it for a free, naturally aligned 8-byte MMIO
window. Use devm_request_mem_region() for each candidate so resource-tree
traversal, conflict checking and IORESOURCE_BUSY registration are handled
by the resource core under its lock.

Before reprogramming the watchdog, save the original PM control and base
registers as well as the PCI watchdog decode register. Disable PCI MMIO
decode and the watchdog timer before changing the byte-wise base
registers, avoiding transient decoded addresses while BASE0..3 are being
updated.

Restore the original firmware PM and PCI state on probe failure or device
removal and release the relocated MMIO resource through devres.

On the tested system the complete load/unload lifecycle is:

  firmware:  0xfec000f0, PM control 0x03
  relocated: 0xfec00400, PM control 0x06
  restored:  0xfec000f0, PM control 0x03

The relocated range appears as an exclusive SP5100 TCO resource in
/proc/iomem, /dev/watchdog0 registers successfully, and unloading the
module restores the original firmware state and removes the resource.

A warm reboot with the relocated watchdog loaded was also tested
successfully; the machine booted normally and the firmware watchdog state
after reboot was again 0xfec000f0 with PM control 0x03.

Tested on Linux 7.1.8 with an AMD SBx00 SMBus controller
(PCI 1002:4385, revision 0x3c). The sp5100_tco.c driver in current
mainline is identical to the tested 7.1.8 version.

Signed-off-by: Christoph Berliner <caberliner@gmail.com>
---
Changes in v2:
- replace direct iomem resource-tree traversal with
  devm_request_mem_region(), providing locked conflict handling and
  IORESOURCE_BUSY semantics
- bound the relocation search to a 4 KiB window
- disable PCI watchdog MMIO decode while rewriting BASE0..3
- save and restore the original PCI watchdog decode register
- retain reversible restoration on probe failure and device removal
- hardware-test the updated load/unload lifecycle and a warm reboot

 drivers/watchdog/sp5100_tco.c | 239 +++++++++++++++++++++++++++++++++-
 1 file changed, 235 insertions(+), 4 deletions(-)

diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
index 7e99c3b1f367..dbe6946d8f95 100644
--- a/drivers/watchdog/sp5100_tco.c
+++ b/drivers/watchdog/sp5100_tco.c
@@ -229,6 +229,215 @@ static void tco_timer_enable(struct sp5100_tco *tco)
 	}
 }
 
+static u32 sp5100_tco_prepare_base(struct sp5100_tco *tco,
+				   u32 mmio_addr,
+				   u32 alt_mmio_addr,
+				   const char *dev_name,
+				   bool report_error);
+
+#define SP5100_WDT_RELOCATION_RANGE	0x1000
+
+/*
+ * Legacy SP5100/SB7x0 fallback.
+ *
+ * Some firmware uses 0xfec000f0 for the watchdog MMIO window.
+ * This may overlap the IOAPIC resource. Historical Linux versions
+ * relocated the watchdog and reprogrammed PM registers 0x6c..0x6f.
+ */
+static void sp5100_tco_write_pm_reg8(u8 index, u8 val)
+{
+	outb(index, SP5100_IO_PM_INDEX_REG);
+	outb(val, SP5100_IO_PM_DATA_REG);
+}
+
+struct sp5100_tco_relocation {
+	u32 saved_pci_misc;
+	u8 saved_control;
+	u8 saved_base[4];
+	bool programmed;
+	bool restored;
+};
+
+static void sp5100_tco_restore_relocation(struct sp5100_tco_relocation *reloc)
+{
+	u32 val;
+	int i;
+
+	if (!reloc || !reloc->programmed || reloc->restored)
+		return;
+
+	if (!request_muxed_region(SP5100_IO_PM_INDEX_REG,
+				  SP5100_PM_IOPORTS_SIZE,
+				  "sp5100_tco restore"))
+		return;
+
+	/*
+	 * MMIO decode is controlled independently of the PM watchdog-disable
+	 * bit. Disable decode before changing the byte-wise base registers so
+	 * that no intermediate address can become active.
+	 */
+	pci_read_config_dword(sp5100_tco_pci,
+			      SP5100_PCI_WATCHDOG_MISC_REG, &val);
+	pci_write_config_dword(sp5100_tco_pci,
+			       SP5100_PCI_WATCHDOG_MISC_REG,
+			       val & ~SP5100_PCI_WATCHDOG_DECODE_EN);
+
+	sp5100_tco_update_pm_reg8(SP5100_PM_WATCHDOG_CONTROL,
+				  0xff,
+				  SP5100_PM_WATCHDOG_DISABLE);
+
+	for (i = 0; i < 4; i++)
+		sp5100_tco_write_pm_reg8(SP5100_PM_WATCHDOG_BASE + i,
+					 reloc->saved_base[i]);
+
+	sp5100_tco_write_pm_reg8(SP5100_PM_WATCHDOG_CONTROL,
+				 reloc->saved_control);
+
+	/*
+	 * Restore the firmware-programmed PCI state only after the original
+	 * watchdog base and PM control register have been restored.
+	 */
+	pci_write_config_dword(sp5100_tco_pci,
+			       SP5100_PCI_WATCHDOG_MISC_REG,
+			       reloc->saved_pci_misc);
+
+	reloc->restored = true;
+
+	release_region(SP5100_IO_PM_INDEX_REG,
+		       SP5100_PM_IOPORTS_SIZE);
+}
+
+static void sp5100_tco_release_relocation(void *data)
+{
+	struct sp5100_tco_relocation *reloc = data;
+
+	sp5100_tco_restore_relocation(reloc);
+}
+
+/*
+ * Older SP5100/SB7x0 firmware may place the watchdog MMIO window inside
+ * another reserved resource, historically most notably the IOAPIC area.
+ *
+ * E820 reserved device address ranges are represented as non-busy
+ * containers in the iomem tree. devm_request_mem_region() performs the
+ * resource-tree traversal and conflict checking under resource_lock and
+ * creates an IORESOURCE_BUSY child for a successful reservation.
+ *
+ * Keep the search deliberately local to the firmware-provided address
+ * instead of walking an arbitrarily large reserved address range.
+ */
+static int sp5100_tco_reprogram_base(struct sp5100_tco *tco,
+				     u32 conflict_addr,
+				     const char *dev_name)
+{
+	struct device *dev = tco->wdd.parent;
+	struct sp5100_tco_relocation *reloc;
+	struct resource *res = NULL;
+	resource_size_t candidate;
+	resource_size_t search_end;
+	u32 mmio_addr;
+	u8 base0_reserved;
+	int ret;
+	int i;
+
+	reloc = devm_kzalloc(dev, sizeof(*reloc), GFP_KERNEL);
+	if (!reloc)
+		return -ENOMEM;
+
+	search_end = min_t(resource_size_t, U32_MAX,
+			   (resource_size_t)conflict_addr +
+			   SP5100_WDT_RELOCATION_RANGE);
+
+	candidate = ALIGN((resource_size_t)conflict_addr +
+			  SP5100_WDT_MEM_MAP_SIZE,
+			  SP5100_WDT_MEM_MAP_SIZE);
+
+	for (; candidate <= search_end &&
+	     search_end - candidate + 1 >= SP5100_WDT_MEM_MAP_SIZE;
+	     candidate += SP5100_WDT_MEM_MAP_SIZE) {
+		res = devm_request_mem_region(dev, candidate,
+					      SP5100_WDT_MEM_MAP_SIZE,
+					     dev_name);
+		if (res)
+			break;
+	}
+
+	if (!res) {
+		dev_err(dev,
+			"No free watchdog MMIO slot found within 0x%x bytes of 0x%08x\n",
+			SP5100_WDT_RELOCATION_RANGE, conflict_addr);
+		return -EBUSY;
+	}
+
+	mmio_addr = (u32)res->start;
+
+	/*
+	 * Save all firmware-programmed watchdog and decode state before
+	 * changing anything.
+	 */
+	reloc->saved_control =
+		sp5100_tco_read_pm_reg8(SP5100_PM_WATCHDOG_CONTROL);
+
+	for (i = 0; i < 4; i++)
+		reloc->saved_base[i] =
+			sp5100_tco_read_pm_reg8(SP5100_PM_WATCHDOG_BASE + i);
+
+	pci_read_config_dword(sp5100_tco_pci,
+			      SP5100_PCI_WATCHDOG_MISC_REG,
+			      &reloc->saved_pci_misc);
+
+	ret = devm_add_action_or_reset(dev,
+				       sp5100_tco_release_relocation,
+				       reloc);
+	if (ret)
+		return ret;
+
+	/*
+	 * Disable MMIO decode before byte-wise modification of BASE0..3.
+	 */
+	pci_write_config_dword(sp5100_tco_pci,
+			       SP5100_PCI_WATCHDOG_MISC_REG,
+			       reloc->saved_pci_misc &
+			       ~SP5100_PCI_WATCHDOG_DECODE_EN);
+
+	/* Stop the watchdog timer as well before changing its base. */
+	sp5100_tco_update_pm_reg8(SP5100_PM_WATCHDOG_CONTROL,
+				  0xff,
+				  SP5100_PM_WATCHDOG_DISABLE);
+
+	base0_reserved = reloc->saved_base[0] & 0x07;
+
+	for (i = 0; i < 4; i++) {
+		u8 val = (mmio_addr >> (8 * i)) & 0xff;
+
+		if (!i)
+			val |= base0_reserved;
+
+		sp5100_tco_write_pm_reg8(SP5100_PM_WATCHDOG_BASE + i, val);
+	}
+
+	reloc->programmed = true;
+
+	dev_info(dev,
+		 "Relocated legacy SP5100 watchdog MMIO from 0x%08x to 0x%08x\n",
+		 conflict_addr, mmio_addr);
+
+	tco->tcobase = devm_ioremap(dev, mmio_addr,
+				    SP5100_WDT_MEM_MAP_SIZE);
+	if (!tco->tcobase) {
+		dev_err(dev,
+			"Relocated watchdog MMIO address 0x%08x failed mapping\n",
+			mmio_addr);
+		return -ENOMEM;
+	}
+
+	dev_info(dev,
+		 "Using relocated 0x%08x for watchdog MMIO address\n",
+		 mmio_addr);
+
+	return 0;
+}
+
 static u32 sp5100_tco_read_pm_reg32(u8 index)
 {
 	u32 val = 0;
@@ -256,7 +465,8 @@ static u32 sp5100_tco_request_region(struct device *dev,
 static u32 sp5100_tco_prepare_base(struct sp5100_tco *tco,
 				   u32 mmio_addr,
 				   u32 alt_mmio_addr,
-				   const char *dev_name)
+				   const char *dev_name,
+				   bool report_error)
 {
 	struct device *dev = tco->wdd.parent;
 
@@ -273,7 +483,9 @@ static u32 sp5100_tco_prepare_base(struct sp5100_tco *tco,
 		mmio_addr = sp5100_tco_request_region(dev, alt_mmio_addr, dev_name);
 
 	if (!mmio_addr) {
-		dev_err(dev, "Failed to reserve MMIO or alternate MMIO region\n");
+		if (report_error)
+			dev_err(dev,
+				"Failed to reserve MMIO or alternate MMIO region\n");
 		return -EBUSY;
 	}
 
@@ -405,7 +617,7 @@ static int sp5100_tco_setupdevice_mmio(struct device *dev,
 		alt_mmio_addr = EFCH_PM_ACPI_MMIO_ADDR +
 			EFCH_PM_ACPI_MMIO_WDT_OFFSET;
 
-	ret = sp5100_tco_prepare_base(tco, mmio_addr, alt_mmio_addr, dev_name);
+	ret = sp5100_tco_prepare_base(tco, mmio_addr, alt_mmio_addr, dev_name, true);
 	if (!ret) {
 		tco_timer_enable_mmio(addr);
 		ret = sp5100_tco_timer_init(tco);
@@ -428,6 +640,8 @@ static int sp5100_tco_setupdevice(struct device *dev,
 	const char *dev_name;
 	u32 mmio_addr = 0, val;
 	u32 alt_mmio_addr = 0;
+	u32 conflict_addr;
+	bool report_error;
 	int ret;
 
 	if (tco->tco_reg_layout == efch_mmio)
@@ -491,7 +705,24 @@ static int sp5100_tco_setupdevice(struct device *dev,
 		return -ENODEV;
 	}
 
-	ret = sp5100_tco_prepare_base(tco, mmio_addr, alt_mmio_addr, dev_name);
+	report_error = tco->tco_reg_layout != sp5100;
+	ret = sp5100_tco_prepare_base(tco, mmio_addr, alt_mmio_addr, dev_name, report_error);
+
+	/*
+	 * Legacy SP5100/SB7x0 firmware commonly uses 0xfec000f0,
+	 * which can overlap the IOAPIC resource. If neither firmware
+	 * MMIO location can be reserved, restore the historical
+	 * relocation fallback.
+	 */
+	if (ret == -EBUSY && tco->tco_reg_layout == sp5100) {
+		conflict_addr = mmio_addr ? mmio_addr : alt_mmio_addr;
+		ret = sp5100_tco_reprogram_base(tco, conflict_addr, dev_name);
+		if (ret)
+			dev_err(dev,
+				"Failed to reserve or relocate watchdog MMIO region: %d\n",
+				ret);
+	}
+
 	if (!ret) {
 		/* Setup the watchdog timer */
 		tco_timer_enable(tco);
-- 
2.55.0

Re: [PATCH v2] watchdog: sp5100_tco: add reversible legacy MMIO relocation

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Hardware register restoration relies on a devres action which is not executed during system reboot or shutdown, causing SB700 machines to hang on warm reboot.
- [High] Unaligned PCI configuration dword accesses cause watchdog decode disable to silently fail, leaving intermediate addresses decoded and reading uninitialized stack memory.
--

--- Patch [1]: [PATCH v2] watchdog: sp5100_tco: add reversible legacy MMIO relocation ---
commit 3a0c050c3a37ed10584cd869f59392a7482a02f9
Author: Christoph Berliner <caberliner@gmail.com>

watchdog: sp5100_tco: add reversible legacy MMIO relocation

This commit adds a reversible relocation path for the legacy SP5100
register layout when the firmware-provided watchdog address cannot be
reserved. It searches for a free MMIO window, saves the original state,
and restores the firmware state on probe failure or device removal.

> diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
> index 2bd3dc25cb030..99b2f5787b6cd 100644
> --- a/drivers/watchdog/sp5100_tco.c
> +++ b/drivers/watchdog/sp5100_tco.c
[ ... ]
> @@ -228,6 +228,215 @@ static void tco_timer_enable(struct sp5100_tco *tco)
>  	}
>  }
>  
> +static u32 sp5100_tco_prepare_base(struct sp5100_tco *tco,
> +				   u32 mmio_addr,
> +				   u32 alt_mmio_addr,
> +				   const char *dev_name,
> +				   bool report_error);
> +
> +#define SP5100_WDT_RELOCATION_RANGE	0x1000
> +
> +/*
> + * Legacy SP5100/SB7x0 fallback.
> + *
> + * Some firmware uses 0xfec000f0 for the watchdog MMIO window.
> + * This may overlap the IOAPIC resource. Historical Linux versions
> + * relocated the watchdog and reprogrammed PM registers 0x6c..0x6f.
> + */
> +static void sp5100_tco_write_pm_reg8(u8 index, u8 val)
> +{
> +	outb(index, SP5100_IO_PM_INDEX_REG);
> +	outb(val, SP5100_IO_PM_DATA_REG);
> +}
> +
> +struct sp5100_tco_relocation {
> +	u32 saved_pci_misc;
> +	u8 saved_control;
> +	u8 saved_base[4];
> +	bool programmed;
> +	bool restored;
> +};
> +
> +static void sp5100_tco_restore_relocation(struct sp5100_tco_relocation *reloc)
> +{
> +	u32 val;
> +	int i;
> +
> +	if (!reloc || !reloc->programmed || reloc->restored)
> +		return;
> +
> +	if (!request_muxed_region(SP5100_IO_PM_INDEX_REG,
> +				  SP5100_PM_IOPORTS_SIZE,
> +				  "sp5100_tco restore"))
> +		return;
> +
> +	/*
> +	 * MMIO decode is controlled independently of the PM watchdog-disable
> +	 * bit. Disable decode before changing the byte-wise base registers so
> +	 * that no intermediate address can become active.
> +	 */
> +	pci_read_config_dword(sp5100_tco_pci,
> +			      SP5100_PCI_WATCHDOG_MISC_REG, &val);
> +	pci_write_config_dword(sp5100_tco_pci,
> +			       SP5100_PCI_WATCHDOG_MISC_REG,
> +			       val & ~SP5100_PCI_WATCHDOG_DECODE_EN);

[Severity: High]
Since SP5100_PCI_WATCHDOG_MISC_REG is 0x41, which is not 4-byte aligned, will
pci_read_config_dword() silently return PCIBIOS_BAD_REGISTER_NUMBER?

If so, does this leave the stack variable 'val' uninitialized and cause the
subsequent pci_write_config_dword() to write garbage back to the PCI
configuration space?

[ ... ]
> +static int sp5100_tco_reprogram_base(struct sp5100_tco *tco,
> +				     u32 conflict_addr,
> +				     const char *dev_name)
> +{
> +	struct device *dev = tco->wdd.parent;
> +	struct sp5100_tco_relocation *reloc;
> +	struct resource *res = NULL;
[ ... ]
> +	/*
> +	 * Save all firmware-programmed watchdog and decode state before
> +	 * changing anything.
> +	 */
> +	reloc->saved_control =
> +		sp5100_tco_read_pm_reg8(SP5100_PM_WATCHDOG_CONTROL);
> +
> +	for (i = 0; i < 4; i++)
> +		reloc->saved_base[i] =
> +			sp5100_tco_read_pm_reg8(SP5100_PM_WATCHDOG_BASE + i);
> +
> +	pci_read_config_dword(sp5100_tco_pci,
> +			      SP5100_PCI_WATCHDOG_MISC_REG,
> +			      &reloc->saved_pci_misc);
> +
> +	ret = devm_add_action_or_reset(dev,
> +				       sp5100_tco_release_relocation,
> +				       reloc);

[Severity: High]
Since devres callbacks like sp5100_tco_release_relocation() are only executed
during driver unbind, and not during system shutdown or reboot, will the
watchdog MMIO remain relocated in hardware during a reboot?

Because the platform driver lacks a .shutdown callback, could this cause SB700
machines to hang on warm reboot?

> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Disable MMIO decode before byte-wise modification of BASE0..3.
> +	 */
> +	pci_write_config_dword(sp5100_tco_pci,
> +			       SP5100_PCI_WATCHDOG_MISC_REG,
> +			       reloc->saved_pci_misc &
> +			       ~SP5100_PCI_WATCHDOG_DECODE_EN);

[Severity: High]
As mentioned earlier, does this unaligned pci_write_config_dword() silently
fail?

Could this leave intermediate addresses decoded while the byte-wise base
registers are rewritten, breaking the intended hardware isolation?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260820231409.8427-1-caberliner@gmail.com?part=1

Re: [PATCH v2] watchdog: sp5100_tco: add reversible legacy MMIO relocation

[Guenter Roeck]

On 8/20/26 16:14, Christoph Berliner wrote:
> Older SP5100/SB7x0 systems may provide a watchdog MMIO base that
> overlaps another reserved resource. On the tested system firmware
> programs the watchdog at 0xfec000f0, which falls inside the IOAPIC
> resource and causes sp5100_tco to fail probing with -EBUSY.
> 
> A relocation mechanism for this class of conflict was introduced by
> commit 740fbddf5c3f ("watchdog: sp5100_tco: Add SB8x0 chipset support")
> for Linux 3.8.
> 
> It was deliberately removed shortly afterwards by commit 18e4321276fc
> ("watchdog: sp5100_tco: Remove code that may cause a boot failure")
> after an SB700 machine failed to load BIOS after running a kernel
> containing the relocation path until power was completely removed.
> 
> The exact cause of that failure is not documented. One relevant
> difference is that the old relocation path reprogrammed the watchdog
> base but did not restore the firmware-programmed PM register state when
> the driver was removed.
> 
> Add a reversible relocation path for the legacy SP5100 register layout.
> 
> When the firmware-provided watchdog address cannot be reserved, search a
> bounded 4 KiB range above it for a free, naturally aligned 8-byte MMIO
> window. Use devm_request_mem_region() for each candidate so resource-tree
> traversal, conflict checking and IORESOURCE_BUSY registration are handled
> by the resource core under its lock.
> 
> Before reprogramming the watchdog, save the original PM control and base
> registers as well as the PCI watchdog decode register. Disable PCI MMIO
> decode and the watchdog timer before changing the byte-wise base
> registers, avoiding transient decoded addresses while BASE0..3 are being
> updated.
> 
> Restore the original firmware PM and PCI state on probe failure or device
> removal and release the relocated MMIO resource through devres.
> 
> On the tested system the complete load/unload lifecycle is:
> 
>    firmware:  0xfec000f0, PM control 0x03
>    relocated: 0xfec00400, PM control 0x06
>    restored:  0xfec000f0, PM control 0x03
> 
> The relocated range appears as an exclusive SP5100 TCO resource in
> /proc/iomem, /dev/watchdog0 registers successfully, and unloading the
> module restores the original firmware state and removes the resource.
> 
> A warm reboot with the relocated watchdog loaded was also tested
> successfully; the machine booted normally and the firmware watchdog state
> after reboot was again 0xfec000f0 with PM control 0x03.
> 
> Tested on Linux 7.1.8 with an AMD SBx00 SMBus controller
> (PCI 1002:4385, revision 0x3c). The sp5100_tco.c driver in current
> mainline is identical to the tested 7.1.8 version.
> 
> Signed-off-by: Christoph Berliner <caberliner@gmail.com>

Your solution is way too risky. Try to find one that lets the driver use the
region without reserving it, specifically suited for the system affected
by the problem. You should be able to identify that system using DMI.

Thanks,
Guenter