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@xxxxxxxxx> > > 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.
