[PATCH] drm/i915: Allow NULL memory region

[Dan Carpenter]

On Wed, Jul 17, 2024 at 05:21:38PM +0200, Nirmoy Das wrote:
> 
> 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.
> 
> 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;
>                 }

Obviously you intended to change the IS_ERR() to IS_ERR_OR_NULL() but
also we want to continue instead of goto out_cleanup.

> 
> PTR_ERR(NULL) should be 0 I think and could even add a info saying
> skipping setting up that reason.

Don't print stuff on the success path.

regards,
dan carpenter