[PATCH] i2c-i801: Fix resume when PEC is used

[PATCH] i2c-i801: Fix resume when PEC is used

[Jean Delvare]

Fix for bug #6395:
Fail to resume on Tecra M2 with ADM1032 and Intel 82801DBM

The BIOS of the Tecra M2 doesn't like it when it has to reboot or
resume after the i2c-i801 driver has left the SMBus in PEC mode. So we
need to add proper .suspend, .resume and .shutdown callbacks to that
driver, where we clear and restore the PEC mode bit appropriately.

Thanks to Daniele Gaffuri for the very good report and contribution.

Signed-off-by: Jean Delvare <khali@linux-fr.org>
---
 drivers/i2c/busses/i2c-i801.c |   30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

--- linux-2.6.17-rc1.orig/drivers/i2c/busses/i2c-i801.c	2006-04-18 09:34:15.000000000 +0200
+++ linux-2.6.17-rc1/drivers/i2c/busses/i2c-i801.c	2006-04-18 12:47:07.000000000 +0200
@@ -110,6 +110,7 @@
 static struct pci_driver i801_driver;
 static struct pci_dev *I801_dev;
 static int isich4;
+static u8 saved_auxctl;
 
 static int i801_setup(struct pci_dev *dev)
 {
@@ -551,17 +552,46 @@
 	return i2c_add_adapter(&i801_adapter);
 }
 
+static void i801_shutdown(struct pci_dev *dev)
+{
+	if (isich4) {
+		/* Disable PEC mode before leaving, else some BIOSes will
+		   fail to resume/reboot. */
+		outb_p(0, SMBAUXCTL);
+	}
+}
+
 static void __devexit i801_remove(struct pci_dev *dev)
 {
 	i2c_del_adapter(&i801_adapter);
+	i801_shutdown(dev);
 	release_region(i801_smba, (isich4 ? 16 : 8));
 }
 
+static int i801_suspend(struct pci_dev *dev, pm_message_t state)
+{
+	i801_shutdown(dev);
+	return pci_save_state(dev);
+}
+
+static int i801_resume(struct pci_dev *dev)
+{
+	pci_restore_state(dev);
+	if (isich4) {
+		/* Restore PEC mode */
+		outb_p(saved_auxctl, SMBAUXCTL);
+	}
+	return 0;
+}
+
 static struct pci_driver i801_driver = {
 	.name		= "i801_smbus",
 	.id_table	= i801_ids,
 	.probe		= i801_probe,
 	.remove		= __devexit_p(i801_remove),
+	.suspend	= i801_suspend,
+	.resume		= i801_resume,
+	.shutdown	= i801_shutdown,
 };
 
 static int __init i2c_i801_init(void)


-- 
Jean Delvare

Re: [lm-sensors] [PATCH] i2c-i801: Fix resume when PEC is used

[Mark M. Hoffman]

Hi Jean:

* Jean Delvare <khali@linux-fr.org> [2006-04-18 14:06:29 +0200]:
> Fix for bug #6395:
> Fail to resume on Tecra M2 with ADM1032 and Intel 82801DBM
> 
> The BIOS of the Tecra M2 doesn't like it when it has to reboot or
> resume after the i2c-i801 driver has left the SMBus in PEC mode. So we
> need to add proper .suspend, .resume and .shutdown callbacks to that
> driver, where we clear and restore the PEC mode bit appropriately.

NACK: saved_auxctl is uninitialized in this patch (what happened to
the patch I looked at last night?)

Also, now that I look at it again... wouldn't it be much easier to just
disable PEC again after every transfer?  That's safer too:  the call-
backs might not be enough e.g. if the user hits the reset button after
a PEC transfer.

> Thanks to Daniele Gaffuri for the very good report and contribution.
> 
> Signed-off-by: Jean Delvare <khali@linux-fr.org>
> ---
>  drivers/i2c/busses/i2c-i801.c |   30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> --- linux-2.6.17-rc1.orig/drivers/i2c/busses/i2c-i801.c	2006-04-18 09:34:15.000000000 +0200
> +++ linux-2.6.17-rc1/drivers/i2c/busses/i2c-i801.c	2006-04-18 12:47:07.000000000 +0200
> @@ -110,6 +110,7 @@
>  static struct pci_driver i801_driver;
>  static struct pci_dev *I801_dev;
>  static int isich4;
> +static u8 saved_auxctl;
>  
>  static int i801_setup(struct pci_dev *dev)
>  {
> @@ -551,17 +552,46 @@
>  	return i2c_add_adapter(&i801_adapter);
>  }
>  
> +static void i801_shutdown(struct pci_dev *dev)
> +{
> +	if (isich4) {
> +		/* Disable PEC mode before leaving, else some BIOSes will
> +		   fail to resume/reboot. */
> +		outb_p(0, SMBAUXCTL);
> +	}
> +}
> +
>  static void __devexit i801_remove(struct pci_dev *dev)
>  {
>  	i2c_del_adapter(&i801_adapter);
> +	i801_shutdown(dev);
>  	release_region(i801_smba, (isich4 ? 16 : 8));
>  }
>  
> +static int i801_suspend(struct pci_dev *dev, pm_message_t state)
> +{
> +	i801_shutdown(dev);
> +	return pci_save_state(dev);
> +}
> +
> +static int i801_resume(struct pci_dev *dev)
> +{
> +	pci_restore_state(dev);
> +	if (isich4) {
> +		/* Restore PEC mode */
> +		outb_p(saved_auxctl, SMBAUXCTL);
> +	}
> +	return 0;
> +}
> +
>  static struct pci_driver i801_driver = {
>  	.name		= "i801_smbus",
>  	.id_table	= i801_ids,
>  	.probe		= i801_probe,
>  	.remove		= __devexit_p(i801_remove),
> +	.suspend	= i801_suspend,
> +	.resume		= i801_resume,
> +	.shutdown	= i801_shutdown,
>  };
>  
>  static int __init i2c_i801_init(void)
> 

Regards,

-- 
Mark M. Hoffman
mhoffman@lightlink.com

Re: [PATCH] i2c-i801: Fix resume when PEC is used

[Jean Delvare]

Hi Mark,

> * Jean Delvare <khali@linux-fr.org> [2006-04-18 14:06:29 +0200]:
> > Fix for bug #6395:
> > Fail to resume on Tecra M2 with ADM1032 and Intel 82801DBM
> > 
> > The BIOS of the Tecra M2 doesn't like it when it has to reboot or
> > resume after the i2c-i801 driver has left the SMBus in PEC mode. So we
> > need to add proper .suspend, .resume and .shutdown callbacks to that
> > driver, where we clear and restore the PEC mode bit appropriately.
> 
> NACK: saved_auxctl is uninitialized in this patch (what happened to
> the patch I looked at last night?)

Doh! You're right, and I am so glad you caught it. I tried to refactor
some code from the original patch and one line was lost in the battle :(

> Also, now that I look at it again... wouldn't it be much easier to just
> disable PEC again after every transfer?  That's safer too:  the call-
> backs might not be enough e.g. if the user hits the reset button after
> a PEC transfer.

This is exactly the patch I sent for -stable (as 2.6.16 is affected
too.) This is also what the i2c-i801 driver was doing up to 2.6.15,
inclusive.

However I thought using the proper driver model callbacks would be
better for 2.6.17, for the following reasons:
* Nothing currently prevents the user from suspending or rebooting the
system while there is a transaction in progress. If this happens, the
callbacks are needed to clear and restore the PEC bit; resetting at the
end of the transaction won't work.
* Small performance benefit, although I admit it's a last resort
consideration.

Also note that, even if we disable PEC after each transaction, the user
can still leave the system in a broken state by hitting the reset
button during a transaction. This is less likely to happen than if we
disable PEC in .suspend and .shutdown, but it can still happen. The
only way to fix that is to get the BIOS itself fixed, which is out of
our hands.

Anyway, on second thought I believe you're right, the most simple
approach will be fine for 2.6.17 too. There's little point in trying
to handle suspend/resume if we don't prevent it from happening in the
middle of a transaction. Fixing that is beyond the scope of 2.6.17.

I'll send a new patch soon.

-- 
Jean Delvare

Re: [PATCH] i2c-i801: Fix resume when PEC is used

[Jean Delvare]

Quoting myself: 
> Anyway, on second thought I believe you're right, the most simple
> approach will be fine for 2.6.17 too. There's little point in trying
> to handle suspend/resume if we don't prevent it from happening in the
> middle of a transaction. Fixing that is beyond the scope of 2.6.17.
> 
> I'll send a new patch soon.

Here it is. This must go in 2.6.17.

* * * * *

Fix for bug #6395:
Fail to resume on Tecra M2 with ADM1032 and Intel 82801DBM

The BIOS of the Tecra M2 doesn't like it when it has to reboot or
resume after the i2c-i801 driver has left the SMBus in PEC mode. The
most simple fix is to clear the PEC bit after after every transaction.
That's what this driver was doing up to 2.6.15 (inclusive).

Thanks to Daniele Gaffuri for the very good report.

Signed-off-by: Jean Delvare <khali@linux-fr.org>
---
 drivers/i2c/busses/i2c-i801.c |    5 +++++
 1 file changed, 5 insertions(+)

--- linux-2.6.16.5.orig/drivers/i2c/busses/i2c-i801.c	2006-03-22 17:07:28.000000000 +0100
+++ linux-2.6.16.5/drivers/i2c/busses/i2c-i801.c	2006-04-17 11:11:06.000000000 +0200
@@ -478,6 +478,11 @@
 		ret = i801_transaction();
 	}
 
+	/* Some BIOSes don't like it when PEC is enabled at reboot or resume
+	   time, so we forcibly disable it after every transaction. */
+	if (hwpec)
+		outb_p(0, SMBAUXCTL);
+
 	if(block)
 		return ret;
 	if(ret)



-- 
Jean Delvare

Re: [PATCH] i2c-i801: Fix resume when PEC is used

[Linus Torvalds]

On Wed, 19 Apr 2006, Jean Delvare wrote:
> 
> The BIOS of the Tecra M2 doesn't like it when it has to reboot or
> resume after the i2c-i801 driver has left the SMBus in PEC mode. The
> most simple fix is to clear the PEC bit after after every transaction.

Just wondering.. Wouldn't it make more sense to clear it when closing the 
device or when shutting down, rather than after each transaction?

		Linus

Re: [PATCH] i2c-i801: Fix resume when PEC is used

[Jean Delvare]

Hi Linus,

> On Wed, 19 Apr 2006, Jean Delvare wrote:
> > The BIOS of the Tecra M2 doesn't like it when it has to reboot or
> > resume after the i2c-i801 driver has left the SMBus in PEC mode. The
> > most simple fix is to clear the PEC bit after after every transaction.

The same day, Linus Torvalds replied:
> Just wondering.. Wouldn't it make more sense to clear it when closing the 
> device or when shutting down, rather than after each transaction?

That's what my original patch was doing, but Mark M. Hoffman objected
that it didn't cover the case where the user hits the reset button.

Given that PEC is only rarely used, the slight performance impact isn't
an issue, so I'm fine with both patches.

-- 
Jean Delvare