On 07/25/2013 07:16 PM, Zheng, Lv wrote:
If I understand this correctly, the problem would be if:
rem_time = wait_for_completion_timeout(&tx_msg->tx_complete,
IPMI_TIMEOUT);
returns on a timeout, then checks msg_done and races with something setting
msg_done. If that is the case, you would need the smp_rmb() before checking
msg_done.
However, the timeout above is unnecessary. You are using
ipmi_request_settime(), so you can set the timeout when the IPMI command
fails and returns a failure message. The driver guarantees a return message
for each request. Just remove the timeout from the completion, set the
timeout and retries in the ipmi request, and the completion should handle the
barrier issues.
It's just difficult for me to determine retry count and timeout value, maybe retry=0, timeout=IPMI_TIMEOUT is OK.
The code of the timeout completion is already there, I think the quick fix code should not introduce this logic.
I'll add a new patch to apply your comment.
Since it is a local BMC, I doubt a retry is required. That is probably
fine. Or you could set retry=1 and timeout=IPMI_TIMEOUT/2 if you wanted
to be more sure, but I doubt it would make a difference. The only time
you really need to worry about retries is if you are resetting the BMC
or it is being overloaded.
Plus, from a quick glance at the code, it doesn't look like it will properly handle a
situation where the timeout occurs and is handled then the response comes in
later.
PATCH 07 fixed this issue.
Here we just need the smp_rmb() or holding tx_msg_lock() around the acpi_format_ipmi_response().
If you apply the fix like I suggest, then the race goes away. If
there's no timeout and it just waits for the completion, things get a
lot simpler.
Thanks for commenting.
No problem, thanks for working on this.
-corey
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
