Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1671322 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 0/5 v3] Fix sp5100_tco watchdog driver regression Zoltán Böszörményi          <zboszor@pr.hu> - 2017-06-21 06:10 +0200
    [PATCH 3/5] usb: pci-quirks: Protect the I/O port pair of SB800 Zoltán Böszörményi          <zboszor@pr.hu> - 2017-06-21 06:10 +0200
    [PATCH 2/5] Modify behaviour of request_*muxed_region() Zoltán Böszörményi          <zboszor@pr.hu> - 2017-06-21 06:10 +0200
    [PATCH 5/5] watchdog: sp5100_tco: Use request_declared_muxed_region() Zoltán Böszörményi          <zboszor@pr.hu> - 2017-06-21 06:10 +0200
      Re: [PATCH 5/5] watchdog: sp5100_tco: Use  request_declared_muxed_region() Guenter Roeck <linux@roeck-us.net> - 2017-06-21 17:10 +0200
    [PATCH 1/5] Extend the request_region() infrastructure Zoltán Böszörményi          <zboszor@pr.hu> - 2017-06-21 06:10 +0200
    [PATCH 4/5] i2c: i2c-piix4: Use request_declared_muxed_region() Zoltán Böszörményi          <zboszor@pr.hu> - 2017-06-21 06:10 +0200
    [PATCH 1/5 v2] Extend the request_region() infrastructure Zoltán Böszörményi          <zboszor@pr.hu> - 2017-06-22 15:30 +0200
    [PATCH 0/5 v4] Fix sp5100_tco watchdog driver regression Zoltán Böszörményi          <zboszor@pr.hu> - 2017-06-22 15:30 +0200
      [PATCH 2/5 v2] Modify behaviour of request_*muxed_region() Zoltán Böszörményi          <zboszor@pr.hu> - 2017-06-22 15:30 +0200
      [PATCH 3/5 v4] usb: pci-quirks: Protect the I/O port pair of SB800 Zoltán Böszörményi          <zboszor@pr.hu> - 2017-06-22 15:30 +0200
      [PATCH 4/5 v4] i2c: i2c-piix4: Use request_declared_muxed_region() Zoltán Böszörményi          <zboszor@pr.hu> - 2017-06-22 15:30 +0200
      [PATCH 5/5 v4] watchdog: sp5100_tco: Use request_declared_muxed_region() Zoltán Böszörményi          <zboszor@pr.hu> - 2017-06-22 15:30 +0200

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

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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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
> 

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web