diff mbox series

[v2,05/16] drm/dp_mst: Fix payload deallocation on hotplugs using malloc refs

Message ID 20181220001959.12486-6-lyude@redhat.com (mailing list archive)
State New, archived
Headers show
Series MST refcounting/atomic helpers cleanup | expand

Commit Message

Lyude Paul Dec. 20, 2018, 12:19 a.m. UTC 
Up until now, freeing payloads on remote MST hubs that just had ports
removed has almost never worked because we've been relying on port
validation in order to stop us from accessing ports that have already
been freed from memory, but ports which need their payloads released due
to being removed will never be a valid part of the topology after
they've been removed.

Since we've introduced malloc refs, we can replace all of the validation
logic in payload helpers which are used for deallocation with some
well-placed malloc krefs. This ensures that regardless of whether or not
the ports are still valid and in the topology, any port which has an
allocated payload will remain allocated in memory until it's payloads
have been removed - finally allowing us to actually release said
payloads correctly.

Signed-off-by: Lyude Paul <lyude@redhat.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: David Airlie <airlied@redhat.com>
Cc: Jerry Zuo <Jerry.Zuo@amd.com>
Cc: Harry Wentland <harry.wentland@amd.com>
Cc: Juston Li <juston.li@intel.com>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 54 +++++++++++++++------------
 1 file changed, 30 insertions(+), 24 deletions(-)

Comments

Harry Wentland Jan. 8, 2019, 7:27 p.m. UTC | #1 
On 2018-12-19 7:19 p.m., Lyude Paul wrote:
> Up until now, freeing payloads on remote MST hubs that just had ports
> removed has almost never worked because we've been relying on port
> validation in order to stop us from accessing ports that have already
> been freed from memory, but ports which need their payloads released due
> to being removed will never be a valid part of the topology after
> they've been removed.
> 
> Since we've introduced malloc refs, we can replace all of the validation
> logic in payload helpers which are used for deallocation with some
> well-placed malloc krefs. This ensures that regardless of whether or not
> the ports are still valid and in the topology, any port which has an
> allocated payload will remain allocated in memory until it's payloads
> have been removed - finally allowing us to actually release said
> payloads correctly.
> 
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: David Airlie <airlied@redhat.com>
> Cc: Jerry Zuo <Jerry.Zuo@amd.com>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Cc: Juston Li <juston.li@intel.com>

Reviewed-by: Harry Wentland <harry.wentland@amd.com>

Harry

> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 54 +++++++++++++++------------
>  1 file changed, 30 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> index ef8637f37564..11dd3ede7b7d 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -2100,10 +2100,6 @@ static int drm_dp_payload_send_msg(struct drm_dp_mst_topology_mgr *mgr,
>  	u8 sinks[DRM_DP_MAX_SDP_STREAMS];
>  	int i;
>  
> -	port = drm_dp_mst_topology_get_port_validated(mgr, port);
> -	if (!port)
> -		return -EINVAL;
> -
>  	port_num = port->port_num;
>  	mstb = drm_dp_mst_topology_get_mstb_validated(mgr, port->parent);
>  	if (!mstb) {
> @@ -2111,10 +2107,8 @@ static int drm_dp_payload_send_msg(struct drm_dp_mst_topology_mgr *mgr,
>  							       port->parent,
>  							       &port_num);
>  
> -		if (!mstb) {
> -			drm_dp_mst_topology_put_port(port);
> +		if (!mstb)
>  			return -EINVAL;
> -		}
>  	}
>  
>  	txmsg = kzalloc(sizeof(*txmsg), GFP_KERNEL);
> @@ -2151,7 +2145,6 @@ static int drm_dp_payload_send_msg(struct drm_dp_mst_topology_mgr *mgr,
>  	kfree(txmsg);
>  fail_put:
>  	drm_dp_mst_topology_put_mstb(mstb);
> -	drm_dp_mst_topology_put_port(port);
>  	return ret;
>  }
>  
> @@ -2256,15 +2249,16 @@ static int drm_dp_destroy_payload_step2(struct drm_dp_mst_topology_mgr *mgr,
>   */
>  int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr)
>  {
> -	int i, j;
> -	int cur_slots = 1;
>  	struct drm_dp_payload req_payload;
>  	struct drm_dp_mst_port *port;
> +	int i, j;
> +	int cur_slots = 1;
>  
>  	mutex_lock(&mgr->payload_lock);
>  	for (i = 0; i < mgr->max_payloads; i++) {
>  		struct drm_dp_vcpi *vcpi = mgr->proposed_vcpis[i];
>  		struct drm_dp_payload *payload = &mgr->payloads[i];
> +		bool put_port = false;
>  
>  		/* solve the current payloads - compare to the hw ones
>  		   - update the hw view */
> @@ -2272,12 +2266,20 @@ int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr)
>  		if (vcpi) {
>  			port = container_of(vcpi, struct drm_dp_mst_port,
>  					    vcpi);
> -			port = drm_dp_mst_topology_get_port_validated(mgr,
> -								      port);
> -			if (!port) {
> -				mutex_unlock(&mgr->payload_lock);
> -				return -EINVAL;
> +
> +			/* Validated ports don't matter if we're releasing
> +			 * VCPI
> +			 */
> +			if (vcpi->num_slots) {
> +				port = drm_dp_mst_topology_get_port_validated(
> +				    mgr, port);
> +				if (!port) {
> +					mutex_unlock(&mgr->payload_lock);
> +					return -EINVAL;
> +				}
> +				put_port = true;
>  			}
> +
>  			req_payload.num_slots = vcpi->num_slots;
>  			req_payload.vcpi = vcpi->vcpi;
>  		} else {
> @@ -2309,7 +2311,7 @@ int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr)
>  		}
>  		cur_slots += req_payload.num_slots;
>  
> -		if (port)
> +		if (put_port)
>  			drm_dp_mst_topology_put_port(port);
>  	}
>  
> @@ -3124,6 +3126,8 @@ bool drm_dp_mst_allocate_vcpi(struct drm_dp_mst_topology_mgr *mgr,
>  	DRM_DEBUG_KMS("initing vcpi for pbn=%d slots=%d\n",
>  		      pbn, port->vcpi.num_slots);
>  
> +	/* Keep port allocated until it's payload has been removed */
> +	drm_dp_mst_get_port_malloc(port);
>  	drm_dp_mst_topology_put_port(port);
>  	return true;
>  out:
> @@ -3153,11 +3157,12 @@ EXPORT_SYMBOL(drm_dp_mst_get_vcpi_slots);
>   */
>  void drm_dp_mst_reset_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port)
>  {
> -	port = drm_dp_mst_topology_get_port_validated(mgr, port);
> -	if (!port)
> -		return;
> +	/*
> +	 * A port with VCPI will remain allocated until it's VCPI is
> +	 * released, no verified ref needed
> +	 */
> +
>  	port->vcpi.num_slots = 0;
> -	drm_dp_mst_topology_put_port(port);
>  }
>  EXPORT_SYMBOL(drm_dp_mst_reset_vcpi_slots);
>  
> @@ -3169,16 +3174,17 @@ EXPORT_SYMBOL(drm_dp_mst_reset_vcpi_slots);
>  void drm_dp_mst_deallocate_vcpi(struct drm_dp_mst_topology_mgr *mgr,
>  				struct drm_dp_mst_port *port)
>  {
> -	port = drm_dp_mst_topology_get_port_validated(mgr, port);
> -	if (!port)
> -		return;
> +	/*
> +	 * A port with VCPI will remain allocated until it's VCPI is
> +	 * released, no verified ref needed
> +	 */
>  
>  	drm_dp_mst_put_payload_id(mgr, port->vcpi.vcpi);
>  	port->vcpi.num_slots = 0;
>  	port->vcpi.pbn = 0;
>  	port->vcpi.aligned_pbn = 0;
>  	port->vcpi.vcpi = 0;
> -	drm_dp_mst_topology_put_port(port);
> +	drm_dp_mst_put_port_malloc(port);
>  }
>  EXPORT_SYMBOL(drm_dp_mst_deallocate_vcpi);
>  
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index ef8637f37564..11dd3ede7b7d 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -2100,10 +2100,6 @@  static int drm_dp_payload_send_msg(struct drm_dp_mst_topology_mgr *mgr,
 	u8 sinks[DRM_DP_MAX_SDP_STREAMS];
 	int i;
 
-	port = drm_dp_mst_topology_get_port_validated(mgr, port);
-	if (!port)
-		return -EINVAL;
-
 	port_num = port->port_num;
 	mstb = drm_dp_mst_topology_get_mstb_validated(mgr, port->parent);
 	if (!mstb) {
@@ -2111,10 +2107,8 @@  static int drm_dp_payload_send_msg(struct drm_dp_mst_topology_mgr *mgr,
 							       port->parent,
 							       &port_num);
 
-		if (!mstb) {
-			drm_dp_mst_topology_put_port(port);
+		if (!mstb)
 			return -EINVAL;
-		}
 	}
 
 	txmsg = kzalloc(sizeof(*txmsg), GFP_KERNEL);
@@ -2151,7 +2145,6 @@  static int drm_dp_payload_send_msg(struct drm_dp_mst_topology_mgr *mgr,
 	kfree(txmsg);
 fail_put:
 	drm_dp_mst_topology_put_mstb(mstb);
-	drm_dp_mst_topology_put_port(port);
 	return ret;
 }
 
@@ -2256,15 +2249,16 @@  static int drm_dp_destroy_payload_step2(struct drm_dp_mst_topology_mgr *mgr,
  */
 int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr)
 {
-	int i, j;
-	int cur_slots = 1;
 	struct drm_dp_payload req_payload;
 	struct drm_dp_mst_port *port;
+	int i, j;
+	int cur_slots = 1;
 
 	mutex_lock(&mgr->payload_lock);
 	for (i = 0; i < mgr->max_payloads; i++) {
 		struct drm_dp_vcpi *vcpi = mgr->proposed_vcpis[i];
 		struct drm_dp_payload *payload = &mgr->payloads[i];
+		bool put_port = false;
 
 		/* solve the current payloads - compare to the hw ones
 		   - update the hw view */
@@ -2272,12 +2266,20 @@  int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr)
 		if (vcpi) {
 			port = container_of(vcpi, struct drm_dp_mst_port,
 					    vcpi);
-			port = drm_dp_mst_topology_get_port_validated(mgr,
-								      port);
-			if (!port) {
-				mutex_unlock(&mgr->payload_lock);
-				return -EINVAL;
+
+			/* Validated ports don't matter if we're releasing
+			 * VCPI
+			 */
+			if (vcpi->num_slots) {
+				port = drm_dp_mst_topology_get_port_validated(
+				    mgr, port);
+				if (!port) {
+					mutex_unlock(&mgr->payload_lock);
+					return -EINVAL;
+				}
+				put_port = true;
 			}
+
 			req_payload.num_slots = vcpi->num_slots;
 			req_payload.vcpi = vcpi->vcpi;
 		} else {
@@ -2309,7 +2311,7 @@  int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr)
 		}
 		cur_slots += req_payload.num_slots;
 
-		if (port)
+		if (put_port)
 			drm_dp_mst_topology_put_port(port);
 	}
 
@@ -3124,6 +3126,8 @@  bool drm_dp_mst_allocate_vcpi(struct drm_dp_mst_topology_mgr *mgr,
 	DRM_DEBUG_KMS("initing vcpi for pbn=%d slots=%d\n",
 		      pbn, port->vcpi.num_slots);
 
+	/* Keep port allocated until it's payload has been removed */
+	drm_dp_mst_get_port_malloc(port);
 	drm_dp_mst_topology_put_port(port);
 	return true;
 out:
@@ -3153,11 +3157,12 @@  EXPORT_SYMBOL(drm_dp_mst_get_vcpi_slots);
  */
 void drm_dp_mst_reset_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port)
 {
-	port = drm_dp_mst_topology_get_port_validated(mgr, port);
-	if (!port)
-		return;
+	/*
+	 * A port with VCPI will remain allocated until it's VCPI is
+	 * released, no verified ref needed
+	 */
+
 	port->vcpi.num_slots = 0;
-	drm_dp_mst_topology_put_port(port);
 }
 EXPORT_SYMBOL(drm_dp_mst_reset_vcpi_slots);
 
@@ -3169,16 +3174,17 @@  EXPORT_SYMBOL(drm_dp_mst_reset_vcpi_slots);
 void drm_dp_mst_deallocate_vcpi(struct drm_dp_mst_topology_mgr *mgr,
 				struct drm_dp_mst_port *port)
 {
-	port = drm_dp_mst_topology_get_port_validated(mgr, port);
-	if (!port)
-		return;
+	/*
+	 * A port with VCPI will remain allocated until it's VCPI is
+	 * released, no verified ref needed
+	 */
 
 	drm_dp_mst_put_payload_id(mgr, port->vcpi.vcpi);
 	port->vcpi.num_slots = 0;
 	port->vcpi.pbn = 0;
 	port->vcpi.aligned_pbn = 0;
 	port->vcpi.vcpi = 0;
-	drm_dp_mst_topology_put_port(port);
+	drm_dp_mst_put_port_malloc(port);
 }
 EXPORT_SYMBOL(drm_dp_mst_deallocate_vcpi);