Bug#853122: [Regression] Changes to i2c-piix4.c initialisation prevent loading of sp5100_tco watchdog driver on AMD SB800 chipset

Bug#853122: [Regression] Changes to i2c-piix4.c initialisation prevent loading of sp5100_tco watchdog driver on AMD SB800 chipset

[Paul Menzel]

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

Dear Linux folks,


Unfortunately, commit 2fee61d22e (i2c: piix4: Add support for
multiplexed main adapter in SB800) [1] caused a regression. Tim
reported that to the Linux Kernel Bugtracker as bug #170741 last
September [2], but it looks like the affected subsystems don’t use it.

So I just copy his report in here, and put all the people in CC
mentioned in the commit, the bug report, and in the subsystems
I2C/SMBUS CONTROLLER DRIVERS FOR PC and WATCHDOG DEVICE DRIVERS
mentioned in the file `MAINTAINERS`.

> On the AMD Turion N40L and other related SoCs, the i2c-piix4 driver
> now claims the 0xcd6 ioport, preventing the sp5100_tco watchdog
> driver from loading:
> 
> piix4_smbus 0000:00:14.0: SMBus Host Controller at 0xb00, revision 0
> piix4_smbus 0000:00:14.0: Using register 0x2c for SMBus port selection
> piix4_smbus 0000:00:14.0: Auxiliary SMBus Host Controller at 0xb20
> sp5100_tco: SP5100/SB800 TCO WatchDog Timer Driver v0.05
> sp5100_tco: PCI Vendor ID: 0x1002, Device ID: 0x4385, Revision ID: 0x42
> sp5100_tco: I/O address 0x0cd6 already in use
> 
> 
> This breaks watchdog operation on existing systems on upgrade and new
> deployments unless the i2c-piix4 driver is blacklisted.
> 
> See:
> 
> drivers/watchdog/sp5100_tco.c
> 
> tco_timer_enable(void)
> 
> (SB800_IO_PM_INDEX_REG is defined in drivers/watchdog/sp5100_tco.h)
> 
> This is the commit which prevents the watchdog driver from loading:
> 
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=2fee61d22e606fc99ade9079fda15fdee83ec33e
> 
> See also AMD docs
> 
> 45483_sb800_bdg_pub_3.03
> 
> Perhaps a fix is to switch both drivers from static to dynamic
> allocation of the IO ports in question, since the watchdog driver
> only accesses the port during initialisation (with backoff/retry
> maybe to avoid races?).

Is there somebody having the resources to implement the dynamic
allocation to solve this regression?


Thanks,

Paul


[1] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=2fee61d22e606fc99ade9079fda15fdee83ec33e
[2] https://bugzilla.kernel.org/show_bug.cgi?id=170741

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [Regression] Changes to i2c-piix4.c initialisation prevent loading of sp5100_tco watchdog driver on AMD SB800 chipset

[Wolfram Sang]

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


> Unfortunately, commit 2fee61d22e (i2c: piix4: Add support for
> multiplexed main adapter in SB800) [1] caused a regression. Tim
> reported that to the Linux Kernel Bugtracker as bug #170741 last
> September [2], but it looks like the affected subsystems don’t use it.

Jean Delvare pointed out this issue amongst others[1] last year already.
Let me quote:

===

5* The I/O ports used for SMBus configuration and port switching are
also needed by a watchdog driver, sp5100_tco. Both drivers request the
region, so the first one wins, and the other driver can't be loaded.
sp5100_tco was there first, so the changes done to the i2c-piix4 driver
recently will cause a regression for some users by preventing them
from using the sp5100_tco and i2c-piix4 drivers at the same time. In
the long run I guess we will need a helper module to handle this shared
resource. Unless IORESOURCE_MUXED can be used for that. Either way,
that's more work than I can put into this before kernel v4.5 is
released. For the time being, I think we should simply make it
non-fatal if the I/O ports can't be requested, and continue without
multiplexing (as before.)

===

Seems nobody had the resources, so far. I don't have the HW and not much
experience with non-embedded platforms. I wonder, though, if we really
need to convert the drivers to MFD ones, or if we could use the simpler
MFD_SYSCON mechanism which helps in exactly such cases for embedded
platforms. But I am really lacking details here and am afraid this is
probably all the input I can give currently.

Regards,

   Wolfram

[1] http://www.spinics.net/lists/linux-i2c/msg23437.html


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

Re: [Regression] Changes to i2c-piix4.c initialisation prevent loading of sp5100_tco watchdog driver on AMD SB800 chipset

[Paul Menzel]

[-- Attachment #1.1: Type: text/plain, Size: 2281 bytes --]

Dear Wolfram,


Thank you for the reply, which we talked about briefly at the
Chemnitzer LinuxTage.


Am Freitag, den 03.03.2017, 11:17 +0100 schrieb Wolfram Sang:
> > Unfortunately, commit 2fee61d22e (i2c: piix4: Add support for
> > multiplexed main adapter in SB800) [1] caused a regression. Tim
> > reported that to the Linux Kernel Bugtracker as bug #170741 last
> > September [2], but it looks like the affected subsystems don’t use it.
> 
> Jean Delvare pointed out this issue amongst others[1] last year already.
> Let me quote:
> 
> ===
> 
> 5* The I/O ports used for SMBus configuration and port switching are
> also needed by a watchdog driver, sp5100_tco. Both drivers request the
> region, so the first one wins, and the other driver can't be loaded.
> sp5100_tco was there first, so the changes done to the i2c-piix4 driver
> recently will cause a regression for some users by preventing them
> from using the sp5100_tco and i2c-piix4 drivers at the same time. In
> the long run I guess we will need a helper module to handle this shared
> resource. Unless IORESOURCE_MUXED can be used for that. Either way,
> that's more work than I can put into this before kernel v4.5 is
> released. For the time being, I think we should simply make it
> non-fatal if the I/O ports can't be requested, and continue without
> multiplexing (as before.)
> 
> ===
> 
> Seems nobody had the resources, so far.

I still don’t understand, why Jean then not immediately reverted the
commit to adhere to the Linux Kernel’s no-regression-policy.

> I don't have the HW and not much experience with non-embedded
> platforms. I wonder, though, if we really need to convert the drivers
> to MFD ones, or if we could use the simpler MFD_SYSCON mechanism
> which helps in exactly such cases for embedded platforms. But I am
> really lacking details here and am afraid this is probably all the
> input I can give currently.

Zoltan stepped up, and uploaded a patch for review to the Kernel.org
Bugzilla [2], also attached to this message.

Christian, Tim, and Nehal could you please test and review it?


Thanks,

Paul


> [1] http://www.spinics.net/lists/linux-i2c/msg23437.html
[2] https://bugzilla.kernel.org/show_bug.cgi?id=170741

[-- Attachment #1.2: i2c-piix4-coexist-with-sp5100_tco.patch --]
[-- Type: text/x-patch, Size: 10897 bytes --]

From: Zoltán Böszörményi <zboszor@pr.hu>
Date: Tue Mar 28 14:53:07 2017 +0200
Subject: [PATCH] watchdog/sp5100_tco: Coexist with i2c-piix

Currently, the kernel says this when i2c-piix loads before sp5100_tco:

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

Both i2c-piix4 and sp5100_tco uses a static request_region() call
so it depends on the load order which one wins.

i2c-piix4 uses a mutex to protect I/O port accesses to the pair of
I/O ports. Replace this mutex lock / unlock with request_muxed_region()
and release_region() around the I/O port accesses in i2c-piix4.

Add request_muxed_region() / release_region() pairs around the I/O
accesses in sp5100_tco.

This will act as a cross-driver mutex.

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

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index c21ca7b..16befdd5 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -40,7 +40,6 @@
 #include <linux/dmi.h>
 #include <linux/acpi.h>
 #include <linux/io.h>
-#include <linux/mutex.h>
 
 
 /* PIIX4 SMBus address offsets */
@@ -144,10 +143,9 @@ static const struct dmi_system_id piix4_dmi_ibm[] = {
 
 /*
  * SB800 globals
- * piix4_mutex_sb800 protects piix4_port_sel_sb800 and the pair
- * of I/O ports at SB800_PIIX4_SMB_IDX.
+ * the pair of I/O ports at SB800_PIIX4_SMB_IDX are protected
+ * by request_muxed_region / release_region
  */
-static DEFINE_MUTEX(piix4_mutex_sb800);
 static u8 piix4_port_sel_sb800;
 static const char *piix4_main_port_names_sb800[PIIX4_MAX_ADAPTERS] = {
 	" port 0", " port 2", " port 3", " port 4"
@@ -157,8 +155,6 @@ static const char *piix4_aux_port_name_sb800 = " port 1";
 struct i2c_piix4_adapdata {
 	unsigned short smba;
 
-	/* SB800 */
-	bool sb800_main;
 	u8 port;		/* Port number, shifted */
 };
 
@@ -261,6 +257,14 @@ static int piix4_setup(struct pci_dev *PIIX4_dev,
 	return piix4_smba;
 }
 
+static inline void enter_sb800(void) {
+	request_muxed_region(SB800_PIIX4_SMB_IDX, 2, "smba_idx");
+}
+
+static inline void leave_sb800(void) {
+	release_region(SB800_PIIX4_SMB_IDX, 2);
+}
+
 static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
 			     const struct pci_device_id *id, u8 aux)
 {
@@ -286,12 +290,12 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
 	else
 		smb_en = (aux) ? 0x28 : 0x2c;
 
-	mutex_lock(&piix4_mutex_sb800);
+	enter_sb800();
 	outb_p(smb_en, SB800_PIIX4_SMB_IDX);
 	smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1);
 	outb_p(smb_en + 1, SB800_PIIX4_SMB_IDX);
 	smba_en_hi = inb_p(SB800_PIIX4_SMB_IDX + 1);
-	mutex_unlock(&piix4_mutex_sb800);
+	leave_sb800();
 
 	if (!smb_en) {
 		smb_en_status = smba_en_lo & 0x10;
@@ -349,13 +353,13 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
 	if (PIIX4_dev->vendor == PCI_VENDOR_ID_AMD) {
 		piix4_port_sel_sb800 = SB800_PIIX4_PORT_IDX_ALT;
 	} else {
-		mutex_lock(&piix4_mutex_sb800);
+		enter_sb800();
 		outb_p(SB800_PIIX4_PORT_IDX_SEL, SB800_PIIX4_SMB_IDX);
 		port_sel = inb_p(SB800_PIIX4_SMB_IDX + 1);
 		piix4_port_sel_sb800 = (port_sel & 0x01) ?
 				       SB800_PIIX4_PORT_IDX_ALT :
 				       SB800_PIIX4_PORT_IDX;
-		mutex_unlock(&piix4_mutex_sb800);
+		leave_sb800();
 	}
 
 	dev_info(&PIIX4_dev->dev,
@@ -592,7 +596,7 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
 	u8 port;
 	int retval;
 
-	mutex_lock(&piix4_mutex_sb800);
+	enter_sb800();
 
 	/* Request the SMBUS semaphore, avoid conflicts with the IMC */
 	smbslvcnt  = inb_p(SMBSLVCNT);
@@ -608,7 +612,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);
+		leave_sb800();
 		return -EBUSY;
 	}
 
@@ -628,7 +632,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);
+	leave_sb800();
 
 	return retval;
 }
@@ -705,7 +709,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,17 +774,9 @@ 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);
 			return retval;
 		}
 
@@ -791,7 +786,6 @@ static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
 		 */
 		retval = piix4_add_adapters_sb800(dev, retval);
 		if (retval < 0) {
-			release_region(SB800_PIIX4_SMB_IDX, 2);
 			return retval;
 		}
 	} else {
@@ -841,11 +835,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);
 	}
diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
index 028618c..ff332a6 100644
--- a/drivers/watchdog/sp5100_tco.c
+++ b/drivers/watchdog/sp5100_tco.c
@@ -53,6 +53,7 @@ static DEFINE_SPINLOCK(tco_lock);	/* Guards the hardware */
 static unsigned long timer_alive;
 static char tco_expect_close;
 static struct pci_dev *sp5100_tco_pci;
+static const char *sp5100_tco_dev_name = NULL;
 
 /* the watchdog platform device */
 static struct platform_device *sp5100_tco_platform_device;
@@ -71,6 +72,20 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started."
 		" (default=" __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
 
 /*
+ * Protect accessing the pair of I/O ports
+ * This relies on the fact that SB800_IO_PM_INDEX_REG and
+ * SP5100_IO_PM_INDEX_REG are the same value.
+ */
+static inline void enter_sp5100(void) {
+	request_muxed_region(SP5100_IO_PM_INDEX_REG, SP5100_PM_IOPORTS_SIZE,
+			(sp5100_tco_dev_name ? sp5100_tco_dev_name : "sp5100_tco") );
+}
+
+static inline void leave_sp5100(void) {
+	release_region(SP5100_IO_PM_INDEX_REG, SP5100_PM_IOPORTS_SIZE);
+}
+
+/*
  * Some TCO specific functions
  */
 
@@ -138,6 +153,8 @@ static void tco_timer_enable(void)
 
 	if (!tco_has_sp5100_reg_layout(sp5100_tco_pci)) {
 		/* For SB800 or later */
+		enter_sp5100();
+
 		/* Set the Watchdog timer resolution to 1 sec */
 		outb(SB800_PM_WATCHDOG_CONFIG, SB800_IO_PM_INDEX_REG);
 		val = inb(SB800_IO_PM_DATA_REG);
@@ -150,6 +167,8 @@ static void tco_timer_enable(void)
 		val |= SB800_PCI_WATCHDOG_DECODE_EN;
 		val &= ~SB800_PM_WATCHDOG_DISABLE;
 		outb(val, SB800_IO_PM_DATA_REG);
+
+		leave_sp5100();
 	} else {
 		/* For SP5100 or SB7x0 */
 		/* Enable watchdog decode bit */
@@ -164,11 +183,13 @@ static void tco_timer_enable(void)
 				       val);
 
 		/* Enable Watchdog timer and set the resolution to 1 sec */
+		enter_sp5100();
 		outb(SP5100_PM_WATCHDOG_CONTROL, SP5100_IO_PM_INDEX_REG);
 		val = inb(SP5100_IO_PM_DATA_REG);
 		val |= SP5100_PM_WATCHDOG_SECOND_RES;
 		val &= ~SP5100_PM_WATCHDOG_DISABLE;
 		outb(val, SP5100_IO_PM_DATA_REG);
+		leave_sp5100();
 	}
 }
 
@@ -327,7 +348,6 @@ MODULE_DEVICE_TABLE(pci, sp5100_tco_pci_tbl);
 static unsigned char sp5100_tco_setupdevice(void)
 {
 	struct pci_dev *dev = NULL;
-	const char *dev_name = NULL;
 	u32 val;
 	u32 index_reg, data_reg, base_addr;
 
@@ -350,27 +370,21 @@ static unsigned char sp5100_tco_setupdevice(void)
 	 * Determine type of southbridge chipset.
 	 */
 	if (tco_has_sp5100_reg_layout(sp5100_tco_pci)) {
-		dev_name = SP5100_DEVNAME;
+		sp5100_tco_dev_name = SP5100_DEVNAME;
 		index_reg = SP5100_IO_PM_INDEX_REG;
 		data_reg = SP5100_IO_PM_DATA_REG;
 		base_addr = SP5100_PM_WATCHDOG_BASE;
 	} else {
-		dev_name = SB800_DEVNAME;
+		sp5100_tco_dev_name = SB800_DEVNAME;
 		index_reg = SB800_IO_PM_INDEX_REG;
 		data_reg = SB800_IO_PM_DATA_REG;
 		base_addr = SB800_PM_WATCHDOG_BASE;
 	}
 
-	/* Request the IO ports used by this driver */
-	pm_iobase = SP5100_IO_PM_INDEX_REG;
-	if (!request_region(pm_iobase, SP5100_PM_IOPORTS_SIZE, dev_name)) {
-		pr_err("I/O address 0x%04x already in use\n", pm_iobase);
-		goto exit;
-	}
-
 	/*
 	 * First, Find the watchdog timer MMIO address from indirect I/O.
 	 */
+	enter_sp5100();
 	outb(base_addr+3, index_reg);
 	val = inb(data_reg);
 	outb(base_addr+2, index_reg);
@@ -380,12 +394,13 @@ static unsigned char sp5100_tco_setupdevice(void)
 	outb(base_addr+0, index_reg);
 	/* Low three bits of BASE are reserved */
 	val = val << 8 | (inb(data_reg) & 0xf8);
+	leave_sp5100();
 
 	pr_debug("Got 0x%04x from indirect I/O\n", val);
 
 	/* Check MMIO address conflict */
 	if (request_mem_region_exclusive(val, SP5100_WDT_MEM_MAP_SIZE,
-								dev_name))
+								sp5100_tco_dev_name))
 		goto setup_wdt;
 	else
 		pr_debug("MMIO address 0x%04x already in use\n", val);
@@ -400,6 +415,7 @@ static unsigned char sp5100_tco_setupdevice(void)
 				      SP5100_SB_RESOURCE_MMIO_BASE, &val);
 	} else {
 		/* Read SBResource_MMIO from AcpiMmioEn(PM_Reg: 24h) */
+		enter_sp5100();
 		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 +424,7 @@ static unsigned char sp5100_tco_setupdevice(void)
 		val = val << 8 | inb(SB800_IO_PM_DATA_REG);
 		outb(SB800_PM_ACPI_MMIO_EN+0, SB800_IO_PM_INDEX_REG);
 		val = val << 8 | inb(SB800_IO_PM_DATA_REG);
+		leave_sp5100();
 	}
 
 	/* The SBResource_MMIO is enabled and mapped memory space? */
@@ -419,7 +436,7 @@ static unsigned char sp5100_tco_setupdevice(void)
 		val += SB800_PM_WDT_MMIO_OFFSET;
 		/* Check MMIO address conflict */
 		if (request_mem_region_exclusive(val, SP5100_WDT_MEM_MAP_SIZE,
-								   dev_name)) {
+								   sp5100_tco_dev_name)) {
 			pr_debug("Got 0x%04x from SBResource_MMIO register\n",
 				val);
 			goto setup_wdt;
@@ -429,7 +446,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 +486,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;
 }

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [Regression] Changes to i2c-piix4.c initialisation prevent loading of sp5100_tco watchdog driver on AMD SB800 chipset

[Guenter Roeck]

Hi Paul,

On 03/31/2017 12:17 AM, Paul Menzel wrote:
> Dear Wolfram,
>
>
> Thank you for the reply, which we talked about briefly at the
> Chemnitzer LinuxTage.
>
>
> Am Freitag, den 03.03.2017, 11:17 +0100 schrieb Wolfram Sang:
>>> Unfortunately, commit 2fee61d22e (i2c: piix4: Add support for
>>> multiplexed main adapter in SB800) [1] caused a regression. Tim
>>> reported that to the Linux Kernel Bugtracker as bug #170741 last
>>> September [2], but it looks like the affected subsystems don’t use it.
>>
>> Jean Delvare pointed out this issue amongst others[1] last year already.
>> Let me quote:
>>
>> ===
>>
>> 5* The I/O ports used for SMBus configuration and port switching are
>> also needed by a watchdog driver, sp5100_tco. Both drivers request the
>> region, so the first one wins, and the other driver can't be loaded.
>> sp5100_tco was there first, so the changes done to the i2c-piix4 driver
>> recently will cause a regression for some users by preventing them
>> from using the sp5100_tco and i2c-piix4 drivers at the same time. In
>> the long run I guess we will need a helper module to handle this shared
>> resource. Unless IORESOURCE_MUXED can be used for that. Either way,
>> that's more work than I can put into this before kernel v4.5 is
>> released. For the time being, I think we should simply make it
>> non-fatal if the I/O ports can't be requested, and continue without
>> multiplexing (as before.)
>>
>> ===
>>
>> Seems nobody had the resources, so far.
>
> I still don’t understand, why Jean then not immediately reverted the
> commit to adhere to the Linux Kernel’s no-regression-policy.
>
>> I don't have the HW and not much experience with non-embedded
>> platforms. I wonder, though, if we really need to convert the drivers
>> to MFD ones, or if we could use the simpler MFD_SYSCON mechanism
>> which helps in exactly such cases for embedded platforms. But I am
>> really lacking details here and am afraid this is probably all the
>> input I can give currently.
>
> Zoltan stepped up, and uploaded a patch for review to the Kernel.org
> Bugzilla [2], also attached to this message.
>

Please don't send patches as attachments.

request_muxed_region() can fail, and literally every other driver
using it checks for that failure. Please do the same.

The sp5100_tco_dev_name change in the watchdog driver is unnecessary.
There are some unnecessary { } in the watchdog driver after the patch
is applied.

Please split the patch into two patches so they can be reviewed and
applied separately.

Thanks,
Guenter

Bug#853122: [Regression] Changes to i2c-piix4.c initialisation prevent loading of sp5100_tco watchdog driver on AMD SB800 chipset

[Boszormenyi Zoltan]

2017-03-31 14:49 keltezéssel, Guenter Roeck írta:
> Hi Paul,
>
> On 03/31/2017 12:17 AM, Paul Menzel wrote:
>> Dear Wolfram,
>>
>>
>> Thank you for the reply, which we talked about briefly at the
>> Chemnitzer LinuxTage.
>>
>>
>> Am Freitag, den 03.03.2017, 11:17 +0100 schrieb Wolfram Sang:
>>>> Unfortunately, commit 2fee61d22e (i2c: piix4: Add support for
>>>> multiplexed main adapter in SB800) [1] caused a regression. Tim
>>>> reported that to the Linux Kernel Bugtracker as bug #170741 last
>>>> September [2], but it looks like the affected subsystems don’t use it.
>>>
>>> Jean Delvare pointed out this issue amongst others[1] last year already.
>>> Let me quote:
>>>
>>> ===
>>>
>>> 5* The I/O ports used for SMBus configuration and port switching are
>>> also needed by a watchdog driver, sp5100_tco. Both drivers request the
>>> region, so the first one wins, and the other driver can't be loaded.
>>> sp5100_tco was there first, so the changes done to the i2c-piix4 driver
>>> recently will cause a regression for some users by preventing them
>>> from using the sp5100_tco and i2c-piix4 drivers at the same time. In
>>> the long run I guess we will need a helper module to handle this shared
>>> resource. Unless IORESOURCE_MUXED can be used for that. Either way,
>>> that's more work than I can put into this before kernel v4.5 is
>>> released. For the time being, I think we should simply make it
>>> non-fatal if the I/O ports can't be requested, and continue without
>>> multiplexing (as before.)
>>>
>>> ===
>>>
>>> Seems nobody had the resources, so far.
>>
>> I still don’t understand, why Jean then not immediately reverted the
>> commit to adhere to the Linux Kernel’s no-regression-policy.
>>
>>> I don't have the HW and not much experience with non-embedded
>>> platforms. I wonder, though, if we really need to convert the drivers
>>> to MFD ones, or if we could use the simpler MFD_SYSCON mechanism
>>> which helps in exactly such cases for embedded platforms. But I am
>>> really lacking details here and am afraid this is probably all the
>>> input I can give currently.
>>
>> Zoltan stepped up, and uploaded a patch for review to the Kernel.org
>> Bugzilla [2], also attached to this message.
>>
>
> Please don't send patches as attachments.
>
> request_muxed_region() can fail, and literally every other driver
> using it checks for that failure. Please do the same.

In what circumstances can request_muxed_region() fail? As far as
I can see, only if two drivers use the same I/O port base and the
already present region did not use IORESOURCE_MUXED which is
not the case here. When request_muxed_region() is used consistently,
subsequent requests are put on a wait queue and the first one is
woken up when the region is released. So, it's basically a mutex.
Am I missing something here?

Alternatively, the original "piix4_mutex_sb800" mutex can be moved out
to its own file and used by both drivers. This way, request_muxed_region()
would not be needed at all among these two drivers.

The third option is to remove the request_*region() calls and the mutex
completely.

After all, drivers/usb/host/pci-quirks.c::usb_amd_quirk_pll() also uses
this I/O port pair and this is done without request_*region() or locking.
It is for some AMD devices, SB800 included, for which the function
accesses the same I/O ports used by both i2c-piix4 and sp5100_tco.

/*
  * The hardware normally enables the A-link power management feature, which
  * lets the system lower the power consumption in idle states.
  *
  * This USB quirk prevents the link going into that lower power state
  * during isochronous transfers.
  *
  * Without this quirk, isochronous stream on OHCI/EHCI/xHCI controllers of
  * some AMD platforms may stutter or have breaks occasionally.
  */
static void usb_amd_quirk_pll(int disable)

This function is hidden behind two wrappers: usb_amd_quirk_pll_disable()
and usb_amd_quirk_pll_enable() and these are used from:

drivers/usb/host/ohci-q.c
drivers/usb/host/xhci-ring.c
drivers/usb/host/ehci-sched.c

> The sp5100_tco_dev_name change in the watchdog driver is unnecessary.

request_muxed_region() requires a const char * name to be passed.
sp5100_tco uses a different DEVNAME for the two device generations.
I wanted to preserve this distinction but dev_name is local to
sp5100_tco_setupdevice() and it would have been an overkill to call
tco_has_sp5100_reg_layout() every time the code executes
request_muxed_region().

Are you saying that this distinction is unnecessary, too?
I would prefer a single DEVNAME that just contains the driver name.
Less code, less confusion.

> There are some unnecessary { } in the watchdog driver after the patch
> is applied.

True, there are two places.

> Please split the patch into two patches so they can be reviewed and
> applied separately.

Likely I will, but I would like to hear the others' opinion, too.

1. a single patch with a single goal: protect I/O port access
    for SB800 across the whole kernel
2. patch series for same goal for the two drivers separately
    (or three if pci-quirks.c needs to change as well)

and

A) a common mutex across the two (three) drivers in the kernel, or
B) request_muxed_region() with retrying in the usual manner if it fails:

    #define enter_sb800() \
       do { \
       } while (!request_muxed_region(...))

Best regards,
Zoltán Böszörményi

Re: [Regression] Changes to i2c-piix4.c initialisation prevent loading of sp5100_tco watchdog driver on AMD SB800 chipset

[Guenter Roeck]

On Fri, Mar 31, 2017 at 04:46:02PM +0200, Boszormenyi Zoltan wrote:
> 2017-03-31 14:49 keltezéssel, Guenter Roeck írta:
> >Hi Paul,
> >
> >On 03/31/2017 12:17 AM, Paul Menzel wrote:
> >>Dear Wolfram,
> >>
> >>
> >>Thank you for the reply, which we talked about briefly at the
> >>Chemnitzer LinuxTage.
> >>
> >>
> >>Am Freitag, den 03.03.2017, 11:17 +0100 schrieb Wolfram Sang:
> >>>>Unfortunately, commit 2fee61d22e (i2c: piix4: Add support for
> >>>>multiplexed main adapter in SB800) [1] caused a regression. Tim
> >>>>reported that to the Linux Kernel Bugtracker as bug #170741 last
> >>>>September [2], but it looks like the affected subsystems don’t use it.
> >>>
> >>>Jean Delvare pointed out this issue amongst others[1] last year already.
> >>>Let me quote:
> >>>
> >>>===
> >>>
> >>>5* The I/O ports used for SMBus configuration and port switching are
> >>>also needed by a watchdog driver, sp5100_tco. Both drivers request the
> >>>region, so the first one wins, and the other driver can't be loaded.
> >>>sp5100_tco was there first, so the changes done to the i2c-piix4 driver
> >>>recently will cause a regression for some users by preventing them
> >>>from using the sp5100_tco and i2c-piix4 drivers at the same time. In
> >>>the long run I guess we will need a helper module to handle this shared
> >>>resource. Unless IORESOURCE_MUXED can be used for that. Either way,
> >>>that's more work than I can put into this before kernel v4.5 is
> >>>released. For the time being, I think we should simply make it
> >>>non-fatal if the I/O ports can't be requested, and continue without
> >>>multiplexing (as before.)
> >>>
> >>>===
> >>>
> >>>Seems nobody had the resources, so far.
> >>
> >>I still don’t understand, why Jean then not immediately reverted the
> >>commit to adhere to the Linux Kernel’s no-regression-policy.
> >>
> >>>I don't have the HW and not much experience with non-embedded
> >>>platforms. I wonder, though, if we really need to convert the drivers
> >>>to MFD ones, or if we could use the simpler MFD_SYSCON mechanism
> >>>which helps in exactly such cases for embedded platforms. But I am
> >>>really lacking details here and am afraid this is probably all the
> >>>input I can give currently.
> >>
> >>Zoltan stepped up, and uploaded a patch for review to the Kernel.org
> >>Bugzilla [2], also attached to this message.
> >>
> >
> >Please don't send patches as attachments.
> >
> >request_muxed_region() can fail, and literally every other driver
> >using it checks for that failure. Please do the same.
> 
> In what circumstances can request_muxed_region() fail? As far as
> I can see, only if two drivers use the same I/O port base and the
> already present region did not use IORESOURCE_MUXED which is
> not the case here. When request_muxed_region() is used consistently,
> subsequent requests are put on a wait queue and the first one is
> woken up when the region is released. So, it's basically a mutex.
> Am I missing something here?
> 

Yes. failure to allocate the resource is one. The other is that you are
making the assumption that all other requesters have IORESOURCE_MUXED set.
There is no guarantee that this is the case.

Guenter

> Alternatively, the original "piix4_mutex_sb800" mutex can be moved out
> to its own file and used by both drivers. This way, request_muxed_region()
> would not be needed at all among these two drivers.
> 
> The third option is to remove the request_*region() calls and the mutex
> completely.
> 
> After all, drivers/usb/host/pci-quirks.c::usb_amd_quirk_pll() also uses
> this I/O port pair and this is done without request_*region() or locking.
> It is for some AMD devices, SB800 included, for which the function
> accesses the same I/O ports used by both i2c-piix4 and sp5100_tco.
> 
> /*
>  * The hardware normally enables the A-link power management feature, which
>  * lets the system lower the power consumption in idle states.
>  *
>  * This USB quirk prevents the link going into that lower power state
>  * during isochronous transfers.
>  *
>  * Without this quirk, isochronous stream on OHCI/EHCI/xHCI controllers of
>  * some AMD platforms may stutter or have breaks occasionally.
>  */
> static void usb_amd_quirk_pll(int disable)
> 
> This function is hidden behind two wrappers: usb_amd_quirk_pll_disable()
> and usb_amd_quirk_pll_enable() and these are used from:
> 
> drivers/usb/host/ohci-q.c
> drivers/usb/host/xhci-ring.c
> drivers/usb/host/ehci-sched.c
> 
> >The sp5100_tco_dev_name change in the watchdog driver is unnecessary.
> 
> request_muxed_region() requires a const char * name to be passed.
> sp5100_tco uses a different DEVNAME for the two device generations.
> I wanted to preserve this distinction but dev_name is local to
> sp5100_tco_setupdevice() and it would have been an overkill to call
> tco_has_sp5100_reg_layout() every time the code executes
> request_muxed_region().
> 
> Are you saying that this distinction is unnecessary, too?
> I would prefer a single DEVNAME that just contains the driver name.
> Less code, less confusion.
> 
> >There are some unnecessary { } in the watchdog driver after the patch
> >is applied.
> 
> True, there are two places.
> 
> >Please split the patch into two patches so they can be reviewed and
> >applied separately.
> 
> Likely I will, but I would like to hear the others' opinion, too.
> 
> 1. a single patch with a single goal: protect I/O port access
>    for SB800 across the whole kernel
> 2. patch series for same goal for the two drivers separately
>    (or three if pci-quirks.c needs to change as well)
> 
> and
> 
> A) a common mutex across the two (three) drivers in the kernel, or
> B) request_muxed_region() with retrying in the usual manner if it fails:
> 
>    #define enter_sb800() \
>       do { \
>       } while (!request_muxed_region(...))
> 
> Best regards,
> Zoltán Böszörményi
> 

Re: [Regression] Changes to i2c-piix4.c initialisation prevent loading of sp5100_tco watchdog driver on AMD SB800 chipset

[Boszormenyi Zoltan]

2017-03-31 17:05 keltezéssel, Guenter Roeck írta:
> On Fri, Mar 31, 2017 at 04:46:02PM +0200, Boszormenyi Zoltan wrote:
>> 2017-03-31 14:49 keltezéssel, Guenter Roeck írta:
>>> request_muxed_region() can fail, and literally every other driver
>>> using it checks for that failure. Please do the same.
>>
>> In what circumstances can request_muxed_region() fail? As far as
>> I can see, only if two drivers use the same I/O port base and the
>> already present region did not use IORESOURCE_MUXED which is
>> not the case here. When request_muxed_region() is used consistently,
>> subsequent requests are put on a wait queue and the first one is
>> woken up when the region is released. So, it's basically a mutex.
>> Am I missing something here?
>>
>
> Yes. failure to allocate the resource is one.

So, a common mutex should be used.

I have also added synchronization to the USB PCI quirks code and
have split the patch into three pieces now (USB quirks, i2c-piix4 and
sp5100_tco) and they were sent to the relevant mailing lists.

I don't know which subsystem wants to take it, all 3 patches are
needed at once.

Best regards,
Zoltán Böszörményi
--
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: [Regression] Changes to i2c-piix4.c initialisation prevent loading of sp5100_tco watchdog driver on AMD SB800 chipset

[Guenter Roeck]

On 04/01/2017 03:13 AM, Boszormenyi Zoltan wrote:
> 2017-03-31 17:05 keltezéssel, Guenter Roeck írta:
>> On Fri, Mar 31, 2017 at 04:46:02PM +0200, Boszormenyi Zoltan wrote:
>>> 2017-03-31 14:49 keltezéssel, Guenter Roeck írta:
>>>> request_muxed_region() can fail, and literally every other driver
>>>> using it checks for that failure. Please do the same.
>>>
>>> In what circumstances can request_muxed_region() fail? As far as
>>> I can see, only if two drivers use the same I/O port base and the
>>> already present region did not use IORESOURCE_MUXED which is
>>> not the case here. When request_muxed_region() is used consistently,
>>> subsequent requests are put on a wait queue and the first one is
>>> woken up when the region is released. So, it's basically a mutex.
>>> Am I missing something here?
>>>
>>
>> Yes. failure to allocate the resource is one.
>
> So, a common mutex should be used.
>

Just because you don't want to check for errors ?

I am not on favor of your new solution. I think it violates layering all over
the place, and I dislike the idea of having a global mutex as you propose.
I won't shut it down, but I'll let others provide feedback on your new series
of patches.

Guenter

> I have also added synchronization to the USB PCI quirks code and
> have split the patch into three pieces now (USB quirks, i2c-piix4 and
> sp5100_tco) and they were sent to the relevant mailing lists.
>
> I don't know which subsystem wants to take it, all 3 patches are
> needed at once.
>
> Best regards,
> Zoltán Böszörményi
>

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

Bug#853122: [Regression] Changes to i2c-piix4.c initialisation prevent loading of sp5100_tco watchdog driver on AMD SB800 chipset

[Boszormenyi Zoltan]

2017-04-01 15:32 keltezéssel, Guenter Roeck írta:
> On 04/01/2017 03:13 AM, Boszormenyi Zoltan wrote:
>> 2017-03-31 17:05 keltezéssel, Guenter Roeck írta:
>>> On Fri, Mar 31, 2017 at 04:46:02PM +0200, Boszormenyi Zoltan wrote:
>>>> 2017-03-31 14:49 keltezéssel, Guenter Roeck írta:
>>>>> request_muxed_region() can fail, and literally every other driver
>>>>> using it checks for that failure. Please do the same.
>>>>
>>>> In what circumstances can request_muxed_region() fail? As far as
>>>> I can see, only if two drivers use the same I/O port base and the
>>>> already present region did not use IORESOURCE_MUXED which is
>>>> not the case here. When request_muxed_region() is used consistently,
>>>> subsequent requests are put on a wait queue and the first one is
>>>> woken up when the region is released. So, it's basically a mutex.
>>>> Am I missing something here?
>>>>
>>>
>>> Yes. failure to allocate the resource is one.
>>
>> So, a common mutex should be used.
>>
>
> Just because you don't want to check for errors ?
>
> I am not on favor of your new solution. I think it violates layering all over
> the place, and I dislike the idea of having a global mutex as you propose.
> I won't shut it down, but I'll let others provide feedback on your new series
> of patches.

It's not because I don't want to check for errors, it's about avoiding them.

It's not my favourite either but there is a lot of weight in existing code.

You cannot avoid layering violations if multiple driver subsystems use
their respective functionality in devices in multiplexed way.

The biggest problem is that quirk functions, including usb_amd_quirk_pll()
has the "void" return type so there is no way to indicate errors to the
callers in case of an error.

The best clean alternative would be add new resource handling infrastructure.
* Expose the currently static alloc_resource() in kernel/resource.c
   With this, driver initialization can allocate the resource once
   for the lifetime of the driver and it it fails,
* Add a new insert_muxed_region() / __insert_muxed_region() function with
   different semantics from request_muxed_region() / __request_region():
   1 Accept a pointer to already allocated resource.
   2 If the conflicting resource doesn't have IORESOURCE_MUXED set,
     complain loudly in the syslog but still go into the wait queue.
     The conflicting resource also has the name which can be printed
     so the inconsistent resource / region usage can be fixed.
   We can also just modify the __request_region() semantics, so:
   1 It accepts a pointer to an allocated resource or NULL.
     In the second case, the resource is allocated internally and can
     still fail.
   2 The above second point. But this may cause an error in code that
     expects the old semantics.

The window for request_muxed_region()+release_region() is so short
that the requested I/O port range would not show up in /proc/ioports.

All this would be to fix only 3 drivers in a no-error scenario and only
achieving the functionality of a mutex seems to be overkill.

Another alternative is to revert commit 2fee61d22e606fc99ade9079fda15fdee83ec33e
that caused the regression in sp5100_tco in the first place.

Best regards,
Zoltán Böszörményi

Re: [Regression] Changes to i2c-piix4.c initialisation prevent loading of sp5100_tco watchdog driver on AMD SB800 chipset

[Boszormenyi Zoltan]

2017-04-01 18:20 keltezéssel, Boszormenyi Zoltan írta:
> The best clean alternative would be add new resource handling infrastructure.
> * Expose the currently static alloc_resource() in kernel/resource.c
>   With this, driver initialization can allocate the resource once
>   for the lifetime of the driver and it it fails,

(unfinished sentence) then the failure is during driver initialization,
not during runtime, possibly days or weeks later.

> * Add a new insert_muxed_region() / __insert_muxed_region() function with
>   different semantics from request_muxed_region() / __request_region():
>   1 Accept a pointer to already allocated resource.
>   2 If the conflicting resource doesn't have IORESOURCE_MUXED set,
>     complain loudly in the syslog but still go into the wait queue.
>     The conflicting resource also has the name which can be printed
>     so the inconsistent resource / region usage can be fixed.
>   We can also just modify the __request_region() semantics, so:
>   1 It accepts a pointer to an allocated resource or NULL.
>     In the second case, the resource is allocated internally and can
>     still fail.
>   2 The above second point. But this may cause an error in code that
>     expects the old semantics.
>
> The window for request_muxed_region()+release_region() is so short
> that the requested I/O port range would not show up in /proc/ioports.
>
> All this would be to fix only 3 drivers in a no-error scenario and only
> achieving the functionality of a mutex seems to be overkill.
>
> Another alternative is to revert commit 2fee61d22e606fc99ade9079fda15fdee83ec33e
> that caused the regression in sp5100_tco in the first place.
>
> Best regards,
> Zoltán Böszörményi
>

Re: [Regression] Changes to i2c-piix4.c initialisation prevent loading of sp5100_tco watchdog driver on AMD SB800 chipset

[Paul Menzel]

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

Dear Zoltán,


Am Samstag, den 01.04.2017, 12:13 +0200 schrieb Boszormenyi Zoltan:

[…]

> and have split the patch into three pieces now (USB quirks, i2c-piix4 
> and sp5100_tco) and they were sent to the relevant mailing lists.

Could you please add me to the receiver list of these patches, so that
I can test them? Maybe also Christian (the commit author introducing
the regression), Tim (bug reporter), and Nehal from AMD?

If you uploaded them to the Kernel.org Bugtracker, that’d also work for
me.


Thanks,

Paul

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [Regression] Changes to i2c-piix4.c initialisation prevent loading of sp5100_tco watchdog driver on AMD SB800 chipset

[Boszormenyi Zoltan]

Hi,

2017-04-03 08:34 keltezéssel, Paul Menzel írta:
> Dear Zoltán,
>
>
> Am Samstag, den 01.04.2017, 12:13 +0200 schrieb Boszormenyi Zoltan:
>
> […]
>
>> and have split the patch into three pieces now (USB quirks, i2c-piix4
>> and sp5100_tco) and they were sent to the relevant mailing lists.
>
> Could you please add me to the receiver list of these patches, so that
> I can test them? Maybe also Christian (the commit author introducing
> the regression), Tim (bug reporter), and Nehal from AMD?
>
> If you uploaded them to the Kernel.org Bugtracker, that’d also work for
> me.

did both.

Best regards,
Zoltán Böszörményi

>
>
> Thanks,
>
> Paul
>

Re: [Regression] Changes to i2c-piix4.c initialisation prevent loading of sp5100_tco watchdog driver on AMD SB800 chipset

[Boszormenyi Zoltan]

2017-04-03 09:59 keltezéssel, Boszormenyi Zoltan írta:
> Hi,
> 
> 2017-04-03 08:34 keltezéssel, Paul Menzel írta:
>> Dear Zoltán,
>>
>>
>> Am Samstag, den 01.04.2017, 12:13 +0200 schrieb Boszormenyi Zoltan:
>>
>> […]
>>
>>> and have split the patch into three pieces now (USB quirks, i2c-piix4
>>> and sp5100_tco) and they were sent to the relevant mailing lists.
>>
>> Could you please add me to the receiver list of these patches, so that
>> I can test them? Maybe also Christian (the commit author introducing
>> the regression), Tim (bug reporter), and Nehal from AMD?
>>
>> If you uploaded them to the Kernel.org Bugtracker, that’d also work for
>> me.
> 
> did both.

There's a new set of packages sent to all relevant mailing lists and
also attached to the kernel bugtracker at
https://bugzilla.kernel.org/show_bug.cgi?id=170741

Hopefully this approach will work for everyone.

> 
> Best regards,
> Zoltán Böszörményi
> 
>>
>>
>> Thanks,
>>
>> Paul
>>
> 

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