[PATCH 3/4] drm: make drm_atomic_set_mode_prop_for_crtc() more reliable

[Daniel Vetter]

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 at 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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch