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

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