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@xxxxxxxxxxxx>
> ---
> 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:
signature.asc
Description: PGP signature
