-----Original Message-----
From: Nirmoy Das <nirmoy.das@xxxxxxxxxxxxxxx>
Sent: Wednesday, July 17, 2024 8:22 AM
To: Cavitt, Jonathan <jonathan.cavitt@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx
Cc: Gupta, saurabhg <saurabhg.gupta@xxxxxxxxx>; dan.carpenter@xxxxxxxxxx; chris.p.wilson@xxxxxxxxxxxxxxx; Andi Shyti <andi.shyti@xxxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxxx>
> > Sent: Wednesday, July 17, 2024 8:06 AM
> > To: Cavitt, Jonathan <jonathan.cavitt@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > Cc: Gupta, saurabhg <saurabhg.gupta@xxxxxxxxx>; dan.carpenter@xxxxxxxxxx; chris.p.wilson@xxxxxxxxxxxxxxx; Andi Shyti <andi.shyti@xxxxxxxxxxxxxxx>
> > 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@xxxxxxxxxx>
> >>> Signed-off-by: Jonathan Cavitt <jonathan.cavitt@xxxxxxxxx>
> >>> ---
> >>> 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.
-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++) {
>
