Message-ID: <CAPTae5KMKN6w5rqDk7TO-_2sxN18Uh5FEV9=4eCSrNpH_Oz0xg@mail.gmail.com>
Date:   Thu, 25 May 2017 11:24:20 -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: [PATCH v2] usb: typec: Add a sysfs node to manage port type

On Thu, May 25, 2017 at 12:48 AM, Guenter Roeck <linux@...ck-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@...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.

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

Powered by blists - more mailing lists