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

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

Michal Wajdeczko <michal.wajdeczko@intel.com> writes:

> On 9/7/2026 9:30 PM, Michal Wajdeczko wrote:
>> 
>> 
>> On 9/5/2026 3:52 PM, Gustavo Sousa wrote:
>>> 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.
>> 
>> for the record: IMO this should be tagged as a pre-existing issue, as even
>> without storing a xe->subplatform_desc, tests code already could pass sentinel
>> instead of NULL to the xe_info_init_early() which could in the future make
>> some decisions based on non-null descriptor
>> 
>> anyway, fix is done in separate series [2], which shall be merged before this
>> 
>> [2] https://patchwork.freedesktop.org/series/173450/ 
>
> this is now merged [3], can we move forward with this patch?
>

Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com>

>
> [3] https://gitlab.freedesktop.org/drm/xe/kernel/-/commit/51c3ac1665265bf107d111927d242f0799ba11f8
>> 
>>>
>>> --
>>> Gustavo Sousa
>>>
>>>>
>>>> -- 
>>>> Sashiko AI review · https://sashiko.dev/#/patchset/20260904170531.516-1-michal.wajdeczko@intel.com?part=2
>>