| Message ID | 20240712214156.3969584-1-jonathan.cavitt@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Allow NULL memory region | expand |
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@linaro.org> > Signed-off-by: Jonathan Cavitt <jonathan.cavitt@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(). 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++) {
-----Original Message----- From: Nirmoy Das <nirmoy.das@linux.intel.com> Sent: Wednesday, July 17, 2024 8:06 AM To: Cavitt, Jonathan <jonathan.cavitt@intel.com>; intel-gfx@lists.freedesktop.org Cc: Gupta, saurabhg <saurabhg.gupta@intel.com>; dan.carpenter@linaro.org; chris.p.wilson@linux.intel.com; Andi Shyti <andi.shyti@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@linaro.org> > > Signed-off-by: Jonathan Cavitt <jonathan.cavitt@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? mem being NULL is a valid use case, so we shouldn't take the error path when it's observed. -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++) { >
On 7/17/2024 5:11 PM, Cavitt, Jonathan wrote: > -----Original Message----- > From: Nirmoy Das <nirmoy.das@linux.intel.com> > Sent: Wednesday, July 17, 2024 8:06 AM > To: Cavitt, Jonathan <jonathan.cavitt@intel.com>; intel-gfx@lists.freedesktop.org > Cc: Gupta, saurabhg <saurabhg.gupta@intel.com>; dan.carpenter@linaro.org; chris.p.wilson@linux.intel.com; Andi Shyti <andi.shyti@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@linaro.org> >>> Signed-off-by: Jonathan Cavitt <jonathan.cavitt@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; } 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++) {
On Wed, Jul 17, 2024 at 05:05:55PM +0200, Nirmoy Das wrote: > > 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@linaro.org> > > Signed-off-by: Jonathan Cavitt <jonathan.cavitt@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(). An error pointer return is normally completely different from a NULL return in how it's handled. Here NULL is a special kind of success. I wrote a blog about this. https://staticthinking.wordpress.com/2022/08/01/mixing-error-pointers-and-null/ regards, 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@linux.intel.com> > > Sent: Wednesday, July 17, 2024 8:06 AM > > To: Cavitt, Jonathan <jonathan.cavitt@intel.com>; intel-gfx@lists.freedesktop.org > > Cc: Gupta, saurabhg <saurabhg.gupta@intel.com>; dan.carpenter@linaro.org; chris.p.wilson@linux.intel.com; Andi Shyti <andi.shyti@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@linaro.org> > > > > Signed-off-by: Jonathan Cavitt <jonathan.cavitt@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
-----Original Message----- From: Nirmoy Das <nirmoy.das@linux.intel.com> Sent: Wednesday, July 17, 2024 8:22 AM To: Cavitt, Jonathan <jonathan.cavitt@intel.com>; intel-gfx@lists.freedesktop.org Cc: Gupta, saurabhg <saurabhg.gupta@intel.com>; dan.carpenter@linaro.org; chris.p.wilson@linux.intel.com; Andi Shyti <andi.shyti@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@linux.intel.com> > > Sent: Wednesday, July 17, 2024 8:06 AM > > To: Cavitt, Jonathan <jonathan.cavitt@intel.com>; intel-gfx@lists.freedesktop.org > > Cc: Gupta, saurabhg <saurabhg.gupta@intel.com>; dan.carpenter@linaro.org; chris.p.wilson@linux.intel.com; Andi Shyti <andi.shyti@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@linaro.org> > >>> Signed-off-by: Jonathan Cavitt <jonathan.cavitt@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. -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++) { >
On 7/17/2024 5:30 PM, Cavitt, Jonathan wrote: > -----Original Message----- > From: Nirmoy Das <nirmoy.das@linux.intel.com> > Sent: Wednesday, July 17, 2024 8:22 AM > To: Cavitt, Jonathan <jonathan.cavitt@intel.com>; intel-gfx@lists.freedesktop.org > Cc: Gupta, saurabhg <saurabhg.gupta@intel.com>; dan.carpenter@linaro.org; chris.p.wilson@linux.intel.com; Andi Shyti <andi.shyti@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@linux.intel.com> >>> Sent: Wednesday, July 17, 2024 8:06 AM >>> To: Cavitt, Jonathan <jonathan.cavitt@intel.com>; intel-gfx@lists.freedesktop.org >>> Cc: Gupta, saurabhg <saurabhg.gupta@intel.com>; dan.carpenter@linaro.org; chris.p.wilson@linux.intel.com; Andi Shyti <andi.shyti@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@linaro.org> >>>>> Signed-off-by: Jonathan Cavitt <jonathan.cavitt@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++) {
On 7/17/2024 5:25 PM, Dan Carpenter wrote: > On Wed, Jul 17, 2024 at 05:05:55PM +0200, Nirmoy Das wrote: >> 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@linaro.org> >>> Signed-off-by: Jonathan Cavitt <jonathan.cavitt@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(). > An error pointer return is normally completely different from a NULL > return in how it's handled. intel_memory_regions_driver_release() skipped my eyes in the cleanup path. > Here NULL is a special kind of success. I > wrote a blog about this. > > https://staticthinking.wordpress.com/2022/08/01/mixing-error-pointers-and-null/ I am the perfect target audience for this blog post :) Thanks, Nirmoy > > regards, > dan carpenter
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@linaro.org> > Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com> Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11704 Reviewed-by: Nirmoy Das <nirmoy.das@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; > + 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++) {
Hi Jonathan, On Fri, Jul 12, 2024 at 02:41:56PM -0700, 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@linaro.org> > Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com> Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> Thanks, Andi
Hi Jonathan, On Fri, Jul 12, 2024 at 02:41:56PM -0700, 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@linaro.org> > Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com> because this is a memory related patch I pushed it in drm-intel-gt-next. I also added the Closes tag as suggested by Nirmoy. Thanks, Andi
Hi, On 12/07/2024 22:41, Jonathan Cavitt wrote: > Prevent a NULL pointer access in intel_memory_regions_hw_probe. For future reference please include some impact assessment in patches tagged as fixes. Makes maintainers job, and even anyone's who tries to backport stuff to stable at some future date, much easier if it is known how important is the fix and in what circumstances can the problem it is fixing trigger. Regards, Tvrtko > Fixes: 05da7d9f717b ("drm/i915/gem: Downgrade stolen lmem setup warning") > Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > Signed-off-by: Jonathan Cavitt <jonathan.cavitt@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; > + 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++) {
On Thu, Jul 25, 2024 at 08:48:35AM +0100, Tvrtko Ursulin wrote: > > Hi, > > On 12/07/2024 22:41, Jonathan Cavitt wrote: > > Prevent a NULL pointer access in intel_memory_regions_hw_probe. > > For future reference please include some impact assessment in patches tagged > as fixes. Makes maintainers job, and even anyone's who tries to backport > stuff to stable at some future date, much easier if it is known how > important is the fix and in what circumstances can the problem it is fixing > trigger. > As someone doing backport work, I think this patch is fine. Everyone knows the impact of a NULL dereference in probe(). I guess with patches that add NULL dereferences, the trick is understanding when people are adding NULL checks to make a static checker happy or when it's a real bug. But the fault lies with the people adding NULL checks just to make the tools happy. Some of these pointless NULL checks end up in stable, but it's fine, extra NULL checks never hurt anyone. If the maintainer wants to be extra safe by adding NULL checks then who are we to say otherwise. In other words normal patches shouldn't have to say. "I'm not lying" at the end. It should be the pointless patches which say, "I'm doing a pointless thing. Don't bother backporting." Most stable patch backports are done automatically and people have various tools and scripts to do that. If the tools don't handle this patch automatically then they are defective. regards, dan carpenter
On Thu, Jul 25, 2024 at 10:58:08AM -0500, Dan Carpenter wrote: > On Thu, Jul 25, 2024 at 08:48:35AM +0100, Tvrtko Ursulin wrote: > > > > Hi, > > > > On 12/07/2024 22:41, Jonathan Cavitt wrote: > > > Prevent a NULL pointer access in intel_memory_regions_hw_probe. > > > > For future reference please include some impact assessment in patches tagged > > as fixes. Makes maintainers job, and even anyone's who tries to backport > > stuff to stable at some future date, much easier if it is known how > > important is the fix and in what circumstances can the problem it is fixing > > trigger. > > > > As someone doing backport work, I think this patch is fine. Everyone > knows the impact of a NULL dereference in probe(). > > I guess with patches that add NULL dereferences, the trick is s/dereferences/checks/ > understanding when people are adding NULL checks to make a static > checker happy or when it's a real bug. regards, dan carpenter
On 25/07/2024 16:58, Dan Carpenter wrote: > On Thu, Jul 25, 2024 at 08:48:35AM +0100, Tvrtko Ursulin wrote: >> >> Hi, >> >> On 12/07/2024 22:41, Jonathan Cavitt wrote: >>> Prevent a NULL pointer access in intel_memory_regions_hw_probe. >> >> For future reference please include some impact assessment in patches tagged >> as fixes. Makes maintainers job, and even anyone's who tries to backport >> stuff to stable at some future date, much easier if it is known how >> important is the fix and in what circumstances can the problem it is fixing >> trigger. >> > > As someone doing backport work, I think this patch is fine. Everyone > knows the impact of a NULL dereference in probe(). > > I guess with patches that add NULL dereferences, the trick is > understanding when people are adding NULL checks to make a static > checker happy or when it's a real bug. But the fault lies with the > people adding NULL checks just to make the tools happy. Some of these > pointless NULL checks end up in stable, but it's fine, extra NULL checks > never hurt anyone. If the maintainer wants to be extra safe by adding > NULL checks then who are we to say otherwise. > > In other words normal patches shouldn't have to say. "I'm not lying" at > the end. It should be the pointless patches which say, "I'm doing a > pointless thing. Don't bother backporting." > > Most stable patch backports are done automatically and people have > various tools and scripts to do that. If the tools don't handle this > patch automatically then they are defective. Right, and every few releases maintainers and authors get a bunch of emails for patches which did not apply to some stable tree. In which case someone has to do manual work and then it is good to know how important it is to backport something. For cases when it is not trivial. It does not apply to this patch, but as a _best practice_ it is good if the commit message explains the impacted platforms and scenarios. In this case I can follow the Fixes: tag and see the fix that this patches fixes is only about ATS-M. Which if it was a more complicated patch might be a reason to not need bother backporting past some kernel version where platform X wasn't even supported. Therefore I think my point is that best practice is to include this the commit text, so any future maintainer/backporter does not have to re-do detective work, stands. Regards, Tvrtko
On Fri, Jul 26, 2024 at 09:17:20AM +0100, Tvrtko Ursulin wrote: > > On 25/07/2024 16:58, Dan Carpenter wrote: > > On Thu, Jul 25, 2024 at 08:48:35AM +0100, Tvrtko Ursulin wrote: > > > > > > Hi, > > > > > > On 12/07/2024 22:41, Jonathan Cavitt wrote: > > > > Prevent a NULL pointer access in intel_memory_regions_hw_probe. > > > > > > For future reference please include some impact assessment in patches tagged > > > as fixes. Makes maintainers job, and even anyone's who tries to backport > > > stuff to stable at some future date, much easier if it is known how > > > important is the fix and in what circumstances can the problem it is fixing > > > trigger. > > > > > > > As someone doing backport work, I think this patch is fine. Everyone > > knows the impact of a NULL dereference in probe(). > > > > I guess with patches that add NULL dereferences, the trick is > > understanding when people are adding NULL checks to make a static > > checker happy or when it's a real bug. But the fault lies with the > > people adding NULL checks just to make the tools happy. Some of these > > pointless NULL checks end up in stable, but it's fine, extra NULL checks > > never hurt anyone. If the maintainer wants to be extra safe by adding > > NULL checks then who are we to say otherwise. > > > > In other words normal patches shouldn't have to say. "I'm not lying" at > > the end. It should be the pointless patches which say, "I'm doing a > > pointless thing. Don't bother backporting." > > > > Most stable patch backports are done automatically and people have > > various tools and scripts to do that. If the tools don't handle this > > patch automatically then they are defective. > > Right, and every few releases maintainers and authors get a bunch of emails > for patches which did not apply to some stable tree. > I believe these emails are only sent for commits that are tagged for stable. For AUTOSEL patches, the backporting is done on a best effort basis. On the other hand, hopefully this patch would have been tagged for stable if we hadn't fixed the bug so quickly. > In which case someone has to do manual work and then it is good to know how > important it is to backport something. For cases when it is not trivial. It > does not apply to this patch, but as a _best practice_ it is good if the > commit message explains the impacted platforms and scenarios. > > In this case I can follow the Fixes: tag and see the fix that this patches > fixes is only about ATS-M. Which if it was a more complicated patch might be > a reason to not need bother backporting past some kernel version where > platform X wasn't even supported. > > Therefore I think my point is that best practice is to include this the > commit text, so any future maintainer/backporter does not have to re-do > detective work, stands. This is a really elaborate hypothetical. Are there kernels which are affected by this bug but don't support ATS-M? regards, dan carpenter
On 26/07/2024 18:00, Dan Carpenter wrote: > On Fri, Jul 26, 2024 at 09:17:20AM +0100, Tvrtko Ursulin wrote: >> >> On 25/07/2024 16:58, Dan Carpenter wrote: >>> On Thu, Jul 25, 2024 at 08:48:35AM +0100, Tvrtko Ursulin wrote: >>>> >>>> Hi, >>>> >>>> On 12/07/2024 22:41, Jonathan Cavitt wrote: >>>>> Prevent a NULL pointer access in intel_memory_regions_hw_probe. >>>> >>>> For future reference please include some impact assessment in patches tagged >>>> as fixes. Makes maintainers job, and even anyone's who tries to backport >>>> stuff to stable at some future date, much easier if it is known how >>>> important is the fix and in what circumstances can the problem it is fixing >>>> trigger. >>>> >>> >>> As someone doing backport work, I think this patch is fine. Everyone >>> knows the impact of a NULL dereference in probe(). >>> >>> I guess with patches that add NULL dereferences, the trick is >>> understanding when people are adding NULL checks to make a static >>> checker happy or when it's a real bug. But the fault lies with the >>> people adding NULL checks just to make the tools happy. Some of these >>> pointless NULL checks end up in stable, but it's fine, extra NULL checks >>> never hurt anyone. If the maintainer wants to be extra safe by adding >>> NULL checks then who are we to say otherwise. >>> >>> In other words normal patches shouldn't have to say. "I'm not lying" at >>> the end. It should be the pointless patches which say, "I'm doing a >>> pointless thing. Don't bother backporting." >>> >>> Most stable patch backports are done automatically and people have >>> various tools and scripts to do that. If the tools don't handle this >>> patch automatically then they are defective. >> >> Right, and every few releases maintainers and authors get a bunch of emails >> for patches which did not apply to some stable tree. >> > > I believe these emails are only sent for commits that are tagged for > stable. For AUTOSEL patches, the backporting is done on a best effort > basis. On the other hand, hopefully this patch would have been tagged > for stable if we hadn't fixed the bug so quickly. > >> In which case someone has to do manual work and then it is good to know how >> important it is to backport something. For cases when it is not trivial. It >> does not apply to this patch, but as a _best practice_ it is good if the >> commit message explains the impacted platforms and scenarios. >> >> In this case I can follow the Fixes: tag and see the fix that this patches >> fixes is only about ATS-M. Which if it was a more complicated patch might be >> a reason to not need bother backporting past some kernel version where >> platform X wasn't even supported. >> >> Therefore I think my point is that best practice is to include this the >> commit text, so any future maintainer/backporter does not have to re-do >> detective work, stands. > > This is a really elaborate hypothetical. Are there kernels which are > affected by this bug but don't support ATS-M? I am not sure why are we arguing against the value of putting a bit more info in commit messages. When I was writing up the drm-intel-next-fixes pull request I already had to follow the Fixes: chain for this one to understand the impact. This patch is already in and all but from my point of view best practice still is for commit messages to be a bit more verbose than "fix null pointer deref". At least when fixes are coming from inside Intel I think we can assume people have enough info to asses and document. Regards, Tvrtko
-----Original Message----- From: Tvrtko Ursulin <tursulin@ursulin.net> Sent: Monday, July 29, 2024 1:21 AM To: Dan Carpenter <dan.carpenter@linaro.org> Cc: Cavitt, Jonathan <jonathan.cavitt@intel.com>; intel-gfx@lists.freedesktop.org; Gupta, saurabhg <saurabhg.gupta@intel.com>; chris.p.wilson@linux.intel.com Subject: Re: [PATCH] drm/i915: Allow NULL memory region > > > On 26/07/2024 18:00, Dan Carpenter wrote: > > On Fri, Jul 26, 2024 at 09:17:20AM +0100, Tvrtko Ursulin wrote: > >> > >> On 25/07/2024 16:58, Dan Carpenter wrote: > >>> On Thu, Jul 25, 2024 at 08:48:35AM +0100, Tvrtko Ursulin wrote: > >>>> > >>>> Hi, > >>>> > >>>> On 12/07/2024 22:41, Jonathan Cavitt wrote: > >>>>> Prevent a NULL pointer access in intel_memory_regions_hw_probe. > >>>> > >>>> For future reference please include some impact assessment in patches tagged > >>>> as fixes. Makes maintainers job, and even anyone's who tries to backport > >>>> stuff to stable at some future date, much easier if it is known how > >>>> important is the fix and in what circumstances can the problem it is fixing > >>>> trigger. > >>>> > >>> > >>> As someone doing backport work, I think this patch is fine. Everyone > >>> knows the impact of a NULL dereference in probe(). > >>> > >>> I guess with patches that add NULL dereferences, the trick is > >>> understanding when people are adding NULL checks to make a static > >>> checker happy or when it's a real bug. But the fault lies with the > >>> people adding NULL checks just to make the tools happy. Some of these > >>> pointless NULL checks end up in stable, but it's fine, extra NULL checks > >>> never hurt anyone. If the maintainer wants to be extra safe by adding > >>> NULL checks then who are we to say otherwise. > >>> > >>> In other words normal patches shouldn't have to say. "I'm not lying" at > >>> the end. It should be the pointless patches which say, "I'm doing a > >>> pointless thing. Don't bother backporting." > >>> > >>> Most stable patch backports are done automatically and people have > >>> various tools and scripts to do that. If the tools don't handle this > >>> patch automatically then they are defective. > >> > >> Right, and every few releases maintainers and authors get a bunch of emails > >> for patches which did not apply to some stable tree. > >> > > > > I believe these emails are only sent for commits that are tagged for > > stable. For AUTOSEL patches, the backporting is done on a best effort > > basis. On the other hand, hopefully this patch would have been tagged > > for stable if we hadn't fixed the bug so quickly. > > > >> In which case someone has to do manual work and then it is good to know how > >> important it is to backport something. For cases when it is not trivial. It > >> does not apply to this patch, but as a _best practice_ it is good if the > >> commit message explains the impacted platforms and scenarios. > >> > >> In this case I can follow the Fixes: tag and see the fix that this patches > >> fixes is only about ATS-M. Which if it was a more complicated patch might be > >> a reason to not need bother backporting past some kernel version where > >> platform X wasn't even supported. > >> > >> Therefore I think my point is that best practice is to include this the > >> commit text, so any future maintainer/backporter does not have to re-do > >> detective work, stands. > > > > This is a really elaborate hypothetical. Are there kernels which are > > affected by this bug but don't support ATS-M? > > I am not sure why are we arguing against the value of putting a bit more > info in commit messages. > > When I was writing up the drm-intel-next-fixes pull request I already > had to follow the Fixes: chain for this one to understand the impact. > > This patch is already in and all but from my point of view best practice > still is for commit messages to be a bit more verbose than "fix null > pointer deref". At least when fixes are coming from inside Intel I think > we can assume people have enough info to asses and document. For future reference, what kind of additional information would you have preferred been added to this patch that was not originally provided, and in what location should that information have been added (as a part of the commit message itself, after the Fixes tag, etc.)? -Jonathan Cavitt > > Regards, > > Tvrtko >
On 29/07/2024 15:59, Cavitt, Jonathan wrote: > -----Original Message----- > From: Tvrtko Ursulin <tursulin@ursulin.net> > Sent: Monday, July 29, 2024 1:21 AM > To: Dan Carpenter <dan.carpenter@linaro.org> > Cc: Cavitt, Jonathan <jonathan.cavitt@intel.com>; intel-gfx@lists.freedesktop.org; Gupta, saurabhg <saurabhg.gupta@intel.com>; chris.p.wilson@linux.intel.com > Subject: Re: [PATCH] drm/i915: Allow NULL memory region >> >> >> On 26/07/2024 18:00, Dan Carpenter wrote: >>> On Fri, Jul 26, 2024 at 09:17:20AM +0100, Tvrtko Ursulin wrote: >>>> >>>> On 25/07/2024 16:58, Dan Carpenter wrote: >>>>> On Thu, Jul 25, 2024 at 08:48:35AM +0100, Tvrtko Ursulin wrote: >>>>>> >>>>>> Hi, >>>>>> >>>>>> On 12/07/2024 22:41, Jonathan Cavitt wrote: >>>>>>> Prevent a NULL pointer access in intel_memory_regions_hw_probe. >>>>>> >>>>>> For future reference please include some impact assessment in patches tagged >>>>>> as fixes. Makes maintainers job, and even anyone's who tries to backport >>>>>> stuff to stable at some future date, much easier if it is known how >>>>>> important is the fix and in what circumstances can the problem it is fixing >>>>>> trigger. >>>>>> >>>>> >>>>> As someone doing backport work, I think this patch is fine. Everyone >>>>> knows the impact of a NULL dereference in probe(). >>>>> >>>>> I guess with patches that add NULL dereferences, the trick is >>>>> understanding when people are adding NULL checks to make a static >>>>> checker happy or when it's a real bug. But the fault lies with the >>>>> people adding NULL checks just to make the tools happy. Some of these >>>>> pointless NULL checks end up in stable, but it's fine, extra NULL checks >>>>> never hurt anyone. If the maintainer wants to be extra safe by adding >>>>> NULL checks then who are we to say otherwise. >>>>> >>>>> In other words normal patches shouldn't have to say. "I'm not lying" at >>>>> the end. It should be the pointless patches which say, "I'm doing a >>>>> pointless thing. Don't bother backporting." >>>>> >>>>> Most stable patch backports are done automatically and people have >>>>> various tools and scripts to do that. If the tools don't handle this >>>>> patch automatically then they are defective. >>>> >>>> Right, and every few releases maintainers and authors get a bunch of emails >>>> for patches which did not apply to some stable tree. >>>> >>> >>> I believe these emails are only sent for commits that are tagged for >>> stable. For AUTOSEL patches, the backporting is done on a best effort >>> basis. On the other hand, hopefully this patch would have been tagged >>> for stable if we hadn't fixed the bug so quickly. >>> >>>> In which case someone has to do manual work and then it is good to know how >>>> important it is to backport something. For cases when it is not trivial. It >>>> does not apply to this patch, but as a _best practice_ it is good if the >>>> commit message explains the impacted platforms and scenarios. >>>> >>>> In this case I can follow the Fixes: tag and see the fix that this patches >>>> fixes is only about ATS-M. Which if it was a more complicated patch might be >>>> a reason to not need bother backporting past some kernel version where >>>> platform X wasn't even supported. >>>> >>>> Therefore I think my point is that best practice is to include this the >>>> commit text, so any future maintainer/backporter does not have to re-do >>>> detective work, stands. >>> >>> This is a really elaborate hypothetical. Are there kernels which are >>> affected by this bug but don't support ATS-M? >> >> I am not sure why are we arguing against the value of putting a bit more >> info in commit messages. >> >> When I was writing up the drm-intel-next-fixes pull request I already >> had to follow the Fixes: chain for this one to understand the impact. >> >> This patch is already in and all but from my point of view best practice >> still is for commit messages to be a bit more verbose than "fix null >> pointer deref". At least when fixes are coming from inside Intel I think >> we can assume people have enough info to asses and document. > > For future reference, what kind of additional information would you have > preferred been added to this patch that was not originally provided, and in > what location should that information have been added (as a part of the > commit message itself, after the Fixes tag, etc.)? In this particular case something as simple as below would have made my job a little bit easier: drm/i915: Allow NULL memory region Prevent a NULL pointer access in intel_memory_regions_hw_probe which can happen on some ATS-M machines with specific BIOS configurations. (And it may be wrong what I added but hey-ho, that's kind of the point of getting the information direct from the source instead of having to figure it out when writing pull requests.) Regards, Tvrtko
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; + 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++) {
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@linaro.org> Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com> --- drivers/gpu/drm/i915/intel_memory_region.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)