diff mbox series

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

Message ID 20200901152221.3cea0048@endymion
State Accepted
Headers show
Series [v2,1/2] i2c: i801: Fix resume bug | expand

Commit Message

Jean Delvare Sept. 1, 2020, 1:22 p.m. UTC 
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(-)

Comments

Jean Delvare Sept. 3, 2020, 5:17 p.m. UTC | #1 
On Tue, 1 Sep 2020 15:22:21 +0200, Jean Delvare wrote:
> 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>

Oh and by the way this deserves a Cc: stable@, methinks.
Volker Rümelin Sept. 6, 2020, 8 a.m. UTC | #2 
Hi Jean,

with these two patches the code in i2c-i801.c looks really good.

But there is an issue with the reproducer.

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

Since commit a9c8088c7988e "i2c: i801: Don't restore config
registers on runtime PM" the reproducer doesn't work anymore.
This is because commit a9c8088c7988e works as intended and the
pm->runtime_* callbacks no longer call i801_suspend() and
i801_resume().

But there is more. With the SMBus master in runtime suspended state
the direct-complete mechanism skips the calls to the pm->suspend
and pm->resume callbacks on system suspend/resume. I am convinced
in nearly all cases this disables the fix from commit a5aaea37858fb
"i2c-i801: Restore the device state before leaving".

At the moment I see two ways to fix this problem. One way is to
revert a9c8088c7988e "i2c: i801: Don't restore config registers
on runtime PM", the other is to set the driver flag
DPM_FLAG_NO_DIRECT_COMPLETE in i801_probe(). I tested both, but I
can't decide which way is better.

With best regards,
Volker
Wolfram Sang Sept. 10, 2020, 7:09 a.m. UTC | #3 
Hi Volker, hi Jean,

On Sun, Sep 06, 2020 at 10:00:50AM +0200, Volker Rümelin wrote:
> Hi Jean,
> 
> with these two patches the code in i2c-i801.c looks really good.
> 
> But there is an issue with the reproducer.

I am not familiar with the HW; do we want these two patches here or does
the issue below need to be solved first? And if we want them, is it
still stable material?

Regards,

   Wolfram

> 
> > 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.
> 
> Since commit a9c8088c7988e "i2c: i801: Don't restore config
> registers on runtime PM" the reproducer doesn't work anymore.
> This is because commit a9c8088c7988e works as intended and the
> pm->runtime_* callbacks no longer call i801_suspend() and
> i801_resume().
> 
> But there is more. With the SMBus master in runtime suspended state
> the direct-complete mechanism skips the calls to the pm->suspend
> and pm->resume callbacks on system suspend/resume. I am convinced
> in nearly all cases this disables the fix from commit a5aaea37858fb
> "i2c-i801: Restore the device state before leaving".
> 
> At the moment I see two ways to fix this problem. One way is to
> revert a9c8088c7988e "i2c: i801: Don't restore config registers
> on runtime PM", the other is to set the driver flag
> DPM_FLAG_NO_DIRECT_COMPLETE in i801_probe(). I tested both, but I
> can't decide which way is better.
> 
> With best regards,
> Volker
>
Jean Delvare Sept. 10, 2020, 9:14 a.m. UTC | #4 
Hi Volker,

On Sun, 6 Sep 2020 10:00:50 +0200, Volker Rümelin wrote:
> with these two patches the code in i2c-i801.c looks really good.

Great, thanks for the review.

> But there is an issue with the reproducer.
> 
> > 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.  
> 
> Since commit a9c8088c7988e "i2c: i801: Don't restore config
> registers on runtime PM" the reproducer doesn't work anymore.
> This is because commit a9c8088c7988e works as intended and the
> pm->runtime_* callbacks no longer call i801_suspend() and
> i801_resume().

To be honest I was surprised that you were able to reproduce the resume
bug with QEMU in the first place. I did not remember about commit
a9c8088c7988e.

> But there is more. With the SMBus master in runtime suspended state
> the direct-complete mechanism skips the calls to the pm->suspend
> and pm->resume callbacks on system suspend/resume. I am convinced
> in nearly all cases this disables the fix from commit a5aaea37858fb
> "i2c-i801: Restore the device state before leaving".

My understanding of the PM subsystem is fairly limited (it evolves
faster than my leaning curve I'm afraid) but you are most certainly
right on that.

> At the moment I see two ways to fix this problem. One way is to
> revert a9c8088c7988e "i2c: i801: Don't restore config registers
> on runtime PM", the other is to set the driver flag
> DPM_FLAG_NO_DIRECT_COMPLETE in i801_probe(). I tested both, but I
> can't decide which way is better.

Reverting a9c8088c7988e would make run-time PM inefficient again, so I
am opposed to doing that. Flag DPM_FLAG_NO_DIRECT_COMPLETE seems to
have been introduced for exactly this purpose, so I would just use
that. To be honest I don't really understand why it is not the default,
but again I don't know much about the PM subsystem.
Jean Delvare Sept. 10, 2020, 9:32 a.m. UTC | #5 
Hi Wolfram,

On Thu, 10 Sep 2020 09:09:13 +0200, Wolfram Sang wrote:
> On Sun, Sep 06, 2020 at 10:00:50AM +0200, Volker Rümelin wrote:
> > with these two patches the code in i2c-i801.c looks really good.
> > 
> > But there is an issue with the reproducer.  
> 
> I am not familiar with the HW; do we want these two patches here or does
> the issue below need to be solved first? And if we want them, is it
> still stable material?

The new issue pointed out by Volker is independent from the bug being
fixed here. We do want these 2 patches applied now, with the 1st one
being stable material. The second patch is only a clean-up so it
doesn't need to go to stable.

This new issue exists since April 2018. I'm surprised it wasn't
reported as a regression earlier. Maybe newer BIOSes are getting better
at not making assumptions about register states at suspend time (let me
dream!) Anyway, it deserves its own fix, which also qualifies for
stable in my opinion.
Wolfram Sang Sept. 14, 2020, 7:03 a.m. UTC | #6 
On Tue, Sep 01, 2020 at 03:22:21PM +0200, Jean Delvare wrote:
> 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>

Added stable and applied to for-current, thanks!
diff mbox series

Patch

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