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@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> 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@xxxxxxxxxx>
> >>>> ---
> >>>> 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@xxxxxxxxxxxxxxx>
> >>>> 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. */
> >>>>
> >>>
> >>
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
