| Message ID | 20240328213107.90632-1-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/guc: Remove bogus null check | expand |
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 <lkp@intel.com> > Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > Closes: https://lore.kernel.org/r/202403101225.7AheJhZJ-lkp@intel.com/ > Cc: Vinay Belgaumkar <vinay.belgaumkar@intel.com> > Cc: John Harrison <John.C.Harrison@Intel.com> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > 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(&policy, ce->guc_id.id); > -- > 2.44.0
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 <lkp@intel.com> > > Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > > Closes: https://lore.kernel.org/r/202403101225.7AheJhZJ-lkp@intel.com/ > > Cc: Vinay Belgaumkar <vinay.belgaumkar@intel.com> > > Cc: John Harrison <John.C.Harrison@Intel.com> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > --- > > 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(&policy, ce->guc_id.id); > > -- > > 2.44.0
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 <lkp@intel.com> > > > Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > > > Closes: https://lore.kernel.org/r/202403101225.7AheJhZJ-lkp@intel.com/ > > > Cc: Vinay Belgaumkar <vinay.belgaumkar@intel.com> > > > Cc: John Harrison <John.C.Harrison@Intel.com> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > --- > > > 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 <andi.shyti@linux.intel.com> 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(&policy, ce->guc_id.id); > > > -- > > > 2.44.0
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
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 >
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)) slpc_ctx_freq_req |= SLPC_CTX_FREQ_REQ_IS_COMPUTE; __guc_context_policy_start_klv(&policy, ce->guc_id.id);
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") Reported-by: kernel test robot <lkp@intel.com> Reported-by: Dan Carpenter <dan.carpenter@linaro.org> Closes: https://lore.kernel.org/r/202403101225.7AheJhZJ-lkp@intel.com/ Cc: Vinay Belgaumkar <vinay.belgaumkar@intel.com> Cc: John Harrison <John.C.Harrison@Intel.com> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)