On Thu, May 25, 2017 at 12:48 AM, Guenter Roeck <li...@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 <li...@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 <bad...@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.kroge...@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. */
>>>>
>>>
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
