All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] i2c: i801: Fix interrupt storm from SMB_ALERT signal
@ 2021-10-19 14:17 Jarkko Nikula
  2021-10-26 21:28 ` Jean Delvare
  0 siblings, 1 reply; 3+ messages in thread
From: Jarkko Nikula @ 2021-10-19 14:17 UTC (permalink / raw)
  To: linux-i2c
  Cc: Jean Delvare, Wolfram Sang, Andy Shevchenko, Jarkko Nikula,
	ck+kernelbugzilla, stephane.poignant

Currently interrupt storm will occur from i2-i801 after first transaction
if SMB_ALERT signal is enabled and ever asserted, 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.

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>
---
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 | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 115660dce722..e95de4ce6b64 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -190,6 +190,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,9 +643,11 @@ static irqreturn_t i801_isr(int irq, void *dev_id)
 	 * Clear irq sources and report transaction result.
 	 * ->status must be cleared before the next transaction is started.
 	 */
-	status &= SMBHSTSTS_INTR | STATUS_ERROR_FLAGS;
-	if (status) {
+	status &= SMBHSTSTS_INTR | STATUS_ERROR_FLAGS | SMBHSTSTS_SMBALERT_STS;
+	if (status)
 		outb_p(status, SMBHSTSTS(priv));
+	status &= ~SMBHSTSTS_SMBALERT_STS;
+	if (status) {
 		priv->status = status;
 		complete(&priv->done);
 	}
@@ -972,9 +975,9 @@ 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 disable SMB_ALERT signal */
+	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


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH] i2c: i801: Fix interrupt storm from SMB_ALERT signal
  2021-10-19 14:17 [PATCH] i2c: i801: Fix interrupt storm from SMB_ALERT signal Jarkko Nikula
@ 2021-10-26 21:28 ` Jean Delvare
  2021-10-27 13:31   ` Jarkko Nikula
  0 siblings, 1 reply; 3+ messages in thread
From: Jean Delvare @ 2021-10-26 21:28 UTC (permalink / raw)
  To: Jarkko Nikula
  Cc: linux-i2c, Wolfram Sang, Andy Shevchenko, ck+kernelbugzilla,
	stephane.poignant

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.

> if SMB_ALERT signal is enabled and ever asserted, even before the driver
> is loaded and does not recover because that interrupt is not
> acknowledged.

Thank you very much for looking into this old bug and finally figuring
it out. Great job!

So basically the interrupt storm begins as soon as SMBHSTCNT_INTREN is
set, because that bit controls both regular SMBus transactions and
SMBALERT# (but not Host Notify which has its own interrupt enable
control bit).

> 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.
> 
> 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>
> ---
> 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.

> interrupt disabling is possible only on ICH3 and later so interrupt
> acknowledging should also help those old platforms.

I'd be very surprised if systems with pre-ICH3 chipsets are still up
and running. Even the ICH3 is 20 year old by now. So I wouldn't bother
with them.

> ---
>  drivers/i2c/busses/i2c-i801.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 115660dce722..e95de4ce6b64 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -190,6 +190,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,9 +643,11 @@ static irqreturn_t i801_isr(int irq, void *dev_id)
>  	 * Clear irq sources and report transaction result.
>  	 * ->status must be cleared before the next transaction is started.
>  	 */
> -	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.

> +	status &= ~SMBHSTSTS_SMBALERT_STS;
> +	if (status) {
>  		priv->status = status;
>  		complete(&priv->done);
>  	}
> @@ -972,9 +975,9 @@ 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 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.

>  
>  	/* 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'm also wondering whether it would make sense to actually support
SMBus Alert in the driver. We have core support for it already. If
there are systems out there which generate such events, then maybe
there's some value to be added. But we'd need to know which devices are
sending them. For the time being I agree that just fixing the interrupt
storms is more important.

-- 
Jean Delvare
SUSE L3 Support

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] i2c: i801: Fix interrupt storm from SMB_ALERT signal
  2021-10-26 21:28 ` Jean Delvare
@ 2021-10-27 13:31   ` Jarkko Nikula
  0 siblings, 0 replies; 3+ messages in thread
From: Jarkko Nikula @ 2021-10-27 13:31 UTC (permalink / raw)
  To: Jean Delvare
  Cc: linux-i2c, Wolfram Sang, Andy Shevchenko, ck+kernelbugzilla,
	stephane.poignant

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-10-27 13:31 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-19 14:17 [PATCH] i2c: i801: Fix interrupt storm from SMB_ALERT signal Jarkko Nikula
2021-10-26 21:28 ` Jean Delvare
2021-10-27 13:31   ` Jarkko Nikula

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.