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@xxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxx>
> > Signed-off-by: Jean Delvare <jdelvare@xxxxxxx>
> > ---
> > 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.
>
