On Tue, 14 Jun 2022 09:28:14 -0700, Dixit, Ashutosh wrote:
On Thu, 02 Jun 2022 10:21:19 -0700, Zhanjun Dong wrote:
@@ -481,12 +481,14 @@ static int wait_for_ct_request_update(struct ct_request *req, u32 *status) #define GUC_CTB_RESPONSE_TIMEOUT_SHORT_MS 10 #define GUC_CTB_RESPONSE_TIMEOUT_LONG_MS 1000 #define done \
- (FIELD_GET(GUC_HXG_MSG_0_ORIGIN, READ_ONCE(req->status)) == \
- (!intel_guc_ct_enabled(ct) || FIELD_GET(GUC_HXG_MSG_0_ORIGIN, READ_ONCE(req->status)) == \ GUC_HXG_ORIGIN_GUC) err = wait_for_us(done, GUC_CTB_RESPONSE_TIMEOUT_SHORT_MS); if (err) err = wait_for(done, GUC_CTB_RESPONSE_TIMEOUT_LONG_MS);
#undef done
- if (!intel_guc_ct_enabled(ct))
err = -ECANCELED;Also, I really don't like intel_guc_ct_enabled() being called in two places. Is there a possibility that intel_guc_ct_enabled() can return false in the first place (causing the wait to exit) and then return true in the second place (so we don't return -ECANCELED)?
Is it possible to change the status of the request to something else from intel_guc_ct_disable() (or wherever ct->enabled is set to false) rather than introducing intel_guc_ct_enabled() checks here. Changing the status of the request when CT goes down would cause the wait's to exit here. And then we can check that special request status signifying CT went down?
I think there are free bits in the request status fields which can be used. But setting the request status say from intel_guc_ct_enabled() might not be straightforward since you have to locate waiting requests and also there may be multiple such requests (waiting in different threads).
Maybe an easier way might be to do something like:
bool foo(ct, req) { if (!intel_guc_ct_enabled(ct)) { req->status = CT_WENT_AWAY; return true; } return false; }
Now in your patch we can substitute foo() instead of !intel_guc_ct_enabled(ct) so that we have:
#define done \ (foo() || FIELD_GET(GUC_HXG_MSG_0_ORIGIN, READ_ONCE(req->status)) == \ GUC_HXG_ORIGIN_GUC)
And then check for req->status == CT_WENT_AWAY (most likely in ct_send()).