On 10/10/2023 15:30, Cavitt, Jonathan wrote:
-----Original Message-----
From: Harrison, John C <john.c.harrison@xxxxxxxxx>
Sent: Tuesday, October 10, 2023 2:51 PM
To: Cavitt, Jonathan <jonathan.cavitt@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx
Cc: Gupta, saurabhg <saurabhg.gupta@xxxxxxxxx>; chris.p.wilson@xxxxxxxxxxxxxxx; Iddamsetty, Aravind <aravind.iddamsetty@xxxxxxxxx>; Yang, Fei <fei.yang@xxxxxxxxx>; Shyti, Andi <andi.shyti@xxxxxxxxx>; Das, Nirmoy <nirmoy.das@xxxxxxxxx>; Krzysztofik, Janusz <janusz.krzysztofik@xxxxxxxxx>; Roper, Matthew D <matthew.d.roper@xxxxxxxxx>; tvrtko.ursulin@xxxxxxxxxxxxxxx; jani.nikula@xxxxxxxxxxxxxxx
Subject: Re: [PATCH v10 3/7] drm/i915: Define and use GuC and CTB TLB invalidation routines
On 10/10/2023 08:02, Jonathan Cavitt wrote:
...
+static void fini_tlb_lookup(struct intel_guc *guc)
+{
+ struct intel_guc_tlb_wait *wait;
+
+ if (!HAS_GUC_TLB_INVALIDATION(guc_to_gt(guc)->i915))
+ return;
+
+ wait = xa_load(&guc->tlb_lookup, guc->serial_slot);
+ kfree(wait);
There was originally a error being printed if wait->busy was still set,
i.e. someone was still waiting on the object that is about to be
destroyed. There were review comments about that being broken in an
intermediate patch set. I don't recall seeing any explanation as to why
the error message should be completely removed.
The GEM_BUG_ON was downgraded to a debug message in an intermediate step
at the request of one of the reviewers (this was a version 8 change, IIRC).
We concluded that if the execution of the system was not impacted by the debug
path, we shouldn't bother with the debug message at all. So we removed it.
I think it was Fei or Andi that suggested it?
-Jonathan Cavitt
I recall it was me that said it should be an error message rather than a
BUG_ON. And my point is that I don't see how this is a 'debug path'. If
a waiter is still waiting on the wait object that is about to be freed
then that is a potential dangling pointer dereference. That totally has
the possibility to impact execution of the system.
John.
