[PATCH] i2c: i801: Restore INTREN on unload

[PATCH] i2c: i801: Restore INTREN on unload

[Jean Delvare]

If driver interrupts are enabled, SMBHSTCNT_INTREN will be 1 after
the first transaction, and will stay to that value forever. This
means that interrupts will be generated for both host-initiated
transactions and also SMBus Alert events even after the driver is
unloaded. To be on the safe side, we should restore the initial state
of this bit at suspend and reboot time, as we do for several other
configuration bits already and for the same reason: the BIOS should
be handed the device in the same configuration state in which we
received it. Otherwise interrupts may be generated which nobody
will process.

Signed-off-by: Jean Delvare <jdelvare@suse.de>
Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
---
This probably doesn't change much on its own in practice, however it
is mandatory to make this change before Jarkko's fix for the SMB_ALERT
interrupt storm gets applied, otherwise the fix will be incomplete.

Jarkko, this is not exactly the patch you tested, I added restoration
to the suspend path as well to be 100% safe.

 drivers/i2c/busses/i2c-i801.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

--- linux-5.14.orig/drivers/i2c/busses/i2c-i801.c	2021-11-09 15:51:03.186452787 +0100
+++ linux-5.14/drivers/i2c/busses/i2c-i801.c	2021-11-09 15:56:05.081477077 +0100
@@ -258,6 +258,7 @@ struct i801_priv {
 	struct i2c_adapter adapter;
 	unsigned long smba;
 	unsigned char original_hstcfg;
+	unsigned char original_hstcnt;
 	unsigned char original_slvcmd;
 	struct pci_dev *pci_dev;
 	unsigned int features;
@@ -1799,7 +1800,8 @@ static int i801_probe(struct pci_dev *de
 		outb_p(inb_p(SMBAUXCTL(priv)) &
 		       ~(SMBAUXCTL_CRC | SMBAUXCTL_E32B), SMBAUXCTL(priv));
 
-	/* Remember original Host Notify setting */
+	/* Remember original Interrupt and Host Notify settings */
+	priv->original_hstcnt = inb_p(SMBHSTCNT(priv)) & ~SMBHSTCNT_KILL;
 	if (priv->features & FEATURE_HOST_NOTIFY)
 		priv->original_slvcmd = inb_p(SMBSLVCMD(priv));
 
@@ -1863,6 +1865,7 @@ static void i801_remove(struct pci_dev *
 {
 	struct i801_priv *priv = pci_get_drvdata(dev);
 
+	outb_p(priv->original_hstcnt, SMBHSTCNT(priv));
 	i801_disable_host_notify(priv);
 	i801_del_mux(priv);
 	i2c_del_adapter(&priv->adapter);
@@ -1886,6 +1889,7 @@ static void i801_shutdown(struct pci_dev
 	struct i801_priv *priv = pci_get_drvdata(dev);
 
 	/* Restore config registers to avoid hard hang on some systems */
+	outb_p(priv->original_hstcnt, SMBHSTCNT(priv));
 	i801_disable_host_notify(priv);
 	pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
 }
@@ -1895,6 +1899,7 @@ static int i801_suspend(struct device *d
 {
 	struct i801_priv *priv = dev_get_drvdata(dev);
 
+	outb_p(priv->original_hstcnt, SMBHSTCNT(priv));
 	pci_write_config_byte(priv->pci_dev, SMBHSTCFG, priv->original_hstcfg);
 	return 0;
 }

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] i2c: i801: Restore INTREN on unload

[Jarkko Nikula]

On 11/9/21 5:02 PM, Jean Delvare wrote:
> If driver interrupts are enabled, SMBHSTCNT_INTREN will be 1 after
> the first transaction, and will stay to that value forever. This
> means that interrupts will be generated for both host-initiated
> transactions and also SMBus Alert events even after the driver is
> unloaded. To be on the safe side, we should restore the initial state
> of this bit at suspend and reboot time, as we do for several other
> configuration bits already and for the same reason: the BIOS should
> be handed the device in the same configuration state in which we
> received it. Otherwise interrupts may be generated which nobody
> will process.
> 
> Signed-off-by: Jean Delvare <jdelvare@suse.de>
> Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> ---
> This probably doesn't change much on its own in practice, however it
> is mandatory to make this change before Jarkko's fix for the SMB_ALERT
> interrupt storm gets applied, otherwise the fix will be incomplete.
> 
> Jarkko, this is not exactly the patch you tested, I added restoration
> to the suspend path as well to be 100% safe.
> 
Same Tested-by holds here.

We have one laptop with RMI4 SMBus connected touchpad and trackpad. I 
wanted to check it does the suspend path here cease them to not work but 
they work after your patch.

Now we restore INTREN on suspend but do not explicitly enable it after 
resume. I didn't fully get how RMI4 stack resumes but perhaps they will 
do some power on, reset, etc command transaction and that gets the 
INTREN enabled and allow host notify.

Jarkko

Re: [PATCH] i2c: i801: Restore INTREN on unload

[Jean Delvare]

On Wed, 10 Nov 2021 16:31:41 +0200, Jarkko Nikula wrote:
> Now we restore INTREN on suspend but do not explicitly enable it after 
> resume.

We also don't enable it when loading the driver, so this is consistent.

> I didn't fully get how RMI4 stack resumes but perhaps they will 
> do some power on, reset, etc command transaction and that gets the 
> INTREN enabled and allow host notify.

I didn't check either, but it shouldn't matter. If INTREN has to be set
for RMI4 to work, then it must have been set even before the i2c-i801
driver was loaded. In this case, my new code is a no-op (SMBHSTCNT will
be set to the value it already had, that is, INTREN is enabled at all
times).

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] i2c: i801: Restore INTREN on unload

[Wolfram Sang]

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

On Tue, Nov 09, 2021 at 04:02:57PM +0100, Jean Delvare wrote:
> If driver interrupts are enabled, SMBHSTCNT_INTREN will be 1 after
> the first transaction, and will stay to that value forever. This
> means that interrupts will be generated for both host-initiated
> transactions and also SMBus Alert events even after the driver is
> unloaded. To be on the safe side, we should restore the initial state
> of this bit at suspend and reboot time, as we do for several other
> configuration bits already and for the same reason: the BIOS should
> be handed the device in the same configuration state in which we
> received it. Otherwise interrupts may be generated which nobody
> will process.
> 
> Signed-off-by: Jean Delvare <jdelvare@suse.de>
> Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Applied to for-current, thanks!


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