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

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

[Zoltan Boszormenyi]

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

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.

v3:

Rebased for 4.15-rc4

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 93b4183cf53d..d198d6a58574 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -53,6 +53,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) */
@@ -218,7 +219,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) \
@@ -230,8 +238,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 54ba6de3757c..05db9c4e3992 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;
 }
 
@@ -1128,8 +1133,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);
@@ -1146,7 +1162,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)
@@ -1156,6 +1171,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 (;;) {
@@ -1184,14 +1219,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.13.6

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

[Zoltan Boszormenyi]

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

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.

v3: Rebased for 4.15-rc4

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 05db9c4e3992..0f26f887ac33 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -1143,6 +1143,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.
@@ -1209,7 +1210,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.13.6

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

[Zoltan Boszormenyi]

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

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.

v5: Rebased for 4.15-rc4

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 161536717025..e5b28464006e 100644
--- a/drivers/usb/host/pci-quirks.c
+++ b/drivers/usb/host/pci-quirks.c
@@ -298,6 +298,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.
@@ -333,11 +335,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.13.6

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

[Zoltan Boszormenyi]

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

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.

v5: Rebased for 4.15-rc4
    Use struct i2c_algorithm pointer directly instead of
    a bool to select the proper algo.

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

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 462948e2c535..1846d9ea7d27 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -153,10 +153,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 u8 piix4_port_mask_sb800;
 static u8 piix4_port_shift_sb800;
@@ -169,7 +170,6 @@ struct i2c_piix4_adapdata {
 	unsigned short smba;
 
 	/* SB800 */
-	bool sb800_main;
 	bool notify_imc;
 	u8 port;		/* Port number, shifted */
 };
@@ -298,12 +298,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;
@@ -373,7 +373,7 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
 			break;
 		}
 	} 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) ?
@@ -381,7 +381,7 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
 				       SB800_PIIX4_PORT_IDX;
 		piix4_port_mask_sb800 = SB800_PIIX4_PORT_IDX_MASK;
 		piix4_port_shift_sb800 = SB800_PIIX4_PORT_IDX_SHIFT;
-		mutex_unlock(&piix4_mutex_sb800);
+		release_declared_region(&sb800_res);
 	}
 
 	dev_info(&PIIX4_dev->dev,
@@ -679,7 +679,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);
@@ -695,7 +695,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;
 	}
 
@@ -753,7 +753,7 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
 	if ((size == I2C_SMBUS_BLOCK_DATA) && adapdata->notify_imc)
 		piix4_imc_wakeup();
 
-	mutex_unlock(&piix4_mutex_sb800);
+	release_declared_region(&sb800_res);
 
 	return retval;
 }
@@ -804,7 +804,8 @@ static struct i2c_adapter *piix4_main_adapters[PIIX4_MAX_ADAPTERS];
 static struct i2c_adapter *piix4_aux_adapter;
 
 static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
-			     bool sb800_main, u8 port, bool notify_imc,
+			     struct i2c_algorithm *algo,
+			     u8 port, bool notify_imc,
 			     const char *name, struct i2c_adapter **padap)
 {
 	struct i2c_adapter *adap;
@@ -819,8 +820,7 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
 
 	adap->owner = THIS_MODULE;
 	adap->class = I2C_CLASS_HWMON | I2C_CLASS_SPD;
-	adap->algo = sb800_main ? &piix4_smbus_algorithm_sb800
-				: &smbus_algorithm;
+	adap->algo = algo;
 
 	adapdata = kzalloc(sizeof(*adapdata), GFP_KERNEL);
 	if (adapdata == NULL) {
@@ -830,7 +830,6 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
 	}
 
 	adapdata->smba = smba;
-	adapdata->sb800_main = sb800_main;
 	adapdata->port = port << piix4_port_shift_sb800;
 	adapdata->notify_imc = notify_imc;
 
@@ -862,7 +861,9 @@ static int piix4_add_adapters_sb800(struct pci_dev *dev, unsigned short smba,
 	int retval;
 
 	for (port = 0; port < PIIX4_MAX_ADAPTERS; port++) {
-		retval = piix4_add_adapter(dev, smba, true, port, notify_imc,
+		retval = piix4_add_adapter(dev, smba,
+					   &piix4_smbus_algorithm_sb800,
+					   port, notify_imc,
 					   piix4_main_port_names_sb800[port],
 					   &piix4_main_adapters[port]);
 		if (retval < 0)
@@ -899,13 +900,6 @@ static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
 		bool notify_imc = false;
 		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;
-		}
-
 		if (dev->vendor == PCI_VENDOR_ID_AMD &&
 		    dev->device == PCI_DEVICE_ID_AMD_KERNCZ_SMBUS) {
 			u8 imc;
@@ -922,27 +916,24 @@ static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
 
 		/* 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, notify_imc);
-		if (retval < 0) {
-			release_region(SB800_PIIX4_SMB_IDX, 2);
+		if (retval < 0)
 			return retval;
-		}
 	} else {
 		retval = piix4_setup(dev, id);
 		if (retval < 0)
 			return retval;
 
 		/* Try to register main SMBus adapter, give up if we can't */
-		retval = piix4_add_adapter(dev, retval, false, 0, false, "",
+		retval = piix4_add_adapter(dev, retval,
+					   &smbus_algorithm, 0, false, "",
 					   &piix4_main_adapters[0]);
 		if (retval < 0)
 			return retval;
@@ -983,11 +974,8 @@ static void piix4_adap_remove(struct i2c_adapter *adap)
 
 	if (adapdata->smba) {
 		i2c_del_adapter(adap);
-		if (adapdata->port == (0 << piix4_port_shift_sb800)) {
+		if (adapdata->port == (0 << piix4_port_shift_sb800))
 			release_region(adapdata->smba, SMBIOSIZE);
-			if (adapdata->sb800_main)
-				release_region(SB800_PIIX4_SMB_IDX, 2);
-		}
 		kfree(adapdata);
 		kfree(adap);
 	}
-- 
2.13.6

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

[Zoltan Boszormenyi]

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

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.

v5: Rebased for 4.15-rc4
    Removed the last unnecessary release_region() call

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

diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
index 028618c5eeba..ac17191a72bd 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,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 +533,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.13.6

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

[Guenter Roeck]

On Mon, Dec 18, 2017 at 09:48:37AM +0100, Zoltan Boszormenyi wrote:
> From: Böszörményi Zoltán <zboszor-v1d7l9VOqKc@public.gmane.org>
> 
> 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.
> 
> v3:
> 
> Rebased for 4.15-rc4
> 
> Signed-off-by: Zoltán Böszörményi <zboszor-v1d7l9VOqKc@public.gmane.org>
> ---
>  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 93b4183cf53d..d198d6a58574 100644
> --- a/include/linux/ioport.h
> +++ b/include/linux/ioport.h
> @@ -53,6 +53,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) */
> @@ -218,7 +219,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) \
> @@ -230,8 +238,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 54ba6de3757c..05db9c4e3992 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;
> +

kzalloc() can fail, thus res can be NULL.

>  	return res;
>  }
>  
> @@ -1128,8 +1133,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);
> @@ -1146,7 +1162,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)
> @@ -1156,6 +1171,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 (;;) {
> @@ -1184,14 +1219,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.13.6
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Guenter Roeck]

On Mon, Dec 18, 2017 at 09:48:37AM +0100, Zoltan Boszormenyi wrote:
> From: Böszörményi Zoltán <zboszor@pr.hu>
> 
> 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.
> 
> v3:
> 
> Rebased for 4.15-rc4
> 
> 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 93b4183cf53d..d198d6a58574 100644
> --- a/include/linux/ioport.h
> +++ b/include/linux/ioport.h
> @@ -53,6 +53,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) */
> @@ -218,7 +219,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) \
> @@ -230,8 +238,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 54ba6de3757c..05db9c4e3992 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;
> +

kzalloc() can fail, thus res can be NULL.

>  	return res;
>  }
>  
> @@ -1128,8 +1133,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);
> @@ -1146,7 +1162,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)
> @@ -1156,6 +1171,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 (;;) {
> @@ -1184,14 +1219,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.13.6
> 

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

[Guenter Roeck]

On Mon, Dec 18, 2017 at 09:48:38AM +0100, Zoltan Boszormenyi wrote:
> From: Böszörményi Zoltán <zboszor-v1d7l9VOqKc@public.gmane.org>
> 
> 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

resource

> inconsistent resource flag usage is logged with KERN_ERR.
> 
> v2: Fixed checkpatch.pl warnings and extended the comment
>     about request_declared_muxed_region.
> 
> v3: Rebased for 4.15-rc4
> 
> Signed-off-by: Zoltán Böszörményi <zboszor-v1d7l9VOqKc@public.gmane.org>
> ---
>  kernel/resource.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/resource.c b/kernel/resource.c
> index 05db9c4e3992..0f26f887ac33 100644
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -1143,6 +1143,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.
> @@ -1209,7 +1210,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);

With this, the muxed resource requestor will hang since the non-muxed
requestor will not release the resource. I understand that you are trying
to get rid of the error return, but is replacing it with a hang really
better ?

>  			add_wait_queue(&muxed_resource_wait, &wait);
>  			write_unlock(&resource_lock);
>  			set_current_state(TASK_UNINTERRUPTIBLE);
> -- 
> 2.13.6
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Guenter Roeck]

On Mon, Dec 18, 2017 at 09:48:38AM +0100, Zoltan Boszormenyi wrote:
> From: Böszörményi Zoltán <zboszor@pr.hu>
> 
> 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

resource

> inconsistent resource flag usage is logged with KERN_ERR.
> 
> v2: Fixed checkpatch.pl warnings and extended the comment
>     about request_declared_muxed_region.
> 
> v3: Rebased for 4.15-rc4
> 
> 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 05db9c4e3992..0f26f887ac33 100644
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -1143,6 +1143,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.
> @@ -1209,7 +1210,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);

With this, the muxed resource requestor will hang since the non-muxed
requestor will not release the resource. I understand that you are trying
to get rid of the error return, but is replacing it with a hang really
better ?

>  			add_wait_queue(&muxed_resource_wait, &wait);
>  			write_unlock(&resource_lock);
>  			set_current_state(TASK_UNINTERRUPTIBLE);
> -- 
> 2.13.6
> 

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

[Boszormenyi Zoltan]

2017-12-18 19:56 keltezéssel, Guenter Roeck írta:
> On Mon, Dec 18, 2017 at 09:48:37AM +0100, Zoltan Boszormenyi wrote:
>> From: Böszörményi Zoltán <zboszor@pr.hu>
>>
>> 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.
>>
>> v3:
>>
>> Rebased for 4.15-rc4
>>
>> 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 93b4183cf53d..d198d6a58574 100644
>> --- a/include/linux/ioport.h
>> +++ b/include/linux/ioport.h
>> @@ -53,6 +53,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) */
>> @@ -218,7 +219,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) \
>> @@ -230,8 +238,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 54ba6de3757c..05db9c4e3992 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;
>> +
> 
> kzalloc() can fail, thus res can be NULL.

Thanks, I will fix it.

> 
>>   	return res;
>>   }
>>   
>> @@ -1128,8 +1133,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);
>> @@ -1146,7 +1162,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)
>> @@ -1156,6 +1171,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 (;;) {
>> @@ -1184,14 +1219,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.13.6
>>
> 

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

[Boszormenyi Zoltan]

2017-12-18 20:07 keltezéssel, Guenter Roeck írta:
> On Mon, Dec 18, 2017 at 09:48:38AM +0100, Zoltan Boszormenyi wrote:
>> From: Böszörményi Zoltán <zboszor@pr.hu>
>>
>> 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
> 
> resource
> 
>> inconsistent resource flag usage is logged with KERN_ERR.
>>
>> v2: Fixed checkpatch.pl warnings and extended the comment
>>      about request_declared_muxed_region.
>>
>> v3: Rebased for 4.15-rc4
>>
>> 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 05db9c4e3992..0f26f887ac33 100644
>> --- a/kernel/resource.c
>> +++ b/kernel/resource.c
>> @@ -1143,6 +1143,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.
>> @@ -1209,7 +1210,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);
> 
> With this, the muxed resource requestor will hang since the non-muxed
> requestor will not release the resource. I understand that you are trying
> to get rid of the error return, but is replacing it with a hang really
> better ?

I think it's a definite yes.

Consider the case of the regression this series is trying to fix.

Two drivers were trying to access a shared resource, both thinking
that it's the only one, which was almost true[1] initially.
An improvement in the i2c-piix4 driver in 4.4-rc3 broke sp5100_tco.
sp5100_tco was loaded second and failed outright.

This didn't make anyone to fix the situation, despite the fact
that there are at least three bugtrackers (kernel, Debian and Red Hat)
that point out the same problem.

So, a failing driver initialization is not horrifying enough to get
something fixed. Maybe a hang is.

Maybe this behaviour is still too weak, instead of doing it for an
inconsistent muxed/non-muxed case, it (at least the logging) should
be done for every case, to detect drivers that try to lock the same
resource.

In the case of an inconsistent resource flags usage, the bug is
not the hang itself, it's the inconsistent resource flags.

[1] The USB PCI quirks were accessing the same I/O resource without
locking and I found that on a Kabini machine, we got rare
"disabled by hub (EMI?), re-enabling..." reports that doesn't occur
with the locking applied. It is possible that two kernel threads were
doing I/O at the same time without synchronization.

> 
>>   			add_wait_queue(&muxed_resource_wait, &wait);
>>   			write_unlock(&resource_lock);
>>   			set_current_state(TASK_UNINTERRUPTIBLE);
>> -- 
>> 2.13.6
>>
> 

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

[Guenter Roeck]

On Tue, Dec 19, 2017 at 07:11:11AM +0100, Boszormenyi Zoltan wrote:
> 2017-12-18 20:07 keltezéssel, Guenter Roeck írta:
> >On Mon, Dec 18, 2017 at 09:48:38AM +0100, Zoltan Boszormenyi wrote:
> >>From: Böszörményi Zoltán <zboszor@pr.hu>
> >>
> >>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
> >
> >resource
> >
> >>inconsistent resource flag usage is logged with KERN_ERR.
> >>
> >>v2: Fixed checkpatch.pl warnings and extended the comment
> >>     about request_declared_muxed_region.
> >>
> >>v3: Rebased for 4.15-rc4
> >>
> >>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 05db9c4e3992..0f26f887ac33 100644
> >>--- a/kernel/resource.c
> >>+++ b/kernel/resource.c
> >>@@ -1143,6 +1143,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.
> >>@@ -1209,7 +1210,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);
> >
> >With this, the muxed resource requestor will hang since the non-muxed
> >requestor will not release the resource. I understand that you are trying
> >to get rid of the error return, but is replacing it with a hang really
> >better ?
> 
> I think it's a definite yes.
> 

I disagree. Some systems are configured to crash on stalls, which is
always worse than failure to load a driver.

I think the real problem is that usb_amd_quirk_pll() does not return
an error. It should, usb_amd_quirk_pll_disable() as well as
usb_amd_quirk_pll_enable() should pass the error to the calling code,
which should handle it. However, there is an even simpler solution -
see below.

> Consider the case of the regression this series is trying to fix.
> 
> Two drivers were trying to access a shared resource, both thinking
> that it's the only one, which was almost true[1] initially.
> An improvement in the i2c-piix4 driver in 4.4-rc3 broke sp5100_tco.
> sp5100_tco was loaded second and failed outright.
> 
> This didn't make anyone to fix the situation, despite the fact
> that there are at least three bugtrackers (kernel, Debian and Red Hat)
> that point out the same problem.
> 
> So, a failing driver initialization is not horrifying enough to get
> something fixed. Maybe a hang is.
> 
> Maybe this behaviour is still too weak, instead of doing it for an
> inconsistent muxed/non-muxed case, it (at least the logging) should
> be done for every case, to detect drivers that try to lock the same
> resource.
> 
> In the case of an inconsistent resource flags usage, the bug is
> not the hang itself, it's the inconsistent resource flags.
> 
Separate problem from the one you are trying to solve. One could argue
that the inconsistent resource use should dump a warning with traceback.
Sure, that would also cause the systems I referred to above to crash,
but it would do so with an actionable traceback, not with an obscure
hang which requires detailed analysis.

> [1] The USB PCI quirks were accessing the same I/O resource without
> locking and I found that on a Kabini machine, we got rare
> "disabled by hub (EMI?), re-enabling..." reports that doesn't occur
> with the locking applied. It is possible that two kernel threads were
> doing I/O at the same time without synchronization.
> 

The usb quirks code only accesses the registers in question to obtain
another set of index registers. It _could_ do that in its initialization
code instead and get the address through amd_chipset_info when needed.
If done corectly, this could actually simplify the related code in
usb_amd_quirk_pll(), since the operation on the address obtained
is the same for all supported chipsets.

Overall, I don't see a need for this API change. There are at least
two other solutions available to handle the usb quirks issue, one of
which is substantially less invasive. The problems in the other drivers
can be solved by using request_muxed_region().

Guenter

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

[Guenter Roeck]

On Tue, Dec 19, 2017 at 07:11:11AM +0100, Boszormenyi Zoltan wrote:
> 2017-12-18 20:07 keltezéssel, Guenter Roeck írta:
> >On Mon, Dec 18, 2017 at 09:48:38AM +0100, Zoltan Boszormenyi wrote:
> >>From: Böszörményi Zoltán <zboszor@pr.hu>
> >>
> >>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
> >
> >resource
> >
> >>inconsistent resource flag usage is logged with KERN_ERR.
> >>
> >>v2: Fixed checkpatch.pl warnings and extended the comment
> >>     about request_declared_muxed_region.
> >>
> >>v3: Rebased for 4.15-rc4
> >>
> >>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 05db9c4e3992..0f26f887ac33 100644
> >>--- a/kernel/resource.c
> >>+++ b/kernel/resource.c
> >>@@ -1143,6 +1143,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.
> >>@@ -1209,7 +1210,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);
> >
> >With this, the muxed resource requestor will hang since the non-muxed
> >requestor will not release the resource. I understand that you are trying
> >to get rid of the error return, but is replacing it with a hang really
> >better ?
> 
> I think it's a definite yes.
> 

I disagree. Some systems are configured to crash on stalls, which is
always worse than failure to load a driver.

I think the real problem is that usb_amd_quirk_pll() does not return
an error. It should, usb_amd_quirk_pll_disable() as well as
usb_amd_quirk_pll_enable() should pass the error to the calling code,
which should handle it. However, there is an even simpler solution -
see below.

> Consider the case of the regression this series is trying to fix.
> 
> Two drivers were trying to access a shared resource, both thinking
> that it's the only one, which was almost true[1] initially.
> An improvement in the i2c-piix4 driver in 4.4-rc3 broke sp5100_tco.
> sp5100_tco was loaded second and failed outright.
> 
> This didn't make anyone to fix the situation, despite the fact
> that there are at least three bugtrackers (kernel, Debian and Red Hat)
> that point out the same problem.
> 
> So, a failing driver initialization is not horrifying enough to get
> something fixed. Maybe a hang is.
> 
> Maybe this behaviour is still too weak, instead of doing it for an
> inconsistent muxed/non-muxed case, it (at least the logging) should
> be done for every case, to detect drivers that try to lock the same
> resource.
> 
> In the case of an inconsistent resource flags usage, the bug is
> not the hang itself, it's the inconsistent resource flags.
> 
Separate problem from the one you are trying to solve. One could argue
that the inconsistent resource use should dump a warning with traceback.
Sure, that would also cause the systems I referred to above to crash,
but it would do so with an actionable traceback, not with an obscure
hang which requires detailed analysis.

> [1] The USB PCI quirks were accessing the same I/O resource without
> locking and I found that on a Kabini machine, we got rare
> "disabled by hub (EMI?), re-enabling..." reports that doesn't occur
> with the locking applied. It is possible that two kernel threads were
> doing I/O at the same time without synchronization.
> 

The usb quirks code only accesses the registers in question to obtain
another set of index registers. It _could_ do that in its initialization
code instead and get the address through amd_chipset_info when needed.
If done corectly, this could actually simplify the related code in
usb_amd_quirk_pll(), since the operation on the address obtained
is the same for all supported chipsets.

Overall, I don't see a need for this API change. There are at least
two other solutions available to handle the usb quirks issue, one of
which is substantially less invasive. The problems in the other drivers
can be solved by using request_muxed_region().

Guenter

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

[Guenter Roeck]

On 12/18/2017 12:48 AM, Zoltan Boszormenyi wrote:
> From: Böszörményi Zoltán <zboszor-v1d7l9VOqKc@public.gmane.org>
> 
> 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.
> 
> v5: Rebased for 4.15-rc4
>      Use struct i2c_algorithm pointer directly instead of
>      a bool to select the proper algo.

This change makes sense, but it should be a separate patch.

Guenter

> 
> Signed-off-by: Zoltán Böszörményi <zboszor-v1d7l9VOqKc@public.gmane.org>
> ---
>   drivers/i2c/busses/i2c-piix4.c | 51 +++++++++++++++---------------------------
>   1 file changed, 18 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
> index 462948e2c535..1846d9ea7d27 100644
> --- a/drivers/i2c/busses/i2c-piix4.c
> +++ b/drivers/i2c/busses/i2c-piix4.c
> @@ -153,10 +153,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 u8 piix4_port_mask_sb800;
>   static u8 piix4_port_shift_sb800;
> @@ -169,7 +170,6 @@ struct i2c_piix4_adapdata {
>   	unsigned short smba;
>   
>   	/* SB800 */
> -	bool sb800_main;
>   	bool notify_imc;
>   	u8 port;		/* Port number, shifted */
>   };
> @@ -298,12 +298,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;
> @@ -373,7 +373,7 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
>   			break;
>   		}
>   	} 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) ?
> @@ -381,7 +381,7 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
>   				       SB800_PIIX4_PORT_IDX;
>   		piix4_port_mask_sb800 = SB800_PIIX4_PORT_IDX_MASK;
>   		piix4_port_shift_sb800 = SB800_PIIX4_PORT_IDX_SHIFT;
> -		mutex_unlock(&piix4_mutex_sb800);
> +		release_declared_region(&sb800_res);
>   	}
>   
>   	dev_info(&PIIX4_dev->dev,
> @@ -679,7 +679,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);
> @@ -695,7 +695,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;
>   	}
>   
> @@ -753,7 +753,7 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
>   	if ((size == I2C_SMBUS_BLOCK_DATA) && adapdata->notify_imc)
>   		piix4_imc_wakeup();
>   
> -	mutex_unlock(&piix4_mutex_sb800);
> +	release_declared_region(&sb800_res);
>   
>   	return retval;
>   }
> @@ -804,7 +804,8 @@ static struct i2c_adapter *piix4_main_adapters[PIIX4_MAX_ADAPTERS];
>   static struct i2c_adapter *piix4_aux_adapter;
>   
>   static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
> -			     bool sb800_main, u8 port, bool notify_imc,
> +			     struct i2c_algorithm *algo,
> +			     u8 port, bool notify_imc,
>   			     const char *name, struct i2c_adapter **padap)
>   {
>   	struct i2c_adapter *adap;
> @@ -819,8 +820,7 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
>   
>   	adap->owner = THIS_MODULE;
>   	adap->class = I2C_CLASS_HWMON | I2C_CLASS_SPD;
> -	adap->algo = sb800_main ? &piix4_smbus_algorithm_sb800
> -				: &smbus_algorithm;
> +	adap->algo = algo;
>   
>   	adapdata = kzalloc(sizeof(*adapdata), GFP_KERNEL);
>   	if (adapdata == NULL) {
> @@ -830,7 +830,6 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
>   	}
>   
>   	adapdata->smba = smba;
> -	adapdata->sb800_main = sb800_main;
>   	adapdata->port = port << piix4_port_shift_sb800;
>   	adapdata->notify_imc = notify_imc;
>   
> @@ -862,7 +861,9 @@ static int piix4_add_adapters_sb800(struct pci_dev *dev, unsigned short smba,
>   	int retval;
>   
>   	for (port = 0; port < PIIX4_MAX_ADAPTERS; port++) {
> -		retval = piix4_add_adapter(dev, smba, true, port, notify_imc,
> +		retval = piix4_add_adapter(dev, smba,
> +					   &piix4_smbus_algorithm_sb800,
> +					   port, notify_imc,
>   					   piix4_main_port_names_sb800[port],
>   					   &piix4_main_adapters[port]);
>   		if (retval < 0)
> @@ -899,13 +900,6 @@ static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
>   		bool notify_imc = false;
>   		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;
> -		}
> -
>   		if (dev->vendor == PCI_VENDOR_ID_AMD &&
>   		    dev->device == PCI_DEVICE_ID_AMD_KERNCZ_SMBUS) {
>   			u8 imc;
> @@ -922,27 +916,24 @@ static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
>   
>   		/* 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, notify_imc);
> -		if (retval < 0) {
> -			release_region(SB800_PIIX4_SMB_IDX, 2);
> +		if (retval < 0)
>   			return retval;
> -		}
>   	} else {
>   		retval = piix4_setup(dev, id);
>   		if (retval < 0)
>   			return retval;
>   
>   		/* Try to register main SMBus adapter, give up if we can't */
> -		retval = piix4_add_adapter(dev, retval, false, 0, false, "",
> +		retval = piix4_add_adapter(dev, retval,
> +					   &smbus_algorithm, 0, false, "",
>   					   &piix4_main_adapters[0]);
>   		if (retval < 0)
>   			return retval;
> @@ -983,11 +974,8 @@ static void piix4_adap_remove(struct i2c_adapter *adap)
>   
>   	if (adapdata->smba) {
>   		i2c_del_adapter(adap);
> -		if (adapdata->port == (0 << piix4_port_shift_sb800)) {
> +		if (adapdata->port == (0 << piix4_port_shift_sb800))
>   			release_region(adapdata->smba, SMBIOSIZE);
> -			if (adapdata->sb800_main)
> -				release_region(SB800_PIIX4_SMB_IDX, 2);
> -		}
>   		kfree(adapdata);
>   		kfree(adap);
>   	}
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Guenter Roeck]

On 12/18/2017 12:48 AM, Zoltan Boszormenyi wrote:
> From: Böszörményi Zoltán <zboszor@pr.hu>
> 
> 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.
> 
> v5: Rebased for 4.15-rc4
>      Use struct i2c_algorithm pointer directly instead of
>      a bool to select the proper algo.

This change makes sense, but it should be a separate patch.

Guenter

> 
> Signed-off-by: Zoltán Böszörményi <zboszor@pr.hu>
> ---
>   drivers/i2c/busses/i2c-piix4.c | 51 +++++++++++++++---------------------------
>   1 file changed, 18 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
> index 462948e2c535..1846d9ea7d27 100644
> --- a/drivers/i2c/busses/i2c-piix4.c
> +++ b/drivers/i2c/busses/i2c-piix4.c
> @@ -153,10 +153,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 u8 piix4_port_mask_sb800;
>   static u8 piix4_port_shift_sb800;
> @@ -169,7 +170,6 @@ struct i2c_piix4_adapdata {
>   	unsigned short smba;
>   
>   	/* SB800 */
> -	bool sb800_main;
>   	bool notify_imc;
>   	u8 port;		/* Port number, shifted */
>   };
> @@ -298,12 +298,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;
> @@ -373,7 +373,7 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
>   			break;
>   		}
>   	} 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) ?
> @@ -381,7 +381,7 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
>   				       SB800_PIIX4_PORT_IDX;
>   		piix4_port_mask_sb800 = SB800_PIIX4_PORT_IDX_MASK;
>   		piix4_port_shift_sb800 = SB800_PIIX4_PORT_IDX_SHIFT;
> -		mutex_unlock(&piix4_mutex_sb800);
> +		release_declared_region(&sb800_res);
>   	}
>   
>   	dev_info(&PIIX4_dev->dev,
> @@ -679,7 +679,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);
> @@ -695,7 +695,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;
>   	}
>   
> @@ -753,7 +753,7 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
>   	if ((size == I2C_SMBUS_BLOCK_DATA) && adapdata->notify_imc)
>   		piix4_imc_wakeup();
>   
> -	mutex_unlock(&piix4_mutex_sb800);
> +	release_declared_region(&sb800_res);
>   
>   	return retval;
>   }
> @@ -804,7 +804,8 @@ static struct i2c_adapter *piix4_main_adapters[PIIX4_MAX_ADAPTERS];
>   static struct i2c_adapter *piix4_aux_adapter;
>   
>   static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
> -			     bool sb800_main, u8 port, bool notify_imc,
> +			     struct i2c_algorithm *algo,
> +			     u8 port, bool notify_imc,
>   			     const char *name, struct i2c_adapter **padap)
>   {
>   	struct i2c_adapter *adap;
> @@ -819,8 +820,7 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
>   
>   	adap->owner = THIS_MODULE;
>   	adap->class = I2C_CLASS_HWMON | I2C_CLASS_SPD;
> -	adap->algo = sb800_main ? &piix4_smbus_algorithm_sb800
> -				: &smbus_algorithm;
> +	adap->algo = algo;
>   
>   	adapdata = kzalloc(sizeof(*adapdata), GFP_KERNEL);
>   	if (adapdata == NULL) {
> @@ -830,7 +830,6 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
>   	}
>   
>   	adapdata->smba = smba;
> -	adapdata->sb800_main = sb800_main;
>   	adapdata->port = port << piix4_port_shift_sb800;
>   	adapdata->notify_imc = notify_imc;
>   
> @@ -862,7 +861,9 @@ static int piix4_add_adapters_sb800(struct pci_dev *dev, unsigned short smba,
>   	int retval;
>   
>   	for (port = 0; port < PIIX4_MAX_ADAPTERS; port++) {
> -		retval = piix4_add_adapter(dev, smba, true, port, notify_imc,
> +		retval = piix4_add_adapter(dev, smba,
> +					   &piix4_smbus_algorithm_sb800,
> +					   port, notify_imc,
>   					   piix4_main_port_names_sb800[port],
>   					   &piix4_main_adapters[port]);
>   		if (retval < 0)
> @@ -899,13 +900,6 @@ static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
>   		bool notify_imc = false;
>   		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;
> -		}
> -
>   		if (dev->vendor == PCI_VENDOR_ID_AMD &&
>   		    dev->device == PCI_DEVICE_ID_AMD_KERNCZ_SMBUS) {
>   			u8 imc;
> @@ -922,27 +916,24 @@ static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
>   
>   		/* 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, notify_imc);
> -		if (retval < 0) {
> -			release_region(SB800_PIIX4_SMB_IDX, 2);
> +		if (retval < 0)
>   			return retval;
> -		}
>   	} else {
>   		retval = piix4_setup(dev, id);
>   		if (retval < 0)
>   			return retval;
>   
>   		/* Try to register main SMBus adapter, give up if we can't */
> -		retval = piix4_add_adapter(dev, retval, false, 0, false, "",
> +		retval = piix4_add_adapter(dev, retval,
> +					   &smbus_algorithm, 0, false, "",
>   					   &piix4_main_adapters[0]);
>   		if (retval < 0)
>   			return retval;
> @@ -983,11 +974,8 @@ static void piix4_adap_remove(struct i2c_adapter *adap)
>   
>   	if (adapdata->smba) {
>   		i2c_del_adapter(adap);
> -		if (adapdata->port == (0 << piix4_port_shift_sb800)) {
> +		if (adapdata->port == (0 << piix4_port_shift_sb800))
>   			release_region(adapdata->smba, SMBIOSIZE);
> -			if (adapdata->sb800_main)
> -				release_region(SB800_PIIX4_SMB_IDX, 2);
> -		}
>   		kfree(adapdata);
>   		kfree(adap);
>   	}
>