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

[Wolfram Sang <wsa@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 1770 bytes --]

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.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]