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

[Guenter Roeck <linux@roeck-us.net>]

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