Message-ID: <20201118174821.GA1966168@google.com>
Date:   Wed, 18 Nov 2020 09:48:21 -0800
From:   Benson Leung <bleung@...gle.com>
To:     Heikki Krogerus <heikki.krogerus@...ux.intel.com>
Cc:     Prashant Malani <pmalani@...omium.org>,
        Benson Leung <bleung@...omium.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org
Subject: Re: [RFC PATCH 2/3] usb: typec: Add product_type sysfs attribute
 file for partners and cables

Hi Heikki,

On Wed, Nov 18, 2020 at 06:00:58PM +0300, Heikki Krogerus wrote:
> USB Power Delivery Specification defines a set of product
> types for partners and cables. The product type is defined
> in the ID Header VDO, which is the first object in the
> response to the Discover Identity command.
> 
> This sysfs attribute file is only created for the partners
> and cables if the product type is really known in the
> driver. Some interfaces do not give access to the Discover
> Identity response from the partner or cable, but they may
> still supply the product type separately in some cases.
> 
> When the product type of the partner or cable is detected,
> uevent is also raised with PRODUCT_TYPE set to show the
> actual product type (for example PRODUCT_TYPE=host).
> 
> Signed-off-by: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> ---
>  Documentation/ABI/testing/sysfs-class-typec |  55 ++++++++
>  drivers/usb/typec/class.c                   | 132 ++++++++++++++++++--
>  2 files changed, 180 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec
> index b7794e02ad205..4c09e327c62be 100644
> --- a/Documentation/ABI/testing/sysfs-class-typec
> +++ b/Documentation/ABI/testing/sysfs-class-typec
> @@ -139,6 +139,42 @@ Description:
>  		Shows if the partner supports USB Power Delivery communication:
>  		Valid values: yes, no
>  
> +What:		/sys/class/typec/<port>-partner/product_type
> +Date:		December 2020
> +Contact:	Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> +Description:	USB Power Delivery Specification defines a set of product types
> +		for the partner devices. This file will show the product type of
> +		the partner if it is known. Dual-role capable partners will have
> +		both UFP and DFP product types defined, but only one that
> +		matches the current role will be active at the time. If the
> +		product type of the partner is not visible to the device driver,
> +		this file will not exist.
> +
> +		When the partner product type is detected, or changed with role
> +		swap, uvevent is also raised that contains PRODUCT_TYPE=<product
> +		type> (for example PRODUCT_TYPE=hub).
> +
> +		Valid values:
> +
> +		UFP / device role
> +		========================  ==========================
> +		undefined		  -
> +		hub			  PDUSB Hub
> +		peripheral		  PDUSB Peripheral
> +		psd			  Power Bank
> +		ama			  Alternate Mode Adapter
> +		vpd			  VCONN Powered USB Device

I have it on good authority that "vpd" is incorrectly categorized here,
and for future proofing, we'd better not introduce vpd as a product
type for UFP...

A vpd is actually more closely related to a "cable" than it is a "UFP."
A closer reading of the USB Type-C and USB PD specs will reveal that
VPDs can only ever appear as SOP' and not as SOP, so having its type
appear under UFP is a mistake.

In other words, the USB PD V3.0 R2.0 spec is wrong. A change has been
working its way through the spec committee to fix this, but it is not yet
published.

In order to reduce the amount of churn, I would recommend not
including vpd as a possible type until a new version of the spec (or the ECN)
is published.

Thanks,
Benson

> +		========================  ==========================
> +
> +		DFP / host role
> +		========================  ==========================
> +		undefined		  -
> +		hub			  PDUSB Hub
> +		host			  PDUSB Host
> +		power_brick		  Power Brick
> +		amc			  Alternate Mode Controller
> +		========================  ==========================
> +
>  What:		/sys/class/typec/<port>-partner>/identity/
>  Date:		April 2017
>  Contact:	Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> @@ -202,6 +238,25 @@ Description:
>  		- type-c
>  		- captive
>  
> +What:		/sys/class/typec/<port>-cable/product_type
> +Date:		December 2020
> +Contact:	Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> +Description:	USB Power Delivery Specification defines a set of product types
> +		for the cables. This file will show the product type of the
> +		cable if it is known. If the product type of the cable is not
> +		visible to the device driver, this file will not exist.
> +
> +		When the cable product type is detected, uvevent is also raised
> +		with PRODUCT_TYPE showing the product type of the cable.
> +
> +		Valid values:
> +
> +		========================  ==========================
> +		undefined		  -
> +		active			  Active Cable
> +		passive			  Passive Cable
> +		========================  ==========================
> +
>  What:		/sys/class/typec/<port>-cable/identity/
>  Date:		April 2017
>  Contact:	Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> index 35eec707cb512..303f054181ff7 100644
> --- a/drivers/usb/typec/class.c
> +++ b/drivers/usb/typec/class.c
> @@ -11,6 +11,7 @@
>  #include <linux/mutex.h>
>  #include <linux/property.h>
>  #include <linux/slab.h>
> +#include <linux/usb/pd_vdo.h>
>  
>  #include "bus.h"
>  
> @@ -81,6 +82,30 @@ static const char * const typec_accessory_modes[] = {
>  	[TYPEC_ACCESSORY_DEBUG]		= "debug",
>  };
>  
> +/* Product types defined in USB PD Specification R3.0 V2.0 */
> +static const char * const product_type_ufp[8] = {
> +	[IDH_PTYPE_UNDEF]		= "undefined",
> +	[IDH_PTYPE_HUB]			= "hub",
> +	[IDH_PTYPE_PERIPH]		= "peripheral",
> +	[IDH_PTYPE_PSD]			= "psd",
> +	[IDH_PTYPE_AMA]			= "ama",
> +	[IDH_PTYPE_VPD]			= "vpd",
> +};
> +
> +static const char * const product_type_dfp[8] = {
> +	[IDH_PTYPE_DFP_UNDEF]		= "undefined",
> +	[IDH_PTYPE_DFP_HUB]		= "hub",
> +	[IDH_PTYPE_DFP_HOST]		= "host",
> +	[IDH_PTYPE_DFP_PB]		= "power_brick",
> +	[IDH_PTYPE_DFP_AMC]		= "amc",
> +};
> +
> +static const char * const product_type_cable[8] = {
> +	[IDH_PTYPE_UNDEF]		= "undefined",
> +	[IDH_PTYPE_PCABLE]		= "passive",
> +	[IDH_PTYPE_ACABLE]		= "active",
> +};
> +
>  static struct usb_pd_identity *get_pd_identity(struct device *dev)
>  {
>  	if (is_typec_partner(dev)) {
> @@ -95,6 +120,24 @@ static struct usb_pd_identity *get_pd_identity(struct device *dev)
>  	return NULL;
>  }
>  
> +static const char *get_pd_product_type(struct device *dev)
> +{
> +	struct typec_port *port = to_typec_port(dev->parent);
> +	struct usb_pd_identity *id = get_pd_identity(dev);
> +	const char *ptype = NULL;
> +
> +	if (is_typec_partner(dev)) {
> +		if (port->data_role == TYPEC_HOST)
> +			ptype = product_type_ufp[PD_IDH_PTYPE(id->id_header)];
> +		else
> +			ptype = product_type_dfp[PD_IDH_DFP_PTYPE(id->id_header)];
> +	} else if (is_typec_cable(dev)) {
> +		ptype = product_type_cable[PD_IDH_PTYPE(id->id_header)];
> +	}
> +
> +	return ptype;
> +}
> +
>  static ssize_t id_header_show(struct device *dev, struct device_attribute *attr,
>  			      char *buf)
>  {
> @@ -139,11 +182,55 @@ static const struct attribute_group *usb_pd_id_groups[] = {
>  	NULL,
>  };
>  
> +static void typec_product_type_notify(struct device *dev)
> +{
> +	const char *ptype;
> +	char *envp[2];
> +
> +	ptype = get_pd_product_type(dev);
> +	if (!ptype)
> +		return;
> +
> +	sysfs_notify(&dev->kobj, NULL, "product_type");
> +
> +	envp[0] = kasprintf(GFP_KERNEL, "PRODUCT_TYPE=%s", ptype);
> +	if (!envp[0])
> +		return;
> +
> +	envp[1] = NULL;
> +	kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);
> +	kfree(envp[0]);
> +}
> +
>  static void typec_report_identity(struct device *dev)
>  {
>  	sysfs_notify(&dev->kobj, "identity", "id_header");
>  	sysfs_notify(&dev->kobj, "identity", "cert_stat");
>  	sysfs_notify(&dev->kobj, "identity", "product");
> +	typec_product_type_notify(dev);
> +}
> +
> +static ssize_t
> +product_type_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	const char *ptype;
> +
> +	ptype = get_pd_product_type(dev);
> +	if (!ptype)
> +		return 0;
> +
> +	return sysfs_emit(buf, "%s\n", ptype);
> +}
> +static DEVICE_ATTR_RO(product_type);
> +
> +static umode_t typec_product_type_attr_is_visible(struct kobject *kobj,
> +						  struct attribute *attr, int n)
> +{
> +	if (attr == &dev_attr_product_type.attr)
> +		if (!get_pd_identity(kobj_to_dev(kobj)))
> +			return 0;
> +
> +	return attr->mode;
>  }
>  
>  /* ------------------------------------------------------------------------- */
> @@ -534,10 +621,20 @@ static DEVICE_ATTR_RO(supports_usb_power_delivery);
>  
>  static struct attribute *typec_partner_attrs[] = {
>  	&dev_attr_accessory_mode.attr,
> +	&dev_attr_product_type.attr,
>  	&dev_attr_supports_usb_power_delivery.attr,
>  	NULL
>  };
> -ATTRIBUTE_GROUPS(typec_partner);
> +
> +static struct attribute_group typec_partner_group = {
> +	.is_visible = typec_product_type_attr_is_visible,
> +	.attrs = typec_partner_attrs,
> +};
> +
> +static const struct attribute_group *typec_partner_groups[] = {
> +	&typec_partner_group,
> +	NULL
> +};
>  
>  static void typec_partner_release(struct device *dev)
>  {
> @@ -773,9 +870,19 @@ static DEVICE_ATTR_RO(plug_type);
>  static struct attribute *typec_cable_attrs[] = {
>  	&dev_attr_type.attr,
>  	&dev_attr_plug_type.attr,
> +	&dev_attr_product_type.attr,
> +	NULL
> +};
> +
> +static struct attribute_group typec_cable_group = {
> +	.is_visible = typec_product_type_attr_is_visible,
> +	.attrs = typec_cable_attrs,
> +};
> +
> +static const struct attribute_group *typec_cable_groups[] = {
> +	&typec_cable_group,
>  	NULL
>  };
> -ATTRIBUTE_GROUPS(typec_cable);
>  
>  static void typec_cable_release(struct device *dev)
>  {
> @@ -1352,6 +1459,11 @@ const struct device_type typec_port_dev_type = {
>  /* --------------------------------------- */
>  /* Driver callbacks to report role updates */
>  
> +static int partner_match(struct device *dev, void *data)
> +{
> +	return is_typec_partner(dev);
> +}
> +
>  /**
>   * typec_set_data_role - Report data role change
>   * @port: The USB Type-C Port where the role was changed
> @@ -1361,12 +1473,23 @@ const struct device_type typec_port_dev_type = {
>   */
>  void typec_set_data_role(struct typec_port *port, enum typec_data_role role)
>  {
> +	struct device *partner_dev;
> +
>  	if (port->data_role == role)
>  		return;
>  
>  	port->data_role = role;
>  	sysfs_notify(&port->dev.kobj, NULL, "data_role");
>  	kobject_uevent(&port->dev.kobj, KOBJ_CHANGE);
> +
> +	partner_dev = device_find_child(&port->dev, NULL, partner_match);
> +	if (!partner_dev)
> +		return;
> +
> +	if (to_typec_partner(partner_dev)->identity)
> +		typec_product_type_notify(partner_dev);
> +
> +	put_device(partner_dev);
>  }
>  EXPORT_SYMBOL_GPL(typec_set_data_role);
>  
> @@ -1407,11 +1530,6 @@ void typec_set_vconn_role(struct typec_port *port, enum typec_role role)
>  }
>  EXPORT_SYMBOL_GPL(typec_set_vconn_role);
>  
> -static int partner_match(struct device *dev, void *data)
> -{
> -	return is_typec_partner(dev);
> -}
> -
>  /**
>   * typec_set_pwr_opmode - Report changed power operation mode
>   * @port: The USB Type-C Port where the mode was changed
> -- 
> 2.29.2
> 

-- 
Benson Leung
Staff Software Engineer
Chrome OS Kernel
Google Inc.
bleung@...gle.com
Chromium OS Project
bleung@...omium.org

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists