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

Groups > linux.kernel > #1614431 > unrolled thread

[PATCH 0/3] Fix sp5100_tco watchdog driver regression

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/3] Fix sp5100_tco watchdog driver regression Zoltán Böszörményi          <zboszor@pr.hu> - 2017-04-01 12:10 +0200
    [PATCH 1/3] usb: pci-quirks: Add a header for SB800 I/O ports and mutex for locking Zoltán Böszörményi          <zboszor@pr.hu> - 2017-04-01 12:10 +0200
    [PATCH 3/3] watchdog: sp5100_tco: Synchronize I/O port accesses Zoltan Boszormenyi <zboszor@pr.hu> - 2017-04-01 13:10 +0200
    [PATCH 1/3] usb: pci-quirks: Add a header for SB800 I/O ports and mutex for locking Zoltan Boszormenyi <zboszor@pr.hu> - 2017-04-01 13:10 +0200
      Re: [PATCH 1/3] usb: pci-quirks: Add a header for SB800 I/O ports  and mutex for locking Greg KH <greg@kroah.com> - 2017-04-01 16:20 +0200
        Re: [PATCH 1/3] usb: pci-quirks: Add a header for SB800 I/O ports  and mutex for locking Alan Stern <stern@rowland.harvard.edu> - 2017-04-01 16:50 +0200
          Re: [PATCH 1/3] usb: pci-quirks: Add a header for SB800 I/O ports and  mutex for locking Boszormenyi Zoltan <zboszor@pr.hu> - 2017-04-01 17:20 +0200
        Re: [PATCH 1/3] usb: pci-quirks: Add a header for SB800 I/O ports and  mutex for locking Boszormenyi Zoltan <zboszor@pr.hu> - 2017-04-01 17:10 +0200
    [PATCH 0/3, resend] Fix sp5100_tco watchdog driver regression Zoltan Boszormenyi <zboszor@pr.hu> - 2017-04-01 13:10 +0200
    [PATCH 2/3] i2c: i2c-piix4: Synchronize I/O port accesses with the SB800 USB quirk Zoltan Boszormenyi <zboszor@pr.hu> - 2017-04-01 13:10 +0200

#1614431 — [PATCH 0/3] Fix sp5100_tco watchdog driver regression

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

[toc] | [next] | [standalone]

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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
>

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

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

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

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

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web