[Intel-gfx] [PATCH v10 3/7] drm/i915: Define and use GuC and CTB TLB invalidation routines

[John Harrison]

On 10/10/2023 15:30, Cavitt, Jonathan wrote:
> -----Original Message-----
> From: Harrison, John C <john.c.harrison at intel.com>
> Sent: Tuesday, October 10, 2023 2:51 PM
> To: Cavitt, Jonathan <jonathan.cavitt at intel.com>; intel-gfx at lists.freedesktop.org
> Cc: Gupta, saurabhg <saurabhg.gupta at intel.com>; chris.p.wilson at linux.intel.com; Iddamsetty, Aravind <aravind.iddamsetty at intel.com>; Yang, Fei <fei.yang at intel.com>; Shyti, Andi <andi.shyti at intel.com>; Das, Nirmoy <nirmoy.das at intel.com>; Krzysztofik, Janusz <janusz.krzysztofik at intel.com>; Roper, Matthew D <matthew.d.roper at intel.com>; tvrtko.ursulin at linux.intel.com; jani.nikula at linux.intel.com
> 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.