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@xxxxxxxxxxx > Reported-by: stephane.poignant@xxxxxxxxxxxxxx > Signed-off-by: Jarkko Nikula <jarkko.nikula@xxxxxxxxxxxxxxx> > --- > 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@xxxxxxx> (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
