Skip to contentSkip to conversation

drm/mst: use kref_get_unless_zero for looking up mst branch device

PATCH In review Dave Airlie 3 participants 3 messages Nov 10, 2014

Subsystems (2)
DRM drivers DRM misc
lore
Share & tools
lore

Dave Airlie

author maintainer
#0 lore raw

PATCH 1/1 drm/mst: use kref_get_unless_zero for looking up mst branch device

From: Dave Airlie <[email protected]>

The backtrace below shows that while we tear down a branch device, when we remove the connector, we can attempt to disable MST on the port we are tearing down, since the port is going away, there it has a refcount of 0 already, so the validated ref lookup is trying to kref_get again and failing.

Use kref_get_unless_zero and just don't return anything if we are looking up the object we are currently also cleaning.

 [<ffffffffa00cc262>] drm_dp_mst_get_validated_mstb_ref_locked+0x92/0xa0 [drm_kms_helper]
 [<ffffffffa00cc211>] drm_dp_mst_get_validated_mstb_ref_locked+0x41/0xa0 [drm_kms_helper]
 [<ffffffffa00cc2aa>] drm_dp_get_validated_mstb_ref+0x3a/0x60 [drm_kms_helper]
 [<ffffffffa00cc2fb>] drm_dp_payload_send_msg.isra.14+0x2b/0x100 [drm_kms_helper]
 [<ffffffffa00cc547>] drm_dp_update_payload_part1+0x177/0x360 [drm_kms_helper]
 [<ffffffffa015c52e>] intel_mst_disable_dp+0x3e/0x80 [i915]
 [<ffffffffa013d60b>] haswell_crtc_disable+0x1cb/0x340 [i915]
 [<ffffffffa0136739>] intel_crtc_control+0x49/0x100 [i915]
 [<ffffffffa0136857>] intel_crtc_update_dpms+0x67/0x80 [i915]
 [<ffffffffa013fa59>] intel_connector_dpms+0x59/0x70 [i915]
 [<ffffffffa015c752>] intel_dp_destroy_mst_connector+0x32/0xc0 [i915]
 [<ffffffffa00cb44b>] drm_dp_destroy_port+0x6b/0xa0 [drm_kms_helper]
 [<ffffffffa00cb588>] drm_dp_destroy_mst_branch_device+0x108/0x130 [drm_kms_helper]
 [<ffffffffa00cb3cd>] drm_dp_port_teardown_pdt+0x3d/0x50 [drm_kms_helper]
 [<ffffffffa00cdb79>] drm_dp_mst_handle_up_req+0x499/0x540 [drm_kms_helper]
 [<ffffffff810d9ead>] ? trace_hardirqs_on_caller+0x15d/0x200
 [<ffffffffa00cdc73>] drm_dp_mst_hpd_irq+0x53/0xa00 [drm_kms_helper]
 [<ffffffffa00c7dfb>] ? drm_dp_dpcd_read+0x1b/0x20 [drm_kms_helper]
 [<ffffffffa0153ed8>] ? intel_dp_dpcd_read_wake+0x38/0x70 [i915]
 [<ffffffffa015a225>] intel_dp_check_mst_status+0xb5/0x250 [i915]
 [<ffffffffa015ac71>] intel_dp_hpd_pulse+0x181/0x210 [i915]
 [<ffffffffa01104f6>] i915_digport_work_func+0x96/0x120 [i915]
Signed-off-by Dave Airlie <[email protected]>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/gpu/drm/drm_dp_mst_topology.c +4 -2
index ce1113c..f703a5b 100644
@@ -882,8 +882,10 @@ static struct drm_dp_mst_branch *drm_dp_mst_get_validated_mstb_ref_locked(struct
 	struct drm_dp_mst_port *port;
 	struct drm_dp_mst_branch *rmstb;
 	if (to_find == mstb) {
-		kref_get(&mstb->kref);
-		return mstb;
+		if (!kref_get_unless_zero(&mstb->kref))
+			return NULL;
+		else
+			return mstb;
 	}
 	list_for_each_entry(port, &mstb->ports, next) {
 		if (port->mstb) {
Signature · 6 lines
2.1.0

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
#1 lore raw
Quoted 7 lines · Dave Airlie
drivers/gpu/drm/drm_dp_mst_topology.c +0 -0
index ce1113c..f703a5b 100644
@@ -882,8 +882,10 @@ static struct drm_dp_mst_branch *drm_dp_mst_get_validated_mstb_ref_locked(struct
        struct drm_dp_mst_port *port;
        struct drm_dp_mst_branch *rmstb;

drivers/gpu/drm/drm_dp_mst_topology.c static struct drm_dp_mst_branch *drm_dp_mst_get_validated_mstb_ref_locked(struct · line 891

Quoted 10 lines
drivers/gpu/drm/drm_dp_mst_topology.c +4 -2
        if (to_find == mstb) {
-               kref_get(&mstb->kref);
-               return mstb;
+               if (!kref_get_unless_zero(&mstb->kref))
+                       return NULL;
+               else
+                       return mstb;
        }
        list_for_each_entry(port, &mstb->ports, next) {
                if (port->mstb) {

kref_get_unless_zero only works with the lookup structure is protected through some locking. But the list removal in drm_dp_destroy_mst_branch_device is outside the mgr->glock, so that needs to be moved I think. That's not directly required by kref_get_unless_zero, just another effect of the ports list not holding a full reference.

Aside for paranoia I'd put a mutex check next to the kref_get_unles_zero, just to document which lock protects this weak reference. And even more tangential: We should probably flatten the recursion, or add a comment stating that the dp spec limits recursion sufficiently.
-Daniel

Signature · 7 lines
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Tested-by on v1 1/1
#2 lore raw

Re: [PATCH] drm/mst: use kref_get_unless_zero for looking

In reply to Dave Airlie
Tested-by PRC QA PRTS (Patch Regression Test System Contact: [email protected])

-------------------------------------Summary-------------------------------------
Platform: baseline_drm_intel_nightly_pass_rate->patch_applied_pass_rate
BYT: pass/total=249/348->248/348
PNV: pass/total=323/328->325/328
ILK: pass/total=328/330->330/330
IVB: pass/total=545/546->545/546
SNB: pass/total=551/556->550/556
HSW: pass/total=577/581->570/581
BDW: pass/total=435/435->434/435
-------------------------------------Detailed-------------------------------------
test_platform: test_suite, test_case, result_with_drm_intel_nightly(count, machine_id...)...->result_with_patch_applied(count, machine_id)...
BYT: Intel_gpu_tools, igt_gem_bad_reloc_negative-reloc-lut, PASS(4, M36) -> NSPT(1, M36)PASS(3, M36)
PNV: Intel_gpu_tools, igt_gem_concurrent_blit_gpu-bcs-early-read, PASS(6, M23M24) -> DMESG_WARN(1, M24)PASS(3, M24)
PNV: Intel_gpu_tools, igt_gem_linear_blits_normal, NSPT(1, M23)PASS(3, M24) -> PASS(4, M24)
PNV: Intel_gpu_tools, igt_gem_mmap_offset_exhaustion, DMESG_WARN(2, M23M24)PASS(2, M24) -> PASS(4, M24)
PNV: Intel_gpu_tools, igt_gem_unref_active_buffers, DMESG_WARN(1, M23)PASS(3, M24) -> PASS(4, M24)
ILK: Intel_gpu_tools, igt_kms_render_gpu-blit, DMESG_WARN(1, M26)PASS(3, M6) -> PASS(4, M6)
ILK: Intel_gpu_tools, igt_kms_flip_wf_vblank-vs-dpms-interruptible, DMESG_WARN(1, M26)PASS(3, M6) -> PASS(4, M6)
IVB: Intel_gpu_tools, igt_gem_bad_reloc_negative-reloc, NSPT(1, M4)PASS(3, M34M4) -> NSPT(2, M4)PASS(2, M4)
IVB: Intel_gpu_tools, igt_kms_plane_plane-position-covered-pipe-A-plane-1, TIMEOUT(1, M34)PASS(3, M4) -> PASS(4, M4)
SNB: Intel_gpu_tools, igt_gem_concurrent_blit_gpuX-bcs-overwrite-source-forked, PASS(4, M35M22) -> FAIL(1, M22)PASS(3, M22)
HSW: Intel_gpu_tools, igt_gem_bad_reloc_negative-reloc, NSPT(1, M39)PASS(3, M40M39) -> NSPT(2, M39)PASS(2, M39)
HSW: Intel_gpu_tools, igt_kms_cursor_crc_cursor-128x128-onscreen, PASS(4, M40M39) -> DMESG_WARN(2, M39)PASS(2, M39)
HSW: Intel_gpu_tools, igt_kms_cursor_crc_cursor-256x256-offscreen, PASS(4, M40M39) -> DMESG_WARN(2, M39)PASS(2, M39)
HSW: Intel_gpu_tools, igt_kms_cursor_crc_cursor-64x64-offscreen, DMESG_WARN(1, M39)PASS(3, M40M39) -> DMESG_WARN(2, M39)PASS(2, M39)
HSW: Intel_gpu_tools, igt_kms_cursor_crc_cursor-64x64-onscreen, DMESG_WARN(1, M39)PASS(3, M40M39) -> DMESG_WARN(1, M39)PASS(3, M39)
HSW: Intel_gpu_tools, igt_kms_cursor_crc_cursor-64x64-sliding, DMESG_WARN(1, M39)PASS(3, M40M39) -> DMESG_WARN(1, M39)PASS(3, M39)
HSW: Intel_gpu_tools, igt_kms_mmio_vs_cs_flip_setplane_vs_cs_flip, PASS(4, M40M39) -> DMESG_WARN(1, M39)PASS(3, M39)
BDW: Intel_gpu_tools, igt_gem_reset_stats_ban-bsd, PASS(4, M30) -> DMESG_WARN(1, M30)PASS(3, M30)
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx