Re: [Intel-gfx] [PATCH] drm/i915/display: pre-initialize some values in probe_gmdid_display()

["Coelho, Luciano" <luciano.coelho@intel.com>]

On Tue, 2023-06-20 at 10:30 +0000, Kandpal, Suraj wrote:
> > When intel_display_device_probe() (and, subsequently,
> > probe_gmdid_display()) returns, the caller expects ver, rel and
> > step to be
> > initialized.  Since there's no way to check that there was a
> > failure and
> > no_display was returned without some further refactoring, pre-
> > initiliaze all
> > these values to zero to keep it simple and safe.
> > 
> > Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
> 
> Looks okay to me just a small suggestion/question below.
> 
> > ---
> >  drivers/gpu/drm/i915/display/intel_display_device.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_device.c
> > b/drivers/gpu/drm/i915/display/intel_display_device.c
> > index 464df1764a86..fb6354e9e704 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_device.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display_device.c
> > @@ -731,6 +731,15 @@ probe_gmdid_display(struct drm_i915_private
> > *i915, u16 *ver, u16 *rel, u16 *step
> >  	u32 val;
> >  	int i;
> > 
> > +	/* The caller expects to ver, rel and step to be
> > initialized
> > +	 * here, and there's no good way to check when there was a
> > +	 * failure and no_display was returned.  So initialize all
> > these
> > +	 * values here zero, to be sure.
> > +	 */
> > +	*ver = 0;
> > +	*rel = 0;
> > +	*step = 0;
> > +
> 
> From what I can see this is only called from
> intel_display_device_probe() which is in turn
> called from intel_device_info_driver_create() where the above
> variables are defined maybe 
> we initialize these values there itself.

Thanks for the review!

I thought about initializing the variables on the caller side, but
reckoned that it would be more intuitive to initialize them in the
probe_gmdid_display() function instead, because the caller expects
those values to be set in successful cases and there's no way for it to
know whether there was a failure or not (because we return a pointer to
local no_display structure that the caller doesn't know about).

Obviously with the current code in the caller, that doesn't make much
difference, but I thought it was cleaner as I did.

But I'm okay to change it and initialize them at the caller, so just
let me know if you want that.

--
Cheers,
Luca.