i801_smbus: clear SMBALERT status bit and disable SMBALERT interrupt

From: Lingyan Xu <lingyan.xu@nokia-sbell.com>

In current i801 driver, SMBALERT interrupt is allowed
(Slave Command Register bit2 is 0).
But these is no handler for SMBALERT interrupt in i801_isr,
if there is SMBALERT interrupt asserted and deasserted,
i801 will have an irq flood for the related status bit is setted.

So SMBALERT interrupt handler is needed, and also, SMBALERT interrupt
will be generated from time to time if slave chip have some fault.
So disable SMBALERT interrupt is also needed.

About the solution,
please see http://www.farnell.com/datasheets/1581967.pdf
Page632 P640 for more.

Signed-off-by: Lingyan Xu <lingyan.xu@nokia-sbell.com>
---
 drivers/i2c/busses/i2c-i801.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

On Mon, Aug 12, 2019 at 10:40:34AM +0800, lingyxu wrote:
> From: Lingyan Xu <lingyan.xu@nokia-sbell.com>
> 
> In current i801 driver, SMBALERT interrupt is allowed
> (Slave Command Register bit2 is 0).
> But these is no handler for SMBALERT interrupt in i801_isr,
> if there is SMBALERT interrupt asserted and deasserted,
> i801 will have an irq flood for the related status bit is setted.
> 
> So SMBALERT interrupt handler is needed, and also, SMBALERT interrupt
> will be generated from time to time if slave chip have some fault.
> So disable SMBALERT interrupt is also needed.
> 
> About the solution,
> please see http://www.farnell.com/datasheets/1581967.pdf
> Page632 P640 for more.
> 
> Signed-off-by: Lingyan Xu <lingyan.xu@nokia-sbell.com>

Jean, this seems important if it fixes an interrupt flood. Can you
review, please?

> ---
>  drivers/i2c/busses/i2c-i801.c |    7 ++++++-
>  1 files changed, 6 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index f295693..033bafe 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -661,9 +661,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.
>  	 */
> +
> +	outb_p(status, SMBHSTSTS(priv));
> +
>  	status &= SMBHSTSTS_INTR | STATUS_ERROR_FLAGS;
>  	if (status) {
> -		outb_p(status, SMBHSTSTS(priv));
>  		priv->status = status;
>  		wake_up(&priv->waitq);
>  	}
> @@ -1810,6 +1812,9 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  	/* Default timeout in interrupt mode: 200 ms */
>  	priv->adapter.timeout = HZ / 5;
>  
> +	/* Disable SMBALERT interrupt */
> +	outb_p(inb_p(SMBSLVCMD(priv)) | BIT(2), SMBSLVCMD(priv));
> +
>  	if (dev->irq == IRQ_NOTCONNECTED)
>  		priv->features &= ~FEATURE_IRQ;
>  
> -- 
> 1.7.1
>

Hi Lingyan,

On Mon, 12 Aug 2019 10:40:34 +0800, lingyxu wrote:
> From: Lingyan Xu <lingyan.xu@nokia-sbell.com>
> 
> In current i801 driver, SMBALERT interrupt is allowed
> (Slave Command Register bit2 is 0).
> But these is no handler for SMBALERT interrupt in i801_isr,
> if there is SMBALERT interrupt asserted and deasserted,
> i801 will have an irq flood for the related status bit is setted.
> 
> So SMBALERT interrupt handler is needed, and also, SMBALERT interrupt
> will be generated from time to time if slave chip have some fault.
> So disable SMBALERT interrupt is also needed.
> 
> About the solution,
> please see http://www.farnell.com/datasheets/1581967.pdf
> Page632 P640 for more.
> 
> Signed-off-by: Lingyan Xu <lingyan.xu@nokia-sbell.com>
> ---
>  drivers/i2c/busses/i2c-i801.c |    7 ++++++-
>  1 files changed, 6 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index f295693..033bafe 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -661,9 +661,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.
>  	 */
> +
> +	outb_p(status, SMBHSTSTS(priv));
> +
>  	status &= SMBHSTSTS_INTR | STATUS_ERROR_FLAGS;
>  	if (status) {
> -		outb_p(status, SMBHSTSTS(priv));
>  		priv->status = status;
>  		wake_up(&priv->waitq);
>  	}

Looks scary. Writing the whole value of SMBHSTSTS back to itself
without selecting which bits you write is dangerous. Specifically,
writing back SMBHSTSTS_BYTE_DONE, SMBHSTSTS_INUSE_STS and
SMBHSTSTS_HOST_BUSY could have unexpected consequences. I would feel
much better if you would just explicitly add SMBHSTSTS_SMBALERT_STS to
the list.

> @@ -1810,6 +1812,9 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  	/* Default timeout in interrupt mode: 200 ms */
>  	priv->adapter.timeout = HZ / 5;
>  
> +	/* Disable SMBALERT interrupt */
> +	outb_p(inb_p(SMBSLVCMD(priv)) | BIT(2), SMBSLVCMD(priv));

Please give SMBSLVCMD's BIT(2) a name and define it after
SMBSLVCMD_HST_NTFY_INTREN.

Also it is mandatory to restore the value of SMBSLVCMD before returning
the control back to the BIOS. Currently this is only being done when
the FEATURE_HOST_NOTIFY bit is set because that's the only case where
we change the value of that register, but if we change it
unconditionally then it must be saved and restored unconditionally too.

> +
>  	if (dev->irq == IRQ_NOTCONNECTED)
>  		priv->features &= ~FEATURE_IRQ;
>  

That being said, if you see this interrupt flood, it means that at
least one device on your SMBus would benefit from SMBus Alert being
supported. The infrastructure is already there as we added support in a
few I2C bus drivers already. So maybe instead of silencing the
interrupts, we could add proper SMBus Alert support to the i2c-i801
driver?

Did you figure out which device is raising the SMBus Alert and why?

Hello Jean,

On 28/08/2019 15:58, Jean Delvare wrote:
> Also it is mandatory to restore the value of SMBSLVCMD before returning
> the control back to the BIOS. Currently this is only being done when
> the FEATURE_HOST_NOTIFY bit is set because that's the only case where
> we change the value of that register, but if we change it
> unconditionally then it must be saved and restored unconditionally too.

could you please tell a bit more about the use case, where/how exactly one
can "return control back to the BIOS"? Maybe referencing the functions in
the driver...

On Fri, 30 Aug 2019 11:35:46 +0000, Sverdlin, Alexander (Nokia - DE/Ulm) wrote:
> Hello Jean,
> 
> On 28/08/2019 15:58, Jean Delvare wrote:
> > Also it is mandatory to restore the value of SMBSLVCMD before returning
> > the control back to the BIOS. Currently this is only being done when
> > the FEATURE_HOST_NOTIFY bit is set because that's the only case where
> > we change the value of that register, but if we change it
> > unconditionally then it must be saved and restored unconditionally too.  
> 
> could you please tell a bit more about the use case, where/how exactly one
> can "return control back to the BIOS"? Maybe referencing the functions in
> the driver...

i801_remove() and i801_shutdown(). Basically when you reboot the
system. Possibly also on suspend-to-disk in certain cases, I'm not
sure. In general shutdown + cold boot is OK either way because on cold
boot the chipsets gets hard-reset anyway.

Hi Jean,
Thanks a lot for your comments. And, yes, it is dangerous that clear all interrupt bit here based my local test. And about the interrupt flood, I will show you in attached file. And I agree with you that add SMBALERT interrupt handler if possible, but I have no idea about what action is need in this handler because that it seams that i801 can not clear salve chip's status bit directly.


Best Regards!
Lingyan xu

-----Original Message-----
From: Jean Delvare <jdelvare@suse.de> 
Sent: 2019年8月28日 21:58
To: Xu, Lingyan (NSB - CN/Hangzhou) <lingyan.xu@nokia-sbell.com>
Cc: Adamski, Krzysztof (Nokia - PL/Wroclaw) <krzysztof.adamski@nokia.com>; Wiebe, Wladislav (Nokia - DE/Ulm) <wladislav.wiebe@nokia.com>; linux-i2c@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH] i801_smbus: clear SMBALERT status bit and disable SMBALERT interrupt

Hi Lingyan,

On Mon, 12 Aug 2019 10:40:34 +0800, lingyxu wrote:
> From: Lingyan Xu <lingyan.xu@nokia-sbell.com>
> 
> In current i801 driver, SMBALERT interrupt is allowed (Slave Command 
> Register bit2 is 0).
> But these is no handler for SMBALERT interrupt in i801_isr, if there 
> is SMBALERT interrupt asserted and deasserted,
> i801 will have an irq flood for the related status bit is setted.
> 
> So SMBALERT interrupt handler is needed, and also, SMBALERT interrupt 
> will be generated from time to time if slave chip have some fault.
> So disable SMBALERT interrupt is also needed.
> 
> About the solution,
> please see http://www.farnell.com/datasheets/1581967.pdf
> Page632 P640 for more.
> 
> Signed-off-by: Lingyan Xu <lingyan.xu@nokia-sbell.com>
> ---
>  drivers/i2c/busses/i2c-i801.c |    7 ++++++-
>  1 files changed, 6 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c 
> b/drivers/i2c/busses/i2c-i801.c index f295693..033bafe 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -661,9 +661,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.
>  	 */
> +
> +	outb_p(status, SMBHSTSTS(priv));
> +
>  	status &= SMBHSTSTS_INTR | STATUS_ERROR_FLAGS;
>  	if (status) {
> -		outb_p(status, SMBHSTSTS(priv));
>  		priv->status = status;
>  		wake_up(&priv->waitq);
>  	}

Looks scary. Writing the whole value of SMBHSTSTS back to itself without selecting which bits you write is dangerous. Specifically, writing back SMBHSTSTS_BYTE_DONE, SMBHSTSTS_INUSE_STS and SMBHSTSTS_HOST_BUSY could have unexpected consequences. I would feel much better if you would just explicitly add SMBHSTSTS_SMBALERT_STS to the list.

> @@ -1810,6 +1812,9 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  	/* Default timeout in interrupt mode: 200 ms */
>  	priv->adapter.timeout = HZ / 5;
>  
> +	/* Disable SMBALERT interrupt */
> +	outb_p(inb_p(SMBSLVCMD(priv)) | BIT(2), SMBSLVCMD(priv));

Please give SMBSLVCMD's BIT(2) a name and define it after SMBSLVCMD_HST_NTFY_INTREN.

Also it is mandatory to restore the value of SMBSLVCMD before returning the control back to the BIOS. Currently this is only being done when the FEATURE_HOST_NOTIFY bit is set because that's the only case where we change the value of that register, but if we change it unconditionally then it must be saved and restored unconditionally too.

> +
>  	if (dev->irq == IRQ_NOTCONNECTED)
>  		priv->features &= ~FEATURE_IRQ;
>  

That being said, if you see this interrupt flood, it means that at least one device on your SMBus would benefit from SMBus Alert being supported. The infrastructure is already there as we added support in a few I2C bus drivers already. So maybe instead of silencing the interrupts, we could add proper SMBus Alert support to the i2c-i801 driver?

Did you figure out which device is raising the SMBus Alert and why?

--
Jean Delvare
SUSE L3 Support

Hi Jean,
After our local test, the action clear SMBALERT status bit is needed only here for the HW interrupt will be cleared by slave chip.
So I change the patch as follow.

From: Lingyan Xu <lingyan.xu@nokia-sbell.com>

In current i801 driver, SMBALERT interrupt is allowed (Slave Command Register bit2 is 0).
But these is no handler for SMBALERT interrupt in i801_isr, if there is SMBALERT interrupt asserted and deasserted,
i801 will have an irq flood for the related status bit is setted.

So SMBALERT status clear is needed.

About the solution,
please see http://www.farnell.com/datasheets/1581967.pdf
Page632 P640 for more.

Signed-off-by: Lingyan Xu <lingyan.xu@nokia-sbell.com>
---
 drivers/i2c/busses/i2c-i801.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c index f295693..29c90f0 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -661,6 +661,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.
 	 */
+	
+	if (status & SMBHSTSTS_SMBALERT_STS) {
+		outb_p(status & SMBHSTSTS_SMBALERT_STS, SMBHSTSTS(priv));
+	}
+
 	status &= SMBHSTSTS_INTR | STATUS_ERROR_FLAGS;
 	if (status) {
 		outb_p(status, SMBHSTSTS(priv));
--
2.6.2

Best Regards!
Lingyan xu

-----Original Message-----
From: Xu, Lingyan (NSB - CN/Hangzhou) 
Sent: 2019年9月3日 10:16
To: 'Jean Delvare' <jdelvare@suse.de>
Cc: Adamski, Krzysztof (Nokia - PL/Wroclaw) <krzysztof.adamski@nokia.com>; Wiebe, Wladislav (Nokia - DE/Ulm) <wladislav.wiebe@nokia.com>; linux-i2c@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: RE: [PATCH] i801_smbus: clear SMBALERT status bit and disable SMBALERT interrupt

Hi Jean,
Thanks a lot for your comments. And, yes, it is dangerous that clear all interrupt bit here based my local test. And about the interrupt flood, I will show you in attached file. And I agree with you that add SMBALERT interrupt handler if possible, but I have no idea about what action is need in this handler because that it seams that i801 can not clear salve chip's status bit directly.


Best Regards!
Lingyan xu

-----Original Message-----
From: Jean Delvare <jdelvare@suse.de>
Sent: 2019年8月28日 21:58
To: Xu, Lingyan (NSB - CN/Hangzhou) <lingyan.xu@nokia-sbell.com>
Cc: Adamski, Krzysztof (Nokia - PL/Wroclaw) <krzysztof.adamski@nokia.com>; Wiebe, Wladislav (Nokia - DE/Ulm) <wladislav.wiebe@nokia.com>; linux-i2c@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH] i801_smbus: clear SMBALERT status bit and disable SMBALERT interrupt

Hi Lingyan,

On Mon, 12 Aug 2019 10:40:34 +0800, lingyxu wrote:
> From: Lingyan Xu <lingyan.xu@nokia-sbell.com>
> 
> In current i801 driver, SMBALERT interrupt is allowed (Slave Command 
> Register bit2 is 0).
> But these is no handler for SMBALERT interrupt in i801_isr, if there 
> is SMBALERT interrupt asserted and deasserted,
> i801 will have an irq flood for the related status bit is setted.
> 
> So SMBALERT interrupt handler is needed, and also, SMBALERT interrupt 
> will be generated from time to time if slave chip have some fault.
> So disable SMBALERT interrupt is also needed.
> 
> About the solution,
> please see http://www.farnell.com/datasheets/1581967.pdf
> Page632 P640 for more.
> 
> Signed-off-by: Lingyan Xu <lingyan.xu@nokia-sbell.com>
> ---
>  drivers/i2c/busses/i2c-i801.c |    7 ++++++-
>  1 files changed, 6 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c 
> b/drivers/i2c/busses/i2c-i801.c index f295693..033bafe 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -661,9 +661,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.
>  	 */
> +
> +	outb_p(status, SMBHSTSTS(priv));
> +
>  	status &= SMBHSTSTS_INTR | STATUS_ERROR_FLAGS;
>  	if (status) {
> -		outb_p(status, SMBHSTSTS(priv));
>  		priv->status = status;
>  		wake_up(&priv->waitq);
>  	}

Looks scary. Writing the whole value of SMBHSTSTS back to itself without selecting which bits you write is dangerous. Specifically, writing back SMBHSTSTS_BYTE_DONE, SMBHSTSTS_INUSE_STS and SMBHSTSTS_HOST_BUSY could have unexpected consequences. I would feel much better if you would just explicitly add SMBHSTSTS_SMBALERT_STS to the list.

> @@ -1810,6 +1812,9 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  	/* Default timeout in interrupt mode: 200 ms */
>  	priv->adapter.timeout = HZ / 5;
>  
> +	/* Disable SMBALERT interrupt */
> +	outb_p(inb_p(SMBSLVCMD(priv)) | BIT(2), SMBSLVCMD(priv));

Please give SMBSLVCMD's BIT(2) a name and define it after SMBSLVCMD_HST_NTFY_INTREN.

Also it is mandatory to restore the value of SMBSLVCMD before returning the control back to the BIOS. Currently this is only being done when the FEATURE_HOST_NOTIFY bit is set because that's the only case where we change the value of that register, but if we change it unconditionally then it must be saved and restored unconditionally too.

> +
>  	if (dev->irq == IRQ_NOTCONNECTED)
>  		priv->features &= ~FEATURE_IRQ;
>  

That being said, if you see this interrupt flood, it means that at least one device on your SMBus would benefit from SMBus Alert being supported. The infrastructure is already there as we added support in a few I2C bus drivers already. So maybe instead of silencing the interrupts, we could add proper SMBus Alert support to the i2c-i801 driver?

Did you figure out which device is raising the SMBus Alert and why?

--
Jean Delvare
SUSE L3 Support

On Tue, 3 Sep 2019 02:15:52 +0000, Xu, Lingyan (NSB - CN/Hangzhou) wrote:
> Thanks a lot for your comments. And, yes, it is dangerous that clear all interrupt bit here based my local test. And about the interrupt flood, I will show you in attached file. And I agree with you that add SMBALERT interrupt handler if possible, but I have no idea about what action is need in this handler because that it seams that i801 can not clear salve chip's status bit directly.

The whole idea of SMBus alert is that the slave lets the master know
that it needs attention. The master's driver should then let the
slave's driver know that its baby is crying for attention, and it is
the slave driver's job to figure out what the exact problem is. Struct
i2c_driver has an "alert" field (callback function) for that purpose.
For a real example of how this can be implemented, see
drivers/i2c/busses/i2c-parport.c for the master side and
drivers/hwmon/lm90.c for the slave side. These are the 2 drivers I used
to initially add the functionality to the kernel.

Now the question is, in your system, which slave device pulls the alert?

If this is of any value to you, I tried implementing it in i2c-i801 a
few days ago. I can't really test it though as I don't have any device
which triggers an alert on my system, but I am sharing it with you if
you want to give it a try. You would still need to write the code in
the slave driver.

---
 drivers/i2c/busses/i2c-i801.c |   77 +++++++++++++++++++++++++++++++++++++-----
 1 file changed, 69 insertions(+), 8 deletions(-)

--- linux-5.2.orig/drivers/i2c/busses/i2c-i801.c	2019-08-28 15:58:52.725828215 +0200
+++ linux-5.2/drivers/i2c/busses/i2c-i801.c	2019-08-28 16:50:09.212696037 +0200
@@ -196,6 +196,7 @@
 
 /* Host Notify Command register bits */
 #define SMBSLVCMD_HST_NTFY_INTREN	BIT(0)
+#define SMBSLVCMD_SMBALERT_DIS		BIT(2)
 
 #define STATUS_ERROR_FLAGS	(SMBHSTSTS_FAILED | SMBHSTSTS_BUS_ERR | \
 				 SMBHSTSTS_DEV_ERR)
@@ -281,6 +282,10 @@ struct i801_priv {
 	 */
 	bool acpi_reserved;
 	struct mutex acpi_lock;
+
+	/* SMBus alert */
+	struct i2c_smbus_alert_setup alert_data;
+	struct i2c_client *ara;
 };
 
 #define FEATURE_SMBUS_PEC	BIT(0)
@@ -289,6 +294,7 @@ struct i801_priv {
 #define FEATURE_I2C_BLOCK_READ	BIT(3)
 #define FEATURE_IRQ		BIT(4)
 #define FEATURE_HOST_NOTIFY	BIT(5)
+#define FEATURE_SMBUS_ALERT	BIT(6)
 /* Not really a feature, but it's convenient to handle it as such */
 #define FEATURE_IDF		BIT(15)
 #define FEATURE_TCO_SPT		BIT(16)
@@ -301,6 +307,7 @@ static const char *i801_feature_names[]
 	"I2C block read",
 	"Interrupt",
 	"SMBus Host Notify",
+	"SMBus Alert",
 };
 
 static unsigned int disable_features;
@@ -310,7 +317,8 @@ MODULE_PARM_DESC(disable_features, "Disa
 	"\t\t  0x02  disable the block buffer\n"
 	"\t\t  0x08  disable the I2C block read functionality\n"
 	"\t\t  0x10  don't use interrupts\n"
-	"\t\t  0x20  disable SMBus Host Notify ");
+	"\t\t  0x20  disable SMBus Host Notify\n"
+	"\t\t  0x40  disable SMBus Alert ");
 
 /* Make sure the SMBus host is ready to start transmitting.
    Return 0 if it is, -EBUSY if it is not. */
@@ -620,8 +628,24 @@ static irqreturn_t i801_host_notify_isr(
 	return IRQ_HANDLED;
 }
 
+static irqreturn_t i801_smbus_alert_isr(struct i801_priv *priv)
+{
+	struct i2c_client *ara = priv->ara;
+
+	if (ara) {
+		dev_dbg(&ara->dev, "SMBus alert received\n");
+		i2c_handle_smbus_alert(ara);
+	} else
+		dev_dbg(&priv->adapter.dev,
+			"SMBus alert received but no ARA client!\n");
+
+	/* clear SMBus Alert bit and return */
+	outb_p(SMBHSTSTS_SMBALERT_STS, SMBHSTSTS(priv));
+	return IRQ_HANDLED;
+}
+
 /*
- * There are three kinds of interrupts:
+ * There are four kinds of interrupts:
  *
  * 1) i801 signals transaction completion with one of these interrupts:
  *      INTR - Success
@@ -635,6 +659,8 @@ static irqreturn_t i801_host_notify_isr(
  *    occurs for each byte of a byte-by-byte to prepare the next byte.
  *
  * 3) Host Notify interrupts
+ *
+ * 4) SMBus Alert interrupts
  */
 static irqreturn_t i801_isr(int irq, void *dev_id)
 {
@@ -653,6 +679,12 @@ static irqreturn_t i801_isr(int irq, voi
 			return i801_host_notify_isr(priv);
 	}
 
+	if (priv->features & FEATURE_SMBUS_ALERT) {
+		status = inb_p(SMBHSTSTS(priv));
+		if (status & SMBHSTSTS_SMBALERT_STS)
+			return i801_smbus_alert_isr(priv);
+	}
+
 	status = inb_p(SMBHSTSTS(priv));
 	if (status & SMBHSTSTS_BYTE_DONE)
 		i801_isr_byte_done(priv);
@@ -1006,9 +1038,35 @@ static void i801_enable_host_notify(stru
 	outb_p(SMBSLVSTS_HST_NTFY_STS, SMBSLVSTS(priv));
 }
 
-static void i801_disable_host_notify(struct i801_priv *priv)
+static void i801_enable_smbus_alert(struct i2c_adapter *adapter)
 {
-	if (!(priv->features & FEATURE_HOST_NOTIFY))
+	struct i801_priv *priv = i2c_get_adapdata(adapter);
+
+	if (!(priv->features & FEATURE_SMBUS_ALERT))
+		return;
+
+	priv->ara = i2c_setup_smbus_alert(adapter, &priv->alert_data);
+	if (!priv->ara) {
+		dev_warn(&adapter->dev, "Failed to register ARA client\n");
+
+		/* Disable SMBus Alert interrupts */
+		if (!(SMBSLVCMD_SMBALERT_DIS & priv->original_slvcmd))
+			outb_p(SMBSLVCMD_SMBALERT_DIS | priv->original_slvcmd,
+			       SMBSLVCMD(priv));
+		return;
+	}
+
+	if (SMBSLVCMD_SMBALERT_DIS & priv->original_slvcmd)
+		outb_p(~SMBSLVCMD_SMBALERT_DIS & priv->original_slvcmd,
+		       SMBSLVCMD(priv));
+
+	/* Clear SMBus Alert bit to allow a new notification */
+	outb_p(SMBHSTSTS_SMBALERT_STS, SMBHSTSTS(priv));
+}
+
+static void i801_restore_slvcmd(struct i801_priv *priv)
+{
+	if (!(priv->features & (FEATURE_HOST_NOTIFY | FEATURE_SMBUS_ALERT)))
 		return;
 
 	outb_p(priv->original_slvcmd, SMBSLVCMD(priv));
@@ -1823,8 +1881,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 */
-	if (priv->features & FEATURE_HOST_NOTIFY)
+	/* Remember original Host Notify and SMBus Alert setting */
+	if (priv->features & (FEATURE_HOST_NOTIFY | FEATURE_SMBUS_ALERT))
 		priv->original_slvcmd = inb_p(SMBSLVCMD(priv));
 
 	/* Default timeout in interrupt mode: 200 ms */
@@ -1875,6 +1933,7 @@ static int i801_probe(struct pci_dev *de
 	}
 
 	i801_enable_host_notify(&priv->adapter);
+	i801_enable_smbus_alert(&priv->adapter);
 
 	i801_probe_optional_slaves(priv);
 	/* We ignore errors - multiplexing is optional */
@@ -1897,8 +1956,10 @@ static void i801_remove(struct pci_dev *
 	pm_runtime_forbid(&dev->dev);
 	pm_runtime_get_noresume(&dev->dev);
 
-	i801_disable_host_notify(priv);
+	i801_restore_slvcmd(priv);
 	i801_del_mux(priv);
+	if (priv->ara)
+		i2c_unregister_device(priv->ara);
 	i2c_del_adapter(&priv->adapter);
 	i801_acpi_remove(priv);
 	pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
@@ -1916,7 +1977,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 */
-	i801_disable_host_notify(priv);
+	i801_restore_slvcmd(priv);
 	pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
 }

Hello Jean,

On 05/09/2019 14:57, Jean Delvare wrote:
> If this is of any value to you, I tried implementing it in i2c-i801 a
> few days ago. I can't really test it though as I don't have any device
> which triggers an alert on my system, but I am sharing it with you if
> you want to give it a try. You would still need to write the code in
> the slave driver.

I'm going to test your patch (I actually made very similar one couple of days
ago), but I have a couple of comments to the below version:

> ---
>  drivers/i2c/busses/i2c-i801.c |   77 +++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 69 insertions(+), 8 deletions(-)
> 
> --- linux-5.2.orig/drivers/i2c/busses/i2c-i801.c	2019-08-28 15:58:52.725828215 +0200
> +++ linux-5.2/drivers/i2c/busses/i2c-i801.c	2019-08-28 16:50:09.212696037 +0200
> @@ -196,6 +196,7 @@
>  
>  /* Host Notify Command register bits */
>  #define SMBSLVCMD_HST_NTFY_INTREN	BIT(0)
> +#define SMBSLVCMD_SMBALERT_DIS		BIT(2)
>  
>  #define STATUS_ERROR_FLAGS	(SMBHSTSTS_FAILED | SMBHSTSTS_BUS_ERR | \
>  				 SMBHSTSTS_DEV_ERR)
> @@ -281,6 +282,10 @@ struct i801_priv {
>  	 */
>  	bool acpi_reserved;
>  	struct mutex acpi_lock;
> +
> +	/* SMBus alert */
> +	struct i2c_smbus_alert_setup alert_data;
> +	struct i2c_client *ara;
>  };
>  
>  #define FEATURE_SMBUS_PEC	BIT(0)
> @@ -289,6 +294,7 @@ struct i801_priv {
>  #define FEATURE_I2C_BLOCK_READ	BIT(3)
>  #define FEATURE_IRQ		BIT(4)
>  #define FEATURE_HOST_NOTIFY	BIT(5)
> +#define FEATURE_SMBUS_ALERT	BIT(6)
>  /* Not really a feature, but it's convenient to handle it as such */
>  #define FEATURE_IDF		BIT(15)
>  #define FEATURE_TCO_SPT		BIT(16)
> @@ -301,6 +307,7 @@ static const char *i801_feature_names[]
>  	"I2C block read",
>  	"Interrupt",
>  	"SMBus Host Notify",
> +	"SMBus Alert",
>  };
>  
>  static unsigned int disable_features;
> @@ -310,7 +317,8 @@ MODULE_PARM_DESC(disable_features, "Disa
>  	"\t\t  0x02  disable the block buffer\n"
>  	"\t\t  0x08  disable the I2C block read functionality\n"
>  	"\t\t  0x10  don't use interrupts\n"
> -	"\t\t  0x20  disable SMBus Host Notify ");
> +	"\t\t  0x20  disable SMBus Host Notify\n"
> +	"\t\t  0x40  disable SMBus Alert ");
>  
>  /* Make sure the SMBus host is ready to start transmitting.
>     Return 0 if it is, -EBUSY if it is not. */
> @@ -620,8 +628,24 @@ static irqreturn_t i801_host_notify_isr(
>  	return IRQ_HANDLED;
>  }
>  
> +static irqreturn_t i801_smbus_alert_isr(struct i801_priv *priv)
> +{
> +	struct i2c_client *ara = priv->ara;
> +
> +	if (ara) {
> +		dev_dbg(&ara->dev, "SMBus alert received\n");
> +		i2c_handle_smbus_alert(ara);
> +	} else
> +		dev_dbg(&priv->adapter.dev,
> +			"SMBus alert received but no ARA client!\n");
> +
> +	/* clear SMBus Alert bit and return */
> +	outb_p(SMBHSTSTS_SMBALERT_STS, SMBHSTSTS(priv));
> +	return IRQ_HANDLED;
> +}
> +
>  /*
> - * There are three kinds of interrupts:
> + * There are four kinds of interrupts:
>   *
>   * 1) i801 signals transaction completion with one of these interrupts:
>   *      INTR - Success
> @@ -635,6 +659,8 @@ static irqreturn_t i801_host_notify_isr(
>   *    occurs for each byte of a byte-by-byte to prepare the next byte.
>   *
>   * 3) Host Notify interrupts
> + *
> + * 4) SMBus Alert interrupts
>   */
>  static irqreturn_t i801_isr(int irq, void *dev_id)
>  {
> @@ -653,6 +679,12 @@ static irqreturn_t i801_isr(int irq, voi
>  			return i801_host_notify_isr(priv);
>  	}
>  
> +	if (priv->features & FEATURE_SMBUS_ALERT) {
> +		status = inb_p(SMBHSTSTS(priv));
> +		if (status & SMBHSTSTS_SMBALERT_STS)
> +			return i801_smbus_alert_isr(priv);
> +	}
> +
>  	status = inb_p(SMBHSTSTS(priv));
>  	if (status & SMBHSTSTS_BYTE_DONE)
>  		i801_isr_byte_done(priv);
> @@ -1006,9 +1038,35 @@ static void i801_enable_host_notify(stru
>  	outb_p(SMBSLVSTS_HST_NTFY_STS, SMBSLVSTS(priv));
>  }
>  
> -static void i801_disable_host_notify(struct i801_priv *priv)
> +static void i801_enable_smbus_alert(struct i2c_adapter *adapter)
>  {
> -	if (!(priv->features & FEATURE_HOST_NOTIFY))
> +	struct i801_priv *priv = i2c_get_adapdata(adapter);
> +
> +	if (!(priv->features & FEATURE_SMBUS_ALERT))
> +		return;
> +
> +	priv->ara = i2c_setup_smbus_alert(adapter, &priv->alert_data);
> +	if (!priv->ara) {
> +		dev_warn(&adapter->dev, "Failed to register ARA client\n");
> +
> +		/* Disable SMBus Alert interrupts */
> +		if (!(SMBSLVCMD_SMBALERT_DIS & priv->original_slvcmd))
> +			outb_p(SMBSLVCMD_SMBALERT_DIS | priv->original_slvcmd,
> +			       SMBSLVCMD(priv));
> +		return;
> +	}
> +
> +	if (SMBSLVCMD_SMBALERT_DIS & priv->original_slvcmd)
> +		outb_p(~SMBSLVCMD_SMBALERT_DIS & priv->original_slvcmd,
> +		       SMBSLVCMD(priv));
> +
> +	/* Clear SMBus Alert bit to allow a new notification */
> +	outb_p(SMBHSTSTS_SMBALERT_STS, SMBHSTSTS(priv));

I'd rather handle the pending ALERT# not to lose the ALERTS from the boot
time for instance. This clearing of the pending bit here doesn't help
anyway, please refer below:

> +}
> +
> +static void i801_restore_slvcmd(struct i801_priv *priv)
> +{
> +	if (!(priv->features & (FEATURE_HOST_NOTIFY | FEATURE_SMBUS_ALERT)))
>  		return;
>  
>  	outb_p(priv->original_slvcmd, SMBSLVCMD(priv));
> @@ -1823,8 +1881,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 */
> -	if (priv->features & FEATURE_HOST_NOTIFY)

I'd propose to disable the ALERT# for the time of setting up ARA here...

> +	/* Remember original Host Notify and SMBus Alert setting */
> +	if (priv->features & (FEATURE_HOST_NOTIFY | FEATURE_SMBUS_ALERT))
>  		priv->original_slvcmd = inb_p(SMBSLVCMD(priv));
>  
>  	/* Default timeout in interrupt mode: 200 ms */

Because between these two hunks we enable the interrupts and we will
jump into IRQ handler without ARA being set up.

> @@ -1875,6 +1933,7 @@ static int i801_probe(struct pci_dev *de
>  	}
>  
>  	i801_enable_host_notify(&priv->adapter);
> +	i801_enable_smbus_alert(&priv->adapter);

... and re-enable ALERT# here again... 

>  	i801_probe_optional_slaves(priv);
>  	/* We ignore errors - multiplexing is optional */
> @@ -1897,8 +1956,10 @@ static void i801_remove(struct pci_dev *
>  	pm_runtime_forbid(&dev->dev);
>  	pm_runtime_get_noresume(&dev->dev);
>  
> -	i801_disable_host_notify(priv);
> +	i801_restore_slvcmd(priv);
>  	i801_del_mux(priv);
> +	if (priv->ara)
> +		i2c_unregister_device(priv->ara);
>  	i2c_del_adapter(&priv->adapter);
>  	i801_acpi_remove(priv);
>  	pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
> @@ -1916,7 +1977,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 */
> -	i801_disable_host_notify(priv);
> +	i801_restore_slvcmd(priv);
>  	pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
>  }

I can do some tests to prove my assumption regarding pending ALERT#.
Or do you agree with the above and I should test only modified version
with disable-registerARA-re-enable setup? What are your thoughts?

Hi Jean,

On 05/09/2019 14:57, Jean Delvare wrote:
> If this is of any value to you, I tried implementing it in i2c-i801 a
> few days ago. I can't really test it though as I don't have any device
> which triggers an alert on my system, but I am sharing it with you if
> you want to give it a try. You would still need to write the code in
> the slave driver.

and I forgot another issue:

> @@ -1897,8 +1956,10 @@ static void i801_remove(struct pci_dev *
>  	pm_runtime_forbid(&dev->dev);
>  	pm_runtime_get_noresume(&dev->dev);
>  
> -	i801_disable_host_notify(priv);
> +	i801_restore_slvcmd(priv);
>  	i801_del_mux(priv);
> +	if (priv->ara)
> +		i2c_unregister_device(priv->ara);

to me it looks like a race with IRQ handler (IRQ is only disabled *after*
remove()), it can still attempt to use freed ARA device after this line. 

>  	i2c_del_adapter(&priv->adapter);
>  	i801_acpi_remove(priv);
>  	pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);

Hello Jean,

On 05/09/2019 14:57, Jean Delvare wrote:
> If this is of any value to you, I tried implementing it in i2c-i801 a
> few days ago. I can't really test it though as I don't have any device
> which triggers an alert on my system, but I am sharing it with you if
> you want to give it a try. You would still need to write the code in
> the slave driver.
> 
> ---
>  drivers/i2c/busses/i2c-i801.c |   77 +++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 69 insertions(+), 8 deletions(-)
> 
> --- linux-5.2.orig/drivers/i2c/busses/i2c-i801.c	2019-08-28 15:58:52.725828215 +0200
> +++ linux-5.2/drivers/i2c/busses/i2c-i801.c	2019-08-28 16:50:09.212696037 +0200
> @@ -196,6 +196,7 @@
>  
>  /* Host Notify Command register bits */
>  #define SMBSLVCMD_HST_NTFY_INTREN	BIT(0)
> +#define SMBSLVCMD_SMBALERT_DIS		BIT(2)
>  
>  #define STATUS_ERROR_FLAGS	(SMBHSTSTS_FAILED | SMBHSTSTS_BUS_ERR | \
>  				 SMBHSTSTS_DEV_ERR)
> @@ -281,6 +282,10 @@ struct i801_priv {
>  	 */
>  	bool acpi_reserved;
>  	struct mutex acpi_lock;
> +
> +	/* SMBus alert */
> +	struct i2c_smbus_alert_setup alert_data;
> +	struct i2c_client *ara;
>  };
>  
>  #define FEATURE_SMBUS_PEC	BIT(0)
> @@ -289,6 +294,7 @@ struct i801_priv {
>  #define FEATURE_I2C_BLOCK_READ	BIT(3)
>  #define FEATURE_IRQ		BIT(4)
>  #define FEATURE_HOST_NOTIFY	BIT(5)
> +#define FEATURE_SMBUS_ALERT	BIT(6)
>  /* Not really a feature, but it's convenient to handle it as such */
>  #define FEATURE_IDF		BIT(15)
>  #define FEATURE_TCO_SPT		BIT(16)
> @@ -301,6 +307,7 @@ static const char *i801_feature_names[]
>  	"I2C block read",
>  	"Interrupt",
>  	"SMBus Host Notify",
> +	"SMBus Alert",
>  };
>  
>  static unsigned int disable_features;
> @@ -310,7 +317,8 @@ MODULE_PARM_DESC(disable_features, "Disa
>  	"\t\t  0x02  disable the block buffer\n"
>  	"\t\t  0x08  disable the I2C block read functionality\n"
>  	"\t\t  0x10  don't use interrupts\n"
> -	"\t\t  0x20  disable SMBus Host Notify ");
> +	"\t\t  0x20  disable SMBus Host Notify\n"
> +	"\t\t  0x40  disable SMBus Alert ");
>  
>  /* Make sure the SMBus host is ready to start transmitting.
>     Return 0 if it is, -EBUSY if it is not. */
> @@ -620,8 +628,24 @@ static irqreturn_t i801_host_notify_isr(
>  	return IRQ_HANDLED;
>  }
>  
> +static irqreturn_t i801_smbus_alert_isr(struct i801_priv *priv)
> +{
> +	struct i2c_client *ara = priv->ara;
> +
> +	if (ara) {
> +		dev_dbg(&ara->dev, "SMBus alert received\n");
> +		i2c_handle_smbus_alert(ara);

I've just tested this approach and must conclude that it doesn't work.
Seems that SMBALERT# in i801 is level-triggered (I wasn't able to
confirm or decline this statement using i801 documentation) and
once SMBALERT# is triggered, I experience the interrupt flood.
Clearing the interrupt request bit below has no effect at this point.

Unfortunately, current smbalert API doesn't provide any synchronous
interface to test another approach.

> +	} else
> +		dev_dbg(&priv->adapter.dev,
> +			"SMBus alert received but no ARA client!\n");
> +
> +	/* clear SMBus Alert bit and return */
> +	outb_p(SMBHSTSTS_SMBALERT_STS, SMBHSTSTS(priv));
> +	return IRQ_HANDLED;
> +}
> +
>  /*
> - * There are three kinds of interrupts:
> + * There are four kinds of interrupts:
>   *
>   * 1) i801 signals transaction completion with one of these interrupts:
>   *      INTR - Success

Therefore, I'd like to ask you (and other I2C maintainers), not that
I spend a time for a solution which has no upstream perspectives, what
shall be the preferred way for i801 driver:

a) We disable SMBALERT# completely

or

b) I export smbus_alert(), so that we can
	- catch the IRQ
	- disable SMBALERT#
	- synchronously smbus_alert()
	- re-enable SMBALERT#

?