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.
> @@ -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:
signature.asc
Description: PGP signature
