1. 11 Jan, 2019 14 commits
    • Lyude Paul's avatar
      drm/nouveau: Keep malloc references to MST ports · 81640f01
      Lyude Paul authored
      Now that we finally have a sane way to keep port allocations around, use
      it to fix the potential unchecked ->port accesses that nouveau makes by
      making sure we keep the mst port allocated for as long as it's
      drm_connector is accessible.
      
      Additionally, now that we've guaranteed that mstc->port is allocated for
      as long as we keep mstc around we can remove the connector registration
      checks for codepaths which release payloads, allowing us to release
      payloads on active topologies properly. These registration checks were
      only required before in order to avoid situations where mstc->port could
      technically be pointing at freed memory.
      Signed-off-by: default avatarLyude Paul <lyude@redhat.com>
      Reviewed-by: default avatarBen Skeggs <bskeggs@redhat.com>
      Cc: Daniel Vetter <daniel@ffwll.ch>
      Cc: David Airlie <airlied@redhat.com>
      Cc: Jerry Zuo <Jerry.Zuo@amd.com>
      Cc: Harry Wentland <harry.wentland@amd.com>
      Cc: Juston Li <juston.li@intel.com>
      Link: https://patchwork.freedesktop.org/patch/msgid/20190111005343.17443-15-lyude@redhat.com
      81640f01
    • Lyude Paul's avatar
      drm/nouveau: Remove unnecessary VCPI checks in nv50_msto_cleanup() · 5e292e76
      Lyude Paul authored
      There is no need to look at the port's VCPI allocation before calling
      drm_dp_mst_deallocate_vcpi(), as we already have msto->disabled to let
      us avoid cleaning up an msto more then once. The DP MST core will never
      call drm_dp_mst_deallocate_vcpi() on it's own, which is presumably what
      these checks are meant to protect against.
      
      More importantly though, we're about to stop clearing mstc->port in the
      next commit, which means if we could potentially hit a use-after-free
      error if we tried to check mstc->port->vcpi here. So to make life easier
      for anyone who bisects this code in the future, use msto->disabled
      instead to check whether or not we need to deallocate VCPI instead.
      Signed-off-by: default avatarLyude Paul <lyude@redhat.com>
      Reviewed-by: default avatarBen Skeggs <bskeggs@redhat.com>
      Cc: Daniel Vetter <daniel@ffwll.ch>
      Cc: David Airlie <airlied@redhat.com>
      Cc: Jerry Zuo <Jerry.Zuo@amd.com>
      Cc: Harry Wentland <harry.wentland@amd.com>
      Cc: Juston Li <juston.li@intel.com>
      Link: https://patchwork.freedesktop.org/patch/msgid/20190111005343.17443-14-lyude@redhat.com
      5e292e76
    • Lyude Paul's avatar
      drm/nouveau: Remove bogus cleanup in nv50_mstm_add_connector() · 01324093
      Lyude Paul authored
      Trying to destroy the connector using mstc->connector.funcs->destroy()
      if connector initialization fails is wrong: there is no possible
      codepath in nv50_mstc_new where nv50_mstm_add_connector() would return
      <0 and mstc would be non-NULL.
      Signed-off-by: default avatarLyude Paul <lyude@redhat.com>
      Reviewed-by: default avatarBen Skeggs <bskeggs@redhat.com>
      Cc: Daniel Vetter <daniel@ffwll.ch>
      Cc: David Airlie <airlied@redhat.com>
      Cc: Jerry Zuo <Jerry.Zuo@amd.com>
      Cc: Harry Wentland <harry.wentland@amd.com>
      Cc: Juston Li <juston.li@intel.com>
      Link: https://patchwork.freedesktop.org/patch/msgid/20190111005343.17443-13-lyude@redhat.com
      01324093
    • Lyude Paul's avatar
      drm/amdgpu/display: Keep malloc ref to MST port · d2568976
      Lyude Paul authored
      Just like i915 and nouveau, it's a good idea for us to hold a malloc
      reference to the port here so that we never pass a freed pointer to any
      of the DP MST helper functions.
      
      Also, we stop unsetting aconnector->port in
      dm_dp_destroy_mst_connector(). There's literally no point to that
      assignment that I can see anyway.
      Signed-off-by: default avatarLyude Paul <lyude@redhat.com>
      Reviewed-by: default avatarHarry Wentland <harry.wentland@amd.com>
      Cc: Daniel Vetter <daniel@ffwll.ch>
      Cc: David Airlie <airlied@redhat.com>
      Cc: Jerry Zuo <Jerry.Zuo@amd.com>
      Cc: Juston Li <juston.li@intel.com>
      Link: https://patchwork.freedesktop.org/patch/msgid/20190111005343.17443-12-lyude@redhat.com
      d2568976
    • Lyude Paul's avatar
      drm/i915: Keep malloc references to MST ports · 79a47cd3
      Lyude Paul authored
      So that the ports stay around until we've destroyed the connectors, in
      order to ensure that we don't pass an invalid pointer to any MST helpers
      once we introduce the new MST VCPI helpers.
      
      Changes since v1:
      * Move drm_dp_mst_get_port_malloc() to where we assign
        intel_connector->port - danvet
      Signed-off-by: default avatarLyude Paul <lyude@redhat.com>
      Reviewed-by: default avatarDaniel Vetter <daniel@ffwll.ch>
      Cc: David Airlie <airlied@redhat.com>
      Cc: Jerry Zuo <Jerry.Zuo@amd.com>
      Cc: Harry Wentland <harry.wentland@amd.com>
      Cc: Juston Li <juston.li@intel.com>
      Link: https://patchwork.freedesktop.org/patch/msgid/20190111005343.17443-11-lyude@redhat.com
      79a47cd3
    • Lyude Paul's avatar
      drm/dp_mst: Fix payload deallocation on hotplugs using malloc refs · cfe9f903
      Lyude Paul authored
      Up until now, freeing payloads on remote MST hubs that just had ports
      removed has almost never worked because we've been relying on port
      validation in order to stop us from accessing ports that have already
      been freed from memory, but ports which need their payloads released due
      to being removed will never be a valid part of the topology after
      they've been removed.
      
      Since we've introduced malloc refs, we can replace all of the validation
      logic in payload helpers which are used for deallocation with some
      well-placed malloc krefs. This ensures that regardless of whether or not
      the ports are still valid and in the topology, any port which has an
      allocated payload will remain allocated in memory until it's payloads
      have been removed - finally allowing us to actually release said
      payloads correctly.
      Signed-off-by: default avatarLyude Paul <lyude@redhat.com>
      Reviewed-by: default avatarDaniel Vetter <daniel.vetter@ffwll.ch>
      Reviewed-by: default avatarHarry Wentland <harry.wentland@amd.com>
      Cc: David Airlie <airlied@redhat.com>
      Cc: Jerry Zuo <Jerry.Zuo@amd.com>
      Cc: Juston Li <juston.li@intel.com>
      Link: https://patchwork.freedesktop.org/patch/msgid/20190111005343.17443-10-lyude@redhat.com
      cfe9f903
    • Lyude Paul's avatar
      drm/dp_mst: Stop releasing VCPI when removing ports from topology · a68f9917
      Lyude Paul authored
      This has never actually worked, and isn't needed anyway: the driver's
      always going to try to deallocate VCPI when it tears down the display
      that the VCPI belongs to.
      Signed-off-by: default avatarLyude Paul <lyude@redhat.com>
      Reviewed-by: default avatarHarry Wentland <harry.wentland@amd.com>
      Reviewed-by: default avatarDaniel Vetter <daniel@ffwll.ch>
      Cc: David Airlie <airlied@redhat.com>
      Cc: Jerry Zuo <Jerry.Zuo@amd.com>
      Cc: Juston Li <juston.li@intel.com>
      Link: https://patchwork.freedesktop.org/patch/msgid/20190111005343.17443-9-lyude@redhat.com
      a68f9917
    • Lyude Paul's avatar
      drm/dp_mst: Restart last_connected_port_and_mstb() if topology ref fails · 56d1c14e
      Lyude Paul authored
      While this isn't a complete fix, this will improve the reliability of
      drm_dp_get_last_connected_port_and_mstb() pretty significantly during
      hotplug events, since there's a chance that the in-memory topology tree
      may not be fully updated when drm_dp_get_last_connected_port_and_mstb()
      is called and thus might end up causing our search to fail on an mstb
      whose topology refcount has reached 0, but has not yet been removed from
      it's parent.
      
      Ideally, we should further fix this problem by ensuring that we deal
      with the potential for racing with a hotplug event, which would look
      like this:
      
      * drm_dp_payload_send_msg() retrieves the last living relative of mstb
        with drm_dp_get_last_connected_port_and_mstb()
      * drm_dp_payload_send_msg() starts building payload message
        At the same time, mstb gets unplugged from the topology and is no
        longer the actual last living relative of the original mstb
      * drm_dp_payload_send_msg() tries sending the payload message, hub times
        out
      * Hub timed out, we give up and run away-resulting in the payload being
        leaked
      
      This could be fixed by restarting the
      drm_dp_get_last_connected_port_and_mstb() search whenever we get a
      timeout, sending the payload to the new mstb, then repeating until
      either the entire topology is removed from the system or
      drm_dp_get_last_connected_port_and_mstb() fails. But since the above
      race condition is not terribly likely, we'll address that in a later
      patch series once we've improved the recovery handling for VCPI
      allocations in the rest of the DP MST helpers.
      
      Changes since v1:
      * Convert kerneldoc for drm_dp_get_last_connected_port_and_mstb to
        normal comment - danvet
      Signed-off-by: default avatarLyude Paul <lyude@redhat.com>
      Reviewed-by: default avatarHarry Wentland <harry.wentland@amd.com>
      Reviewed-by: default avatarDaniel Vetter <daniel@ffwll.ch>
      Cc: David Airlie <airlied@redhat.com>
      Cc: Jerry Zuo <Jerry.Zuo@amd.com>
      Cc: Juston Li <juston.li@intel.com>
      Link: https://patchwork.freedesktop.org/patch/msgid/20190111005343.17443-8-lyude@redhat.com
      56d1c14e
    • Lyude Paul's avatar
      drm/dp_mst: Introduce new refcounting scheme for mstbs and ports · ebcc0e6b
      Lyude Paul authored
      The current way of handling refcounting in the DP MST helpers is really
      confusing and probably just plain wrong because it's been hacked up many
      times over the years without anyone actually going over the code and
      seeing if things could be simplified.
      
      To the best of my understanding, the current scheme works like this:
      drm_dp_mst_port and drm_dp_mst_branch both have a single refcount. When
      this refcount hits 0 for either of the two, they're removed from the
      topology state, but not immediately freed. Both ports and branch devices
      will reinitialize their kref once it's hit 0 before actually destroying
      themselves. The intended purpose behind this is so that we can avoid
      problems like not being able to free a remote payload that might still
      be active, due to us having removed all of the port/branch device
      structures in memory, as per:
      
      commit 91a25e46 ("drm/dp/mst: deallocate payload on port destruction")
      
      Which may have worked, but then it caused use-after-free errors. Being
      new to MST at the time, I tried fixing it;
      
      commit 263efde3 ("drm/dp/mst: Get validated port ref in drm_dp_update_payload_part1()")
      
      But, that was broken: both drm_dp_mst_port and drm_dp_mst_branch structs
      are validated in almost every DP MST helper function. Simply put, this
      means we go through the topology and try to see if the given
      drm_dp_mst_branch or drm_dp_mst_port is still attached to something
      before trying to use it in order to avoid dereferencing freed memory
      (something that has happened a LOT in the past with this library).
      Because of this it doesn't actually matter whether or not we keep keep
      the ports and branches around in memory as that's not enough, because
      any function that validates the branches and ports passed to it will
      still reject them anyway since they're no longer in the topology
      structure. So, use-after-free errors were fixed but payload deallocation
      was completely broken.
      
      Two years later, AMD informed me about this issue and I attempted to
      come up with a temporary fix, pending a long-overdue cleanup of this
      library:
      
      commit c54c7374 ("drm/dp_mst: Skip validating ports during destruction, just ref")
      
      But then that introduced use-after-free errors, so I quickly reverted
      it:
      
      commit 9765635b ("Revert "drm/dp_mst: Skip validating ports during destruction, just ref"")
      
      And in the process, learned that there is just no simple fix for this:
      the design is just broken. Unfortunately, the usage of these helpers are
      quite broken as well. Some drivers like i915 have been smart enough to
      avoid accessing any kind of information from MST port structures, but
      others like nouveau have assumed, understandably so, that
      drm_dp_mst_port structures are normal and can just be accessed at any
      time without worrying about use-after-free errors.
      
      After a lot of discussion, me and Daniel Vetter came up with a better
      idea to replace all of this.
      
      To summarize, since this is documented far more indepth in the
      documentation this patch introduces, we make it so that drm_dp_mst_port
      and drm_dp_mst_branch structures have two different classes of
      refcounts: topology_kref, and malloc_kref. topology_kref corresponds to
      the lifetime of the given drm_dp_mst_port or drm_dp_mst_branch in it's
      given topology. Once it hits zero, any associated connectors are removed
      and the branch or port can no longer be validated. malloc_kref
      corresponds to the lifetime of the memory allocation for the actual
      structure, and will always be non-zero so long as the topology_kref is
      non-zero. This gives us a way to allow callers to hold onto port and
      branch device structures past their topology lifetime, and dramatically
      simplifies the lifetimes of both structures. This also finally fixes the
      port deallocation problem, properly.
      
      Additionally: since this now means that we can keep ports and branch
      devices allocated in memory for however long we need, we no longer need
      a significant amount of the port validation that we currently do.
      
      Additionally, there is one last scenario that this fixes, which couldn't
      have been fixed properly beforehand:
      
      - CPU1 unrefs port from topology (refcount 1->0)
      - CPU2 refs port in topology(refcount 0->1)
      
      Since we now can guarantee memory safety for ports and branches
      as-needed, we also can make our main reference counting functions fix
      this problem by using kref_get_unless_zero() internally so that topology
      refcounts can only ever reach 0 once.
      
      Changes since v4:
      * Change the kernel-figure summary for dp-mst/topology-figure-1.dot a
        bit - danvet
      * Remove figure numbers - danvet
      
      Changes since v3:
      * Remove rebase detritus - danvet
      * Split out purely style changes into separate patches - hwentlan
      
      Changes since v2:
      * Fix commit message - checkpatch
      * s/)-1/) - 1/g - checkpatch
      
      Changes since v1:
      * Remove forward declarations - danvet
      * Move "Branch device and port refcounting" section from documentation
        into kernel-doc comments - danvet
      * Export internal topology lifetime functions into their own section in
        the kernel-docs - danvet
      * s/@/&/g for struct references in kernel-docs - danvet
      * Drop the "when they are no longer being used" bits from the kernel
        docs - danvet
      * Modify diagrams to show how the DRM driver interacts with the topology
        and payloads - danvet
      * Make suggested documentation changes for
        drm_dp_mst_topology_get_mstb() and drm_dp_mst_topology_get_port() -
        danvet
      * Better explain the relationship between malloc refs and topology krefs
        in the documentation for drm_dp_mst_topology_get_port() and
        drm_dp_mst_topology_get_mstb() - danvet
      * Fix "See also" in drm_dp_mst_topology_get_mstb() - danvet
      * Rename drm_dp_mst_topology_get_(port|mstb)() ->
        drm_dp_mst_topology_try_get_(port|mstb)() and
        drm_dp_mst_topology_ref_(port|mstb)() ->
        drm_dp_mst_topology_get_(port|mstb)() - danvet
      * s/should/must in docs - danvet
      * WARN_ON(refcount == 0) in topology_get_(mstb|port) - danvet
      * Move kdocs for mstb/port structs inline - danvet
      * Split drm_dp_get_last_connected_port_and_mstb() changes into their own
        commit - danvet
      Signed-off-by: default avatarLyude Paul <lyude@redhat.com>
      Reviewed-by: default avatarHarry Wentland <harry.wentland@amd.com>
      Reviewed-by: default avatarDaniel Vetter <daniel@ffwll.ch>
      Cc: David Airlie <airlied@redhat.com>
      Cc: Jerry Zuo <Jerry.Zuo@amd.com>
      Cc: Juston Li <juston.li@intel.com>
      Link: https://patchwork.freedesktop.org/patch/msgid/20190111005343.17443-7-lyude@redhat.com
      ebcc0e6b
    • Lyude Paul's avatar
      drm/dp_mst: Rename drm_dp_mst_get_validated_(port|mstb)_ref and friends · d0757afd
      Lyude Paul authored
      s/drm_dp_get_validated_port_ref/drm_dp_mst_topology_get_port_validated/
      s/drm_dp_put_port/drm_dp_mst_topology_put_port/
      s/drm_dp_get_validated_mstb_ref/drm_dp_mst_topology_get_mstb_validated/
      s/drm_dp_put_mst_branch_device/drm_dp_mst_topology_put_mstb/
      
      This is a much more consistent naming scheme, and will make even more
      sense once we redesign how the current refcounting scheme here works.
      Signed-off-by: default avatarLyude Paul <lyude@redhat.com>
      Reviewed-by: default avatarDaniel Vetter <daniel@ffwll.ch>
      Reviewed-by: default avatarHarry Wentland <harry.wentland@amd.com>
      Cc: David Airlie <airlied@redhat.com>
      Cc: Jerry Zuo <Jerry.Zuo@amd.com>
      Cc: Juston Li <juston.li@intel.com>
      Link: https://patchwork.freedesktop.org/patch/msgid/20190111005343.17443-6-lyude@redhat.com
      d0757afd
    • Lyude Paul's avatar
      drm/dp_mst: Fix some formatting in drm_dp_mst_deallocate_vcpi() · 4afb8a26
      Lyude Paul authored
      Split some stuff across multiple lines
      Signed-off-by: default avatarLyude Paul <lyude@redhat.com>
      Reviewed-by: default avatarHarry Wentland <harry.wentland@amd.com>
      Reviewed-by: default avatarDaniel Vetter <daniel@ffwll.ch>
      Cc: David Airlie <airlied@redhat.com>
      Cc: Jerry Zuo <Jerry.Zuo@amd.com>
      Cc: Juston Li <juston.li@intel.com>
      Link: https://patchwork.freedesktop.org/patch/msgid/20190111005343.17443-5-lyude@redhat.com
      4afb8a26
    • Lyude Paul's avatar
      drm/dp_mst: Fix some formatting in drm_dp_mst_allocate_vcpi() · e0ac7113
      Lyude Paul authored
      Fix some indenting, split some stuff across multiple lines.
      Signed-off-by: default avatarLyude Paul <lyude@redhat.com>
      Reviewed-by: default avatarHarry Wentland <harry.wentland@amd.com>
      Reviewed-by: default avatarDaniel Vetter <daniel@ffwll.ch>
      Cc: David Airlie <airlied@redhat.com>
      Cc: Jerry Zuo <Jerry.Zuo@amd.com>
      Cc: Juston Li <juston.li@intel.com>
      Link: https://patchwork.freedesktop.org/patch/msgid/20190111005343.17443-4-lyude@redhat.com
      e0ac7113
    • Lyude Paul's avatar
      drm/dp_mst: Fix some formatting in drm_dp_payload_send_msg() · de6d6818
      Lyude Paul authored
      Split some stuff across multiple lines, remove some unnecessary braces
      Signed-off-by: default avatarLyude Paul <lyude@redhat.com>
      Reviewed-by: default avatarHarry Wentland <harry.wentland@amd.com>
      Reviewed-by: default avatarDaniel Vetter <daniel@ffwll.ch>
      Cc: David Airlie <airlied@redhat.com>
      Cc: Jerry Zuo <Jerry.Zuo@amd.com>
      Cc: Juston Li <juston.li@intel.com>
      Link: https://patchwork.freedesktop.org/patch/msgid/20190111005343.17443-3-lyude@redhat.com
      de6d6818
    • Lyude Paul's avatar
      drm/dp_mst: Fix some formatting in drm_dp_add_port() · 3d76df63
      Lyude Paul authored
      Reindent some stuff, and split some stuff across multiple lines so we
      aren't going over the text width limit.
      Signed-off-by: default avatarLyude Paul <lyude@redhat.com>
      Reviewed-by: default avatarHarry Wentland <harry.wentland@amd.com>
      Reviewed-by: default avatarDaniel Vetter <daniel@ffwll.ch>
      Cc: David Airlie <airlied@redhat.com>
      Cc: Jerry Zuo <Jerry.Zuo@amd.com>
      Cc: Juston Li <juston.li@intel.com>
      Link: https://patchwork.freedesktop.org/patch/msgid/20190111005343.17443-2-lyude@redhat.com
      3d76df63
  2. 10 Jan, 2019 12 commits
  3. 09 Jan, 2019 14 commits