* [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).