[PATCH 2/2] drm/dp_mst: Avoid to mess up payload table by ports in stale topology

[Lin, Wayne]

[Public]

________________________________________
> From: Lyude Paul <lyude at redhat.com>
> Sent: Wednesday, June 16, 2021 03:44
> To: Lin, Wayne; dri-devel at lists.freedesktop.org
> Cc: Kazlauskas, Nicholas; Wentland, Harry; Zuo, Jerry; Pillai, Aurabindo
> Subject: Re: [PATCH 2/2] drm/dp_mst: Avoid to mess up payload table by ports in stale topology
>
> On Fri, 2021-05-28 at 21:55 +0800, Wayne Lin wrote:
> > [Why]
> > After unplug/hotplug hub from the system, userspace might start to
> > clear stale payloads gradually. If we call drm_dp_mst_deallocate_vcpi()
> > to release stale VCPI of those ports which are not relating to current
> > topology, we have chane to wrongly clear active payload table entry for
> > current topology.
> >
> > E.g.
> > We have allocated VCPI 1 in current payload table and we call
> > drm_dp_mst_deallocate_vcpi() to clean VCPI 1 in stale topology. In
> > drm_dp_mst_deallocate_vcpi(), it will call drm_dp_mst_put_payload_id()
> > tp put VCPI 1 and which means ID 1 is available again. Thereafter, if we
> > want to allocate a new payload stream, it will find ID 1 is available by
> > drm_dp_mst_assign_payload_id(). However, ID 1 is being used
> >
> > [How]
> > Check target sink is relating to current topology or not before doing
> > any payload table update.
> > Searching upward to find the target sink's relevant root branch device.
> > If the found root branch device is not the same root of current
> > topology, don't update payload table.
> >
> > Signed-off-by: Wayne Lin <Wayne.Lin at amd.com>
> > ---
> >  drivers/gpu/drm/drm_dp_mst_topology.c | 29 +++++++++++++++++++++++++++
> >  1 file changed, 29 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > index 5fc261014a20..3d988d54df89 100644
> > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > @@ -94,6 +94,9 @@ static int drm_dp_mst_register_i2c_bus(struct
> > drm_dp_mst_port *port);
> >  static void drm_dp_mst_unregister_i2c_bus(struct drm_dp_mst_port *port);
> >  static void drm_dp_mst_kick_tx(struct drm_dp_mst_topology_mgr *mgr);
> >
> > +static bool drm_dp_mst_port_downstream_of_branch(struct drm_dp_mst_port
> > *port,
> > +                                                struct drm_dp_mst_branch
> > *branch);
> > +
> >  #define DBG_PREFIX "[dp_mst]"
> >
> >  #define DP_STR(x) [DP_ ## x] = #x
> > @@ -3360,6 +3363,7 @@ int drm_dp_update_payload_part1(struct
> > drm_dp_mst_topology_mgr *mgr)
> >         struct drm_dp_mst_port *port;
> >         int i, j;
> >         int cur_slots = 1;
> > +       bool skip;
> >
> >         mutex_lock(&mgr->payload_lock);
> >         for (i = 0; i < mgr->max_payloads; i++) {
> > @@ -3374,6 +3378,14 @@ int drm_dp_update_payload_part1(struct
> > drm_dp_mst_topology_mgr *mgr)
> >                         port = container_of(vcpi, struct drm_dp_mst_port,
> >                                             vcpi);
> >
> > +                       mutex_lock(&mgr->lock);
> > +                       skip = !drm_dp_mst_port_downstream_of_branch(port,
> > mgr->mst_primary);
> > +                       mutex_unlock(&mgr->lock);
> > +
> > +                       if (skip) {
> > +                               DRM_DEBUG_KMS("Virtual channel %d is not in
> > current topology\n", i);
>
> sorry I totally missed this on my first look - but this is the wrong debugging
> macro (drm_dbg_kms() should be used) and as well, this patch doesn't actually
> apply because it needs to be rebased against drm-tip. Could you fix this,
> rebase the patches, and send a new version along with the fixes tags I
> mentioned earlier? You can generate them using the dim tool here:
>
> https://drm.pages.freedesktop.org/maintainer-tools/committer-drm-intel.html#labeling-fixes-before-pushing
>

Thanks for your time! Will do.

> > +                               continue;
> > +                       }
> >                         /* Validated ports don't matter if we're releasing
> >                          * VCPI
> >                          */
> > @@ -3473,6 +3485,7 @@ int drm_dp_update_payload_part2(struct
> > drm_dp_mst_topology_mgr *mgr)
> >         struct drm_dp_mst_port *port;
> >         int i;
> >         int ret = 0;
> > +       bool skip;
> >
> >         mutex_lock(&mgr->payload_lock);
> >         for (i = 0; i < mgr->max_payloads; i++) {
> > @@ -3482,6 +3495,13 @@ int drm_dp_update_payload_part2(struct
> > drm_dp_mst_topology_mgr *mgr)
> >
> >                 port = container_of(mgr->proposed_vcpis[i], struct
> > drm_dp_mst_port, vcpi);
> >
> > +               mutex_lock(&mgr->lock);
> > +               skip = !drm_dp_mst_port_downstream_of_branch(port, mgr-
> > >mst_primary);
> > +               mutex_unlock(&mgr->lock);
> > +
> > +               if (skip)
> > +                       continue;
> > +
> >                 DRM_DEBUG_KMS("payload %d %d\n", i, mgr-
> > >payloads[i].payload_state);
> >                 if (mgr->payloads[i].payload_state == DP_PAYLOAD_LOCAL) {
> >                         ret = drm_dp_create_payload_step2(mgr, port, mgr-
> > >proposed_vcpis[i]->vcpi, &mgr->payloads[i]);
> > @@ -4562,9 +4582,18 @@ EXPORT_SYMBOL(drm_dp_mst_reset_vcpi_slots);
> >  void drm_dp_mst_deallocate_vcpi(struct drm_dp_mst_topology_mgr *mgr,
> >                                 struct drm_dp_mst_port *port)
> >  {
> > +       bool skip;
> > +
> >         if (!port->vcpi.vcpi)
> >                 return;
> >
> > +       mutex_lock(&mgr->lock);
> > +       skip = !drm_dp_mst_port_downstream_of_branch(port, mgr->mst_primary);
> > +       mutex_unlock(&mgr->lock);
> > +
> > +       if (skip)
> > +               return;
> > +
> >         drm_dp_mst_put_payload_id(mgr, port->vcpi.vcpi);
> >         port->vcpi.num_slots = 0;
> >         port->vcpi.pbn = 0;
>
> --
> Cheers,
>  Lyude Paul (she/her)
>  Software Engineer at Red Hat
--
Regards,
Wayne Lin