Re: [PATCH v1 2/2] platform/chrome: cros_ec_typec: Add lock per-port

[Benson Leung <bleung@google.com>]

Hi Radu,


On Wed, Jul 09, 2025 at 01:22:32PM +0000, Radu Vele wrote:
> Add a lock associated to each port to protect port data against
> concurrent access. Concurrency may result from sysfs commands
> and ec events.
> 
> Signed-off-by: Radu Vele <raduvele@google.com>
> ---
>  drivers/platform/chrome/cros_ec_typec.c | 38 +++++++++++++++++++------
>  drivers/platform/chrome/cros_ec_typec.h |  3 ++
>  2 files changed, 32 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> index 289429ef959f..bbb9e02624b8 100644
> --- a/drivers/platform/chrome/cros_ec_typec.c
> +++ b/drivers/platform/chrome/cros_ec_typec.c
> @@ -8,6 +8,7 @@
>  
>  #include <linux/acpi.h>
>  #include <linux/module.h>
> +#include <linux/mutex.h>
>  #include <linux/of.h>
>  #include <linux/platform_data/cros_ec_commands.h>
>  #include <linux/platform_data/cros_usbpd_notify.h>
> @@ -54,8 +55,11 @@ static int cros_typec_enter_usb_mode(struct typec_port *tc_port, enum usb_mode m
>  		.mode_to_enter = CROS_EC_ALTMODE_USB4
>  	};
>  
> -	return cros_ec_cmd(port->typec_data->ec, 0, EC_CMD_TYPEC_CONTROL,
> +	mutex_lock(&port->lock);
> +	int ret = cros_ec_cmd(port->typec_data->ec, 0, EC_CMD_TYPEC_CONTROL,
>  			  &req, sizeof(req), NULL, 0);

Please declare "int ret" at the top of the block, as per other examples of it
in this file.

You have it in the middle of a block right now.

> +	mutex_unlock(&port->lock);
> +	return ret;
>  }
>  
>  static int cros_typec_perform_role_swap(struct typec_port *tc_port, int target_role, u8 swap_type)
> @@ -70,6 +74,8 @@ static int cros_typec_perform_role_swap(struct typec_port *tc_port, int target_r
>  	if (!data->pd_ctrl_ver)
>  		return -EOPNOTSUPP;
>  
> +	mutex_lock(&port->lock);
> +
>  	/* First query the state */
>  	req.port = port->port_num;
>  	req.role = USB_PD_CTRL_ROLE_NO_CHANGE;
> @@ -79,7 +85,7 @@ static int cros_typec_perform_role_swap(struct typec_port *tc_port, int target_r
>  	ret = cros_ec_cmd(data->ec, data->pd_ctrl_ver, EC_CMD_USB_PD_CONTROL,
>  				&req, sizeof(req), &resp, sizeof(resp));
>  	if (ret < 0)
> -		return ret;
> +		goto unlock;
>  
>  	switch (swap_type) {
>  	case USB_PD_CTRL_SWAP_DATA:
> @@ -92,18 +98,21 @@ static int cros_typec_perform_role_swap(struct typec_port *tc_port, int target_r
>  		break;
>  	default:
>  		dev_warn(data->dev, "Unsupported role swap type %d", swap_type);
> -		return -EOPNOTSUPP;
> +		ret = -EOPNOTSUPP;
> +		goto unlock;
>  	}
>  
> -	if (role == target_role)
> -		return 0;
> +	if (role == target_role) {
> +		ret = 0;
> +		goto unlock;
> +	}
>  
>  	req.swap = swap_type;
>  	ret = cros_ec_cmd(data->ec, data->pd_ctrl_ver, EC_CMD_USB_PD_CONTROL,
>  				&req, sizeof(req), &resp, sizeof(resp));
>  
>  	if (ret < 0)
> -		return ret;
> +		goto unlock;
>  
>  	switch (swap_type) {
>  	case USB_PD_CTRL_SWAP_DATA:
> @@ -117,8 +126,11 @@ static int cros_typec_perform_role_swap(struct typec_port *tc_port, int target_r
>  									TYPEC_SINK);
>  		break;
>  	}
> +	ret = 0;
>  
> -	return 0;
> +unlock:
> +	mutex_unlock(&port->lock);
> +	return ret;
>  }
>  
>  static int cros_typec_dr_swap(struct typec_port *port, enum typec_data_role role)
> @@ -370,6 +382,7 @@ static void cros_unregister_ports(struct cros_typec_data *typec)
>  		if (!typec->ports[i])
>  			continue;
>  
> +		mutex_lock(&typec->ports[i]->lock);
>  		cros_typec_remove_partner(typec, i);
>  		cros_typec_remove_cable(typec, i);
>  
> @@ -378,6 +391,8 @@ static void cros_unregister_ports(struct cros_typec_data *typec)
>  		typec_mux_put(typec->ports[i]->mux);
>  		cros_typec_unregister_port_altmodes(typec->ports[i]);
>  		typec_unregister_port(typec->ports[i]->port);
> +		mutex_unlock(&typec->ports[i]->lock);
> +		mutex_destroy(&typec->ports[i]->lock);
>  	}
>  }
>  
> @@ -472,6 +487,7 @@ static int cros_typec_init_ports(struct cros_typec_data *typec)
>  			goto unregister_ports;
>  		}
>  
> +		mutex_init(&cros_port->lock);
>  		cros_port->port_num = port_num;
>  		cros_port->typec_data = typec;
>  		typec->ports[port_num] = cros_port;
> @@ -1232,6 +1248,7 @@ static int cros_typec_port_update(struct cros_typec_data *typec, int port_num)
>  		return -EINVAL;
>  	}
>  
> +	mutex_lock(&typec->ports[port_num]->lock);
>  	req.port = port_num;
>  	req.role = USB_PD_CTRL_ROLE_NO_CHANGE;
>  	req.mux = USB_PD_CTRL_MUX_NO_CHANGE;
> @@ -1241,7 +1258,7 @@ static int cros_typec_port_update(struct cros_typec_data *typec, int port_num)
>  			  EC_CMD_USB_PD_CONTROL, &req, sizeof(req),
>  			  &resp, sizeof(resp));
>  	if (ret < 0)
> -		return ret;
> +		goto unlock;
>  
>  	/* Update the switches if they exist, according to requested state */
>  	ret = cros_typec_configure_mux(typec, port_num, &resp);
> @@ -1263,7 +1280,10 @@ static int cros_typec_port_update(struct cros_typec_data *typec, int port_num)
>  	if (typec->typec_cmd_supported)
>  		cros_typec_handle_status(typec, port_num);
>  
> -	return 0;
> +	ret = 0;
> +unlock:
> +	mutex_unlock(&typec->ports[port_num]->lock);
> +	return ret;
>  }
>  
>  static int cros_typec_get_cmd_version(struct cros_typec_data *typec)
> diff --git a/drivers/platform/chrome/cros_ec_typec.h b/drivers/platform/chrome/cros_ec_typec.h
> index f9c31f04c102..d0a8a12ec91a 100644
> --- a/drivers/platform/chrome/cros_ec_typec.h
> +++ b/drivers/platform/chrome/cros_ec_typec.h
> @@ -82,6 +82,9 @@ struct cros_typec_port {
>  	struct usb_power_delivery_capabilities *partner_sink_caps;
>  
>  	struct cros_typec_data *typec_data;
> +
> +	/* Mutex to protect port data against concurrent access */
> +	struct mutex lock;
>  };
>  
>  #endif /*  __CROS_EC_TYPEC__ */
> -- 
> 2.50.0.727.gbf7dc18ff4-goog
>