On Thu, 2023-09-14 at 11:34 -0400, Vivi, Rodrigo wrote: > On Sat, Sep 09, 2023 at 08:58:45PM -0700, Alan Previn wrote: alan:snip > > > + /* Change context state to destroyed and get gt-pm */ > > + __intel_gt_pm_get(gt); > > + set_context_destroyed(ce); > > + clr_context_registered(ce); > > + > > + ret = deregister_context(ce, ce->guc_id.id); > > + if (ret) { > > + /* Undo the state change and put gt-pm if that failed */ > > + set_context_registered(ce); > > + clr_context_destroyed(ce); > > + intel_gt_pm_put(gt); > > This is a might_sleep inside a spin_lock! Are you 100% confident no WARN > was seeing during the tests indicated in the commit msg? > > > + } > > + spin_unlock_irqrestore(&ce->guc_state.lock, flags); > > + > > + return 0; > > If you are always returning 0, there's no pointing in s/void/int... alan: Hi Rodrigo - i actually forget that i need the error value returned so that _guc_context_destroy can put the context back into the guc->submission_state.destroyed_contexts linked list. So i clearly missed returning the error in the case deregister_context fails. > > > } > > > > static void __guc_context_destroy(struct intel_context *ce) > > @@ -3268,7 +3296,22 @@ static void deregister_destroyed_contexts(struct intel_guc *guc) > > if (!ce) > > break; > > > > - guc_lrc_desc_unpin(ce); > > + if (guc_lrc_desc_unpin(ce)) { > > + /* > > + * This means GuC's CT link severed mid-way which could happen > > + * in suspend-resume corner cases. In this case, put the > > + * context back into the destroyed_contexts list which will > > + * get picked up on the next context deregistration event or > > + * purged in a GuC sanitization event (reset/unload/wedged/...). > > + */ > > + spin_lock_irqsave(&guc->submission_state.lock, flags); > > + list_add_tail(&ce->destroyed_link, > > + &guc->submission_state.destroyed_contexts); > > + spin_unlock_irqrestore(&guc->submission_state.lock, flags); alan:snip