On Tue, 2019-08-13 at 16:52 +0200, Daniel Vetter wrote: > On Wed, Jul 17, 2019 at 09:42:29PM -0400, Lyude Paul wrote: > > This will allow us to add some locking for port PDTs, which can't be > > done from drm_dp_destroy_port() since we don't know what locks the > > caller might be holding. Also, this gets rid of a good bit of unneeded > > code. > > > > Cc: Juston Li <juston.li@xxxxxxxxx> > > Cc: Imre Deak <imre.deak@xxxxxxxxx> > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > Cc: Harry Wentland <hwentlan@xxxxxxx> > > Signed-off-by: Lyude Paul <lyude@xxxxxxxxxx> > > --- > > drivers/gpu/drm/drm_dp_mst_topology.c | 42 +++++++++++---------------- > > 1 file changed, 17 insertions(+), 25 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c > > b/drivers/gpu/drm/drm_dp_mst_topology.c > > index defc5e09fb9a..0295e007c836 100644 > > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > > @@ -1509,31 +1509,22 @@ static void drm_dp_destroy_port(struct kref *kref) > > container_of(kref, struct drm_dp_mst_port, topology_kref); > > struct drm_dp_mst_topology_mgr *mgr = port->mgr; > > > > - if (!port->input) { > > - kfree(port->cached_edid); > > - > > - /* > > - * The only time we don't have a connector > > - * on an output port is if the connector init > > - * fails. > > - */ > > - if (port->connector) { > > - /* we can't destroy the connector here, as > > - * we might be holding the mode_config.mutex > > - * from an EDID retrieval */ > > - > > - mutex_lock(&mgr->destroy_connector_lock); > > - list_add(&port->next, &mgr->destroy_connector_list); > > - mutex_unlock(&mgr->destroy_connector_lock); > > - schedule_work(&mgr->destroy_connector_work); > > - return; > > - } > > - /* no need to clean up vcpi > > - * as if we have no connector we never setup a vcpi */ > > - drm_dp_port_teardown_pdt(port, port->pdt); > > - port->pdt = DP_PEER_DEVICE_NONE; > > + /* There's nothing that needs locking to destroy an input port yet */ > > + if (port->input) { > > + drm_dp_mst_put_port_malloc(port); > > + return; > > } > > - drm_dp_mst_put_port_malloc(port); > > + > > + kfree(port->cached_edid); > > + > > + /* > > + * we can't destroy the connector here, as we might be holding the > > + * mode_config.mutex from an EDID retrieval > > + */ > > + mutex_lock(&mgr->destroy_connector_lock); > > + list_add(&port->next, &mgr->destroy_connector_list); > > + mutex_unlock(&mgr->destroy_connector_lock); > > + schedule_work(&mgr->destroy_connector_work); > > So if I'm not completely blind this just flattens the above code flow (by > inverting the if (port->input)). Now I'm really remembering why I refactored this. The control flow on the previous version of this is pretty misleading. To summarize so it's a bit more obvious: if (port->input) { drm_dp_mst_put_port_malloc(port); return; } else if (port->connector) { add_connector_to_destroy_list(); return; /* ^ now, this is where PDT teardown happens */ } else { drm_dp_port_teardown_pdt(port, port->pdt); } /* free edid etc etc */ So, I suppose the title of this patch would be more accurate if it was "drm/dp_mst: Remove PDT teardown in destroy_port() and refactor" I'll go ahead and change that > > > } > > > > /** > > @@ -3881,7 +3872,8 @@ drm_dp_finish_destroy_port(struct drm_dp_mst_port > > *port) > > { > > INIT_LIST_HEAD(&port->next); > > > > - port->mgr->cbs->destroy_connector(port->mgr, port->connector); > > + if (port->connector) > > And this here I can't connect with the commit message. I'm confused, did > something go wrong with some rebase here, and this patch should have a > different title/summary? > -Daniel No, this is correct. In the previous drm_dp_destroy_port() function we only added a port to the delayed destroy work if it had a connector on it. Now that we add ports unconditionally, we have to check port->connector before trying to call port->mgr->cbs->destroy_connector() since port->connector is no longer guaranteed to be != NULL. > > > + port->mgr->cbs->destroy_connector(port->mgr, port->connector); > > > > drm_dp_port_teardown_pdt(port, port->pdt); > > port->pdt = DP_PEER_DEVICE_NONE; > > -- > > 2.21.0 > > -- Cheers, Lyude Paul _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel