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: 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