* [PATCH v2] usb: typec: Add a sysfs node to manage port type
@ 2017-05-24 1:28 Badhri Jagan Sridharan
2017-05-24 2:38 ` Guenter Roeck
0 siblings, 1 reply; 6+ messages in thread
From: Badhri Jagan Sridharan @ 2017-05-24 1:28 UTC (permalink / raw)
To: Heikki Krogerus, Greg Kroah-Hartman, Guenter Roeck, Oliver Neukum
Cc: linux-usb, linux-kernel, 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 <Badhri@google.com>
---
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@linux.intel.com>
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;
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;
+ }
+
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;
+ }
+
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;
+
+ 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;
+
+ type = ret;
+
+ ret = port->cap->port_type_set(port->cap, type);
+ if (ret)
+ return ret;
+
+ port->port_type = type;
+
+ 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");
+ 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. */
--
2.13.0.219.gdb65acc882-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] usb: typec: Add a sysfs node to manage port type
2017-05-24 1:28 [PATCH v2] usb: typec: Add a sysfs node to manage port type Badhri Jagan Sridharan
@ 2017-05-24 2:38 ` Guenter Roeck
2017-05-25 3:10 ` Badhri Jagan Sridharan
0 siblings, 1 reply; 6+ messages in thread
From: Guenter Roeck @ 2017-05-24 2:38 UTC (permalink / raw)
To: Badhri Jagan Sridharan, Heikki Krogerus, Greg Kroah-Hartman,
Oliver Neukum
Cc: linux-usb, linux-kernel
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@google.com>
> ---
> 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@linux.intel.com>
> 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. */
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] usb: typec: Add a sysfs node to manage port type
2017-05-24 2:38 ` Guenter Roeck
@ 2017-05-25 3:10 ` Badhri Jagan Sridharan
2017-05-25 7:48 ` Guenter Roeck
0 siblings, 1 reply; 6+ messages in thread
From: Badhri Jagan Sridharan @ 2017-05-25 3:10 UTC (permalink / raw)
To: Guenter Roeck
Cc: Heikki Krogerus, Greg Kroah-Hartman, Oliver Neukum, USB, LKML
Thanks comments inline.
On Tue, May 23, 2017 at 7:38 PM, Guenter Roeck <linux@roeck-us.net> 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@google.com>
>> ---
>> 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@linux.intel.com>
>> 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 ?
>
>> + }
>> +
>> 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.
>
>> + }
>> +
>> 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. */
>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] usb: typec: Add a sysfs node to manage port type
2017-05-25 3:10 ` Badhri Jagan Sridharan
@ 2017-05-25 7:48 ` Guenter Roeck
2017-05-25 18:24 ` Badhri Jagan Sridharan
0 siblings, 1 reply; 6+ messages in thread
From: Guenter Roeck @ 2017-05-25 7:48 UTC (permalink / raw)
To: Badhri Jagan Sridharan
Cc: Heikki Krogerus, Greg Kroah-Hartman, Oliver Neukum, USB, LKML
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@roeck-us.net> 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@google.com>
>>> ---
>>> 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@linux.intel.com>
>>> 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. */
>>>
>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] usb: typec: Add a sysfs node to manage port type
2017-05-25 7:48 ` Guenter Roeck
@ 2017-05-25 18:24 ` Badhri Jagan Sridharan
2017-05-25 18:41 ` Guenter Roeck
0 siblings, 1 reply; 6+ messages in thread
From: Badhri Jagan Sridharan @ 2017-05-25 18:24 UTC (permalink / raw)
To: Guenter Roeck
Cc: Heikki Krogerus, Greg Kroah-Hartman, Oliver Neukum, USB, LKML
On Thu, May 25, 2017 at 12:48 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> 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@roeck-us.net> 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@google.com>
>>>> ---
>>>> 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@linux.intel.com>
>>>> 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.
Thanks. Sure will stick to just checking port->port_type.
>
>>>
>>>> + }
>>>> +
>>>> 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.
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.
>
>
>>>
>>>> + }
>>>> +
>>>> 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. */
>>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] usb: typec: Add a sysfs node to manage port type
2017-05-25 18:24 ` Badhri Jagan Sridharan
@ 2017-05-25 18:41 ` Guenter Roeck
0 siblings, 0 replies; 6+ messages in thread
From: Guenter Roeck @ 2017-05-25 18:41 UTC (permalink / raw)
To: Badhri Jagan Sridharan
Cc: Heikki Krogerus, Greg Kroah-Hartman, Oliver Neukum, USB, LKML
On Thu, May 25, 2017 at 11:24:20AM -0700, Badhri Jagan Sridharan wrote:
> On Thu, May 25, 2017 at 12:48 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> > 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@roeck-us.net> 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@google.com>
> >>>> ---
> >>>> 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@linux.intel.com>
> >>>> 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.
>
> Thanks. Sure will stick to just checking port->port_type.
>
> >
> >>>
> >>>> + }
> >>>> +
> >>>> 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.
>
> 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.
>
Makes sense to me, though I am not sure why this wasn't checked before.
Thanks,
Guenter
>
> >
> >
> >>>
> >>>> + }
> >>>> +
> >>>> 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. */
> >>>>
> >>>
> >>
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-05-25 18:41 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-24 1:28 [PATCH v2] usb: typec: Add a sysfs node to manage port type Badhri Jagan Sridharan
2017-05-24 2:38 ` Guenter Roeck
2017-05-25 3:10 ` Badhri Jagan Sridharan
2017-05-25 7:48 ` Guenter Roeck
2017-05-25 18:24 ` Badhri Jagan Sridharan
2017-05-25 18:41 ` Guenter Roeck
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).