From: Sean Paul <sean@poorly.run>
To: Lyude Paul <lyude@redhat.com>
Cc: dri-devel@lists.freedesktop.org, nouveau@lists.freedesktop.org,
amd-gfx@lists.freedesktop.org, "Juston Li" <juston.li@intel.com>,
"Imre Deak" <imre.deak@intel.com>,
"Ville Syrjälä" <ville.syrjala@linux.intel.com>,
"Harry Wentland" <hwentlan@amd.com>,
"Daniel Vetter" <daniel.vetter@ffwll.ch>,
"Maarten Lankhorst" <maarten.lankhorst@linux.intel.com>,
"Maxime Ripard" <mripard@kernel.org>,
"Sean Paul" <sean@poorly.run>, "David Airlie" <airlied@linux.ie>,
"Daniel Vetter" <daniel@ffwll.ch>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 20/27] drm/dp_mst: Protect drm_dp_mst_port members with connection_mutex
Date: Wed, 25 Sep 2019 16:00:20 -0400 [thread overview]
Message-ID: <20190925200020.GL218215@art_vandelay> (raw)
In-Reply-To: <20190903204645.25487-21-lyude@redhat.com>
On Tue, Sep 03, 2019 at 04:45:58PM -0400, Lyude Paul wrote:
> Yes-you read that right. Currently there is literally no locking in
> place for any of the drm_dp_mst_port struct members that can be modified
> in response to a link address response, or a connection status response.
> Which literally means if we're unlucky enough to have any sort of
> hotplugging event happen before we're finished with reprobing link
> addresses, we'll race and the contents of said struct members becomes
> undefined. Fun!
>
> So, finally add some simple locking protections to our MST helpers by
> protecting any drm_dp_mst_port members which can be changed by link
> address responses or connection status notifications under
> drm_device->mode_config.connection_mutex.
>
> Cc: Juston Li <juston.li@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Harry Wentland <hwentlan@amd.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> ---
> drivers/gpu/drm/drm_dp_mst_topology.c | 144 +++++++++++++++++++-------
> include/drm/drm_dp_mst_helper.h | 39 +++++--
> 2 files changed, 133 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 5101eeab4485..259634c5d6dc 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -1354,6 +1354,7 @@ static void drm_dp_free_mst_port(struct kref *kref)
> container_of(kref, struct drm_dp_mst_port, malloc_kref);
>
> drm_dp_mst_put_mstb_malloc(port->parent);
> + mutex_destroy(&port->lock);
> kfree(port);
> }
>
> @@ -1906,6 +1907,36 @@ void drm_dp_mst_connector_early_unregister(struct drm_connector *connector,
> }
> EXPORT_SYMBOL(drm_dp_mst_connector_early_unregister);
>
> +static void
> +drm_dp_mst_port_add_connector(struct drm_dp_mst_branch *mstb,
> + struct drm_dp_mst_port *port)
> +{
> + struct drm_dp_mst_topology_mgr *mgr = port->mgr;
> + char proppath[255];
> + int ret;
> +
> + build_mst_prop_path(mstb, port->port_num, proppath, sizeof(proppath));
> + port->connector = mgr->cbs->add_connector(mgr, port, proppath);
> + if (!port->connector) {
> + ret = -ENOMEM;
> + goto error;
> + }
> +
> + if ((port->pdt == DP_PEER_DEVICE_DP_LEGACY_CONV ||
> + port->pdt == DP_PEER_DEVICE_SST_SINK) &&
> + port->port_num >= DP_MST_LOGICAL_PORT_0) {
> + port->cached_edid = drm_get_edid(port->connector,
> + &port->aux.ddc);
> + drm_connector_set_tile_property(port->connector);
> + }
> +
> + mgr->cbs->register_connector(port->connector);
> + return;
> +
> +error:
> + DRM_ERROR("Failed to create connector for port %p: %d\n", port, ret);
> +}
> +
> static void
> drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb,
> struct drm_device *dev,
> @@ -1913,8 +1944,12 @@ drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb,
> {
> struct drm_dp_mst_topology_mgr *mgr = mstb->mgr;
> struct drm_dp_mst_port *port;
> - bool created = false;
> - int old_ddps = 0;
> + struct drm_dp_mst_branch *child_mstb = NULL;
> + struct drm_connector *connector_to_destroy = NULL;
> + int old_ddps = 0, ret;
> + u8 new_pdt = DP_PEER_DEVICE_NONE;
> + bool created = false, send_link_addr = false,
> + create_connector = false;
>
> port = drm_dp_get_port(mstb, port_msg->port_number);
> if (!port) {
> @@ -1923,6 +1958,7 @@ drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb,
> return;
> kref_init(&port->topology_kref);
> kref_init(&port->malloc_kref);
> + mutex_init(&port->lock);
> port->parent = mstb;
> port->port_num = port_msg->port_number;
> port->mgr = mgr;
> @@ -1937,11 +1973,17 @@ drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb,
> drm_dp_mst_get_mstb_malloc(mstb);
>
> created = true;
> - } else {
> - old_ddps = port->ddps;
> }
>
> + mutex_lock(&port->lock);
> + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> +
> + if (!created)
> + old_ddps = port->ddps;
> +
> port->input = port_msg->input_port;
> + if (!port->input)
> + new_pdt = port_msg->peer_device_type;
> port->mcs = port_msg->mcs;
> port->ddps = port_msg->ddps;
> port->ldps = port_msg->legacy_device_plug_status;
> @@ -1969,44 +2011,58 @@ drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb,
> }
> }
>
> - if (!port->input) {
> - int ret = drm_dp_port_set_pdt(port,
> - port_msg->peer_device_type);
> - if (ret == 1) {
> - drm_dp_send_link_address(mgr, port->mstb);
> - } else if (ret < 0) {
> - DRM_ERROR("Failed to change PDT on port %p: %d\n",
> - port, ret);
> - goto fail;
> + ret = drm_dp_port_set_pdt(port, new_pdt);
> + if (ret == 1) {
> + send_link_addr = true;
> + } else if (ret < 0) {
> + DRM_ERROR("Failed to change PDT on port %p: %d\n",
> + port, ret);
> + goto fail_unlock;
> + }
> +
> + if (send_link_addr) {
> + mutex_lock(&mgr->lock);
> + if (port->mstb) {
> + child_mstb = port->mstb;
> + drm_dp_mst_get_mstb_malloc(child_mstb);
> }
> + mutex_unlock(&mgr->lock);
> }
>
> - if (created && !port->input) {
> - char proppath[255];
> + /*
> + * We unset port->connector before dropping connection_mutex so that
> + * there's no chance any of the atomic MST helpers can accidentally
> + * associate a to-be-destroyed connector with a port.
> + */
> + if (port->connector && port->input) {
> + connector_to_destroy = port->connector;
> + port->connector = NULL;
> + } else if (!port->connector && !port->input) {
> + create_connector = true;
> + }
>
> - build_mst_prop_path(mstb, port->port_num, proppath,
> - sizeof(proppath));
> - port->connector = (*mgr->cbs->add_connector)(mgr, port,
> - proppath);
> - if (!port->connector)
> - goto fail;
> + drm_modeset_unlock(&dev->mode_config.connection_mutex);
Do you drop this early b/c it deadlocks with something upstack? If so, I
wonder if you could plumb an acquire context through the appropriate
functions to avoid needing the port->lock at all?
Sean
>
> - if ((port->pdt == DP_PEER_DEVICE_DP_LEGACY_CONV ||
> - port->pdt == DP_PEER_DEVICE_SST_SINK) &&
> - port->port_num >= DP_MST_LOGICAL_PORT_0) {
> - port->cached_edid = drm_get_edid(port->connector,
> - &port->aux.ddc);
> - drm_connector_set_tile_property(port->connector);
> - }
> + if (connector_to_destroy)
> + mgr->cbs->destroy_connector(mgr, connector_to_destroy);
> + else if (create_connector)
> + drm_dp_mst_port_add_connector(mstb, port);
> +
> + mutex_unlock(&port->lock);
>
> - (*mgr->cbs->register_connector)(port->connector);
> + if (send_link_addr && child_mstb) {
> + drm_dp_send_link_address(mgr, child_mstb);
> + drm_dp_mst_put_mstb_malloc(child_mstb);
> }
>
> /* put reference to this port */
> drm_dp_mst_topology_put_port(port);
> return;
>
> -fail:
> +fail_unlock:
> + drm_modeset_unlock(&dev->mode_config.connection_mutex);
> + mutex_unlock(&port->lock);
> +
> /* Remove it from the port list */
> mutex_lock(&mgr->lock);
> list_del(&port->next);
> @@ -2022,6 +2078,7 @@ static void
> drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb,
> struct drm_dp_connection_status_notify *conn_stat)
> {
> + struct drm_device *dev = mstb->mgr->dev;
> struct drm_dp_mst_port *port;
> int old_ddps;
> bool dowork = false;
> @@ -2030,6 +2087,8 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb,
> if (!port)
> return;
>
> + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> +
> old_ddps = port->ddps;
> port->mcs = conn_stat->message_capability_status;
> port->ldps = conn_stat->legacy_device_plug_status;
> @@ -2055,6 +2114,7 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb,
> }
> }
>
> + drm_modeset_unlock(&dev->mode_config.connection_mutex);
> drm_dp_mst_topology_put_port(port);
> if (dowork)
> queue_work(system_long_wq, &mstb->mgr->work);
> @@ -2147,28 +2207,34 @@ drm_dp_get_mst_branch_device_by_guid(struct drm_dp_mst_topology_mgr *mgr,
> static void drm_dp_check_and_send_link_address(struct drm_dp_mst_topology_mgr *mgr,
> struct drm_dp_mst_branch *mstb)
> {
> + struct drm_device *dev = mgr->dev;
> 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;
> +
> + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>
> - if (!port->ddps)
> + if (port->input || !port->ddps) {
> + drm_modeset_unlock(&dev->mode_config.connection_mutex);
> 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);
> - }
> +
> + drm_modeset_unlock(&dev->mode_config.connection_mutex);
> +
> + if (mstb_child) {
> + drm_dp_check_and_send_link_address(mgr, mstb_child);
> + drm_dp_mst_topology_put_mstb(mstb_child);
> }
> }
> }
> diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
> index 7d80c38ee00e..1efbb086f7ac 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -45,23 +45,34 @@ struct drm_dp_vcpi {
> /**
> * struct drm_dp_mst_port - MST port
> * @port_num: port number
> - * @input: if this port is an input port.
> - * @mcs: message capability status - DP 1.2 spec.
> - * @ddps: DisplayPort Device Plug Status - DP 1.2
> - * @pdt: Peer Device Type
> - * @ldps: Legacy Device Plug Status
> - * @dpcd_rev: DPCD revision of device on this port
> - * @num_sdp_streams: Number of simultaneous streams
> - * @num_sdp_stream_sinks: Number of stream sinks
> - * @available_pbn: Available bandwidth for this port.
> + * @input: if this port is an input port. Protected by
> + * &drm_device.mode_config.connection_mutex.
> + * @mcs: message capability status - DP 1.2 spec. Protected by
> + * &drm_device.mode_config.connection_mutex.
> + * @ddps: DisplayPort Device Plug Status - DP 1.2. Protected by
> + * &drm_device.mode_config.connection_mutex.
> + * @pdt: Peer Device Type. Protected by
> + * &drm_device.mode_config.connection_mutex.
> + * @ldps: Legacy Device Plug Status. Protected by
> + * &drm_device.mode_config.connection_mutex.
> + * @dpcd_rev: DPCD revision of device on this port. Protected by
> + * &drm_device.mode_config.connection_mutex.
> + * @num_sdp_streams: Number of simultaneous streams. Protected by
> + * &drm_device.mode_config.connection_mutex.
> + * @num_sdp_stream_sinks: Number of stream sinks. Protected by
> + * &drm_device.mode_config.connection_mutex.
> + * @available_pbn: Available bandwidth for this port. Protected by
> + * &drm_device.mode_config.connection_mutex.
> * @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
> + * by &drm_device.mode_config.connection_mutex.
> * @parent: branch device parent of this port
> * @vcpi: Virtual Channel Payload info for this port.
> - * @connector: DRM connector this port is connected to.
> + * @connector: DRM connector this port is connected to. Protected by @lock.
> + * When there is already a connector registered for this port, this is also
> + * protected by &drm_device.mode_config.connection_mutex.
> * @mgr: topology manager this port lives under.
> *
> * This structure represents an MST port endpoint on a device somewhere
> @@ -100,6 +111,12 @@ struct drm_dp_mst_port {
> struct drm_connector *connector;
> struct drm_dp_mst_topology_mgr *mgr;
>
> + /**
> + * @lock: Protects @connector. If needed, this lock should be grabbed
> + * before &drm_device.mode_config.connection_mutex.
> + */
> + struct mutex lock;
> +
> /**
> * @cached_edid: for DP logical ports - make tiling work by ensuring
> * that the EDID for all connectors is read immediately.
> --
> 2.21.0
>
--
Sean Paul, Software Engineer, Google / Chromium OS
next prev parent reply other threads:[~2019-09-25 20:00 UTC|newest]
Thread overview: 62+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20190903204645.25487-1-lyude@redhat.com>
2019-09-03 20:45 ` [PATCH v2 01/27] drm/dp_mst: Move link address dumping into a function Lyude Paul
2019-09-25 17:45 ` Sean Paul
2019-09-03 20:45 ` [PATCH v2 02/27] drm/dp_mst: Get rid of list clear in destroy_connector_work Lyude Paul
2019-09-25 17:45 ` Sean Paul
2019-09-03 20:45 ` [PATCH v2 03/27] drm/dp_mst: Destroy MSTBs asynchronously Lyude Paul
2019-09-25 18:16 ` Sean Paul
2019-09-25 20:08 ` Lyude Paul
2019-09-27 13:31 ` Sean Paul
2019-10-08 9:45 ` Daniel Vetter
2019-09-03 20:45 ` [PATCH v2 04/27] drm/dp_mst: Move test_calc_pbn_mode() into an actual selftest Lyude Paul
2019-09-25 18:17 ` Sean Paul
2019-09-03 20:45 ` [PATCH v2 05/27] drm/print: Add drm_err_printer() Lyude Paul
2019-09-03 20:45 ` [PATCH v2 06/27] drm/dp_mst: Combine redundant cases in drm_dp_encode_sideband_req() Lyude Paul
2019-09-03 21:35 ` Dave Airlie
2019-09-03 21:57 ` [PATCH v3] " Lyude Paul
2019-09-03 20:45 ` [PATCH v2 07/27] drm/dp_mst: Add sideband down request tracing + selftests Lyude Paul
2019-09-10 9:01 ` Jani Nikula
2019-09-03 20:45 ` [PATCH v2 08/27] drm/dp_mst: Remove PDT teardown in drm_dp_destroy_port() and refactor Lyude Paul
2019-09-25 19:00 ` Sean Paul
2019-09-03 20:45 ` [PATCH v2 09/27] drm/dp_mst: Refactor drm_dp_send_enum_path_resources Lyude Paul
2019-09-03 20:45 ` [PATCH v2 10/27] drm/dp_mst: Remove huge conditional in drm_dp_mst_handle_up_req() Lyude Paul
2019-09-03 20:45 ` [PATCH v2 11/27] drm/dp_mst: Constify guid in drm_dp_get_mst_branch_by_guid() Lyude Paul
2019-09-03 21:41 ` Dave Airlie
2019-09-03 20:45 ` [PATCH v2 12/27] drm/dp_mst: Refactor drm_dp_mst_handle_up_req() Lyude Paul
2019-09-03 20:45 ` [PATCH v2 13/27] drm/dp_mst: Refactor drm_dp_mst_handle_down_rep() Lyude Paul
2019-09-03 20:45 ` [PATCH v2 14/27] drm/dp_mst: Destroy topology_mgr mutexes Lyude Paul
2019-09-25 19:14 ` Sean Paul
2019-09-03 20:45 ` [PATCH v2 15/27] drm/dp_mst: Cleanup drm_dp_send_link_address() a bit Lyude Paul
2019-09-03 21:42 ` Dave Airlie
2019-09-03 20:45 ` [PATCH v2 16/27] drm/dp_mst: Refactor pdt setup/teardown, add more locking Lyude Paul
2019-09-25 19:27 ` Sean Paul
2019-09-25 21:00 ` Lyude Paul
2019-09-27 13:30 ` Sean Paul
2019-09-03 20:45 ` [PATCH v2 17/27] drm/dp_mst: Rename drm_dp_add_port and drm_dp_update_port Lyude Paul
2019-09-25 19:30 ` Sean Paul
2019-09-03 20:45 ` [PATCH v2 18/27] drm/dp_mst: Remove lies in {up,down}_rep_recv documentation Lyude Paul
2019-09-25 19:32 ` Sean Paul
2019-09-03 20:45 ` [PATCH v2 19/27] drm/dp_mst: Handle UP requests asynchronously Lyude Paul
2019-09-25 19:46 ` Sean Paul
2019-09-03 20:45 ` [PATCH v2 20/27] drm/dp_mst: Protect drm_dp_mst_port members with connection_mutex Lyude Paul
2019-09-25 20:00 ` Sean Paul [this message]
2019-09-25 21:01 ` Lyude Paul
2019-09-03 20:45 ` [PATCH v2 21/27] drm/dp_mst: Don't forget to update port->input in drm_dp_mst_handle_conn_stat() Lyude Paul
2019-09-25 20:03 ` Sean Paul
2019-09-03 20:46 ` [PATCH v2 22/27] drm/nouveau: Don't grab runtime PM refs for HPD IRQs Lyude Paul
2019-09-25 20:06 ` Sean Paul
2019-09-03 20:46 ` [PATCH v2 23/27] drm/amdgpu: Iterate through DRM connectors correctly Lyude Paul
2019-09-13 20:45 ` Alex Deucher
2019-09-27 13:48 ` Alex Deucher
2019-09-03 20:46 ` [PATCH v2 24/27] drm/amdgpu/dm: Resume short HPD IRQs before resuming MST topology Lyude Paul
2019-09-13 20:46 ` Alex Deucher
2019-09-25 20:08 ` Sean Paul
2019-09-25 21:52 ` [PATCH v4] " Lyude Paul
2019-09-27 13:29 ` Alex Deucher
2019-09-03 20:46 ` [PATCH v2 25/27] drm/dp_mst: Add basic topology reprobing when resuming Lyude Paul
2019-09-27 13:52 ` Sean Paul
2019-10-09 19:06 ` Lyude Paul
2019-09-03 20:46 ` [PATCH v2 26/27] drm/dp_mst: Also print unhashed pointers for malloc/topology references Lyude Paul
2019-09-27 14:25 ` Sean Paul
2019-10-09 19:40 ` Lyude Paul
2019-09-03 20:46 ` [PATCH v2 27/27] drm/dp_mst: Add topology ref history tracking for debugging Lyude Paul
2019-09-27 14:51 ` Sean Paul
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190925200020.GL218215@art_vandelay \
--to=sean@poorly.run \
--cc=airlied@linux.ie \
--cc=amd-gfx@lists.freedesktop.org \
--cc=daniel.vetter@ffwll.ch \
--cc=daniel@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
--cc=hwentlan@amd.com \
--cc=imre.deak@intel.com \
--cc=juston.li@intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=lyude@redhat.com \
--cc=maarten.lankhorst@linux.intel.com \
--cc=mripard@kernel.org \
--cc=nouveau@lists.freedesktop.org \
--cc=ville.syrjala@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox