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

[Wolfram Sang <wsa+renesas@sang-engineering.com>]

[-- 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 --]