| Message ID | 20200303142347.15696-1-aditya.swarup@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/selftests: Fix uninitialized variable | expand |
On Tue, 03 Mar 2020, Aditya Swarup <aditya.swarup@intel.com> wrote: > Static code analysis tool identified struct lrc_timestamp data as being > uninitialized and then data.ce[] is being checked for NULL/negative > value in the error path. Initializing data variable fixes the issue. > > Cc: Matt Roper <matthew.d.roper@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Aditya Swarup <aditya.swarup@intel.com> > --- > drivers/gpu/drm/i915/gt/selftest_lrc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/gt/selftest_lrc.c b/drivers/gpu/drm/i915/gt/selftest_lrc.c > index ccf9debacd90..9b75b3c77a5b 100644 > --- a/drivers/gpu/drm/i915/gt/selftest_lrc.c > +++ b/drivers/gpu/drm/i915/gt/selftest_lrc.c > @@ -4725,7 +4725,7 @@ static int live_lrc_timestamp(void *arg) > { > struct intel_gt *gt = arg; > enum intel_engine_id id; > - struct lrc_timestamp data; > + struct lrc_timestamp data = { 0 }; {} is preferred over {0}. BR, Jani. > const u32 poison[] = { > 0, > S32_MAX,
On Tue, Mar 03, 2020 at 06:23:47AM -0800, Aditya Swarup wrote: > Static code analysis tool identified struct lrc_timestamp data as being > uninitialized and then data.ce[] is being checked for NULL/negative > value in the error path. Initializing data variable fixes the issue. > > Cc: Matt Roper <matthew.d.roper@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Aditya Swarup <aditya.swarup@intel.com> Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > --- > drivers/gpu/drm/i915/gt/selftest_lrc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/gt/selftest_lrc.c b/drivers/gpu/drm/i915/gt/selftest_lrc.c > index ccf9debacd90..9b75b3c77a5b 100644 > --- a/drivers/gpu/drm/i915/gt/selftest_lrc.c > +++ b/drivers/gpu/drm/i915/gt/selftest_lrc.c > @@ -4725,7 +4725,7 @@ static int live_lrc_timestamp(void *arg) > { > struct intel_gt *gt = arg; > enum intel_engine_id id; > - struct lrc_timestamp data; > + struct lrc_timestamp data = { 0 }; > const u32 poison[] = { > 0, > S32_MAX, > -- > 2.25.0 >
On Tue, Mar 03, 2020 at 05:25:21PM +0200, Jani Nikula wrote: > On Tue, 03 Mar 2020, Aditya Swarup <aditya.swarup@intel.com> wrote: > > Static code analysis tool identified struct lrc_timestamp data as being > > uninitialized and then data.ce[] is being checked for NULL/negative > > value in the error path. Initializing data variable fixes the issue. > > > > Cc: Matt Roper <matthew.d.roper@intel.com> > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Signed-off-by: Aditya Swarup <aditya.swarup@intel.com> > > --- > > drivers/gpu/drm/i915/gt/selftest_lrc.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/gt/selftest_lrc.c b/drivers/gpu/drm/i915/gt/selftest_lrc.c > > index ccf9debacd90..9b75b3c77a5b 100644 > > --- a/drivers/gpu/drm/i915/gt/selftest_lrc.c > > +++ b/drivers/gpu/drm/i915/gt/selftest_lrc.c > > @@ -4725,7 +4725,7 @@ static int live_lrc_timestamp(void *arg) > > { > > struct intel_gt *gt = arg; > > enum intel_engine_id id; > > - struct lrc_timestamp data; > > + struct lrc_timestamp data = { 0 }; > > {} is preferred over {0}. Is there a reference for this (e.g., in the kernel coding style)? I thought this came up a couple years ago and the consensus was the other way, although I could be misremembering. Unless it's changed in a recent standard, I think {} is only legal in C++, so using it in C code is a gcc-ism? Matt > > BR, > Jani. > > > const u32 poison[] = { > > 0, > > S32_MAX, > > -- > Jani Nikula, Intel Open Source Graphics Center > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, 03 Mar 2020, Matt Roper <matthew.d.roper@intel.com> wrote: > On Tue, Mar 03, 2020 at 05:25:21PM +0200, Jani Nikula wrote: >> On Tue, 03 Mar 2020, Aditya Swarup <aditya.swarup@intel.com> wrote: >> > - struct lrc_timestamp data; >> > + struct lrc_timestamp data = { 0 }; >> >> {} is preferred over {0}. > > Is there a reference for this (e.g., in the kernel coding style)? I > thought this came up a couple years ago and the consensus was the other > way, although I could be misremembering. Unless it's changed in a > recent standard, I think {} is only legal in C++, so using it in C code > is a gcc-ism? Both are widely used in the kernel. I think we've mostly converged to {} in i915. Yes, it's a gcc-ism in C code, but the kernel is gcc, not standard C. I can't find a reference right now, but ISTR there are some warnings issued in some cases with the {0} initializer, depending on the struct and perhaps on the compiler. Anyway, we're 71 to 9 in favor of {} in i915, so please go with that. BR, Jani.
On Wed, 04 Mar 2020, Jani Nikula <jani.nikula@linux.intel.com> wrote: > On Tue, 03 Mar 2020, Matt Roper <matthew.d.roper@intel.com> wrote: >> On Tue, Mar 03, 2020 at 05:25:21PM +0200, Jani Nikula wrote: >>> On Tue, 03 Mar 2020, Aditya Swarup <aditya.swarup@intel.com> wrote: >>> > - struct lrc_timestamp data; >>> > + struct lrc_timestamp data = { 0 }; >>> >>> {} is preferred over {0}. >> >> Is there a reference for this (e.g., in the kernel coding style)? I >> thought this came up a couple years ago and the consensus was the other >> way, although I could be misremembering. Unless it's changed in a >> recent standard, I think {} is only legal in C++, so using it in C code >> is a gcc-ism? > > Both are widely used in the kernel. I think we've mostly converged to {} > in i915. Yes, it's a gcc-ism in C code, but the kernel is gcc, not > standard C. > > I can't find a reference right now, but ISTR there are some warnings > issued in some cases with the {0} initializer, depending on the struct > and perhaps on the compiler. Here's one [1]. BR, Jani. [1] http://patchwork.freedesktop.org/patch/msgid/20200304183654.GA9011@paulmck-ThinkPad-P72
diff --git a/drivers/gpu/drm/i915/gt/selftest_lrc.c b/drivers/gpu/drm/i915/gt/selftest_lrc.c index ccf9debacd90..9b75b3c77a5b 100644 --- a/drivers/gpu/drm/i915/gt/selftest_lrc.c +++ b/drivers/gpu/drm/i915/gt/selftest_lrc.c @@ -4725,7 +4725,7 @@ static int live_lrc_timestamp(void *arg) { struct intel_gt *gt = arg; enum intel_engine_id id; - struct lrc_timestamp data; + struct lrc_timestamp data = { 0 }; const u32 poison[] = { 0, S32_MAX,
Static code analysis tool identified struct lrc_timestamp data as being uninitialized and then data.ce[] is being checked for NULL/negative value in the error path. Initializing data variable fixes the issue. Cc: Matt Roper <matthew.d.roper@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Aditya Swarup <aditya.swarup@intel.com> --- drivers/gpu/drm/i915/gt/selftest_lrc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)