linux-kernel - Re: [RFC PATCHv2] 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]

Message-ID: <20160603183927.GA12659@roeck-us.net>
Date:	Fri, 3 Jun 2016 11:39:27 -0700
From:	Guenter Roeck <linux@...ck-us.net>
To:	Heikki Krogerus <heikki.krogerus@...ux.intel.com>
Cc:	Oliver Neukum <oneukum@...e.com>,
	Andy Shevchenko <andy.shevchenko@...il.com>,
	Rajaram R <rajaram.officemail@...il.com>,
	Felipe Balbi <felipe.balbi@...ux.intel.com>,
	Mathias Nyman <mathias.nyman@...ux.intel.com>,
	Greg KH <gregkh@...uxfoundation.org>,
	linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org
Subject: Re: [RFC PATCHv2] usb: USB Type-C Connector Class

On Fri, Jun 03, 2016 at 06:17:46PM +0300, Heikki Krogerus wrote:
[ ... ]
> > > > 
> > > > In my test case, this gives me
> > > >      /sys/class/type-c/usbc0/
> > > > 	usbc0.svid:18d1
> > > > 	usbc0.svid:18d1/mode0
> > > > 	usbc0.svid:18d1/mode0/vdo
> > > > 	usbc0.svid:18d1/mode0/description
> > > > 	usbc0.svid:18d1/mode0/active
> > > > 	...
> > > > 	usbc0.svid:ff01
> > > > 	usbc0.svid:ff01/mode0/vdo
> > > > 	usbc0.svid:ff01/mode0/description
> > > > 	usbc0.svid:ff01/mode0/active
> > 
> > Side note: I didn't provide a description/name for the modes, because that
> > would result in something like usbc0.DisplayPort/ instead of usbc0.svid:ff01/,
> > and I prefer a consistent ABI. Since this _is_ part of the ABI, would it make
> > sense to standardize on names for modes in sysfs ? For example, how should
> > a "Display Port" mode directory be named ? It doesn't sound good if I
> > use "usbc0.svid:ff01", someone else uses "usbc0.DisplayPort", and yet
> > someone else uses "usbc0.displayport".
> 
> Yeah, let's make them standard.
> 
Any name preferences ?

> > 
> > Also, do we at some point need to standardize the ABI for the standard
> > alternate modes such as DisplayPort (if there are any - again I am not
> > there yet) ?
> 
> I don't have an answer to that.
> 
Ok, I'll look into it as I proceed with my implementation.

> > 
> > Sounds good to me. Many other subsystems do the same, ie create the subsystem
> > device(s) during registration with the subsystem, so this is in line with other
> > kernel code.
> > 
> > Should I send you a follow-up patch on top of yours ?
> 
> Sure. I'm a little bit stuck with an other tasks, so let's keep this
> thing rolling.
> 
See below.

Thanks,
Guenter

---
>From ab1f2d0671e3cda74b80c6d17d99cb3e386c0d08 Mon Sep 17 00:00:00 2001
From: Guenter Roeck <groeck@...omium.org>
Date: Thu, 2 Jun 2016 10:09:50 -0700
Subject: [PATCH] usb: typec: Register supported alternate modes in
 typec_register_port()

By registering supported alternate modes when registering the port, we
automatically get the correct directory hierarchie in the class device.

Change-Id: I543da5f4ce922ded0532e6b0a0fdb8bc55cb5a80
Signed-off-by: Guenter Roeck <groeck@...omium.org>
---
 drivers/usb/type-c/typec.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/type-c/typec.c b/drivers/usb/type-c/typec.c
index 41ad955..22ee7eb 100644
--- a/drivers/usb/type-c/typec.c
+++ b/drivers/usb/type-c/typec.c
@@ -892,14 +892,22 @@ struct typec_port *typec_register_port(struct device *dev,
 	typec_init_roles(port);
 
 	ret = device_register(&port->dev);
-	if (ret) {
-		ida_simple_remove(&typec_index_ida, id);
-		put_device(&port->dev);
-		kfree(port);
-		return ERR_PTR(ret);
-	}
+	if (ret)
+		goto reg_err;
+
+	ret = typec_register_altmodes(&port->dev, cap->alt_modes);
+	if (ret)
+		goto alt_err;
 
 	return port;
+
+alt_err:
+	device_unregister(&port->dev);
+reg_err:
+	ida_simple_remove(&typec_index_ida, id);
+	put_device(&port->dev);
+	kfree(port);
+	return ERR_PTR(ret);
 }
 EXPORT_SYMBOL_GPL(typec_register_port);
 
@@ -908,6 +916,7 @@ void typec_unregister_port(struct typec_port *port)
 	if (port->connected)
 		typec_disconnect(port);
 
+	typec_unregister_altmodes(port->cap->alt_modes);
 	device_unregister(&port->dev);
 }
 EXPORT_SYMBOL_GPL(typec_unregister_port);
-- 
2.1.2