Re: [bug report] drm/xe: Reset VMA attributes to default in SVM garbage collector

["Ghimiray, Himal Prasad" <himal.prasad.ghimiray@intel.com>]

On 28-08-2025 19:58, Dan Carpenter wrote:
> On Thu, Aug 28, 2025 at 04:18:11PM +0530, Ghimiray, Himal Prasad wrote:
>>
>>
>> On 28-08-2025 16:02, Dan Carpenter wrote:
>>> Hello Himal Prasad Ghimiray,
>>>
>>> Commit a2eb8aec3ebe ("drm/xe: Reset VMA attributes to default in SVM
>>> garbage collector") from Aug 21, 2025 (linux-next), leads to the
>>> following Smatch static checker warning:
>>>
>>> 	drivers/gpu/drm/xe/xe_vm.c:4442 xe_vm_alloc_vma()
>>> 	error: uninitialized symbol 'default_pat'.
>>>
>>> drivers/gpu/drm/xe/xe_vm.c
>>>       4396 static int xe_vm_alloc_vma(struct xe_vm *vm,
>>>       4397                            struct drm_gpuvm_map_req *map_req,
>>>       4398                            bool is_madvise)
>>>       4399 {
>>>       4400         struct xe_vma_ops vops;
>>>       4401         struct drm_gpuva_ops *ops = NULL;
>>>       4402         struct drm_gpuva_op *__op;
>>>       4403         bool is_cpu_addr_mirror = false;
>>>       4404         bool remap_op = false;
>>>       4405         struct xe_vma_mem_attr tmp_attr;
>>>       4406         u16 default_pat;
>>>       4407         int err;
>>>       4408
>>>       4409         lockdep_assert_held_write(&vm->lock);
>>>       4410
>>>       4411         if (is_madvise)
>>>       4412                 ops = drm_gpuvm_madvise_ops_create(&vm->gpuvm, map_req);
>>>       4413         else
>>>       4414                 ops = drm_gpuvm_sm_map_ops_create(&vm->gpuvm, map_req);
>>>       4415
>>>       4416         if (IS_ERR(ops))
>>>       4417                 return PTR_ERR(ops);
>>>       4418
>>>       4419         if (list_empty(&ops->list)) {
>>>       4420                 err = 0;
>>>       4421                 goto free_ops;
>>>       4422         }
>>>       4423
>>>       4424         drm_gpuva_for_each_op(__op, ops) {
>>>       4425                 struct xe_vma_op *op = gpuva_op_to_vma_op(__op);
>>>       4426                 struct xe_vma *vma = NULL;
>>>       4427
>>>       4428                 if (!is_madvise) {
>>>       4429                         if (__op->op == DRM_GPUVA_OP_UNMAP) {
>                                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
>>>       4430                                 vma = gpuva_to_vma(op->base.unmap.va);
>>>       4431                                 XE_WARN_ON(!xe_vma_has_default_mem_attrs(vma));
>>>       4432                                 default_pat = vma->attr.default_pat_index;
>>>       4433                         }
>>>       4434
>>>       4435                         if (__op->op == DRM_GPUVA_OP_REMAP) {
>                                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
>>>       4436                                 vma = gpuva_to_vma(op->base.remap.unmap->va);
>>>       4437                                 default_pat = vma->attr.default_pat_index;
>>>       4438                         }
>>>       4439
>>>       4440                         if (__op->op == DRM_GPUVA_OP_MAP) {
>>>       4441                                 op->map.is_cpu_addr_mirror = true;
>>> --> 4442                                 op->map.pat_index = default_pat;
>>>
>>> Maybe we *know* the __ops are always in a specifc order so
>>> REMAP and UNMAP come before MAP??
>>
>> True MAP is always follwed by REMAP.
> 
> s/followed by/preceeded by/...  

yes it should be preceeded by.
For this to avoid an uninitialized
> variable bug we first have to process the UNMAP or REMAP before we
> process the MAP.  The more I look at this the  more if feels like a
> bug, but okay...

Your analysis is right. REMAP or UNMAP will always happen first, which 
will set up the default_pat to be used by MAP. So the default_pat will 
always have sane value for MAP.

> 
> regards,
> dan carpenter
>