linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/6] usb: Linking ports to their Type-C connectors
@ 2021-03-25 12:29 Heikki Krogerus
  2021-03-25 12:29 ` [PATCH 1/6] usb: Iterator for ports Heikki Krogerus
                   ` (5 more replies)
  0 siblings, 6 replies; 14+ messages in thread
From: Heikki Krogerus @ 2021-03-25 12:29 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Benson Leung, Prashant Malani, Guenter Roeck, linux-usb, linux-kernel

Hi,

Adding a simple function typec_link_port() that can be used to create
a symlink "connector" that points to the USB Type-C connector of a
port. It is used with USB ports initially, but hopefully later also
with other things like DisplayPorts.

Being able to see which connector is connected to a port is important
in general, but it is really important when for example the data or
power role of a device needs to swapped. The user probable wants to
know which USB device is disconnected if role swap on a USB Type-C
connector is executed.

Hope these are OK.

thanks,

Heikki Krogerus (6):
  usb: Iterator for ports
  usb: typec: Organize the private headers properly
  usb: typec: Declare the typec_class static
  usb: typec: Port mapping utility
  usb: Link the ports to the connectors they are attached to
  usb: typec: Link all ports during connector registration

 Documentation/ABI/testing/sysfs-bus-usb |   9 +
 drivers/usb/core/port.c                 |   3 +
 drivers/usb/core/usb.c                  |  43 ++++
 drivers/usb/typec/Makefile              |   1 +
 drivers/usb/typec/bus.c                 |   2 +
 drivers/usb/typec/bus.h                 |  19 +-
 drivers/usb/typec/class.c               | 101 +++------
 drivers/usb/typec/class.h               |  94 ++++++++
 drivers/usb/typec/mux.c                 |   4 +-
 drivers/usb/typec/mux.h                 |  21 ++
 drivers/usb/typec/port-mapper.c         | 283 ++++++++++++++++++++++++
 include/linux/usb.h                     |   1 +
 include/linux/usb/typec.h               |  13 ++
 13 files changed, 499 insertions(+), 95 deletions(-)
 create mode 100644 drivers/usb/typec/class.h
 create mode 100644 drivers/usb/typec/mux.h
 create mode 100644 drivers/usb/typec/port-mapper.c

-- 
2.30.2


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 1/6] usb: Iterator for ports
  2021-03-25 12:29 [PATCH 0/6] usb: Linking ports to their Type-C connectors Heikki Krogerus
@ 2021-03-25 12:29 ` Heikki Krogerus
  2021-03-25 14:41   ` Alan Stern
  2021-03-25 12:29 ` [PATCH 2/6] usb: typec: Organize the private headers properly Heikki Krogerus
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 14+ messages in thread
From: Heikki Krogerus @ 2021-03-25 12:29 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Benson Leung, Prashant Malani, Guenter Roeck, linux-usb, linux-kernel

Introducing usb_for_each_port(). It works the same way as
usb_for_each_dev(), but instead of going through every USB
device in the system, it walks through the USB ports in the
system.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/core/usb.c | 43 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/usb.h    |  1 +
 2 files changed, 44 insertions(+)

diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index 2ce3667ec6fae..6d49db9a1b208 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -398,6 +398,49 @@ int usb_for_each_dev(void *data, int (*fn)(struct usb_device *, void *))
 }
 EXPORT_SYMBOL_GPL(usb_for_each_dev);
 
+struct each_hub_arg {
+	void *data;
+	int (*fn)(struct device *, void *);
+};
+
+static int __each_hub(struct device *dev, void *data)
+{
+	struct each_hub_arg *arg = (struct each_hub_arg *)data;
+	struct usb_device *hdev = to_usb_device(dev);
+	struct usb_hub *hub;
+	int ret;
+	int i;
+
+	hub = usb_hub_to_struct_hub(hdev);
+	if (!hub)
+		return 0;
+
+	for (i = 0; i < hdev->maxchild; i++) {
+		ret = arg->fn(&hub->ports[i]->dev, arg->data);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+/**
+ * usb_for_each_port - interate over all USB ports in the system
+ * @data: data pointer that will be handed to the callback function
+ * @fn: callback function to be called for each USB port
+ *
+ * Iterate over all USB ports and call @fn for each, passing it @data. If it
+ * returns anything other than 0, we break the iteration prematurely and return
+ * that value.
+ */
+int usb_for_each_port(void *data, int (*fn)(struct device *, void *))
+{
+	struct each_hub_arg arg = {data, fn};
+
+	return bus_for_each_dev(&usb_bus_type, NULL, &arg, __each_hub);
+}
+EXPORT_SYMBOL_GPL(usb_for_each_port);
+
 /**
  * usb_release_dev - free a usb device structure when all users of it are finished.
  * @dev: device that's been disconnected
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 57c1e0ce5eba4..06ed5779fb4d8 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -869,6 +869,7 @@ extern int usb_match_one_id(struct usb_interface *interface,
 			    const struct usb_device_id *id);
 
 extern int usb_for_each_dev(void *data, int (*fn)(struct usb_device *, void *));
+int usb_for_each_port(void *data, int (*fn)(struct device *, void *));
 extern struct usb_interface *usb_find_interface(struct usb_driver *drv,
 		int minor);
 extern struct usb_interface *usb_ifnum_to_if(const struct usb_device *dev,
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 2/6] usb: typec: Organize the private headers properly
  2021-03-25 12:29 [PATCH 0/6] usb: Linking ports to their Type-C connectors Heikki Krogerus
  2021-03-25 12:29 ` [PATCH 1/6] usb: Iterator for ports Heikki Krogerus
@ 2021-03-25 12:29 ` Heikki Krogerus
  2021-03-25 12:29 ` [PATCH 3/6] usb: typec: Declare the typec_class static Heikki Krogerus
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 14+ messages in thread
From: Heikki Krogerus @ 2021-03-25 12:29 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Benson Leung, Prashant Malani, Guenter Roeck, linux-usb, linux-kernel

Adding a header file for each subsystem - the connector
class, alt mode bus and the class for the muxes.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/bus.c   |  2 ++
 drivers/usb/typec/bus.h   | 19 +---------
 drivers/usb/typec/class.c | 69 +++--------------------------------
 drivers/usb/typec/class.h | 76 +++++++++++++++++++++++++++++++++++++++
 drivers/usb/typec/mux.c   |  4 +--
 drivers/usb/typec/mux.h   | 21 +++++++++++
 6 files changed, 107 insertions(+), 84 deletions(-)
 create mode 100644 drivers/usb/typec/class.h
 create mode 100644 drivers/usb/typec/mux.h

diff --git a/drivers/usb/typec/bus.c b/drivers/usb/typec/bus.c
index e8ddb81cb6df4..7f3c9a8e2bf08 100644
--- a/drivers/usb/typec/bus.c
+++ b/drivers/usb/typec/bus.c
@@ -9,6 +9,8 @@
 #include <linux/usb/pd_vdo.h>
 
 #include "bus.h"
+#include "class.h"
+#include "mux.h"
 
 static inline int
 typec_altmode_set_mux(struct altmode *alt, unsigned long conf, void *data)
diff --git a/drivers/usb/typec/bus.h b/drivers/usb/typec/bus.h
index 8ba8112d2740d..56dec268d4dd9 100644
--- a/drivers/usb/typec/bus.h
+++ b/drivers/usb/typec/bus.h
@@ -4,9 +4,9 @@
 #define __USB_TYPEC_ALTMODE_H__
 
 #include <linux/usb/typec_altmode.h>
-#include <linux/usb/typec_mux.h>
 
 struct bus_type;
+struct typec_mux;
 
 struct altmode {
 	unsigned int			id;
@@ -28,24 +28,7 @@ struct altmode {
 
 extern struct bus_type typec_bus;
 extern const struct device_type typec_altmode_dev_type;
-extern const struct device_type typec_port_dev_type;
 
 #define is_typec_altmode(_dev_) (_dev_->type == &typec_altmode_dev_type)
-#define is_typec_port(_dev_) (_dev_->type == &typec_port_dev_type)
-
-extern struct class typec_mux_class;
-
-struct typec_switch {
-	struct device dev;
-	typec_switch_set_fn_t set;
-};
-
-struct typec_mux {
-	struct device dev;
-	typec_mux_set_fn_t set;
-};
-
-#define to_typec_switch(_dev_) container_of(_dev_, struct typec_switch, dev)
-#define to_typec_mux(_dev_) container_of(_dev_, struct typec_mux, dev)
 
 #endif /* __USB_TYPEC_ALTMODE_H__ */
diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index 45f0bf65e9aba..5fa279a96b6ef 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -6,74 +6,15 @@
  * Author: Heikki Krogerus <heikki.krogerus@linux.intel.com>
  */
 
-#include <linux/device.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/property.h>
 #include <linux/slab.h>
 #include <linux/usb/pd_vdo.h>
+#include <linux/usb/typec_mux.h>
 
 #include "bus.h"
-
-struct typec_plug {
-	struct device			dev;
-	enum typec_plug_index		index;
-	struct ida			mode_ids;
-	int				num_altmodes;
-};
-
-struct typec_cable {
-	struct device			dev;
-	enum typec_plug_type		type;
-	struct usb_pd_identity		*identity;
-	unsigned int			active:1;
-	u16				pd_revision; /* 0300H = "3.0" */
-};
-
-struct typec_partner {
-	struct device			dev;
-	unsigned int			usb_pd:1;
-	struct usb_pd_identity		*identity;
-	enum typec_accessory		accessory;
-	struct ida			mode_ids;
-	int				num_altmodes;
-	u16				pd_revision; /* 0300H = "3.0" */
-	enum usb_pd_svdm_ver		svdm_version;
-};
-
-struct typec_port {
-	unsigned int			id;
-	struct device			dev;
-	struct ida			mode_ids;
-
-	int				prefer_role;
-	enum typec_data_role		data_role;
-	enum typec_role			pwr_role;
-	enum typec_role			vconn_role;
-	enum typec_pwr_opmode		pwr_opmode;
-	enum typec_port_type		port_type;
-	struct mutex			port_type_lock;
-
-	enum typec_orientation		orientation;
-	struct typec_switch		*sw;
-	struct typec_mux		*mux;
-
-	const struct typec_capability	*cap;
-	const struct typec_operations   *ops;
-};
-
-#define to_typec_port(_dev_) container_of(_dev_, struct typec_port, dev)
-#define to_typec_plug(_dev_) container_of(_dev_, struct typec_plug, dev)
-#define to_typec_cable(_dev_) container_of(_dev_, struct typec_cable, dev)
-#define to_typec_partner(_dev_) container_of(_dev_, struct typec_partner, dev)
-
-static const struct device_type typec_partner_dev_type;
-static const struct device_type typec_cable_dev_type;
-static const struct device_type typec_plug_dev_type;
-
-#define is_typec_partner(_dev_) (_dev_->type == &typec_partner_dev_type)
-#define is_typec_cable(_dev_) (_dev_->type == &typec_cable_dev_type)
-#define is_typec_plug(_dev_) (_dev_->type == &typec_plug_dev_type)
+#include "class.h"
 
 static DEFINE_IDA(typec_index_ida);
 static struct class *typec_class;
@@ -726,7 +667,7 @@ static void typec_partner_release(struct device *dev)
 	kfree(partner);
 }
 
-static const struct device_type typec_partner_dev_type = {
+const struct device_type typec_partner_dev_type = {
 	.name = "typec_partner",
 	.groups = typec_partner_groups,
 	.release = typec_partner_release,
@@ -941,7 +882,7 @@ static const struct attribute_group *typec_plug_groups[] = {
 	NULL
 };
 
-static const struct device_type typec_plug_dev_type = {
+const struct device_type typec_plug_dev_type = {
 	.name = "typec_plug",
 	.groups = typec_plug_groups,
 	.release = typec_plug_release,
@@ -1089,7 +1030,7 @@ static void typec_cable_release(struct device *dev)
 	kfree(cable);
 }
 
-static const struct device_type typec_cable_dev_type = {
+const struct device_type typec_cable_dev_type = {
 	.name = "typec_cable",
 	.groups = typec_cable_groups,
 	.release = typec_cable_release,
diff --git a/drivers/usb/typec/class.h b/drivers/usb/typec/class.h
new file mode 100644
index 0000000000000..d414be58d122e
--- /dev/null
+++ b/drivers/usb/typec/class.h
@@ -0,0 +1,76 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef __USB_TYPEC_CLASS__
+#define __USB_TYPEC_CLASS__
+
+#include <linux/device.h>
+#include <linux/usb/typec.h>
+
+struct typec_mux;
+struct typec_switch;
+
+struct typec_plug {
+	struct device			dev;
+	enum typec_plug_index		index;
+	struct ida			mode_ids;
+	int				num_altmodes;
+};
+
+struct typec_cable {
+	struct device			dev;
+	enum typec_plug_type		type;
+	struct usb_pd_identity		*identity;
+	unsigned int			active:1;
+	u16				pd_revision; /* 0300H = "3.0" */
+};
+
+struct typec_partner {
+	struct device			dev;
+	unsigned int			usb_pd:1;
+	struct usb_pd_identity		*identity;
+	enum typec_accessory		accessory;
+	struct ida			mode_ids;
+	int				num_altmodes;
+	u16				pd_revision; /* 0300H = "3.0" */
+	enum usb_pd_svdm_ver		svdm_version;
+};
+
+struct typec_port {
+	unsigned int			id;
+	struct device			dev;
+	struct ida			mode_ids;
+
+	int				prefer_role;
+	enum typec_data_role		data_role;
+	enum typec_role			pwr_role;
+	enum typec_role			vconn_role;
+	enum typec_pwr_opmode		pwr_opmode;
+	enum typec_port_type		port_type;
+	struct mutex			port_type_lock;
+
+	enum typec_orientation		orientation;
+	struct typec_switch		*sw;
+	struct typec_mux		*mux;
+
+	const struct typec_capability	*cap;
+	const struct typec_operations   *ops;
+};
+
+#define to_typec_port(_dev_) container_of(_dev_, struct typec_port, dev)
+#define to_typec_plug(_dev_) container_of(_dev_, struct typec_plug, dev)
+#define to_typec_cable(_dev_) container_of(_dev_, struct typec_cable, dev)
+#define to_typec_partner(_dev_) container_of(_dev_, struct typec_partner, dev)
+
+extern const struct device_type typec_partner_dev_type;
+extern const struct device_type typec_cable_dev_type;
+extern const struct device_type typec_plug_dev_type;
+extern const struct device_type typec_port_dev_type;
+
+#define is_typec_partner(dev) ((dev)->type == &typec_partner_dev_type)
+#define is_typec_cable(dev) ((dev)->type == &typec_cable_dev_type)
+#define is_typec_plug(dev) ((dev)->type == &typec_plug_dev_type)
+#define is_typec_port(dev) ((dev)->type == &typec_port_dev_type)
+
+extern struct class typec_mux_class;
+
+#endif /* __USB_TYPEC_CLASS__ */
diff --git a/drivers/usb/typec/mux.c b/drivers/usb/typec/mux.c
index cf720e944aaaa..9da22ae3006c9 100644
--- a/drivers/usb/typec/mux.c
+++ b/drivers/usb/typec/mux.c
@@ -13,9 +13,9 @@
 #include <linux/mutex.h>
 #include <linux/property.h>
 #include <linux/slab.h>
-#include <linux/usb/typec_mux.h>
 
-#include "bus.h"
+#include "class.h"
+#include "mux.h"
 
 static bool dev_name_ends_with(struct device *dev, const char *suffix)
 {
diff --git a/drivers/usb/typec/mux.h b/drivers/usb/typec/mux.h
new file mode 100644
index 0000000000000..4fd9426ee44f6
--- /dev/null
+++ b/drivers/usb/typec/mux.h
@@ -0,0 +1,21 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef __USB_TYPEC_MUX__
+#define __USB_TYPEC_MUX__
+
+#include <linux/usb/typec_mux.h>
+
+struct typec_switch {
+	struct device dev;
+	typec_switch_set_fn_t set;
+};
+
+struct typec_mux {
+	struct device dev;
+	typec_mux_set_fn_t set;
+};
+
+#define to_typec_switch(_dev_) container_of(_dev_, struct typec_switch, dev)
+#define to_typec_mux(_dev_) container_of(_dev_, struct typec_mux, dev)
+
+#endif /* __USB_TYPEC_MUX__ */
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 3/6] usb: typec: Declare the typec_class static
  2021-03-25 12:29 [PATCH 0/6] usb: Linking ports to their Type-C connectors Heikki Krogerus
  2021-03-25 12:29 ` [PATCH 1/6] usb: Iterator for ports Heikki Krogerus
  2021-03-25 12:29 ` [PATCH 2/6] usb: typec: Organize the private headers properly Heikki Krogerus
@ 2021-03-25 12:29 ` Heikki Krogerus
  2021-03-25 12:29 ` [PATCH 4/6] usb: typec: Port mapping utility Heikki Krogerus
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 14+ messages in thread
From: Heikki Krogerus @ 2021-03-25 12:29 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Benson Leung, Prashant Malani, Guenter Roeck, linux-usb, linux-kernel

This is only to make the handling of the class consistent
with the two other susbsystems - the alt mode bus and the
mux class.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/class.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index 5fa279a96b6ef..d3e1002386357 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -17,7 +17,11 @@
 #include "class.h"
 
 static DEFINE_IDA(typec_index_ida);
-static struct class *typec_class;
+
+static struct class typec_class = {
+	.name = "typec",
+	.owner = THIS_MODULE,
+};
 
 /* ------------------------------------------------------------------------- */
 /* Common attributes */
@@ -551,7 +555,7 @@ typec_register_altmode(struct device *parent,
 
 	/* Plug alt modes need a class to generate udev events. */
 	if (is_typec_plug(parent))
-		alt->adev.dev.class = typec_class;
+		alt->adev.dev.class = &typec_class;
 
 	ret = device_register(&alt->adev.dev);
 	if (ret) {
@@ -815,7 +819,7 @@ struct typec_partner *typec_register_partner(struct typec_port *port,
 		partner->identity = desc->identity;
 	}
 
-	partner->dev.class = typec_class;
+	partner->dev.class = &typec_class;
 	partner->dev.parent = &port->dev;
 	partner->dev.type = &typec_partner_dev_type;
 	dev_set_name(&partner->dev, "%s-partner", dev_name(&port->dev));
@@ -967,7 +971,7 @@ struct typec_plug *typec_register_plug(struct typec_cable *cable,
 	ida_init(&plug->mode_ids);
 	plug->num_altmodes = -1;
 	plug->index = desc->index;
-	plug->dev.class = typec_class;
+	plug->dev.class = &typec_class;
 	plug->dev.parent = &cable->dev;
 	plug->dev.type = &typec_plug_dev_type;
 	dev_set_name(&plug->dev, "%s-%s", dev_name(cable->dev.parent), name);
@@ -1132,7 +1136,7 @@ struct typec_cable *typec_register_cable(struct typec_port *port,
 		cable->identity = desc->identity;
 	}
 
-	cable->dev.class = typec_class;
+	cable->dev.class = &typec_class;
 	cable->dev.parent = &port->dev;
 	cable->dev.type = &typec_cable_dev_type;
 	dev_set_name(&cable->dev, "%s-cable", dev_name(&port->dev));
@@ -1986,7 +1990,7 @@ struct typec_port *typec_register_port(struct device *parent,
 	port->prefer_role = cap->prefer_role;
 
 	device_initialize(&port->dev);
-	port->dev.class = typec_class;
+	port->dev.class = &typec_class;
 	port->dev.parent = parent;
 	port->dev.fwnode = cap->fwnode;
 	port->dev.type = &typec_port_dev_type;
@@ -2049,11 +2053,9 @@ static int __init typec_init(void)
 	if (ret)
 		goto err_unregister_bus;
 
-	typec_class = class_create(THIS_MODULE, "typec");
-	if (IS_ERR(typec_class)) {
-		ret = PTR_ERR(typec_class);
+	ret = class_register(&typec_class);
+	if (ret)
 		goto err_unregister_mux_class;
-	}
 
 	return 0;
 
@@ -2069,7 +2071,7 @@ subsys_initcall(typec_init);
 
 static void __exit typec_exit(void)
 {
-	class_destroy(typec_class);
+	class_unregister(&typec_class);
 	ida_destroy(&typec_index_ida);
 	bus_unregister(&typec_bus);
 	class_unregister(&typec_mux_class);
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 4/6] usb: typec: Port mapping utility
  2021-03-25 12:29 [PATCH 0/6] usb: Linking ports to their Type-C connectors Heikki Krogerus
                   ` (2 preceding siblings ...)
  2021-03-25 12:29 ` [PATCH 3/6] usb: typec: Declare the typec_class static Heikki Krogerus
@ 2021-03-25 12:29 ` Heikki Krogerus
  2021-03-25 12:29 ` [PATCH 5/6] usb: Link the ports to the connectors they are attached to Heikki Krogerus
  2021-03-25 12:29 ` [PATCH 6/6] usb: typec: Link all ports during connector registration Heikki Krogerus
  5 siblings, 0 replies; 14+ messages in thread
From: Heikki Krogerus @ 2021-03-25 12:29 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Benson Leung, Prashant Malani, Guenter Roeck, linux-usb, linux-kernel

Adding functions that can be used to link/unlink ports -
USB ports, TBT3/USB4 ports, DisplayPorts and so on - to
the USB Type-C connectors they are attached to inside a
system. The symlink that is created for the port device is
named "connector".

Initially only ACPI is supported. ACPI port object shares
the _PLD (Physical Location of Device) with the USB Type-C
connector that it's attached to.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/Makefile      |   1 +
 drivers/usb/typec/class.c       |   7 +-
 drivers/usb/typec/class.h       |  18 +++
 drivers/usb/typec/port-mapper.c | 225 ++++++++++++++++++++++++++++++++
 include/linux/usb/typec.h       |  13 ++
 5 files changed, 263 insertions(+), 1 deletion(-)
 create mode 100644 drivers/usb/typec/port-mapper.c

diff --git a/drivers/usb/typec/Makefile b/drivers/usb/typec/Makefile
index a820e6e8c1ffc..ef790cb72d8c3 100644
--- a/drivers/usb/typec/Makefile
+++ b/drivers/usb/typec/Makefile
@@ -4,6 +4,7 @@ CFLAGS_tps6598x.o		:= -I$(src)
 
 obj-$(CONFIG_TYPEC)		+= typec.o
 typec-y				:= class.o mux.o bus.o
+typec-$(CONFIG_ACPI)		+= port-mapper.o
 obj-$(CONFIG_TYPEC)		+= altmodes/
 obj-$(CONFIG_TYPEC_TCPM)	+= tcpm/
 obj-$(CONFIG_TYPEC_UCSI)	+= ucsi/
diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index d3e1002386357..ff199e2d26c7b 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -18,7 +18,7 @@
 
 static DEFINE_IDA(typec_index_ida);
 
-static struct class typec_class = {
+struct class typec_class = {
 	.name = "typec",
 	.owner = THIS_MODULE,
 };
@@ -1601,6 +1601,7 @@ static void typec_release(struct device *dev)
 	ida_destroy(&port->mode_ids);
 	typec_switch_put(port->sw);
 	typec_mux_put(port->mux);
+	free_pld(port->pld);
 	kfree(port->cap);
 	kfree(port);
 }
@@ -1983,6 +1984,8 @@ struct typec_port *typec_register_port(struct device *parent,
 
 	ida_init(&port->mode_ids);
 	mutex_init(&port->port_type_lock);
+	mutex_init(&port->port_list_lock);
+	INIT_LIST_HEAD(&port->port_list);
 
 	port->id = id;
 	port->ops = cap->ops;
@@ -2024,6 +2027,8 @@ struct typec_port *typec_register_port(struct device *parent,
 		return ERR_PTR(ret);
 	}
 
+	port->pld = get_pld(&port->dev);
+
 	return port;
 }
 EXPORT_SYMBOL_GPL(typec_register_port);
diff --git a/drivers/usb/typec/class.h b/drivers/usb/typec/class.h
index d414be58d122e..f3bc4d175d79c 100644
--- a/drivers/usb/typec/class.h
+++ b/drivers/usb/typec/class.h
@@ -54,6 +54,11 @@ struct typec_port {
 
 	const struct typec_capability	*cap;
 	const struct typec_operations   *ops;
+
+	struct list_head		port_list;
+	struct mutex			port_list_lock; /* Port list lock */
+
+	void				*pld;
 };
 
 #define to_typec_port(_dev_) container_of(_dev_, struct typec_port, dev)
@@ -72,5 +77,18 @@ extern const struct device_type typec_port_dev_type;
 #define is_typec_port(dev) ((dev)->type == &typec_port_dev_type)
 
 extern struct class typec_mux_class;
+extern struct class typec_class;
+
+#ifdef CONFIG_ACPI
+void *get_pld(struct device *dev);
+void free_pld(void *pld);
+#else
+static inline void *get_pld(struct device *dev)
+{
+	return NULL;
+}
+
+static inline void free_pld(void *pld) { }
+#endif
 
 #endif /* __USB_TYPEC_CLASS__ */
diff --git a/drivers/usb/typec/port-mapper.c b/drivers/usb/typec/port-mapper.c
new file mode 100644
index 0000000000000..97264a4f11967
--- /dev/null
+++ b/drivers/usb/typec/port-mapper.c
@@ -0,0 +1,225 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * USB Type-C Connector Class Port Mapping Utility
+ *
+ * Copyright (C) 2021, Intel Corporation
+ * Author: Heikki Krogerus <heikki.krogerus@linux.intel.com>
+ */
+
+#include <linux/acpi.h>
+#include <linux/usb.h>
+#include <linux/usb/typec.h>
+
+#include "class.h"
+
+struct port_node {
+	struct list_head list;
+	struct device *dev;
+	void *pld;
+};
+
+static int acpi_pld_match(const struct acpi_pld_info *pld1,
+			  const struct acpi_pld_info *pld2)
+{
+	if (!pld1 || !pld2)
+		return 0;
+
+	/*
+	 * To speed things up, first checking only the group_position. It seems
+	 * to often have the first unique value in the _PLD.
+	 */
+	if (pld1->group_position == pld2->group_position)
+		return !memcmp(pld1, pld2, sizeof(struct acpi_pld_info));
+
+	return 0;
+}
+
+void *get_pld(struct device *dev)
+{
+	struct acpi_pld_info *pld;
+	acpi_status status;
+
+	if (!has_acpi_companion(dev))
+		return NULL;
+
+	status = acpi_get_physical_device_location(ACPI_HANDLE(dev), &pld);
+	if (ACPI_FAILURE(status))
+		return NULL;
+
+	return pld;
+}
+
+void free_pld(void *pld)
+{
+	ACPI_FREE(pld);
+}
+
+static int __link_port(struct typec_port *con, struct port_node *node)
+{
+	int ret;
+
+	ret = sysfs_create_link(&node->dev->kobj, &con->dev.kobj, "connector");
+	if (ret)
+		return ret;
+
+	ret = sysfs_create_link(&con->dev.kobj, &node->dev->kobj,
+				dev_name(node->dev));
+	if (ret) {
+		sysfs_remove_link(&node->dev->kobj, "connector");
+		return ret;
+	}
+
+	list_add_tail(&node->list, &con->port_list);
+
+	return 0;
+}
+
+static int link_port(struct typec_port *con, struct port_node *node)
+{
+	int ret;
+
+	mutex_lock(&con->port_list_lock);
+	ret = __link_port(con, node);
+	mutex_unlock(&con->port_list_lock);
+
+	return ret;
+}
+
+static void __unlink_port(struct typec_port *con, struct port_node *node)
+{
+	sysfs_remove_link(&con->dev.kobj, dev_name(node->dev));
+	sysfs_remove_link(&node->dev->kobj, "connector");
+	list_del(&node->list);
+}
+
+static void unlink_port(struct typec_port *con, struct port_node *node)
+{
+	mutex_lock(&con->port_list_lock);
+	__unlink_port(con, node);
+	mutex_unlock(&con->port_list_lock);
+}
+
+static struct port_node *create_port_node(struct device *port)
+{
+	struct port_node *node;
+
+	node = kzalloc(sizeof(*node), GFP_KERNEL);
+	if (!node)
+		return ERR_PTR(-ENOMEM);
+
+	node->dev = get_device(port);
+	node->pld = get_pld(port);
+
+	return node;
+}
+
+static void remove_port_node(struct port_node *node)
+{
+	put_device(node->dev);
+	free_pld(node->pld);
+	kfree(node);
+}
+
+static int connector_pld_match(struct device *dev, const void *pld)
+{
+	if (!is_typec_port(dev))
+		return 0;
+
+	return acpi_pld_match(to_typec_port(dev)->pld, pld);
+}
+
+static struct device *find_connector(struct port_node *node)
+{
+	if (!node->pld)
+		return NULL;
+
+	return class_find_device(&typec_class, NULL, node->pld, connector_pld_match);
+}
+
+/**
+ * typec_link_port - Link a port to its connector
+ * @port: The port device
+ *
+ * Find the connector of @port and create symlink named "connector" for it.
+ * Returns 0 on success, or errno in case of a failure.
+ *
+ * NOTE. The function increments the reference count of @port on success.
+ */
+int typec_link_port(struct device *port)
+{
+	struct device *connector;
+	struct port_node *node;
+	int ret = 0;
+
+	node = create_port_node(port);
+	if (IS_ERR(node))
+		return PTR_ERR(node);
+
+	connector = find_connector(node);
+	if (!connector)
+		goto remove_node;
+
+	ret = link_port(to_typec_port(connector), node);
+	if (ret)
+		goto put_connector;
+
+	return 0;
+
+put_connector:
+	put_device(connector);
+remove_node:
+	remove_port_node(node);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(typec_link_port);
+
+struct port_match_data {
+	struct device *port;
+	struct device *connector;
+	struct port_node *node;
+};
+
+static int port_match(struct device *connector, void *_data)
+{
+	struct port_match_data *data = _data;
+	struct port_node *node;
+	int ret;
+
+	if (!is_typec_port(connector))
+		return 0;
+
+	mutex_lock(&to_typec_port(connector)->port_list_lock);
+	list_for_each_entry(node, &to_typec_port(connector)->port_list, list) {
+		ret = node->dev == data->port;
+		if (ret) {
+			data->connector = connector;
+			data->node = node;
+			break;
+		}
+	}
+	mutex_unlock(&to_typec_port(connector)->port_list_lock);
+
+	return ret;
+}
+
+/**
+ * typec_unlink_port - Unlink port from its connector
+ * @port: The port device
+ *
+ * Removes the symlink "connector" and decrements the reference count of @port.
+ */
+void typec_unlink_port(struct device *port)
+{
+	struct port_match_data data = { .port = port, };
+	int ret;
+
+	ret = class_for_each_device(&typec_class, NULL, &data, port_match);
+	if (!ret)
+		return;
+
+	unlink_port(to_typec_port(data.connector), data.node);
+	remove_port_node(data.node);
+	put_device(data.connector);
+}
+EXPORT_SYMBOL_GPL(typec_unlink_port);
diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
index 91b4303ca305c..604cd2da15e83 100644
--- a/include/linux/usb/typec.h
+++ b/include/linux/usb/typec.h
@@ -298,4 +298,17 @@ int typec_find_port_data_role(const char *name);
 void typec_partner_set_svdm_version(struct typec_partner *partner,
 				    enum usb_pd_svdm_ver svdm_version);
 int typec_get_negotiated_svdm_version(struct typec_port *port);
+
+#if IS_ENABLED(CONFIG_ACPI) && IS_REACHABLE(CONFIG_TYPEC)
+int typec_link_port(struct device *port);
+void typec_unlink_port(struct device *port);
+#else
+static inline int typec_link_port(struct device *port)
+{
+	return 0;
+}
+
+static inline void typec_unlink_port(struct device *port) { }
+#endif
+
 #endif /* __LINUX_USB_TYPEC_H */
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 5/6] usb: Link the ports to the connectors they are attached to
  2021-03-25 12:29 [PATCH 0/6] usb: Linking ports to their Type-C connectors Heikki Krogerus
                   ` (3 preceding siblings ...)
  2021-03-25 12:29 ` [PATCH 4/6] usb: typec: Port mapping utility Heikki Krogerus
@ 2021-03-25 12:29 ` Heikki Krogerus
  2021-03-25 12:29 ` [PATCH 6/6] usb: typec: Link all ports during connector registration Heikki Krogerus
  5 siblings, 0 replies; 14+ messages in thread
From: Heikki Krogerus @ 2021-03-25 12:29 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Benson Leung, Prashant Malani, Guenter Roeck, linux-usb, linux-kernel

Creating link to the USB Type-C connector for every new port
that is added when possible.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 Documentation/ABI/testing/sysfs-bus-usb | 9 +++++++++
 drivers/usb/core/port.c                 | 3 +++
 2 files changed, 12 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-usb b/Documentation/ABI/testing/sysfs-bus-usb
index bf2c1968525f0..8b4303a0ff51d 100644
--- a/Documentation/ABI/testing/sysfs-bus-usb
+++ b/Documentation/ABI/testing/sysfs-bus-usb
@@ -255,6 +255,15 @@ Description:
 		is permitted, "u2" if only u2 is permitted, "u1_u2" if both u1 and
 		u2 are permitted.
 
+What:		/sys/bus/usb/devices/.../(hub interface)/portX/connector
+Date:		April 2021
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		Link to the USB Type-C connector when available. This link is
+		only created when USB Type-C Connector Class is enabled, and
+		only if the system firmware is capable of describing the
+		connection between a port and its connector.
+
 What:		/sys/bus/usb/devices/.../power/usb2_lpm_l1_timeout
 Date:		May 2013
 Contact:	Mathias Nyman <mathias.nyman@linux.intel.com>
diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index dfcca9c876c73..3c382a4b648ec 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -9,6 +9,7 @@
 
 #include <linux/slab.h>
 #include <linux/pm_qos.h>
+#include <linux/usb/typec.h>
 
 #include "hub.h"
 
@@ -576,6 +577,7 @@ int usb_hub_create_port_device(struct usb_hub *hub, int port1)
 	}
 
 	find_and_link_peer(hub, port1);
+	typec_link_port(&port_dev->dev);
 
 	/*
 	 * Enable runtime pm and hold a refernce that hub_configure()
@@ -619,5 +621,6 @@ void usb_hub_remove_port_device(struct usb_hub *hub, int port1)
 	peer = port_dev->peer;
 	if (peer)
 		unlink_peers(port_dev, peer);
+	typec_unlink_port(&port_dev->dev);
 	device_unregister(&port_dev->dev);
 }
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 6/6] usb: typec: Link all ports during connector registration
  2021-03-25 12:29 [PATCH 0/6] usb: Linking ports to their Type-C connectors Heikki Krogerus
                   ` (4 preceding siblings ...)
  2021-03-25 12:29 ` [PATCH 5/6] usb: Link the ports to the connectors they are attached to Heikki Krogerus
@ 2021-03-25 12:29 ` Heikki Krogerus
  2021-03-25 19:10   ` kernel test robot
  5 siblings, 1 reply; 14+ messages in thread
From: Heikki Krogerus @ 2021-03-25 12:29 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Benson Leung, Prashant Malani, Guenter Roeck, linux-usb, linux-kernel

The connectors may be registered after the ports, so the
"connector" links need to be created for the ports also when
ever a new connector gets registered.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/class.c       |  9 +++--
 drivers/usb/typec/class.h       | 10 +++---
 drivers/usb/typec/port-mapper.c | 62 +++++++++++++++++++++++++++++++--
 3 files changed, 71 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index ff199e2d26c7b..f1c2d823c6509 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -1601,7 +1601,6 @@ static void typec_release(struct device *dev)
 	ida_destroy(&port->mode_ids);
 	typec_switch_put(port->sw);
 	typec_mux_put(port->mux);
-	free_pld(port->pld);
 	kfree(port->cap);
 	kfree(port);
 }
@@ -2027,7 +2026,9 @@ struct typec_port *typec_register_port(struct device *parent,
 		return ERR_PTR(ret);
 	}
 
-	port->pld = get_pld(&port->dev);
+	ret = typec_link_ports(port);
+	if (ret)
+		dev_warn(&port->dev, "failed to create symlinks (%d)\n", ret);
 
 	return port;
 }
@@ -2041,8 +2042,10 @@ EXPORT_SYMBOL_GPL(typec_register_port);
  */
 void typec_unregister_port(struct typec_port *port)
 {
-	if (!IS_ERR_OR_NULL(port))
+	if (!IS_ERR_OR_NULL(port)) {
+		typec_unlink_ports(port);
 		device_unregister(&port->dev);
+	}
 }
 EXPORT_SYMBOL_GPL(typec_unregister_port);
 
diff --git a/drivers/usb/typec/class.h b/drivers/usb/typec/class.h
index f3bc4d175d79c..a6a034f49c228 100644
--- a/drivers/usb/typec/class.h
+++ b/drivers/usb/typec/class.h
@@ -80,15 +80,15 @@ extern struct class typec_mux_class;
 extern struct class typec_class;
 
 #ifdef CONFIG_ACPI
-void *get_pld(struct device *dev);
-void free_pld(void *pld);
+int typec_link_ports(struct typec_port *connector);
+void typec_unlink_ports(struct typec_port *connector);
 #else
-static inline void *get_pld(struct device *dev)
+static inline int typec_link_ports(struct typec_port *connector)
 {
-	return NULL;
+	return 0;
 }
 
-static inline void free_pld(void *pld) { }
+static inline void typec_unlink_ports(struct typec_port *connector) { }
 #endif
 
 #endif /* __USB_TYPEC_CLASS__ */
diff --git a/drivers/usb/typec/port-mapper.c b/drivers/usb/typec/port-mapper.c
index 97264a4f11967..98eda37d99117 100644
--- a/drivers/usb/typec/port-mapper.c
+++ b/drivers/usb/typec/port-mapper.c
@@ -34,7 +34,7 @@ static int acpi_pld_match(const struct acpi_pld_info *pld1,
 	return 0;
 }
 
-void *get_pld(struct device *dev)
+static void *get_pld(struct device *dev)
 {
 	struct acpi_pld_info *pld;
 	acpi_status status;
@@ -49,7 +49,7 @@ void *get_pld(struct device *dev)
 	return pld;
 }
 
-void free_pld(void *pld)
+static void free_pld(void *pld)
 {
 	ACPI_FREE(pld);
 }
@@ -223,3 +223,61 @@ void typec_unlink_port(struct device *port)
 	put_device(data.connector);
 }
 EXPORT_SYMBOL_GPL(typec_unlink_port);
+
+static int connector_match(struct device *port, void *connector)
+{
+	struct port_node *node;
+	int ret;
+
+	node = create_port_node(port);
+	if (IS_ERR(node))
+		return PTR_ERR(node);
+
+	if (!node->pld || !connector_pld_match(connector, node->pld)) {
+		remove_port_node(node);
+		return 0;
+	}
+
+	ret = link_port(to_typec_port(connector), node);
+	if (ret) {
+		remove_port_node(node->pld);
+		return ret;
+	}
+
+	get_device(connector);
+
+	return 0;
+}
+
+int typec_link_ports(struct typec_port *con)
+{
+	int ret;
+
+	con->pld = get_pld(&con->dev);
+	if (!con->pld)
+		return 0;
+
+	ret = usb_for_each_port(&con->dev, connector_match);
+	if (ret)
+		typec_unlink_ports(con);
+
+	return ret;
+}
+
+void typec_unlink_ports(struct typec_port *con)
+{
+	struct port_node *node;
+	struct port_node *tmp;
+
+	mutex_lock(&con->port_list_lock);
+
+	list_for_each_entry_safe(node, tmp, &con->port_list, list) {
+		__unlink_port(con, node);
+		remove_port_node(node);
+		put_device(&con->dev);
+	}
+
+	mutex_unlock(&con->port_list_lock);
+
+	free_pld(con->pld);
+}
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/6] usb: Iterator for ports
  2021-03-25 12:29 ` [PATCH 1/6] usb: Iterator for ports Heikki Krogerus
@ 2021-03-25 14:41   ` Alan Stern
  2021-03-25 15:14     ` Heikki Krogerus
  0 siblings, 1 reply; 14+ messages in thread
From: Alan Stern @ 2021-03-25 14:41 UTC (permalink / raw)
  To: Heikki Krogerus
  Cc: Greg Kroah-Hartman, Benson Leung, Prashant Malani, Guenter Roeck,
	linux-usb, linux-kernel

On Thu, Mar 25, 2021 at 03:29:21PM +0300, Heikki Krogerus wrote:
> Introducing usb_for_each_port(). It works the same way as
> usb_for_each_dev(), but instead of going through every USB
> device in the system, it walks through the USB ports in the
> system.
> 
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

This has a couple of nasty errors.

> ---
>  drivers/usb/core/usb.c | 43 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/usb.h    |  1 +
>  2 files changed, 44 insertions(+)
> 
> diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> index 2ce3667ec6fae..6d49db9a1b208 100644
> --- a/drivers/usb/core/usb.c
> +++ b/drivers/usb/core/usb.c
> @@ -398,6 +398,49 @@ int usb_for_each_dev(void *data, int (*fn)(struct usb_device *, void *))
>  }
>  EXPORT_SYMBOL_GPL(usb_for_each_dev);
>  
> +struct each_hub_arg {
> +	void *data;
> +	int (*fn)(struct device *, void *);
> +};
> +
> +static int __each_hub(struct device *dev, void *data)
> +{
> +	struct each_hub_arg *arg = (struct each_hub_arg *)data;
> +	struct usb_device *hdev = to_usb_device(dev);

to_usb_device() won't work properly if the struct device isn't embedded 
in an actual usb_device structure.  And that will happen, since the USB 
bus type holds usb_interface structures as well as usb_devices.

In fact, you should use usb_for_each_dev here; it already does what you 
want.

> +	struct usb_hub *hub;
> +	int ret;
> +	int i;
> +
> +	hub = usb_hub_to_struct_hub(hdev);
> +	if (!hub)
> +		return 0;
> +
> +	for (i = 0; i < hdev->maxchild; i++) {
> +		ret = arg->fn(&hub->ports[i]->dev, arg->data);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}

Don't you need some sort of locking or refcounting here?  What would 
happen if this hub got removed while the routine was running?

Alan Stern

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/6] usb: Iterator for ports
  2021-03-25 14:41   ` Alan Stern
@ 2021-03-25 15:14     ` Heikki Krogerus
  2021-03-25 15:20       ` Greg Kroah-Hartman
                         ` (2 more replies)
  0 siblings, 3 replies; 14+ messages in thread
From: Heikki Krogerus @ 2021-03-25 15:14 UTC (permalink / raw)
  To: Alan Stern
  Cc: Greg Kroah-Hartman, Benson Leung, Prashant Malani, Guenter Roeck,
	linux-usb, linux-kernel

On Thu, Mar 25, 2021 at 10:41:09AM -0400, Alan Stern wrote:
> On Thu, Mar 25, 2021 at 03:29:21PM +0300, Heikki Krogerus wrote:
> > Introducing usb_for_each_port(). It works the same way as
> > usb_for_each_dev(), but instead of going through every USB
> > device in the system, it walks through the USB ports in the
> > system.
> > 
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> 
> This has a couple of nasty errors.
> 
> > ---
> >  drivers/usb/core/usb.c | 43 ++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/usb.h    |  1 +
> >  2 files changed, 44 insertions(+)
> > 
> > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> > index 2ce3667ec6fae..6d49db9a1b208 100644
> > --- a/drivers/usb/core/usb.c
> > +++ b/drivers/usb/core/usb.c
> > @@ -398,6 +398,49 @@ int usb_for_each_dev(void *data, int (*fn)(struct usb_device *, void *))
> >  }
> >  EXPORT_SYMBOL_GPL(usb_for_each_dev);
> >  
> > +struct each_hub_arg {
> > +	void *data;
> > +	int (*fn)(struct device *, void *);
> > +};
> > +
> > +static int __each_hub(struct device *dev, void *data)
> > +{
> > +	struct each_hub_arg *arg = (struct each_hub_arg *)data;
> > +	struct usb_device *hdev = to_usb_device(dev);
> 
> to_usb_device() won't work properly if the struct device isn't embedded 
> in an actual usb_device structure.  And that will happen, since the USB 
> bus type holds usb_interface structures as well as usb_devices.

OK, so I need to filter them out.

> In fact, you should use usb_for_each_dev here; it already does what you 
> want.

Unfortunately I can't use usb_for_each_dev here, because it deals with
struct usb_device while I need to deal with struct device in the
callback.

> > +	struct usb_hub *hub;
> > +	int ret;
> > +	int i;
> > +
> > +	hub = usb_hub_to_struct_hub(hdev);
> > +	if (!hub)
> > +		return 0;
> > +
> > +	for (i = 0; i < hdev->maxchild; i++) {
> > +		ret = arg->fn(&hub->ports[i]->dev, arg->data);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> 
> Don't you need some sort of locking or refcounting here?  What would 
> happen if this hub got removed while the routine was running?

I'll use a lock then.

thanks,

-- 
heikki

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/6] usb: Iterator for ports
  2021-03-25 15:14     ` Heikki Krogerus
@ 2021-03-25 15:20       ` Greg Kroah-Hartman
  2021-03-25 15:33         ` Heikki Krogerus
  2021-03-25 15:23       ` Heikki Krogerus
  2021-03-25 15:31       ` Alan Stern
  2 siblings, 1 reply; 14+ messages in thread
From: Greg Kroah-Hartman @ 2021-03-25 15:20 UTC (permalink / raw)
  To: Heikki Krogerus
  Cc: Alan Stern, Benson Leung, Prashant Malani, Guenter Roeck,
	linux-usb, linux-kernel

On Thu, Mar 25, 2021 at 05:14:42PM +0200, Heikki Krogerus wrote:
> On Thu, Mar 25, 2021 at 10:41:09AM -0400, Alan Stern wrote:
> > On Thu, Mar 25, 2021 at 03:29:21PM +0300, Heikki Krogerus wrote:
> > > Introducing usb_for_each_port(). It works the same way as
> > > usb_for_each_dev(), but instead of going through every USB
> > > device in the system, it walks through the USB ports in the
> > > system.
> > > 
> > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > 
> > This has a couple of nasty errors.
> > 
> > > ---
> > >  drivers/usb/core/usb.c | 43 ++++++++++++++++++++++++++++++++++++++++++
> > >  include/linux/usb.h    |  1 +
> > >  2 files changed, 44 insertions(+)
> > > 
> > > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> > > index 2ce3667ec6fae..6d49db9a1b208 100644
> > > --- a/drivers/usb/core/usb.c
> > > +++ b/drivers/usb/core/usb.c
> > > @@ -398,6 +398,49 @@ int usb_for_each_dev(void *data, int (*fn)(struct usb_device *, void *))
> > >  }
> > >  EXPORT_SYMBOL_GPL(usb_for_each_dev);
> > >  
> > > +struct each_hub_arg {
> > > +	void *data;
> > > +	int (*fn)(struct device *, void *);
> > > +};
> > > +
> > > +static int __each_hub(struct device *dev, void *data)
> > > +{
> > > +	struct each_hub_arg *arg = (struct each_hub_arg *)data;
> > > +	struct usb_device *hdev = to_usb_device(dev);
> > 
> > to_usb_device() won't work properly if the struct device isn't embedded 
> > in an actual usb_device structure.  And that will happen, since the USB 
> > bus type holds usb_interface structures as well as usb_devices.
> 
> OK, so I need to filter them out.
> 
> > In fact, you should use usb_for_each_dev here; it already does what you 
> > want.
> 
> Unfortunately I can't use usb_for_each_dev here, because it deals with
> struct usb_device while I need to deal with struct device in the
> callback.

Why do you need 'struct device' in the callback?  All you really want is
the hub devices, right?

> > > +	struct usb_hub *hub;
> > > +	int ret;
> > > +	int i;
> > > +
> > > +	hub = usb_hub_to_struct_hub(hdev);
> > > +	if (!hub)
> > > +		return 0;
> > > +
> > > +	for (i = 0; i < hdev->maxchild; i++) {
> > > +		ret = arg->fn(&hub->ports[i]->dev, arg->data);
> > > +		if (ret)
> > > +			return ret;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > 
> > Don't you need some sort of locking or refcounting here?  What would 
> > happen if this hub got removed while the routine was running?
> 
> I'll use a lock then.

That's not going to work to be held over a callback.  Just properly
increment the reference count.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/6] usb: Iterator for ports
  2021-03-25 15:14     ` Heikki Krogerus
  2021-03-25 15:20       ` Greg Kroah-Hartman
@ 2021-03-25 15:23       ` Heikki Krogerus
  2021-03-25 15:31       ` Alan Stern
  2 siblings, 0 replies; 14+ messages in thread
From: Heikki Krogerus @ 2021-03-25 15:23 UTC (permalink / raw)
  To: Alan Stern
  Cc: Greg Kroah-Hartman, Benson Leung, Prashant Malani, Guenter Roeck,
	linux-usb, linux-kernel

On Thu, Mar 25, 2021 at 05:14:45PM +0200, Heikki Krogerus wrote:
> On Thu, Mar 25, 2021 at 10:41:09AM -0400, Alan Stern wrote:
> > On Thu, Mar 25, 2021 at 03:29:21PM +0300, Heikki Krogerus wrote:
> > > Introducing usb_for_each_port(). It works the same way as
> > > usb_for_each_dev(), but instead of going through every USB
> > > device in the system, it walks through the USB ports in the
> > > system.
> > > 
> > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > 
> > This has a couple of nasty errors.
> > 
> > > ---
> > >  drivers/usb/core/usb.c | 43 ++++++++++++++++++++++++++++++++++++++++++
> > >  include/linux/usb.h    |  1 +
> > >  2 files changed, 44 insertions(+)
> > > 
> > > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> > > index 2ce3667ec6fae..6d49db9a1b208 100644
> > > --- a/drivers/usb/core/usb.c
> > > +++ b/drivers/usb/core/usb.c
> > > @@ -398,6 +398,49 @@ int usb_for_each_dev(void *data, int (*fn)(struct usb_device *, void *))
> > >  }
> > >  EXPORT_SYMBOL_GPL(usb_for_each_dev);
> > >  
> > > +struct each_hub_arg {
> > > +	void *data;
> > > +	int (*fn)(struct device *, void *);
> > > +};
> > > +
> > > +static int __each_hub(struct device *dev, void *data)
> > > +{
> > > +	struct each_hub_arg *arg = (struct each_hub_arg *)data;
> > > +	struct usb_device *hdev = to_usb_device(dev);
> > 
> > to_usb_device() won't work properly if the struct device isn't embedded 
> > in an actual usb_device structure.  And that will happen, since the USB 
> > bus type holds usb_interface structures as well as usb_devices.
> 
> OK, so I need to filter them out.
> 
> > In fact, you should use usb_for_each_dev here; it already does what you 
> > want.
> 
> Unfortunately I can't use usb_for_each_dev here, because it deals with
> struct usb_device while I need to deal with struct device in the
> callback.

Ah, I can use it instead of bus_for_each_dev() in usb_for_each_port().
I'll fix these in v2.

For the lock I guess I can just use the peer lock (usb_port_peer_mutex)?

thanks,

-- 
heikki

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/6] usb: Iterator for ports
  2021-03-25 15:14     ` Heikki Krogerus
  2021-03-25 15:20       ` Greg Kroah-Hartman
  2021-03-25 15:23       ` Heikki Krogerus
@ 2021-03-25 15:31       ` Alan Stern
  2 siblings, 0 replies; 14+ messages in thread
From: Alan Stern @ 2021-03-25 15:31 UTC (permalink / raw)
  To: Heikki Krogerus
  Cc: Greg Kroah-Hartman, Benson Leung, Prashant Malani, Guenter Roeck,
	linux-usb, linux-kernel

On Thu, Mar 25, 2021 at 05:14:42PM +0200, Heikki Krogerus wrote:
> On Thu, Mar 25, 2021 at 10:41:09AM -0400, Alan Stern wrote:
> > On Thu, Mar 25, 2021 at 03:29:21PM +0300, Heikki Krogerus wrote:
> > > Introducing usb_for_each_port(). It works the same way as
> > > usb_for_each_dev(), but instead of going through every USB
> > > device in the system, it walks through the USB ports in the
> > > system.
> > > 
> > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > 
> > This has a couple of nasty errors.
> > 
> > > ---
> > >  drivers/usb/core/usb.c | 43 ++++++++++++++++++++++++++++++++++++++++++
> > >  include/linux/usb.h    |  1 +
> > >  2 files changed, 44 insertions(+)
> > > 
> > > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> > > index 2ce3667ec6fae..6d49db9a1b208 100644
> > > --- a/drivers/usb/core/usb.c
> > > +++ b/drivers/usb/core/usb.c
> > > @@ -398,6 +398,49 @@ int usb_for_each_dev(void *data, int (*fn)(struct usb_device *, void *))
> > >  }
> > >  EXPORT_SYMBOL_GPL(usb_for_each_dev);
> > >  
> > > +struct each_hub_arg {
> > > +	void *data;
> > > +	int (*fn)(struct device *, void *);
> > > +};
> > > +
> > > +static int __each_hub(struct device *dev, void *data)
> > > +{
> > > +	struct each_hub_arg *arg = (struct each_hub_arg *)data;
> > > +	struct usb_device *hdev = to_usb_device(dev);
> > 
> > to_usb_device() won't work properly if the struct device isn't embedded 
> > in an actual usb_device structure.  And that will happen, since the USB 
> > bus type holds usb_interface structures as well as usb_devices.
> 
> OK, so I need to filter them out.
> 
> > In fact, you should use usb_for_each_dev here; it already does what you 
> > want.
> 
> Unfortunately I can't use usb_for_each_dev here, because it deals with
> struct usb_device while I need to deal with struct device in the
> callback.

I see; the prototypes of arg->fn are different.  Oh well, it's a shame 
the code can't be reused.  In any case, you should copy what 
usb.c:__each_dev() does.

Alan Stern

> > > +	struct usb_hub *hub;
> > > +	int ret;
> > > +	int i;
> > > +
> > > +	hub = usb_hub_to_struct_hub(hdev);
> > > +	if (!hub)
> > > +		return 0;
> > > +
> > > +	for (i = 0; i < hdev->maxchild; i++) {
> > > +		ret = arg->fn(&hub->ports[i]->dev, arg->data);
> > > +		if (ret)
> > > +			return ret;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > 
> > Don't you need some sort of locking or refcounting here?  What would 
> > happen if this hub got removed while the routine was running?
> 
> I'll use a lock then.
> 
> thanks,
> 
> -- 
> heikki

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/6] usb: Iterator for ports
  2021-03-25 15:20       ` Greg Kroah-Hartman
@ 2021-03-25 15:33         ` Heikki Krogerus
  0 siblings, 0 replies; 14+ messages in thread
From: Heikki Krogerus @ 2021-03-25 15:33 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Alan Stern, Benson Leung, Prashant Malani, Guenter Roeck,
	linux-usb, linux-kernel

On Thu, Mar 25, 2021 at 04:20:15PM +0100, Greg Kroah-Hartman wrote:
> On Thu, Mar 25, 2021 at 05:14:42PM +0200, Heikki Krogerus wrote:
> > On Thu, Mar 25, 2021 at 10:41:09AM -0400, Alan Stern wrote:
> > > On Thu, Mar 25, 2021 at 03:29:21PM +0300, Heikki Krogerus wrote:
> > > > Introducing usb_for_each_port(). It works the same way as
> > > > usb_for_each_dev(), but instead of going through every USB
> > > > device in the system, it walks through the USB ports in the
> > > > system.
> > > > 
> > > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > 
> > > This has a couple of nasty errors.
> > > 
> > > > ---
> > > >  drivers/usb/core/usb.c | 43 ++++++++++++++++++++++++++++++++++++++++++
> > > >  include/linux/usb.h    |  1 +
> > > >  2 files changed, 44 insertions(+)
> > > > 
> > > > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> > > > index 2ce3667ec6fae..6d49db9a1b208 100644
> > > > --- a/drivers/usb/core/usb.c
> > > > +++ b/drivers/usb/core/usb.c
> > > > @@ -398,6 +398,49 @@ int usb_for_each_dev(void *data, int (*fn)(struct usb_device *, void *))
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(usb_for_each_dev);
> > > >  
> > > > +struct each_hub_arg {
> > > > +	void *data;
> > > > +	int (*fn)(struct device *, void *);
> > > > +};
> > > > +
> > > > +static int __each_hub(struct device *dev, void *data)
> > > > +{
> > > > +	struct each_hub_arg *arg = (struct each_hub_arg *)data;
> > > > +	struct usb_device *hdev = to_usb_device(dev);
> > > 
> > > to_usb_device() won't work properly if the struct device isn't embedded 
> > > in an actual usb_device structure.  And that will happen, since the USB 
> > > bus type holds usb_interface structures as well as usb_devices.
> > 
> > OK, so I need to filter them out.
> > 
> > > In fact, you should use usb_for_each_dev here; it already does what you 
> > > want.
> > 
> > Unfortunately I can't use usb_for_each_dev here, because it deals with
> > struct usb_device while I need to deal with struct device in the
> > callback.
> 
> Why do you need 'struct device' in the callback?  All you really want is
> the hub devices, right?

I need the ports, not the hubs.

> > > > +	struct usb_hub *hub;
> > > > +	int ret;
> > > > +	int i;
> > > > +
> > > > +	hub = usb_hub_to_struct_hub(hdev);
> > > > +	if (!hub)
> > > > +		return 0;
> > > > +
> > > > +	for (i = 0; i < hdev->maxchild; i++) {
> > > > +		ret = arg->fn(&hub->ports[i]->dev, arg->data);
> > > > +		if (ret)
> > > > +			return ret;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > 
> > > Don't you need some sort of locking or refcounting here?  What would 
> > > happen if this hub got removed while the routine was running?
> > 
> > I'll use a lock then.
> 
> That's not going to work to be held over a callback.  Just properly
> increment the reference count.

I though we have done that already. Does bus_for_each_dev() do that
for the device that it passes to the callback until that callback
returns?

thanks,

-- 
heikki

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 6/6] usb: typec: Link all ports during connector registration
  2021-03-25 12:29 ` [PATCH 6/6] usb: typec: Link all ports during connector registration Heikki Krogerus
@ 2021-03-25 19:10   ` kernel test robot
  0 siblings, 0 replies; 14+ messages in thread
From: kernel test robot @ 2021-03-25 19:10 UTC (permalink / raw)
  To: Heikki Krogerus, Greg Kroah-Hartman
  Cc: kbuild-all, Benson Leung, Prashant Malani, Guenter Roeck,
	linux-usb, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 2026 bytes --]

Hi Heikki,

I love your patch! Yet something to improve:

[auto build test ERROR on peter.chen-usb/for-usb-next]
[also build test ERROR on linus/master v5.12-rc4 next-20210325]
[cannot apply to usb/usb-testing chrome-platform-linux/for-next balbi-usb/testing/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Heikki-Krogerus/usb-Linking-ports-to-their-Type-C-connectors/20210325-203239
base:   https://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/usb.git for-usb-next
config: i386-randconfig-a015-20210325 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/3f5d8681c4754fba373563812803b8d5eb11639a
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Heikki-Krogerus/usb-Linking-ports-to-their-Type-C-connectors/20210325-203239
        git checkout 3f5d8681c4754fba373563812803b8d5eb11639a
        # save the attached .config to linux build tree
        make W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   ld: drivers/usb/typec/port-mapper.o: in function `typec_link_ports':
>> drivers/usb/typec/port-mapper.c:260: undefined reference to `usb_for_each_port'


vim +260 drivers/usb/typec/port-mapper.c

   251	
   252	int typec_link_ports(struct typec_port *con)
   253	{
   254		int ret;
   255	
   256		con->pld = get_pld(&con->dev);
   257		if (!con->pld)
   258			return 0;
   259	
 > 260		ret = usb_for_each_port(&con->dev, connector_match);
   261		if (ret)
   262			typec_unlink_ports(con);
   263	
   264		return ret;
   265	}
   266	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 40039 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2021-03-25 19:11 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-25 12:29 [PATCH 0/6] usb: Linking ports to their Type-C connectors Heikki Krogerus
2021-03-25 12:29 ` [PATCH 1/6] usb: Iterator for ports Heikki Krogerus
2021-03-25 14:41   ` Alan Stern
2021-03-25 15:14     ` Heikki Krogerus
2021-03-25 15:20       ` Greg Kroah-Hartman
2021-03-25 15:33         ` Heikki Krogerus
2021-03-25 15:23       ` Heikki Krogerus
2021-03-25 15:31       ` Alan Stern
2021-03-25 12:29 ` [PATCH 2/6] usb: typec: Organize the private headers properly Heikki Krogerus
2021-03-25 12:29 ` [PATCH 3/6] usb: typec: Declare the typec_class static Heikki Krogerus
2021-03-25 12:29 ` [PATCH 4/6] usb: typec: Port mapping utility Heikki Krogerus
2021-03-25 12:29 ` [PATCH 5/6] usb: Link the ports to the connectors they are attached to Heikki Krogerus
2021-03-25 12:29 ` [PATCH 6/6] usb: typec: Link all ports during connector registration Heikki Krogerus
2021-03-25 19:10   ` kernel test robot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).