linux-kernel - Re: [PATCH] usb: typec: Add sysfs node to show cc orientation

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20191016125846.GC17542@kuha.fi.intel.com>
Date:   Wed, 16 Oct 2019 15:58:46 +0300
From:   Heikki Krogerus <heikki.krogerus@...ux.intel.com>
To:     pumahsu <pumahsu@...gle.com>
Cc:     gregkh@...uxfoundation.org, badhri@...gle.com, kyletso@...gle.com,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] usb: typec: Add sysfs node to show cc orientation

On Wed, Oct 16, 2019 at 11:43:14AM +0800, pumahsu wrote:
> Export the Type-C cc orientation so that user space can
> get this information.

For what do you need this information in user space? I'm guessing you
have something else in mind besides exposing this as just generic
information, or debugging purposes, no?

Please keep in mind that we do not always know the cable orientation.
UCSI for example does not give any clues about which way the cable
plug was connected to the connector. That means this sysfs file will
most likely need to be hidden in those cases, which I guess is
acceptable, but definitely not ideal.

> Signed-off-by: pumahsu <pumahsu@...gle.com>
> ---
>  Documentation/ABI/testing/sysfs-class-typec |  7 +++++++
>  drivers/usb/typec/class.c                   | 11 +++++++++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec
> index d7647b258c3c..419f952c991d 100644
> --- a/Documentation/ABI/testing/sysfs-class-typec
> +++ b/Documentation/ABI/testing/sysfs-class-typec
> @@ -108,6 +108,13 @@ Contact:	Heikki Krogerus <heikki.krogerus@...ux.intel.com>
>  Description:
>  		Revision number of the supported USB Type-C specification.
>  
> +What:		/sys/class/typec/<port>/cc_orientation
> +Date:		September 2019
> +Contact:	Puma Hsu <pumahsu@...gle.com>
> +Description:
> +		Indicates which cc orientation is active now, or 0 when
> +		nothing is connected.

cc_orientation is a bit cryptic. I think if this is part of the port
ABI, then we should talk about something like "connector_orientation".

>  USB Type-C partner devices (eg. /sys/class/typec/port0-partner/)
>  
> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> index 7d8805d9bf37..00edae63da8e 100644
> --- a/drivers/usb/typec/class.c
> +++ b/drivers/usb/typec/class.c
> @@ -1238,6 +1238,16 @@ static ssize_t usb_power_delivery_revision_show(struct device *dev,
>  }
>  static DEVICE_ATTR_RO(usb_power_delivery_revision);
>  
> +static ssize_t cc_orientation_show(struct device *dev,
> +						struct device_attribute *attr,
> +						char *buf)
> +{
> +	struct typec_port *p = to_typec_port(dev);
> +
> +	return sprintf(buf, "%d\n", typec_get_orientation(p));
> +}
> +static DEVICE_ATTR_RO(cc_orientation);

Now you are returning 0, 1 or 2 which to me is not ideal. This really
should return a string, something like "normal" / "reversed", and in
case the orientation is TYPEC_ORIENTATION_NONE, empty string.

>  static struct attribute *typec_attrs[] = {
>  	&dev_attr_data_role.attr,
>  	&dev_attr_power_operation_mode.attr,
> @@ -1248,6 +1258,7 @@ static struct attribute *typec_attrs[] = {
>  	&dev_attr_usb_typec_revision.attr,
>  	&dev_attr_vconn_source.attr,
>  	&dev_attr_port_type.attr,
> +	&dev_attr_cc_orientation.attr,
>  	NULL,
>  };
>  ATTRIBUTE_GROUPS(typec);
> -- 
> 2.23.0.700.g56cf767bdb-goog

thanks,

-- 
heikki