Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1650143 > unrolled thread
| Started by | Badhri Jagan Sridharan <badhri@google.com> |
|---|---|
| First post | 2017-05-25 05:20 +0200 |
| Last post | 2017-05-25 20:50 +0200 |
| Articles | 4 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH v2] usb: typec: Add a sysfs node to manage port type Badhri Jagan Sridharan <badhri@google.com> - 2017-05-25 05:20 +0200
Re: [PATCH v2] usb: typec: Add a sysfs node to manage port type Guenter Roeck <linux@roeck-us.net> - 2017-05-25 09:50 +0200
Re: [PATCH v2] usb: typec: Add a sysfs node to manage port type Badhri Jagan Sridharan <badhri@google.com> - 2017-05-25 20:30 +0200
Re: [PATCH v2] usb: typec: Add a sysfs node to manage port type Guenter Roeck <linux@roeck-us.net> - 2017-05-25 20:50 +0200
| From | Badhri Jagan Sridharan <badhri@google.com> |
|---|---|
| Date | 2017-05-25 05:20 +0200 |
| Subject | Re: [PATCH v2] usb: typec: Add a sysfs node to manage port type |
| Message-ID | <tKRtD-2IA-1@gated-at.bofh.it> |
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. */
>>
>
[toc] | [next] | [standalone]
| From | Guenter Roeck <linux@roeck-us.net> |
|---|---|
| Date | 2017-05-25 09:50 +0200 |
| Message-ID | <tKVGV-5jX-11@gated-at.bofh.it> |
| In reply to | #1650143 |
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. */
>>>
>>
>
[toc] | [prev] | [next] | [standalone]
| From | Badhri Jagan Sridharan <badhri@google.com> |
|---|---|
| Date | 2017-05-25 20:30 +0200 |
| Message-ID | <tL5Gi-3fL-23@gated-at.bofh.it> |
| In reply to | #1650261 |
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. */
>>>>
>>>
>>
>
[toc] | [prev] | [next] | [standalone]
| From | Guenter Roeck <linux@roeck-us.net> |
|---|---|
| Date | 2017-05-25 20:50 +0200 |
| Message-ID | <tL5ZE-3mc-7@gated-at.bofh.it> |
| In reply to | #1650721 |
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. */
> >>>>
> >>>
> >>
> >
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web