Re: [PATCH v2] usb: typec: Add a sysfs node to manage port type

[Badhri Jagan Sridharan]

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 ?

>
>> +       }
>> +
>>         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. */
>>
>