Re: [PATCH v2] usb: typec: Add a sysfs node to manage port type
From: Guenter Roeck
Date: Thu May 25 2017 - 03:49:16 EST
On 05/24/2017 08:10 PM, Badhri Jagan Sridharan wrote:
Thanks comments inline.
On Tue, May 23, 2017 at 7:38 PM, Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
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
?).
Yes.. I missed it while cleaning up the patch. Will add it to my next patch.
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 ?
I am checking against the current value of port_type variable.
Dont we want to reject role swaps if the port_type is not
TYPEC_PORT_DRP ? My understanding is that if the port type
is fixed at say PORT_TYPE_DFP by userspace, then unless
the userspace sets port_type back to PORT_TYPE_DRP,
role swap requests have to rejected. Is my understanding not
correct ?
Ah yes, the existing check is for port->cap->type. But why not just
replace that check with port->port_type ? Checking both seems overkill.
+ }
+
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.
similar to what I am doing for data_role_store function.
Not sure here. This is currently treated differently. A host-only
or device-only port was still considered supportable if it supports
power delivery.
+ }
+
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'.
Will fix in my next patch.
+ 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.
Will fix in my next patch.
+ 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.
Going to introduce a mutex port_type_lock which will protect the port_type
variable.
+
+ 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.
Sounds good to me.
+ 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. */