Re: drm/i915: Copy the staged connector config to the legacy atomic state

Re: drm/i915: Copy the staged connector config to the legacy atomic state

[Dan Carpenter]

Hello Ander Conselvan de Oliveira,

The patch 944b0c765757: "drm/i915: Copy the staged connector config
to the legacy atomic state" from Mar 20, 2015, leads to the following
Smatch warning:

	drivers/gpu/drm/i915/intel_display.c:11937 intel_modeset_stage_output_state()
	error: 'connector_state' dereferencing possible ERR_PTR()

drivers/gpu/drm/i915/intel_display.c
 11928          /* Now we've also updated encoder->new_crtc for all encoders. */
 11929          for_each_intel_connector(dev, connector) {
 11930                  connector_state =
 11931                          drm_atomic_get_connector_state(state, &connector->base);
                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
It's complaining that this can return an ERR_PTR.

 11932  
 11933                  if (connector->new_encoder) {
 11934                          if (connector->new_encoder != connector->encoder)
 11935                                  connector->encoder = connector->new_encoder;
 11936                  } else {
 11937                          connector_state->crtc = NULL;
 11938                  }
 11939          }


regards,
dan carpenter
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Handle error to get connector state when staging config

[Ander Conselvan de Oliveira]

The return value of one of the calls to drm_atomic_get_connector_state()
in intel_modeset_stage_output_state() wasn't checked for errors.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 68fe8b8..1329ed2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12000,6 +12000,8 @@ intel_modeset_stage_output_state(struct drm_device *dev,
 	for_each_intel_connector(dev, connector) {
 		connector_state =
 			drm_atomic_get_connector_state(state, &connector->base);
+		if (IS_ERR(connector_state))
+			return PTR_ERR(connector_state);
 
 		if (connector->new_encoder) {
 			if (connector->new_encoder != connector->encoder)
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: drm/i915: Copy the staged connector config to the legacy atomic state

[Ander Conselvan De Oliveira]

On Fri, 2015-03-27 at 16:15 +0300, Dan Carpenter wrote:
> Hello Ander Conselvan de Oliveira,
> 
> The patch 944b0c765757: "drm/i915: Copy the staged connector config
> to the legacy atomic state" from Mar 20, 2015, leads to the following
> Smatch warning:
> 
> 	drivers/gpu/drm/i915/intel_display.c:11937 intel_modeset_stage_output_state()
> 	error: 'connector_state' dereferencing possible ERR_PTR()
> 
> drivers/gpu/drm/i915/intel_display.c
>  11928          /* Now we've also updated encoder->new_crtc for all encoders. */
>  11929          for_each_intel_connector(dev, connector) {
>  11930                  connector_state =
>  11931                          drm_atomic_get_connector_state(state, &connector->base);
>                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> It's complaining that this can return an ERR_PTR.

Yeah, oops, I missed that one. Thanks for reporting.

Cheers,
Ander

>  11932  
>  11933                  if (connector->new_encoder) {
>  11934                          if (connector->new_encoder != connector->encoder)
>  11935                                  connector->encoder = connector->new_encoder;
>  11936                  } else {
>  11937                          connector_state->crtc = NULL;
>  11938                  }
>  11939          }
> 
> 
> regards,
> dan carpenter


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Handle error to get connector state when staging config

[Daniel Vetter]

On Fri, Mar 27, 2015 at 03:33:51PM +0200, Ander Conselvan de Oliveira wrote:
> The return value of one of the calls to drm_atomic_get_connector_state()
> in intel_modeset_stage_output_state() wasn't checked for errors.
> 
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>

Queued for -next, thanks for the patch.
-Daniel
> ---
>  drivers/gpu/drm/i915/intel_display.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 68fe8b8..1329ed2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12000,6 +12000,8 @@ intel_modeset_stage_output_state(struct drm_device *dev,
>  	for_each_intel_connector(dev, connector) {
>  		connector_state =
>  			drm_atomic_get_connector_state(state, &connector->base);
> +		if (IS_ERR(connector_state))
> +			return PTR_ERR(connector_state);
>  
>  		if (connector->new_encoder) {
>  			if (connector->new_encoder != connector->encoder)
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: drm/i915: Copy the staged connector config to the legacy atomic state

[Dan Carpenter]

Hello Ander Conselvan de Oliveira,

This is a semi-automatic email about new static checker warnings.

The patch 944b0c765757: "drm/i915: Copy the staged connector config 
to the legacy atomic state" from Mar 20, 2015, leads to the following 
Smatch complaint:

drivers/gpu/drm/i915/intel_display.c:9118 intel_release_load_detect_pipe()
	 error: we previously assumed 'state' could be null (see line 9082)

drivers/gpu/drm/i915/intel_display.c
  9081			state = drm_atomic_state_alloc(dev);
  9082			if (!state)
  9083				goto fail;
                                ^^^^^^^^^
Patch changes a return into a goto.

  9084	
  9085			state->acquire_ctx = ctx;
  9086	
  9087			connector_state = drm_atomic_get_connector_state(state, connector);
  9088			if (IS_ERR(connector_state))
  9089				goto fail;
  9090	
  9091			to_intel_connector(connector)->new_encoder = NULL;
  9092			intel_encoder->new_crtc = NULL;
  9093			intel_crtc->new_enabled = false;
  9094			intel_crtc->new_config = NULL;
  9095	
  9096			connector_state->best_encoder = NULL;
  9097			connector_state->crtc = NULL;
  9098	
  9099			intel_set_mode(crtc, NULL, 0, 0, NULL, state);
  9100	
  9101			drm_atomic_state_free(state);
  9102	
  9103			if (old->release_fb) {
  9104				drm_framebuffer_unregister_private(old->release_fb);
  9105				drm_framebuffer_unreference(old->release_fb);
  9106			}
  9107	
  9108			return;
  9109		}
  9110	
  9111		/* Switch crtc and encoder back off if necessary */
  9112		if (old->dpms_mode != DRM_MODE_DPMS_ON)
  9113			connector->funcs->dpms(connector, old->dpms_mode);
  9114	
  9115		return;
  9116	fail:
  9117		DRM_DEBUG_KMS("Couldn't release load detect pipe.\n");
  9118		drm_atomic_state_free(state);
                                      ^^^^^
Dereferenced inside function call.

  9119	}
  9120	

regards,
dan carpenter
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Handle error to get connector state when staging config

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6081
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  276/276              276/276
ILK                                  303/303              303/303
SNB                                  304/304              304/304
IVB                 -1              330/330              329/330
BYT                                  287/287              287/287
HSW                                  361/361              361/361
BDW                                  309/309              309/309
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*IVB  igt@gem_pwrite_pread@snooped-copy-performance      PASS(5)      DMESG_WARN(1)PASS(1)
(dmesg patch applied)drm:i915_hangcheck_elapsed[i915]]*ERROR*Hangcheck_timer_elapsed...blitter_ring_idle@Hangcheck timer elapsed... blitter ring idle
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: drm/i915: Copy the staged connector config to the legacy atomic state

[Ander Conselvan De Oliveira]

On Fri, 2015-03-27 at 17:36 +0300, Dan Carpenter wrote:
> Hello Ander Conselvan de Oliveira,
> 
> This is a semi-automatic email about new static checker warnings.
> 
> The patch 944b0c765757: "drm/i915: Copy the staged connector config 
> to the legacy atomic state" from Mar 20, 2015, leads to the following 
> Smatch complaint:
> 
> drivers/gpu/drm/i915/intel_display.c:9118 intel_release_load_detect_pipe()
> 	 error: we previously assumed 'state' could be null (see line 9082)
> 
> drivers/gpu/drm/i915/intel_display.c
>   9081			state = drm_atomic_state_alloc(dev);
>   9082			if (!state)
>   9083				goto fail;
>                                 ^^^^^^^^^
> Patch changes a return into a goto.
> 
>   9084	
>   9085			state->acquire_ctx = ctx;
>   9086	
>   9087			connector_state = drm_atomic_get_connector_state(state, connector);
>   9088			if (IS_ERR(connector_state))
>   9089				goto fail;
>   9090	
>   9091			to_intel_connector(connector)->new_encoder = NULL;
>   9092			intel_encoder->new_crtc = NULL;
>   9093			intel_crtc->new_enabled = false;
>   9094			intel_crtc->new_config = NULL;
>   9095	
>   9096			connector_state->best_encoder = NULL;
>   9097			connector_state->crtc = NULL;
>   9098	
>   9099			intel_set_mode(crtc, NULL, 0, 0, NULL, state);
>   9100	
>   9101			drm_atomic_state_free(state);
>   9102	
>   9103			if (old->release_fb) {
>   9104				drm_framebuffer_unregister_private(old->release_fb);
>   9105				drm_framebuffer_unreference(old->release_fb);
>   9106			}
>   9107	
>   9108			return;
>   9109		}
>   9110	
>   9111		/* Switch crtc and encoder back off if necessary */
>   9112		if (old->dpms_mode != DRM_MODE_DPMS_ON)
>   9113			connector->funcs->dpms(connector, old->dpms_mode);
>   9114	
>   9115		return;
>   9116	fail:
>   9117		DRM_DEBUG_KMS("Couldn't release load detect pipe.\n");
>   9118		drm_atomic_state_free(state);
>                                       ^^^^^
> Dereferenced inside function call.

That's odd, this is checking the "fail" label inside another function.
The change above was in intel_get_load_detect_pipe(), and this is in
intel_release_load_detect_pipe().

Anyway, the fail path in intel_get_load_detect_pipe() does check for
state being NULL before using it.

Cheers,
Ander


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: drm/i915: Copy the staged connector config to the legacy atomic state

[David Weinehall]

On Fri, Mar 27, 2015 at 05:36:40PM +0300, Dan Carpenter wrote:
> Hello Ander Conselvan de Oliveira,
> 
> This is a semi-automatic email about new static checker warnings.
> 
> The patch 944b0c765757: "drm/i915: Copy the staged connector config 
> to the legacy atomic state" from Mar 20, 2015, leads to the following 
> Smatch complaint:
> 
> drivers/gpu/drm/i915/intel_display.c:9118 intel_release_load_detect_pipe()
> 	 error: we previously assumed 'state' could be null (see line 9082)
> 
> drivers/gpu/drm/i915/intel_display.c
>   9081			state = drm_atomic_state_alloc(dev);
>   9082			if (!state)
>   9083				goto fail;
>                                 ^^^^^^^^^
> Patch changes a return into a goto.
> 
>   9084	
>   9085			state->acquire_ctx = ctx;
>   9086	
>   9087			connector_state = drm_atomic_get_connector_state(state, connector);
>   9088			if (IS_ERR(connector_state))
>   9089				goto fail;
>   9090	
>   9091			to_intel_connector(connector)->new_encoder = NULL;
>   9092			intel_encoder->new_crtc = NULL;
>   9093			intel_crtc->new_enabled = false;
>   9094			intel_crtc->new_config = NULL;
>   9095	
>   9096			connector_state->best_encoder = NULL;
>   9097			connector_state->crtc = NULL;
>   9098	
>   9099			intel_set_mode(crtc, NULL, 0, 0, NULL, state);
>   9100	
>   9101			drm_atomic_state_free(state);
>   9102	
>   9103			if (old->release_fb) {
>   9104				drm_framebuffer_unregister_private(old->release_fb);
>   9105				drm_framebuffer_unreference(old->release_fb);
>   9106			}
>   9107	
>   9108			return;
>   9109		}
>   9110	
>   9111		/* Switch crtc and encoder back off if necessary */
>   9112		if (old->dpms_mode != DRM_MODE_DPMS_ON)
>   9113			connector->funcs->dpms(connector, old->dpms_mode);
>   9114	
>   9115		return;
>   9116	fail:
>   9117		DRM_DEBUG_KMS("Couldn't release load detect pipe.\n");
>   9118		drm_atomic_state_free(state);
>                                       ^^^^^
> Dereferenced inside function call.
> 
>   9119	}
>   9120	

Wouldn't it make more sense to make drm_atomic_state_free() follow
the same semantics as *free() functions typically do
(no operation performed)?


Kind regards, David Weinehall
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: drm/i915: Copy the staged connector config to the legacy atomic state

[Ander Conselvan De Oliveira]

On Mon, 2015-03-30 at 13:50 +0300, David Weinehall wrote:
> On Fri, Mar 27, 2015 at 05:36:40PM +0300, Dan Carpenter wrote:
> > Hello Ander Conselvan de Oliveira,
> > 
> > This is a semi-automatic email about new static checker warnings.
> > 
> > The patch 944b0c765757: "drm/i915: Copy the staged connector config 
> > to the legacy atomic state" from Mar 20, 2015, leads to the following 
> > Smatch complaint:
> > 
> > drivers/gpu/drm/i915/intel_display.c:9118 intel_release_load_detect_pipe()
> > 	 error: we previously assumed 'state' could be null (see line 9082)
> > 
> > drivers/gpu/drm/i915/intel_display.c
> >   9081			state = drm_atomic_state_alloc(dev);
> >   9082			if (!state)
> >   9083				goto fail;
> >                                 ^^^^^^^^^
> > Patch changes a return into a goto.
> > 
> >   9084	
> >   9085			state->acquire_ctx = ctx;
> >   9086	
> >   9087			connector_state = drm_atomic_get_connector_state(state, connector);
> >   9088			if (IS_ERR(connector_state))
> >   9089				goto fail;
> >   9090	
> >   9091			to_intel_connector(connector)->new_encoder = NULL;
> >   9092			intel_encoder->new_crtc = NULL;
> >   9093			intel_crtc->new_enabled = false;
> >   9094			intel_crtc->new_config = NULL;
> >   9095	
> >   9096			connector_state->best_encoder = NULL;
> >   9097			connector_state->crtc = NULL;
> >   9098	
> >   9099			intel_set_mode(crtc, NULL, 0, 0, NULL, state);
> >   9100	
> >   9101			drm_atomic_state_free(state);
> >   9102	
> >   9103			if (old->release_fb) {
> >   9104				drm_framebuffer_unregister_private(old->release_fb);
> >   9105				drm_framebuffer_unreference(old->release_fb);
> >   9106			}
> >   9107	
> >   9108			return;
> >   9109		}
> >   9110	
> >   9111		/* Switch crtc and encoder back off if necessary */
> >   9112		if (old->dpms_mode != DRM_MODE_DPMS_ON)
> >   9113			connector->funcs->dpms(connector, old->dpms_mode);
> >   9114	
> >   9115		return;
> >   9116	fail:
> >   9117		DRM_DEBUG_KMS("Couldn't release load detect pipe.\n");
> >   9118		drm_atomic_state_free(state);
> >                                       ^^^^^
> > Dereferenced inside function call.
> > 
> >   9119	}
> >   9120	
> 
> Wouldn't it make more sense to make drm_atomic_state_free() follow
> the same semantics as *free() functions typically do
> (no operation performed)?

It would. I already signed up to send that patch, even. Just didn't get
around to yet.

Ander

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: drm/i915: Copy the staged connector config to the legacy atomic state

[Dan Carpenter]

This is a typical "one err bug" which is a kind of bug where there is
just one err: label which does all the error handling.  One err bugs are
one of the most common class of bugs in the kernel.  "Oh, we didn't
initialize that struct member" or "That pointer is NULL."

In networking they have resisted patches to add sanity checks to
free_netdev() so that people can't do one err style error handling.  You
always have to know if the dev is allocated or not.

Having one error label makes sense if the label is "unlock:" but labels
like fail:, err:, out:, or bail are not very good label names.

The disadvantage of having multiple returns is that it means you can't
have a debug statement, but debug statements are mostly a waste of RAM
and we can use ftrace for that.

regards,
dan carpenter
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx