Re: [Nouveau] [PATCH v5 05/14] drm/dp_mst: Add probe_lock
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 >lock. But this becomes nearly impossible to do from
> the context of link address probes due to the fact that >lock is
> usually grabbed under random various modesetting locks, meaning that
> there's no way we can just invert the >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 >lock in these
> workers for reads. We only need to grab >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 >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(>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
>
> Signed-off-by: Lyude Paul
> Cc: Juston Li
> Cc: Imre Deak
> Cc: Ville Syrjälä
> Cc: Harry Wentland
> Cc: Daniel Vetter
> Cc: Sean Paul
> ---
> 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, >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(>probe_lock);
> +
> mutex_lock(>lock);
> mstb = mgr->mst_primary;
> if (mstb) {
> @@ -2190,6 +2193,7 @@ static void drm_dp_mst_link_probe_work(struct
>
[Nouveau] [PATCH v5 05/14] drm/dp_mst: Add probe_lock
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 >lock. But this becomes nearly impossible to do from
the context of link address probes due to the fact that >lock is
usually grabbed under random various modesetting locks, meaning that
there's no way we can just invert the >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 >lock in these
workers for reads. We only need to grab >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 >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
Cc: Juston Li
Cc: Imre Deak
Cc: Ville Syrjälä
Cc: Harry Wentland
Cc: Daniel Vetter
Cc: Sean Paul
---
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, >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(>probe_lock);
+
mutex_lock(>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(>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(>probe_lock);
while (true) {
mutex_lock(>up_req_lock);
up_req =
