| 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
| ||
|
Date: Fri, 26 May 2017 13:37:30 -0700
From: Badhri Jagan Sridharan <badhri@...gle.com>
To: Guenter Roeck <linux@...ck-us.net>
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: [PATCHv3] usb: typec: Add a sysfs node to manage port type
On Fri, May 26, 2017 at 11:00 AM, Guenter Roeck <linux@...ck-us.net> wrote:
> On Fri, May 26, 2017 at 10:45:59AM -0700, 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>
>> Change-Id: Ia76877558c47271a34d912a54eea3459655dda3c
>> ---
>> 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.
>>
>> Changelog since v2:
>> - introduced a new mutex lock to protect port_type for addressing
>> the race conditions identified by Geunter Roeck
>> - added typec_port_types_drp for printing port_type when cap->type
>> is TYPE_PORT_DRP as suggested by Geunter Roeck
>> - Power role swap and data role swaps would be rejected unless
>> port port_type == TYPE_PORT_DRP
>> - port_type_store would return immediately if the current port_type
>> is same as the new port_type as suggested by Geunter Roeck
>>
>> drivers/usb/typec/typec.c | 127 ++++++++++++++++++++++++++++++++++++++++------
>> include/linux/usb/typec.h | 4 ++
>> 2 files changed, 116 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/usb/typec/typec.c b/drivers/usb/typec/typec.c
>> index 89e540bb7ff3..a0d8887ad560 100644
>> --- a/drivers/usb/typec/typec.c
>> +++ b/drivers/usb/typec/typec.c
>> @@ -11,9 +11,11 @@
>>
>> #include <linux/device.h>
>> #include <linux/module.h>
>> +#include <linux/mutex.h>
>> #include <linux/slab.h>
>> #include <linux/usb/typec.h>
>>
>> +
>
> Single empty lines only, please.
>
>> struct typec_mode {
>> int index;
>> u32 vdo;
>> @@ -69,6 +71,8 @@ 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;
>> + struct mutex port_type_lock;
>>
>> const struct typec_capability *cap;
>> };
>> @@ -789,6 +793,19 @@ 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 const char * const typec_port_types_drp[] = {
>> + [TYPEC_PORT_DFP] = "dual [source] sink",
>> + [TYPEC_PORT_UFP] = "dual source [sink]",
>> + [TYPEC_PORT_DRP] = "[dual] source sink",
>> +};
>> +
>> +
> Single empty line.
>
>> static ssize_t
>> preferred_role_store(struct device *dev, struct device_attribute *attr,
>> const char *buf, size_t size)
>> @@ -846,25 +863,34 @@ static ssize_t data_role_store(struct device *dev,
>> struct typec_port *port = to_typec_port(dev);
>> int ret;
>>
>> - if (port->cap->type != TYPEC_PORT_DRP) {
>> - dev_dbg(dev, "data role swap only supported with DRP ports\n");
>> - return -EOPNOTSUPP;
>> - }
>> + mutex_lock(&port->port_type_lock);
>>
>> if (!port->cap->dr_set) {
>> dev_dbg(dev, "data role swapping not supported\n");
>> - return -EOPNOTSUPP;
>> + ret = -EOPNOTSUPP;
>> + goto unlock_and_ret;
>> + }
>
> I think you can check this outside the lock; capabilities won't change.
>
>> +
>> + if (port->port_type != TYPEC_PORT_DRP) {
>> + dev_dbg(dev, "port type fixed at \"%s\"",
>> + typec_port_types[port->port_type]);
>> + ret = -EOPNOTSUPP;
>> + goto unlock_and_ret;
>> }
>>
>> ret = sysfs_match_string(typec_data_roles, buf);
>> if (ret < 0)
>> - return ret;
>> + goto unlock_and_ret;
>
> Move outside the lock ?
>
>>
>> ret = port->cap->dr_set(port->cap, ret);
>> if (ret)
>> - return ret;
>> + goto unlock_and_ret;
>>
>> - return size;
>> + ret = size;
>> +
>> +unlock_and_ret:
>> + mutex_unlock(&port->port_type_lock);
>> + return ret;
>> }
>>
>> static ssize_t data_role_show(struct device *dev,
>> @@ -885,32 +911,47 @@ static ssize_t power_role_store(struct device *dev,
>> const char *buf, size_t size)
>> {
>> struct typec_port *port = to_typec_port(dev);
>> - int ret = size;
>> + int ret;
>>
>> + mutex_lock(&port->port_type_lock);
>> if (!port->cap->pd_revision) {
>> dev_dbg(dev, "USB Power Delivery not supported\n");
>> - return -EOPNOTSUPP;
>> + ret = -EOPNOTSUPP;
>> + goto unlock_and_ret;
>> }
>>
>> if (!port->cap->pr_set) {
>> dev_dbg(dev, "power role swapping not supported\n");
>> - return -EOPNOTSUPP;
>> + ret = -EOPNOTSUPP;
>> + goto unlock_and_ret;
>> + }
>> +
> Capabilities can be checked outside the lock.
>
>> + if (port->port_type != TYPEC_PORT_DRP) {
>> + dev_dbg(dev, "port type fixed at \"%s\"",
>> + typec_port_types[port->port_type]);
>> + ret = -EOPNOTSUPP;
>> + goto unlock_and_ret;
>> }
>>
>> if (port->pwr_opmode != TYPEC_PWR_MODE_PD) {
>> dev_dbg(dev, "partner unable to swap power role\n");
>> - return -EIO;
>> + ret = -EIO;
>> + goto unlock_and_ret;
>> }
>>
>> ret = sysfs_match_string(typec_roles, buf);
>> if (ret < 0)
>> - return ret;
>> + goto unlock_and_ret;
>>
>> ret = port->cap->pr_set(port->cap, ret);
>> if (ret)
>> - return ret;
>> + goto unlock_and_ret;
>>
>> - return size;
>> + ret = size;
>> +
>> +unlock_and_ret:
>> + mutex_unlock(&port->port_type_lock);
>> + return ret;
>> }
>>
>> static ssize_t power_role_show(struct device *dev,
>> @@ -926,6 +967,59 @@ 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;
>> + enum typec_port_type type;
>> +
>> + mutex_lock(&port->port_type_lock);
>> +
>> + if (!port->cap->port_type_set || port->cap->type != TYPEC_PORT_DRP) {
>> + dev_dbg(dev, "changing port type not supported\n");
>> + ret = -EOPNOTSUPP;
>> + goto unlock_and_ret;
>> + }
>> +
>> + ret = sysfs_match_string(typec_port_types, buf);
>> + if (ret < 0)
>> + goto unlock_and_ret;
>> +
> I would suggest to check as much as possible outside the lock.
Moving as much as possible outside the lock as suggested.
>
>> + type = ret;
>> +
>> + if (port->port_type == type) {
>> + ret = size;
>> + goto unlock_and_ret;
>> + }
>> +
>> + ret = port->cap->port_type_set(port->cap, type);
>> + if (ret)
>> + goto unlock_and_ret;
>> +
>> + port->port_type = type;
>> + ret = size;
>> +
>> +unlock_and_ret:
>> + mutex_unlock(&port->port_type_lock);
>> + 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)
>> + return sprintf(buf, "%s\n",
>> + typec_port_types_drp[port->port_type]);
>> +
>> + 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 +1129,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);
>> @@ -1211,6 +1306,8 @@ struct typec_port *typec_register_port(struct device *parent,
>>
>> port->id = id;
>> port->cap = cap;
>> + port->port_type = cap->type;
>> + mutex_init(&port->port_type_lock);
>> port->prefer_role = cap->prefer_role;
>>
>> port->dev.class = typec_class;
>> diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
>> index ec78204964ab..b174248af7c9 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
>>
Powered by blists - more mailing lists