Re: [PATCH v2 2/2] drm/dp_mst: Avoid to mess up payload table by ports in stale topology

["Souza, Jose" <jose.souza@intel.com>]

On Wed, 2021-06-16 at 11:55 +0800, Wayne Lin wrote:
> [Why]
> After unplug/hotplug hub from the system, userspace might start to
> clear stale payloads gradually. If we call drm_dp_mst_deallocate_vcpi()
> to release stale VCPI of those ports which are not relating to current
> topology, we have chane to wrongly clear active payload table entry for
> current topology.
> 
> E.g.
> We have allocated VCPI 1 in current payload table and we call
> drm_dp_mst_deallocate_vcpi() to clean VCPI 1 in stale topology. In
> drm_dp_mst_deallocate_vcpi(), it will call drm_dp_mst_put_payload_id()
> tp put VCPI 1 and which means ID 1 is available again. Thereafter, if we
> want to allocate a new payload stream, it will find ID 1 is available by
> drm_dp_mst_assign_payload_id(). However, ID 1 is being used
> 
> [How]
> Check target sink is relating to current topology or not before doing
> any payload table update.
> Searching upward to find the target sink's relevant root branch device.
> If the found root branch device is not the same root of current
> topology, don't update payload table.
> 
> Changes since v1:
> * Change debug macro to use drm_dbg_kms() instead
> * Amend the commit message to add Cc tag.
> 
> Signed-off-by: Wayne Lin <Wayne.Lin@amd.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 29 +++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> index b41b837db66d..9ac148efd9e4 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -94,6 +94,9 @@ static int drm_dp_mst_register_i2c_bus(struct drm_dp_mst_port *port);
>  static void drm_dp_mst_unregister_i2c_bus(struct drm_dp_mst_port *port);
>  static void drm_dp_mst_kick_tx(struct drm_dp_mst_topology_mgr *mgr);
>  
> +static bool drm_dp_mst_port_downstream_of_branch(struct drm_dp_mst_port *port,
> +						 struct drm_dp_mst_branch *branch);
> +
>  #define DBG_PREFIX "[dp_mst]"
>  
>  #define DP_STR(x) [DP_ ## x] = #x
> @@ -3366,6 +3369,7 @@ int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr)
>  	struct drm_dp_mst_port *port;
>  	int i, j;
>  	int cur_slots = 1;
> +	bool skip;
>  
>  	mutex_lock(&mgr->payload_lock);
>  	for (i = 0; i < mgr->max_payloads; i++) {
> @@ -3380,6 +3384,14 @@ int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr)
>  			port = container_of(vcpi, struct drm_dp_mst_port,
>  					    vcpi);
>  
> +			mutex_lock(&mgr->lock);
> +			skip = !drm_dp_mst_port_downstream_of_branch(port, mgr->mst_primary);
> +			mutex_unlock(&mgr->lock);
> +
> +			if (skip) {
> +				drm_dbg_kms("Virtual channel %d is not in current topology\n", i);

Missing dev/drm parameter and breaking the build.

> +				continue;
> +			}
>  			/* Validated ports don't matter if we're releasing
>  			 * VCPI
>  			 */
> @@ -3479,6 +3491,7 @@ int drm_dp_update_payload_part2(struct drm_dp_mst_topology_mgr *mgr)
>  	struct drm_dp_mst_port *port;
>  	int i;
>  	int ret = 0;
> +	bool skip;
>  
>  	mutex_lock(&mgr->payload_lock);
>  	for (i = 0; i < mgr->max_payloads; i++) {
> @@ -3488,6 +3501,13 @@ int drm_dp_update_payload_part2(struct drm_dp_mst_topology_mgr *mgr)
>  
>  		port = container_of(mgr->proposed_vcpis[i], struct drm_dp_mst_port, vcpi);
>  
> +		mutex_lock(&mgr->lock);
> +		skip = !drm_dp_mst_port_downstream_of_branch(port, mgr->mst_primary);
> +		mutex_unlock(&mgr->lock);
> +
> +		if (skip)
> +			continue;
> +
>  		drm_dbg_kms(mgr->dev, "payload %d %d\n", i, mgr->payloads[i].payload_state);
>  		if (mgr->payloads[i].payload_state == DP_PAYLOAD_LOCAL) {
>  			ret = drm_dp_create_payload_step2(mgr, port, mgr->proposed_vcpis[i]->vcpi, &mgr->payloads[i]);
> @@ -4574,9 +4594,18 @@ 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)
>  {
> +	bool skip;
> +
>  	if (!port->vcpi.vcpi)
>  		return;
>  
> +	mutex_lock(&mgr->lock);
> +	skip = !drm_dp_mst_port_downstream_of_branch(port, mgr->mst_primary);
> +	mutex_unlock(&mgr->lock);
> +
> +	if (skip)
> +		return;
> +
>  	drm_dp_mst_put_payload_id(mgr, port->vcpi.vcpi);
>  	port->vcpi.num_slots = 0;
>  	port->vcpi.pbn = 0;