Re: [PATCH] i2c: i801: Fix resume bug

[Bjorn Helgaas <helgaas@kernel.org>]

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