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@xxxxxxxxxxxxxxx>
