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

[Ander Conselvan De Oliveira <conselvan2@gmail.com>]

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