[PATCH] drm/crtc: fix connector reference counting mismatch in drm_crtc_helper_set_config

[Daniel Vetter]

On Thu, Jun 02, 2016 at 07:28:30PM +0200, Philipp Zabel wrote:
> Hi Daniel,
> 
> Am Donnerstag, den 02.06.2016, 18:21 +0200 schrieb Daniel Vetter:
> [...]
> > > Only the reference count of connectors that weren't previously bound to
> > > an encoder should be incremented after a call to drm_crtc_helper_set_config.
> > > And only the reference count of connectors that were previously bound to
> > > an encoder and are unbound afterwards should ever be decremented.
> > > The reference counts of the temporary copies in the save_connectors
> > > should not be touched at all.
> > > 
> > > This patch fixes the above error by only incrementing the reference count
> > > of those connectors in the set that are initially not bound to any encoder,
> > > and also by restoring the reference count of only those connectors in the
> > > set in the failure case.
> > > 
> > > Fixes: 0955c1250e96 ("drm/crtc: take references to connectors used in a modeset. (v2)")
> > > Signed-off-by: Philipp Zabel <p.zabel at pengutronix.de>
> > 
> > I think this is worse. We can't save/restore the connector when there's a
> > kref inside of it. But there's really only 2 things we need to save
> > restore:
> > - connector->encoder pointer, for each connector.
> > - encoder->crtc pointer, for each encoder.
> > 
> > For a proper fix I think we need to rectify that first, then apply your
> > bugfix on top. The refcount logic itself seems sound, but I've screwed
> > that up way too often already.
> 
> Ok. Do we have to do it first, though? If we first get rid of the
>     drm_connector_unreference(&save_connectors[count++]);
> part first, we can don't have to fix it up when the connector array goes
> away.

It's just a bugfix for the failure case. Doing a memcpy over a live kref
is definitely not something we should ever do, and will break the
refcounting. We want both patches in 4.7.

> > Also I'm somewhat confused about how you manage to blow things up. The
> > refcount of each connector we're seeing should be 1 already: Once from the
> > connector list, and a 2nd one from the lookup in the setcrtc ioctl code.
> 
> All I saw is that inside drm_crtc_helper_set_config all connectors in
> the set have their refcount incremented and then *all* connectors have
> their refcount decremented. Isn't the lookup refcount increment only
> done for connectors in the set?
> I see it look up connector 36, then reference it once more as part of
> the set. At the end it tries to unreference connector 34 from
> save_connectors, followed by the error:
> 
>     [drm:drm_ioctl] pid=205, dev=0xe201, auth=1, DRM_IOCTL_MODE_SETCRTC
>     [drm:drm_mode_setcrtc] [CRTC:24:crtc-0]
>     [drm:drm_mode_setcrtc] [CONNECTOR:36:LVDS-1]
>     [drm:drm_crtc_helper_set_config] 
>     [drm:drm_crtc_helper_set_config] [CRTC:24:c 13.982686] [drm:drm_mode_debug_printmodeline] Modeline 0:"" 0 0 0 0 0 0 0 0 0 0 0x0 0x0
>     [drm:drm_mode_debug_printmodeline] Modeline 41:"1280x800" 60 71100 1280 1281 1439 1440 800 801 822 823 0x40 0x0
>     [drm:drm_mode_object_reference] OBJ ID: 36 (2)
>     [drm:drm_crtc_helper_set_config] encoder changed, full mode switch
>     [drm:drm_crtc_helper_set_config] crtc changed, full mode switch
>     [drm:drm_crtc_helper_set_config] [CONNECTOR:36:LVDS-1] to [CRtc-0]
>     [drm:drm_crtc_helper_set_mode] [ENCODER:35:LVDS-35] set [MODE:41:1280x800]
>     [drm:drm_calc_timestamping_constants] crtc 24: hwmode: htotal 1440, vtotal 823, vdisplay 800
>     [drm:drm_calc_timestamping_constants] crtc 24: clock 71100 kHz framedur 16668354 linedur 20253
>     [drm:drm_crtc_helper_set_config] Setting connector DPMS state to on
>     [drm:drm_crtc_helper_set_config] 	[CONNECTOR:36:LVDS-1] set DPMS on
>     [drm:drm_mode_object_unreference] OBJ ID: 34 (1)

The additional get/put I'm talking off happens in drm_mode_setcrtc (the
get is hidden in drm_connector_lookup). Might be interesting to trace the
refcount for each connector after each get and before each put.

> > Which means when we drop that 1 reference in the saved connector (which is
> > entirely bogus code, I agree) it should only drop to 1, not 0. And youre
> > cleanup code should not be called.
> >
> > The other thing is that radeon/nouveau also uses this code, and no one
> > complained yet. Together I think that's some good evidence that suggests
> > there's also something strange going on in imx itself.
> 
> That's what I wonder, too. Right now I can't see it though.

Yep, something funny is definitely going on here.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch