linux-kernel - Re: [PATCH] usb: typec: Add a sysfs node to manage port type

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <b307ac6b-e076-1e29-64a5-ce66b5a20578@roeck-us.net>
Date:   Tue, 23 May 2017 07:06:13 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     Heikki Krogerus <heikki.krogerus@...ux.intel.com>
Cc:     Badhri Jagan Sridharan <badhri@...gle.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        Oliver Neukum <oneukum@...e.com>
Subject: Re: [PATCH] usb: typec: Add a sysfs node to manage port type

On 05/23/2017 06:39 AM, Heikki Krogerus wrote:
> On Tue, May 23, 2017 at 06:16:28AM -0700, Guenter Roeck wrote:
>> On 05/23/2017 03:46 AM, Heikki Krogerus wrote:
>>> Hi,
>>>
>>> On Mon, May 22, 2017 at 01:05:42PM -0700, 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>
>>>> ---
>>>>    Documentation/ABI/testing/sysfs-class-typec | 13 ++++++++++
>>>>    drivers/usb/typec/typec.c                   | 40 +++++++++++++++++++++++++++++
>>>>    include/linux/usb/typec.h                   |  4 +++
>>>>    3 files changed, 57 insertions(+)
>>>>
>>>> diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec
>>>> index d4a3d23eb09c..853b2ef73641 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:
>>>> +		- DRP
>>>> +		- DFP
>>>> +		- UFP
>>>>
>>>>    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..684a13bb744d 100644
>>>> --- a/drivers/usb/typec/typec.c
>>>> +++ b/drivers/usb/typec/typec.c
>>>> @@ -789,6 +789,12 @@ static const char * const typec_data_roles[] = {
>>>>    	[TYPEC_HOST]	= "host",
>>>>    };
>>>> +static const char * const typec_port_types[] = {
>>>> +	[TYPEC_PORT_DFP] = "dfp",
>>>> +	[TYPEC_PORT_UFP] = "ufp",
>>>> +	[TYPEC_PORT_DRP] = "drp",
>>>> +};
>>>
>>> The meaning of those abbreviations has changed in every version of the
>>> spec since v1.0 which makes me a bit uncomfortable using them with the
>>> attributes. In USB Type-C specification v1.2, DRP now means
>>> Dual-Role-Power, but DFP and UFP are used with USB data operation.
>>>
>>> I would prefer "source, "sink" and "drp". Actually, I don't even like
>>> "drp". How about "dual" instead?
>>>
>>>>    static ssize_t
>>>>    preferred_role_store(struct device *dev, struct device_attribute *attr,
>>>>    		     const char *buf, size_t size)
>>>> @@ -926,6 +932,39 @@ 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;
>>>> +
>>>> +	if (!port->cap->port_type_set) {
>>>> +		dev_dbg(dev, "changing port type not supported\n");
>>>> +		return -EOPNOTSUPP;
>>>> +	}
>>>> +
>>>> +	ret = sysfs_match_string(typec_port_types, buf);
>>>> +	if (ret < 0)
>>>> +		return ret;
>>>> +
>>>> +	ret = port->cap->port_type_set(port->cap, ret);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	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);
>>>> +
>>>> +	return sprintf(buf, "%s\n", typec_port_types[port->cap->type]);
>>>> +}
>>>> +static DEVICE_ATTR_RW(port_type);
>>>
>>> This doesn't tell the user the capabilities of the port. All the
>>> supported roles should be listed here like with the other attributes,
>>> the active one in brackets. This probable means we need a small
>>> addition/change to the API too.
> 
> Sorry, not the API..
> 
>> typec_capability already lists the port type. Presumably it can be
>> restricted to TYPEC_PORT_DFP or TYPEC_PORT_UFP only if it is reported
>> as TYPEC_PORT_DRP. Am I missing something ?
> 
> I mean, we should not overwrite the type member in typec_capability.
> DRP capable port is still DRP capable even if we fix it to DFP or UFP.
> So the active, fixed role, should be stored to struct typec_port.
> 
Yes, you are right. Makes sense.

Thanks,
Guenter