Re: [PATCH] drm/i915/guc: Remove bogus null check
On Fri, Mar 29, 2024 at 07:38:11PM +0300, Dan Carpenter wrote:
> On Fri, Mar 29, 2024 at 08:09:38AM +0100, Andi Shyti wrote:
> > Hi Rodrigo,
> >
> > On Thu, Mar 28, 2024 at 09:39:17PM -0400, Rodrigo Vivi wrote:
> > > On Thu, Mar 28, 2024 at 10:41:55PM +0100, Andi Shyti wrote:
> > > > On Thu, Mar 28, 2024 at 05:31:07PM -0400, Rodrigo Vivi wrote:
> > > > > This null check is bogus because we are already using 'ce' stuff
> > > > > in many places before this function is called.
> > > > >
> > > > > Having this here is useless and confuses static analyzer tools
> > > > > that can see:
> > > > >
> > > > > struct intel_engine_cs *engine = ce->engine;
> > > > >
> > > > > before this check, in the same function.
> > > > >
> > > > > Fixes: cec82816d0d0 ("drm/i915/guc: Use context hints for GT
> > > > > frequency")
> > > >
> > > > there is no need to have the Fixes tag here.
> > >
> > > why not? I imagine distros that have this commit cec82816d0d0 and use
> > > static analyzers would also want this patch ported to silent those, no?!
> >
> > Still... it's not a bug. The tag "Fixes:" should be used when a
> > bug is fixed, but not for harmless static analyzer reports.
> >
> > Besides, if we want to keep the Fixes tag we should also Cc
> > stable, i guess checkpatch.pl complains about it.
> >
> > (BTW, Cc'ed in this mail we have the inventor of the tag and he
> > can confirm after having had his morning coffee :-) ).
> >
>
> Good. I keep reminding people that I invented the Fixes tag because it
> is my proudest achievement. :)
>
> No. Only use Fixes tags for bug fixes.
Thanks for the clarifications and reviews. I have removed the 'Fixes:' tag
and pushed the patch as is.
>
> regards,
> dan carpenter
>
Re: [PATCH] drm/i915/guc: Remove bogus null check
On Fri, Mar 29, 2024 at 08:09:38AM +0100, Andi Shyti wrote:
> Hi Rodrigo,
>
> On Thu, Mar 28, 2024 at 09:39:17PM -0400, Rodrigo Vivi wrote:
> > On Thu, Mar 28, 2024 at 10:41:55PM +0100, Andi Shyti wrote:
> > > On Thu, Mar 28, 2024 at 05:31:07PM -0400, Rodrigo Vivi wrote:
> > > > This null check is bogus because we are already using 'ce' stuff
> > > > in many places before this function is called.
> > > >
> > > > Having this here is useless and confuses static analyzer tools
> > > > that can see:
> > > >
> > > > struct intel_engine_cs *engine = ce->engine;
> > > >
> > > > before this check, in the same function.
> > > >
> > > > Fixes: cec82816d0d0 ("drm/i915/guc: Use context hints for GT frequency")
> > >
> > > there is no need to have the Fixes tag here.
> >
> > why not? I imagine distros that have this commit cec82816d0d0 and use
> > static analyzers would also want this patch ported to silent those, no?!
>
> Still... it's not a bug. The tag "Fixes:" should be used when a
> bug is fixed, but not for harmless static analyzer reports.
>
> Besides, if we want to keep the Fixes tag we should also Cc
> stable, i guess checkpatch.pl complains about it.
>
> (BTW, Cc'ed in this mail we have the inventor of the tag and he
> can confirm after having had his morning coffee :-) ).
>
Good. I keep reminding people that I invented the Fixes tag because it
is my proudest achievement. :)
No. Only use Fixes tags for bug fixes.
regards,
dan carpenter
Re: [PATCH] drm/i915/guc: Remove bogus null check
Hi Rodrigo,
On Thu, Mar 28, 2024 at 09:39:17PM -0400, Rodrigo Vivi wrote:
> On Thu, Mar 28, 2024 at 10:41:55PM +0100, Andi Shyti wrote:
> > On Thu, Mar 28, 2024 at 05:31:07PM -0400, Rodrigo Vivi wrote:
> > > This null check is bogus because we are already using 'ce' stuff
> > > in many places before this function is called.
> > >
> > > Having this here is useless and confuses static analyzer tools
> > > that can see:
> > >
> > > struct intel_engine_cs *engine = ce->engine;
> > >
> > > before this check, in the same function.
> > >
> > > Fixes: cec82816d0d0 ("drm/i915/guc: Use context hints for GT frequency")
> >
> > there is no need to have the Fixes tag here.
>
> why not? I imagine distros that have this commit cec82816d0d0 and use
> static analyzers would also want this patch ported to silent those, no?!
Still... it's not a bug. The tag "Fixes:" should be used when a
bug is fixed, but not for harmless static analyzer reports.
Besides, if we want to keep the Fixes tag we should also Cc
stable, i guess checkpatch.pl complains about it.
(BTW, Cc'ed in this mail we have the inventor of the tag and he
can confirm after having had his morning coffee :-) ).
> > > Reported-by: kernel test robot
> > > Reported-by: Dan Carpenter
> > > Closes: https://lore.kernel.org/r/202403101225.7ahejhzj-...@intel.com/
> > > Cc: Vinay Belgaumkar
> > > Cc: John Harrison
> > > Signed-off-by: Rodrigo Vivi
> > > ---
> > > drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > > b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > > index 01d0ec1b30f2..24a82616f844 100644
> > > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > > @@ -2677,7 +2677,7 @@ static int guc_context_policy_init_v70(struct
> > > intel_context *ce, bool loop)
> > > execution_quantum = engine->props.timeslice_duration_ms * 1000;
> > > preemption_timeout = engine->props.preempt_timeout_ms * 1000;
> > >
> > > - if (ce && (ce->flags & BIT(CONTEXT_LOW_LATENCY)))
> > > + if (ce->flags & BIT(CONTEXT_LOW_LATENCY))
> >
> > We could keep the check but make it earlier.
>
> yes, that's another alternative.
>
>
> -struct intel_engine_cs *engine = ce->engine;
> +struct intel_engine_cs *engine;
>
> if (!ce)
>return;
>
> engine = ce->engine.
this is what I meant...
> But looking to the 2 places where this function is getting called,
> we already have ce->something used.
... and I also checked where the function is called and how it's
called: I see that if we get here then for sure 'ce' is not NULL.
But for robustness we could still keep it.
Either way I think your patch is good except for the "Fixes:"
tag:
Reviewed-by: Andi Shyti
Thanks,
Andi
> I can make the change to be like that if you believe that there's
> a possibility in the future that we change that, just to be on
> the safe side.
>
> or anything else I might be missing?
>
> Thanks for looking into this,
> Rodrigo.
>
> >
> > Thanks,
> > Andi
> >
> > > slpc_ctx_freq_req |= SLPC_CTX_FREQ_REQ_IS_COMPUTE;
> > >
> > > __guc_context_policy_start_klv(, ce->guc_id.id);
> > > --
> > > 2.44.0
Re: [PATCH] drm/i915/guc: Remove bogus null check
On Thu, Mar 28, 2024 at 10:41:55PM +0100, Andi Shyti wrote:
> Hi Rodrigo,
>
> On Thu, Mar 28, 2024 at 05:31:07PM -0400, Rodrigo Vivi wrote:
> > This null check is bogus because we are already using 'ce' stuff
> > in many places before this function is called.
> >
> > Having this here is useless and confuses static analyzer tools
> > that can see:
> >
> > struct intel_engine_cs *engine = ce->engine;
> >
> > before this check, in the same function.
> >
> > Fixes: cec82816d0d0 ("drm/i915/guc: Use context hints for GT frequency")
>
> there is no need to have the Fixes tag here.
why not? I imagine distros that have this commit cec82816d0d0 and use
static analyzers would also want this patch ported to silent those, no?!
>
> > Reported-by: kernel test robot
> > Reported-by: Dan Carpenter
> > Closes: https://lore.kernel.org/r/202403101225.7ahejhzj-...@intel.com/
> > Cc: Vinay Belgaumkar
> > Cc: John Harrison
> > Signed-off-by: Rodrigo Vivi
> > ---
> > drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > index 01d0ec1b30f2..24a82616f844 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > @@ -2677,7 +2677,7 @@ static int guc_context_policy_init_v70(struct
> > intel_context *ce, bool loop)
> > execution_quantum = engine->props.timeslice_duration_ms * 1000;
> > preemption_timeout = engine->props.preempt_timeout_ms * 1000;
> >
> > - if (ce && (ce->flags & BIT(CONTEXT_LOW_LATENCY)))
> > + if (ce->flags & BIT(CONTEXT_LOW_LATENCY))
>
> We could keep the check but make it earlier.
yes, that's another alternative.
-struct intel_engine_cs *engine = ce->engine;
+struct intel_engine_cs *engine;
if (!ce)
return;
engine = ce->engine.
But looking to the 2 places where this function is getting called,
we already have ce->something used.
I can make the change to be like that if you believe that there's
a possibility in the future that we change that, just to be on
the safe side.
or anything else I might be missing?
Thanks for looking into this,
Rodrigo.
>
> Thanks,
> Andi
>
> > slpc_ctx_freq_req |= SLPC_CTX_FREQ_REQ_IS_COMPUTE;
> >
> > __guc_context_policy_start_klv(, ce->guc_id.id);
> > --
> > 2.44.0
Re: [PATCH] drm/i915/guc: Remove bogus null check
Hi Rodrigo,
On Thu, Mar 28, 2024 at 05:31:07PM -0400, Rodrigo Vivi wrote:
> This null check is bogus because we are already using 'ce' stuff
> in many places before this function is called.
>
> Having this here is useless and confuses static analyzer tools
> that can see:
>
> struct intel_engine_cs *engine = ce->engine;
>
> before this check, in the same function.
>
> Fixes: cec82816d0d0 ("drm/i915/guc: Use context hints for GT frequency")
there is no need to have the Fixes tag here.
> Reported-by: kernel test robot
> Reported-by: Dan Carpenter
> Closes: https://lore.kernel.org/r/202403101225.7ahejhzj-...@intel.com/
> Cc: Vinay Belgaumkar
> Cc: John Harrison
> Signed-off-by: Rodrigo Vivi
> ---
> drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> index 01d0ec1b30f2..24a82616f844 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> @@ -2677,7 +2677,7 @@ static int guc_context_policy_init_v70(struct
> intel_context *ce, bool loop)
> execution_quantum = engine->props.timeslice_duration_ms * 1000;
> preemption_timeout = engine->props.preempt_timeout_ms * 1000;
>
> - if (ce && (ce->flags & BIT(CONTEXT_LOW_LATENCY)))
> + if (ce->flags & BIT(CONTEXT_LOW_LATENCY))
We could keep the check but make it earlier.
Thanks,
Andi
> slpc_ctx_freq_req |= SLPC_CTX_FREQ_REQ_IS_COMPUTE;
>
> __guc_context_policy_start_klv(, ce->guc_id.id);
> --
> 2.44.0
