Re: [PATCH 4/5] drm/xe: Guard against NULL GT in xe_guc.c

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

On 9/26/2025 9:26 PM, Matt Roper wrote:
> On Fri, Sep 26, 2025 at 08:36:31PM +0200, Michal Wajdeczko wrote:
>>
>>
>> On 9/26/2025 5:59 PM, Jonathan Cavitt wrote:
>>> Static analysis reveals the following issue:
>>> xe_device_get_gt is theoretically able to return NULL in some cases, but
>>> several use cases don't check the return value before performing a
>>> dereference, resulting in a NULL pointer dereference.
>>>
>>> Use xe_root_mmio_gt instead of xe_device_get_gt to prevent this in
>>> xe_guc.c
>>>
>>> Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
>>> ---
>>>  drivers/gpu/drm/xe/xe_guc.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_guc.c b/drivers/gpu/drm/xe/xe_guc.c
>>> index d5adbbb013ec..0bf1402c61a0 100644
>>> --- a/drivers/gpu/drm/xe/xe_guc.c
>>> +++ b/drivers/gpu/drm/xe/xe_guc.c
>>> @@ -466,7 +466,7 @@ static int guc_g2g_alloc(struct xe_guc *guc)
>>>  		return 0;
>>>  
>>>  	if (gt->info.id != 0) {
>>> -		struct xe_gt *root_gt = xe_device_get_gt(xe, 0);
>>> +		struct xe_gt *root_gt = xe_root_mmio_gt(xe);
>>
>> while this looks ok today, I'm not sure how this will work after [1]
>>
>> maybe we should check and abort here? + @Matt
> 
> On today's driver, xe_device_get_gt(xe, 0); can never return NULL.
> Hardware-wise there's always at least one tile, and every tile has a
> primary GT.  If something went wrong during init of the tile or GT and
> we couldn't create/initialize the structures, then we already aborted
> the device probe immediately and we'll never get further on to places in
> the code that would be chasing a NULL pointer.
> 
> As Michal pointed out, there's work to change this (i.e., allow the
> primary GT to be disabled via configfs for debugging purposes), but even
> with the referenced series we're not at the point where that can happen
> yet.  More patches will need to be written and land before we relax the
> probe-time checks and allow probe without a primary GT.  Once that
> happens, xe_device_get_gt(xe, 0) and xe_root_mmio_gt(xe) are both
> equally problematic, so replacing one with the other doesn't really
> help.

maybe we should have two helpers

* one that never fails - to be used when are sure that given GT exists
* other that may return NULL - in case we don't know if ID is valid

	/**
	 * @gtid: the GT ID of the valid GT to get
	 * Return: asserted pointer to &xe_gt
	 */
	xe_device_get_gt(xe, gtid)
vs
	/**
	 * @gtid: the GT ID to lookup
	 * Return: asserted pointer to &xe_gt or NULL if not found
	 */
	xe_device_lookup_gt(xe, gtid)

> 
> 
> Matt
> 
>>
>> [1] https://patchwork.freedesktop.org/series/154739/
>>
>>>  		struct xe_guc *root_guc = &root_gt->uc.guc;
>>>  		struct xe_bo *bo;
>>>  
>>
>