Re: [PATCH v2] i2c: i801: Fix interrupt storm from SMB_ALERT signal

[Jarkko Nikula <jarkko.nikula@linux.intel.com>]

On 10/29/21 5:25 PM, Jean Delvare wrote:
> In my first review, I suggested restoring SMBHSTCNT_INTREN to its
> original value at driver unload time. I stand on this position. If
> SMBHSTCNT_INTREN was originally 0, it will be 1 after the first
> transaction run by the driver. That's fine as long as
> SMBSLVCMD_SMBALERT_DISABLE is set, however i801_disable_host_notify()
> will clear that bit when you unload the driver. At that point, SMBus
> Alerts will keep generating interrupts. I'm not sure what happens if
> nobody is listening to that interrupt. But in case of a shared
> interrupt, the other driver is going to be flooded with interrupts
> forever if SMBus Alerts are being generated for whatever reason.
> 
Ah, yeah and it's really happening. I wanted to debug it by installing a 
shared dummy handler in another driver :-)

> So I suggest something like:
> 
> --- linux-5.14.orig/drivers/i2c/busses/i2c-i801.c	2021-10-29 11:15:48.283807055 +0200
> +++ linux-5.14/drivers/i2c/busses/i2c-i801.c	2021-10-29 16:21:32.392978256 +0200
> @@ -259,6 +259,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;
> @@ -1811,7 +1812,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));
>   
> @@ -1875,6 +1877,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);
> @@ -1898,6 +1901,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);
>   }
> 
> In addition to your changes. What do you think?
> 
I think this is worth of a separate patch before mine since it does one 
driver improvement and helps to stop interrupt flood coming from the 
SMBus controller by unloading the driver.

You may add my Tested-by if you plan to send a patch or I can add commit 
log from you and send both patches together.

Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>