Re: [PATCHv3] usb: typec: Add a sysfs node to manage port type

[Guenter Roeck <linux@roeck-us.net>]

On Fri, May 26, 2017 at 10:45:59AM -0700, Badhri Jagan Sridharan wrote:
> User space applications in some cases have the need to enforce a
> specific port type(DFP/UFP/DRP). This change allows userspace to
> attempt setting the desired port type. Low level drivers can
> however reject the request if the specific port type is not supported.
> 
> Signed-off-by: Badhri Jagan Sridharan <Badhri@google.com>
> Change-Id: Ia76877558c47271a34d912a54eea3459655dda3c
> ---
> Changelog since v1:
> - introduced a new variable port_type in struct typec_port to track
>   the current port type instead of changing type member in
>   typec_capability to address Heikki Krogerus comments.
> - changed the output format and strings that would be displayed as
>   suggested by Heikki Krogerus.
> 
> Changelog since v2:
> - introduced a new mutex lock to protect port_type for addressing
>   the race conditions identified by Geunter Roeck
> - added typec_port_types_drp for printing port_type when cap->type
>   is TYPE_PORT_DRP as suggested by Geunter Roeck
> - Power role swap and data role swaps would be rejected unless
>   port port_type == TYPE_PORT_DRP
> - port_type_store would return immediately if the current port_type
>   is same as the new port_type as suggested by Geunter Roeck
> 
>  drivers/usb/typec/typec.c | 127 ++++++++++++++++++++++++++++++++++++++++------
>  include/linux/usb/typec.h |   4 ++
>  2 files changed, 116 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/usb/typec/typec.c b/drivers/usb/typec/typec.c
> index 89e540bb7ff3..a0d8887ad560 100644
> --- a/drivers/usb/typec/typec.c
> +++ b/drivers/usb/typec/typec.c
> @@ -11,9 +11,11 @@
>  
>  #include <linux/device.h>
>  #include <linux/module.h>
> +#include <linux/mutex.h>
>  #include <linux/slab.h>
>  #include <linux/usb/typec.h>
>  
> +

Single empty lines only, please.

>  struct typec_mode {
>  	int				index;
>  	u32				vdo;
> @@ -69,6 +71,8 @@ struct typec_port {
>  	enum typec_role			pwr_role;
>  	enum typec_role			vconn_role;
>  	enum typec_pwr_opmode		pwr_opmode;
> +	enum typec_port_type		port_type;
> +	struct mutex			port_type_lock;
>  
>  	const struct typec_capability	*cap;
>  };
> @@ -789,6 +793,19 @@ static const char * const typec_data_roles[] = {
>  	[TYPEC_HOST]	= "host",
>  };
>  
> +static const char * const typec_port_types[] = {
> +	[TYPEC_PORT_DFP] = "source",
> +	[TYPEC_PORT_UFP] = "sink",
> +	[TYPEC_PORT_DRP] = "dual",
> +};
> +
> +static const char * const typec_port_types_drp[] = {
> +	[TYPEC_PORT_DFP] = "dual [source] sink",
> +	[TYPEC_PORT_UFP] = "dual source [sink]",
> +	[TYPEC_PORT_DRP] = "[dual] source sink",
> +};
> +
> +
Single empty line.

>  static ssize_t
>  preferred_role_store(struct device *dev, struct device_attribute *attr,
>  		     const char *buf, size_t size)
> @@ -846,25 +863,34 @@ static ssize_t data_role_store(struct device *dev,
>  	struct typec_port *port = to_typec_port(dev);
>  	int ret;
>  
> -	if (port->cap->type != TYPEC_PORT_DRP) {
> -		dev_dbg(dev, "data role swap only supported with DRP ports\n");
> -		return -EOPNOTSUPP;
> -	}
> +	mutex_lock(&port->port_type_lock);
>  
>  	if (!port->cap->dr_set) {
>  		dev_dbg(dev, "data role swapping not supported\n");
> -		return -EOPNOTSUPP;
> +		ret = -EOPNOTSUPP;
> +		goto unlock_and_ret;
> +	}

I think you can check this outside the lock; capabilities won't change.

> +
> +	if (port->port_type != TYPEC_PORT_DRP) {
> +		dev_dbg(dev, "port type fixed at \"%s\"",
> +			     typec_port_types[port->port_type]);
> +		ret = -EOPNOTSUPP;
> +		goto unlock_and_ret;
>  	}
>  
>  	ret = sysfs_match_string(typec_data_roles, buf);
>  	if (ret < 0)
> -		return ret;
> +		goto unlock_and_ret;

Move outside the lock ?

>  
>  	ret = port->cap->dr_set(port->cap, ret);
>  	if (ret)
> -		return ret;
> +		goto unlock_and_ret;
>  
> -	return size;
> +	ret = size;
> +
> +unlock_and_ret:
> +	mutex_unlock(&port->port_type_lock);
> +	return ret;
>  }
>  
>  static ssize_t data_role_show(struct device *dev,
> @@ -885,32 +911,47 @@ static ssize_t power_role_store(struct device *dev,
>  				const char *buf, size_t size)
>  {
>  	struct typec_port *port = to_typec_port(dev);
> -	int ret = size;
> +	int ret;
>  
> +	mutex_lock(&port->port_type_lock);
>  	if (!port->cap->pd_revision) {
>  		dev_dbg(dev, "USB Power Delivery not supported\n");
> -		return -EOPNOTSUPP;
> +		ret = -EOPNOTSUPP;
> +		goto unlock_and_ret;
>  	}
>  
>  	if (!port->cap->pr_set) {
>  		dev_dbg(dev, "power role swapping not supported\n");
> -		return -EOPNOTSUPP;
> +		ret = -EOPNOTSUPP;
> +		goto unlock_and_ret;
> +	}
> +
Capabilities can be checked outside the lock.

> +	if (port->port_type != TYPEC_PORT_DRP) {
> +		dev_dbg(dev, "port type fixed at \"%s\"",
> +			     typec_port_types[port->port_type]);
> +		ret = -EOPNOTSUPP;
> +		goto unlock_and_ret;
>  	}
>  
>  	if (port->pwr_opmode != TYPEC_PWR_MODE_PD) {
>  		dev_dbg(dev, "partner unable to swap power role\n");
> -		return -EIO;
> +		ret = -EIO;
> +		goto unlock_and_ret;
>  	}
>  
>  	ret = sysfs_match_string(typec_roles, buf);
>  	if (ret < 0)
> -		return ret;
> +		goto unlock_and_ret;
>  
>  	ret = port->cap->pr_set(port->cap, ret);
>  	if (ret)
> -		return ret;
> +		goto unlock_and_ret;
>  
> -	return size;
> +	ret = size;
> +
> +unlock_and_ret:
> +	mutex_unlock(&port->port_type_lock);
> +	return ret;
>  }
>  
>  static ssize_t power_role_show(struct device *dev,
> @@ -926,6 +967,59 @@ static ssize_t power_role_show(struct device *dev,
>  }
>  static DEVICE_ATTR_RW(power_role);
>  
> +static ssize_t
> +port_type_store(struct device *dev, struct device_attribute *attr,
> +			const char *buf, size_t size)
> +{
> +	struct typec_port *port = to_typec_port(dev);
> +	int ret;
> +	enum typec_port_type type;
> +
> +	mutex_lock(&port->port_type_lock);
> +
> +	if (!port->cap->port_type_set || port->cap->type != TYPEC_PORT_DRP) {
> +		dev_dbg(dev, "changing port type not supported\n");
> +		ret = -EOPNOTSUPP;
> +		goto unlock_and_ret;
> +	}
> +
> +	ret = sysfs_match_string(typec_port_types, buf);
> +	if (ret < 0)
> +		goto unlock_and_ret;
> +
I would suggest to check as much as possible outside the lock.

> +	type = ret;
> +
> +	if (port->port_type == type) {
> +		ret = size;
> +		goto unlock_and_ret;
> +	}
> +
> +	ret = port->cap->port_type_set(port->cap, type);
> +	if (ret)
> +		goto unlock_and_ret;
> +
> +	port->port_type = type;
> +	ret = size;
> +
> +unlock_and_ret:
> +	mutex_unlock(&port->port_type_lock);
> +	return size;
> +}
> +
> +static ssize_t
> +port_type_show(struct device *dev, struct device_attribute *attr,
> +		char *buf)
> +{
> +	struct typec_port *port = to_typec_port(dev);
> +
> +	if (port->cap->type == TYPEC_PORT_DRP)
> +		return sprintf(buf, "%s\n",
> +			       typec_port_types_drp[port->port_type]);
> +
> +	return sprintf(buf, "[%s]\n", typec_port_types[port->cap->type]);
> +}
> +static DEVICE_ATTR_RW(port_type);
> +
>  static const char * const typec_pwr_opmodes[] = {
>  	[TYPEC_PWR_MODE_USB]	= "default",
>  	[TYPEC_PWR_MODE_1_5A]	= "1.5A",
> @@ -1035,6 +1129,7 @@ static struct attribute *typec_attrs[] = {
>  	&dev_attr_usb_power_delivery_revision.attr,
>  	&dev_attr_usb_typec_revision.attr,
>  	&dev_attr_vconn_source.attr,
> +	&dev_attr_port_type.attr,
>  	NULL,
>  };
>  ATTRIBUTE_GROUPS(typec);
> @@ -1211,6 +1306,8 @@ struct typec_port *typec_register_port(struct device *parent,
>  
>  	port->id = id;
>  	port->cap = cap;
> +	port->port_type = cap->type;
> +	mutex_init(&port->port_type_lock);
>  	port->prefer_role = cap->prefer_role;
>  
>  	port->dev.class = typec_class;
> diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
> index ec78204964ab..b174248af7c9 100644
> --- a/include/linux/usb/typec.h
> +++ b/include/linux/usb/typec.h
> @@ -190,6 +190,7 @@ struct typec_partner_desc {
>   * @pr_set: Set Power Role
>   * @vconn_set: Set VCONN Role
>   * @activate_mode: Enter/exit given Alternate Mode
> + * @port_type_set: Set port type
>   *
>   * Static capabilities of a single USB Type-C port.
>   */
> @@ -214,6 +215,9 @@ struct typec_capability {
>  
>  	int		(*activate_mode)(const struct typec_capability *,
>  					 int mode, int activate);
> +	int		(*port_type_set)(const struct typec_capability *,
> +					enum typec_port_type);
> +
>  };
>  
>  /* Specific to try_role(). Indicates the user want's to clear the preference. */
> -- 
> 2.13.0.219.gdb65acc882-goog
>