[PATCH] drm/i915: Allow NULL memory region

[Nirmoy Das]

On 7/17/2024 5:30 PM, Cavitt, Jonathan wrote:
> -----Original Message-----
> From: Nirmoy Das <nirmoy.das at linux.intel.com>
> Sent: Wednesday, July 17, 2024 8:22 AM
> To: Cavitt, Jonathan <jonathan.cavitt at intel.com>; intel-gfx at lists.freedesktop.org
> Cc: Gupta, saurabhg <saurabhg.gupta at intel.com>; dan.carpenter at linaro.org; chris.p.wilson at linux.intel.com; Andi Shyti <andi.shyti at linux.intel.com>
> Subject: Re: [PATCH] drm/i915: Allow NULL memory region
>>
>> On 7/17/2024 5:11 PM, Cavitt, Jonathan wrote:
>>> -----Original Message-----
>>> From: Nirmoy Das <nirmoy.das at linux.intel.com>
>>> Sent: Wednesday, July 17, 2024 8:06 AM
>>> To: Cavitt, Jonathan <jonathan.cavitt at intel.com>; intel-gfx at lists.freedesktop.org
>>> Cc: Gupta, saurabhg <saurabhg.gupta at intel.com>; dan.carpenter at linaro.org; chris.p.wilson at linux.intel.com; Andi Shyti <andi.shyti at linux.intel.com>
>>> Subject: Re: [PATCH] drm/i915: Allow NULL memory region
>>>> On 7/12/2024 11:41 PM, Jonathan Cavitt wrote:
>>>>> Prevent a NULL pointer access in intel_memory_regions_hw_probe.
>>>>>
>>>>> Fixes: 05da7d9f717b ("drm/i915/gem: Downgrade stolen lmem setup warning")
>>>>> Reported-by: Dan Carpenter <dan.carpenter at linaro.org>
>>>>> Signed-off-by: Jonathan Cavitt <jonathan.cavitt at intel.com>
>>>>> ---
>>>>>     drivers/gpu/drm/i915/intel_memory_region.c | 6 ++++--
>>>>>     1 file changed, 4 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/intel_memory_region.c b/drivers/gpu/drm/i915/intel_memory_region.c
>>>>> index 172dfa7c3588b..d40ee1b42110a 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_memory_region.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_memory_region.c
>>>>> @@ -368,8 +368,10 @@ int intel_memory_regions_hw_probe(struct drm_i915_private *i915)
>>>>>     			goto out_cleanup;
>>>>>     		}
>>>>>     
>>>>> -		mem->id = i;
>>>>> -		i915->mm.regions[i] = mem;
>>>> There is a check for mem just before that. You could use
>>>> IS_ERR_OR_NULL(mem) instead of IS_ERR().
>>> I think you're referring to the "goto out_cleanup" path?
>> Yes.
>>
>>> mem being NULL is a valid use case, so we
>>> shouldn't take the error path when it's observed.
>> Not an error path if you return expected/correct value.
> intel_memory_regions_driver_release releases all previously
> grabbed memory regions in the out_cleanup path.


Ah, yes. Isn't so simple as I thought.  Never mind ignore my previous 
comment.

> -Jonathan Cavitt
>
>> You could do
>> diff --git a/drivers/gpu/drm/i915/intel_memory_region.c b/drivers/gpu/drm/i915/intel_memory_region.c
>> index 172dfa7c3588..41ef7fdfa69b 100644
>> --- a/drivers/gpu/drm/i915/intel_memory_region.c
>> +++ b/drivers/gpu/drm/i915/intel_memory_region.c
>> @@ -362,9 +362,10 @@ int intel_memory_regions_hw_probe(struct drm_i915_private *i915)
>>
>>                   if (IS_ERR(mem)) {
>>                           err = PTR_ERR(mem);
>> -                       drm_err(&i915->drm,
>> -                               "Failed to setup region(%d) type=%d\n",
>> -                               err, type);
>> +                       if (err)
>> +                               drm_err(&i915->drm,
>> +                                       "Failed to setup region(%d) type=%d\n",
>> +                                       err, type);
>>                           goto out_cleanup;
>>                   }
>>
>> PTR_ERR(NULL) should be 0 I think and could even add a info saying skipping setting up that reason.
>>
>> Regards,
>> Nirmoy
>>
>>> -Jonathan Cavitt
>>>
>>>> Regards,
>>>>
>>>> Nirmoy
>>>>
>>>>> +		if (mem) { /* Skip on non-fatal errors */
>>>>> +			mem->id = i;
>>>>> +			i915->mm.regions[i] = mem;
>>>>> +		}
>>>>>     	}
>>>>>     
>>>>>     	for (i = 0; i < ARRAY_SIZE(i915->mm.regions); i++) {