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

[Hogander, Jouni]

On Thu, 2023-06-22 at 12:09 +0000, Coelho, Luciano wrote:
> On Thu, 2023-06-22 at 10:08 +0000, Kandpal, Suraj wrote:
> > > 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 at 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.
> > 
> > I don’t think it needs to be changed then and the explanation looks
> > reasonable.
> > So this LGTM
> > 
> > Reviewed-by: Suraj Kandpal <suraj.kandpal at intel.com>
> > 
> 
> Thanks, Suraj! Can someone merge this for me, please?

This is now merged.

BR,

Jouni Högander
> 
> --
> Cheers,
> Luca.