| Message ID | 20200825191303.4a258073@endymion |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i2c: i801: Fix resume bug | expand |
On Tue, Aug 25, 2020 at 07:13:03PM +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, simplified a condition.] > > 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 initialize 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. > > Note from JD: I can't test this. Thanks for keeping up with this one. I have one high level comment but I hope Volker, Bjorn, and Vaibhav have comments/tags, too. > +static unsigned char i801_setup_hstcfg(unsigned char hstcfg) > +{ > + hstcfg &= ~SMBHSTCFG_I2C_EN; /* SMBus timing */ > + hstcfg |= SMBHSTCFG_HST_EN; > + return hstcfg; > +} What about putting the write to SMBHSTCFG here, too. When I read the function name, I assumed it will do that. > @@ -1961,8 +1965,11 @@ static int i801_suspend(struct device *d > > static int i801_resume(struct device *dev) > { > - struct i801_priv *priv = dev_get_drvdata(dev); > + struct pci_dev *pci_dev = to_pci_dev(dev); > + struct i801_priv *priv = pci_get_drvdata(pci_dev); > + unsigned char hstcfg = i801_setup_hstcfg(priv->original_hstcfg); > > + pci_write_config_byte(pci_dev, SMBHSTCFG, hstcfg); And on top of that, we could skip the 'hstcfg' variable here.
On Fri, Aug 28, 2020 at 10:57:53AM +0200, Wolfram Sang wrote: > On Tue, Aug 25, 2020 at 07:13:03PM +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, simplified a condition.] > > > > 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 initialize 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. > > > > Note from JD: I can't test this. > > Thanks for keeping up with this one. I have one high level comment but I > hope Volker, Bjorn, and Vaibhav have comments/tags, too. > > > +static unsigned char i801_setup_hstcfg(unsigned char hstcfg) > > +{ > > + hstcfg &= ~SMBHSTCFG_I2C_EN; /* SMBus timing */ > > + hstcfg |= SMBHSTCFG_HST_EN; > > + return hstcfg; > > +} > > What about putting the write to SMBHSTCFG here, too. When I read the > function name, I assumed it will do that. From the point of view of suspend/resume, I think it's nice to have a write in i801_resume() to match the one in i801_suspend(). Putting the write inside i801_setup_hstcfg() would mean i801_probe() would do *two* writes instead of the one it currently does. But I don't really care other than that :) > > @@ -1961,8 +1965,11 @@ static int i801_suspend(struct device *d > > > > static int i801_resume(struct device *dev) > > { > > - struct i801_priv *priv = dev_get_drvdata(dev); > > + struct pci_dev *pci_dev = to_pci_dev(dev); > > + struct i801_priv *priv = pci_get_drvdata(pci_dev); > > + unsigned char hstcfg = i801_setup_hstcfg(priv->original_hstcfg); > > > > + pci_write_config_byte(pci_dev, SMBHSTCFG, hstcfg); > > And on top of that, we could skip the 'hstcfg' variable here. >
Hi Bjorn, Wolfram, On Fri, 28 Aug 2020 11:40:36 -0500, Bjorn Helgaas wrote: > On Fri, Aug 28, 2020 at 10:57:53AM +0200, Wolfram Sang wrote: > > On Tue, Aug 25, 2020 at 07:13:03PM +0200, Jean Delvare wrote: > > > +static unsigned char i801_setup_hstcfg(unsigned char hstcfg) > > > +{ > > > + hstcfg &= ~SMBHSTCFG_I2C_EN; /* SMBus timing */ > > > + hstcfg |= SMBHSTCFG_HST_EN; > > > + return hstcfg; > > > +} > > > > What about putting the write to SMBHSTCFG here, too. When I read the > > function name, I assumed it will do that. > > From the point of view of suspend/resume, I think it's nice to have a > write in i801_resume() to match the one in i801_suspend(). I agree that symmetry is nice in general, but I must agree with Wolfram, a function named "setup" which doesn't actually perform any action is kind of confusing. I believe that moving the write in there makes sense as both callers will do exactly that immediately after. > Putting the write inside i801_setup_hstcfg() would mean i801_probe() > would do *two* writes instead of the one it currently does. I can't see why that would be the case.
On Fri, Aug 28, 2020 at 10:57:53AM +0200, Wolfram Sang wrote: > On Tue, Aug 25, 2020 at 07:13:03PM +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, simplified a condition.] > > > > 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 initialize 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. > > > > Note from JD: I can't test this. > > Thanks for keeping up with this one. I have one high level comment but I > hope Volker, Bjorn, and Vaibhav have comments/tags, too. > > > +static unsigned char i801_setup_hstcfg(unsigned char hstcfg) > > +{ > > + hstcfg &= ~SMBHSTCFG_I2C_EN; /* SMBus timing */ > > + hstcfg |= SMBHSTCFG_HST_EN; > > + return hstcfg; > > +} > > What about putting the write to SMBHSTCFG here, too. When I read the > function name, I assumed it will do that. > > > @@ -1961,8 +1965,11 @@ static int i801_suspend(struct device *d > > > > static int i801_resume(struct device *dev) > > { > > - struct i801_priv *priv = dev_get_drvdata(dev); > > + struct pci_dev *pci_dev = to_pci_dev(dev); > > + struct i801_priv *priv = pci_get_drvdata(pci_dev); > > + unsigned char hstcfg = i801_setup_hstcfg(priv->original_hstcfg); > > > > + pci_write_config_byte(pci_dev, SMBHSTCFG, hstcfg); > > And on top of that, we could skip the 'hstcfg' variable here. > Add extra parameter of "struct pci_dev*" type in i801_setup_hstcfg(). We can also skip the introduction of "struct pci_dev*" type variable in .resume() then. Just use to_pci_dev(dev) . Thanks Vaibhav Gupta
On Mon, 31 Aug 2020 20:29:29 +0530, Vaibhav Gupta wrote: > On Fri, Aug 28, 2020 at 10:57:53AM +0200, Wolfram Sang wrote: > > On Tue, Aug 25, 2020 at 07:13:03PM +0200, Jean Delvare wrote: > > > @@ -1961,8 +1965,11 @@ static int i801_suspend(struct device *d > > > > > > static int i801_resume(struct device *dev) > > > { > > > - struct i801_priv *priv = dev_get_drvdata(dev); > > > + struct pci_dev *pci_dev = to_pci_dev(dev); > > > + struct i801_priv *priv = pci_get_drvdata(pci_dev); > > > + unsigned char hstcfg = i801_setup_hstcfg(priv->original_hstcfg); > > > > > > + pci_write_config_byte(pci_dev, SMBHSTCFG, hstcfg); > > > > And on top of that, we could skip the 'hstcfg' variable here. > > Add extra parameter of "struct pci_dev*" type in i801_setup_hstcfg(). That was my first approach, yes. But then I tried passing the struct i801_priv * instead, and it looks even neater to me (but I'm biased because I did it). I'll post that for review shortly. > We can > also skip the introduction of "struct pci_dev*" type variable in .resume() > then. Just use to_pci_dev(dev) . I don't really mind keeping the pci_dev variable, for symmetry with .suspend(), and the compiler will optimize it away anyway. There's a minor cleanup possible in this area though, to which I'll give a try. Thanks,
--- linux-5.8.orig/drivers/i2c/busses/i2c-i801.c 2020-08-25 11:54:30.371682962 +0200 +++ linux-5.8/drivers/i2c/busses/i2c-i801.c 2020-08-25 14:06:06.168116285 +0200 @@ -1709,6 +1709,13 @@ static inline int i801_acpi_probe(struct static inline void i801_acpi_remove(struct i801_priv *priv) { } #endif +static unsigned char i801_setup_hstcfg(unsigned char hstcfg) +{ + hstcfg &= ~SMBHSTCFG_I2C_EN; /* SMBus timing */ + hstcfg |= SMBHSTCFG_HST_EN; + return hstcfg; +} + static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) { unsigned char temp; @@ -1830,13 +1837,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->original_hstcfg); + 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) { @@ -1961,8 +1965,11 @@ static int i801_suspend(struct device *d static int i801_resume(struct device *dev) { - struct i801_priv *priv = dev_get_drvdata(dev); + struct pci_dev *pci_dev = to_pci_dev(dev); + struct i801_priv *priv = pci_get_drvdata(pci_dev); + unsigned char hstcfg = i801_setup_hstcfg(priv->original_hstcfg); + pci_write_config_byte(pci_dev, SMBHSTCFG, hstcfg); i801_enable_host_notify(&priv->adapter); return 0;