Re: [PATCH] drm/xe: Check pat.ops before dumping PAT settings

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

On 08.04.2024 09:23, Piotr Piórkowski wrote:
> Michal Wajdeczko <michal.wajdeczko@intel.com> wrote on pią [2024-kwi-05 16:36:25 +0200]:
>> We may leave pat.ops unset when running on brand new platform or
>> when running as a VF.  While the former is unlikely, the latter
>> is valid (future) use case and will cause NPD when someone will
>> try to dump PAT settings by debugfs.
>>
>> It's better to check pointer to pat.ops instead of specific .dump
>> hook, as we have this hook always defined for every .ops variant.
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> ---
>>  drivers/gpu/drm/xe/xe_pat.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_pat.c b/drivers/gpu/drm/xe/xe_pat.c
>> index 66d8e3dd8237..f0031c2e9818 100644
>> --- a/drivers/gpu/drm/xe/xe_pat.c
>> +++ b/drivers/gpu/drm/xe/xe_pat.c
>> @@ -447,7 +447,7 @@ void xe_pat_dump(struct xe_gt *gt, struct drm_printer *p)
>>  {
>>  	struct xe_device *xe = gt_to_xe(gt);
>>  
>> -	if (!xe->pat.ops->dump)
>> +	if (!xe->pat.ops)
> 
> You are right that we currently have a dump pointer set for each xe_pat_ops structure,
> and in this situation it is enough to check the ops for the cases you listed.
> But I assume that since we are verifying the dump pointer here, that formally, for some
> future case, we may not set this pointer.
> Therefore, it seems to me that it would be more correct for you to check both pointers
> here: ops and dump.

that was also my first choice but after looking at xe_pat_init() and
reviewing existing ops that choice didn't hold as IMO keeping runtime
check for future potential lack of .dump hook is very questionable

what I was considered instead was to add to xe_pat_init_early():

	xe_assert(xe, !xe->pat.ops || xe->pat.ops.dump);

to perform early checkout of the selected .ops and make sure that we
didn't miss to setup .dump hoot but then realized that the other hook
.program_media is used without any extra runtime or debug check while
some .ops may have it unset. so finally decided to just go with quick
fix to close existing gap, postpone further fixes to follow up series
(that likely could be done by the PAT code owners)

Michal

> 
> Thanks,
> Piotr
> 
>>  		return;
>>  
>>  	xe->pat.ops->dump(gt, p);
>> -- 
>> 2.43.0
>>
>