[PATCH 0/3] Fix sp5100_tco watchdog driver regression

[PATCH 0/3] Fix sp5100_tco watchdog driver regression

[Zoltán Böszörményi]

Three drivers are accessing the same I/O ports (0xcd6 / 0xcd7) on
AMD SB800 based machines without synchronization or with excluding
each other out:
* the USB quirk for isochronous transfers on SB800 (no locking)
* sp5100_tco (request_region)
* i2c-piix4 (request_region)

Historically, the sp5100_tco watchdog driver used request_region()
for these I/O ports but an i2c-piix4 improvement for SB800 in
Linux 4.4-rc4 also added a request_region() call. Because of this
and the load order of these drivers, this caused a regression and
the watchdog function became non-functional. The commit that caused
the regression is:

commit 2fee61d22e606fc99ade9079fda15fdee83ec33e
Author: Christian Fetzer <fetzer.ch@gmail.com>
Date:   Thu Nov 19 20:13:48 2015 +0100

    i2c: piix4: Add support for multiplexed main adapter in SB800

I was informed by Guenter Roeck <linux@roeck-us.net> that the
alternative, i.e. using request_muxed_region() can fail, either
because of a resource allocation failure, which is quite possible
with long uptimes, or because there are no guarantees that an as yet
unknown driver would also use request_muxed_region() consistently.

Because of this, a common mutex across these driver was chosen to
synchronize I/O port accesses and request_region() calls are removed
from both i2c-piix4 and sp5100_tco to make the code uniform.

This patch series implements this and restores the watchdog function.

Signed-off-by: Zoltán Böszörményi <zboszor@pr.hu>

 drivers/i2c/busses/i2c-piix4.c | 59 ++++++++++++--------------------------
 drivers/usb/host/pci-quirks.c  | 14 ++++++---
 drivers/watchdog/sp5100_tco.c  | 24 +++++++---------
 drivers/watchdog/sp5100_tco.h  | 10 ++++---
 include/linux/sb800.h          | 15 ++++++++++
 5 files changed, 61 insertions(+), 61 deletions(-)

[PATCH 1/3] usb: pci-quirks: Add a header for SB800 I/O ports and mutex for locking

[Zoltán Böszörményi]

From: Böszörményi Zoltán <zboszor@pr.hu>

This patch adds:
* a mutex in the USB PCI quirks code for synchronizing access to
  the I/O ports on SB800
* a new header that contains symbols for the index and data I/O ports
  and wrappers for locking and unlocking the mutex.
* locking around the I/O port access for SB800

Signed-off-by: Zoltán Böszörményi <zboszor@pr.hu>
---
 drivers/usb/host/pci-quirks.c | 14 ++++++++++----
 include/linux/sb800.h         | 15 +++++++++++++++
 2 files changed, 25 insertions(+), 4 deletions(-)
 create mode 100644 include/linux/sb800.h

diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
index a9a1e4c..9b0445c 100644
--- a/drivers/usb/host/pci-quirks.c
+++ b/drivers/usb/host/pci-quirks.c
@@ -15,6 +15,7 @@
 #include <linux/export.h>
 #include <linux/acpi.h>
 #include <linux/dmi.h>
+#include <linux/sb800.h>
 #include "pci-quirks.h"
 #include "xhci-ext-caps.h"
 
@@ -279,6 +280,9 @@ bool usb_amd_prefetch_quirk(void)
 }
 EXPORT_SYMBOL_GPL(usb_amd_prefetch_quirk);
 
+DEFINE_MUTEX(sb800_mutex);
+EXPORT_SYMBOL_GPL(sb800_mutex);
+
 /*
  * The hardware normally enables the A-link power management feature, which
  * lets the system lower the power consumption in idle states.
@@ -314,11 +318,13 @@ static void usb_amd_quirk_pll(int disable)
 	if (amd_chipset.sb_type.gen == AMD_CHIPSET_SB800 ||
 			amd_chipset.sb_type.gen == AMD_CHIPSET_HUDSON2 ||
 			amd_chipset.sb_type.gen == AMD_CHIPSET_BOLTON) {
-		outb_p(AB_REG_BAR_LOW, 0xcd6);
-		addr_low = inb_p(0xcd7);
-		outb_p(AB_REG_BAR_HIGH, 0xcd6);
-		addr_high = inb_p(0xcd7);
+		enter_sb800();
+		outb_p(AB_REG_BAR_LOW, SB800_PIIX4_SMB_IDX);
+		addr_low = inb_p(SB800_PIIX4_SMB_DATA);
+		outb_p(AB_REG_BAR_HIGH, SB800_PIIX4_SMB_IDX);
+		addr_high = inb_p(SB800_PIIX4_SMB_DATA);
 		addr = addr_high << 8 | addr_low;
+		leave_sb800();
 
 		outl_p(0x30, AB_INDX(addr));
 		outl_p(0x40, AB_DATA(addr));
diff --git a/include/linux/sb800.h b/include/linux/sb800.h
new file mode 100644
index 0000000..5650b7d
--- /dev/null
+++ b/include/linux/sb800.h
@@ -0,0 +1,15 @@
+
+#ifndef SB800_H
+#define SB800_H
+
+#include <linux/mutex.h>
+
+#define SB800_PIIX4_SMB_IDX		0xcd6
+#define SB800_PIIX4_SMB_DATA	0xcd7
+
+extern struct mutex sb800_mutex;
+
+#define enter_sb800()	mutex_lock(&sb800_mutex)
+#define leave_sb800()	mutex_unlock(&sb800_mutex)
+
+#endif
-- 
2.9.3

[PATCH 3/3] watchdog: sp5100_tco: Synchronize I/O port accesses

[Zoltán Böszörményi]

From: Böszörményi Zoltán <zboszor@pr.hu>

Use the new header and the mutex from usb/host/pci-quirks.c

This change will allow accesses to the SB800 I/O port pair
synchronized with the PCI quirk when isochronous USB transfers
are performed and with i2c-piix4.

At the same time, remove the request_region() call to reserve
these I/O ports, similarly to i2c-piix4 so the code is now uniform
across the three individual drivers.

Signed-off-by: Zoltán Böszörményi <zboszor@pr.hu>
---
 drivers/watchdog/sp5100_tco.c | 24 +++++++++++-------------
 drivers/watchdog/sp5100_tco.h | 10 ++++++----
 2 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
index 028618c..dc125df 100644
--- a/drivers/watchdog/sp5100_tco.c
+++ b/drivers/watchdog/sp5100_tco.c
@@ -48,7 +48,6 @@
 static u32 tcobase_phys;
 static u32 tco_wdt_fired;
 static void __iomem *tcobase;
-static unsigned int pm_iobase;
 static DEFINE_SPINLOCK(tco_lock);	/* Guards the hardware */
 static unsigned long timer_alive;
 static char tco_expect_close;
@@ -138,6 +137,8 @@ static void tco_timer_enable(void)
 
 	if (!tco_has_sp5100_reg_layout(sp5100_tco_pci)) {
 		/* For SB800 or later */
+		enter_sb800();
+
 		/* Set the Watchdog timer resolution to 1 sec */
 		outb(SB800_PM_WATCHDOG_CONFIG, SB800_IO_PM_INDEX_REG);
 		val = inb(SB800_IO_PM_DATA_REG);
@@ -150,6 +151,8 @@ static void tco_timer_enable(void)
 		val |= SB800_PCI_WATCHDOG_DECODE_EN;
 		val &= ~SB800_PM_WATCHDOG_DISABLE;
 		outb(val, SB800_IO_PM_DATA_REG);
+
+		leave_sb800();
 	} else {
 		/* For SP5100 or SB7x0 */
 		/* Enable watchdog decode bit */
@@ -164,11 +167,13 @@ static void tco_timer_enable(void)
 				       val);
 
 		/* Enable Watchdog timer and set the resolution to 1 sec */
+		enter_sb800();
 		outb(SP5100_PM_WATCHDOG_CONTROL, SP5100_IO_PM_INDEX_REG);
 		val = inb(SP5100_IO_PM_DATA_REG);
 		val |= SP5100_PM_WATCHDOG_SECOND_RES;
 		val &= ~SP5100_PM_WATCHDOG_DISABLE;
 		outb(val, SP5100_IO_PM_DATA_REG);
+		leave_sb800();
 	}
 }
 
@@ -361,16 +366,10 @@ static unsigned char sp5100_tco_setupdevice(void)
 		base_addr = SB800_PM_WATCHDOG_BASE;
 	}
 
-	/* Request the IO ports used by this driver */
-	pm_iobase = SP5100_IO_PM_INDEX_REG;
-	if (!request_region(pm_iobase, SP5100_PM_IOPORTS_SIZE, dev_name)) {
-		pr_err("I/O address 0x%04x already in use\n", pm_iobase);
-		goto exit;
-	}
-
 	/*
 	 * First, Find the watchdog timer MMIO address from indirect I/O.
 	 */
+	enter_sb800();
 	outb(base_addr+3, index_reg);
 	val = inb(data_reg);
 	outb(base_addr+2, index_reg);
@@ -380,6 +379,7 @@ static unsigned char sp5100_tco_setupdevice(void)
 	outb(base_addr+0, index_reg);
 	/* Low three bits of BASE are reserved */
 	val = val << 8 | (inb(data_reg) & 0xf8);
+	leave_sb800();
 
 	pr_debug("Got 0x%04x from indirect I/O\n", val);
 
@@ -400,6 +400,7 @@ static unsigned char sp5100_tco_setupdevice(void)
 				      SP5100_SB_RESOURCE_MMIO_BASE, &val);
 	} else {
 		/* Read SBResource_MMIO from AcpiMmioEn(PM_Reg: 24h) */
+		enter_sb800();
 		outb(SB800_PM_ACPI_MMIO_EN+3, SB800_IO_PM_INDEX_REG);
 		val = inb(SB800_IO_PM_DATA_REG);
 		outb(SB800_PM_ACPI_MMIO_EN+2, SB800_IO_PM_INDEX_REG);
@@ -408,6 +409,7 @@ static unsigned char sp5100_tco_setupdevice(void)
 		val = val << 8 | inb(SB800_IO_PM_DATA_REG);
 		outb(SB800_PM_ACPI_MMIO_EN+0, SB800_IO_PM_INDEX_REG);
 		val = val << 8 | inb(SB800_IO_PM_DATA_REG);
+		leave_sb800();
 	}
 
 	/* The SBResource_MMIO is enabled and mapped memory space? */
@@ -429,7 +431,7 @@ static unsigned char sp5100_tco_setupdevice(void)
 		pr_debug("SBResource_MMIO is disabled(0x%04x)\n", val);
 
 	pr_notice("failed to find MMIO address, giving up.\n");
-	goto  unreg_region;
+	goto  exit;
 
 setup_wdt:
 	tcobase_phys = val;
@@ -469,8 +471,6 @@ static unsigned char sp5100_tco_setupdevice(void)
 
 unreg_mem_region:
 	release_mem_region(tcobase_phys, SP5100_WDT_MEM_MAP_SIZE);
-unreg_region:
-	release_region(pm_iobase, SP5100_PM_IOPORTS_SIZE);
 exit:
 	return 0;
 }
@@ -517,7 +517,6 @@ static int sp5100_tco_init(struct platform_device *dev)
 exit:
 	iounmap(tcobase);
 	release_mem_region(tcobase_phys, SP5100_WDT_MEM_MAP_SIZE);
-	release_region(pm_iobase, SP5100_PM_IOPORTS_SIZE);
 	return ret;
 }
 
@@ -531,7 +530,6 @@ static void sp5100_tco_cleanup(void)
 	misc_deregister(&sp5100_tco_miscdev);
 	iounmap(tcobase);
 	release_mem_region(tcobase_phys, SP5100_WDT_MEM_MAP_SIZE);
-	release_region(pm_iobase, SP5100_PM_IOPORTS_SIZE);
 }
 
 static int sp5100_tco_remove(struct platform_device *dev)
diff --git a/drivers/watchdog/sp5100_tco.h b/drivers/watchdog/sp5100_tco.h
index 2b28c00..f5d9402 100644
--- a/drivers/watchdog/sp5100_tco.h
+++ b/drivers/watchdog/sp5100_tco.h
@@ -6,6 +6,8 @@
  *	TCO timer driver for sp5100 chipsets
  */
 
+#include <linux/sb800.h>
+
 /*
  * Some address definitions for the Watchdog
  */
@@ -24,8 +26,8 @@
  */
 
 /*  For SP5100/SB7x0 chipset */
-#define SP5100_IO_PM_INDEX_REG		0xCD6
-#define SP5100_IO_PM_DATA_REG		0xCD7
+#define SP5100_IO_PM_INDEX_REG		SB800_PIIX4_SMB_IDX
+#define SP5100_IO_PM_DATA_REG		SB800_PIIX4_SMB_DATA
 
 #define SP5100_SB_RESOURCE_MMIO_BASE	0x9C
 
@@ -45,8 +47,8 @@
 
 
 /*  For SB8x0(or later) chipset */
-#define SB800_IO_PM_INDEX_REG		0xCD6
-#define SB800_IO_PM_DATA_REG		0xCD7
+#define SB800_IO_PM_INDEX_REG		SB800_PIIX4_SMB_IDX
+#define SB800_IO_PM_DATA_REG		SB800_PIIX4_SMB_DATA
 
 #define SB800_PM_ACPI_MMIO_EN		0x24
 #define SB800_PM_WATCHDOG_CONTROL	0x48
-- 
2.9.3

[PATCH 0/3, resend] Fix sp5100_tco watchdog driver regression

[Zoltan Boszormenyi]

My name with Hungarian accented characters from the Signed-off-by line
is also used in cc: by git send-email but it gets rejected by
mailer damons. Why only patch 2/3 was rejected is a mystery.
Resending with my name in 7-bit ASCII.

Three drivers are accessing the same I/O ports (0xcd6 / 0xcd7) on
AMD SB800 based machines without synchronization or with excluding
each other out:
* the USB quirk for isochronous transfers on SB800 (no locking)
* sp5100_tco (request_region)
* i2c-piix4 (request_region)

Historically, the sp5100_tco watchdog driver used request_region()
for these I/O ports but an i2c-piix4 improvement for SB800 in
Linux 4.4-rc4 also added a request_region() call. Because of this
and the load order, this cause a regression and the watchdog function
became non-functional. The commit that caused the regression is:

commit 2fee61d22e606fc99ade9079fda15fdee83ec33e
Author: Christian Fetzer <fetzer.ch@gmail.com>
Date:   Thu Nov 19 20:13:48 2015 +0100

    i2c: piix4: Add support for multiplexed main adapter in SB800

I was informed by Guenter Roeck <linux@roeck-us.net> that the
alternative, i.e. using request_muxed_region() can fail, either
because of a resource allocation failure, which is quite possible
with long uptimes, or because there are no guarantees that an as yet
unknown driver would also use request_muxed_region() consistently.

Because of this, a solution using a common mutex was chosen to
synchronize I/O port accesses and request_region() calls are removed
from both i2c-piix4 and sp5100_tco to make the code uniform.

This patch series implements this and restores the watchdog function.

Signed-off-by: Zoltan Boszormenyi <zboszor@pr.hu>

 drivers/i2c/busses/i2c-piix4.c | 59 ++++++++++++--------------------------
 drivers/usb/host/pci-quirks.c  | 14 ++++++---
 drivers/watchdog/sp5100_tco.c  | 24 +++++++---------
 drivers/watchdog/sp5100_tco.h  | 10 ++++---
 include/linux/sb800.h          | 15 ++++++++++
 5 files changed, 61 insertions(+), 61 deletions(-)

[PATCH 1/3] usb: pci-quirks: Add a header for SB800 I/O ports and mutex for locking

[Zoltan Boszormenyi]

From: Böszörményi Zoltán <zboszor@pr.hu>

This patch adds:
* a mutex in the USB PCI quirks code for synchronizing access to
  the I/O ports on SB800
* a new header that contains symbols for the index and data I/O ports
  and wrappers for locking and unlocking the mutex.
* locking around the I/O port access for SB800

Signed-off-by: Zoltan Boszormenyi <zboszor@pr.hu>
---
 drivers/usb/host/pci-quirks.c | 14 ++++++++++----
 include/linux/sb800.h         | 15 +++++++++++++++
 2 files changed, 25 insertions(+), 4 deletions(-)
 create mode 100644 include/linux/sb800.h

diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
index a9a1e4c..9b0445c 100644
--- a/drivers/usb/host/pci-quirks.c
+++ b/drivers/usb/host/pci-quirks.c
@@ -15,6 +15,7 @@
 #include <linux/export.h>
 #include <linux/acpi.h>
 #include <linux/dmi.h>
+#include <linux/sb800.h>
 #include "pci-quirks.h"
 #include "xhci-ext-caps.h"
 
@@ -279,6 +280,9 @@ bool usb_amd_prefetch_quirk(void)
 }
 EXPORT_SYMBOL_GPL(usb_amd_prefetch_quirk);
 
+DEFINE_MUTEX(sb800_mutex);
+EXPORT_SYMBOL_GPL(sb800_mutex);
+
 /*
  * The hardware normally enables the A-link power management feature, which
  * lets the system lower the power consumption in idle states.
@@ -314,11 +318,13 @@ static void usb_amd_quirk_pll(int disable)
 	if (amd_chipset.sb_type.gen == AMD_CHIPSET_SB800 ||
 			amd_chipset.sb_type.gen == AMD_CHIPSET_HUDSON2 ||
 			amd_chipset.sb_type.gen == AMD_CHIPSET_BOLTON) {
-		outb_p(AB_REG_BAR_LOW, 0xcd6);
-		addr_low = inb_p(0xcd7);
-		outb_p(AB_REG_BAR_HIGH, 0xcd6);
-		addr_high = inb_p(0xcd7);
+		enter_sb800();
+		outb_p(AB_REG_BAR_LOW, SB800_PIIX4_SMB_IDX);
+		addr_low = inb_p(SB800_PIIX4_SMB_DATA);
+		outb_p(AB_REG_BAR_HIGH, SB800_PIIX4_SMB_IDX);
+		addr_high = inb_p(SB800_PIIX4_SMB_DATA);
 		addr = addr_high << 8 | addr_low;
+		leave_sb800();
 
 		outl_p(0x30, AB_INDX(addr));
 		outl_p(0x40, AB_DATA(addr));
diff --git a/include/linux/sb800.h b/include/linux/sb800.h
new file mode 100644
index 0000000..5650b7d
--- /dev/null
+++ b/include/linux/sb800.h
@@ -0,0 +1,15 @@
+
+#ifndef SB800_H
+#define SB800_H
+
+#include <linux/mutex.h>
+
+#define SB800_PIIX4_SMB_IDX		0xcd6
+#define SB800_PIIX4_SMB_DATA	0xcd7
+
+extern struct mutex sb800_mutex;
+
+#define enter_sb800()	mutex_lock(&sb800_mutex)
+#define leave_sb800()	mutex_unlock(&sb800_mutex)
+
+#endif
-- 
2.9.3

[PATCH 3/3] watchdog: sp5100_tco: Synchronize I/O port accesses

[Zoltan Boszormenyi]

From: Böszörményi Zoltán <zboszor@pr.hu>

Use the new header and the mutex from usb/host/pci-quirks.c

This change will allow accesses to the SB800 I/O port pair
synchronized with the PCI quirk when isochronous USB transfers
are performed and with i2c-piix4.

At the same time, remove the request_region() call to reserve
these I/O ports, similarly to i2c-piix4 so the code is now uniform
across the three individual drivers.

Signed-off-by: Zoltan Boszormenyi <zboszor@pr.hu>
---
 drivers/watchdog/sp5100_tco.c | 24 +++++++++++-------------
 drivers/watchdog/sp5100_tco.h | 10 ++++++----
 2 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
index 028618c..dc125df 100644
--- a/drivers/watchdog/sp5100_tco.c
+++ b/drivers/watchdog/sp5100_tco.c
@@ -48,7 +48,6 @@
 static u32 tcobase_phys;
 static u32 tco_wdt_fired;
 static void __iomem *tcobase;
-static unsigned int pm_iobase;
 static DEFINE_SPINLOCK(tco_lock);	/* Guards the hardware */
 static unsigned long timer_alive;
 static char tco_expect_close;
@@ -138,6 +137,8 @@ static void tco_timer_enable(void)
 
 	if (!tco_has_sp5100_reg_layout(sp5100_tco_pci)) {
 		/* For SB800 or later */
+		enter_sb800();
+
 		/* Set the Watchdog timer resolution to 1 sec */
 		outb(SB800_PM_WATCHDOG_CONFIG, SB800_IO_PM_INDEX_REG);
 		val = inb(SB800_IO_PM_DATA_REG);
@@ -150,6 +151,8 @@ static void tco_timer_enable(void)
 		val |= SB800_PCI_WATCHDOG_DECODE_EN;
 		val &= ~SB800_PM_WATCHDOG_DISABLE;
 		outb(val, SB800_IO_PM_DATA_REG);
+
+		leave_sb800();
 	} else {
 		/* For SP5100 or SB7x0 */
 		/* Enable watchdog decode bit */
@@ -164,11 +167,13 @@ static void tco_timer_enable(void)
 				       val);
 
 		/* Enable Watchdog timer and set the resolution to 1 sec */
+		enter_sb800();
 		outb(SP5100_PM_WATCHDOG_CONTROL, SP5100_IO_PM_INDEX_REG);
 		val = inb(SP5100_IO_PM_DATA_REG);
 		val |= SP5100_PM_WATCHDOG_SECOND_RES;
 		val &= ~SP5100_PM_WATCHDOG_DISABLE;
 		outb(val, SP5100_IO_PM_DATA_REG);
+		leave_sb800();
 	}
 }
 
@@ -361,16 +366,10 @@ static unsigned char sp5100_tco_setupdevice(void)
 		base_addr = SB800_PM_WATCHDOG_BASE;
 	}
 
-	/* Request the IO ports used by this driver */
-	pm_iobase = SP5100_IO_PM_INDEX_REG;
-	if (!request_region(pm_iobase, SP5100_PM_IOPORTS_SIZE, dev_name)) {
-		pr_err("I/O address 0x%04x already in use\n", pm_iobase);
-		goto exit;
-	}
-
 	/*
 	 * First, Find the watchdog timer MMIO address from indirect I/O.
 	 */
+	enter_sb800();
 	outb(base_addr+3, index_reg);
 	val = inb(data_reg);
 	outb(base_addr+2, index_reg);
@@ -380,6 +379,7 @@ static unsigned char sp5100_tco_setupdevice(void)
 	outb(base_addr+0, index_reg);
 	/* Low three bits of BASE are reserved */
 	val = val << 8 | (inb(data_reg) & 0xf8);
+	leave_sb800();
 
 	pr_debug("Got 0x%04x from indirect I/O\n", val);
 
@@ -400,6 +400,7 @@ static unsigned char sp5100_tco_setupdevice(void)
 				      SP5100_SB_RESOURCE_MMIO_BASE, &val);
 	} else {
 		/* Read SBResource_MMIO from AcpiMmioEn(PM_Reg: 24h) */
+		enter_sb800();
 		outb(SB800_PM_ACPI_MMIO_EN+3, SB800_IO_PM_INDEX_REG);
 		val = inb(SB800_IO_PM_DATA_REG);
 		outb(SB800_PM_ACPI_MMIO_EN+2, SB800_IO_PM_INDEX_REG);
@@ -408,6 +409,7 @@ static unsigned char sp5100_tco_setupdevice(void)
 		val = val << 8 | inb(SB800_IO_PM_DATA_REG);
 		outb(SB800_PM_ACPI_MMIO_EN+0, SB800_IO_PM_INDEX_REG);
 		val = val << 8 | inb(SB800_IO_PM_DATA_REG);
+		leave_sb800();
 	}
 
 	/* The SBResource_MMIO is enabled and mapped memory space? */
@@ -429,7 +431,7 @@ static unsigned char sp5100_tco_setupdevice(void)
 		pr_debug("SBResource_MMIO is disabled(0x%04x)\n", val);
 
 	pr_notice("failed to find MMIO address, giving up.\n");
-	goto  unreg_region;
+	goto  exit;
 
 setup_wdt:
 	tcobase_phys = val;
@@ -469,8 +471,6 @@ static unsigned char sp5100_tco_setupdevice(void)
 
 unreg_mem_region:
 	release_mem_region(tcobase_phys, SP5100_WDT_MEM_MAP_SIZE);
-unreg_region:
-	release_region(pm_iobase, SP5100_PM_IOPORTS_SIZE);
 exit:
 	return 0;
 }
@@ -517,7 +517,6 @@ static int sp5100_tco_init(struct platform_device *dev)
 exit:
 	iounmap(tcobase);
 	release_mem_region(tcobase_phys, SP5100_WDT_MEM_MAP_SIZE);
-	release_region(pm_iobase, SP5100_PM_IOPORTS_SIZE);
 	return ret;
 }
 
@@ -531,7 +530,6 @@ static void sp5100_tco_cleanup(void)
 	misc_deregister(&sp5100_tco_miscdev);
 	iounmap(tcobase);
 	release_mem_region(tcobase_phys, SP5100_WDT_MEM_MAP_SIZE);
-	release_region(pm_iobase, SP5100_PM_IOPORTS_SIZE);
 }
 
 static int sp5100_tco_remove(struct platform_device *dev)
diff --git a/drivers/watchdog/sp5100_tco.h b/drivers/watchdog/sp5100_tco.h
index 2b28c00..f5d9402 100644
--- a/drivers/watchdog/sp5100_tco.h
+++ b/drivers/watchdog/sp5100_tco.h
@@ -6,6 +6,8 @@
  *	TCO timer driver for sp5100 chipsets
  */
 
+#include <linux/sb800.h>
+
 /*
  * Some address definitions for the Watchdog
  */
@@ -24,8 +26,8 @@
  */
 
 /*  For SP5100/SB7x0 chipset */
-#define SP5100_IO_PM_INDEX_REG		0xCD6
-#define SP5100_IO_PM_DATA_REG		0xCD7
+#define SP5100_IO_PM_INDEX_REG		SB800_PIIX4_SMB_IDX
+#define SP5100_IO_PM_DATA_REG		SB800_PIIX4_SMB_DATA
 
 #define SP5100_SB_RESOURCE_MMIO_BASE	0x9C
 
@@ -45,8 +47,8 @@
 
 
 /*  For SB8x0(or later) chipset */
-#define SB800_IO_PM_INDEX_REG		0xCD6
-#define SB800_IO_PM_DATA_REG		0xCD7
+#define SB800_IO_PM_INDEX_REG		SB800_PIIX4_SMB_IDX
+#define SB800_IO_PM_DATA_REG		SB800_PIIX4_SMB_DATA
 
 #define SB800_PM_ACPI_MMIO_EN		0x24
 #define SB800_PM_WATCHDOG_CONTROL	0x48
-- 
2.9.3

[PATCH 2/3] i2c: i2c-piix4: Synchronize I/O port accesses with the SB800 USB quirk

[Zoltan Boszormenyi]

Use the new header and the common mutex in the i2c-piix4 driver.

At the same time, remove the request_region() call to reserve
these I/O ports, so the sp5100_tco watchdog driver is fixed.
The mutex is enough to protect the I/O port accesses. This is
an old regression in Linux 4.4-rc4, caused by:

commit 2fee61d22e606fc99ade9079fda15fdee83ec33e
Author: Christian Fetzer <fetzer.ch@gmail.com>
Date:   Thu Nov 19 20:13:48 2015 +0100

    i2c: piix4: Add support for multiplexed main adapter in SB800

Signed-off-by: Zoltan Boszormenyi <zboszor@pr.hu>
---
 drivers/i2c/busses/i2c-piix4.c | 59 ++++++++++++++----------------------------
 1 file changed, 19 insertions(+), 40 deletions(-)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index c21ca7b..a4549e5 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -40,7 +40,7 @@
 #include <linux/dmi.h>
 #include <linux/acpi.h>
 #include <linux/io.h>
-#include <linux/mutex.h>
+#include <linux/sb800.h>
 
 
 /* PIIX4 SMBus address offsets */
@@ -82,9 +82,6 @@
 /* Multi-port constants */
 #define PIIX4_MAX_ADAPTERS 4
 
-/* SB800 constants */
-#define SB800_PIIX4_SMB_IDX		0xcd6
-
 /*
  * SB800 port is selected by bits 2:1 of the smb_en register (0x2c)
  * or the smb_sel register (0x2e), depending on bit 0 of register 0x2f.
@@ -144,10 +141,9 @@ static const struct dmi_system_id piix4_dmi_ibm[] = {
 
 /*
  * SB800 globals
- * piix4_mutex_sb800 protects piix4_port_sel_sb800 and the pair
- * of I/O ports at SB800_PIIX4_SMB_IDX.
+ * sb800_mutex in drivers/usb/host/pci-quirks.c protects
+ * piix4_port_sel_sb800 and the pair of I/O ports at SB800_PIIX4_SMB_IDX.
  */
-static DEFINE_MUTEX(piix4_mutex_sb800);
 static u8 piix4_port_sel_sb800;
 static const char *piix4_main_port_names_sb800[PIIX4_MAX_ADAPTERS] = {
 	" port 0", " port 2", " port 3", " port 4"
@@ -157,8 +153,6 @@ static const char *piix4_aux_port_name_sb800 = " port 1";
 struct i2c_piix4_adapdata {
 	unsigned short smba;
 
-	/* SB800 */
-	bool sb800_main;
 	u8 port;		/* Port number, shifted */
 };
 
@@ -286,12 +280,12 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
 	else
 		smb_en = (aux) ? 0x28 : 0x2c;
 
-	mutex_lock(&piix4_mutex_sb800);
+	enter_sb800();
 	outb_p(smb_en, SB800_PIIX4_SMB_IDX);
-	smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1);
+	smba_en_lo = inb_p(SB800_PIIX4_SMB_DATA);
 	outb_p(smb_en + 1, SB800_PIIX4_SMB_IDX);
-	smba_en_hi = inb_p(SB800_PIIX4_SMB_IDX + 1);
-	mutex_unlock(&piix4_mutex_sb800);
+	smba_en_hi = inb_p(SB800_PIIX4_SMB_DATA);
+	leave_sb800();
 
 	if (!smb_en) {
 		smb_en_status = smba_en_lo & 0x10;
@@ -349,13 +343,13 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
 	if (PIIX4_dev->vendor == PCI_VENDOR_ID_AMD) {
 		piix4_port_sel_sb800 = SB800_PIIX4_PORT_IDX_ALT;
 	} else {
-		mutex_lock(&piix4_mutex_sb800);
+		enter_sb800();
 		outb_p(SB800_PIIX4_PORT_IDX_SEL, SB800_PIIX4_SMB_IDX);
-		port_sel = inb_p(SB800_PIIX4_SMB_IDX + 1);
+		port_sel = inb_p(SB800_PIIX4_SMB_DATA);
 		piix4_port_sel_sb800 = (port_sel & 0x01) ?
 				       SB800_PIIX4_PORT_IDX_ALT :
 				       SB800_PIIX4_PORT_IDX;
-		mutex_unlock(&piix4_mutex_sb800);
+		leave_sb800();
 	}
 
 	dev_info(&PIIX4_dev->dev,
@@ -592,7 +586,7 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
 	u8 port;
 	int retval;
 
-	mutex_lock(&piix4_mutex_sb800);
+	enter_sb800();
 
 	/* Request the SMBUS semaphore, avoid conflicts with the IMC */
 	smbslvcnt  = inb_p(SMBSLVCNT);
@@ -608,27 +602,27 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
 	} while (--retries);
 	/* SMBus is still owned by the IMC, we give up */
 	if (!retries) {
-		mutex_unlock(&piix4_mutex_sb800);
+		leave_sb800();
 		return -EBUSY;
 	}
 
 	outb_p(piix4_port_sel_sb800, SB800_PIIX4_SMB_IDX);
-	smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1);
+	smba_en_lo = inb_p(SB800_PIIX4_SMB_DATA);
 
 	port = adapdata->port;
 	if ((smba_en_lo & SB800_PIIX4_PORT_IDX_MASK) != port)
 		outb_p((smba_en_lo & ~SB800_PIIX4_PORT_IDX_MASK) | port,
-		       SB800_PIIX4_SMB_IDX + 1);
+		       SB800_PIIX4_SMB_DATA);
 
 	retval = piix4_access(adap, addr, flags, read_write,
 			      command, size, data);
 
-	outb_p(smba_en_lo, SB800_PIIX4_SMB_IDX + 1);
+	outb_p(smba_en_lo, SB800_PIIX4_SMB_DATA);
 
 	/* Release the semaphore */
 	outb_p(smbslvcnt | 0x20, SMBSLVCNT);
 
-	mutex_unlock(&piix4_mutex_sb800);
+	leave_sb800();
 
 	return retval;
 }
@@ -705,7 +699,6 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
 	}
 
 	adapdata->smba = smba;
-	adapdata->sb800_main = sb800_main;
 	adapdata->port = port << 1;
 
 	/* set up the sysfs linkage to our parent device */
@@ -771,29 +764,18 @@ static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	    dev->vendor == PCI_VENDOR_ID_AMD) {
 		is_sb800 = true;
 
-		if (!request_region(SB800_PIIX4_SMB_IDX, 2, "smba_idx")) {
-			dev_err(&dev->dev,
-			"SMBus base address index region 0x%x already in use!\n",
-			SB800_PIIX4_SMB_IDX);
-			return -EBUSY;
-		}
-
 		/* base address location etc changed in SB800 */
 		retval = piix4_setup_sb800(dev, id, 0);
-		if (retval < 0) {
-			release_region(SB800_PIIX4_SMB_IDX, 2);
+		if (retval < 0)
 			return retval;
-		}
 
 		/*
 		 * Try to register multiplexed main SMBus adapter,
 		 * give up if we can't
 		 */
 		retval = piix4_add_adapters_sb800(dev, retval);
-		if (retval < 0) {
-			release_region(SB800_PIIX4_SMB_IDX, 2);
+		if (retval < 0)
 			return retval;
-		}
 	} else {
 		retval = piix4_setup(dev, id);
 		if (retval < 0)
@@ -841,11 +823,8 @@ static void piix4_adap_remove(struct i2c_adapter *adap)
 
 	if (adapdata->smba) {
 		i2c_del_adapter(adap);
-		if (adapdata->port == (0 << 1)) {
+		if (adapdata->port == (0 << 1))
 			release_region(adapdata->smba, SMBIOSIZE);
-			if (adapdata->sb800_main)
-				release_region(SB800_PIIX4_SMB_IDX, 2);
-		}
 		kfree(adapdata);
 		kfree(adap);
 	}
-- 
2.9.3

Re: [PATCH 1/3] usb: pci-quirks: Add a header for SB800 I/O ports and mutex for locking

[Greg KH]

On Sat, Apr 01, 2017 at 01:02:21PM +0200, Zoltan Boszormenyi wrote:
> From: Böszörményi Zoltán <zboszor@pr.hu>
> 
> This patch adds:
> * a mutex in the USB PCI quirks code for synchronizing access to
>   the I/O ports on SB800
> * a new header that contains symbols for the index and data I/O ports
>   and wrappers for locking and unlocking the mutex.
> * locking around the I/O port access for SB800
> 
> Signed-off-by: Zoltan Boszormenyi <zboszor@pr.hu>
> ---
>  drivers/usb/host/pci-quirks.c | 14 ++++++++++----
>  include/linux/sb800.h         | 15 +++++++++++++++
>  2 files changed, 25 insertions(+), 4 deletions(-)
>  create mode 100644 include/linux/sb800.h
> 
> diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
> index a9a1e4c..9b0445c 100644
> --- a/drivers/usb/host/pci-quirks.c
> +++ b/drivers/usb/host/pci-quirks.c
> @@ -15,6 +15,7 @@
>  #include <linux/export.h>
>  #include <linux/acpi.h>
>  #include <linux/dmi.h>
> +#include <linux/sb800.h>
>  #include "pci-quirks.h"
>  #include "xhci-ext-caps.h"
>  
> @@ -279,6 +280,9 @@ bool usb_amd_prefetch_quirk(void)
>  }
>  EXPORT_SYMBOL_GPL(usb_amd_prefetch_quirk);
>  
> +DEFINE_MUTEX(sb800_mutex);
> +EXPORT_SYMBOL_GPL(sb800_mutex);
> +
>  /*
>   * The hardware normally enables the A-link power management feature, which
>   * lets the system lower the power consumption in idle states.
> @@ -314,11 +318,13 @@ static void usb_amd_quirk_pll(int disable)
>  	if (amd_chipset.sb_type.gen == AMD_CHIPSET_SB800 ||
>  			amd_chipset.sb_type.gen == AMD_CHIPSET_HUDSON2 ||
>  			amd_chipset.sb_type.gen == AMD_CHIPSET_BOLTON) {
> -		outb_p(AB_REG_BAR_LOW, 0xcd6);
> -		addr_low = inb_p(0xcd7);
> -		outb_p(AB_REG_BAR_HIGH, 0xcd6);
> -		addr_high = inb_p(0xcd7);
> +		enter_sb800();
> +		outb_p(AB_REG_BAR_LOW, SB800_PIIX4_SMB_IDX);
> +		addr_low = inb_p(SB800_PIIX4_SMB_DATA);
> +		outb_p(AB_REG_BAR_HIGH, SB800_PIIX4_SMB_IDX);
> +		addr_high = inb_p(SB800_PIIX4_SMB_DATA);
>  		addr = addr_high << 8 | addr_low;
> +		leave_sb800();
>  
>  		outl_p(0x30, AB_INDX(addr));
>  		outl_p(0x40, AB_DATA(addr));
> diff --git a/include/linux/sb800.h b/include/linux/sb800.h
> new file mode 100644
> index 0000000..5650b7d
> --- /dev/null
> +++ b/include/linux/sb800.h
> @@ -0,0 +1,15 @@
> +
> +#ifndef SB800_H
> +#define SB800_H
> +
> +#include <linux/mutex.h>
> +
> +#define SB800_PIIX4_SMB_IDX		0xcd6
> +#define SB800_PIIX4_SMB_DATA	0xcd7
> +
> +extern struct mutex sb800_mutex;
> +
> +#define enter_sb800()	mutex_lock(&sb800_mutex)
> +#define leave_sb800()	mutex_unlock(&sb800_mutex)

Don't hide the mutex, just spell it out in the code itself.  No need for
these defines at all.

thanks,

greg k-h

Re: [PATCH 1/3] usb: pci-quirks: Add a header for SB800 I/O ports and mutex for locking

[Alan Stern]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=UTF-8, Size: 1318 bytes --]

On Sat, 1 Apr 2017, Greg KH wrote:

> On Sat, Apr 01, 2017 at 01:02:21PM +0200, Zoltan Boszormenyi wrote:
> > From: Böszörményi Zoltán <zboszor@pr.hu>
> > 
> > This patch adds:
> > * a mutex in the USB PCI quirks code for synchronizing access to
> >   the I/O ports on SB800
> > * a new header that contains symbols for the index and data I/O ports
> >   and wrappers for locking and unlocking the mutex.
> > * locking around the I/O port access for SB800
> > 
> > Signed-off-by: Zoltan Boszormenyi <zboszor@pr.hu>
> > ---

> > diff --git a/include/linux/sb800.h b/include/linux/sb800.h
> > new file mode 100644
> > index 0000000..5650b7d
> > --- /dev/null
> > +++ b/include/linux/sb800.h
> > @@ -0,0 +1,15 @@
> > +
> > +#ifndef SB800_H
> > +#define SB800_H
> > +
> > +#include <linux/mutex.h>
> > +
> > +#define SB800_PIIX4_SMB_IDX		0xcd6
> > +#define SB800_PIIX4_SMB_DATA	0xcd7
> > +
> > +extern struct mutex sb800_mutex;
> > +
> > +#define enter_sb800()	mutex_lock(&sb800_mutex)
> > +#define leave_sb800()	mutex_unlock(&sb800_mutex)

Is include/linux/ the best place for this new header file?  Aren't 
there other locations more suitable for something that's 
board-specific?

Alan Stern

> Don't hide the mutex, just spell it out in the code itself.  No need for
> these defines at all.
> 
> thanks,
> 
> greg k-h

Re: [PATCH 1/3] usb: pci-quirks: Add a header for SB800 I/O ports and mutex for locking

[Boszormenyi Zoltan]

2017-04-01 15:59 keltezéssel, Greg KH írta:
> On Sat, Apr 01, 2017 at 01:02:21PM +0200, Zoltan Boszormenyi wrote:
>> From: Böszörményi Zoltán <zboszor@pr.hu>
>>
>> This patch adds:
>> * a mutex in the USB PCI quirks code for synchronizing access to
>>   the I/O ports on SB800
>> * a new header that contains symbols for the index and data I/O ports
>>   and wrappers for locking and unlocking the mutex.
>> * locking around the I/O port access for SB800
>>
>> Signed-off-by: Zoltan Boszormenyi <zboszor@pr.hu>
>> ---
>>  drivers/usb/host/pci-quirks.c | 14 ++++++++++----
>>  include/linux/sb800.h         | 15 +++++++++++++++
>>  2 files changed, 25 insertions(+), 4 deletions(-)
>>  create mode 100644 include/linux/sb800.h
>>
>> diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
>> index a9a1e4c..9b0445c 100644
>> --- a/drivers/usb/host/pci-quirks.c
>> +++ b/drivers/usb/host/pci-quirks.c
>> @@ -15,6 +15,7 @@
>>  #include <linux/export.h>
>>  #include <linux/acpi.h>
>>  #include <linux/dmi.h>
>> +#include <linux/sb800.h>
>>  #include "pci-quirks.h"
>>  #include "xhci-ext-caps.h"
>>
>> @@ -279,6 +280,9 @@ bool usb_amd_prefetch_quirk(void)
>>  }
>>  EXPORT_SYMBOL_GPL(usb_amd_prefetch_quirk);
>>
>> +DEFINE_MUTEX(sb800_mutex);
>> +EXPORT_SYMBOL_GPL(sb800_mutex);
>> +
>>  /*
>>   * The hardware normally enables the A-link power management feature, which
>>   * lets the system lower the power consumption in idle states.
>> @@ -314,11 +318,13 @@ static void usb_amd_quirk_pll(int disable)
>>  	if (amd_chipset.sb_type.gen == AMD_CHIPSET_SB800 ||
>>  			amd_chipset.sb_type.gen == AMD_CHIPSET_HUDSON2 ||
>>  			amd_chipset.sb_type.gen == AMD_CHIPSET_BOLTON) {
>> -		outb_p(AB_REG_BAR_LOW, 0xcd6);
>> -		addr_low = inb_p(0xcd7);
>> -		outb_p(AB_REG_BAR_HIGH, 0xcd6);
>> -		addr_high = inb_p(0xcd7);
>> +		enter_sb800();
>> +		outb_p(AB_REG_BAR_LOW, SB800_PIIX4_SMB_IDX);
>> +		addr_low = inb_p(SB800_PIIX4_SMB_DATA);
>> +		outb_p(AB_REG_BAR_HIGH, SB800_PIIX4_SMB_IDX);
>> +		addr_high = inb_p(SB800_PIIX4_SMB_DATA);
>>  		addr = addr_high << 8 | addr_low;
>> +		leave_sb800();
>>
>>  		outl_p(0x30, AB_INDX(addr));
>>  		outl_p(0x40, AB_DATA(addr));
>> diff --git a/include/linux/sb800.h b/include/linux/sb800.h
>> new file mode 100644
>> index 0000000..5650b7d
>> --- /dev/null
>> +++ b/include/linux/sb800.h
>> @@ -0,0 +1,15 @@
>> +
>> +#ifndef SB800_H
>> +#define SB800_H
>> +
>> +#include <linux/mutex.h>
>> +
>> +#define SB800_PIIX4_SMB_IDX		0xcd6
>> +#define SB800_PIIX4_SMB_DATA	0xcd7
>> +
>> +extern struct mutex sb800_mutex;
>> +
>> +#define enter_sb800()	mutex_lock(&sb800_mutex)
>> +#define leave_sb800()	mutex_unlock(&sb800_mutex)
>
> Don't hide the mutex, just spell it out in the code itself.  No need for
> these defines at all.

Thanks, I will change it.

>
> thanks,
>
> greg k-h
>

Re: [PATCH 1/3] usb: pci-quirks: Add a header for SB800 I/O ports and mutex for locking

[Boszormenyi Zoltan]

2017-04-01 16:40 keltezéssel, Alan Stern írta:
> On Sat, 1 Apr 2017, Greg KH wrote:
>
>> On Sat, Apr 01, 2017 at 01:02:21PM +0200, Zoltan Boszormenyi wrote:
>>> From: B�sz�rm�nyi Zolt�n <zboszor@pr.hu>
>>>
>>> This patch adds:
>>> * a mutex in the USB PCI quirks code for synchronizing access to
>>>   the I/O ports on SB800
>>> * a new header that contains symbols for the index and data I/O ports
>>>   and wrappers for locking and unlocking the mutex.
>>> * locking around the I/O port access for SB800
>>>
>>> Signed-off-by: Zoltan Boszormenyi <zboszor@pr.hu>
>>> ---
>
>>> diff --git a/include/linux/sb800.h b/include/linux/sb800.h
>>> new file mode 100644
>>> index 0000000..5650b7d
>>> --- /dev/null
>>> +++ b/include/linux/sb800.h
>>> @@ -0,0 +1,15 @@
>>> +
>>> +#ifndef SB800_H
>>> +#define SB800_H
>>> +
>>> +#include <linux/mutex.h>
>>> +
>>> +#define SB800_PIIX4_SMB_IDX		0xcd6
>>> +#define SB800_PIIX4_SMB_DATA	0xcd7
>>> +
>>> +extern struct mutex sb800_mutex;
>>> +
>>> +#define enter_sb800()	mutex_lock(&sb800_mutex)
>>> +#define leave_sb800()	mutex_unlock(&sb800_mutex)
>
> Is include/linux/ the best place for this new header file?  Aren't
> there other locations more suitable for something that's
> board-specific?

Are there? Which subdirectory is better suited?

Would it be acceptable to not use a header at all but spell out
the "extern struct mutex..." in the two other drivers?

Thanks,
Zoltán Böszörményi

>
> Alan Stern
>
>> Don't hide the mutex, just spell it out in the code itself.  No need for
>> these defines at all.
>>
>> thanks,
>>
>> greg k-h
>
>

[PATCH 0/3 v2] Fix regression in the sp5100_tco driver

[Zoltan Boszormenyi]

Three drivers are accessing the same I/O ports (0xcd6 / 0xcd7) on
AMD SB800 based machines without synchronization or with excluding
each other out:
* the USB quirk for isochronous transfers on SB800 (no locking)
* sp5100_tco (request_region)
* i2c-piix4 (request_region)

Historically, the sp5100_tco watchdog driver used request_region()
for these I/O ports but an i2c-piix4 improvement for SB800 in
Linux 4.4-rc4 also added a request_region() call. Because of this
and the load order, this cause a regression and the watchdog function
became non-functional. The commit that caused the regression is:

commit 2fee61d22e606fc99ade9079fda15fdee83ec33e
Author: Christian Fetzer <fetzer.ch@gmail.com>
Date:   Thu Nov 19 20:13:48 2015 +0100

    i2c: piix4: Add support for multiplexed main adapter in SB800

I was informed by Guenter Roeck <linux@roeck-us.net> that the
alternative, i.e. using request_muxed_region() can fail, either
because of a resource allocation failure, which is quite possible
with long uptimes, or because there are no guarantees that an as yet
unknown driver would also use request_muxed_region() consistently.

Because of this, a solution using a common mutex was chosen to
synchronize I/O port accesses and request_region() calls are removed
from both i2c-piix4 and sp5100_tco to make the code uniform.

This patch series implements this and restores the watchdog function.

v2: Don't introduce a new header, reference sb800_mutex explicitly

Signed-off-by: Zoltan Boszormenyi <zboszor@pr.hu>

 drivers/i2c/busses/i2c-piix4.c | 43 +++++++++++++------------------------------
 drivers/usb/host/pci-quirks.c  |  5 +++++
 drivers/watchdog/sp5100_tco.c  | 31 ++++++++++++++++++-------------
 3 files changed, 36 insertions(+), 43 deletions(-)

[PATCH 1/3 v2] usb: pci-quirks: Add a common mutex for the I/O port pair of SB800

[Zoltan Boszormenyi]

This patch adds a common mutex in the USB PCI quirks code and starts
using it to synchronize access to the I/O port pair 0xcd6 / 0xcd7 on
SB800.

These I/O ports are also used by i2c-piix4 and sp5100_tco, the next
two patches port these drivers to use this common mutex.

v2: No extra header and no wrapper for mutex_lock() / mutex_unlock()

Signed-off-by: Zoltan Boszormenyi <zboszor@pr.hu>
---
 drivers/usb/host/pci-quirks.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
index a9a1e4c..1ef0ada 100644
--- a/drivers/usb/host/pci-quirks.c
+++ b/drivers/usb/host/pci-quirks.c
@@ -279,6 +279,9 @@ bool usb_amd_prefetch_quirk(void)
 }
 EXPORT_SYMBOL_GPL(usb_amd_prefetch_quirk);
 
+DEFINE_MUTEX(sb800_mutex);
+EXPORT_SYMBOL_GPL(sb800_mutex);
+
 /*
  * The hardware normally enables the A-link power management feature, which
  * lets the system lower the power consumption in idle states.
@@ -314,11 +317,13 @@ static void usb_amd_quirk_pll(int disable)
 	if (amd_chipset.sb_type.gen == AMD_CHIPSET_SB800 ||
 			amd_chipset.sb_type.gen == AMD_CHIPSET_HUDSON2 ||
 			amd_chipset.sb_type.gen == AMD_CHIPSET_BOLTON) {
+		mutex_lock(&sb800_mutex);
 		outb_p(AB_REG_BAR_LOW, 0xcd6);
 		addr_low = inb_p(0xcd7);
 		outb_p(AB_REG_BAR_HIGH, 0xcd6);
 		addr_high = inb_p(0xcd7);
 		addr = addr_high << 8 | addr_low;
+		mutex_unlock(&sb800_mutex);
 
 		outl_p(0x30, AB_INDX(addr));
 		outl_p(0x40, AB_DATA(addr));
-- 
2.9.3

[PATCH 2/3 v2] i2c: i2c-piix4: Use the common mutex

[Zoltan Boszormenyi]

Use the common mutex from driver/usb/host/pci-quirks.c in the
i2c-piix4 driver to synchronize access to the I/O port pair
0xcd6 / 0xcd7.

At the same time, remove the request_region() call to reserve
these I/O ports, so the sp5100_tco watchdog driver can also
load. The mutex is enough to protect the I/O port accesses.
This is an old regression in Linux 4.4-rc4, caused by:

v2: Explicit extern reference for sb800_mutex

commit 2fee61d22e606fc99ade9079fda15fdee83ec33e
Author: Christian Fetzer <fetzer.ch@gmail.com>
Date:   Thu Nov 19 20:13:48 2015 +0100

    i2c: piix4: Add support for multiplexed main adapter in SB800

Signed-off-by: Zoltan Boszormenyi <zboszor@pr.hu>
---
 drivers/i2c/busses/i2c-piix4.c | 43 +++++++++++++-----------------------------
 1 file changed, 13 insertions(+), 30 deletions(-)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index c21ca7b..6c56e96 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -144,10 +144,10 @@ static const struct dmi_system_id piix4_dmi_ibm[] = {
 
 /*
  * SB800 globals
- * piix4_mutex_sb800 protects piix4_port_sel_sb800 and the pair
- * of I/O ports at SB800_PIIX4_SMB_IDX.
+ * sb800_mutex in drivers/usb/host/pci-quirks.c protects
+ * piix4_port_sel_sb800 and the pair of I/O ports at SB800_PIIX4_SMB_IDX.
  */
-static DEFINE_MUTEX(piix4_mutex_sb800);
+extern struct mutex sb800_mutex;
 static u8 piix4_port_sel_sb800;
 static const char *piix4_main_port_names_sb800[PIIX4_MAX_ADAPTERS] = {
 	" port 0", " port 2", " port 3", " port 4"
@@ -157,8 +157,6 @@ static const char *piix4_aux_port_name_sb800 = " port 1";
 struct i2c_piix4_adapdata {
 	unsigned short smba;
 
-	/* SB800 */
-	bool sb800_main;
 	u8 port;		/* Port number, shifted */
 };
 
@@ -286,12 +284,12 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
 	else
 		smb_en = (aux) ? 0x28 : 0x2c;
 
-	mutex_lock(&piix4_mutex_sb800);
+	mutex_lock(&sb800_mutex);
 	outb_p(smb_en, SB800_PIIX4_SMB_IDX);
 	smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1);
 	outb_p(smb_en + 1, SB800_PIIX4_SMB_IDX);
 	smba_en_hi = inb_p(SB800_PIIX4_SMB_IDX + 1);
-	mutex_unlock(&piix4_mutex_sb800);
+	mutex_unlock(&sb800_mutex);
 
 	if (!smb_en) {
 		smb_en_status = smba_en_lo & 0x10;
@@ -349,13 +347,13 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
 	if (PIIX4_dev->vendor == PCI_VENDOR_ID_AMD) {
 		piix4_port_sel_sb800 = SB800_PIIX4_PORT_IDX_ALT;
 	} else {
-		mutex_lock(&piix4_mutex_sb800);
+		mutex_lock(&sb800_mutex);
 		outb_p(SB800_PIIX4_PORT_IDX_SEL, SB800_PIIX4_SMB_IDX);
 		port_sel = inb_p(SB800_PIIX4_SMB_IDX + 1);
 		piix4_port_sel_sb800 = (port_sel & 0x01) ?
 				       SB800_PIIX4_PORT_IDX_ALT :
 				       SB800_PIIX4_PORT_IDX;
-		mutex_unlock(&piix4_mutex_sb800);
+		mutex_unlock(&sb800_mutex);
 	}
 
 	dev_info(&PIIX4_dev->dev,
@@ -592,7 +590,7 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
 	u8 port;
 	int retval;
 
-	mutex_lock(&piix4_mutex_sb800);
+	mutex_lock(&sb800_mutex);
 
 	/* Request the SMBUS semaphore, avoid conflicts with the IMC */
 	smbslvcnt  = inb_p(SMBSLVCNT);
@@ -608,7 +606,7 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
 	} while (--retries);
 	/* SMBus is still owned by the IMC, we give up */
 	if (!retries) {
-		mutex_unlock(&piix4_mutex_sb800);
+		mutex_unlock(&sb800_mutex);
 		return -EBUSY;
 	}
 
@@ -628,7 +626,7 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
 	/* Release the semaphore */
 	outb_p(smbslvcnt | 0x20, SMBSLVCNT);
 
-	mutex_unlock(&piix4_mutex_sb800);
+	mutex_unlock(&sb800_mutex);
 
 	return retval;
 }
@@ -705,7 +703,6 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
 	}
 
 	adapdata->smba = smba;
-	adapdata->sb800_main = sb800_main;
 	adapdata->port = port << 1;
 
 	/* set up the sysfs linkage to our parent device */
@@ -771,29 +768,18 @@ static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	    dev->vendor == PCI_VENDOR_ID_AMD) {
 		is_sb800 = true;
 
-		if (!request_region(SB800_PIIX4_SMB_IDX, 2, "smba_idx")) {
-			dev_err(&dev->dev,
-			"SMBus base address index region 0x%x already in use!\n",
-			SB800_PIIX4_SMB_IDX);
-			return -EBUSY;
-		}
-
 		/* base address location etc changed in SB800 */
 		retval = piix4_setup_sb800(dev, id, 0);
-		if (retval < 0) {
-			release_region(SB800_PIIX4_SMB_IDX, 2);
+		if (retval < 0)
 			return retval;
-		}
 
 		/*
 		 * Try to register multiplexed main SMBus adapter,
 		 * give up if we can't
 		 */
 		retval = piix4_add_adapters_sb800(dev, retval);
-		if (retval < 0) {
-			release_region(SB800_PIIX4_SMB_IDX, 2);
+		if (retval < 0)
 			return retval;
-		}
 	} else {
 		retval = piix4_setup(dev, id);
 		if (retval < 0)
@@ -841,11 +827,8 @@ static void piix4_adap_remove(struct i2c_adapter *adap)
 
 	if (adapdata->smba) {
 		i2c_del_adapter(adap);
-		if (adapdata->port == (0 << 1)) {
+		if (adapdata->port == (0 << 1))
 			release_region(adapdata->smba, SMBIOSIZE);
-			if (adapdata->sb800_main)
-				release_region(SB800_PIIX4_SMB_IDX, 2);
-		}
 		kfree(adapdata);
 		kfree(adap);
 	}
-- 
2.9.3

[PATCH 3/3 v2] watchdog: sp5100_tco: Use the common mutex

[Zoltan Boszormenyi]

Use the common mutex from usb/host/pci-quirks.c to synchronize
accesses to the SB800 I/O port pair (0xcd6 / 0xcd7) with the
PCI quirk for isochronous USB transfers and with the i2c-piix4
driver.

At the same time, remove the request_region() call to reserve
these I/O ports, similarly to i2c-piix4 so the code is now uniform
across the three individual drivers.

v2: Explicit extern reference for sb800_mutex

Signed-off-by: Zoltan Boszormenyi <zboszor@pr.hu>
---
 drivers/watchdog/sp5100_tco.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
index 028618c..4dca9e89 100644
--- a/drivers/watchdog/sp5100_tco.c
+++ b/drivers/watchdog/sp5100_tco.c
@@ -36,9 +36,16 @@
 #include <linux/platform_device.h>
 #include <linux/uaccess.h>
 #include <linux/io.h>
+#include <linux/mutex.h>
 
 #include "sp5100_tco.h"
 
+/*
+ * sb800_mutex in drivers/usb/host/pci-quirks.c protects
+ * parallel access to the I/O port pair 0xCD6 / 0xCD7.
+ */
+extern struct mutex sb800_mutex;
+
 /* Module and version information */
 #define TCO_VERSION "0.05"
 #define TCO_MODULE_NAME "SP5100 TCO timer"
@@ -48,7 +55,6 @@
 static u32 tcobase_phys;
 static u32 tco_wdt_fired;
 static void __iomem *tcobase;
-static unsigned int pm_iobase;
 static DEFINE_SPINLOCK(tco_lock);	/* Guards the hardware */
 static unsigned long timer_alive;
 static char tco_expect_close;
@@ -138,6 +144,8 @@ static void tco_timer_enable(void)
 
 	if (!tco_has_sp5100_reg_layout(sp5100_tco_pci)) {
 		/* For SB800 or later */
+		mutex_lock(&sb800_mutex);
+
 		/* Set the Watchdog timer resolution to 1 sec */
 		outb(SB800_PM_WATCHDOG_CONFIG, SB800_IO_PM_INDEX_REG);
 		val = inb(SB800_IO_PM_DATA_REG);
@@ -150,6 +158,8 @@ static void tco_timer_enable(void)
 		val |= SB800_PCI_WATCHDOG_DECODE_EN;
 		val &= ~SB800_PM_WATCHDOG_DISABLE;
 		outb(val, SB800_IO_PM_DATA_REG);
+
+		mutex_unlock(&sb800_mutex);
 	} else {
 		/* For SP5100 or SB7x0 */
 		/* Enable watchdog decode bit */
@@ -164,11 +174,13 @@ static void tco_timer_enable(void)
 				       val);
 
 		/* Enable Watchdog timer and set the resolution to 1 sec */
+		mutex_lock(&sb800_mutex);
 		outb(SP5100_PM_WATCHDOG_CONTROL, SP5100_IO_PM_INDEX_REG);
 		val = inb(SP5100_IO_PM_DATA_REG);
 		val |= SP5100_PM_WATCHDOG_SECOND_RES;
 		val &= ~SP5100_PM_WATCHDOG_DISABLE;
 		outb(val, SP5100_IO_PM_DATA_REG);
+		mutex_unlock(&sb800_mutex);
 	}
 }
 
@@ -361,16 +373,10 @@ static unsigned char sp5100_tco_setupdevice(void)
 		base_addr = SB800_PM_WATCHDOG_BASE;
 	}
 
-	/* Request the IO ports used by this driver */
-	pm_iobase = SP5100_IO_PM_INDEX_REG;
-	if (!request_region(pm_iobase, SP5100_PM_IOPORTS_SIZE, dev_name)) {
-		pr_err("I/O address 0x%04x already in use\n", pm_iobase);
-		goto exit;
-	}
-
 	/*
 	 * First, Find the watchdog timer MMIO address from indirect I/O.
 	 */
+	mutex_lock(&sb800_mutex);
 	outb(base_addr+3, index_reg);
 	val = inb(data_reg);
 	outb(base_addr+2, index_reg);
@@ -380,6 +386,7 @@ static unsigned char sp5100_tco_setupdevice(void)
 	outb(base_addr+0, index_reg);
 	/* Low three bits of BASE are reserved */
 	val = val << 8 | (inb(data_reg) & 0xf8);
+	mutex_unlock(&sb800_mutex);
 
 	pr_debug("Got 0x%04x from indirect I/O\n", val);
 
@@ -400,6 +407,7 @@ static unsigned char sp5100_tco_setupdevice(void)
 				      SP5100_SB_RESOURCE_MMIO_BASE, &val);
 	} else {
 		/* Read SBResource_MMIO from AcpiMmioEn(PM_Reg: 24h) */
+		mutex_lock(&sb800_mutex);
 		outb(SB800_PM_ACPI_MMIO_EN+3, SB800_IO_PM_INDEX_REG);
 		val = inb(SB800_IO_PM_DATA_REG);
 		outb(SB800_PM_ACPI_MMIO_EN+2, SB800_IO_PM_INDEX_REG);
@@ -408,6 +416,7 @@ static unsigned char sp5100_tco_setupdevice(void)
 		val = val << 8 | inb(SB800_IO_PM_DATA_REG);
 		outb(SB800_PM_ACPI_MMIO_EN+0, SB800_IO_PM_INDEX_REG);
 		val = val << 8 | inb(SB800_IO_PM_DATA_REG);
+		mutex_unlock(&sb800_mutex);
 	}
 
 	/* The SBResource_MMIO is enabled and mapped memory space? */
@@ -429,7 +438,7 @@ static unsigned char sp5100_tco_setupdevice(void)
 		pr_debug("SBResource_MMIO is disabled(0x%04x)\n", val);
 
 	pr_notice("failed to find MMIO address, giving up.\n");
-	goto  unreg_region;
+	goto  exit;
 
 setup_wdt:
 	tcobase_phys = val;
@@ -469,8 +478,6 @@ static unsigned char sp5100_tco_setupdevice(void)
 
 unreg_mem_region:
 	release_mem_region(tcobase_phys, SP5100_WDT_MEM_MAP_SIZE);
-unreg_region:
-	release_region(pm_iobase, SP5100_PM_IOPORTS_SIZE);
 exit:
 	return 0;
 }
@@ -517,7 +524,6 @@ static int sp5100_tco_init(struct platform_device *dev)
 exit:
 	iounmap(tcobase);
 	release_mem_region(tcobase_phys, SP5100_WDT_MEM_MAP_SIZE);
-	release_region(pm_iobase, SP5100_PM_IOPORTS_SIZE);
 	return ret;
 }
 
@@ -531,7 +537,6 @@ static void sp5100_tco_cleanup(void)
 	misc_deregister(&sp5100_tco_miscdev);
 	iounmap(tcobase);
 	release_mem_region(tcobase_phys, SP5100_WDT_MEM_MAP_SIZE);
-	release_region(pm_iobase, SP5100_PM_IOPORTS_SIZE);
 }
 
 static int sp5100_tco_remove(struct platform_device *dev)
-- 
2.9.3

Re: [PATCH 2/3 v2] i2c: i2c-piix4: Use the common mutex

[Wolfram Sang]

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

On Mon, Apr 03, 2017 at 09:51:32AM +0200, Zoltan Boszormenyi wrote:
> Use the common mutex from driver/usb/host/pci-quirks.c in the
> i2c-piix4 driver to synchronize access to the I/O port pair
> 0xcd6 / 0xcd7.
> 
> At the same time, remove the request_region() call to reserve
> these I/O ports, so the sp5100_tco watchdog driver can also
> load. The mutex is enough to protect the I/O port accesses.
> This is an old regression in Linux 4.4-rc4, caused by:
> 
> v2: Explicit extern reference for sb800_mutex
> 
> commit 2fee61d22e606fc99ade9079fda15fdee83ec33e
> Author: Christian Fetzer <fetzer.ch@gmail.com>
> Date:   Thu Nov 19 20:13:48 2015 +0100
> 
>     i2c: piix4: Add support for multiplexed main adapter in SB800
> 
> Signed-off-by: Zoltan Boszormenyi <zboszor@pr.hu>

Given the global mutex approach is accepted, the actual changes here
look okay to me. I'd really like a tag from Jean here, still. He is the
expert for this hardware which I barely know.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [1/3, v2] usb: pci-quirks: Add a common mutex for the I/O port pair of SB800

[Guenter Roeck]

On Mon, Apr 03, 2017 at 09:51:31AM +0200, Zoltán Böszörményi wrote:
> This patch adds a common mutex in the USB PCI quirks code and starts
> using it to synchronize access to the I/O port pair 0xcd6 / 0xcd7 on
> SB800.
> 
> These I/O ports are also used by i2c-piix4 and sp5100_tco, the next
> two patches port these drivers to use this common mutex.
> 
> v2: No extra header and no wrapper for mutex_lock() / mutex_unlock()
> 

So now the callers have to declare the mutex in a source file, which results
in a checkpatch warning. This is not acceptable.

This is not my major issue with this patch, though. It creates an artificial
dependency between the watchdog driver, the i2c driver, and the USB host
code, which I think is a really bad idea.

Either the drivers accessing the IO region in question should use
request_muxed_region(), or there should be an explicit API, located somewhere
in core AMD kernel code, to access it.

Guenter

> Signed-off-by: Zoltan Boszormenyi <zboszor@pr.hu>
> ---
>  drivers/usb/host/pci-quirks.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
> index a9a1e4c..1ef0ada 100644
> --- a/drivers/usb/host/pci-quirks.c
> +++ b/drivers/usb/host/pci-quirks.c
> @@ -279,6 +279,9 @@ bool usb_amd_prefetch_quirk(void)
>  }
>  EXPORT_SYMBOL_GPL(usb_amd_prefetch_quirk);
>  
> +DEFINE_MUTEX(sb800_mutex);
> +EXPORT_SYMBOL_GPL(sb800_mutex);
> +
>  /*
>   * The hardware normally enables the A-link power management feature, which
>   * lets the system lower the power consumption in idle states.
> @@ -314,11 +317,13 @@ static void usb_amd_quirk_pll(int disable)
>  	if (amd_chipset.sb_type.gen == AMD_CHIPSET_SB800 ||
>  			amd_chipset.sb_type.gen == AMD_CHIPSET_HUDSON2 ||
>  			amd_chipset.sb_type.gen == AMD_CHIPSET_BOLTON) {
> +		mutex_lock(&sb800_mutex);
>  		outb_p(AB_REG_BAR_LOW, 0xcd6);
>  		addr_low = inb_p(0xcd7);
>  		outb_p(AB_REG_BAR_HIGH, 0xcd6);
>  		addr_high = inb_p(0xcd7);
>  		addr = addr_high << 8 | addr_low;
> +		mutex_unlock(&sb800_mutex);
>  
>  		outl_p(0x30, AB_INDX(addr));
>  		outl_p(0x40, AB_DATA(addr));

[PATCH 0/5 v3] Fix sp5100_tco watchdog driver regression

[Zoltán Böszörményi]

This patch series fixes a regression introduced by:

commit 2fee61d22e606fc99ade9079fda15fdee83ec33e
Author: Christian Fetzer <fetzer.ch@gmail.com>
Date:   Thu Nov 19 20:13:48 2015 +0100

    i2c: piix4: Add support for multiplexed main adapter in SB800

The regression caused sp5100_tco fail to load:

piix4_smbus 0000:00:14.0: SMBus Host Controller at 0xb00, revision 0
piix4_smbus 0000:00:14.0: Using register 0x2c for SMBus port selection
piix4_smbus 0000:00:14.0: Auxiliary SMBus Host Controller at 0xb20
sp5100_tco: SP5100/SB800 TCO WatchDog Timer Driver v0.05
sp5100_tco: PCI Vendor ID: 0x1002, Device ID: 0x4385, Revision ID: 0x42
sp5100_tco: I/O address 0x0cd6 already in use

Notable bugzilla links about this issue:
https://bugzilla.kernel.org/show_bug.cgi?id=170741
https://bugzilla.redhat.com/show_bug.cgi?id=1369269
https://bugzilla.redhat.com/show_bug.cgi?id=1406844

The previous two versions of this patch series introduced
a common mutex to synchronize access to the I/O port pair
0xcd6 / 0xcd7 used by the AMD SB800 USB PCI quirk code and
the i2c-piix and sp5100_tco drivers. The common mutex was
criticized because it introduces an inter-dependency between
drivers.

This approach modifies the request_muxed_region() semantics and
modifies the possible use cases.

The first patch in the series adds a new IORESOURCE_ALLOCATED
flag that alloc_resource() sets and free_resource() considers.
The core of __request_region() is factored out into a new function
that doesn't allocate. With this change, drivers can use the
pre-existing DEFINE_RES_IO_NAMED() static initialized macro
to declare struct resource statically (e.g. on the stack)
and pass the address of it to the new __request_declared_region()
function. A new macro called request_declared_muxed_region()
was added to exploit this functionality. Because of the new
IORESOURCE_ALLOCATED resource flag, release_region() can still
be called with the old interface (the port region start and
end values) and it won't attempt to free a non-allocated resource.
This eliminated one failure case that can come from allocation
errors.

The second patch modifies the behaviour of IORESOURCE_MUXED,
a.k.a. the request_*muxed_region() macros. When these macros
are called, the caller goes to sleep when there is any conflicting
regions, even if the conflicting region did not use the
IORESOURCE_MUXED flag. The kernel logs this inconsistent
flag usage with KERN_ERR. This change eliminates the second
failure case for IORESOURCE_MUXED and request_muxed_region()
can be used like mutex_lock(), i.e. it returns only in case it
could successfully request the region.

The last three patches adds proper synchronization to
the USB PCI quirks code and the i2c-piix and sp5100_tco drivers.

The result is that the sp5100_tco driver can load and works again:

piix4_smbus 0000:00:14.0: SMBus Host Controller at 0xb00, revision 0
piix4_smbus 0000:00:14.0: Using register 0x2c for SMBus port selection
piix4_smbus 0000:00:14.0: Auxiliary SMBus Host Controller at 0xb20
sp5100_tco: SP5100/SB800 TCO WatchDog Timer Driver v0.05
sp5100_tco: PCI Vendor ID: 0x1002, Device ID: 0x4385, Revision ID: 0x42
sp5100_tco: Using 0xfed80b00 for watchdog MMIO address
sp5100_tco: Last reboot was not triggered by watchdog.
sp5100_tco: initialized (0xffffba2f4192db00). heartbeat=60 sec (nowayout=0)

Signed-off-by: Zoltán Böszörményi <zboszor@pr.hu>
---
 drivers/i2c/busses/i2c-piix4.c | 42 ++++++++++--------------------
 drivers/usb/host/pci-quirks.c  |  3 +++
 drivers/watchdog/sp5100_tco.c  | 25 +++++++++---------
 include/linux/ioport.h         |  5 ++++
 kernel/resource.c              | 59 ++++++++++++++++++++++++++++++------------
 5 files changed, 75 insertions(+), 59 deletions(-)

[PATCH 1/5] Extend the request_region() infrastructure

[Zoltán Böszörményi]

Add a new IORESOURCE_ALLOCATED flag that is automatically used
when alloc_resource() is used internally in kernel/resource.c
and free_resource() now takes this flag into account.

The core of __request_region() was factored out into a new function
called __request_declared_region() that needs struct resource *
instead of the (start, n, name) triplet.

These changes allow using statically declared struct resource
data coupled with the pre-existing DEFINE_RES_IO_NAMED() static
initializer macro. The new macro exploiting
__request_declared_region() is request_declared_muxed_region()

Signed-off-by: Zoltán Böszörményi <zboszor@pr.hu>
---
 include/linux/ioport.h |  5 +++++
 kernel/resource.c      | 55 +++++++++++++++++++++++++++++++++++---------------
 2 files changed, 44 insertions(+), 16 deletions(-)

diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index 6230064..a3c4e08 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -52,6 +52,7 @@ struct resource {
 #define IORESOURCE_MEM_64	0x00100000
 #define IORESOURCE_WINDOW	0x00200000	/* forwarded by bridge */
 #define IORESOURCE_MUXED	0x00400000	/* Resource is software muxed */
+#define IORESOURCE_ALLOCATED	0x00800000	/* Resource was allocated */
 
 #define IORESOURCE_EXT_TYPE_BITS 0x01000000	/* Resource extended types */
 #define IORESOURCE_SYSRAM	0x01000000	/* System RAM (modifier) */
@@ -216,12 +217,16 @@ static inline bool resource_contains(struct resource *r1, struct resource *r2)
 /* Convenience shorthand with allocation */
 #define request_region(start,n,name)		__request_region(&ioport_resource, (start), (n), (name), 0)
 #define request_muxed_region(start,n,name)	__request_region(&ioport_resource, (start), (n), (name), IORESOURCE_MUXED)
+#define request_declared_muxed_region(res)	__request_declared_region(&ioport_resource, (res), IORESOURCE_MUXED)
 #define __request_mem_region(start,n,name, excl) __request_region(&iomem_resource, (start), (n), (name), excl)
 #define request_mem_region(start,n,name) __request_region(&iomem_resource, (start), (n), (name), 0)
 #define request_mem_region_exclusive(start,n,name) \
 	__request_region(&iomem_resource, (start), (n), (name), IORESOURCE_EXCLUSIVE)
 #define rename_region(region, newname) do { (region)->name = (newname); } while (0)
 
+extern struct resource * __request_declared_region(struct resource *,
+					struct resource *res, int flags);
+
 extern struct resource * __request_region(struct resource *,
 					resource_size_t start,
 					resource_size_t n,
diff --git a/kernel/resource.c b/kernel/resource.c
index 9b5f044..220f695 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -184,6 +184,9 @@ static void free_resource(struct resource *res)
 	if (!res)
 		return;
 
+	if (!(res->flags & IORESOURCE_ALLOCATED))
+		return;
+
 	if (!PageSlab(virt_to_head_page(res))) {
 		spin_lock(&bootmem_resource_lock);
 		res->sibling = bootmem_resource_free;
@@ -210,6 +213,8 @@ static struct resource *alloc_resource(gfp_t flags)
 	else
 		res = kzalloc(sizeof(struct resource), flags);
 
+	res->flags = IORESOURCE_ALLOCATED;
+
 	return res;
 }
 
@@ -1117,26 +1122,15 @@ resource_size_t resource_alignment(struct resource *res)
 static DECLARE_WAIT_QUEUE_HEAD(muxed_resource_wait);
 
 /**
- * __request_region - create a new busy resource region
+ * __request_declared_region - create a new busy resource region
  * @parent: parent resource descriptor
- * @start: resource start address
- * @n: resource region size
- * @name: reserving caller's ID string
+ * @res: child resource descriptor
  * @flags: IO resource flags
  */
-struct resource * __request_region(struct resource *parent,
-				   resource_size_t start, resource_size_t n,
-				   const char *name, int flags)
+struct resource * __request_declared_region(struct resource *parent,
+				   struct resource *res, int flags)
 {
 	DECLARE_WAITQUEUE(wait, current);
-	struct resource *res = alloc_resource(GFP_KERNEL);
-
-	if (!res)
-		return NULL;
-
-	res->name = name;
-	res->start = start;
-	res->end = start + n - 1;
 
 	write_lock(&resource_lock);
 
@@ -1166,13 +1160,42 @@ struct resource * __request_region(struct resource *parent,
 			continue;
 		}
 		/* Uhhuh, that didn't work out.. */
-		free_resource(res);
 		res = NULL;
 		break;
 	}
 	write_unlock(&resource_lock);
 	return res;
 }
+EXPORT_SYMBOL(__request_declared_region);
+
+/**
+ * __request_region - create a new busy resource region
+ * @parent: parent resource descriptor
+ * @start: resource start address
+ * @n: resource region size
+ * @name: reserving caller's ID string
+ * @flags: IO resource flags
+ */
+struct resource * __request_region(struct resource *parent,
+				   resource_size_t start, resource_size_t n,
+				   const char *name, int flags)
+{
+	struct resource *res = alloc_resource(GFP_KERNEL);
+
+	if (!res)
+		return NULL;
+
+	res->name = name;
+	res->start = start;
+	res->end = start + n - 1;
+
+	if (!__request_declared_region(parent, res, flags)) {
+		free_resource(res);
+		res = NULL;
+	}
+
+	return res;
+}
 EXPORT_SYMBOL(__request_region);
 
 /**
-- 
2.9.4

[PATCH 2/5] Modify behaviour of request_*muxed_region()

[Zoltán Böszörményi]

In order to make request_*muxed_region() behave more like mutex_lock(),
a possible failure case needs to be eliminated. When drivers do not
properly share the same I/O region, e.g. one is using request_region()
and the other is using request_muxed_region(), the kernel didn't
warn the user about it. This change modifies IORESOURCE_MUXED behaviour
so it always goes to sleep waiting for the resuorce to be freed
and the inconsistent resource flag usage is logged with KERN_ERR.

Signed-off-by: Zoltán Böszörményi <zboszor@pr.hu>
---
 kernel/resource.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/kernel/resource.c b/kernel/resource.c
index 220f695..59fa426 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -1150,7 +1150,9 @@ struct resource * __request_declared_region(struct resource *parent,
 				continue;
 			}
 		}
-		if (conflict->flags & flags & IORESOURCE_MUXED) {
+		if (flags & IORESOURCE_MUXED) {
+			if (!(conflict->flags & IORESOURCE_MUXED))
+				printk(KERN_ERR "Resource conflict between muxed \"%s\" and non-muxed \"%s\" I/O regions!\n", res->name, conflict->name);
 			add_wait_queue(&muxed_resource_wait, &wait);
 			write_unlock(&resource_lock);
 			set_current_state(TASK_UNINTERRUPTIBLE);
-- 
2.9.4

[PATCH 3/5] usb: pci-quirks: Protect the I/O port pair of SB800

[Zoltán Böszörményi]

This patch uses the previously introduced macro called
request_declared_muxed_region() to synchronize access to
the I/O port pair 0xcd6 / 0xcd7 on SB800.

These I/O ports are also used by i2c-piix4 and sp5100_tco,
the next two patches port these drivers to use the new macro.

Signed-off-by: Zoltán Böszörményi <zboszor@pr.hu>
---
 drivers/usb/host/pci-quirks.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
index a9a1e4c..703eb65 100644
--- a/drivers/usb/host/pci-quirks.c
+++ b/drivers/usb/host/pci-quirks.c
@@ -314,11 +314,14 @@ static void usb_amd_quirk_pll(int disable)
 	if (amd_chipset.sb_type.gen == AMD_CHIPSET_SB800 ||
 			amd_chipset.sb_type.gen == AMD_CHIPSET_HUDSON2 ||
 			amd_chipset.sb_type.gen == AMD_CHIPSET_BOLTON) {
+		struct resource res = DEFINE_RES_IO_NAMED(0xcd6, 2, "USB host SB800/HUDSON2/BOLTON");
+		request_declared_muxed_region(&res);
 		outb_p(AB_REG_BAR_LOW, 0xcd6);
 		addr_low = inb_p(0xcd7);
 		outb_p(AB_REG_BAR_HIGH, 0xcd6);
 		addr_high = inb_p(0xcd7);
 		addr = addr_high << 8 | addr_low;
+		release_region(0xcd6, 2);
 
 		outl_p(0x30, AB_INDX(addr));
 		outl_p(0x40, AB_DATA(addr));
-- 
2.9.4

[PATCH 4/5] i2c: i2c-piix4: Use request_declared_muxed_region()

[Zoltán Böszörményi]

Use the new request_declared_muxed_region() macro to
synchronize access to the I/O port pair 0xcd6 / 0xcd7.

At the same time, remove the long lifetime request_region()
call to reserve these I/O ports, so the sp5100_tco watchdog
driver can also load.

This fixes an old regression in Linux 4.4-rc4, caused by:

commit 2fee61d22e606fc99ade9079fda15fdee83ec33e
Author: Christian Fetzer <fetzer.ch@gmail.com>
Date:   Thu Nov 19 20:13:48 2015 +0100

    i2c: piix4: Add support for multiplexed main adapter in SB800

Signed-off-by: Zoltán Böszörményi <zboszor@pr.hu>
---
 drivers/i2c/busses/i2c-piix4.c | 42 +++++++++++++-----------------------------
 1 file changed, 13 insertions(+), 29 deletions(-)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 0ecdb47..010daa8 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -144,10 +144,7 @@ static const struct dmi_system_id piix4_dmi_ibm[] = {
 
 /*
  * SB800 globals
- * piix4_mutex_sb800 protects piix4_port_sel_sb800 and the pair
- * of I/O ports at SB800_PIIX4_SMB_IDX.
  */
-static DEFINE_MUTEX(piix4_mutex_sb800);
 static u8 piix4_port_sel_sb800;
 static const char *piix4_main_port_names_sb800[PIIX4_MAX_ADAPTERS] = {
 	" port 0", " port 2", " port 3", " port 4"
@@ -158,7 +155,6 @@ struct i2c_piix4_adapdata {
 	unsigned short smba;
 
 	/* SB800 */
-	bool sb800_main;
 	u8 port;		/* Port number, shifted */
 };
 
@@ -264,6 +260,7 @@ static int piix4_setup(struct pci_dev *PIIX4_dev,
 static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
 			     const struct pci_device_id *id, u8 aux)
 {
+	struct resource res = DEFINE_RES_IO_NAMED(SB800_PIIX4_SMB_IDX, 2, "i2c-piix4");
 	unsigned short piix4_smba;
 	u8 smba_en_lo, smba_en_hi, smb_en, smb_en_status, port_sel;
 	u8 i2ccfg, i2ccfg_offset = 0x10;
@@ -286,12 +283,12 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
 	else
 		smb_en = (aux) ? 0x28 : 0x2c;
 
-	mutex_lock(&piix4_mutex_sb800);
+	request_declared_muxed_region(&res);
 	outb_p(smb_en, SB800_PIIX4_SMB_IDX);
 	smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1);
 	outb_p(smb_en + 1, SB800_PIIX4_SMB_IDX);
 	smba_en_hi = inb_p(SB800_PIIX4_SMB_IDX + 1);
-	mutex_unlock(&piix4_mutex_sb800);
+	release_region(SB800_PIIX4_SMB_IDX, 2);
 
 	if (!smb_en) {
 		smb_en_status = smba_en_lo & 0x10;
@@ -349,13 +346,14 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
 	if (PIIX4_dev->vendor == PCI_VENDOR_ID_AMD) {
 		piix4_port_sel_sb800 = SB800_PIIX4_PORT_IDX_ALT;
 	} else {
-		mutex_lock(&piix4_mutex_sb800);
+		struct resource res = DEFINE_RES_IO_NAMED(SB800_PIIX4_SMB_IDX, 2, "i2c-piix4");
+		request_declared_muxed_region(&res);
 		outb_p(SB800_PIIX4_PORT_IDX_SEL, SB800_PIIX4_SMB_IDX);
 		port_sel = inb_p(SB800_PIIX4_SMB_IDX + 1);
 		piix4_port_sel_sb800 = (port_sel & 0x01) ?
 				       SB800_PIIX4_PORT_IDX_ALT :
 				       SB800_PIIX4_PORT_IDX;
-		mutex_unlock(&piix4_mutex_sb800);
+		release_region(SB800_PIIX4_SMB_IDX, 2);
 	}
 
 	dev_info(&PIIX4_dev->dev,
@@ -584,6 +582,7 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
 		 unsigned short flags, char read_write,
 		 u8 command, int size, union i2c_smbus_data *data)
 {
+	struct resource res = DEFINE_RES_IO_NAMED(SB800_PIIX4_SMB_IDX, 2, "i2c-piix4");
 	struct i2c_piix4_adapdata *adapdata = i2c_get_adapdata(adap);
 	unsigned short piix4_smba = adapdata->smba;
 	int retries = MAX_TIMEOUT;
@@ -592,7 +591,7 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
 	u8 port;
 	int retval;
 
-	mutex_lock(&piix4_mutex_sb800);
+	request_declared_muxed_region(&res);
 
 	/* Request the SMBUS semaphore, avoid conflicts with the IMC */
 	smbslvcnt  = inb_p(SMBSLVCNT);
@@ -608,7 +607,7 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
 	} while (--retries);
 	/* SMBus is still owned by the IMC, we give up */
 	if (!retries) {
-		mutex_unlock(&piix4_mutex_sb800);
+		release_region(SB800_PIIX4_SMB_IDX, 2);
 		return -EBUSY;
 	}
 
@@ -628,7 +627,7 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
 	/* Release the semaphore */
 	outb_p(smbslvcnt | 0x20, SMBSLVCNT);
 
-	mutex_unlock(&piix4_mutex_sb800);
+	release_region(SB800_PIIX4_SMB_IDX, 2);
 
 	return retval;
 }
@@ -705,7 +704,6 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
 	}
 
 	adapdata->smba = smba;
-	adapdata->sb800_main = sb800_main;
 	adapdata->port = port << 1;
 
 	/* set up the sysfs linkage to our parent device */
@@ -771,29 +769,18 @@ static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	    dev->vendor == PCI_VENDOR_ID_AMD) {
 		is_sb800 = true;
 
-		if (!request_region(SB800_PIIX4_SMB_IDX, 2, "smba_idx")) {
-			dev_err(&dev->dev,
-			"SMBus base address index region 0x%x already in use!\n",
-			SB800_PIIX4_SMB_IDX);
-			return -EBUSY;
-		}
-
 		/* base address location etc changed in SB800 */
 		retval = piix4_setup_sb800(dev, id, 0);
-		if (retval < 0) {
-			release_region(SB800_PIIX4_SMB_IDX, 2);
+		if (retval < 0)
 			return retval;
-		}
 
 		/*
 		 * Try to register multiplexed main SMBus adapter,
 		 * give up if we can't
 		 */
 		retval = piix4_add_adapters_sb800(dev, retval);
-		if (retval < 0) {
-			release_region(SB800_PIIX4_SMB_IDX, 2);
+		if (retval < 0)
 			return retval;
-		}
 	} else {
 		retval = piix4_setup(dev, id);
 		if (retval < 0)
@@ -841,11 +828,8 @@ static void piix4_adap_remove(struct i2c_adapter *adap)
 
 	if (adapdata->smba) {
 		i2c_del_adapter(adap);
-		if (adapdata->port == (0 << 1)) {
+		if (adapdata->port == (0 << 1))
 			release_region(adapdata->smba, SMBIOSIZE);
-			if (adapdata->sb800_main)
-				release_region(SB800_PIIX4_SMB_IDX, 2);
-		}
 		kfree(adapdata);
 		kfree(adap);
 	}
-- 
2.9.4

[PATCH 5/5] watchdog: sp5100_tco: Use request_declared_muxed_region()

[Zoltán Böszörményi]

Use the new request_declared_muxed_region() macro to synchronize
accesses to the SB800 I/O port pair (0xcd6 / 0xcd7) with the
PCI quirk for isochronous USB transfers and with the i2c-piix4
driver.

At the same time, remove the long lifetime request_region() call
to reserve these I/O ports, similarly to i2c-piix4 so the code is
now uniform across the three drivers.

Signed-off-by: Zoltán Böszörményi <zboszor@pr.hu>
---
 drivers/watchdog/sp5100_tco.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
index 028618c..9bb3bcb 100644
--- a/drivers/watchdog/sp5100_tco.c
+++ b/drivers/watchdog/sp5100_tco.c
@@ -48,7 +48,6 @@
 static u32 tcobase_phys;
 static u32 tco_wdt_fired;
 static void __iomem *tcobase;
-static unsigned int pm_iobase;
 static DEFINE_SPINLOCK(tco_lock);	/* Guards the hardware */
 static unsigned long timer_alive;
 static char tco_expect_close;
@@ -134,11 +133,13 @@ static int tco_timer_set_heartbeat(int t)
 
 static void tco_timer_enable(void)
 {
+	struct resource res = DEFINE_RES_IO_NAMED(SB800_IO_PM_INDEX_REG, SP5100_PM_IOPORTS_SIZE, TCO_MODULE_NAME);
 	int val;
 
 	if (!tco_has_sp5100_reg_layout(sp5100_tco_pci)) {
 		/* For SB800 or later */
 		/* Set the Watchdog timer resolution to 1 sec */
+		request_declared_muxed_region(&res);
 		outb(SB800_PM_WATCHDOG_CONFIG, SB800_IO_PM_INDEX_REG);
 		val = inb(SB800_IO_PM_DATA_REG);
 		val |= SB800_PM_WATCHDOG_SECOND_RES;
@@ -150,6 +151,7 @@ static void tco_timer_enable(void)
 		val |= SB800_PCI_WATCHDOG_DECODE_EN;
 		val &= ~SB800_PM_WATCHDOG_DISABLE;
 		outb(val, SB800_IO_PM_DATA_REG);
+		release_region(SB800_IO_PM_INDEX_REG, SP5100_PM_IOPORTS_SIZE);
 	} else {
 		/* For SP5100 or SB7x0 */
 		/* Enable watchdog decode bit */
@@ -164,11 +166,13 @@ static void tco_timer_enable(void)
 				       val);
 
 		/* Enable Watchdog timer and set the resolution to 1 sec */
+		request_declared_muxed_region(&res);
 		outb(SP5100_PM_WATCHDOG_CONTROL, SP5100_IO_PM_INDEX_REG);
 		val = inb(SP5100_IO_PM_DATA_REG);
 		val |= SP5100_PM_WATCHDOG_SECOND_RES;
 		val &= ~SP5100_PM_WATCHDOG_DISABLE;
 		outb(val, SP5100_IO_PM_DATA_REG);
+		release_region(SB800_IO_PM_INDEX_REG, 2);
 	}
 }
 
@@ -326,6 +330,7 @@ MODULE_DEVICE_TABLE(pci, sp5100_tco_pci_tbl);
  */
 static unsigned char sp5100_tco_setupdevice(void)
 {
+	struct resource res = DEFINE_RES_IO_NAMED(SP5100_IO_PM_INDEX_REG, SP5100_PM_IOPORTS_SIZE, TCO_MODULE_NAME);
 	struct pci_dev *dev = NULL;
 	const char *dev_name = NULL;
 	u32 val;
@@ -361,16 +366,10 @@ static unsigned char sp5100_tco_setupdevice(void)
 		base_addr = SB800_PM_WATCHDOG_BASE;
 	}
 
-	/* Request the IO ports used by this driver */
-	pm_iobase = SP5100_IO_PM_INDEX_REG;
-	if (!request_region(pm_iobase, SP5100_PM_IOPORTS_SIZE, dev_name)) {
-		pr_err("I/O address 0x%04x already in use\n", pm_iobase);
-		goto exit;
-	}
-
 	/*
 	 * First, Find the watchdog timer MMIO address from indirect I/O.
 	 */
+	request_declared_muxed_region(&res);
 	outb(base_addr+3, index_reg);
 	val = inb(data_reg);
 	outb(base_addr+2, index_reg);
@@ -380,6 +379,7 @@ static unsigned char sp5100_tco_setupdevice(void)
 	outb(base_addr+0, index_reg);
 	/* Low three bits of BASE are reserved */
 	val = val << 8 | (inb(data_reg) & 0xf8);
+	release_region(SP5100_IO_PM_INDEX_REG, SP5100_PM_IOPORTS_SIZE);
 
 	pr_debug("Got 0x%04x from indirect I/O\n", val);
 
@@ -400,6 +400,7 @@ static unsigned char sp5100_tco_setupdevice(void)
 				      SP5100_SB_RESOURCE_MMIO_BASE, &val);
 	} else {
 		/* Read SBResource_MMIO from AcpiMmioEn(PM_Reg: 24h) */
+		request_declared_muxed_region(&res);
 		outb(SB800_PM_ACPI_MMIO_EN+3, SB800_IO_PM_INDEX_REG);
 		val = inb(SB800_IO_PM_DATA_REG);
 		outb(SB800_PM_ACPI_MMIO_EN+2, SB800_IO_PM_INDEX_REG);
@@ -408,6 +409,7 @@ static unsigned char sp5100_tco_setupdevice(void)
 		val = val << 8 | inb(SB800_IO_PM_DATA_REG);
 		outb(SB800_PM_ACPI_MMIO_EN+0, SB800_IO_PM_INDEX_REG);
 		val = val << 8 | inb(SB800_IO_PM_DATA_REG);
+		release_region(SP5100_IO_PM_INDEX_REG, SP5100_PM_IOPORTS_SIZE);
 	}
 
 	/* The SBResource_MMIO is enabled and mapped memory space? */
@@ -429,7 +431,7 @@ static unsigned char sp5100_tco_setupdevice(void)
 		pr_debug("SBResource_MMIO is disabled(0x%04x)\n", val);
 
 	pr_notice("failed to find MMIO address, giving up.\n");
-	goto  unreg_region;
+	goto  exit;
 
 setup_wdt:
 	tcobase_phys = val;
@@ -469,8 +471,6 @@ static unsigned char sp5100_tco_setupdevice(void)
 
 unreg_mem_region:
 	release_mem_region(tcobase_phys, SP5100_WDT_MEM_MAP_SIZE);
-unreg_region:
-	release_region(pm_iobase, SP5100_PM_IOPORTS_SIZE);
 exit:
 	return 0;
 }
@@ -517,7 +517,7 @@ static int sp5100_tco_init(struct platform_device *dev)
 exit:
 	iounmap(tcobase);
 	release_mem_region(tcobase_phys, SP5100_WDT_MEM_MAP_SIZE);
-	release_region(pm_iobase, SP5100_PM_IOPORTS_SIZE);
+	release_region(SB800_IO_PM_INDEX_REG, SP5100_PM_IOPORTS_SIZE);
 	return ret;
 }
 
@@ -531,7 +531,6 @@ static void sp5100_tco_cleanup(void)
 	misc_deregister(&sp5100_tco_miscdev);
 	iounmap(tcobase);
 	release_mem_region(tcobase_phys, SP5100_WDT_MEM_MAP_SIZE);
-	release_region(pm_iobase, SP5100_PM_IOPORTS_SIZE);
 }
 
 static int sp5100_tco_remove(struct platform_device *dev)
-- 
2.9.4

Re: [PATCH 5/5] watchdog: sp5100_tco: Use request_declared_muxed_region()

[Guenter Roeck]

On Wed, Jun 21, 2017 at 05:53:49AM +0200, Zoltán Böszörményi wrote:
> Use the new request_declared_muxed_region() macro to synchronize
> accesses to the SB800 I/O port pair (0xcd6 / 0xcd7) with the
> PCI quirk for isochronous USB transfers and with the i2c-piix4
> driver.
> 
> At the same time, remove the long lifetime request_region() call
> to reserve these I/O ports, similarly to i2c-piix4 so the code is
> now uniform across the three drivers.
> 
> Signed-off-by: Zoltán Böszörményi <zboszor@pr.hu>
> ---
>  drivers/watchdog/sp5100_tco.c | 25 ++++++++++++-------------
>  1 file changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
> index 028618c..9bb3bcb 100644
> --- a/drivers/watchdog/sp5100_tco.c
> +++ b/drivers/watchdog/sp5100_tco.c
> @@ -48,7 +48,6 @@
>  static u32 tcobase_phys;
>  static u32 tco_wdt_fired;
>  static void __iomem *tcobase;
> -static unsigned int pm_iobase;
>  static DEFINE_SPINLOCK(tco_lock);	/* Guards the hardware */
>  static unsigned long timer_alive;
>  static char tco_expect_close;
> @@ -134,11 +133,13 @@ static int tco_timer_set_heartbeat(int t)
>  
>  static void tco_timer_enable(void)
>  {
> +	struct resource res = DEFINE_RES_IO_NAMED(SB800_IO_PM_INDEX_REG, SP5100_PM_IOPORTS_SIZE, TCO_MODULE_NAME);

Is it necessary to have this as a local variable ?
If not, please declare as static variable.

Either case, please run our patches through checkpatch and address 
any problems it reports.

>  	int val;
>  
>  	if (!tco_has_sp5100_reg_layout(sp5100_tco_pci)) {
>  		/* For SB800 or later */
>  		/* Set the Watchdog timer resolution to 1 sec */
> +		request_declared_muxed_region(&res);
>  		outb(SB800_PM_WATCHDOG_CONFIG, SB800_IO_PM_INDEX_REG);
>  		val = inb(SB800_IO_PM_DATA_REG);
>  		val |= SB800_PM_WATCHDOG_SECOND_RES;
> @@ -150,6 +151,7 @@ static void tco_timer_enable(void)
>  		val |= SB800_PCI_WATCHDOG_DECODE_EN;
>  		val &= ~SB800_PM_WATCHDOG_DISABLE;
>  		outb(val, SB800_IO_PM_DATA_REG);
> +		release_region(SB800_IO_PM_INDEX_REG, SP5100_PM_IOPORTS_SIZE);
>  	} else {
>  		/* For SP5100 or SB7x0 */
>  		/* Enable watchdog decode bit */
> @@ -164,11 +166,13 @@ static void tco_timer_enable(void)
>  				       val);
>  
>  		/* Enable Watchdog timer and set the resolution to 1 sec */
> +		request_declared_muxed_region(&res);
>  		outb(SP5100_PM_WATCHDOG_CONTROL, SP5100_IO_PM_INDEX_REG);
>  		val = inb(SP5100_IO_PM_DATA_REG);
>  		val |= SP5100_PM_WATCHDOG_SECOND_RES;
>  		val &= ~SP5100_PM_WATCHDOG_DISABLE;
>  		outb(val, SP5100_IO_PM_DATA_REG);
> +		release_region(SB800_IO_PM_INDEX_REG, 2);
>  	}
>  }
>  
> @@ -326,6 +330,7 @@ MODULE_DEVICE_TABLE(pci, sp5100_tco_pci_tbl);
>   */
>  static unsigned char sp5100_tco_setupdevice(void)
>  {
> +	struct resource res = DEFINE_RES_IO_NAMED(SP5100_IO_PM_INDEX_REG, SP5100_PM_IOPORTS_SIZE, TCO_MODULE_NAME);
>  	struct pci_dev *dev = NULL;
>  	const char *dev_name = NULL;
>  	u32 val;
> @@ -361,16 +366,10 @@ static unsigned char sp5100_tco_setupdevice(void)
>  		base_addr = SB800_PM_WATCHDOG_BASE;
>  	}
>  
> -	/* Request the IO ports used by this driver */
> -	pm_iobase = SP5100_IO_PM_INDEX_REG;
> -	if (!request_region(pm_iobase, SP5100_PM_IOPORTS_SIZE, dev_name)) {
> -		pr_err("I/O address 0x%04x already in use\n", pm_iobase);
> -		goto exit;
> -	}
> -
>  	/*
>  	 * First, Find the watchdog timer MMIO address from indirect I/O.
>  	 */
> +	request_declared_muxed_region(&res);
>  	outb(base_addr+3, index_reg);
>  	val = inb(data_reg);
>  	outb(base_addr+2, index_reg);
> @@ -380,6 +379,7 @@ static unsigned char sp5100_tco_setupdevice(void)
>  	outb(base_addr+0, index_reg);
>  	/* Low three bits of BASE are reserved */
>  	val = val << 8 | (inb(data_reg) & 0xf8);
> +	release_region(SP5100_IO_PM_INDEX_REG, SP5100_PM_IOPORTS_SIZE);
>  
>  	pr_debug("Got 0x%04x from indirect I/O\n", val);
>  
> @@ -400,6 +400,7 @@ static unsigned char sp5100_tco_setupdevice(void)
>  				      SP5100_SB_RESOURCE_MMIO_BASE, &val);
>  	} else {
>  		/* Read SBResource_MMIO from AcpiMmioEn(PM_Reg: 24h) */
> +		request_declared_muxed_region(&res);
>  		outb(SB800_PM_ACPI_MMIO_EN+3, SB800_IO_PM_INDEX_REG);
>  		val = inb(SB800_IO_PM_DATA_REG);
>  		outb(SB800_PM_ACPI_MMIO_EN+2, SB800_IO_PM_INDEX_REG);
> @@ -408,6 +409,7 @@ static unsigned char sp5100_tco_setupdevice(void)
>  		val = val << 8 | inb(SB800_IO_PM_DATA_REG);
>  		outb(SB800_PM_ACPI_MMIO_EN+0, SB800_IO_PM_INDEX_REG);
>  		val = val << 8 | inb(SB800_IO_PM_DATA_REG);
> +		release_region(SP5100_IO_PM_INDEX_REG, SP5100_PM_IOPORTS_SIZE);
>  	}
>  
>  	/* The SBResource_MMIO is enabled and mapped memory space? */
> @@ -429,7 +431,7 @@ static unsigned char sp5100_tco_setupdevice(void)
>  		pr_debug("SBResource_MMIO is disabled(0x%04x)\n", val);
>  
>  	pr_notice("failed to find MMIO address, giving up.\n");
> -	goto  unreg_region;
> +	goto  exit;
>  
>  setup_wdt:
>  	tcobase_phys = val;
> @@ -469,8 +471,6 @@ static unsigned char sp5100_tco_setupdevice(void)
>  
>  unreg_mem_region:
>  	release_mem_region(tcobase_phys, SP5100_WDT_MEM_MAP_SIZE);
> -unreg_region:
> -	release_region(pm_iobase, SP5100_PM_IOPORTS_SIZE);
>  exit:
>  	return 0;
>  }
> @@ -517,7 +517,7 @@ static int sp5100_tco_init(struct platform_device *dev)
>  exit:
>  	iounmap(tcobase);
>  	release_mem_region(tcobase_phys, SP5100_WDT_MEM_MAP_SIZE);
> -	release_region(pm_iobase, SP5100_PM_IOPORTS_SIZE);
> +	release_region(SB800_IO_PM_INDEX_REG, SP5100_PM_IOPORTS_SIZE);
>  	return ret;
>  }
>  
> @@ -531,7 +531,6 @@ static void sp5100_tco_cleanup(void)
>  	misc_deregister(&sp5100_tco_miscdev);
>  	iounmap(tcobase);
>  	release_mem_region(tcobase_phys, SP5100_WDT_MEM_MAP_SIZE);
> -	release_region(pm_iobase, SP5100_PM_IOPORTS_SIZE);
>  }
>  
>  static int sp5100_tco_remove(struct platform_device *dev)
> -- 
> 2.9.4
> 

[PATCH 0/5 v4] Fix sp5100_tco watchdog driver regression

[Zoltán Böszörményi]

This patch series fixes a regression introduced by:

commit 2fee61d22e606fc99ade9079fda15fdee83ec33e
Author: Christian Fetzer <fetzer.ch@gmail.com>
Date:   Thu Nov 19 20:13:48 2015 +0100

    i2c: piix4: Add support for multiplexed main adapter in SB800

The regression caused sp5100_tco fail to load:

sp5100_tco: SP5100/SB800 TCO WatchDog Timer Driver v0.05
sp5100_tco: PCI Vendor ID: 0x1002, Device ID: 0x4385, Revision ID: 0x42
sp5100_tco: I/O address 0x0cd6 already in use

Notable bugzilla links about this issue:
https://bugzilla.kernel.org/show_bug.cgi?id=170741
https://bugzilla.redhat.com/show_bug.cgi?id=1369269
https://bugzilla.redhat.com/show_bug.cgi?id=1406844

The previous two versions of this patch series introduced
a common mutex to synchronize access to the I/O port pair
0xcd6 / 0xcd7 used by the AMD SB800 USB PCI quirk code and
the i2c-piix and sp5100_tco drivers. The common mutex was
criticized because it introduces an inter-dependency between
drivers.

This approach modifies the request_muxed_region() semantics and
modifies the possible use cases.

The first patch in the series adds a new IORESOURCE_ALLOCATED
flag that alloc_resource() sets and free_resource() considers.
The core of __request_region() is factored out into a new function
that doesn't allocate. With this change, drivers can use the
pre-existing DEFINE_RES_IO_NAMED() static initialized macro
to declare struct resource statically (e.g. on the stack)
and pass the address of it to the new __request_declared_region()
function. A new macro called request_declared_muxed_region()
was added to exploit this functionality. Because of the new
IORESOURCE_ALLOCATED resource flag, release_region() can still
be called with the old interface (the port region start and
end values) and it won't attempt to free a non-allocated resource.
This eliminated one failure case that can come from allocation
errors.

The second patch modifies the behaviour of IORESOURCE_MUXED,
a.k.a. the request_*muxed_region() macros. When these macros
are called, the caller goes to sleep when there is any conflicting
regions, even if the conflicting region did not use the
IORESOURCE_MUXED flag. The kernel logs this inconsistent
flag usage with KERN_ERR. This change eliminates the second
failure case for IORESOURCE_MUXED and request_muxed_region()
can be used like mutex_lock(), i.e. it returns only in case it
could successfully request the region.

The last three patches adds proper synchronization between the
USB PCI quirks code and the i2c-piix and sp5100_tco drivers.

The result is that the sp5100_tco driver can load and works again:

sp5100_tco: SP5100/SB800 TCO WatchDog Timer Driver v0.05
sp5100_tco: PCI Vendor ID: 0x1002, Device ID: 0x4385, Revision ID: 0x42
sp5100_tco: Using 0xfed80b00 for watchdog MMIO address
sp5100_tco: Last reboot was not triggered by watchdog.
sp5100_tco: initialized (0xffffba2f4192db00). heartbeat=60 sec (nowayout=0)

Signed-off-by: Zoltán Böszörményi <zboszor@pr.hu>
---
 drivers/i2c/busses/i2c-piix4.c | 41 ++++++++++++-------------------------
 drivers/usb/host/pci-quirks.c  |  4 ++++
 drivers/watchdog/sp5100_tco.c  | 28 +++++++++++++------------
 include/linux/ioport.h         | 14 +++++++++++++
 kernel/resource.c              | 46 ++++++++++++++++++++++++++++++++++++++----
 5 files changed, 88 insertions(+), 45 deletions(-)

[PATCH 1/5 v2] Extend the request_region() infrastructure

[Zoltán Böszörményi]

Add a new IORESOURCE_ALLOCATED flag that is automatically used
when alloc_resource() is used internally in kernel/resource.c
and free_resource() now takes this flag into account.

The core of __request_region() was factored out into a new function
called __request_declared_region() that needs struct resource *
instead of the (start, n, name) triplet.

These changes allow using statically declared struct resource
data coupled with the pre-existing DEFINE_RES_IO_NAMED() static
initializer macro. The new macro exploiting
__request_declared_region() is request_declared_muxed_region()

v2:

Fixed checkpatch.pl warnings and errors and extended the macro
API with request_declared_region() and release_declared_region()

Reversed the order of __request_declared_region and __request_region

Added high level description of the muxed and declared variants
of the macros.

Signed-off-by: Zoltán Böszörményi <zboszor@pr.hu>
---
 include/linux/ioport.h | 14 ++++++++++++++
 kernel/resource.c      | 40 +++++++++++++++++++++++++++++++++++++---
 2 files changed, 51 insertions(+), 3 deletions(-)

diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index 6230064..6ebcd39 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -52,6 +52,7 @@ struct resource {
 #define IORESOURCE_MEM_64	0x00100000
 #define IORESOURCE_WINDOW	0x00200000	/* forwarded by bridge */
 #define IORESOURCE_MUXED	0x00400000	/* Resource is software muxed */
+#define IORESOURCE_ALLOCATED	0x00800000	/* Resource was allocated */
 
 #define IORESOURCE_EXT_TYPE_BITS 0x01000000	/* Resource extended types */
 #define IORESOURCE_SYSRAM	0x01000000	/* System RAM (modifier) */
@@ -215,7 +216,14 @@ static inline bool resource_contains(struct resource *r1, struct resource *r2)
 
 /* Convenience shorthand with allocation */
 #define request_region(start,n,name)		__request_region(&ioport_resource, (start), (n), (name), 0)
+#define request_declared_region(res)		__request_region( \
+							&ioport_resource, \
+							(res), 0)
 #define request_muxed_region(start,n,name)	__request_region(&ioport_resource, (start), (n), (name), IORESOURCE_MUXED)
+#define request_declared_muxed_region(res)	__request_declared_region( \
+							&ioport_resource, \
+							(res), \
+							IORESOURCE_MUXED)
 #define __request_mem_region(start,n,name, excl) __request_region(&iomem_resource, (start), (n), (name), excl)
 #define request_mem_region(start,n,name) __request_region(&iomem_resource, (start), (n), (name), 0)
 #define request_mem_region_exclusive(start,n,name) \
@@ -227,8 +235,14 @@ extern struct resource * __request_region(struct resource *,
 					resource_size_t n,
 					const char *name, int flags);
 
+extern struct resource *__request_declared_region(struct resource *parent,
+					struct resource *res, int flags);
+
 /* Compatibility cruft */
 #define release_region(start,n)	__release_region(&ioport_resource, (start), (n))
+#define release_declared_region(res)	__release_region(&ioport_resource, \
+						(res)->start, \
+						(res)->end - (res)->start + 1)
 #define release_mem_region(start,n)	__release_region(&iomem_resource, (start), (n))
 
 extern void __release_region(struct resource *, resource_size_t,
diff --git a/kernel/resource.c b/kernel/resource.c
index 9b5f044..2be7029 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -184,6 +184,9 @@ static void free_resource(struct resource *res)
 	if (!res)
 		return;
 
+	if (!(res->flags & IORESOURCE_ALLOCATED))
+		return;
+
 	if (!PageSlab(virt_to_head_page(res))) {
 		spin_lock(&bootmem_resource_lock);
 		res->sibling = bootmem_resource_free;
@@ -210,6 +213,8 @@ static struct resource *alloc_resource(gfp_t flags)
 	else
 		res = kzalloc(sizeof(struct resource), flags);
 
+	res->flags = IORESOURCE_ALLOCATED;
+
 	return res;
 }
 
@@ -1110,8 +1115,19 @@ resource_size_t resource_alignment(struct resource *res)
  * the IO flag meanings (busy etc).
  *
  * request_region creates a new busy region.
+ * The resource descriptor is allocated by this function.
+ *
+ * request_declared_region creates a new busy region
+ * described in an existing resource descriptor.
+ *
+ * request_muxed_region creates a new shared busy region.
+ * The resource descriptor is allocated by this function.
+ *
+ * request_declared_muxed_region creates a new shared busy region
+ * described in an existing resource descriptor.
  *
  * release_region releases a matching busy region.
+ * The region is only freed if it was allocated.
  */
 
 static DECLARE_WAIT_QUEUE_HEAD(muxed_resource_wait);
@@ -1128,7 +1144,6 @@ struct resource * __request_region(struct resource *parent,
 				   resource_size_t start, resource_size_t n,
 				   const char *name, int flags)
 {
-	DECLARE_WAITQUEUE(wait, current);
 	struct resource *res = alloc_resource(GFP_KERNEL);
 
 	if (!res)
@@ -1138,6 +1153,26 @@ struct resource * __request_region(struct resource *parent,
 	res->start = start;
 	res->end = start + n - 1;
 
+	if (!__request_declared_region(parent, res, flags)) {
+		free_resource(res);
+		res = NULL;
+	}
+
+	return res;
+}
+EXPORT_SYMBOL(__request_region);
+
+/**
+ * __request_declared_region - create a new busy resource region
+ * @parent: parent resource descriptor
+ * @res: child resource descriptor
+ * @flags: IO resource flags
+ */
+struct resource *__request_declared_region(struct resource *parent,
+				   struct resource *res, int flags)
+{
+	DECLARE_WAITQUEUE(wait, current);
+
 	write_lock(&resource_lock);
 
 	for (;;) {
@@ -1166,14 +1201,13 @@ struct resource * __request_region(struct resource *parent,
 			continue;
 		}
 		/* Uhhuh, that didn't work out.. */
-		free_resource(res);
 		res = NULL;
 		break;
 	}
 	write_unlock(&resource_lock);
 	return res;
 }
-EXPORT_SYMBOL(__request_region);
+EXPORT_SYMBOL(__request_declared_region);
 
 /**
  * __release_region - release a previously reserved resource region
-- 
2.9.4

[PATCH 2/5 v2] Modify behaviour of request_*muxed_region()

[Zoltán Böszörményi]

In order to make request_*muxed_region() behave more like
mutex_lock(), a possible failure case needs to be eliminated.
When drivers do not properly share the same I/O region, e.g.
one is using request_region() and the other is using
request_muxed_region(), the kernel didn't warn the user about it.
This change modifies IORESOURCE_MUXED behaviour so it always
goes to sleep waiting for the resuorce to be freed and the
inconsistent resource flag usage is logged with KERN_ERR.

v2: Fixed checkpatch.pl warnings and extended the comment
    about request_declared_muxed_region.

Signed-off-by: Zoltán Böszörményi <zboszor@pr.hu>
---
 kernel/resource.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/kernel/resource.c b/kernel/resource.c
index 2be7029..5df2731 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -1125,6 +1125,7 @@ resource_size_t resource_alignment(struct resource *res)
  *
  * request_declared_muxed_region creates a new shared busy region
  * described in an existing resource descriptor.
+ * It only returns if it succeeded.
  *
  * release_region releases a matching busy region.
  * The region is only freed if it was allocated.
@@ -1191,7 +1192,10 @@ struct resource *__request_declared_region(struct resource *parent,
 				continue;
 			}
 		}
-		if (conflict->flags & flags & IORESOURCE_MUXED) {
+		if (flags & IORESOURCE_MUXED) {
+			if (!(conflict->flags & IORESOURCE_MUXED))
+				pr_err("Resource conflict between muxed \"%s\" and non-muxed \"%s\" I/O regions!\n",
+					res->name, conflict->name);
 			add_wait_queue(&muxed_resource_wait, &wait);
 			write_unlock(&resource_lock);
 			set_current_state(TASK_UNINTERRUPTIBLE);
-- 
2.9.4

[PATCH 3/5 v4] usb: pci-quirks: Protect the I/O port pair of SB800

[Zoltán Böszörményi]

This patch uses the previously introduced macro called
request_declared_muxed_region() to synchronize access to
the I/O port pair 0xcd6 / 0xcd7 on SB800.

These I/O ports are also used by i2c-piix4 and sp5100_tco,
so synchronization is necessary. The other drivers will also
be modified to use the new macro in subsequest patched.

v1: Started with a common mutex in a C source file.

v2: Declared the common mutex in drivers/usb/host/pci-quirks.c
    instead of in a common C file.

v3: Switched to using the new request_declared_muxed_region
    macro.

v4: Fixed checkpatch.pl warnings and use the new
    release_declared_region() macro.

Signed-off-by: Zoltán Böszörményi <zboszor@pr.hu>
---
 drivers/usb/host/pci-quirks.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
index a9a1e4c..593942a 100644
--- a/drivers/usb/host/pci-quirks.c
+++ b/drivers/usb/host/pci-quirks.c
@@ -279,6 +279,8 @@ bool usb_amd_prefetch_quirk(void)
 }
 EXPORT_SYMBOL_GPL(usb_amd_prefetch_quirk);
 
+static struct resource sb800_res = DEFINE_RES_IO_NAMED(0xcd6, 2, "SB800 USB");
+
 /*
  * The hardware normally enables the A-link power management feature, which
  * lets the system lower the power consumption in idle states.
@@ -314,11 +316,13 @@ static void usb_amd_quirk_pll(int disable)
 	if (amd_chipset.sb_type.gen == AMD_CHIPSET_SB800 ||
 			amd_chipset.sb_type.gen == AMD_CHIPSET_HUDSON2 ||
 			amd_chipset.sb_type.gen == AMD_CHIPSET_BOLTON) {
+		request_declared_muxed_region(&sb800_res);
 		outb_p(AB_REG_BAR_LOW, 0xcd6);
 		addr_low = inb_p(0xcd7);
 		outb_p(AB_REG_BAR_HIGH, 0xcd6);
 		addr_high = inb_p(0xcd7);
 		addr = addr_high << 8 | addr_low;
+		release_declared_region(&sb800_res);
 
 		outl_p(0x30, AB_INDX(addr));
 		outl_p(0x40, AB_DATA(addr));
-- 
2.9.4

[PATCH 4/5 v4] i2c: i2c-piix4: Use request_declared_muxed_region()

[Zoltán Böszörményi]

Use the new request_declared_muxed_region() macro to
synchronize access to the I/O port pair 0xcd6 / 0xcd7.

At the same time, remove the long lifetime request_region()
call to reserve these I/O ports, so the sp5100_tco watchdog
driver can also load.

This fixes an old regression in Linux 4.4-rc4, caused by
commit 2fee61d22e60 ("i2c: piix4: Add support for multiplexed
main adapter in SB800")

v1: Started with a common mutex in a C source file.

v2: Referenced to common mutex from drivers/usb/host/pci-quirks.c

v3: Switched to using the new request_declared_muxed_region
    macro.

v4: Fixed checkpatch.pl warnings and use the new
    release_declared_region() macro.

Signed-off-by: Zoltán Böszörményi <zboszor@pr.hu>
---
 drivers/i2c/busses/i2c-piix4.c | 41 +++++++++++++----------------------------
 1 file changed, 13 insertions(+), 28 deletions(-)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 0ecdb47..a204344 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -144,10 +144,11 @@ static const struct dmi_system_id piix4_dmi_ibm[] = {
 
 /*
  * SB800 globals
- * piix4_mutex_sb800 protects piix4_port_sel_sb800 and the pair
+ * request_declared_muxed_region() protects piix4_port_sel_sb800 and the pair
  * of I/O ports at SB800_PIIX4_SMB_IDX.
  */
-static DEFINE_MUTEX(piix4_mutex_sb800);
+static struct resource sb800_res = DEFINE_RES_IO_NAMED(SB800_PIIX4_SMB_IDX, 2,
+						 "i2c-piix4");
 static u8 piix4_port_sel_sb800;
 static const char *piix4_main_port_names_sb800[PIIX4_MAX_ADAPTERS] = {
 	" port 0", " port 2", " port 3", " port 4"
@@ -158,7 +159,6 @@ struct i2c_piix4_adapdata {
 	unsigned short smba;
 
 	/* SB800 */
-	bool sb800_main;
 	u8 port;		/* Port number, shifted */
 };
 
@@ -286,12 +286,12 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
 	else
 		smb_en = (aux) ? 0x28 : 0x2c;
 
-	mutex_lock(&piix4_mutex_sb800);
+	request_declared_muxed_region(&sb800_res);
 	outb_p(smb_en, SB800_PIIX4_SMB_IDX);
 	smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1);
 	outb_p(smb_en + 1, SB800_PIIX4_SMB_IDX);
 	smba_en_hi = inb_p(SB800_PIIX4_SMB_IDX + 1);
-	mutex_unlock(&piix4_mutex_sb800);
+	release_declared_region(&sb800_res);
 
 	if (!smb_en) {
 		smb_en_status = smba_en_lo & 0x10;
@@ -349,13 +349,13 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
 	if (PIIX4_dev->vendor == PCI_VENDOR_ID_AMD) {
 		piix4_port_sel_sb800 = SB800_PIIX4_PORT_IDX_ALT;
 	} else {
-		mutex_lock(&piix4_mutex_sb800);
+		request_declared_muxed_region(&sb800_res);
 		outb_p(SB800_PIIX4_PORT_IDX_SEL, SB800_PIIX4_SMB_IDX);
 		port_sel = inb_p(SB800_PIIX4_SMB_IDX + 1);
 		piix4_port_sel_sb800 = (port_sel & 0x01) ?
 				       SB800_PIIX4_PORT_IDX_ALT :
 				       SB800_PIIX4_PORT_IDX;
-		mutex_unlock(&piix4_mutex_sb800);
+		release_declared_region(&sb800_res);
 	}
 
 	dev_info(&PIIX4_dev->dev,
@@ -592,7 +592,7 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
 	u8 port;
 	int retval;
 
-	mutex_lock(&piix4_mutex_sb800);
+	request_declared_muxed_region(&sb800_res);
 
 	/* Request the SMBUS semaphore, avoid conflicts with the IMC */
 	smbslvcnt  = inb_p(SMBSLVCNT);
@@ -608,7 +608,7 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
 	} while (--retries);
 	/* SMBus is still owned by the IMC, we give up */
 	if (!retries) {
-		mutex_unlock(&piix4_mutex_sb800);
+		release_declared_region(&sb800_res);
 		return -EBUSY;
 	}
 
@@ -628,7 +628,7 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
 	/* Release the semaphore */
 	outb_p(smbslvcnt | 0x20, SMBSLVCNT);
 
-	mutex_unlock(&piix4_mutex_sb800);
+	release_declared_region(&sb800_res);
 
 	return retval;
 }
@@ -705,7 +705,6 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
 	}
 
 	adapdata->smba = smba;
-	adapdata->sb800_main = sb800_main;
 	adapdata->port = port << 1;
 
 	/* set up the sysfs linkage to our parent device */
@@ -771,29 +770,18 @@ static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	    dev->vendor == PCI_VENDOR_ID_AMD) {
 		is_sb800 = true;
 
-		if (!request_region(SB800_PIIX4_SMB_IDX, 2, "smba_idx")) {
-			dev_err(&dev->dev,
-			"SMBus base address index region 0x%x already in use!\n",
-			SB800_PIIX4_SMB_IDX);
-			return -EBUSY;
-		}
-
 		/* base address location etc changed in SB800 */
 		retval = piix4_setup_sb800(dev, id, 0);
-		if (retval < 0) {
-			release_region(SB800_PIIX4_SMB_IDX, 2);
+		if (retval < 0)
 			return retval;
-		}
 
 		/*
 		 * Try to register multiplexed main SMBus adapter,
 		 * give up if we can't
 		 */
 		retval = piix4_add_adapters_sb800(dev, retval);
-		if (retval < 0) {
-			release_region(SB800_PIIX4_SMB_IDX, 2);
+		if (retval < 0)
 			return retval;
-		}
 	} else {
 		retval = piix4_setup(dev, id);
 		if (retval < 0)
@@ -841,11 +829,8 @@ static void piix4_adap_remove(struct i2c_adapter *adap)
 
 	if (adapdata->smba) {
 		i2c_del_adapter(adap);
-		if (adapdata->port == (0 << 1)) {
+		if (adapdata->port == (0 << 1))
 			release_region(adapdata->smba, SMBIOSIZE);
-			if (adapdata->sb800_main)
-				release_region(SB800_PIIX4_SMB_IDX, 2);
-		}
 		kfree(adapdata);
 		kfree(adap);
 	}
-- 
2.9.4

[PATCH 5/5 v4] watchdog: sp5100_tco: Use request_declared_muxed_region()

[Zoltán Böszörményi]

Use the new request_declared_muxed_region() macro to synchronize
accesses to the SB800 I/O port pair (0xcd6 / 0xcd7) with the
PCI quirk for isochronous USB transfers and with the i2c-piix4
driver.

At the same time, remove the long lifetime request_region() call
to reserve these I/O ports, similarly to i2c-piix4 so the code is
now uniform across the three drivers.

v1: Started with a common mutex in a C source file.

v2: Referenced the common mutex from drivers/usb/host/pci-quirks.c

v3: Switched to using the new request_declared_muxed_region
    macro.

v4: Fixed checkpatch.pl warnings and use the new
    release_declared_region() macro.

Signed-off-by: Zoltán Böszörményi <zboszor@pr.hu>
---
 drivers/watchdog/sp5100_tco.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
index 028618c..cb42b72 100644
--- a/drivers/watchdog/sp5100_tco.c
+++ b/drivers/watchdog/sp5100_tco.c
@@ -48,7 +48,6 @@
 static u32 tcobase_phys;
 static u32 tco_wdt_fired;
 static void __iomem *tcobase;
-static unsigned int pm_iobase;
 static DEFINE_SPINLOCK(tco_lock);	/* Guards the hardware */
 static unsigned long timer_alive;
 static char tco_expect_close;
@@ -70,6 +69,11 @@ module_param(nowayout, bool, 0);
 MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started."
 		" (default=" __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
 
+/* synchronized access to the I/O port pair */
+static struct resource sp5100_res = DEFINE_RES_IO_NAMED(SB800_IO_PM_INDEX_REG,
+						SP5100_PM_IOPORTS_SIZE,
+						TCO_MODULE_NAME);
+
 /*
  * Some TCO specific functions
  */
@@ -139,6 +143,7 @@ static void tco_timer_enable(void)
 	if (!tco_has_sp5100_reg_layout(sp5100_tco_pci)) {
 		/* For SB800 or later */
 		/* Set the Watchdog timer resolution to 1 sec */
+		request_declared_muxed_region(&sp5100_res);
 		outb(SB800_PM_WATCHDOG_CONFIG, SB800_IO_PM_INDEX_REG);
 		val = inb(SB800_IO_PM_DATA_REG);
 		val |= SB800_PM_WATCHDOG_SECOND_RES;
@@ -150,6 +155,7 @@ static void tco_timer_enable(void)
 		val |= SB800_PCI_WATCHDOG_DECODE_EN;
 		val &= ~SB800_PM_WATCHDOG_DISABLE;
 		outb(val, SB800_IO_PM_DATA_REG);
+		release_declared_region(&sp5100_res);
 	} else {
 		/* For SP5100 or SB7x0 */
 		/* Enable watchdog decode bit */
@@ -164,11 +170,13 @@ static void tco_timer_enable(void)
 				       val);
 
 		/* Enable Watchdog timer and set the resolution to 1 sec */
+		request_declared_muxed_region(&sp5100_res);
 		outb(SP5100_PM_WATCHDOG_CONTROL, SP5100_IO_PM_INDEX_REG);
 		val = inb(SP5100_IO_PM_DATA_REG);
 		val |= SP5100_PM_WATCHDOG_SECOND_RES;
 		val &= ~SP5100_PM_WATCHDOG_DISABLE;
 		outb(val, SP5100_IO_PM_DATA_REG);
+		release_declared_region(&sp5100_res);
 	}
 }
 
@@ -361,16 +369,10 @@ static unsigned char sp5100_tco_setupdevice(void)
 		base_addr = SB800_PM_WATCHDOG_BASE;
 	}
 
-	/* Request the IO ports used by this driver */
-	pm_iobase = SP5100_IO_PM_INDEX_REG;
-	if (!request_region(pm_iobase, SP5100_PM_IOPORTS_SIZE, dev_name)) {
-		pr_err("I/O address 0x%04x already in use\n", pm_iobase);
-		goto exit;
-	}
-
 	/*
 	 * First, Find the watchdog timer MMIO address from indirect I/O.
 	 */
+	request_declared_muxed_region(&sp5100_res);
 	outb(base_addr+3, index_reg);
 	val = inb(data_reg);
 	outb(base_addr+2, index_reg);
@@ -380,6 +382,7 @@ static unsigned char sp5100_tco_setupdevice(void)
 	outb(base_addr+0, index_reg);
 	/* Low three bits of BASE are reserved */
 	val = val << 8 | (inb(data_reg) & 0xf8);
+	release_declared_region(&sp5100_res);
 
 	pr_debug("Got 0x%04x from indirect I/O\n", val);
 
@@ -400,6 +403,7 @@ static unsigned char sp5100_tco_setupdevice(void)
 				      SP5100_SB_RESOURCE_MMIO_BASE, &val);
 	} else {
 		/* Read SBResource_MMIO from AcpiMmioEn(PM_Reg: 24h) */
+		request_declared_muxed_region(&sp5100_res);
 		outb(SB800_PM_ACPI_MMIO_EN+3, SB800_IO_PM_INDEX_REG);
 		val = inb(SB800_IO_PM_DATA_REG);
 		outb(SB800_PM_ACPI_MMIO_EN+2, SB800_IO_PM_INDEX_REG);
@@ -408,6 +412,7 @@ static unsigned char sp5100_tco_setupdevice(void)
 		val = val << 8 | inb(SB800_IO_PM_DATA_REG);
 		outb(SB800_PM_ACPI_MMIO_EN+0, SB800_IO_PM_INDEX_REG);
 		val = val << 8 | inb(SB800_IO_PM_DATA_REG);
+		release_declared_region(&sp5100_res);
 	}
 
 	/* The SBResource_MMIO is enabled and mapped memory space? */
@@ -429,7 +434,7 @@ static unsigned char sp5100_tco_setupdevice(void)
 		pr_debug("SBResource_MMIO is disabled(0x%04x)\n", val);
 
 	pr_notice("failed to find MMIO address, giving up.\n");
-	goto  unreg_region;
+	goto  exit;
 
 setup_wdt:
 	tcobase_phys = val;
@@ -469,8 +474,6 @@ static unsigned char sp5100_tco_setupdevice(void)
 
 unreg_mem_region:
 	release_mem_region(tcobase_phys, SP5100_WDT_MEM_MAP_SIZE);
-unreg_region:
-	release_region(pm_iobase, SP5100_PM_IOPORTS_SIZE);
 exit:
 	return 0;
 }
@@ -517,7 +520,7 @@ static int sp5100_tco_init(struct platform_device *dev)
 exit:
 	iounmap(tcobase);
 	release_mem_region(tcobase_phys, SP5100_WDT_MEM_MAP_SIZE);
-	release_region(pm_iobase, SP5100_PM_IOPORTS_SIZE);
+	release_region(SB800_IO_PM_INDEX_REG, SP5100_PM_IOPORTS_SIZE);
 	return ret;
 }
 
@@ -531,7 +534,6 @@ static void sp5100_tco_cleanup(void)
 	misc_deregister(&sp5100_tco_miscdev);
 	iounmap(tcobase);
 	release_mem_region(tcobase_phys, SP5100_WDT_MEM_MAP_SIZE);
-	release_region(pm_iobase, SP5100_PM_IOPORTS_SIZE);
 }
 
 static int sp5100_tco_remove(struct platform_device *dev)
-- 
2.9.4

Re: [PATCH 0/5 v4] Fix sp5100_tco watchdog driver regression

[Boszormenyi Zoltan]

Hi,

ping for the series.

Adding Greg Kroah-Hartman to the cc: list, both for the USB core
and stable series maintainership.

2017-06-22 15:21 keltezéssel, Zoltán Böszörményi írta:
> This patch series fixes a regression introduced by:
> 
> commit 2fee61d22e606fc99ade9079fda15fdee83ec33e
> Author: Christian Fetzer <fetzer.ch@gmail.com>
> Date:   Thu Nov 19 20:13:48 2015 +0100
> 
>      i2c: piix4: Add support for multiplexed main adapter in SB800
> 
> The regression caused sp5100_tco fail to load:
> 
> sp5100_tco: SP5100/SB800 TCO WatchDog Timer Driver v0.05
> sp5100_tco: PCI Vendor ID: 0x1002, Device ID: 0x4385, Revision ID: 0x42
> sp5100_tco: I/O address 0x0cd6 already in use
> 
> Notable bugzilla links about this issue:
> https://bugzilla.kernel.org/show_bug.cgi?id=170741
> https://bugzilla.redhat.com/show_bug.cgi?id=1369269
> https://bugzilla.redhat.com/show_bug.cgi?id=1406844
> 
> The previous two versions of this patch series introduced
> a common mutex to synchronize access to the I/O port pair
> 0xcd6 / 0xcd7 used by the AMD SB800 USB PCI quirk code and
> the i2c-piix and sp5100_tco drivers. The common mutex was
> criticized because it introduces an inter-dependency between
> drivers.
> 
> This approach modifies the request_muxed_region() semantics and
> modifies the possible use cases.
> 
> The first patch in the series adds a new IORESOURCE_ALLOCATED
> flag that alloc_resource() sets and free_resource() considers.
> The core of __request_region() is factored out into a new function
> that doesn't allocate. With this change, drivers can use the
> pre-existing DEFINE_RES_IO_NAMED() static initialized macro
> to declare struct resource statically (e.g. on the stack)
> and pass the address of it to the new __request_declared_region()
> function. A new macro called request_declared_muxed_region()
> was added to exploit this functionality. Because of the new
> IORESOURCE_ALLOCATED resource flag, release_region() can still
> be called with the old interface (the port region start and
> end values) and it won't attempt to free a non-allocated resource.
> This eliminated one failure case that can come from allocation
> errors.
> 
> The second patch modifies the behaviour of IORESOURCE_MUXED,
> a.k.a. the request_*muxed_region() macros. When these macros
> are called, the caller goes to sleep when there is any conflicting
> regions, even if the conflicting region did not use the
> IORESOURCE_MUXED flag. The kernel logs this inconsistent
> flag usage with KERN_ERR. This change eliminates the second
> failure case for IORESOURCE_MUXED and request_muxed_region()
> can be used like mutex_lock(), i.e. it returns only in case it
> could successfully request the region.
> 
> The last three patches adds proper synchronization between the
> USB PCI quirks code and the i2c-piix and sp5100_tco drivers.
> 
> The result is that the sp5100_tco driver can load and works again:
> 
> sp5100_tco: SP5100/SB800 TCO WatchDog Timer Driver v0.05
> sp5100_tco: PCI Vendor ID: 0x1002, Device ID: 0x4385, Revision ID: 0x42
> sp5100_tco: Using 0xfed80b00 for watchdog MMIO address
> sp5100_tco: Last reboot was not triggered by watchdog.
> sp5100_tco: initialized (0xffffba2f4192db00). heartbeat=60 sec (nowayout=0)
> 
> Signed-off-by: Zoltán Böszörményi <zboszor@pr.hu>
> ---
>   drivers/i2c/busses/i2c-piix4.c | 41 ++++++++++++-------------------------
>   drivers/usb/host/pci-quirks.c  |  4 ++++
>   drivers/watchdog/sp5100_tco.c  | 28 +++++++++++++------------
>   include/linux/ioport.h         | 14 +++++++++++++
>   kernel/resource.c              | 46 ++++++++++++++++++++++++++++++++++++++----
>   5 files changed, 88 insertions(+), 45 deletions(-)
> 
> 

The synchronized access to the SB800 I/O ports seems to also have made a rare
"disabled by hub (EMI?), re-enabling..." report from the kernel disappear.

Can someone review the series?

Thanks in advance,
Zoltán Böszörményi

Re: [PATCH 0/5 v4] Fix sp5100_tco watchdog driver regression

[Marcelo "Marc" Ranolfi]

(Reposting without html subpart, second attempt)

On Thu, Jul 6, 2017 at 11:09 PM, Marcelo "Marc" Ranolfi
<marc.2377@gmail.com> wrote:
Hi Zoltán and all,

I _was_ testing the patch series (the latest version), system worked
OK for two days, but then I got a system crash (probably hardware
related) and lost my files in the main partition. Posted about it in
the F2FS mailing list
(https://sourceforge.net/p/linux-f2fs/mailman/message/35928135/).

While I don't believe that the freeze/crash was caused by the watchdog
driver (many other things to blame: amdgpu driver on Southern Islands,
CPU voltage lower than recommended...), it's worth mentioning that the
computer was completely frozen for a little over 4 minutes, and did
not reset as I'd expect the watchdog to. Just mentioning because I'm
not sure of the implications.

Once I get my system back online this weekend I'll have a good look at
the code. Anyway, thanks again for the patches and I'm also expecting
someone more qualified (than me, at least) to review them.

Regards
Marcelo "Marc" Ranolfi

Re: [2/5,v2] Modify behaviour of request_*muxed_region()

[Guenter Roeck]

On Thu, Jun 22, 2017 at 03:21:31PM +0200, Zoltán Böszörményi wrote:
> In order to make request_*muxed_region() behave more like
> mutex_lock(), a possible failure case needs to be eliminated.
> When drivers do not properly share the same I/O region, e.g.
> one is using request_region() and the other is using
> request_muxed_region(), the kernel didn't warn the user about it.
> This change modifies IORESOURCE_MUXED behaviour so it always
> goes to sleep waiting for the resuorce to be freed and the
> inconsistent resource flag usage is logged with KERN_ERR.
> 
> v2: Fixed checkpatch.pl warnings and extended the comment
>     about request_declared_muxed_region.
> 
> Signed-off-by: Zoltán Böszörményi <zboszor@pr.hu>

It might make sense to go through your series, determine who touched
the files you are changing and who was Cc:'d, and use that to create
a useful list of people to Cc:. You copied a lot of people, but I don't
see anyone who recently touched this file. The same is true for your other
'core' patches. In some cases you did not even copy the maintainer(s).
It might also make sense to add explicit "Cc:" entries, to inform people
that you expect them to provide feedback.

As it is, it will be all but impossible to get your patch series accepted,
simply because the people in position to approve the core changes don't
know about it.

Guenter

> ---
>  kernel/resource.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/resource.c b/kernel/resource.c
> index 2be7029..5df2731 100644
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -1125,6 +1125,7 @@ resource_size_t resource_alignment(struct resource *res)
>   *
>   * request_declared_muxed_region creates a new shared busy region
>   * described in an existing resource descriptor.
> + * It only returns if it succeeded.
>   *
>   * release_region releases a matching busy region.
>   * The region is only freed if it was allocated.
> @@ -1191,7 +1192,10 @@ struct resource *__request_declared_region(struct resource *parent,
>  				continue;
>  			}
>  		}
> -		if (conflict->flags & flags & IORESOURCE_MUXED) {
> +		if (flags & IORESOURCE_MUXED) {
> +			if (!(conflict->flags & IORESOURCE_MUXED))
> +				pr_err("Resource conflict between muxed \"%s\" and non-muxed \"%s\" I/O regions!\n",
> +					res->name, conflict->name);
>  			add_wait_queue(&muxed_resource_wait, &wait);
>  			write_unlock(&resource_lock);
>  			set_current_state(TASK_UNINTERRUPTIBLE);

Re: [PATCH 1/5 v2] Extend the request_region() infrastructure

[Boszormenyi Zoltan]

2017-06-22 15:21 keltezéssel, Zoltán Böszörményi írta:
> Add a new IORESOURCE_ALLOCATED flag that is automatically used
> when alloc_resource() is used internally in kernel/resource.c
> and free_resource() now takes this flag into account.
> 
> The core of __request_region() was factored out into a new function
> called __request_declared_region() that needs struct resource *
> instead of the (start, n, name) triplet.
> 
> These changes allow using statically declared struct resource
> data coupled with the pre-existing DEFINE_RES_IO_NAMED() static
> initializer macro. The new macro exploiting
> __request_declared_region() is request_declared_muxed_region()
> 
> v2:
> 
> Fixed checkpatch.pl warnings and errors and extended the macro
> API with request_declared_region() and release_declared_region()
> 
> Reversed the order of __request_declared_region and __request_region
> 
> Added high level description of the muxed and declared variants
> of the macros.
> 
> Signed-off-by: Zoltán Böszörményi <zboszor@pr.hu>
> ---
>   include/linux/ioport.h | 14 ++++++++++++++
>   kernel/resource.c      | 40 +++++++++++++++++++++++++++++++++++++---
>   2 files changed, 51 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/ioport.h b/include/linux/ioport.h
> index 6230064..6ebcd39 100644
> --- a/include/linux/ioport.h
> +++ b/include/linux/ioport.h
> @@ -52,6 +52,7 @@ struct resource {
>   #define IORESOURCE_MEM_64	0x00100000
>   #define IORESOURCE_WINDOW	0x00200000	/* forwarded by bridge */
>   #define IORESOURCE_MUXED	0x00400000	/* Resource is software muxed */
> +#define IORESOURCE_ALLOCATED	0x00800000	/* Resource was allocated */
>   
>   #define IORESOURCE_EXT_TYPE_BITS 0x01000000	/* Resource extended types */
>   #define IORESOURCE_SYSRAM	0x01000000	/* System RAM (modifier) */
> @@ -215,7 +216,14 @@ static inline bool resource_contains(struct resource *r1, struct resource *r2)
>   
>   /* Convenience shorthand with allocation */
>   #define request_region(start,n,name)		__request_region(&ioport_resource, (start), (n), (name), 0)
> +#define request_declared_region(res)		__request_region( \
> +							&ioport_resource, \
> +							(res), 0)
>   #define request_muxed_region(start,n,name)	__request_region(&ioport_resource, (start), (n), (name), IORESOURCE_MUXED)
> +#define request_declared_muxed_region(res)	__request_declared_region( \
> +							&ioport_resource, \
> +							(res), \
> +							IORESOURCE_MUXED)
>   #define __request_mem_region(start,n,name, excl) __request_region(&iomem_resource, (start), (n), (name), excl)
>   #define request_mem_region(start,n,name) __request_region(&iomem_resource, (start), (n), (name), 0)
>   #define request_mem_region_exclusive(start,n,name) \
> @@ -227,8 +235,14 @@ extern struct resource * __request_region(struct resource *,
>   					resource_size_t n,
>   					const char *name, int flags);
>   
> +extern struct resource *__request_declared_region(struct resource *parent,
> +					struct resource *res, int flags);
> +
>   /* Compatibility cruft */
>   #define release_region(start,n)	__release_region(&ioport_resource, (start), (n))
> +#define release_declared_region(res)	__release_region(&ioport_resource, \
> +						(res)->start, \
> +						(res)->end - (res)->start + 1)
>   #define release_mem_region(start,n)	__release_region(&iomem_resource, (start), (n))
>   
>   extern void __release_region(struct resource *, resource_size_t,
> diff --git a/kernel/resource.c b/kernel/resource.c
> index 9b5f044..2be7029 100644
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -184,6 +184,9 @@ static void free_resource(struct resource *res)
>   	if (!res)
>   		return;
>   
> +	if (!(res->flags & IORESOURCE_ALLOCATED))
> +		return;
> +
>   	if (!PageSlab(virt_to_head_page(res))) {
>   		spin_lock(&bootmem_resource_lock);
>   		res->sibling = bootmem_resource_free;
> @@ -210,6 +213,8 @@ static struct resource *alloc_resource(gfp_t flags)
>   	else
>   		res = kzalloc(sizeof(struct resource), flags);
>   
> +	res->flags = IORESOURCE_ALLOCATED;
> +
>   	return res;
>   }
>   
> @@ -1110,8 +1115,19 @@ resource_size_t resource_alignment(struct resource *res)
>    * the IO flag meanings (busy etc).
>    *
>    * request_region creates a new busy region.
> + * The resource descriptor is allocated by this function.
> + *
> + * request_declared_region creates a new busy region
> + * described in an existing resource descriptor.
> + *
> + * request_muxed_region creates a new shared busy region.
> + * The resource descriptor is allocated by this function.
> + *
> + * request_declared_muxed_region creates a new shared busy region
> + * described in an existing resource descriptor.
>    *
>    * release_region releases a matching busy region.
> + * The region is only freed if it was allocated.
>    */
>   
>   static DECLARE_WAIT_QUEUE_HEAD(muxed_resource_wait);
> @@ -1128,7 +1144,6 @@ struct resource * __request_region(struct resource *parent,
>   				   resource_size_t start, resource_size_t n,
>   				   const char *name, int flags)
>   {
> -	DECLARE_WAITQUEUE(wait, current);
>   	struct resource *res = alloc_resource(GFP_KERNEL);
>   
>   	if (!res)
> @@ -1138,6 +1153,26 @@ struct resource * __request_region(struct resource *parent,
>   	res->start = start;
>   	res->end = start + n - 1;
>   
> +	if (!__request_declared_region(parent, res, flags)) {
> +		free_resource(res);
> +		res = NULL;
> +	}
> +
> +	return res;
> +}
> +EXPORT_SYMBOL(__request_region);
> +
> +/**
> + * __request_declared_region - create a new busy resource region
> + * @parent: parent resource descriptor
> + * @res: child resource descriptor
> + * @flags: IO resource flags
> + */
> +struct resource *__request_declared_region(struct resource *parent,
> +				   struct resource *res, int flags)
> +{
> +	DECLARE_WAITQUEUE(wait, current);
> +
>   	write_lock(&resource_lock);
>   
>   	for (;;) {
> @@ -1166,14 +1201,13 @@ struct resource * __request_region(struct resource *parent,
>   			continue;
>   		}
>   		/* Uhhuh, that didn't work out.. */
> -		free_resource(res);
>   		res = NULL;
>   		break;
>   	}
>   	write_unlock(&resource_lock);
>   	return res;
>   }
> -EXPORT_SYMBOL(__request_region);
> +EXPORT_SYMBOL(__request_declared_region);
>   
>   /**
>    * __release_region - release a previously reserved resource region
> 

Re: [PATCH 3/5 v4] usb: pci-quirks: Protect the I/O port pair of SB800

[Boszormenyi Zoltan]

2017-06-22 15:21 keltezéssel, Zoltán Böszörményi írta:
> This patch uses the previously introduced macro called
> request_declared_muxed_region() to synchronize access to
> the I/O port pair 0xcd6 / 0xcd7 on SB800.
> 
> These I/O ports are also used by i2c-piix4 and sp5100_tco,
> so synchronization is necessary. The other drivers will also
> be modified to use the new macro in subsequest patched.
> 
> v1: Started with a common mutex in a C source file.
> 
> v2: Declared the common mutex in drivers/usb/host/pci-quirks.c
>      instead of in a common C file.
> 
> v3: Switched to using the new request_declared_muxed_region
>      macro.
> 
> v4: Fixed checkpatch.pl warnings and use the new
>      release_declared_region() macro.
> 
> Signed-off-by: Zoltán Böszörményi <zboszor@pr.hu>
> ---
>   drivers/usb/host/pci-quirks.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
> index a9a1e4c..593942a 100644
> --- a/drivers/usb/host/pci-quirks.c
> +++ b/drivers/usb/host/pci-quirks.c
> @@ -279,6 +279,8 @@ bool usb_amd_prefetch_quirk(void)
>   }
>   EXPORT_SYMBOL_GPL(usb_amd_prefetch_quirk);
>   
> +static struct resource sb800_res = DEFINE_RES_IO_NAMED(0xcd6, 2, "SB800 USB");
> +
>   /*
>    * The hardware normally enables the A-link power management feature, which
>    * lets the system lower the power consumption in idle states.
> @@ -314,11 +316,13 @@ static void usb_amd_quirk_pll(int disable)
>   	if (amd_chipset.sb_type.gen == AMD_CHIPSET_SB800 ||
>   			amd_chipset.sb_type.gen == AMD_CHIPSET_HUDSON2 ||
>   			amd_chipset.sb_type.gen == AMD_CHIPSET_BOLTON) {
> +		request_declared_muxed_region(&sb800_res);
>   		outb_p(AB_REG_BAR_LOW, 0xcd6);
>   		addr_low = inb_p(0xcd7);
>   		outb_p(AB_REG_BAR_HIGH, 0xcd6);
>   		addr_high = inb_p(0xcd7);
>   		addr = addr_high << 8 | addr_low;
> +		release_declared_region(&sb800_res);
>   
>   		outl_p(0x30, AB_INDX(addr));
>   		outl_p(0x40, AB_DATA(addr));
> 

Re: [PATCH 5/5 v4] watchdog: sp5100_tco: Use request_declared_muxed_region()

[Boszormenyi Zoltan]

2017-06-22 15:21 keltezéssel, Zoltán Böszörményi írta:
> Use the new request_declared_muxed_region() macro to synchronize
> accesses to the SB800 I/O port pair (0xcd6 / 0xcd7) with the
> PCI quirk for isochronous USB transfers and with the i2c-piix4
> driver.
> 
> At the same time, remove the long lifetime request_region() call
> to reserve these I/O ports, similarly to i2c-piix4 so the code is
> now uniform across the three drivers.
> 
> v1: Started with a common mutex in a C source file.
> 
> v2: Referenced the common mutex from drivers/usb/host/pci-quirks.c
> 
> v3: Switched to using the new request_declared_muxed_region
>      macro.
> 
> v4: Fixed checkpatch.pl warnings and use the new
>      release_declared_region() macro.
> 
> Signed-off-by: Zoltán Böszörményi <zboszor@pr.hu>
> ---
>   drivers/watchdog/sp5100_tco.c | 28 +++++++++++++++-------------
>   1 file changed, 15 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
> index 028618c..cb42b72 100644
> --- a/drivers/watchdog/sp5100_tco.c
> +++ b/drivers/watchdog/sp5100_tco.c
> @@ -48,7 +48,6 @@
>   static u32 tcobase_phys;
>   static u32 tco_wdt_fired;
>   static void __iomem *tcobase;
> -static unsigned int pm_iobase;
>   static DEFINE_SPINLOCK(tco_lock);	/* Guards the hardware */
>   static unsigned long timer_alive;
>   static char tco_expect_close;
> @@ -70,6 +69,11 @@ module_param(nowayout, bool, 0);
>   MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started."
>   		" (default=" __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>   
> +/* synchronized access to the I/O port pair */
> +static struct resource sp5100_res = DEFINE_RES_IO_NAMED(SB800_IO_PM_INDEX_REG,
> +						SP5100_PM_IOPORTS_SIZE,
> +						TCO_MODULE_NAME);
> +
>   /*
>    * Some TCO specific functions
>    */
> @@ -139,6 +143,7 @@ static void tco_timer_enable(void)
>   	if (!tco_has_sp5100_reg_layout(sp5100_tco_pci)) {
>   		/* For SB800 or later */
>   		/* Set the Watchdog timer resolution to 1 sec */
> +		request_declared_muxed_region(&sp5100_res);
>   		outb(SB800_PM_WATCHDOG_CONFIG, SB800_IO_PM_INDEX_REG);
>   		val = inb(SB800_IO_PM_DATA_REG);
>   		val |= SB800_PM_WATCHDOG_SECOND_RES;
> @@ -150,6 +155,7 @@ static void tco_timer_enable(void)
>   		val |= SB800_PCI_WATCHDOG_DECODE_EN;
>   		val &= ~SB800_PM_WATCHDOG_DISABLE;
>   		outb(val, SB800_IO_PM_DATA_REG);
> +		release_declared_region(&sp5100_res);
>   	} else {
>   		/* For SP5100 or SB7x0 */
>   		/* Enable watchdog decode bit */
> @@ -164,11 +170,13 @@ static void tco_timer_enable(void)
>   				       val);
>   
>   		/* Enable Watchdog timer and set the resolution to 1 sec */
> +		request_declared_muxed_region(&sp5100_res);
>   		outb(SP5100_PM_WATCHDOG_CONTROL, SP5100_IO_PM_INDEX_REG);
>   		val = inb(SP5100_IO_PM_DATA_REG);
>   		val |= SP5100_PM_WATCHDOG_SECOND_RES;
>   		val &= ~SP5100_PM_WATCHDOG_DISABLE;
>   		outb(val, SP5100_IO_PM_DATA_REG);
> +		release_declared_region(&sp5100_res);
>   	}
>   }
>   
> @@ -361,16 +369,10 @@ static unsigned char sp5100_tco_setupdevice(void)
>   		base_addr = SB800_PM_WATCHDOG_BASE;
>   	}
>   
> -	/* Request the IO ports used by this driver */
> -	pm_iobase = SP5100_IO_PM_INDEX_REG;
> -	if (!request_region(pm_iobase, SP5100_PM_IOPORTS_SIZE, dev_name)) {
> -		pr_err("I/O address 0x%04x already in use\n", pm_iobase);
> -		goto exit;
> -	}
> -
>   	/*
>   	 * First, Find the watchdog timer MMIO address from indirect I/O.
>   	 */
> +	request_declared_muxed_region(&sp5100_res);
>   	outb(base_addr+3, index_reg);
>   	val = inb(data_reg);
>   	outb(base_addr+2, index_reg);
> @@ -380,6 +382,7 @@ static unsigned char sp5100_tco_setupdevice(void)
>   	outb(base_addr+0, index_reg);
>   	/* Low three bits of BASE are reserved */
>   	val = val << 8 | (inb(data_reg) & 0xf8);
> +	release_declared_region(&sp5100_res);
>   
>   	pr_debug("Got 0x%04x from indirect I/O\n", val);
>   
> @@ -400,6 +403,7 @@ static unsigned char sp5100_tco_setupdevice(void)
>   				      SP5100_SB_RESOURCE_MMIO_BASE, &val);
>   	} else {
>   		/* Read SBResource_MMIO from AcpiMmioEn(PM_Reg: 24h) */
> +		request_declared_muxed_region(&sp5100_res);
>   		outb(SB800_PM_ACPI_MMIO_EN+3, SB800_IO_PM_INDEX_REG);
>   		val = inb(SB800_IO_PM_DATA_REG);
>   		outb(SB800_PM_ACPI_MMIO_EN+2, SB800_IO_PM_INDEX_REG);
> @@ -408,6 +412,7 @@ static unsigned char sp5100_tco_setupdevice(void)
>   		val = val << 8 | inb(SB800_IO_PM_DATA_REG);
>   		outb(SB800_PM_ACPI_MMIO_EN+0, SB800_IO_PM_INDEX_REG);
>   		val = val << 8 | inb(SB800_IO_PM_DATA_REG);
> +		release_declared_region(&sp5100_res);
>   	}
>   
>   	/* The SBResource_MMIO is enabled and mapped memory space? */
> @@ -429,7 +434,7 @@ static unsigned char sp5100_tco_setupdevice(void)
>   		pr_debug("SBResource_MMIO is disabled(0x%04x)\n", val);
>   
>   	pr_notice("failed to find MMIO address, giving up.\n");
> -	goto  unreg_region;
> +	goto  exit;
>   
>   setup_wdt:
>   	tcobase_phys = val;
> @@ -469,8 +474,6 @@ static unsigned char sp5100_tco_setupdevice(void)
>   
>   unreg_mem_region:
>   	release_mem_region(tcobase_phys, SP5100_WDT_MEM_MAP_SIZE);
> -unreg_region:
> -	release_region(pm_iobase, SP5100_PM_IOPORTS_SIZE);
>   exit:
>   	return 0;
>   }
> @@ -517,7 +520,7 @@ static int sp5100_tco_init(struct platform_device *dev)
>   exit:
>   	iounmap(tcobase);
>   	release_mem_region(tcobase_phys, SP5100_WDT_MEM_MAP_SIZE);
> -	release_region(pm_iobase, SP5100_PM_IOPORTS_SIZE);
> +	release_region(SB800_IO_PM_INDEX_REG, SP5100_PM_IOPORTS_SIZE);
>   	return ret;
>   }
>   
> @@ -531,7 +534,6 @@ static void sp5100_tco_cleanup(void)
>   	misc_deregister(&sp5100_tco_miscdev);
>   	iounmap(tcobase);
>   	release_mem_region(tcobase_phys, SP5100_WDT_MEM_MAP_SIZE);
> -	release_region(pm_iobase, SP5100_PM_IOPORTS_SIZE);
>   }
>   
>   static int sp5100_tco_remove(struct platform_device *dev)
> 

Re: [PATCH 3/5 v4] usb: pci-quirks: Protect the I/O port pair of SB800

[Greg Kroah-Hartman]

On Fri, Jul 14, 2017 at 10:34:20AM +0200, Boszormenyi Zoltan wrote:
> 2017-06-22 15:21 keltezéssel, Zoltán Böszörményi írta:
> > This patch uses the previously introduced macro called
> > request_declared_muxed_region() to synchronize access to
> > the I/O port pair 0xcd6 / 0xcd7 on SB800.
> > 
> > These I/O ports are also used by i2c-piix4 and sp5100_tco,
> > so synchronization is necessary. The other drivers will also
> > be modified to use the new macro in subsequest patched.
> > 
> > v1: Started with a common mutex in a C source file.
> > 
> > v2: Declared the common mutex in drivers/usb/host/pci-quirks.c
> >      instead of in a common C file.
> > 
> > v3: Switched to using the new request_declared_muxed_region
> >      macro.
> > 
> > v4: Fixed checkpatch.pl warnings and use the new
> >      release_declared_region() macro.
> > 
> > Signed-off-by: Zoltán Böszörményi <zboszor@pr.hu>

Why are you responding to your own patches?

You do know this is the middle of the merge window, and we can't do
anything here with patches, right?

thanks,

greg k-h