Patchwork drm: make sure to check callbacks of ENCODER and CRTC helpers

login
register
mail settings
Submitter Inki Dae
Date Aug. 16, 2015, 4:12 a.m.
Message ID <1439698334-19925-1-git-send-email-inki.dae@samsung.com>
Download mbox | patch
Permalink /patch/7021891/
State New
Headers show

Comments

Inki Dae - Aug. 16, 2015, 4:12 a.m.
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@samsung.com>
---
 drivers/gpu/drm/drm_crtc_helper.c | 44 ++++++++++++++++++++++++++-------------
 1 file changed, 29 insertions(+), 15 deletions(-)
Daniel Vetter - Aug. 25, 2015, 8:26 a.m.
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@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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Patch

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);
 	}