On Tue, Jun 14, 2016 at 06:08:35PM +0200, Daniel Vetter wrote:
> On Thu, Jun 02, 2016 at 07:27:51PM +0200, Philipp Zabel wrote:
> > Since commit 0955c1250e96 ("drm/crtc: take references to connectors used
> > in a modeset. (v2)"), the reference counts of all connectors in the
> > drm_mode_set given to drm_crtc_helper_set_config are incremented, and then
> > the reference counts of all connectors are decremented on success, but in a
> > temporary copy of the connector structure. This leads to the following
> > error after the first modeset on imx-drm:
> >
> > Unable to handle kernel NULL pointer dereference at virtual address 00000004
> > pgd = ad8c4000
> > [00000004] *pgd=3d9c5831, *pte=00000000, *ppte=00000000
> > Internal error: Oops: 817 [#1] PREEMPT SMP ARM
> > Modules linked in:
> > CPU: 1 PID: 190 Comm: kmsfb-manage Not tainted 4.7.0-rc1+ #657
> > Hardware name: Freescale i.MX6 Quad/DualLit: [<80506098>] lr : [<80252e94>] psr: 200c0013
> > sp : adca7ca8 ip : adca7b90 fp : adca7cd4
> > r10: 00000000 r9 : 00000100 r8 : 00000200
> > r7 : af3c9800 r6 : aded7848 r5 : aded7800 r4 : 00000000
> > r3 : af3ca058 r2 : 00000200 r1 : af3ca058 r0 : 00000000
> > Flags: nzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none
> > Control: 10c5387d Table: 3d8c404a DAC: 00000051
> > Process kmsfb-manage (pid: 190, stack limit = 0xadca6210)
> > Stack: (0xadca7ca8 to 0xadca8000)
> > 7ca0: 805190e0 aded7800 aded7820 80501a88 8155a290 af3c9c6c
> > 7cc0: adca7ddc 0000000f adca7cec adca7cd8 80519104 80506044 805190e0 aded7800
> > 7ce0: adca7d04 adca7cf0 80501ac0 805190ec aded7820 aded7814 adca7d24 adca7d08
> > 7d00: 804fdb80 80501a94 aded7800 af3ca010 aded7afc af3c9c60 adca7d94 adca7d28
> > 7d20: 804e3518 804fdb20 00000000 af3c9b1c adca7d50 81506f44 00000000 8093c500
> > 7d40: af3c9c6c ae4f2ca8 ae4f2c18 00000000 00000000 ae637f00 00000000 aded7800
> > 7d60: 00000001 af3c9800 af23c300 ae77fcc0 ae4f2c18 00000001 af3c9800 8155a290
> > 7d80: af1af700 adca6000 adca7db4 adca7d98 804fea6c 804e2de4 adca7e50 adb3d940
> > 7da0: 00000001 af3c9800 adca7e24 adca7db8 8050440c 804fea0c ae77fcc0 00000003
> > 7dc0: adca7e24 adb3d940 af1af700 ae77fcc0 ae77fccc ae4f2c18 8083d44c ae77fcc0
> > 7de0: ae4002 80d03040 adca7e64 adca7e40 adca7e50 80503f08
> > 7e40: 7ebd5630 adca7e50 00000068 c06864a2 7ebd5be8 00000000 00000001 00000018
> > 7e60: 00000026 00000000 00000000 00000000 00000001 000115bc 05010500 05a0059f
> > 7e80: 03200000 03360321 00000337 0000003c 00000000 00000040 30383231 30303878
> > 7ea0: 00000000 00000000 00000000 00000000 00000000 00000000 80173058 80172e30
> > 7ec0: 80d77d32 00004000 adf7d900 00000003 00000000 7ebd5630 af342bb0 adfe3b80
> > 7ee0: 80272f50 00000003 adca6000 00000000 adca7f7c adca7f00 802725ec 804f52cc
> > 7f00: 802809cc 80178450 00000000 00000000 80280880 80145904 adb3d8c0 adf7d990
> > 7f20: ffffffff 00000003 00004000 01614c10 c06864a2 00000003 adca6000 00000000
> > 7f40: adca7f6c adca7f50 80280b04 8028088c 000115bc adfe3b81 7ebd5630 adfe3b80
> > 7f60: c06864a2 00000003 adca6000 00000000 adca7fa4 adca7f80 80272f50 80272548
> > 7f80: 000115bc 00017050 00000001 01614c10 00000036 801089e4 00000000 adca7fa8
> > 7fa0: 80108840 80272f18 00017050 00000001 00000003 c06864a2 7ebd5630 000115bc
> > 7fc0: 00017050 00000001 01614c10 00000036 00000003 00000000 00000026 00000018
> > 7fe0: 00016f38 7ebd562c 0000b5e9 76ef31e6 400c0030 00000003 ff5f37db bfe7dd4d
> > Backtrace:
> > [<80506038>] (drm_connector_cleanup) from [<80519104>] (dw_hdmi_connector_destroy+0x24/0x28)
> > r10:0000000f r9:adca7ddc r8:af3c9c6c r7:8155a290 r6:80501a88 r5:aded7820
> > r4:aded7800 r3:805190e0
> > [<805190e0>] (dw_hdmi_connector_destroy) from [<80501ac0>] (drm_connector_free+0x38/0x3c)
> > r4:aded7800 nreference) from [<804e3518>] (drm_crtc_helper_set_config+0x740/0xbf4)
> > r6:af3c9c60 r5:aded7afc r4:af3ca010 r3:aded7800
> > [<804e2dd8>] (drm_crtc_helper_set_config) from [<804fea6c>] (drm_mode_set_config_internal+0x6c/0xf4)
> > r10:adca6000 r9:af1af700 r8:8155a290 r7:af3c9800 r6:00000001 r5:ae4f2c18
> > r4:ae77fcc0
> > [<804fea00>] (drm_mode_set_config_internal) from [<8050440c>] (drm_mode_setcrtc+0x504/0x57c)
> > r7:af3c9800 r6:00000001 r5:adb3d940 r4:adca7e50
> > [<80503f08>] (drm_mode_setcrtc) from [<804f5404>] (drm_ioctl+0x144/0x4dc)
> > r10:ada2e000 r9:000000a2 r8:af3c9800 r7:8155a290 r6:809320b4 r5:00000051
> > r4:adca7e50
> > [<804f52c0>] (drm_ioctl) from [<802725ec>] (do_vfs_ioctl+0xb0/0x9d0)
> > r10:00000000 r9:adca6000 r8:00000003 r7:80272f50 r6:adfe3b80 r5:af342bb0
> > r4:7ebd5630
> > [<8027253c>] (do_vfs_ioctl) from [<80272f50>] (SyS_ioctl+0x44/0x6c)
> > r10:00000000 r9:adca6000 r8:00000003 r7:c06864a2 r6:adfe3b80 r5:7ebd5630
> > r4:adfe3b81
> > [<80272f0c>] (SyS_ioctl) from [<80108840>] (ret_fast_syscall+0x0/0x1c)
> > r8:801089e4 r7:00000036 r6:01614c10 r5:00000001 r4:00017050 r3:000115bc
> > Code: 0a00000c e5932004 e1a01003 e1a0a004 (e5842004)
> > ---[ end trace 9a7257572ccacb16 ]---
> >
> > 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@xxxxxxxxxxxxxx>
>
> Commit message needs to be augmented:
>
> "Note that this can only be hit when fbdev emulation is disabled, since
> then the refcount drops from 1 to 0 and we call the connector destroy
> functions on the backup copy, which eventually results in tears. With
> fbdev emulation the refcount only goes down from 2 to 1 ever. And since we
> unconditionally increment the refcount on the real object, the refcount of
> that will slowly increase. The backup connector's refcount doesn't matter,
> since we kfree() that either way in the end of
> drm_crtc_helper_set_config()."
> > ---
> > drivers/gpu/drm/drm_crtc_helper.c | 18 ++++++++++--------
> > 1 file changed, 10 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> > index a6e4243..1c4d674 100644
> > --- a/drivers/gpu/drm/drm_crtc_helper.c
> > +++ b/drivers/gpu/drm/drm_crtc_helper.c
> > @@ -631,8 +631,12 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set)
> > mode_changed = true;
> > }
> >
> > - /* take a reference on all connectors in set */
> > + /* take a reference on all unbound connectors in set, reuse the
> > + * already taken reference for bound connectors
> > + */
> > for (ro = 0; ro < set->num_connectors; ro++) {
> > + if (set->connectors[ro]->encoder)
> > + continue;
>
> The real bugfix is that you're switching from save_connectors to
> set->connectors. This filtering here should be irrelevant.
>
> > drm_connector_reference(set->connectors[ro]);
> > }
> >
> > @@ -754,12 +758,6 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set)
> > }
> > }
> >
> > - /* after fail drop reference on all connectors in save set */
> > - count = 0;
> > - drm_for_each_connector(connector, dev) {
> > - drm_connector_unreference(&save_connectors[count++]);
>
> And we need to keep the unreference here (but on set->connectors instead),
> otherwise this will leak badly. In short this is the only place you need
> to fix up, the other hunks shouldn't be needed.
>
> > - }
> > -
> > kfree(save_connectors);
> > kfree(save_encoders);
> > return 0;
> > @@ -776,8 +774,12 @@ fail:
> > *connector = save_connectors[count++];
> > }
> >
> > - /* after fail drop reference on all connectors in set */
> > + /* after fail drop reference on all unbound connectors in set, let
> > + * bound connectors keep their reference
> > + */
> > for (ro = 0; ro < set->num_connectors; ro++) {
> > + if (set->connectors[ro]->encoder)
> > + continue;
>
> Again, filtering on ->encoder is superflous.
>
> > drm_connector_unreference(set->connectors[ro]);
> > }
>
> With the bugs in the code fixed and the commit message augmented:
Ok, I thought about this some more while eating, and I was totally off the
tracks. The patch is sound, but still needs the commit message ammended.
With that:
Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
