usb: typec: Add a sysfs node to manage port type
Subsystems (2)
PATCH v2 1/1 usb: typec: Add a sysfs node to manage port type
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.
---
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(+)
Documentation/ABI/testing/sysfs-class-typec +13 -0
@@ -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 <[email protected]>
drivers/usb/typec/typec.c +66 -0
102 lines not on this page · raw message
include/linux/usb/typec.h +4 -0
@@ -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. */
Signature · 1 line
2.13.0.219.gdb65acc882-goog
Quoted 47 lines · Badhri Jagan Sridharan
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 <[email protected]>---
Changelog since v1:
- introduced a new variableport_typein structtypec_portto track the current port type instead of changing type member intypec_capabilityto address Heikki Krogerus comments.
- changed the output format and strings that would be displayed as suggested by Heikki Krogerus.Quoted 1 line
Documentation/ABI/testing/sysfs-class-typec | 13 ++++++drivers/usb/typec/typec.c | 66 +++++++++++++++++++++++++++++ include/linux/usb/typec.h | 4 ++ 3 files changed, 83 insertions(+)Documentation/ABI/testing/sysfs-class-typec +13
-0@@ -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 <[email protected]>drivers/usb/typec/typec.c +0
-0
drivers/usb/typec/typec.c struct typec_port { · line 72
Quoted 4 lines
drivers/usb/typec/typec.c +1
-0@@ -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 ?).
Quoted 17 lines
drivers/usb/typec/typec.c +6
-0const 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)
drivers/usb/typec/typec.c static ssize_t data_role_store(struct device *dev, · line 869
Quoted 7 lines
drivers/usb/typec/typec.c +4
-0@@ -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 ?
drivers/usb/typec/typec.c static ssize_t power_role_store(struct device *dev, · line 916
Quoted 13 lines
drivers/usb/typec/typec.c +6
-0+ } + 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.
Quoted 6 lines
drivers/usb/typec/typec.c +2
-0+ } +if (port->pwr_opmode !=TYPEC_PWR_MODE_PD ) {dev_dbg (dev,"partner unable to swap power role\n" );return -EIO ;
drivers/usb/typec/typec.c static ssize_t power_role_show(struct device *dev, · line 954
Quoted 10 lines
drivers/usb/typec/typec.c +7
-0@@ -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'.
drivers/usb/typec/typec.c static ssize_t power_role_show(struct device *dev, · line 963
Quoted 9 lines
drivers/usb/typec/typec.c +9
-0+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.
drivers/usb/typec/typec.c static ssize_t power_role_show(struct device *dev, · line 970
Quoted 7 lines
drivers/usb/typec/typec.c +7
-0+ 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.
Quoted 5 lines
drivers/usb/typec/typec.c +5
-0+ +return size; +} + +static ssize_t
drivers/usb/typec/typec.c static ssize_t power_role_show(struct device *dev, · line 985
Quoted 10 lines
drivers/usb/typec/typec.c +10
-0+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.
Quoted 42 lines
drivers/usb/typec/typec.c +9
-0+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);include/linux/usb/typec.h +4
-0@@ -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. */
Thanks comments inline.
Quoted 51 lines · Guenter Roeck
Quoted 50 lines · Badhri Jagan Sridharan
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 <[email protected]>---
Changelog since v1:
- introduced a new variableport_typein structtypec_portto track the current port type instead of changing type member intypec_capabilityto address Heikki Krogerus comments.
- changed the output format and strings that would be displayed as suggested by Heikki Krogerus.Quoted 1 line
Documentation/ABI/testing/sysfs-class-typec | 13 ++++++drivers/usb/typec/typec.c | 66 +++++++++++++++++++++++++++++ include/linux/usb/typec.h | 4 ++ 3 files changed, 83 insertions(+)Documentation/ABI/testing/sysfs-class-typec +3
-0@@ -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 usedfor
Documentation/ABI/testing/sysfs-class-typec +3
-0+ 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 hasfinished.
Documentation/ABI/testing/sysfs-class-typec +6
-0+ + Valid values: + - source + - sink + - dual + What: /sys/class/typec/<port>/supported_accessory_modes Date: April 2017 Contact: Heikki Krogerus <[email protected]>drivers/usb/typec/typec.c +0
-0@@ -69,6 +69,7 @@ struct typec_port {
Quoted 8 lines
drivers/usb/typec/typec.c struct typec_port { · line 72
Quoted 4 lines
drivers/usb/typec/typec.c +1
-0enum 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.
Quoted 17 lines
Quoted 17 lines
drivers/usb/typec/typec.c +5
-0const 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 ;
Quoted 8 lines
drivers/usb/typec/typec.c static ssize_t data_role_store(struct device *dev, · line 869
Quoted 5 lines
drivers/usb/typec/typec.c +3
-0} + 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 ?
Quoted 17 lines
drivers/usb/typec/typec.c static ssize_t power_role_store(struct device *dev, · line 915
Quoted 13 lines
drivers/usb/typec/typec.c +5
-0+ } + 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.
Quoted 11 lines
Quoted 11 lines
drivers/usb/typec/typec.c +3
-0+ } +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,
Quoted 8 lines
drivers/usb/typec/typec.c static ssize_t power_role_show(struct device *dev, · line 954
Quoted 5 lines
drivers/usb/typec/typec.c +5
-0+ 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.
Quoted 13 lines
drivers/usb/typec/typec.c static ssize_t power_role_show(struct device *dev, · line 963
Quoted 11 lines
drivers/usb/typec/typec.c +9
-0+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.
Quoted 16 lines
drivers/usb/typec/typec.c static ssize_t power_role_show(struct device *dev, · line 970
Quoted 8 lines
drivers/usb/typec/typec.c +7
-0+ 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 inport->port_typematches the actual port type (for example, if the code flow is interrupted beforeport_typeis set).
Quoted 5 lines
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_typeis in sync with the low level driver.
Going to introduce a mutex port_type_lock which will protect the port_type variable.
Quoted 25 lines
Quoted 6 lines
drivers/usb/typec/typec.c +5
-0+ +return size; +} + +static ssize_tdrivers/usb/typec/typec.c static ssize_t power_role_show(struct device *dev, · line 985
Quoted 10 lines
drivers/usb/typec/typec.c +10
-0+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 ofport->port_type,
or introduce an array with the three strings and use something like
Quoted 8 lines
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.
Quoted 44 lines
Quoted 43 lines
drivers/usb/typec/typec.c +9
-0+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);include/linux/usb/typec.h +4
-0@@ -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. */
Quoted 91 lines · Badhri Jagan Sridharan
Thanks comments inline.
Quoted 51 lines · Guenter Roeck
Quoted 50 lines · Badhri Jagan Sridharan
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 <[email protected]>---
Changelog since v1:
- introduced a new variableport_typein structtypec_portto track the current port type instead of changing type member intypec_capabilityto address Heikki Krogerus comments.
- changed the output format and strings that would be displayed as suggested by Heikki Krogerus.Quoted 1 line
Documentation/ABI/testing/sysfs-class-typec | 13 ++++++drivers/usb/typec/typec.c | 66 +++++++++++++++++++++++++++++ include/linux/usb/typec.h | 4 ++ 3 files changed, 83 insertions(+)Documentation/ABI/testing/sysfs-class-typec +3
-0@@ -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 usedfor
Documentation/ABI/testing/sysfs-class-typec +3
-0+ 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 hasfinished.
Documentation/ABI/testing/sysfs-class-typec +6
-0+ + Valid values: + - source + - sink + - dual + What: /sys/class/typec/<port>/supported_accessory_modes Date: April 2017 Contact: Heikki Krogerus <[email protected]>drivers/usb/typec/typec.c +0
-0@@ -69,6 +69,7 @@ struct typec_port {Quoted 8 lines
drivers/usb/typec/typec.c struct typec_port { · line 72
Quoted 4 lines
drivers/usb/typec/typec.c +1
-0enum 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.
Quoted 17 lines
Quoted 17 lines
drivers/usb/typec/typec.c +5
-0const 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 ;Quoted 8 lines
drivers/usb/typec/typec.c static ssize_t data_role_store(struct device *dev, · line 869
Quoted 5 lines
drivers/usb/typec/typec.c +3
-0} + 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 ?
Quoted 8 lines
I am checking against the current value of
port_typevariable. Dont we want to reject role swaps if theport_typeis notTYPEC_PORT_DRP? My understanding is that if the port type is fixed at sayPORT_TYPE_DFPby userspace, then unless the userspace setsport_typeback toPORT_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.
Quoted 18 lines
Quoted 17 lines
drivers/usb/typec/typec.c static ssize_t power_role_store(struct device *dev, · line 915
Quoted 13 lines
drivers/usb/typec/typec.c +5
-0+ } + 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.
Quoted 2 lines
similar to what I am doing for
data_role_storefunction.
Not sure here. This is currently treated differently. A host-only or device-only port was still considered supportable if it supports power delivery.
Quoted 144 lines
Quoted 11 lines
Quoted 11 lines
drivers/usb/typec/typec.c +3
-0+ } +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,Quoted 8 lines
drivers/usb/typec/typec.c static ssize_t power_role_show(struct device *dev, · line 954
Quoted 5 lines
drivers/usb/typec/typec.c +5
-0+ 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.
Quoted 13 lines
drivers/usb/typec/typec.c static ssize_t power_role_show(struct device *dev, · line 963
Quoted 11 lines
drivers/usb/typec/typec.c +9
-0+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.
Quoted 16 lines
drivers/usb/typec/typec.c static ssize_t power_role_show(struct device *dev, · line 970
Quoted 8 lines
drivers/usb/typec/typec.c +7
-0+ 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 inport->port_typematches the actual port type (for example, if the code flow is interrupted beforeport_typeis set).Quoted 5 lines
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_typeis in sync with the low level driver.Going to introduce a mutex
port_type_lockwhich will protect theport_typevariable.Quoted 25 lines
Quoted 6 lines
drivers/usb/typec/typec.c +5
-0+ +return size; +} + +static ssize_tdrivers/usb/typec/typec.c static ssize_t power_role_show(struct device *dev, · line 985
Quoted 10 lines
drivers/usb/typec/typec.c +10
-0+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 ofport->port_type,
or introduce an array with the three strings and use something likeQuoted 8 lines
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.
Quoted 44 lines
Quoted 43 lines
drivers/usb/typec/typec.c +9
-0+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);include/linux/usb/typec.h +4
-0@@ -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. */
Quoted 115 lines · Guenter Roeck
Quoted 105 lines · Badhri Jagan Sridharan
Thanks comments inline.
Quoted 59 lines · Guenter Roeck
Quoted 57 lines · Badhri Jagan Sridharan
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 <[email protected]>---
Changelog since v1:
- introduced a new variableport_typein structtypec_portto track the current port type instead of changing type member intypec_capabilityto address Heikki Krogerus comments.
- changed the output format and strings that would be displayed as suggested by Heikki Krogerus.Quoted 2 lines
Documentation/ABI/testing/sysfs-class-typec | 13 ++++++drivers/usb/typec/typec.c | 66 +++++++++++++++++++++++++++++ include/linux/usb/typec.h | 4 ++ 3 files changed, 83 insertions(+)Documentation/ABI/testing/sysfs-class-typec +3
-0@@ -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 beused
for
+ requesting a change in the port type. Port type change
isDocumentation/ABI/testing/sysfs-class-typec +2
-0+ supported as a synchronous operation, so write(2) to the + attribute will not return until the operation hasfinished.
Documentation/ABI/testing/sysfs-class-typec +6
-0+ + Valid values: + - source + - sink + - dual + What: /sys/class/typec/<port>/supported_accessory_modes Date: April 2017 Contact: Heikki Krogerus <[email protected]>drivers/usb/typec/typec.c +0
-0@@ -69,6 +69,7 @@ struct typec_port {enum typec_role pwr_role;enum typec_role vconn_role;Quoted 8 lines
drivers/usb/typec/typec.c struct typec_port { · line 72
Quoted 2 lines
drivers/usb/typec/typec.c +1
-0enum 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.
Quoted 19 lines
Quoted 19 lines
drivers/usb/typec/typec.c +5
-0const 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 ; }Quoted 8 lines
drivers/usb/typec/typec.c static ssize_t data_role_store(struct device *dev, · line 869
Quoted 4 lines
drivers/usb/typec/typec.c +3
-0+ 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 ?
Quoted 8 lines
I am checking against the current value of
port_typevariable. Dont we want to reject role swaps if theport_typeis notTYPEC_PORT_DRP? My understanding is that if the port type is fixed at sayPORT_TYPE_DFPby userspace, then unless the userspace setsport_typeback toPORT_TYPE_DRP, role swap requests have to rejected. Is my understanding not correct ?
Quoted 2 lines
Ah yes, the existing check is for
port->cap->type. But why not just replace that check withport->port_type? Checking both seems overkill.
Thanks. Sure will stick to just checking port->port_type.
Quoted 21 lines
Quoted 21 lines
Quoted 19 lines
Quoted 8 lines
drivers/usb/typec/typec.c +2
-0+ } + ret =sysfs_match_string (typec_data_roles, buf);if (ret <0 )return ret;drivers/usb/typec/typec.c static ssize_t power_role_store(struct device *dev, · line 915
Quoted 6 lines
drivers/usb/typec/typec.c +3
-0@@ -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.
Quoted 6 lines
Quoted 2 lines
similar to what I am doing for
data_role_storefunction.Not sure here. This is currently treated differently. A host-only or device-only port was still considered supportable if it supports power delivery.
Anh I see. Can we reject the role swap requests when the port_type is not set to TYPEC_PORT_DRP ? So when the port_type is:
source -> The port will behave as source only DFP.
sink -> The port will behave as sink only UFP.
drp -> dual-role-data and dual-role-power port.
Quoted 166 lines
Quoted 165 lines
Quoted 14 lines
Quoted 14 lines
drivers/usb/typec/typec.c +4
-0+ } +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)Quoted 8 lines
drivers/usb/typec/typec.c static ssize_t power_role_show(struct device *dev, · line 954
Quoted 4 lines
drivers/usb/typec/typec.c +4
-0+{ +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.
Quoted 15 lines
drivers/usb/typec/typec.c static ssize_t power_role_show(struct device *dev, · line 963
Quoted 11 lines
drivers/usb/typec/typec.c +9
-0+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.
Quoted 17 lines
drivers/usb/typec/typec.c static ssize_t power_role_show(struct device *dev, · line 970
Quoted 8 lines
drivers/usb/typec/typec.c +7
-0+ 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 inport->port_typematches the actual port type (for example, if the code flow is interrupted beforeport_typeis set).Quoted 7 lines
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 theport->port_typeis in sync with the low level driver.Going to introduce a mutex
port_type_lockwhich will protect theport_typevariable.Quoted 32 lines
Quoted 8 lines
drivers/usb/typec/typec.c +7
-0+ +return size; +} + +static ssize_t +port_type_show (struct device *dev,struct device_attribute *attr, +char *buf)drivers/usb/typec/typec.c static ssize_t power_role_show(struct device *dev,
Quoted 10 lines
drivers/usb/typec/typec.c +8
-0+{ +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 ofport->port_type,
or introduce an array with the three strings and use something likeconst char *something[] = {
[TYPEC_PORT_DRP] = "[dual] source sink",
...Quoted 6 lines
}; ... 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.
Quoted 48 lines
Quoted 47 lines
drivers/usb/typec/typec.c +2
-0+else + return sprintf(buf, "%s\n", "dual source[sink]");
drivers/usb/typec/typec.c +3
-0+ } + + return sprintf(buf, "[%s]\n",typec_port_types[port->cap->type]);
drivers/usb/typec/typec.c +4
-0+} +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);include/linux/usb/typec.h +4
-0@@ -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. */
Quoted 149 lines · Badhri Jagan Sridharan
Quoted 115 lines · Guenter Roeck
Quoted 105 lines · Badhri Jagan Sridharan
Thanks comments inline.
Quoted 59 lines · Guenter Roeck
Quoted 57 lines · Badhri Jagan Sridharan
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 <[email protected]>---
Changelog since v1:
- introduced a new variableport_typein structtypec_portto track the current port type instead of changing type member intypec_capabilityto address Heikki Krogerus comments.
- changed the output format and strings that would be displayed as suggested by Heikki Krogerus.Quoted 2 lines
Documentation/ABI/testing/sysfs-class-typec | 13 ++++++drivers/usb/typec/typec.c | 66 +++++++++++++++++++++++++++++ include/linux/usb/typec.h | 4 ++ 3 files changed, 83 insertions(+)Documentation/ABI/testing/sysfs-class-typec +3
-0@@ -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 beused
for
+ requesting a change in the port type. Port type change
isDocumentation/ABI/testing/sysfs-class-typec +2
-0+ supported as a synchronous operation, so write(2) to the + attribute will not return until the operation hasfinished.
Documentation/ABI/testing/sysfs-class-typec +6
-0+ + Valid values: + - source + - sink + - dual + What: /sys/class/typec/<port>/supported_accessory_modes Date: April 2017 Contact: Heikki Krogerus <[email protected]>drivers/usb/typec/typec.c +0
-0@@ -69,6 +69,7 @@ struct typec_port {enum typec_role pwr_role;enum typec_role vconn_role;Quoted 8 lines
drivers/usb/typec/typec.c struct typec_port { · line 72
Quoted 2 lines
drivers/usb/typec/typec.c +1
-0enum 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.
Quoted 19 lines
Quoted 19 lines
drivers/usb/typec/typec.c +5
-0const 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 ; }Quoted 8 lines
drivers/usb/typec/typec.c static ssize_t data_role_store(struct device *dev, · line 869
Quoted 4 lines
drivers/usb/typec/typec.c +3
-0+ 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 ?
Quoted 8 lines
I am checking against the current value of
port_typevariable. Dont we want to reject role swaps if theport_typeis notTYPEC_PORT_DRP? My understanding is that if the port type is fixed at sayPORT_TYPE_DFPby userspace, then unless the userspace setsport_typeback toPORT_TYPE_DRP, role swap requests have to rejected. Is my understanding not correct ?Quoted 2 lines
Ah yes, the existing check is for
port->cap->type. But why not just replace that check withport->port_type? Checking both seems overkill.Thanks. Sure will stick to just checking
port->port_type.Quoted 21 lines
Quoted 21 lines
Quoted 19 lines
Quoted 8 lines
drivers/usb/typec/typec.c +2
-0+ } + ret =sysfs_match_string (typec_data_roles, buf);if (ret <0 )return ret;drivers/usb/typec/typec.c static ssize_t power_role_store(struct device *dev, · line 915
Quoted 6 lines
drivers/usb/typec/typec.c +3
-0@@ -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.Quoted 6 lines
Quoted 2 lines
similar to what I am doing for
data_role_storefunction.Not sure here. This is currently treated differently. A host-only or device-only port was still considered supportable if it supports power delivery.
Quoted 6 lines
Anh I see. Can we reject the role swap requests when the
port_typeis not set toTYPEC_PORT_DRP? So when theport_typeis:
source -> The port will behave as source only DFP.
sink -> The port will behave as sink only UFP.
drp -> dual-role-data and dual-role-power port.
Makes sense to me, though I am not sure why this wasn't checked before.
Thanks,
Guenter
Quoted 167 lines
Quoted 167 lines
Quoted 166 lines
Quoted 15 lines
Quoted 15 lines
drivers/usb/typec/typec.c +4
-0+ } +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)Quoted 8 lines
drivers/usb/typec/typec.c static ssize_t power_role_show(struct device *dev, · line 954
Quoted 4 lines
drivers/usb/typec/typec.c +4
-0+{ +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.
Quoted 15 lines
drivers/usb/typec/typec.c static ssize_t power_role_show(struct device *dev, · line 963
Quoted 11 lines
drivers/usb/typec/typec.c +9
-0+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.
Quoted 17 lines
drivers/usb/typec/typec.c static ssize_t power_role_show(struct device *dev, · line 970
Quoted 8 lines
drivers/usb/typec/typec.c +7
-0+ 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 inport->port_typematches the actual port type (for example, if the code flow is interrupted beforeport_typeis set).Quoted 7 lines
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 theport->port_typeis in sync with the low level driver.Going to introduce a mutex
port_type_lockwhich will protect theport_typevariable.Quoted 32 lines
Quoted 8 lines
drivers/usb/typec/typec.c +7
-0+ +return size; +} + +static ssize_t +port_type_show (struct device *dev,struct device_attribute *attr, +char *buf)drivers/usb/typec/typec.c static ssize_t power_role_show(struct device *dev,
Quoted 10 lines
drivers/usb/typec/typec.c +8
-0+{ +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 ofport->port_type,
or introduce an array with the three strings and use something likeconst char *something[] = {
[TYPEC_PORT_DRP] = "[dual] source sink",
...Quoted 6 lines
}; ... 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.
Quoted 48 lines
Quoted 47 lines
drivers/usb/typec/typec.c +2
-0+else + return sprintf(buf, "%s\n", "dual source[sink]");
drivers/usb/typec/typec.c +3
-0+ } + + return sprintf(buf, "[%s]\n",typec_port_types[port->cap->type]);
drivers/usb/typec/typec.c +4
-0+} +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);include/linux/usb/typec.h +4
-0@@ -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. */