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

["Piotr Piórkowski" <piotr.piorkowski@intel.com>]

Michal Wajdeczko <michal.wajdeczko@intel.com> wrote on pon [2024-kwi-08 11:06:27 +0200]:
> 
> 
> 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)

Yes, you are right that the other pointers are also not checked.
However, we have a small difference here:
If someone in the future forgets to set program_media or program_graphics,
and he tests it on a newly added platform, he will quickly find out about it
during device probe, because it is called in:
xe_device_probe() -> xe_gt_init_hwconfig() -> xe_pat_init()
The dump() is called only in dump_pat_on_error() or in debugfs.
So in the case of dump(), there is a higher chance that the end user has face
a NULL pointer. And I think we prefer to avoid the NULL pointer (even more so
in this case, where dump is not a critical functionality).

[Intentionally I'm ignoring the fact here that I guess we have tests checking debugfs]

Piotr

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

--