| Message ID | 20230601090338.80284-1-luciano.coelho@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/display: pre-initialize some values in probe_gmdid_display() | expand |
> 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. Regards, Suraj Kandpal > addr = pci_iomap_range(pdev, 0, > i915_mmio_reg_offset(GMD_ID_DISPLAY), sizeof(u32)); > if (!addr) { > drm_err(&i915->drm, "Cannot map MMIO BAR to read > display GMD_ID\n"); > -- > 2.39.2
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.
> 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. I don’t think it needs to be changed then and the explanation looks reasonable. So this LGTM Reviewed-by: Suraj Kandpal <suraj.kandpal@intel.com> > > -- > Cheers, > Luca.
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@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@intel.com> > Thanks, Suraj! Can someone merge this for me, please? -- Cheers, Luca.
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@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@intel.com> > > > > Thanks, Suraj! Can someone merge this for me, please? This is now merged. BR, Jouni Högander > > -- > Cheers, > Luca.
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; + addr = pci_iomap_range(pdev, 0, i915_mmio_reg_offset(GMD_ID_DISPLAY), sizeof(u32)); if (!addr) { drm_err(&i915->drm, "Cannot map MMIO BAR to read display GMD_ID\n");
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> --- drivers/gpu/drm/i915/display/intel_display_device.c | 9 +++++++++ 1 file changed, 9 insertions(+)