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

[Guenter Roeck]

On 05/23/2017 06:28 PM, 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@xxxxxxxxxx>
> ---
> 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.
> >   Documentation/ABI/testing/sysfs-class-typec | 13 ++++++
>   drivers/usb/typec/typec.c                   | 66 +++++++++++++++++++++++++++++
>   include/linux/usb/typec.h                   |  4 ++
>   3 files changed, 83 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec
> index d4a3d23eb09c..1f224c2e391f 100644
> --- a/Documentation/ABI/testing/sysfs-class-typec
> +++ b/Documentation/ABI/testing/sysfs-class-typec
> @@ -73,6 +73,19 @@ Description:
>   
>   		Valid values: source, sink, none (to remove preference)
>   
> +What:           /sys/class/typec/<port>/port_type
> +Date:           May 2017
> +Description:
> +		Indicates the type of the port. This attribute can be used for
> +		requesting a change in the port type. Port type change is
> +		supported as a synchronous operation, so write(2) to the
> +		attribute will not return until the operation has finished.
> +
> +		Valid values:
> +		- source
> +		- sink
> +		- dual
> +
>   What:		/sys/class/typec/<port>/supported_accessory_modes
>   Date:		April 2017
>   Contact:	Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> diff --git a/drivers/usb/typec/typec.c b/drivers/usb/typec/typec.c
> index 89e540bb7ff3..5063d6e0f8c7 100644
> --- a/drivers/usb/typec/typec.c
> +++ b/drivers/usb/typec/typec.c
> @@ -69,6 +69,7 @@ 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;

I am missing where this variable is initialized (when the port is registered ?).

>   
>   	const struct typec_capability	*cap;
>   };
> @@ -789,6 +790,12 @@ 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 ssize_t
>   preferred_role_store(struct device *dev, struct device_attribute *attr,
>   		     const char *buf, size_t size)
> @@ -856,6 +863,12 @@ static ssize_t data_role_store(struct device *dev,
>   		return -EOPNOTSUPP;
>   	}
>   
> +	if (port->port_type != TYPEC_PORT_DRP) {
> +		dev_dbg(dev, "port type fixed at \"%s\"",
> +			     typec_port_types[port->port_type]);
> +		return -EIO;

?? This is already there, or am I missing something ?

> +	}
> +
>   	ret = sysfs_match_string(typec_data_roles, buf);
>   	if (ret < 0)
>   		return ret;
> @@ -897,6 +910,12 @@ static ssize_t power_role_store(struct device *dev,
>   		return -EOPNOTSUPP;
>   	}
>   
> +	if (port->port_type != TYPEC_PORT_DRP) {
> +		dev_dbg(dev, "port type fixed at \"%s\"",
> +			     typec_port_types[port->port_type]);
> +		return -EIO;

Unrelated change; should be in a separate patch. Also, it should
probably return -EOPNOTSUPP.

> +	}
> +
>   	if (port->pwr_opmode != TYPEC_PWR_MODE_PD) {
>   		dev_dbg(dev, "partner unable to swap power role\n");
>   		return -EIO;
> @@ -926,6 +945,52 @@ 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, type;
> +

I think 'type' should be 'enum typec_port_type'.

> +	if (!port->cap->port_type_set || port->cap->type != TYPEC_PORT_DRP) {
> +		dev_dbg(dev, "changing port type not supported\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	ret = sysfs_match_string(typec_port_types, buf);
> +	if (ret < 0)
> +		return ret;
> +
If the new type matches the current type, you could return immediately here.

> +	type = ret;
> +
> +	ret = port->cap->port_type_set(port->cap, type);
> +	if (ret)
> +		return ret;
> +
> +	port->port_type = type;

We have no locking here, meaning a second request could be processed in parallel.
There is no guarantee that the resulting value in port->port_type matches
the actual port type (for example, if the code flow is interrupted before
port_type is set).

For other functions we have a callback from the driver, and the driver is
responsible for all locking. That doesn't work here, and a callback from
the driver to update the port type does not seem necessary (the port type,
unlike roles, does not change by itself). That means you'll need locking
to make sure that the port->port_type is in sync with the low level driver.

> +
> +	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) {
> +		if (port->port_type == TYPEC_PORT_DRP)
> +			return sprintf(buf, "%s\n", "[dual] source sink");
> +		else if (port->port_type == TYPEC_PORT_DFP)
> +			return sprintf(buf, "%s\n", "dual [source] sink");

Hmm.. I think this is another race condition. The port type could change from
DFP to DRP after the variable was read the first time, and we would display
it as sink. You would need a mutex to protect against changes of port->port_type,
or introduce an array with the three strings and use something like

const char *something[] = {
	[TYPEC_PORT_DRP] = "[dual] source sink",
	...
};
	...
		return sprintf(buf, ""%s\n", something[port->port_type]);

which would be less code. After all, the strings are needed anyway.

> +		else
> +			return sprintf(buf, "%s\n", "dual source [sink]");
> +	}
> +
> +	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 +1100,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);
> diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
> index ec78204964ab..5badf6f66479 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. */