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
