* [PATCH 1/5] Extend the request_region() infrastructure
2017-06-21 3:53 ` [PATCH 0/5 v3] Fix sp5100_tco watchdog driver regression Zoltán Böszörményi
@ 2017-06-21 3:53 ` Zoltán Böszörményi
2017-06-21 3:53 ` [PATCH 2/5] Modify behaviour of request_*muxed_region() Zoltán Böszörményi
` (4 subsequent siblings)
5 siblings, 0 replies; 37+ messages in thread
From: Zoltán Böszörményi @ 2017-06-21 3:53 UTC (permalink / raw)
To: linux-kernel
Cc: linux-usb, linux-watchdog, linux-i2c, Paul Menzel,
Christian Fetzer, Jean Delvare, Nehal Shah, Tim Small,
Guenter Roeck, kernel, wim, jlayton, marc.2377, cshorler, wsa,
regressions, 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
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH 2/5] Modify behaviour of request_*muxed_region()
2017-06-21 3:53 ` [PATCH 0/5 v3] Fix sp5100_tco watchdog driver regression Zoltán Böszörményi
2017-06-21 3:53 ` [PATCH 1/5] Extend the request_region() infrastructure Zoltán Böszörményi
@ 2017-06-21 3:53 ` Zoltán Böszörményi
2017-06-21 3:53 ` [PATCH 3/5] usb: pci-quirks: Protect the I/O port pair of SB800 Zoltán Böszörményi
` (3 subsequent siblings)
5 siblings, 0 replies; 37+ messages in thread
From: Zoltán Böszörményi @ 2017-06-21 3:53 UTC (permalink / raw)
To: linux-kernel
Cc: linux-usb, linux-watchdog, linux-i2c, Paul Menzel,
Christian Fetzer, Jean Delvare, Nehal Shah, Tim Small,
Guenter Roeck, kernel, wim, jlayton, marc.2377, cshorler, wsa,
regressions, 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
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH 3/5] usb: pci-quirks: Protect the I/O port pair of SB800
2017-06-21 3:53 ` [PATCH 0/5 v3] Fix sp5100_tco watchdog driver regression Zoltán Böszörményi
2017-06-21 3:53 ` [PATCH 1/5] Extend the request_region() infrastructure Zoltán Böszörményi
2017-06-21 3:53 ` [PATCH 2/5] Modify behaviour of request_*muxed_region() Zoltán Böszörményi
@ 2017-06-21 3:53 ` Zoltán Böszörményi
2017-06-21 3:53 ` [PATCH 4/5] i2c: i2c-piix4: Use request_declared_muxed_region() Zoltán Böszörményi
` (2 subsequent siblings)
5 siblings, 0 replies; 37+ messages in thread
From: Zoltán Böszörményi @ 2017-06-21 3:53 UTC (permalink / raw)
To: linux-kernel
Cc: linux-usb, linux-watchdog, linux-i2c, Paul Menzel,
Christian Fetzer, Jean Delvare, Nehal Shah, Tim Small,
Guenter Roeck, kernel, wim, jlayton, marc.2377, cshorler, wsa,
regressions, 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
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH 4/5] i2c: i2c-piix4: Use request_declared_muxed_region()
2017-06-21 3:53 ` [PATCH 0/5 v3] Fix sp5100_tco watchdog driver regression Zoltán Böszörményi
` (2 preceding siblings ...)
2017-06-21 3:53 ` [PATCH 3/5] usb: pci-quirks: Protect the I/O port pair of SB800 Zoltán Böszörményi
@ 2017-06-21 3:53 ` Zoltán Böszörményi
2017-06-21 3:53 ` [PATCH 5/5] watchdog: sp5100_tco: " Zoltán Böszörményi
2017-06-22 13:21 ` [PATCH 0/5 v4] Fix sp5100_tco watchdog driver regression Zoltán Böszörményi
5 siblings, 0 replies; 37+ messages in thread
From: Zoltán Böszörményi @ 2017-06-21 3:53 UTC (permalink / raw)
To: linux-kernel
Cc: linux-usb, linux-watchdog, linux-i2c, Paul Menzel,
Christian Fetzer, Jean Delvare, Nehal Shah, Tim Small,
Guenter Roeck, kernel, wim, jlayton, marc.2377, cshorler, wsa,
regressions, 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
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH 5/5] watchdog: sp5100_tco: Use request_declared_muxed_region()
2017-06-21 3:53 ` [PATCH 0/5 v3] Fix sp5100_tco watchdog driver regression Zoltán Böszörményi
` (3 preceding siblings ...)
2017-06-21 3:53 ` [PATCH 4/5] i2c: i2c-piix4: Use request_declared_muxed_region() Zoltán Böszörményi
@ 2017-06-21 3:53 ` Zoltán Böszörményi
2017-06-21 15:09 ` Guenter Roeck
2017-06-22 13:21 ` [PATCH 0/5 v4] Fix sp5100_tco watchdog driver regression Zoltán Böszörményi
5 siblings, 1 reply; 37+ messages in thread
From: Zoltán Böszörményi @ 2017-06-21 3:53 UTC (permalink / raw)
To: linux-kernel
Cc: linux-usb, linux-watchdog, linux-i2c, Paul Menzel,
Christian Fetzer, Jean Delvare, Nehal Shah, Tim Small,
Guenter Roeck, kernel, wim, jlayton, marc.2377, cshorler, wsa,
regressions, 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
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH 5/5] watchdog: sp5100_tco: Use request_declared_muxed_region()
2017-06-21 3:53 ` [PATCH 5/5] watchdog: sp5100_tco: " Zoltán Böszörményi
@ 2017-06-21 15:09 ` Guenter Roeck
0 siblings, 0 replies; 37+ messages in thread
From: Guenter Roeck @ 2017-06-21 15:09 UTC (permalink / raw)
To: Zoltán Böszörményi
Cc: linux-kernel, linux-usb, linux-watchdog, linux-i2c, Paul Menzel,
Christian Fetzer, Jean Delvare, Nehal Shah, Tim Small, kernel,
wim, jlayton, marc.2377, cshorler, wsa, regressions
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
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 0/5 v4] Fix sp5100_tco watchdog driver regression
2017-06-21 3:53 ` [PATCH 0/5 v3] Fix sp5100_tco watchdog driver regression Zoltán Böszörményi
` (4 preceding siblings ...)
2017-06-21 3:53 ` [PATCH 5/5] watchdog: sp5100_tco: " Zoltán Böszörményi
@ 2017-06-22 13:21 ` Zoltán Böszörményi
2017-06-22 13:21 ` [PATCH 1/5 v2] Extend the request_region() infrastructure Zoltán Böszörményi
` (5 more replies)
5 siblings, 6 replies; 37+ messages in thread
From: Zoltán Böszörményi @ 2017-06-22 13:21 UTC (permalink / raw)
To: linux-kernel
Cc: linux-usb, linux-watchdog, linux-i2c, Paul Menzel,
Christian Fetzer, Jean Delvare, Nehal Shah, Tim Small,
Guenter Roeck, kernel, wim, jlayton, marc.2377, cshorler, wsa,
regressions, 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(-)
^ permalink raw reply [flat|nested] 37+ messages in thread* [PATCH 1/5 v2] Extend the request_region() infrastructure
2017-06-22 13:21 ` [PATCH 0/5 v4] Fix sp5100_tco watchdog driver regression Zoltán Böszörményi
@ 2017-06-22 13:21 ` Zoltán Böszörményi
2017-07-14 8:33 ` Boszormenyi Zoltan
2017-06-22 13:21 ` [PATCH 2/5 v2] Modify behaviour of request_*muxed_region() Zoltán Böszörményi
` (4 subsequent siblings)
5 siblings, 1 reply; 37+ messages in thread
From: Zoltán Böszörményi @ 2017-06-22 13:21 UTC (permalink / raw)
To: linux-kernel
Cc: linux-usb, linux-watchdog, linux-i2c, Paul Menzel,
Christian Fetzer, Jean Delvare, Nehal Shah, Tim Small,
Guenter Roeck, kernel, wim, jlayton, marc.2377, cshorler, wsa,
regressions, 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
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH 1/5 v2] Extend the request_region() infrastructure
2017-06-22 13:21 ` [PATCH 1/5 v2] Extend the request_region() infrastructure Zoltán Böszörményi
@ 2017-07-14 8:33 ` Boszormenyi Zoltan
0 siblings, 0 replies; 37+ messages in thread
From: Boszormenyi Zoltan @ 2017-07-14 8:33 UTC (permalink / raw)
To: linux-kernel
Cc: linux-usb, linux-watchdog, linux-i2c, Paul Menzel,
Christian Fetzer, Jean Delvare, Nehal Shah, Tim Small,
Guenter Roeck, kernel, wim, jlayton, marc.2377, cshorler, wsa,
regressions, Alex Williamson, Linus Torvalds, Bjorn Helgaas,
Toshi Kani, Jiang Liu, Jakub Sitnicki, Thierry Reding,
Vivek Goyal, Ingo Molnar, Simon Guinot, Dan Williams,
Mike Travis, Daeseok Youn, Huang Rui, Andiry Xu,
Greg Kroah-Hartman, Alan Cox, David Howells,
Ricardo Ribalda Delgado, Alexandre Desnoyers, Andy Shevchenko,
Grygorii Strashko, Mika Westerberg, Wan ZongShun, Ulf Hansson,
Lucas Stach, Denis Turischev
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
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 2/5 v2] Modify behaviour of request_*muxed_region()
2017-06-22 13:21 ` [PATCH 0/5 v4] Fix sp5100_tco watchdog driver regression Zoltán Böszörményi
2017-06-22 13:21 ` [PATCH 1/5 v2] Extend the request_region() infrastructure Zoltán Böszörményi
@ 2017-06-22 13:21 ` Zoltán Böszörményi
2017-07-08 15:37 ` [2/5,v2] " Guenter Roeck
2017-06-22 13:21 ` [PATCH 3/5 v4] usb: pci-quirks: Protect the I/O port pair of SB800 Zoltán Böszörményi
` (3 subsequent siblings)
5 siblings, 1 reply; 37+ messages in thread
From: Zoltán Böszörményi @ 2017-06-22 13:21 UTC (permalink / raw)
To: linux-kernel
Cc: linux-usb, linux-watchdog, linux-i2c, Paul Menzel,
Christian Fetzer, Jean Delvare, Nehal Shah, Tim Small,
Guenter Roeck, kernel, wim, jlayton, marc.2377, cshorler, wsa,
regressions, 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
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [2/5,v2] Modify behaviour of request_*muxed_region()
2017-06-22 13:21 ` [PATCH 2/5 v2] Modify behaviour of request_*muxed_region() Zoltán Böszörményi
@ 2017-07-08 15:37 ` Guenter Roeck
0 siblings, 0 replies; 37+ messages in thread
From: Guenter Roeck @ 2017-07-08 15:37 UTC (permalink / raw)
To: Zoltán Böszörményi
Cc: linux-kernel, linux-usb, linux-watchdog, linux-i2c, Paul Menzel,
Christian Fetzer, Jean Delvare, Nehal Shah, Tim Small, kernel,
wim, jlayton, marc.2377, cshorler, wsa, regressions
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);
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 3/5 v4] usb: pci-quirks: Protect the I/O port pair of SB800
2017-06-22 13:21 ` [PATCH 0/5 v4] Fix sp5100_tco watchdog driver regression Zoltán Böszörményi
2017-06-22 13:21 ` [PATCH 1/5 v2] Extend the request_region() infrastructure Zoltán Böszörményi
2017-06-22 13:21 ` [PATCH 2/5 v2] Modify behaviour of request_*muxed_region() Zoltán Böszörményi
@ 2017-06-22 13:21 ` Zoltán Böszörményi
2017-07-14 8:34 ` Boszormenyi Zoltan
2017-06-22 13:21 ` [PATCH 4/5 v4] i2c: i2c-piix4: Use request_declared_muxed_region() Zoltán Böszörményi
` (2 subsequent siblings)
5 siblings, 1 reply; 37+ messages in thread
From: Zoltán Böszörményi @ 2017-06-22 13:21 UTC (permalink / raw)
To: linux-kernel
Cc: linux-usb, linux-watchdog, linux-i2c, Paul Menzel,
Christian Fetzer, Jean Delvare, Nehal Shah, Tim Small,
Guenter Roeck, kernel, wim, jlayton, marc.2377, cshorler, wsa,
regressions, 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
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH 3/5 v4] usb: pci-quirks: Protect the I/O port pair of SB800
2017-06-22 13:21 ` [PATCH 3/5 v4] usb: pci-quirks: Protect the I/O port pair of SB800 Zoltán Böszörményi
@ 2017-07-14 8:34 ` Boszormenyi Zoltan
2017-07-14 11:36 ` Greg Kroah-Hartman
0 siblings, 1 reply; 37+ messages in thread
From: Boszormenyi Zoltan @ 2017-07-14 8:34 UTC (permalink / raw)
To: linux-kernel
Cc: linux-usb, linux-watchdog, linux-i2c, Paul Menzel,
Christian Fetzer, Jean Delvare, Nehal Shah, Tim Small,
Guenter Roeck, kernel, wim, jlayton, marc.2377, cshorler, wsa,
regressions, Alex Williamson, Linus Torvalds, Bjorn Helgaas,
Toshi Kani, Jiang Liu, Jakub Sitnicki, Thierry Reding,
Vivek Goyal, Ingo Molnar, Simon Guinot, Dan Williams,
Mike Travis, Daeseok Youn, Huang Rui, Andiry Xu,
Greg Kroah-Hartman, Alan Cox, David Howells,
Ricardo Ribalda Delgado, Alexandre Desnoyers, Andy Shevchenko,
Grygorii Strashko, Mika Westerberg, Wan ZongShun, Ulf Hansson,
Lucas Stach, Denis Turischev
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));
>
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 3/5 v4] usb: pci-quirks: Protect the I/O port pair of SB800
2017-07-14 8:34 ` Boszormenyi Zoltan
@ 2017-07-14 11:36 ` Greg Kroah-Hartman
0 siblings, 0 replies; 37+ messages in thread
From: Greg Kroah-Hartman @ 2017-07-14 11:36 UTC (permalink / raw)
To: Boszormenyi Zoltan
Cc: linux-kernel, linux-usb, linux-watchdog, linux-i2c, Paul Menzel,
Christian Fetzer, Jean Delvare, Nehal Shah, Tim Small,
Guenter Roeck, kernel, wim, jlayton, marc.2377, cshorler, wsa,
regressions, Alex Williamson, Linus Torvalds, Bjorn Helgaas,
Toshi Kani, Jiang Liu, Jakub Sitnicki, Thierry Reding,
Vivek Goyal, Ingo Molnar, Simon Guinot, Dan Williams,
Mike Travis, Daeseok Youn, Huang Rui, Andiry Xu, Alan Cox,
David Howells, Ricardo Ribalda Delgado, Alexandre Desnoyers,
Andy Shevchenko, Grygorii Strashko, Mika Westerberg,
Wan ZongShun, Ulf Hansson, Lucas Stach, Denis Turischev
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
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 4/5 v4] i2c: i2c-piix4: Use request_declared_muxed_region()
2017-06-22 13:21 ` [PATCH 0/5 v4] Fix sp5100_tco watchdog driver regression Zoltán Böszörményi
` (2 preceding siblings ...)
2017-06-22 13:21 ` [PATCH 3/5 v4] usb: pci-quirks: Protect the I/O port pair of SB800 Zoltán Böszörményi
@ 2017-06-22 13:21 ` Zoltán Böszörményi
2017-06-22 13:21 ` [PATCH 5/5 v4] watchdog: sp5100_tco: " Zoltán Böszörményi
2017-07-06 7:50 ` [PATCH 0/5 v4] Fix sp5100_tco watchdog driver regression Boszormenyi Zoltan
5 siblings, 0 replies; 37+ messages in thread
From: Zoltán Böszörményi @ 2017-06-22 13:21 UTC (permalink / raw)
To: linux-kernel
Cc: linux-usb, linux-watchdog, linux-i2c, Paul Menzel,
Christian Fetzer, Jean Delvare, Nehal Shah, Tim Small,
Guenter Roeck, kernel, wim, jlayton, marc.2377, cshorler, wsa,
regressions, 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
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH 5/5 v4] watchdog: sp5100_tco: Use request_declared_muxed_region()
2017-06-22 13:21 ` [PATCH 0/5 v4] Fix sp5100_tco watchdog driver regression Zoltán Böszörményi
` (3 preceding siblings ...)
2017-06-22 13:21 ` [PATCH 4/5 v4] i2c: i2c-piix4: Use request_declared_muxed_region() Zoltán Böszörményi
@ 2017-06-22 13:21 ` Zoltán Böszörményi
2017-07-14 8:34 ` Boszormenyi Zoltan
2017-07-06 7:50 ` [PATCH 0/5 v4] Fix sp5100_tco watchdog driver regression Boszormenyi Zoltan
5 siblings, 1 reply; 37+ messages in thread
From: Zoltán Böszörményi @ 2017-06-22 13:21 UTC (permalink / raw)
To: linux-kernel
Cc: linux-usb, linux-watchdog, linux-i2c, Paul Menzel,
Christian Fetzer, Jean Delvare, Nehal Shah, Tim Small,
Guenter Roeck, kernel, wim, jlayton, marc.2377, cshorler, wsa,
regressions, 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
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH 5/5 v4] watchdog: sp5100_tco: Use request_declared_muxed_region()
2017-06-22 13:21 ` [PATCH 5/5 v4] watchdog: sp5100_tco: " Zoltán Böszörményi
@ 2017-07-14 8:34 ` Boszormenyi Zoltan
0 siblings, 0 replies; 37+ messages in thread
From: Boszormenyi Zoltan @ 2017-07-14 8:34 UTC (permalink / raw)
To: linux-kernel
Cc: linux-usb, linux-watchdog, linux-i2c, Paul Menzel,
Christian Fetzer, Jean Delvare, Nehal Shah, Tim Small,
Guenter Roeck, kernel, wim, jlayton, marc.2377, cshorler, wsa,
regressions, Alex Williamson, Linus Torvalds, Bjorn Helgaas,
Toshi Kani, Jiang Liu, Jakub Sitnicki, Thierry Reding,
Vivek Goyal, Ingo Molnar, Simon Guinot, Dan Williams,
Mike Travis, Daeseok Youn, Huang Rui, Andiry Xu,
Greg Kroah-Hartman, Alan Cox, David Howells,
Ricardo Ribalda Delgado, Alexandre Desnoyers, Andy Shevchenko,
Grygorii Strashko, Mika Westerberg, Wan ZongShun, Ulf Hansson,
Lucas Stach, Denis Turischev
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)
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 0/5 v4] Fix sp5100_tco watchdog driver regression
2017-06-22 13:21 ` [PATCH 0/5 v4] Fix sp5100_tco watchdog driver regression Zoltán Böszörményi
` (4 preceding siblings ...)
2017-06-22 13:21 ` [PATCH 5/5 v4] watchdog: sp5100_tco: " Zoltán Böszörményi
@ 2017-07-06 7:50 ` Boszormenyi Zoltan
[not found] ` <CAETC-g-0ZYiD=j_sAtYpfr0t-rciiTx3ruvpGuwEud+nbbpiBg@mail.gmail.com>
5 siblings, 1 reply; 37+ messages in thread
From: Boszormenyi Zoltan @ 2017-07-06 7:50 UTC (permalink / raw)
To: linux-kernel
Cc: linux-usb, linux-watchdog, linux-i2c, Paul Menzel,
Christian Fetzer, Jean Delvare, Nehal Shah, Tim Small,
Guenter Roeck, kernel, wim, jlayton, marc.2377, cshorler, wsa,
regressions, Greg Kroah-Hartman
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
^ permalink raw reply [flat|nested] 37+ messages in thread