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

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

On 10/27/21 12:28 AM, Jean Delvare wrote:
> Hi Jarkko,
> 
> On Tue, 19 Oct 2021 17:17:00 +0300, Jarkko Nikula wrote:
>> Currently interrupt storm will occur from i2-i801 after first transaction
> 
> Typo: i2c-i801.
> 
Will fix.

>> 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
> 
> The storm only starts with the first transaction, loading the driver
> does not start it, so I can't see the window at load time.
> 
> For driver unload time, maybe we should restore SMBHSTCNT_INTREN to
> its original value at that time, to prevent an interrupt storm from
> happening. If we don't, then I suspect we'll have a storm not only for
> a short time window but actually forever.
> 
I had to hack and retest how I got that load and unload time interrupts 
but managed to do it with a code that doesn't acknowledge SMB_ALERT. It 
wasn't actually straight after boot but hacking with rmmod/modprobe, 
transactions and SMB_ALERT during runtime. Here in simplest form:

0. Boot
1. Do transaction and get the SMBHSTCNT_INTREN enabled
2. SMB_ALERT or rmmod
    - if SMB_ALERT first, then interrupts during this unload
    - if rmmod first, then interrupts during next unload
3. modprobe
    - SMBHSTCNT_INTREN already set
    -> PCI_STATUS_INTERRUPT set, "An interrupt is pending!" followed by 
interrupts between devm_request_irq and i801_enable_host_notify where 
SMB_ALERT disabled by the code change.

>> -	status &= SMBHSTSTS_INTR | STATUS_ERROR_FLAGS;
>> -	if (status) {
>> +	status &= SMBHSTSTS_INTR | STATUS_ERROR_FLAGS | SMBHSTSTS_SMBALERT_STS;
>> +	if (status)
>>   		outb_p(status, SMBHSTSTS(priv));
> 
> Might be worth a comment.
> 
...
>> -	if (!(SMBSLVCMD_HST_NTFY_INTREN & priv->original_slvcmd))
>> -		outb_p(SMBSLVCMD_HST_NTFY_INTREN | priv->original_slvcmd,
>> -		       SMBSLVCMD(priv));
>> +	/* Enable host notify interrupt and disable SMB_ALERT signal */
>> +	outb_p(SMBSLVCMD_HST_NTFY_INTREN | SMBSLVCMD_SMBALERT_DISABLE |
>> +	       priv->original_slvcmd, SMBSLVCMD(priv));
> 
> A more verbose comment would be welcome too. Right now we know why we
> are doing that, but in a few years we won't remember.
> 
Agree to both, will update.

>>   
>>   	/* clear Host Notify bit to allow a new notification */
>>   	outb_p(SMBSLVSTS_HST_NTFY_STS, SMBSLVSTS(priv));
> 
> Tested-by: Jean Delvare <jdelvare@suse.de>
> 
> (But my system was not affected by the issue in the first place, so
> it's only a non-regression test.)
> 
I was fortunate to find a reference design in our lab where SMB_ALERT 
wasn't connected anywhere else than a pull-up and was able to 
relatively safely pull it down by a grounded wire. Just have to be 
careful to not touch anything else with the wire :-)

Jarkko