Re: [PATCH 2/3] drm/xe/vf: Remove lmtt->ops null check in xe_lmtt_estimate_pt_size

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

On 08.03.2024 17:22, Ghimiray, Himal Prasad wrote:
> 
> On 08-03-2024 20:22, Rodrigo Vivi wrote:
>> On Fri, Mar 08, 2024 at 10:06:50AM +0530, Himal Prasad Ghimiray wrote:
>>> In xe_lmtt_estimate_pt_size: Pointer is checked against null but then
>>> dereferenced anyway.
>> And what's the problem?
>>
>> In the line below it access beyond this pointer, so it is a fair
>> case.
> 
> The problem is even if it is NULL it will be  try to derefrence it.
> Which might lead to segmentation fault.
> 
>>
>>> Since xe_lmtt_init ensures lmtt->ops is populated
>>> remove the check.
>> With this in mind we could simply remove all the asserts in the code.

the purpose of this particular xe_assert() in xe_lmtt_estimate_pt_size()
is to express the SLA for the caller that it shall call the
xe_lmtt_init() prior to calling this function

>>
>> I believe that if someone introduced it here it is likely because
>> during some development or refactor this ended up being a problem
>> and want some earlier kind of warning with backtrace information.

true as this is the goal of all our xe_assert() to have early and clear
notification about the problem due to code refactor or new development

>>
>>> Reported by static analyzer.
>> Perhaps then replace with an
>> if (!lmtt->ops) {
>>      drm_WARN(...);
>>      return;
>> }

no, we don't want to have runtime checks in production driver against
programming errors that should be seen only during early development

> 
> I am also of the opinion that this is the correct check to have instead
> of just warning

see also [1] why we use xe_asserts instead of BUG or WARN

[1] https://docs.kernel.org/gpu/xe/xe_debugging.html

> 
> about lmtt->ops being NULL and continue to dereference it. Need clarity
> on what should we return in
> 
> case of lmtt->ops being NULL since expected return type is u64.

we shouldn't try to hide the problem with fake result as we shall never
call this function with lmtt->ops being NULL in the first place

> 
>>
>> and/or mark the tool as a false positive?!

I would make another step and just fix the tool to be run against the
production code, without xe_assert() being enabled, as all our
xe_asserts() are to some extend "redundant" with the production code,
and the tool will complain not only against extra/missed NULL checks but
also against other conditions that will look like 'not-possible' at
given code snapshot, but the purpose of xe_asserts is to protect
ourselves against future code updates/changes that current code might
not be prepared to or it has different assumptions.

Michal

>>
>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>> Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
>>> ---
>>>   drivers/gpu/drm/xe/xe_lmtt.c | 1 -
>>>   1 file changed, 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_lmtt.c b/drivers/gpu/drm/xe/xe_lmtt.c
>>> index 0d7c5514e092..d6d75414bb99 100644
>>> --- a/drivers/gpu/drm/xe/xe_lmtt.c
>>> +++ b/drivers/gpu/drm/xe/xe_lmtt.c
>>> @@ -487,7 +487,6 @@ u64 xe_lmtt_estimate_pt_size(struct xe_lmtt
>>> *lmtt, u64 size)
>>>         lmtt_assert(lmtt, IS_SRIOV_PF(lmtt_to_xe(lmtt)));
>>>       lmtt_assert(lmtt, IS_DGFX(lmtt_to_xe(lmtt)));
>>> -    lmtt_assert(lmtt, lmtt->ops);
>>>         pt_size = PAGE_ALIGN(lmtt->ops->lmtt_pte_size(level) *
>>>                    lmtt->ops->lmtt_pte_num(level));
>>> -- 
>>> 2.25.1
>>>