| Message ID | 1464686243-25418-4-git-send-email-tomi.valkeinen@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, May 31, 2016 at 12:17:22PM +0300, Tomi Valkeinen wrote: > drm_atomic_set_mode_prop_for_crtc() has a few issues: > > - it doesn't clear the state->mode, so old data may be left there when a > new mode is set. > - if an error happens, state->mode is left in a partially updated state. > > This patch improves the situation by: > > - bail out early if blob is of wrong length. > - construct drm_display_mode first in an initialized local variable, and > copy it to state->mode only when we know the call has succeeded. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > --- > drivers/gpu/drm/drm_atomic.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 3ff1ed7b33db..5bfecb2bbedf 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -348,16 +348,24 @@ int drm_atomic_set_mode_prop_for_crtc(struct drm_crtc_state *state, > if (blob == state->mode_blob) > return 0; > > + if (blob && blob->length != sizeof(struct drm_mode_modeinfo)) > + return -EINVAL; > + > drm_property_unreference_blob(state->mode_blob); > state->mode_blob = NULL; > > if (blob) { > - if (blob->length != sizeof(struct drm_mode_modeinfo) || > - drm_mode_convert_umode(&state->mode, > - (const struct drm_mode_modeinfo *) > - blob->data)) > + const struct drm_mode_modeinfo *umode = > + (const struct drm_mode_modeinfo *)blob->data; > + struct drm_display_mode mode; > + > + memset(&mode, 0, sizeof(mode)); > + > + if (drm_mode_convert_umode(&mode, umode)) > return -EINVAL; The code flow changes aren't required - when an atomic commit fails we throw away all the state structures, they're duplicates. Hence leaving garabge in there is totally fine. I'd just move the memset in front of the if (blob), that should take care of everything. -Daniel > > + state->mode = mode; > + > state->mode_blob = drm_property_reference_blob(blob); > state->enable = true; > DRM_DEBUG_ATOMIC("Set [MODE:%s] for CRTC state %p\n", > -- > 2.5.0 >
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 3ff1ed7b33db..5bfecb2bbedf 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -348,16 +348,24 @@ int drm_atomic_set_mode_prop_for_crtc(struct drm_crtc_state *state, if (blob == state->mode_blob) return 0; + if (blob && blob->length != sizeof(struct drm_mode_modeinfo)) + return -EINVAL; + drm_property_unreference_blob(state->mode_blob); state->mode_blob = NULL; if (blob) { - if (blob->length != sizeof(struct drm_mode_modeinfo) || - drm_mode_convert_umode(&state->mode, - (const struct drm_mode_modeinfo *) - blob->data)) + const struct drm_mode_modeinfo *umode = + (const struct drm_mode_modeinfo *)blob->data; + struct drm_display_mode mode; + + memset(&mode, 0, sizeof(mode)); + + if (drm_mode_convert_umode(&mode, umode)) return -EINVAL; + state->mode = mode; + state->mode_blob = drm_property_reference_blob(blob); state->enable = true; DRM_DEBUG_ATOMIC("Set [MODE:%s] for CRTC state %p\n",
drm_atomic_set_mode_prop_for_crtc() has a few issues: - it doesn't clear the state->mode, so old data may be left there when a new mode is set. - if an error happens, state->mode is left in a partially updated state. This patch improves the situation by: - bail out early if blob is of wrong length. - construct drm_display_mode first in an initialized local variable, and copy it to state->mode only when we know the call has succeeded. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> --- drivers/gpu/drm/drm_atomic.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)