[PATCH 0/2] i2c: smbus: Handle stuck alerts

[PATCH 0/2] i2c: smbus: Handle stuck alerts

[Guenter Roeck]

While playing with SMBus alert functionality, I noticed the following
messages if alert was active on more than once device.

smbus_alert 3-000c: SMBALERT# from dev 0x0c, flag 0
smbus_alert 3-000c: no driver alert()!

or:

smbus_alert 3-000c: SMBALERT# from dev 0x28, flag 0

This is seen if multiple devices assert alert at the same time and at least
one of them does not or not correctly implement SMBus arbitration.

Once it starts, this message repeats forever at high rate.
Worst case, the problem turn resulted in system crashes after a while.

The following two patches fix the problem for me. The first patch
aborts the endless loop in smbus_alert() if no handler is found
for an alert address. The second patch sends alerts to all devices
with alert handler if that situation is observed.

I split the changes into two patches since I figured that the first patch
might be easier to accept. However, both patches are really needed to
fix the problem for good.

Note that there is one situation which is not addressed by this set of
patches: If the corrupted address points to yet another device with alert
handler on the same bus, the alert handler of that device will be called.
If it is not a source of the alert, we are back to the original problem.
I do not know how to address this case.

----------------------------------------------------------------
Guenter Roeck (2):
      i2c: smbus: Improve handling of stuck alerts
      i2c: smbus: Send alert notifications to all devices if source not found

 drivers/i2c/i2c-smbus.c | 64 ++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 58 insertions(+), 6 deletions(-)

[PATCH 1/2] i2c: smbus: Improve handling of stuck alerts

[Guenter Roeck]

The following messages were observed while testing alert functionality
on systems with multiple I2C devices on a single bus if alert was active
on more than one chip.

smbus_alert 3-000c: SMBALERT# from dev 0x0c, flag 0
smbus_alert 3-000c: no driver alert()!

and:

smbus_alert 3-000c: SMBALERT# from dev 0x28, flag 0

Once it starts, this message repeats forever at high rate. There is no
device at any of the reported addresses.

Analysis shows that this is seen if multiple devices have the alert pin
active. Apparently some devices do not support SMBus arbitration correctly.
They keep sending address bits after detecting an address collision and
handle the collision not at all or too late.
Specifically, address 0x0c is seen with ADT7461A at address 0x4c and
ADM1021 at address 0x18 if alert is active on both chips. Address 0x28 is
seen with ADT7483 at address 0x2a and ADT7461 at address 0x4c if alert is
active on both chips.

Once the system is in bad state (alert is set by more than one chip),
it often only recovers by power cycling.

To reduce the impact of this problem, abort the endless loop in
smbus_alert() if the same address is read more than once and not
handled by a driver.

Fixes: b5527a7766f0 ("i2c: Add SMBus alert support")
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/i2c/i2c-smbus.c | 30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
index d3d06e3b4f3b..533c885b99ac 100644
--- a/drivers/i2c/i2c-smbus.c
+++ b/drivers/i2c/i2c-smbus.c
@@ -34,6 +34,7 @@ static int smbus_do_alert(struct device *dev, void *addrp)
 	struct i2c_client *client = i2c_verify_client(dev);
 	struct alert_data *data = addrp;
 	struct i2c_driver *driver;
+	int ret;
 
 	if (!client || client->addr != data->addr)
 		return 0;
@@ -47,16 +48,21 @@ static int smbus_do_alert(struct device *dev, void *addrp)
 	device_lock(dev);
 	if (client->dev.driver) {
 		driver = to_i2c_driver(client->dev.driver);
-		if (driver->alert)
+		if (driver->alert) {
 			driver->alert(client, data->type, data->data);
-		else
+			ret = -EBUSY;
+		} else {
 			dev_warn(&client->dev, "no driver alert()!\n");
-	} else
+			ret = -EOPNOTSUPP;
+		}
+	} else {
 		dev_dbg(&client->dev, "alert with no driver\n");
+		ret = -ENODEV;
+	}
 	device_unlock(dev);
 
 	/* Stop iterating after we find the device */
-	return -EBUSY;
+	return ret;
 }
 
 /*
@@ -67,6 +73,7 @@ static irqreturn_t smbus_alert(int irq, void *d)
 {
 	struct i2c_smbus_alert *alert = d;
 	struct i2c_client *ara;
+	unsigned short prev_addr = 0;	/* Not a valid address */
 
 	ara = alert->ara;
 
@@ -94,8 +101,19 @@ static irqreturn_t smbus_alert(int irq, void *d)
 			data.addr, data.data);
 
 		/* Notify driver for the device which issued the alert */
-		device_for_each_child(&ara->adapter->dev, &data,
-				      smbus_do_alert);
+		status = device_for_each_child(&ara->adapter->dev, &data,
+					       smbus_do_alert);
+		/*
+		 * If we read the same address more than once, and the alert
+		 * was not handled by a driver, it won't do any good to repeat
+		 * the loop because it will never terminate.
+		 * Bail out in this case.
+		 * Note: This assumes that a driver with alert handler handles
+		 * the alert properly and clears it if necessary.
+		 */
+		if (data.addr == prev_addr && status != -EBUSY)
+			break;
+		prev_addr = data.addr;
 	}
 
 	return IRQ_HANDLED;
-- 
2.33.0

[PATCH 2/2] i2c: smbus: Send alert notifications to all devices if source not found

[Guenter Roeck]

If a SMBUs alert is received and the originating device is not found,
the reason may be that the address reported on the SMBus alert address
is corrupted, for example because multiple devices asserted alert and
do not correctly implement SMBus arbitration.

If this happens, call alert handlers on all devices connected to the
given I2C bus, in the hope that this cleans up the situation. Retry
twice before giving up.

This change reliably fixed the problem on a system with multiple devices
on a single bus. Example log where the device on address 0x18 (ADM1021)
and on address 0x4c (ADM7461A) both had the alert line asserted:

smbus_alert 3-000c: SMBALERT# from dev 0x0c, flag 0
smbus_alert 3-000c: no driver alert()!
smbus_alert 3-000c: SMBALERT# from dev 0x0c, flag 0
smbus_alert 3-000c: no driver alert()!
lm90 3-0018: temp1 out of range, please check!
lm90 3-0018: Disabling ALERT#
lm90 3-0029: Everything OK
lm90 3-002a: Everything OK
lm90 3-004c: temp1 out of range, please check!
lm90 3-004c: temp2 out of range, please check!
lm90 3-004c: Disabling ALERT#

Fixes: b5527a7766f0 ("i2c: Add SMBus alert support")
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/i2c/i2c-smbus.c | 38 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 36 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
index 533c885b99ac..f48cec19db41 100644
--- a/drivers/i2c/i2c-smbus.c
+++ b/drivers/i2c/i2c-smbus.c
@@ -65,6 +65,32 @@ static int smbus_do_alert(struct device *dev, void *addrp)
 	return ret;
 }
 
+/* Same as above, but call back all drivers with alert handler */
+
+static int smbus_do_alert_force(struct device *dev, void *addrp)
+{
+	struct i2c_client *client = i2c_verify_client(dev);
+	struct alert_data *data = addrp;
+	struct i2c_driver *driver;
+
+	if (!client || (client->flags & I2C_CLIENT_TEN))
+		return 0;
+
+	/*
+	 * Drivers should either disable alerts, or provide at least
+	 * a minimal handler. Lock so the driver won't change.
+	 */
+	device_lock(dev);
+	if (client->dev.driver) {
+		driver = to_i2c_driver(client->dev.driver);
+		if (driver->alert)
+			driver->alert(client, data->type, data->data);
+	}
+	device_unlock(dev);
+
+	return 0;
+}
+
 /*
  * The alert IRQ handler needs to hand work off to a task which can issue
  * SMBus calls, because those sleeping calls can't be made in IRQ context.
@@ -74,6 +100,7 @@ static irqreturn_t smbus_alert(int irq, void *d)
 	struct i2c_smbus_alert *alert = d;
 	struct i2c_client *ara;
 	unsigned short prev_addr = 0;	/* Not a valid address */
+	int retries = 0;
 
 	ara = alert->ara;
 
@@ -111,8 +138,15 @@ static irqreturn_t smbus_alert(int irq, void *d)
 		 * Note: This assumes that a driver with alert handler handles
 		 * the alert properly and clears it if necessary.
 		 */
-		if (data.addr == prev_addr && status != -EBUSY)
-			break;
+		if (data.addr == prev_addr && status != -EBUSY) {
+			/* retry once */
+			if (retries++)
+				break;
+			device_for_each_child(&ara->adapter->dev, &data,
+					      smbus_do_alert_force);
+		} else {
+			retries = 0;
+		}
 		prev_addr = data.addr;
 	}
 
-- 
2.33.0

Re: [PATCH 0/2] i2c: smbus: Handle stuck alerts

[Guenter Roeck]

Hi,

On Mon, Jan 10, 2022 at 09:28:55AM -0800, Guenter Roeck wrote:
> While playing with SMBus alert functionality, I noticed the following
> messages if alert was active on more than once device.
> 
> smbus_alert 3-000c: SMBALERT# from dev 0x0c, flag 0
> smbus_alert 3-000c: no driver alert()!
> 
> or:
> 
> smbus_alert 3-000c: SMBALERT# from dev 0x28, flag 0
> 
> This is seen if multiple devices assert alert at the same time and at least
> one of them does not or not correctly implement SMBus arbitration.
> 
> Once it starts, this message repeats forever at high rate.
> Worst case, the problem turn resulted in system crashes after a while.
> 
> The following two patches fix the problem for me. The first patch
> aborts the endless loop in smbus_alert() if no handler is found
> for an alert address. The second patch sends alerts to all devices
> with alert handler if that situation is observed.
> 
> I split the changes into two patches since I figured that the first patch
> might be easier to accept. However, both patches are really needed to
> fix the problem for good.
> 
> Note that there is one situation which is not addressed by this set of
> patches: If the corrupted address points to yet another device with alert
> handler on the same bus, the alert handler of that device will be called.
> If it is not a source of the alert, we are back to the original problem.
> I do not know how to address this case.
> 
> ----------------------------------------------------------------
> Guenter Roeck (2):
>       i2c: smbus: Improve handling of stuck alerts
>       i2c: smbus: Send alert notifications to all devices if source not found
> 
>  drivers/i2c/i2c-smbus.c | 64 ++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 58 insertions(+), 6 deletions(-)

Looking through the patches I carry locally, I just noticed that
I never got a reply to this series. Is there a problem with it,
or did it just get lost ?

Thanks,
Guenter

Re: [PATCH 0/2] i2c: smbus: Handle stuck alerts

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 321 bytes --]


> Looking through the patches I carry locally, I just noticed that
> I never got a reply to this series. Is there a problem with it,
> or did it just get lost ?

The only problem was that I didn't have the bandwidth. But luckily, I
need to work on SMBALERT myself now, so I will handle all related
commits around that.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/2] i2c: smbus: Handle stuck alerts

[Guenter Roeck]

On 6/12/24 13:21, Wolfram Sang wrote:
> 
>> Looking through the patches I carry locally, I just noticed that
>> I never got a reply to this series. Is there a problem with it,
>> or did it just get lost ?
> 
> The only problem was that I didn't have the bandwidth. But luckily, I
> need to work on SMBALERT myself now, so I will handle all related
> commits around that.
> 

Ah, just the "normal" problem. Let me know if I can help.
I still have the hardware that I used to test that code.

Guenter

Re: [PATCH 0/2] i2c: smbus: Handle stuck alerts

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 887 bytes --]

Hi Guenter,

as I mentioned before I now have to deal with SMBusAlert as well and had
a chance to review and test this series. When developing the SMBAlert
trigger mechanism for the i2c testunit, I also experienced the interrupt
storm and your patches helped. See later mails for details.

> Note that there is one situation which is not addressed by this set of
> patches: If the corrupted address points to yet another device with alert
> handler on the same bus, the alert handler of that device will be called.
> If it is not a source of the alert, we are back to the original problem.
> I do not know how to address this case.

I think this can only work if we require .alert-handlers to start with a
sanity check to make sure their device really raised an interrupt
condition. And then return either -EBUSY or 0, similar to IRQ_HANDLED or
IRQ_NONE. Or?

All the best,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] i2c: smbus: Improve handling of stuck alerts

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 4166 bytes --]

On Mon, Jan 10, 2022 at 09:28:56AM -0800, Guenter Roeck wrote:
> The following messages were observed while testing alert functionality
> on systems with multiple I2C devices on a single bus if alert was active
> on more than one chip.
> 
> smbus_alert 3-000c: SMBALERT# from dev 0x0c, flag 0
> smbus_alert 3-000c: no driver alert()!
> 
> and:
> 
> smbus_alert 3-000c: SMBALERT# from dev 0x28, flag 0
> 
> Once it starts, this message repeats forever at high rate. There is no
> device at any of the reported addresses.
> 
> Analysis shows that this is seen if multiple devices have the alert pin
> active. Apparently some devices do not support SMBus arbitration correctly.
> They keep sending address bits after detecting an address collision and
> handle the collision not at all or too late.
> Specifically, address 0x0c is seen with ADT7461A at address 0x4c and
> ADM1021 at address 0x18 if alert is active on both chips. Address 0x28 is
> seen with ADT7483 at address 0x2a and ADT7461 at address 0x4c if alert is
> active on both chips.
> 
> Once the system is in bad state (alert is set by more than one chip),
> it often only recovers by power cycling.
> 
> To reduce the impact of this problem, abort the endless loop in
> smbus_alert() if the same address is read more than once and not
> handled by a driver.
> 
> Fixes: b5527a7766f0 ("i2c: Add SMBus alert support")
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

It fixed the interrupt storm for me and I like the approach. I'd apply
it to for-current once rc1 is released. Two minor changes, though.

> ---
>  drivers/i2c/i2c-smbus.c | 30 ++++++++++++++++++++++++------
>  1 file changed, 24 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
> index d3d06e3b4f3b..533c885b99ac 100644
> --- a/drivers/i2c/i2c-smbus.c
> +++ b/drivers/i2c/i2c-smbus.c
> @@ -34,6 +34,7 @@ static int smbus_do_alert(struct device *dev, void *addrp)
>  	struct i2c_client *client = i2c_verify_client(dev);
>  	struct alert_data *data = addrp;
>  	struct i2c_driver *driver;
> +	int ret;
>  
>  	if (!client || client->addr != data->addr)
>  		return 0;
> @@ -47,16 +48,21 @@ static int smbus_do_alert(struct device *dev, void *addrp)
>  	device_lock(dev);
>  	if (client->dev.driver) {
>  		driver = to_i2c_driver(client->dev.driver);
> -		if (driver->alert)
> +		if (driver->alert) {
>  			driver->alert(client, data->type, data->data);
> -		else
> +			ret = -EBUSY;
> +		} else {
>  			dev_warn(&client->dev, "no driver alert()!\n");
> -	} else
> +			ret = -EOPNOTSUPP;
> +		}
> +	} else {
>  		dev_dbg(&client->dev, "alert with no driver\n");
> +		ret = -ENODEV;
> +	}
>  	device_unlock(dev);
>  
>  	/* Stop iterating after we find the device */

I moved this comment up where -EBUSY is assigned to 'ret'.

> -	return -EBUSY;
> +	return ret;
>  }
>  
>  /*
> @@ -67,6 +73,7 @@ static irqreturn_t smbus_alert(int irq, void *d)
>  {
>  	struct i2c_smbus_alert *alert = d;
>  	struct i2c_client *ara;
> +	unsigned short prev_addr = 0;	/* Not a valid address */

I used I2C_CLIENT_END as an invalid address. We use it for the same
purpose in other parts of the core as well.

>  
>  	ara = alert->ara;
>  
> @@ -94,8 +101,19 @@ static irqreturn_t smbus_alert(int irq, void *d)
>  			data.addr, data.data);
>  
>  		/* Notify driver for the device which issued the alert */
> -		device_for_each_child(&ara->adapter->dev, &data,
> -				      smbus_do_alert);
> +		status = device_for_each_child(&ara->adapter->dev, &data,
> +					       smbus_do_alert);
> +		/*
> +		 * If we read the same address more than once, and the alert
> +		 * was not handled by a driver, it won't do any good to repeat
> +		 * the loop because it will never terminate.
> +		 * Bail out in this case.
> +		 * Note: This assumes that a driver with alert handler handles
> +		 * the alert properly and clears it if necessary.
> +		 */
> +		if (data.addr == prev_addr && status != -EBUSY)
> +			break;
> +		prev_addr = data.addr;
>  	}
>  
>  	return IRQ_HANDLED;
> -- 
> 2.33.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] i2c: smbus: Send alert notifications to all devices if source not found

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 3708 bytes --]

On Mon, Jan 10, 2022 at 09:28:57AM -0800, Guenter Roeck wrote:
> If a SMBUs alert is received and the originating device is not found,
> the reason may be that the address reported on the SMBus alert address
> is corrupted, for example because multiple devices asserted alert and
> do not correctly implement SMBus arbitration.
> 
> If this happens, call alert handlers on all devices connected to the
> given I2C bus, in the hope that this cleans up the situation. Retry
> twice before giving up.

High level question: why the retry? Did you experience address
collisions going away on the second try? My guess is that they would be
mostly persistent, so we could call smbus_do_alert_force() right away?

> 
> This change reliably fixed the problem on a system with multiple devices
> on a single bus. Example log where the device on address 0x18 (ADM1021)
> and on address 0x4c (ADM7461A) both had the alert line asserted:
> 
> smbus_alert 3-000c: SMBALERT# from dev 0x0c, flag 0
> smbus_alert 3-000c: no driver alert()!
> smbus_alert 3-000c: SMBALERT# from dev 0x0c, flag 0
> smbus_alert 3-000c: no driver alert()!
> lm90 3-0018: temp1 out of range, please check!
> lm90 3-0018: Disabling ALERT#
> lm90 3-0029: Everything OK
> lm90 3-002a: Everything OK
> lm90 3-004c: temp1 out of range, please check!
> lm90 3-004c: temp2 out of range, please check!
> lm90 3-004c: Disabling ALERT#
> 
> Fixes: b5527a7766f0 ("i2c: Add SMBus alert support")
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/i2c/i2c-smbus.c | 38 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 36 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
> index 533c885b99ac..f48cec19db41 100644
> --- a/drivers/i2c/i2c-smbus.c
> +++ b/drivers/i2c/i2c-smbus.c
> @@ -65,6 +65,32 @@ static int smbus_do_alert(struct device *dev, void *addrp)
>  	return ret;
>  }
>  
> +/* Same as above, but call back all drivers with alert handler */
> +
> +static int smbus_do_alert_force(struct device *dev, void *addrp)
> +{
> +	struct i2c_client *client = i2c_verify_client(dev);
> +	struct alert_data *data = addrp;
> +	struct i2c_driver *driver;
> +
> +	if (!client || (client->flags & I2C_CLIENT_TEN))
> +		return 0;
> +
> +	/*
> +	 * Drivers should either disable alerts, or provide at least
> +	 * a minimal handler. Lock so the driver won't change.
> +	 */
> +	device_lock(dev);
> +	if (client->dev.driver) {
> +		driver = to_i2c_driver(client->dev.driver);
> +		if (driver->alert)
> +			driver->alert(client, data->type, data->data);
> +	}
> +	device_unlock(dev);
> +
> +	return 0;
> +}
> +
>  /*
>   * The alert IRQ handler needs to hand work off to a task which can issue
>   * SMBus calls, because those sleeping calls can't be made in IRQ context.
> @@ -74,6 +100,7 @@ static irqreturn_t smbus_alert(int irq, void *d)
>  	struct i2c_smbus_alert *alert = d;
>  	struct i2c_client *ara;
>  	unsigned short prev_addr = 0;	/* Not a valid address */
> +	int retries = 0;
>  
>  	ara = alert->ara;
>  
> @@ -111,8 +138,15 @@ static irqreturn_t smbus_alert(int irq, void *d)
>  		 * Note: This assumes that a driver with alert handler handles
>  		 * the alert properly and clears it if necessary.
>  		 */
> -		if (data.addr == prev_addr && status != -EBUSY)
> -			break;
> +		if (data.addr == prev_addr && status != -EBUSY) {
> +			/* retry once */
> +			if (retries++)
> +				break;
> +			device_for_each_child(&ara->adapter->dev, &data,
> +					      smbus_do_alert_force);
> +		} else {
> +			retries = 0;
> +		}
>  		prev_addr = data.addr;
>  	}
>  
> -- 
> 2.33.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/2] i2c: smbus: Handle stuck alerts

[Guenter Roeck]

On 7/28/24 12:59, Wolfram Sang wrote:
> Hi Guenter,
> 
> as I mentioned before I now have to deal with SMBusAlert as well and had
> a chance to review and test this series. When developing the SMBAlert
> trigger mechanism for the i2c testunit, I also experienced the interrupt
> storm and your patches helped. See later mails for details.
> 
>> Note that there is one situation which is not addressed by this set of
>> patches: If the corrupted address points to yet another device with alert
>> handler on the same bus, the alert handler of that device will be called.
>> If it is not a source of the alert, we are back to the original problem.
>> I do not know how to address this case.
> 
> I think this can only work if we require .alert-handlers to start with a
> sanity check to make sure their device really raised an interrupt
> condition. And then return either -EBUSY or 0, similar to IRQ_HANDLED or
> IRQ_NONE. Or?
> 

I think so, but I am not sure if it is worth the effort. It would require
changing the API, and each driver supporting alert callbacks would have
to implement code to detect if it actually got an interrupt.

Thanks,
Guenter

Re: [PATCH 2/2] i2c: smbus: Send alert notifications to all devices if source not found

[Guenter Roeck]

On 7/28/24 13:04, Wolfram Sang wrote:
> On Mon, Jan 10, 2022 at 09:28:57AM -0800, Guenter Roeck wrote:
>> If a SMBUs alert is received and the originating device is not found,
>> the reason may be that the address reported on the SMBus alert address
>> is corrupted, for example because multiple devices asserted alert and
>> do not correctly implement SMBus arbitration.
>>
>> If this happens, call alert handlers on all devices connected to the
>> given I2C bus, in the hope that this cleans up the situation. Retry
>> twice before giving up.
> 
> High level question: why the retry? Did you experience address
> collisions going away on the second try? My guess is that they would be
> mostly persistent, so we could call smbus_do_alert_force() right away?
> 

I honestly don't recall. I had some brute force code to trigger alerts
on connected chips. Maybe the idea was to catch situations where another
alert was raised after or during the first cycle.

As for "call smbus_do_alert_force() right away", I am not sure I understand.
Isn't that what the code is doing twice ?

Thanks,
Guenter

>>
>> This change reliably fixed the problem on a system with multiple devices
>> on a single bus. Example log where the device on address 0x18 (ADM1021)
>> and on address 0x4c (ADM7461A) both had the alert line asserted:
Side note: That was ADT7461A, not ADM7461A.


>>
>> smbus_alert 3-000c: SMBALERT# from dev 0x0c, flag 0
>> smbus_alert 3-000c: no driver alert()!
>> smbus_alert 3-000c: SMBALERT# from dev 0x0c, flag 0
>> smbus_alert 3-000c: no driver alert()!
>> lm90 3-0018: temp1 out of range, please check!
>> lm90 3-0018: Disabling ALERT#
>> lm90 3-0029: Everything OK
>> lm90 3-002a: Everything OK
>> lm90 3-004c: temp1 out of range, please check!
>> lm90 3-004c: temp2 out of range, please check!
>> lm90 3-004c: Disabling ALERT#
>>
>> Fixes: b5527a7766f0 ("i2c: Add SMBus alert support")
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>>   drivers/i2c/i2c-smbus.c | 38 ++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 36 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
>> index 533c885b99ac..f48cec19db41 100644
>> --- a/drivers/i2c/i2c-smbus.c
>> +++ b/drivers/i2c/i2c-smbus.c
>> @@ -65,6 +65,32 @@ static int smbus_do_alert(struct device *dev, void *addrp)
>>   	return ret;
>>   }
>>   
>> +/* Same as above, but call back all drivers with alert handler */
>> +
>> +static int smbus_do_alert_force(struct device *dev, void *addrp)
>> +{
>> +	struct i2c_client *client = i2c_verify_client(dev);
>> +	struct alert_data *data = addrp;
>> +	struct i2c_driver *driver;
>> +
>> +	if (!client || (client->flags & I2C_CLIENT_TEN))
>> +		return 0;
>> +
>> +	/*
>> +	 * Drivers should either disable alerts, or provide at least
>> +	 * a minimal handler. Lock so the driver won't change.
>> +	 */
>> +	device_lock(dev);
>> +	if (client->dev.driver) {
>> +		driver = to_i2c_driver(client->dev.driver);
>> +		if (driver->alert)
>> +			driver->alert(client, data->type, data->data);
>> +	}
>> +	device_unlock(dev);
>> +
>> +	return 0;
>> +}
>> +
>>   /*
>>    * The alert IRQ handler needs to hand work off to a task which can issue
>>    * SMBus calls, because those sleeping calls can't be made in IRQ context.
>> @@ -74,6 +100,7 @@ static irqreturn_t smbus_alert(int irq, void *d)
>>   	struct i2c_smbus_alert *alert = d;
>>   	struct i2c_client *ara;
>>   	unsigned short prev_addr = 0;	/* Not a valid address */
>> +	int retries = 0;
>>   
>>   	ara = alert->ara;
>>   
>> @@ -111,8 +138,15 @@ static irqreturn_t smbus_alert(int irq, void *d)
>>   		 * Note: This assumes that a driver with alert handler handles
>>   		 * the alert properly and clears it if necessary.
>>   		 */
>> -		if (data.addr == prev_addr && status != -EBUSY)
>> -			break;
>> +		if (data.addr == prev_addr && status != -EBUSY) {
>> +			/* retry once */
>> +			if (retries++)
>> +				break;
>> +			device_for_each_child(&ara->adapter->dev, &data,
>> +					      smbus_do_alert_force);
>> +		} else {
>> +			retries = 0;
>> +		}
>>   		prev_addr = data.addr;
>>   	}
>>   
>> -- 
>> 2.33.0
>>

Re: [PATCH 1/2] i2c: smbus: Improve handling of stuck alerts

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1543 bytes --]

On Mon, Jan 10, 2022 at 09:28:56AM -0800, Guenter Roeck wrote:
> The following messages were observed while testing alert functionality
> on systems with multiple I2C devices on a single bus if alert was active
> on more than one chip.
> 
> smbus_alert 3-000c: SMBALERT# from dev 0x0c, flag 0
> smbus_alert 3-000c: no driver alert()!
> 
> and:
> 
> smbus_alert 3-000c: SMBALERT# from dev 0x28, flag 0
> 
> Once it starts, this message repeats forever at high rate. There is no
> device at any of the reported addresses.
> 
> Analysis shows that this is seen if multiple devices have the alert pin
> active. Apparently some devices do not support SMBus arbitration correctly.
> They keep sending address bits after detecting an address collision and
> handle the collision not at all or too late.
> Specifically, address 0x0c is seen with ADT7461A at address 0x4c and
> ADM1021 at address 0x18 if alert is active on both chips. Address 0x28 is
> seen with ADT7483 at address 0x2a and ADT7461 at address 0x4c if alert is
> active on both chips.
> 
> Once the system is in bad state (alert is set by more than one chip),
> it often only recovers by power cycling.
> 
> To reduce the impact of this problem, abort the endless loop in
> smbus_alert() if the same address is read more than once and not
> handled by a driver.
> 
> Fixes: b5527a7766f0 ("i2c: Add SMBus alert support")
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

Applied to for-current (with the minor changes mentioned before),
thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] i2c: smbus: Send alert notifications to all devices if source not found

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1326 bytes --]

Hi Guenter,

thanks for the feedback!

> > High level question: why the retry? Did you experience address
> > collisions going away on the second try? My guess is that they would be
> > mostly persistent, so we could call smbus_do_alert_force() right away?
> > 
> 
> I honestly don't recall. I had some brute force code to trigger alerts
> on connected chips. Maybe the idea was to catch situations where another
> alert was raised after or during the first cycle.

Hmm, I'd think that SMBAlert then stays asserted and the whole alert
handling will be started right away a second time? Given that all
hardware works correctly, of course. Your setup showed that arbitration
does not work well with actual hardware. Props for finding this out!

> As for "call smbus_do_alert_force() right away", I am not sure I understand.
> Isn't that what the code is doing twice ?

It calls smbus_do_alert() twice (without '_force'). If that fails, it
calls the _force version. I am wondering now if we can't call the _force
version right after smbus_do_alert() fails once. Meaning we could remove
all the "retries" code from your patch. If there is no clear reason for
the code, not having it is easier to maintain. That's why I ask.

I hope the question is understandable now.

Happy hacking,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/2] i2c: smbus: Handle stuck alerts

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 799 bytes --]


> > I think this can only work if we require .alert-handlers to start with a
> > sanity check to make sure their device really raised an interrupt
> > condition. And then return either -EBUSY or 0, similar to IRQ_HANDLED or
> > IRQ_NONE. Or?
> 
> I think so, but I am not sure if it is worth the effort. It would require
> changing the API, and each driver supporting alert callbacks would have
> to implement code to detect if it actually got an interrupt.

The more I think about it, the more I like this proposal. I mean, irq
subsystem has IRQ_NONE also for a reason. And we do not have that many
alert handlers to convert. They could all return EBUSY by default
because that is what they currently behave like anyhow. That being said,
it will not be a top priority on my list.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] i2c: smbus: Send alert notifications to all devices if source not found

[Guenter Roeck]

On 7/29/24 00:57, Wolfram Sang wrote:
> Hi Guenter,
> 
> thanks for the feedback!
> 
>>> High level question: why the retry? Did you experience address
>>> collisions going away on the second try? My guess is that they would be
>>> mostly persistent, so we could call smbus_do_alert_force() right away?
>>>
>>
>> I honestly don't recall. I had some brute force code to trigger alerts
>> on connected chips. Maybe the idea was to catch situations where another
>> alert was raised after or during the first cycle.
> 
> Hmm, I'd think that SMBAlert then stays asserted and the whole alert
> handling will be started right away a second time? Given that all
> hardware works correctly, of course. Your setup showed that arbitration
> does not work well with actual hardware. Props for finding this out!
> 
>> As for "call smbus_do_alert_force() right away", I am not sure I understand.
>> Isn't that what the code is doing twice ?
> 
> It calls smbus_do_alert() twice (without '_force'). If that fails, it
> calls the _force version. I am wondering now if we can't call the _force
> version right after smbus_do_alert() fails once. Meaning we could remove
> all the "retries" code from your patch. If there is no clear reason for
> the code, not having it is easier to maintain. That's why I ask.
> 
> I hope the question is understandable now.
> 

I looked into the code again. The sequence is (or is supposed to be):

1st loop:
	if (!alert_pending)
		break;
	smbus_do_alert()
	if (failed at same address)
		smbus_do_alert_force()

2nd loop:
	if (!alert_pending)
		break;
	smbus_do_alert()
	if (failed at same address)
		break;

I think what you are suggesting is

1st loop:
	if (!alert_pending)
		break;
	smbus_do_alert()
	if (failed at same address)
		retries++;
2nd loop:
	if (!alert_pending)
		break;
	smbus_do_alert_force()
	if (failed at same address && retries)
		break;

But in reality that would not be much different because the alert status
is checked prior to calling smbus_do_alert() again.

With your suggestion (if I understand it correctly), the code would be
something like

                 /* Notify driver for the device which issued the alert */
                 status = device_for_each_child(&ara->adapter->dev, &data,
                                                retries ? smbus_do_alert_force : smbus_do_alert);
                 /*
                  * If we read the same address more than once, and the alert
                  * was not handled by a driver, it won't do any good to repeat
                  * the loop because it will never terminate.
                  * Bail out in this case.
                  * Note: This assumes that a driver with alert handler handles
                  * the alert properly and clears it if necessary.
                  */
                 if (data.addr == prev_addr && status != -EBUSY) {
                         /* retry once */
                         if (retries++)
                                 break;
                 } else {
                         retries = 0;
                 }

I don't know, I prefer my code. It keeps the exception /retry handling in one
place. Personal preference, maybe. Either case, retries could probably be made
a boolean.

Thanks,
Guenter

Re: [PATCH 2/2] i2c: smbus: Send alert notifications to all devices if source not found

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1148 bytes --]


> I looked into the code again. The sequence is (or is supposed to be):
> 
> 1st loop:
> 	if (!alert_pending)
> 		break;
> 	smbus_do_alert()
> 	if (failed at same address)
> 		smbus_do_alert_force()
> 
> 2nd loop:
> 	if (!alert_pending)
> 		break;
> 	smbus_do_alert()
> 	if (failed at same address)
> 		break;
> 
> I think what you are suggesting is
...

What I am suggesting is more like this:

1st loop:

 	smbus_do_alert()
	//impossible to have same address on first run, so go to 2nd loop

2nd loop:

 	smbus_do_alert()
 	if (failed at same address)
 		smbus_do_alert_force()
		break;

As I understand it, your sequence is missing "my" 1st loop with the
invalid address, so you will end up having 3 loops altogether?

The code I am suggesting is bascially yours without the retries
variable:

	status = device_for_each_child(&ara->adapter->dev, &data,
				       smbus_do_alert);
	if (data.addr == prev_addr && status != -EBUSY) {
		device_for_each_child(&ara->adapter->dev, &data,
				      smbus_do_alert_force);
		break;
	}
	prev_addr = data.addr;

Makes sense or am I missing something?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] i2c: smbus: Send alert notifications to all devices if source not found

[Guenter Roeck]

On 7/29/24 11:36, Wolfram Sang wrote:
> 
>> I looked into the code again. The sequence is (or is supposed to be):
>>
>> 1st loop:
>> 	if (!alert_pending)
>> 		break;
>> 	smbus_do_alert()
>> 	if (failed at same address)
>> 		smbus_do_alert_force()
>>
>> 2nd loop:
>> 	if (!alert_pending)
>> 		break;
>> 	smbus_do_alert()
>> 	if (failed at same address)
>> 		break;
>>
>> I think what you are suggesting is
> ...
> 
> What I am suggesting is more like this:
> 
> 1st loop:
> 
>   	smbus_do_alert()
> 	//impossible to have same address on first run, so go to 2nd loop
> 
> 2nd loop:
> 
>   	smbus_do_alert()
>   	if (failed at same address)
>   		smbus_do_alert_force()
> 		break;
> 
> As I understand it, your sequence is missing "my" 1st loop with the
> invalid address, so you will end up having 3 loops altogether?
> 
> The code I am suggesting is bascially yours without the retries
> variable:
> 
> 	status = device_for_each_child(&ara->adapter->dev, &data,
> 				       smbus_do_alert);
> 	if (data.addr == prev_addr && status != -EBUSY) {
> 		device_for_each_child(&ara->adapter->dev, &data,
> 				      smbus_do_alert_force);
> 		break;
> 	}
> 	prev_addr = data.addr;
> 
> Makes sense or am I missing something?
> 

Yes, that should work and is indeed simpler. You are correct, the
additional loop should not be necessary since smbus_do_alert_force()
should already call all connected drivers and hopefully clear
the alert condition on those.

Thanks,
Guenter

Re: [PATCH 2/2] i2c: smbus: Send alert notifications to all devices if source not found

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 773 bytes --]


> > The code I am suggesting is bascially yours without the retries
> > variable:
> > 
> > 	status = device_for_each_child(&ara->adapter->dev, &data,
> > 				       smbus_do_alert);
> > 	if (data.addr == prev_addr && status != -EBUSY) {
> > 		device_for_each_child(&ara->adapter->dev, &data,
> > 				      smbus_do_alert_force);
> > 		break;
> > 	}
> > 	prev_addr = data.addr;
> > 
> > Makes sense or am I missing something?
> > 
> 
> Yes, that should work and is indeed simpler. You are correct, the
> additional loop should not be necessary since smbus_do_alert_force()
> should already call all connected drivers and hopefully clear
> the alert condition on those.

Great that we are aligned now! Do you have time to rework and test the
patch?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] i2c: smbus: Send alert notifications to all devices if source not found

[Guenter Roeck]

On 7/29/24 13:52, Wolfram Sang wrote:
> 
>>> The code I am suggesting is bascially yours without the retries
>>> variable:
>>>
>>> 	status = device_for_each_child(&ara->adapter->dev, &data,
>>> 				       smbus_do_alert);
>>> 	if (data.addr == prev_addr && status != -EBUSY) {
>>> 		device_for_each_child(&ara->adapter->dev, &data,
>>> 				      smbus_do_alert_force);
>>> 		break;
>>> 	}
>>> 	prev_addr = data.addr;
>>>
>>> Makes sense or am I missing something?
>>>
>>
>> Yes, that should work and is indeed simpler. You are correct, the
>> additional loop should not be necessary since smbus_do_alert_force()
>> should already call all connected drivers and hopefully clear
>> the alert condition on those.
> 
> Great that we are aligned now! Do you have time to rework and test the
> patch?
> 

I'll give it a try.

Guenter