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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Wed, Nov 10, 2021 at 04:10:32PM +0200, Jarkko Nikula wrote:
> Currently interrupt storm will occur from i2c-i801 after first
> transaction if SMB_ALERT signal is enabled and ever asserted. It is
> enough if the signal is asserted once even before the driver is loaded
> and does not recover because that interrupt is not acknowledged.
> 
> This fix aims to fix it by two ways:
> - Add acknowledging for the SMB_ALERT interrupt status
> - Disable the SMB_ALERT interrupt on platforms where possible since the
>   driver currently does not make use for it
> 
> Acknowledging resets the SMB_ALERT interrupt status on all platforms and
> also should help to avoid interrupt storm on older platforms where the
> SMB_ALERT interrupt disabling is not available.
> 
> For simplicity this fix reuses the host notify feature for disabling and
> restoring original register value.

With proposed modification (to the text) below, FWIW,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Link: https://bugzilla.kernel.org/show_bug.cgi?id=177311
> Reported-by: ck+kernelbugzilla@bl4ckb0x.de
> Reported-by: stephane.poignant@protonmail.com
> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> ---
> v3: Add comment from Jean Delvare why SMB_ALERT interrupt is blocked for
>     now. Simplify comment about SMB_ALERT status clearing
> v2: More verbose commenting around interrupt acknowledging, reporting
>     and SMB_ALERT disabling. Fix typo in commit log and split the first
>     sentence.
> v1:
> Hi Conrad and Stephane. This patch is otherwise the same than the one I
> had in bugzilla but this adds also acknowledging for the SMB_ALERT
> interrupt. There is short time window during driver load and unload
> where interrupt storm will still occur if signal was asserted. Also
> interrupt disabling is possible only on ICH3 and later so interrupt
> acknowledging should also help those old platforms.
> ---
>  drivers/i2c/busses/i2c-i801.c | 25 +++++++++++++++++++------
>  1 file changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index ed271274250b..21be28b7955f 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -191,6 +191,7 @@
>  #define SMBSLVSTS_HST_NTFY_STS	BIT(0)
>  
>  /* Host Notify Command register bits */
> +#define SMBSLVCMD_SMBALERT_DISABLE	BIT(2)
>  #define SMBSLVCMD_HST_NTFY_INTREN	BIT(0)
>  
>  #define STATUS_ERROR_FLAGS	(SMBHSTSTS_FAILED | SMBHSTSTS_BUS_ERR | \
> @@ -642,12 +643,20 @@ static irqreturn_t i801_isr(int irq, void *dev_id)
>  		i801_isr_byte_done(priv);
>  
>  	/*
> -	 * Clear irq sources and report transaction result.
> +	 * Clear remaining irq sources: Completion of last command, errors

irq --> IRQ
of last --> of the last

> +	 * and the SMB_ALERT signal. SMB_ALERT status is set after signal
> +	 * assertion independently is the interrupt generation blocked or not

is --> if ?

> +	 * so clear it always when the status is set.
> +	 */
> +	status &= SMBHSTSTS_INTR | STATUS_ERROR_FLAGS | SMBHSTSTS_SMBALERT_STS;
> +	if (status)
> +		outb_p(status, SMBHSTSTS(priv));
> +	status &= ~SMBHSTSTS_SMBALERT_STS; /* SMB_ALERT not reported */
> +	/*
> +	 * Report transaction result.
>  	 * ->status must be cleared before the next transaction is started.
>  	 */
> -	status &= SMBHSTSTS_INTR | STATUS_ERROR_FLAGS;
>  	if (status) {
> -		outb_p(status, SMBHSTSTS(priv));
>  		priv->status = status;
>  		complete(&priv->done);
>  	}
> @@ -975,9 +984,13 @@ static void i801_enable_host_notify(struct i2c_adapter *adapter)
>  	if (!(priv->features & FEATURE_HOST_NOTIFY))
>  		return;
>  
> -	if (!(SMBSLVCMD_HST_NTFY_INTREN & priv->original_slvcmd))
> -		outb_p(SMBSLVCMD_HST_NTFY_INTREN | priv->original_slvcmd,
> -		       SMBSLVCMD(priv));
> +	/*
> +	 * Enable host notify interrupt and block the generation of interrupt
> +	 * from the SMB_ALERT signal because the driver does not support
> +	 * SMBus Alert yet.

does not support ... yet -->
  has not supported ... yet
  does not support

?

> +	 */
> +	outb_p(SMBSLVCMD_HST_NTFY_INTREN | SMBSLVCMD_SMBALERT_DISABLE |
> +	       priv->original_slvcmd, SMBSLVCMD(priv));
>  
>  	/* clear Host Notify bit to allow a new notification */
>  	outb_p(SMBSLVSTS_HST_NTFY_STS, SMBSLVSTS(priv));
> -- 
> 2.33.0
> 

-- 
With Best Regards,
Andy Shevchenko