Re: [PATCH 1/1] drm/xe: Add null pointer check for xe_migrate_copy

["Dong, Zhanjun" <zhanjun.dong@intel.com>]

See my comments inline below.

Regards,
Zhanjun Dong

On 2024-09-18 6:35 p.m., Matthew Brost wrote:
> On Wed, Sep 18, 2024 at 03:10:00PM -0700, Zhanjun Dong wrote:
>> Add null pointer check for parameter src.
>> Update lack source flag to include resource is null case in xe_bo_move
>> before xe_migrate_copy called.
>>
>> Signed-off-by: Zhanjun Dong <zhanjun.dong@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_bo.c      |  4 ++--
>>   drivers/gpu/drm/xe/xe_migrate.c | 24 ++++++++++++++++--------
>>   2 files changed, 18 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
>> index 5f2f1ec46b57..761130f0e9a9 100644
>> --- a/drivers/gpu/drm/xe/xe_bo.c
>> +++ b/drivers/gpu/drm/xe/xe_bo.c
>> @@ -682,8 +682,8 @@ static int xe_bo_move(struct ttm_buffer_object *ttm_bo, bool evict,
>>   	tt_has_data = ttm && (ttm_tt_is_populated(ttm) ||
>>   			      (ttm->page_flags & TTM_TT_FLAG_SWAPPED));
>>   
>> -	move_lacks_source = handle_system_ccs ? (!bo->ccs_cleared)  :
>> -						(!mem_type_is_vram(old_mem_type) && !tt_has_data);
>> +	move_lacks_source = !old_mem ? true : (handle_system_ccs ? (!bo->ccs_cleared)  :
>> +					       (!mem_type_is_vram(old_mem_type) && !tt_has_data));
> 
> I'd write it like this:
> 
> old_mem || (conditional)
Sure
> 
> But I think if old_mem is NULL this condition always evaluates to true.

The NULL ptr issue is found by CI:
https://intel-gfx-ci.01.org/tree/intel-xe/xe-pw-128077v21/bat-lnl-2/igt@xe_live_ktest@xe_bo.html#dmesg-warnings5406

To get there, 2 conditions are:
src == null
And
move_lacks_source == false

> 
> - old_mem_type will be XE_PL_SYSTEM.
> - ttm should be NULL (I think), thus handle_system_ccs should be false
>    and tt_has_data should be false
> 
>>   
>>   	needs_clear = (ttm && ttm->page_flags & TTM_TT_FLAG_ZERO_ALLOC) ||
>>   		(!ttm && ttm_bo->type == ttm_bo_type_device);
>> diff --git a/drivers/gpu/drm/xe/xe_migrate.c b/drivers/gpu/drm/xe/xe_migrate.c
>> index cfd31ae49cc1..45bba0d731ec 100644
>> --- a/drivers/gpu/drm/xe/xe_migrate.c
>> +++ b/drivers/gpu/drm/xe/xe_migrate.c
>> @@ -774,14 +774,22 @@ struct dma_fence *xe_migrate_copy(struct xe_migrate *m,
>>   	u64 src_L0, dst_L0;
>>   	int pass = 0;
>>   	int err;
>> -	bool src_is_pltt = src->mem_type == XE_PL_TT;
>> -	bool dst_is_pltt = dst->mem_type == XE_PL_TT;
>> -	bool src_is_vram = mem_type_is_vram(src->mem_type);
>> -	bool dst_is_vram = mem_type_is_vram(dst->mem_type);
>> -	bool copy_ccs = xe_device_has_flat_ccs(xe) &&
>> -		xe_bo_needs_ccs_pages(src_bo) && xe_bo_needs_ccs_pages(dst_bo);
>> -	bool copy_system_ccs = copy_ccs && (!src_is_vram || !dst_is_vram);
>> -	bool use_comp_pat = xe_device_has_flat_ccs(xe) &&
>> +	bool src_is_pltt, dst_is_pltt;
>> +	bool src_is_vram, dst_is_vram;
>> +	bool copy_ccs, copy_system_ccs;
>> +	bool use_comp_pat;
>> +
>> +	if (!src)
>> +		return ERR_PTR(-EINVAL);
> 
> Can you explain if this function is called with src == NULL? That seems
> to be problem in the upper layers if that happens.
I agree, the src should not be null when this function was called.
The previous move_lacks_source change should prevent it being called.

Maybe keep the xe_migrate_copy not changed here and only keep the above 
part in xe_bo_move?

> 
> Matt
> 
>> +
>> +	src_is_pltt = src->mem_type == XE_PL_TT;
>> +	dst_is_pltt = dst->mem_type == XE_PL_TT;
>> +	src_is_vram = mem_type_is_vram(src->mem_type);
>> +	dst_is_vram = mem_type_is_vram(dst->mem_type);
>> +	copy_ccs = xe_device_has_flat_ccs(xe) && xe_bo_needs_ccs_pages(src_bo) &&
>> +		xe_bo_needs_ccs_pages(dst_bo);
>> +	copy_system_ccs = copy_ccs && (!src_is_vram || !dst_is_vram);
>> +	use_comp_pat = xe_device_has_flat_ccs(xe) &&
>>   		GRAPHICS_VER(xe) >= 20 && src_is_vram && !dst_is_vram;
>>   
>>   	/* Copying CCS between two different BOs is not supported yet. */
>> -- 
>> 2.34.1
>>