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: <3adf0b8f-2e12-413a-a76f-866e56bf096c@roeck-us.net>
Date: Mon, 29 Jul 2024 07:23:57 -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/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