linux-kernel - Re: [PATCH 06/26] drm/dp_mst: Move PDT teardown for ports into destroy_connector

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190813145242.GW7444@phenom.ffwll.local>
Date:   Tue, 13 Aug 2019 16:52:42 +0200
From:   Daniel Vetter <daniel@...ll.ch>
To:     Lyude Paul <lyude@...hat.com>
Cc:     dri-devel@...ts.freedesktop.org, Juston Li <juston.li@...el.com>,
        Imre Deak <imre.deak@...el.com>,
        Ville Syrjälä 
        <ville.syrjala@...ux.intel.com>, Harry Wentland <hwentlan@....com>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <maxime.ripard@...tlin.com>,
        Sean Paul <sean@...rly.run>, David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 06/26] drm/dp_mst: Move PDT teardown for ports into
 destroy_connector_work

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@...el.com>
> Cc: Imre Deak <imre.deak@...el.com>
> Cc: Ville Syrjälä <ville.syrjala@...ux.intel.com>
> Cc: Harry Wentland <hwentlan@....com>
> Signed-off-by: Lyude Paul <lyude@...hat.com>
> ---
>  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)).

>  }
>  
>  /**
> @@ -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

> +		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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch