Currently, MST lacks locking in a lot of places that really should have
some sort of locking. Hotplugging and link address code paths are some
of the offenders here, as there is actually nothing preventing us from
running a link address probe while at the same time handling a
connection status update request - something that's likely always been
possible but never seen in the wild because hotplugging has been broken
for ages now (with the exception of amdgpu, for reasons I don't think
are worth digging into very far).
Note: I'm going to start using the term "in-memory topology layout" here
to refer to drm_dp_mst_port->mstb and drm_dp_mst_branch->ports.
Locking in these places is a little tougher then it looks though.
Generally we protect anything having to do with the in-memory topology
layout under &mgr->lock. But this becomes nearly impossible to do from
the context of link address probes due to the fact that &mgr->lock is
usually grabbed under random various modesetting locks, meaning that
there's no way we can just invert the &mgr->lock order and keep it
locked throughout the whole process of updating the topology.
Luckily there are only two workers which can modify the in-memory
topology layout: drm_dp_mst_up_req_work() and
drm_dp_mst_link_probe_work(), meaning as long as we prevent these two
workers from traveling the topology layout in parallel with the intent
of updating it we don't need to worry about grabbing &mgr->lock in these
workers for reads. We only need to grab &mgr->lock in these workers for
writes, so that readers outside these two workers are still protected
from the topology layout changing beneath them.
So, add the new &mgr->probe_lock and use it in both
drm_dp_mst_link_probe_work() and drm_dp_mst_up_req_work(). Additionally,
add some more detailed explanations for how this locking is intended to
work to drm_dp_mst_port->mstb and drm_dp_mst_branch->ports.
Signed-off-by: Lyude Paul <[email protected]>
Cc: Juston Li <[email protected]>
Cc: Imre Deak <[email protected]>
Cc: Ville Syrjälä <[email protected]>
Cc: Harry Wentland <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: Sean Paul <[email protected]>
---
drivers/gpu/drm/drm_dp_mst_topology.c | 28 ++++++++++++++---------
include/drm/drm_dp_mst_helper.h | 32 +++++++++++++++++++++++----
2 files changed, 46 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 08c316a727df..11d842f0bff5 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -2147,37 +2147,40 @@ static void drm_dp_check_and_send_link_address(struct drm_dp_mst_topology_mgr *m
struct drm_dp_mst_branch *mstb)
{
struct drm_dp_mst_port *port;
- struct drm_dp_mst_branch *mstb_child;
+
if (!mstb->link_address_sent)
drm_dp_send_link_address(mgr, mstb);
list_for_each_entry(port, &mstb->ports, next) {
- if (port->input)
- continue;
+ struct drm_dp_mst_branch *mstb_child = NULL;
- if (!port->ddps)
+ if (port->input || !port->ddps)
continue;
if (!port->available_pbn)
drm_dp_send_enum_path_resources(mgr, mstb, port);
- if (port->mstb) {
+ if (port->mstb)
mstb_child = drm_dp_mst_topology_get_mstb_validated(
mgr, port->mstb);
- if (mstb_child) {
- drm_dp_check_and_send_link_address(mgr, mstb_child);
- drm_dp_mst_topology_put_mstb(mstb_child);
- }
+
+ if (mstb_child) {
+ drm_dp_check_and_send_link_address(mgr, mstb_child);
+ drm_dp_mst_topology_put_mstb(mstb_child);
}
}
}
static void drm_dp_mst_link_probe_work(struct work_struct *work)
{
- struct drm_dp_mst_topology_mgr *mgr = container_of(work, struct drm_dp_mst_topology_mgr, work);
+ struct drm_dp_mst_topology_mgr *mgr =
+ container_of(work, struct drm_dp_mst_topology_mgr, work);
+ struct drm_device *dev = mgr->dev;
struct drm_dp_mst_branch *mstb;
int ret;
+ mutex_lock(&mgr->probe_lock);
+
mutex_lock(&mgr->lock);
mstb = mgr->mst_primary;
if (mstb) {
@@ -2190,6 +2193,7 @@ static void drm_dp_mst_link_probe_work(struct work_struct *work)
drm_dp_check_and_send_link_address(mgr, mstb);
drm_dp_mst_topology_put_mstb(mstb);
}
+ mutex_unlock(&mgr->probe_lock);
}
static bool drm_dp_validate_guid(struct drm_dp_mst_topology_mgr *mgr,
@@ -3313,6 +3317,7 @@ static void drm_dp_mst_up_req_work(struct work_struct *work)
up_req_work);
struct drm_dp_pending_up_req *up_req;
+ mutex_lock(&mgr->probe_lock);
while (true) {
mutex_lock(&mgr->up_req_lock);
up_req = list_first_entry_or_null(&mgr->up_req_list,
@@ -3328,6 +3333,7 @@ static void drm_dp_mst_up_req_work(struct work_struct *work)
drm_dp_mst_process_up_req(mgr, up_req);
kfree(up_req);
}
+ mutex_unlock(&mgr->probe_lock);
}
static int drm_dp_mst_handle_up_req(struct drm_dp_mst_topology_mgr *mgr)
@@ -4349,6 +4355,7 @@ int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr,
mutex_init(&mgr->payload_lock);
mutex_init(&mgr->delayed_destroy_lock);
mutex_init(&mgr->up_req_lock);
+ mutex_init(&mgr->probe_lock);
INIT_LIST_HEAD(&mgr->tx_msg_downq);
INIT_LIST_HEAD(&mgr->destroy_port_list);
INIT_LIST_HEAD(&mgr->destroy_branch_device_list);
@@ -4414,6 +4421,7 @@ void drm_dp_mst_topology_mgr_destroy(struct drm_dp_mst_topology_mgr *mgr)
mutex_destroy(&mgr->qlock);
mutex_destroy(&mgr->lock);
mutex_destroy(&mgr->up_req_lock);
+ mutex_destroy(&mgr->probe_lock);
}
EXPORT_SYMBOL(drm_dp_mst_topology_mgr_destroy);
diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
index 7d80c38ee00e..bccb5514e0ef 100644
--- a/include/drm/drm_dp_mst_helper.h
+++ b/include/drm/drm_dp_mst_helper.h
@@ -55,8 +55,6 @@ struct drm_dp_vcpi {
* @num_sdp_stream_sinks: Number of stream sinks
* @available_pbn: Available bandwidth for this port.
* @next: link to next port on this branch device
- * @mstb: branch device on this port, protected by
- * &drm_dp_mst_topology_mgr.lock
* @aux: i2c aux transport to talk to device connected to this port, protected
* by &drm_dp_mst_topology_mgr.lock
* @parent: branch device parent of this port
@@ -92,7 +90,17 @@ struct drm_dp_mst_port {
u8 num_sdp_stream_sinks;
uint16_t available_pbn;
struct list_head next;
- struct drm_dp_mst_branch *mstb; /* pointer to an mstb if this port has one */
+ /**
+ * @mstb: the branch device connected to this port, if there is one.
+ * This should be considered protected for reading by
+ * &drm_dp_mst_topology_mgr.lock. There are two exceptions to this:
+ * &drm_dp_mst_topology_mgr.up_req_work and
+ * &drm_dp_mst_topology_mgr.work, which do not grab
+ * &drm_dp_mst_topology_mgr.lock during reads but are the only
+ * updaters of this list and are protected from writing concurrently
+ * by &drm_dp_mst_topology_mgr.probe_lock.
+ */
+ struct drm_dp_mst_branch *mstb;
struct drm_dp_aux aux; /* i2c bus for this port? */
struct drm_dp_mst_branch *parent;
@@ -118,7 +126,6 @@ struct drm_dp_mst_port {
* @lct: Link count total to talk to this branch device.
* @num_ports: number of ports on the branch.
* @msg_slots: one bit per transmitted msg slot.
- * @ports: linked list of ports on this branch.
* @port_parent: pointer to the port parent, NULL if toplevel.
* @mgr: topology manager for this branch device.
* @tx_slots: transmission slots for this device.
@@ -156,6 +163,16 @@ struct drm_dp_mst_branch {
int num_ports;
int msg_slots;
+ /**
+ * @ports: the list of ports on this branch device. This should be
+ * considered protected for reading by &drm_dp_mst_topology_mgr.lock.
+ * There are two exceptions to this:
+ * &drm_dp_mst_topology_mgr.up_req_work and
+ * &drm_dp_mst_topology_mgr.work, which do not grab
+ * &drm_dp_mst_topology_mgr.lock during reads but are the only
+ * updaters of this list and are protected from updating the list
+ * concurrently by @drm_dp_mst_topology_mgr.probe_lock
+ */
struct list_head ports;
/* list of tx ops queue for this port */
@@ -502,6 +519,13 @@ struct drm_dp_mst_topology_mgr {
*/
struct mutex lock;
+ /**
+ * @probe_lock: Prevents @work and @up_req_work, the only writers of
+ * &drm_dp_mst_port.mstb and &drm_dp_mst_branch.ports, from racing
+ * while they update the topology.
+ */
+ struct mutex probe_lock;
+
/**
* @mst_state: If this manager is enabled for an MST capable port. False
* if no MST sink/branch devices is connected.
--
2.21.0
On Mon, Oct 21, 2019 at 10:36:00PM -0400, Lyude Paul wrote:
> Currently, MST lacks locking in a lot of places that really should have
> some sort of locking. Hotplugging and link address code paths are some
> of the offenders here, as there is actually nothing preventing us from
> running a link address probe while at the same time handling a
> connection status update request - something that's likely always been
> possible but never seen in the wild because hotplugging has been broken
> for ages now (with the exception of amdgpu, for reasons I don't think
> are worth digging into very far).
>
> Note: I'm going to start using the term "in-memory topology layout" here
> to refer to drm_dp_mst_port->mstb and drm_dp_mst_branch->ports.
>
> Locking in these places is a little tougher then it looks though.
> Generally we protect anything having to do with the in-memory topology
> layout under &mgr->lock. But this becomes nearly impossible to do from
> the context of link address probes due to the fact that &mgr->lock is
> usually grabbed under random various modesetting locks, meaning that
> there's no way we can just invert the &mgr->lock order and keep it
> locked throughout the whole process of updating the topology.
>
> Luckily there are only two workers which can modify the in-memory
> topology layout: drm_dp_mst_up_req_work() and
> drm_dp_mst_link_probe_work(), meaning as long as we prevent these two
> workers from traveling the topology layout in parallel with the intent
> of updating it we don't need to worry about grabbing &mgr->lock in these
> workers for reads. We only need to grab &mgr->lock in these workers for
> writes, so that readers outside these two workers are still protected
> from the topology layout changing beneath them.
>
> So, add the new &mgr->probe_lock and use it in both
> drm_dp_mst_link_probe_work() and drm_dp_mst_up_req_work(). Additionally,
> add some more detailed explanations for how this locking is intended to
> work to drm_dp_mst_port->mstb and drm_dp_mst_branch->ports.
This seems like a good solution to me, thanks for working this through!
Any chance we could add a WARN_ON(!mutex_is_locked(&mgr->probe_lock)); somewhere
centrally called by all paths modifying the in-memory topology layout?
drm_dp_add_port perhaps? That way we have a fallback in case something else
starts mucking with the topology.
Other than that,
Reviewed-by: Sean Paul <[email protected]>
>
> Signed-off-by: Lyude Paul <[email protected]>
> Cc: Juston Li <[email protected]>
> Cc: Imre Deak <[email protected]>
> Cc: Ville Syrj?l? <[email protected]>
> Cc: Harry Wentland <[email protected]>
> Cc: Daniel Vetter <[email protected]>
> Cc: Sean Paul <[email protected]>
> ---
> drivers/gpu/drm/drm_dp_mst_topology.c | 28 ++++++++++++++---------
> include/drm/drm_dp_mst_helper.h | 32 +++++++++++++++++++++++----
> 2 files changed, 46 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 08c316a727df..11d842f0bff5 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -2147,37 +2147,40 @@ static void drm_dp_check_and_send_link_address(struct drm_dp_mst_topology_mgr *m
> struct drm_dp_mst_branch *mstb)
> {
> struct drm_dp_mst_port *port;
> - struct drm_dp_mst_branch *mstb_child;
> +
> if (!mstb->link_address_sent)
> drm_dp_send_link_address(mgr, mstb);
>
> list_for_each_entry(port, &mstb->ports, next) {
> - if (port->input)
> - continue;
> + struct drm_dp_mst_branch *mstb_child = NULL;
>
> - if (!port->ddps)
> + if (port->input || !port->ddps)
> continue;
>
> if (!port->available_pbn)
> drm_dp_send_enum_path_resources(mgr, mstb, port);
>
> - if (port->mstb) {
> + if (port->mstb)
> mstb_child = drm_dp_mst_topology_get_mstb_validated(
> mgr, port->mstb);
> - if (mstb_child) {
> - drm_dp_check_and_send_link_address(mgr, mstb_child);
> - drm_dp_mst_topology_put_mstb(mstb_child);
> - }
> +
> + if (mstb_child) {
> + drm_dp_check_and_send_link_address(mgr, mstb_child);
> + drm_dp_mst_topology_put_mstb(mstb_child);
> }
> }
> }
>
> static void drm_dp_mst_link_probe_work(struct work_struct *work)
> {
> - struct drm_dp_mst_topology_mgr *mgr = container_of(work, struct drm_dp_mst_topology_mgr, work);
> + struct drm_dp_mst_topology_mgr *mgr =
> + container_of(work, struct drm_dp_mst_topology_mgr, work);
> + struct drm_device *dev = mgr->dev;
> struct drm_dp_mst_branch *mstb;
> int ret;
>
> + mutex_lock(&mgr->probe_lock);
> +
> mutex_lock(&mgr->lock);
> mstb = mgr->mst_primary;
> if (mstb) {
> @@ -2190,6 +2193,7 @@ static void drm_dp_mst_link_probe_work(struct work_struct *work)
> drm_dp_check_and_send_link_address(mgr, mstb);
> drm_dp_mst_topology_put_mstb(mstb);
> }
> + mutex_unlock(&mgr->probe_lock);
> }
>
> static bool drm_dp_validate_guid(struct drm_dp_mst_topology_mgr *mgr,
> @@ -3313,6 +3317,7 @@ static void drm_dp_mst_up_req_work(struct work_struct *work)
> up_req_work);
> struct drm_dp_pending_up_req *up_req;
>
> + mutex_lock(&mgr->probe_lock);
> while (true) {
> mutex_lock(&mgr->up_req_lock);
> up_req = list_first_entry_or_null(&mgr->up_req_list,
> @@ -3328,6 +3333,7 @@ static void drm_dp_mst_up_req_work(struct work_struct *work)
> drm_dp_mst_process_up_req(mgr, up_req);
> kfree(up_req);
> }
> + mutex_unlock(&mgr->probe_lock);
> }
>
> static int drm_dp_mst_handle_up_req(struct drm_dp_mst_topology_mgr *mgr)
> @@ -4349,6 +4355,7 @@ int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr,
> mutex_init(&mgr->payload_lock);
> mutex_init(&mgr->delayed_destroy_lock);
> mutex_init(&mgr->up_req_lock);
> + mutex_init(&mgr->probe_lock);
> INIT_LIST_HEAD(&mgr->tx_msg_downq);
> INIT_LIST_HEAD(&mgr->destroy_port_list);
> INIT_LIST_HEAD(&mgr->destroy_branch_device_list);
> @@ -4414,6 +4421,7 @@ void drm_dp_mst_topology_mgr_destroy(struct drm_dp_mst_topology_mgr *mgr)
> mutex_destroy(&mgr->qlock);
> mutex_destroy(&mgr->lock);
> mutex_destroy(&mgr->up_req_lock);
> + mutex_destroy(&mgr->probe_lock);
> }
> EXPORT_SYMBOL(drm_dp_mst_topology_mgr_destroy);
>
> diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
> index 7d80c38ee00e..bccb5514e0ef 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -55,8 +55,6 @@ struct drm_dp_vcpi {
> * @num_sdp_stream_sinks: Number of stream sinks
> * @available_pbn: Available bandwidth for this port.
> * @next: link to next port on this branch device
> - * @mstb: branch device on this port, protected by
> - * &drm_dp_mst_topology_mgr.lock
> * @aux: i2c aux transport to talk to device connected to this port, protected
> * by &drm_dp_mst_topology_mgr.lock
> * @parent: branch device parent of this port
> @@ -92,7 +90,17 @@ struct drm_dp_mst_port {
> u8 num_sdp_stream_sinks;
> uint16_t available_pbn;
> struct list_head next;
> - struct drm_dp_mst_branch *mstb; /* pointer to an mstb if this port has one */
> + /**
> + * @mstb: the branch device connected to this port, if there is one.
> + * This should be considered protected for reading by
> + * &drm_dp_mst_topology_mgr.lock. There are two exceptions to this:
> + * &drm_dp_mst_topology_mgr.up_req_work and
> + * &drm_dp_mst_topology_mgr.work, which do not grab
> + * &drm_dp_mst_topology_mgr.lock during reads but are the only
> + * updaters of this list and are protected from writing concurrently
> + * by &drm_dp_mst_topology_mgr.probe_lock.
> + */
> + struct drm_dp_mst_branch *mstb;
> struct drm_dp_aux aux; /* i2c bus for this port? */
> struct drm_dp_mst_branch *parent;
>
> @@ -118,7 +126,6 @@ struct drm_dp_mst_port {
> * @lct: Link count total to talk to this branch device.
> * @num_ports: number of ports on the branch.
> * @msg_slots: one bit per transmitted msg slot.
> - * @ports: linked list of ports on this branch.
> * @port_parent: pointer to the port parent, NULL if toplevel.
> * @mgr: topology manager for this branch device.
> * @tx_slots: transmission slots for this device.
> @@ -156,6 +163,16 @@ struct drm_dp_mst_branch {
> int num_ports;
>
> int msg_slots;
> + /**
> + * @ports: the list of ports on this branch device. This should be
> + * considered protected for reading by &drm_dp_mst_topology_mgr.lock.
> + * There are two exceptions to this:
> + * &drm_dp_mst_topology_mgr.up_req_work and
> + * &drm_dp_mst_topology_mgr.work, which do not grab
> + * &drm_dp_mst_topology_mgr.lock during reads but are the only
> + * updaters of this list and are protected from updating the list
> + * concurrently by @drm_dp_mst_topology_mgr.probe_lock
> + */
> struct list_head ports;
>
> /* list of tx ops queue for this port */
> @@ -502,6 +519,13 @@ struct drm_dp_mst_topology_mgr {
> */
> struct mutex lock;
>
> + /**
> + * @probe_lock: Prevents @work and @up_req_work, the only writers of
> + * &drm_dp_mst_port.mstb and &drm_dp_mst_branch.ports, from racing
> + * while they update the topology.
> + */
> + struct mutex probe_lock;
> +
> /**
> * @mst_state: If this manager is enabled for an MST capable port. False
> * if no MST sink/branch devices is connected.
> --
> 2.21.0
>
--
Sean Paul, Software Engineer, Google / Chromium OS