diff mbox

drm/dp-mst: Remove branches before dropping the reference

Message ID 1418075722-28487-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive)
State New, archived
Headers show

Commit Message

Daniel Vetter Dec. 8, 2014, 9:55 p.m. UTC 
When we unplug a dp mst branch we unreference the entire tree from
the root towards the leaves. Which is ok, since that's the way the
pointers and so also the refcounts go.

But when we drop the reference we must make sure that we remove the
branches/ports from the lists/pointers before dropping the reference.
Otherwise the get_validated functions will still return it instead
of returning NULL (which indicates a potentially on-going unplug).

The mst branch destroy gets this right for ports: First it deletes
the port from the ports list, then it unrefs. But the ports destroy
function gets it wrong: First it unrefs, then it drops the ref. Which
means a zombie mst branch can still be validate with get_validated_mstb_ref
when it shouldn't.

Fix this.

This should address a backtrace Dave dug out somewhere on unplug:

 [<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]

Cc: Dave Airlie <airlied@redhat.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Daniel Martin Dec. 9, 2014, 8:31 a.m. UTC | #1 
On 8 December 2014 at 22:55, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> When we unplug a dp mst branch we unreference the entire tree from
> the root towards the leaves. Which is ok, since that's the way the
> pointers and so also the refcounts go.
>
> But when we drop the reference we must make sure that we remove the
> branches/ports from the lists/pointers before dropping the reference.
> Otherwise the get_validated functions will still return it instead
> of returning NULL (which indicates a potentially on-going unplug).
>
> The mst branch destroy gets this right for ports: First it deletes
> the port from the ports list, then it unrefs. But the ports destroy
> function gets it wrong: First it unrefs, then it drops the ref. Which
> means a zombie mst branch can still be validate with get_validated_mstb_ref
> when it shouldn't.
>
> Fix this.
>
> This should address a backtrace Dave dug out somewhere on unplug:
>
>  [<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]
>
> Cc: Dave Airlie <airlied@redhat.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 5682d7e9f1ec..71a56d65a0d2 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -839,6 +839,8 @@ static void drm_dp_put_mst_branch_device(struct drm_dp_mst_branch *mstb)
>
>  static void drm_dp_port_teardown_pdt(struct drm_dp_mst_port *port, int old_pdt)
>  {
> +       struct drm_dp_mst_branch *mstb;
> +
>         switch (old_pdt) {
>         case DP_PEER_DEVICE_DP_LEGACY_CONV:
>         case DP_PEER_DEVICE_SST_SINK:
> @@ -846,8 +848,9 @@ static void drm_dp_port_teardown_pdt(struct drm_dp_mst_port *port, int old_pdt)
>                 drm_dp_mst_unregister_i2c_bus(&port->aux);
>                 break;
>         case DP_PEER_DEVICE_MST_BRANCHING:
> -               drm_dp_put_mst_branch_device(port->mstb);
> +               mstb = port_mstb;

drivers/gpu/drm/drm_dp_mst_topology.c:851:10: error: ‘port_mstb’ undeclared

This needs to be port->mstb. Spotted while testing this for
    https://bugs.freedesktop.org/show_bug.cgi?id=87099

>                 port->mstb = NULL;
> +               drm_dp_put_mst_branch_device(mstb);
>                 break;
>         }
>  }
> --
> 2.1.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
David Airlie Dec. 9, 2014, 8:51 p.m. UTC | #2 
> On 8 December 2014 at 22:55, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > When we unplug a dp mst branch we unreference the entire tree from
> > the root towards the leaves. Which is ok, since that's the way the
> > pointers and so also the refcounts go.
> >
> > But when we drop the reference we must make sure that we remove the
> > branches/ports from the lists/pointers before dropping the reference.
> > Otherwise the get_validated functions will still return it instead
> > of returning NULL (which indicates a potentially on-going unplug).
> >
> > The mst branch destroy gets this right for ports: First it deletes
> > the port from the ports list, then it unrefs. But the ports destroy
> > function gets it wrong: First it unrefs, then it drops the ref. Which
> > means a zombie mst branch can still be validate with get_validated_mstb_ref
> > when it shouldn't.
> >
> > Fix this.
> >
> > This should address a backtrace Dave dug out somewhere on unplug:
> >
> >  [<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]
> >
> > Cc: Dave Airlie <airlied@redhat.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/drm_dp_mst_topology.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > index 5682d7e9f1ec..71a56d65a0d2 100644
> > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > @@ -839,6 +839,8 @@ static void drm_dp_put_mst_branch_device(struct
> > drm_dp_mst_branch *mstb)
> >
> >  static void drm_dp_port_teardown_pdt(struct drm_dp_mst_port *port, int
> >  old_pdt)
> >  {
> > +       struct drm_dp_mst_branch *mstb;
> > +
> >         switch (old_pdt) {
> >         case DP_PEER_DEVICE_DP_LEGACY_CONV:
> >         case DP_PEER_DEVICE_SST_SINK:
> > @@ -846,8 +848,9 @@ static void drm_dp_port_teardown_pdt(struct
> > drm_dp_mst_port *port, int old_pdt)
> >                 drm_dp_mst_unregister_i2c_bus(&port->aux);
> >                 break;
> >         case DP_PEER_DEVICE_MST_BRANCHING:
> > -               drm_dp_put_mst_branch_device(port->mstb);
> > +               mstb = port_mstb;
> 
> drivers/gpu/drm/drm_dp_mst_topology.c:851:10: error: ‘port_mstb’ undeclared
> 
> This needs to be port->mstb. Spotted while testing this for
>     https://bugs.freedesktop.org/show_bug.cgi?id=87099

The version I checked in fixed this.

Dave.
diff mbox

Patch

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 5682d7e9f1ec..71a56d65a0d2 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -839,6 +839,8 @@  static void drm_dp_put_mst_branch_device(struct drm_dp_mst_branch *mstb)
 
 static void drm_dp_port_teardown_pdt(struct drm_dp_mst_port *port, int old_pdt)
 {
+	struct drm_dp_mst_branch *mstb;
+
 	switch (old_pdt) {
 	case DP_PEER_DEVICE_DP_LEGACY_CONV:
 	case DP_PEER_DEVICE_SST_SINK:
@@ -846,8 +848,9 @@  static void drm_dp_port_teardown_pdt(struct drm_dp_mst_port *port, int old_pdt)
 		drm_dp_mst_unregister_i2c_bus(&port->aux);
 		break;
 	case DP_PEER_DEVICE_MST_BRANCHING:
-		drm_dp_put_mst_branch_device(port->mstb);
+		mstb = port_mstb;
 		port->mstb = NULL;
+		drm_dp_put_mst_branch_device(mstb);
 		break;
 	}
 }