Hi Zhanjun,
On Fri, Aug 11, 2023 at 11:20:11AM -0700, Zhanjun Dong wrote:
> This attempts to avoid circular locking dependency between flush delayed
> work and intel_gt_reset.
> When intel_gt_reset was called, task will hold a lock.
> To cacel delayed work here, the _sync version will also acquire a lock,
/cacel/cancel
> which might trigger the possible cirular locking dependency warning.
> When intel_gt_reset called, reset_in_progress flag will be set, add code
> to check the flag, call async verion if reset is in progress.
/verion/version/
> Signed-off-by: Zhanjun Dong <zhanjun.dong@xxxxxxxxx>
> Cc: John Harrison <John.C.Harrison@xxxxxxxxx>
> Cc: Andi Shyti <andi.shyti@xxxxxxxxxxxxxxx>
> Cc: Daniel Vetter <daniel@xxxxxxxx>
> ---
There is no changelog here :/
Can you please add the changelog after the '---' section?
The commit log has changed and...
> drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> index a0e3ef1c65d2..600388c849f7 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> @@ -1359,7 +1359,16 @@ static void guc_enable_busyness_worker(struct intel_guc *guc)
>
> static void guc_cancel_busyness_worker(struct intel_guc *guc)
> {
> - cancel_delayed_work_sync(&guc->timestamp.work);
> + /*
> + * When intel_gt_reset was called, task will hold a lock.
> + * To cacel delayed work here, the _sync version will also acquire a lock, which might
> + * trigger the possible cirular locking dependency warning.
> + * Check the reset_in_progress flag, call async verion if reset is in progress.
> + */
> + if (guc_to_gt(guc)->uc.reset_in_progress)
> + cancel_delayed_work(&guc->timestamp.work);
> + else
> + cancel_delayed_work_sync(&guc->timestamp.work);
... now you are checking out of reset_in_progress.
Normally the convention here is to have the *_locked() version of
the function. But I'm OK with this, as well... John, any opinion?
Anyway, comparing with your previous patch the decision is made
out of different elements and only __reset_guc_busyness_stats()
needed this change.
Andi
> }
>
> static void __reset_guc_busyness_stats(struct intel_guc *guc)
> --
> 2.34.1
