Re: [PATCH 2/6] drm/xe: Keep reference to device descriptor

[Gustavo Sousa <gustavo.sousa@intel.com>]

sashiko-bot@kernel.org writes:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] In test initialization, `xe->subplatform_desc` is incorrectly assigned a pointer to a sentinel element instead of `NULL` when no subplatform is matched.
> --
>
> commit 2fbc7ea3deaa915eef829a4c64ec08fac1f4a8a9
> Author: Michal Wajdeczko <michal.wajdeczko@intel.com>
>
> drm/xe: Keep reference to device descriptor
>
> This patch stores a reference to the whole device descriptor and the
> matched subplatform descriptor within the xe_device structure. This
> change enables access to additional non-mutable device parameters.
>
>> diff --git a/drivers/gpu/drm/xe/tests/xe_pci.c b/drivers/gpu/drm/xe/tests/xe_pci.c
>> index 4965a01f6ba52..48d5ee478fe69 100644
>> --- a/drivers/gpu/drm/xe/tests/xe_pci.c
>> +++ b/drivers/gpu/drm/xe/tests/xe_pci.c
>> @@ -374,6 +374,8 @@ int xe_pci_fake_device_init(struct xe_device *xe)
>>  		return -ENODEV;
>>  
>>  done:
>> +	xe->desc = desc;
>> +	xe->subplatform_desc = subplatform_desc;
>
> [Severity: Medium]
> Will this assign a pointer to a sentinel element instead of NULL when no
> subplatform is matched?
>
> In xe_pci_fake_device_init(), the search loop terminates when it reaches the
> sentinel element:
>
> 	for (subplatform_desc = desc->subplatforms;
> 	     subplatform_desc && subplatform_desc->subplatform;
> 	     subplatform_desc++)
> 		if (subplatform_desc->subplatform == data->subplatform)
> 			break;
>
> If data->subplatform is XE_SUBPLATFORM_NONE, the loop exits with
> subplatform_desc pointing to the sentinel element of desc->subplatforms, not
> NULL. The subsequent check (!subplatform_desc) evaluates to false.
>
> By directly assigning this value to xe->subplatform_desc, the test mock
> environment now gets a non-NULL pointer (the sentinel) for devices without a
> subplatform. Could this create a state inconsistency with production code,
> causing tests checking if (xe->subplatform_desc) to falsely evaluate to
> true?

Sashiko seems to be right here. I think we could initialize
subplatform_desc to NULL and assign the one from the loop if we find a
match.

--
Gustavo Sousa

>
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260904170531.516-1-michal.wajdeczko@intel.com?part=2