| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <08597bc7-b447-ec8d-8331-3f59e0fa16ab@roeck-us.net>
Date: Thu, 25 May 2017 00:48:53 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Badhri Jagan Sridharan <badhri@...gle.com>
Cc: Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Oliver Neukum <oneukum@...e.com>,
USB <linux-usb@...r.kernel.org>,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] usb: typec: Add a sysfs node to manage port type
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@...ck-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@...gle.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@...ux.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. */
>>>
>>
>
Powered by blists - more mailing lists