[PATCH] i2c: i801: Fix resume bug

[PATCH] i2c: i801: Fix resume bug

[Jean Delvare]

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.

 drivers/i2c/busses/i2c-i801.c |   21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

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


-- 
Jean Delvare
SUSE L3 Support

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

[Wolfram Sang]

[-- 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 --]

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

[Bjorn Helgaas]

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

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

[Jean Delvare]

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.

-- 
Jean Delvare
SUSE L3 Support

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

[Vaibhav Gupta]

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

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

[Jean Delvare]

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,
-- 
Jean Delvare
SUSE L3 Support