Re: [PATCH] drm/i915: Allow NULL memory region

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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.

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++) {



[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux