| Message ID | 20200901152221.3cea0048@endymion |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/2] i2c: i801: Fix resume bug | expand |
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.
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 >
On Tue, Sep 01, 2020 at 03:28:37PM +0200, Jean Delvare wrote: > We don't actually need to derive the PCI device from the device > structure, as we already have a pointer to it in our private data > structure. > > Signed-off-by: Jean Delvare <jdelvare@suse.de> To reduce dependencies, this is also applied to for-current, thanks!
--- 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;