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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7ad68f35-2e90-41b7-a95d-efe5f7db8f3b@roeck-us.net>
Date: Sun, 28 Jul 2024 17:31:54 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Wolfram Sang <wsa+renesas@...g-engineering.com>,
 Wolfram Sang <wsa@...nel.org>, Jean Delvare <khali@...ux-fr.org>,
 linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] i2c: smbus: Send alert notifications to all devices
 if source not found

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@...ck-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
>>