linux-kernel - Re: [PATCH 1/3] usb: USB Type-C Connector Class

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]
Date:	Wed, 10 Feb 2016 11:49:02 +0100
From:	Oliver Neukum <oneukum@...e.com>
To:	Heikki Krogerus <heikki.krogerus@...ux.intel.com>
Cc:	Greg KH <gregkh@...uxfoundation.org>,
	Felipe Balbi <balbi@...nel.org>,
	Mathias Nyman <mathias.nyman@...ux.intel.com>,
	linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org
Subject: Re: [PATCH 1/3] usb: USB Type-C Connector Class

On Tue, 2016-02-09 at 19:01 +0200, Heikki Krogerus wrote:
> The purpose of this class is to provide unified interface
> for user space to get the status and basic information about
> USB Type-C Connectors in the system, control data role
> swapping, and when USB PD is available, also power role
> swapping and Altenate Modes.
> 
> The class will export the following interfaces for every
> USB Type-C Connector in the system to sysfs:
> 
> 1. connected - Connection status of the connector
> 2. alternate_mode - The current Alternate Mode
> 3. alternate_modes - Lists all Alternate Modes the connector supports

These names are a bit problematic, as they are too similar.
How about

current_alternate_mode
potential_alternate_modes

> 4. partner_alt_modes - Lists partner's Alternate Modes when connected
> 5. partner_type - Can be USB, Charger, Alt Mode or Accessory
> 6. data_role - The current data role, host or device
> 7. data_roles - Data roles supported by the connector
> 8. power_role - Connector's current power role, source or sink
> 9. power_roles - Power roles supported by the connector
> 10. power_operation_mode - The current power level in use
> 11. usb_pd - yes if the connector supports USB PD.
> 12. audio_accessory - yes if the connector supports Audio Accessory
> 13. debug_accessory - yes if the connector supports Debug Accessory
> 
> The data_role, power_role and alternate_mode are also
> writable and can be used for executing role swapping and
> entering modes. When USB PD is not supported by the
> connector or partner, power_role will reflect the value of
> the data_role, and is not swappable independently.
> 
> Signed-off-by: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> ---
>  drivers/usb/Kconfig         |   2 +
>  drivers/usb/Makefile        |   2 +
>  drivers/usb/type-c/Kconfig  |   7 +
>  drivers/usb/type-c/Makefile |   1 +
>  drivers/usb/type-c/typec.c  | 446 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/usb/typec.h   | 114 +++++++++++
>  6 files changed, 572 insertions(+)
>  create mode 100644 drivers/usb/type-c/Kconfig
>  create mode 100644 drivers/usb/type-c/Makefile
>  create mode 100644 drivers/usb/type-c/typec.c
>  create mode 100644 include/linux/usb/typec.h
> 
> diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig
> index 8ed451d..0c45547 100644
> --- a/drivers/usb/Kconfig
> +++ b/drivers/usb/Kconfig
> @@ -151,6 +151,8 @@ source "drivers/usb/phy/Kconfig"
>  
>  source "drivers/usb/gadget/Kconfig"
>  
> +source "drivers/usb/type-c/Kconfig"
> +
>  config USB_LED_TRIG
>  	bool "USB LED Triggers"
>  	depends on LEDS_CLASS && USB_COMMON && LEDS_TRIGGERS
> diff --git a/drivers/usb/Makefile b/drivers/usb/Makefile
> index d5c57f1..4d712ee 100644
> --- a/drivers/usb/Makefile
> +++ b/drivers/usb/Makefile
> @@ -61,3 +61,5 @@ obj-$(CONFIG_USB_GADGET)	+= gadget/
>  obj-$(CONFIG_USB_COMMON)	+= common/
>  
>  obj-$(CONFIG_USBIP_CORE)	+= usbip/
> +
> +obj-$(CONFIG_TYPEC)		+= type-c/
> diff --git a/drivers/usb/type-c/Kconfig b/drivers/usb/type-c/Kconfig
> new file mode 100644
> index 0000000..b229fb9
> --- /dev/null
> +++ b/drivers/usb/type-c/Kconfig
> @@ -0,0 +1,7 @@
> +
> +menu "USB PD and Type-C drivers"
> +
> +config TYPEC
> +	tristate
> +
> +endmenu
> diff --git a/drivers/usb/type-c/Makefile b/drivers/usb/type-c/Makefile
> new file mode 100644
> index 0000000..1012a8b
> --- /dev/null
> +++ b/drivers/usb/type-c/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_TYPEC)		+= typec.o
> diff --git a/drivers/usb/type-c/typec.c b/drivers/usb/type-c/typec.c
> new file mode 100644
> index 0000000..e425955
> --- /dev/null
> +++ b/drivers/usb/type-c/typec.c
> @@ -0,0 +1,446 @@
> +/*
> + * USB Type-C class
> + *
> + * Copyright (C) 2016, Intel Corporation
> + * Author: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/usb/typec.h>
> +
> +#define to_typec_port(p) container_of(p, struct typec_port, dev)
> +
> +static DEFINE_IDA(typec_index_ida);
> +
> +/* -------------------------------- */
> +
> +int typec_connect(struct typec_port *port)
> +{
> +	kobject_uevent(&port->dev.kobj, KOBJ_CHANGE);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(typec_connect);
> +
> +void typec_disconnect(struct typec_port *port)
> +{
> +	kobject_uevent(&port->dev.kobj, KOBJ_CHANGE);
> +}
> +EXPORT_SYMBOL_GPL(typec_disconnect);
> +
> +/* -------------------------------- */
> +
> +static ssize_t alternate_mode_store(struct device *dev,
> +				    struct device_attribute *attr,
> +				    const char *buf, size_t size)
> +{
> +	struct typec_port *port = to_typec_port(dev);
> +	struct typec_alt_mode alt_mode;
> +	int ret;
> +
> +	if (!port->cap->set_alt_mode) {
> +		dev_warn(dev, "entering Alternate Modes not supported\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (!port->connected)
> +		return -ENXIO;

Doesn't this need locking?
And why wouldn't user space want to preselect a mode?

	Regards
		Oliver