[PATCH] drm: make sure to check callbacks of ENCODER and CRTC helpers

[Daniel Vetter]

On Sun, Aug 16, 2015 at 01:12:14PM +0900, Inki Dae wrote:
> This patch fixes NULL pointer access issues to ENCODER
> and CRTC drivers.
> 
> Vendor specific DRM KMS drivers may not set their helper callbacks
> to ENCODER dn CRTC drivers. In this case, that will incur NULL
> pointer access when modeset is tried.
> 
> So this patch makes sure to check if the callbacks are NULL or not.
> 
> Signed-off-by: Inki Dae <inki.dae at samsung.com>

Is there really a lot of value in tuning legacy crtc helpers? New drivers
should just use atomic (where we've made all these changes already),
drivers which are still transitioning can just carry around dummy
functions for now.
-Daniel

> ---
>  drivers/gpu/drm/drm_crtc_helper.c | 44 ++++++++++++++++++++++++++-------------
>  1 file changed, 29 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> index ef53475..aa1f42f 100644
> --- a/drivers/gpu/drm/drm_crtc_helper.c
> +++ b/drivers/gpu/drm/drm_crtc_helper.c
> @@ -318,16 +318,22 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
>  		}
>  
>  		encoder_funcs = encoder->helper_private;
> -		if (!(ret = encoder_funcs->mode_fixup(encoder, mode,
> -						      adjusted_mode))) {
> -			DRM_DEBUG_KMS("Encoder fixup failed\n");
> -			goto done;
> +		if (encoder_funcs->mode_fixup) {
> +			ret = encoder_funcs->mode_fixup(encoder, mode,
> +							adjusted_mode);
> +			if (!ret) {
> +				DRM_DEBUG_KMS("Encoder fixup failed\n");
> +				goto done;
> +			}
>  		}
>  	}
>  
> -	if (!(ret = crtc_funcs->mode_fixup(crtc, mode, adjusted_mode))) {
> -		DRM_DEBUG_KMS("CRTC fixup failed\n");
> -		goto done;
> +	if (crtc_funcs->mode_fixup) {
> +		ret = crtc_funcs->mode_fixup(crtc, mode, adjusted_mode);
> +		if (!ret) {
> +			DRM_DEBUG_KMS("CRTC fixup failed\n");
> +			goto done;
> +		}
>  	}
>  	DRM_DEBUG_KMS("[CRTC:%d]\n", crtc->base.id);
>  
> @@ -343,21 +349,26 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
>  
>  		encoder_funcs = encoder->helper_private;
>  		/* Disable the encoders as the first thing we do. */
> -		encoder_funcs->prepare(encoder);
> +		if (encoder_funcs->prepare)
> +			encoder_funcs->prepare(encoder);
>  
>  		drm_bridge_post_disable(encoder->bridge);
>  	}
>  
>  	drm_crtc_prepare_encoders(dev);
>  
> -	crtc_funcs->prepare(crtc);
> +	if (crtc_funcs->prepare)
> +		crtc_funcs->prepare(crtc);
>  
>  	/* Set up the DPLL and any encoders state that needs to adjust or depend
>  	 * on the DPLL.
>  	 */
> -	ret = !crtc_funcs->mode_set(crtc, mode, adjusted_mode, x, y, old_fb);
> -	if (!ret)
> -	    goto done;
> +	if (crtc_funcs->mode_set) {
> +		ret = !crtc_funcs->mode_set(crtc, mode, adjusted_mode, x, y,
> +					    old_fb);
> +		if (!ret)
> +			goto done;
> +	}
>  
>  	drm_for_each_encoder(encoder, dev) {
>  
> @@ -368,13 +379,15 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
>  			encoder->base.id, encoder->name,
>  			mode->base.id, mode->name);
>  		encoder_funcs = encoder->helper_private;
> -		encoder_funcs->mode_set(encoder, mode, adjusted_mode);
> +		if (encoder_funcs->mode_set)
> +			encoder_funcs->mode_set(encoder, mode, adjusted_mode);
>  
>  		drm_bridge_mode_set(encoder->bridge, mode, adjusted_mode);
>  	}
>  
>  	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
> -	crtc_funcs->commit(crtc);
> +	if (crtc_funcs->commit)
> +		crtc_funcs->commit(crtc);
>  
>  	drm_for_each_encoder(encoder, dev) {
>  
> @@ -384,7 +397,8 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
>  		drm_bridge_pre_enable(encoder->bridge);
>  
>  		encoder_funcs = encoder->helper_private;
> -		encoder_funcs->commit(encoder);
> +		if (encoder_funcs->commit)
> +			encoder_funcs->commit(encoder);
>  
>  		drm_bridge_enable(encoder->bridge);
>  	}
> -- 
> 1.9.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

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