[PATCH v2 1/2] i2c: i801: Fix resume bug

* [PATCH v2 1/2] i2c: i801: Fix resume bug
@ 2020-09-01 13:22 Jean Delvare
  2020-09-01 13:28 ` [PATCH v2 2/2] i2c: i801: Simplify the suspend callback Jean Delvare
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Jean Delvare @ 2020-09-01 13:22 UTC (permalink / raw)
  To: Linux I2C; +Cc: Volker Rümelin, Bjorn Helgaas, Vaibhav Gupta, Wolfram Sang

From: Volker Rümelin <vr_qemu@t-online.de>

On suspend the original host configuration gets restored. The
resume routine has to undo this, otherwise the SMBus master
may be left in disabled state or in i2c mode.

[JD: Rebased on v5.8, moved the write into i801_setup_hstcfg.]

Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
Signed-off-by: Jean Delvare <jdelvare@suse.de>
---
I noticed this bug in a QEMU x86_64 q35 VM booted with OVMF. OVMF
doesn't inititialize the SMBus master. After 1s of SMBus inactivity
autosuspend disables the SMBus master. To reproduce please note QEMU's
ICH9 SMBus emulation does not handle interrupts and it's necessary
to pass the parameter disable_features=0x10 to the i2c_i801 driver.

Changes since v1:
 * Move the write into i801_setup_hstcfg() itself (suggested by Wolfram
   Sang).
 * Pass struct i801_priv * as the parameter to i801_setup_hstcfg(). It
   includes everything we need.

 drivers/i2c/busses/i2c-i801.c |   21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

--- linux-5.8.orig/drivers/i2c/busses/i2c-i801.c	2020-08-31 15:09:29.221616330 +0200
+++ linux-5.8/drivers/i2c/busses/i2c-i801.c	2020-09-01 12:42:46.003491616 +0200
@@ -1709,6 +1709,16 @@ static inline int i801_acpi_probe(struct
 static inline void i801_acpi_remove(struct i801_priv *priv) { }
 #endif
 
+static unsigned char i801_setup_hstcfg(struct i801_priv *priv)
+{
+	unsigned char hstcfg = priv->original_hstcfg;
+
+	hstcfg &= ~SMBHSTCFG_I2C_EN;	/* SMBus timing */
+	hstcfg |= SMBHSTCFG_HST_EN;
+	pci_write_config_byte(priv->pci_dev, SMBHSTCFG, hstcfg);
+	return hstcfg;
+}
+
 static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 {
 	unsigned char temp;
@@ -1830,14 +1840,10 @@ static int i801_probe(struct pci_dev *de
 		return err;
 	}
 
-	pci_read_config_byte(priv->pci_dev, SMBHSTCFG, &temp);
-	priv->original_hstcfg = temp;
-	temp &= ~SMBHSTCFG_I2C_EN;	/* SMBus timing */
-	if (!(temp & SMBHSTCFG_HST_EN)) {
+	pci_read_config_byte(priv->pci_dev, SMBHSTCFG, &priv->original_hstcfg);
+	temp = i801_setup_hstcfg(priv);
+	if (!(priv->original_hstcfg & SMBHSTCFG_HST_EN))
 		dev_info(&dev->dev, "Enabling SMBus device\n");
-		temp |= SMBHSTCFG_HST_EN;
-	}
-	pci_write_config_byte(priv->pci_dev, SMBHSTCFG, temp);
 
 	if (temp & SMBHSTCFG_SMB_SMI_EN) {
 		dev_dbg(&dev->dev, "SMBus using interrupt SMI#\n");
@@ -1963,6 +1969,7 @@ static int i801_resume(struct device *de
 {
 	struct i801_priv *priv = dev_get_drvdata(dev);
 
+	i801_setup_hstcfg(priv);
 	i801_enable_host_notify(&priv->adapter);
 
 	return 0;


-- 
Jean Delvare
SUSE L3 Support

^ permalink raw reply	[flat|nested] 9+ messages in thread