Hi,
This is the second version of this series. The "Iterator for ports"
patch is now moved to the end of the series (5/6).
I'm now using usb_for_each_dev() in usb_for_each_port like Alan
suggested, and I'm now using usb_port_peer_mutex to lock the ports
while we're dealing with them in __each_hub().
The original cover letter:
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: 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: Iterator for ports
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 | 46 ++++
drivers/usb/typec/Makefile | 2 +-
drivers/usb/typec/bus.c | 2 +
drivers/usb/typec/bus.h | 19 +-
drivers/usb/typec/class.c | 101 +++------
drivers/usb/typec/class.h | 85 +++++++
drivers/usb/typec/mux.c | 4 +-
drivers/usb/typec/mux.h | 21 ++
drivers/usb/typec/port-mapper.c | 280 ++++++++++++++++++++++++
include/linux/usb.h | 1 +
include/linux/usb/typec.h | 13 ++
13 files changed, 490 insertions(+), 96 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
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 <[email protected]>
---
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
Creating link to the USB Type-C connector for every new port
that is added when possible.
Signed-off-by: Heikki Krogerus <[email protected]>
---
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 <[email protected]>
+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 <[email protected]>
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
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 <[email protected]>
---
drivers/usb/typec/class.c | 9 +++--
drivers/usb/typec/class.h | 4 +-
drivers/usb/typec/port-mapper.c | 65 ++++++++++++++++++++++++++++++++-
3 files changed, 71 insertions(+), 7 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 52294f7020a8b..aef03eb7e1523 100644
--- a/drivers/usb/typec/class.h
+++ b/drivers/usb/typec/class.h
@@ -79,7 +79,7 @@ extern const struct device_type typec_port_dev_type;
extern struct class typec_mux_class;
extern struct class typec_class;
-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);
#endif /* __USB_TYPEC_CLASS__ */
diff --git a/drivers/usb/typec/port-mapper.c b/drivers/usb/typec/port-mapper.c
index 5bee7a97242fe..fafd98de89df5 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)
{
#ifdef CONFIG_ACPI
struct acpi_pld_info *pld;
@@ -53,7 +53,7 @@ void *get_pld(struct device *dev)
#endif
}
-void free_pld(void *pld)
+static void free_pld(void *pld)
{
#ifdef CONFIG_ACPI
ACPI_FREE(pld);
@@ -217,3 +217,64 @@ void typec_unlink_port(struct device *port)
class_for_each_device(&typec_class, NULL, port, port_match_and_unlink);
}
EXPORT_SYMBOL_GPL(typec_unlink_port);
+
+#ifdef CONFIG_USB
+static int each_port(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 (!connector_match(connector, node)) {
+ 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;
+}
+#endif
+
+int typec_link_ports(struct typec_port *con)
+{
+ int ret = 0;
+
+ con->pld = get_pld(&con->dev);
+ if (!con->pld)
+ return 0;
+
+#ifdef CONFIG_USB
+ ret = usb_for_each_port(&con->dev, each_port);
+ if (ret)
+ typec_unlink_ports(con);
+#endif
+ 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
On Mon, Mar 29, 2021 at 11:44:26AM +0300, Heikki Krogerus wrote:
> +#ifdef CONFIG_USB
This feels odd in a file under drivers/usb/ is it still relevant? Will
this code get built for non-USB systems (i.e. gadget only?)
> +static int each_port(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 (!connector_match(connector, node)) {
> + 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;
> +}
> +#endif
> +
> +int typec_link_ports(struct typec_port *con)
> +{
> + int ret = 0;
> +
> + con->pld = get_pld(&con->dev);
> + if (!con->pld)
> + return 0;
> +
> +#ifdef CONFIG_USB
> + ret = usb_for_each_port(&con->dev, each_port);
> + if (ret)
> + typec_unlink_ports(con);
If you have proper #ifdef for CONFIG_USB in the .h file, then there's no
need for the #ifdef in the .c file.
thanks,
greg k-h
Adding a header file for each subsystem - the connector
class, alt mode bus and the class for the muxes.
Signed-off-by: Heikki Krogerus <[email protected]>
---
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 <[email protected]>
*/
-#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
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 <[email protected]>
---
drivers/usb/core/usb.c | 46 ++++++++++++++++++++++++++++++++++++++++++
include/linux/usb.h | 1 +
2 files changed, 47 insertions(+)
diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index 2ce3667ec6fae..62368c4ed37af 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -398,6 +398,52 @@ 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 usb_device *hdev, void *data)
+{
+ struct each_hub_arg *arg = (struct each_hub_arg *)data;
+ struct usb_hub *hub;
+ int ret = 0;
+ int i;
+
+ hub = usb_hub_to_struct_hub(hdev);
+ if (!hub)
+ return 0;
+
+ mutex_lock(&usb_port_peer_mutex);
+
+ for (i = 0; i < hdev->maxchild; i++) {
+ ret = arg->fn(&hub->ports[i]->dev, arg->data);
+ if (ret)
+ break;
+ }
+
+ mutex_unlock(&usb_port_peer_mutex);
+
+ return ret;
+}
+
+/**
+ * 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 usb_for_each_dev(&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 ddd2f5b2a2827..e4d2eb703cf89 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -871,6 +871,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
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 <[email protected]>
---
drivers/usb/typec/Makefile | 2 +-
drivers/usb/typec/class.c | 7 +-
drivers/usb/typec/class.h | 9 ++
drivers/usb/typec/port-mapper.c | 219 ++++++++++++++++++++++++++++++++
include/linux/usb/typec.h | 13 ++
5 files changed, 248 insertions(+), 2 deletions(-)
create mode 100644 drivers/usb/typec/port-mapper.c
diff --git a/drivers/usb/typec/Makefile b/drivers/usb/typec/Makefile
index a820e6e8c1ffc..1e1868832b8d8 100644
--- a/drivers/usb/typec/Makefile
+++ b/drivers/usb/typec/Makefile
@@ -3,7 +3,7 @@
CFLAGS_tps6598x.o := -I$(src)
obj-$(CONFIG_TYPEC) += typec.o
-typec-y := class.o mux.o bus.o
+typec-y := class.o mux.o bus.o 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..52294f7020a8b 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,9 @@ 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;
+
+void *get_pld(struct device *dev);
+void free_pld(void *pld);
#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..5bee7a97242fe
--- /dev/null
+++ b/drivers/usb/typec/port-mapper.c
@@ -0,0 +1,219 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * USB Type-C Connector Class Port Mapping Utility
+ *
+ * Copyright (C) 2021, Intel Corporation
+ * Author: Heikki Krogerus <[email protected]>
+ */
+
+#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)
+{
+#ifdef CONFIG_ACPI
+ 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;
+#else
+ return NULL;
+#endif
+}
+
+void free_pld(void *pld)
+{
+#ifdef CONFIG_ACPI
+ ACPI_FREE(pld);
+#endif
+}
+
+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_match(struct device *dev, const void *data)
+{
+ const struct port_node *node = data;
+
+ if (!is_typec_port(dev))
+ return 0;
+
+ return acpi_pld_match(to_typec_port(dev)->pld, node->pld);
+}
+
+static struct device *find_connector(struct port_node *node)
+{
+ if (!node->pld)
+ return NULL;
+
+ return class_find_device(&typec_class, NULL, node, connector_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);
+
+static int port_match_and_unlink(struct device *connector, void *port)
+{
+ struct port_node *node;
+ struct port_node *tmp;
+ int ret = 0;
+
+ if (!is_typec_port(connector))
+ return 0;
+
+ mutex_lock(&to_typec_port(connector)->port_list_lock);
+ list_for_each_entry_safe(node, tmp, &to_typec_port(connector)->port_list, list) {
+ ret = node->dev == port;
+ if (ret) {
+ unlink_port(to_typec_port(connector), node);
+ remove_port_node(node);
+ put_device(connector);
+ 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)
+{
+ class_for_each_device(&typec_class, NULL, port, port_match_and_unlink);
+}
+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
On Mon, Mar 29, 2021 at 10:48:19AM +0200, Greg Kroah-Hartman wrote:
> On Mon, Mar 29, 2021 at 11:44:26AM +0300, Heikki Krogerus wrote:
> > +#ifdef CONFIG_USB
>
> This feels odd in a file under drivers/usb/ is it still relevant? Will
> this code get built for non-USB systems (i.e. gadget only?)
Yes, later. The typec connector class can not depend on CONFIG_USB for
sure.
> > +static int each_port(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 (!connector_match(connector, node)) {
> > + 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;
> > +}
> > +#endif
> > +
> > +int typec_link_ports(struct typec_port *con)
> > +{
> > + int ret = 0;
> > +
> > + con->pld = get_pld(&con->dev);
> > + if (!con->pld)
> > + return 0;
> > +
> > +#ifdef CONFIG_USB
> > + ret = usb_for_each_port(&con->dev, each_port);
> > + if (ret)
> > + typec_unlink_ports(con);
>
> If you have proper #ifdef for CONFIG_USB in the .h file, then there's no
> need for the #ifdef in the .c file.
We could do that now, but we will have to move the ifdef back to the
C file the moment we add support for Thunderbolt ports and/or
DisplayPorts.
I could make a stub for the usb_for_each_port() function in case
CONFIG_USB is not enable. Would that work?
thanks,
--
heikki
On Mon, Mar 29, 2021 at 12:19:36PM +0300, Heikki Krogerus wrote:
> > I could make a stub for the usb_for_each_port() function in case
> > CONFIG_USB is not enable. Would that work?
>
> Ah, I think that's what you meant :-)
Yes, that's what I meant :)
thanks,
greg k-h
> I could make a stub for the usb_for_each_port() function in case
> CONFIG_USB is not enable. Would that work?
Ah, I think that's what you meant :-)
I'll fix it.
thaks,
--
heikki
Hi Heikki,
I love your patch! Yet something to improve:
[auto build test ERROR on next-20210326]
[also build test ERROR on v5.12-rc5]
[cannot apply to usb/usb-testing chrome-platform-linux/for-next linus/master v5.12-rc5 v5.12-rc4 v5.12-rc3]
[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/20210329-164859
base: 931294922e65a23e1aad6398b9ae02df74044679
config: nios2-allyesconfig (attached as .config)
compiler: nios2-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/e86970d606657b7ce44bbdb80f4d25048bca710f
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/20210329-164859
git checkout e86970d606657b7ce44bbdb80f4d25048bca710f
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=nios2
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
drivers/usb/typec/port-mapper.c:156:5: error: redefinition of 'typec_link_port'
156 | int typec_link_port(struct device *port)
| ^~~~~~~~~~~~~~~
In file included from drivers/usb/typec/port-mapper.c:11:
include/linux/usb/typec.h:306:19: note: previous definition of 'typec_link_port' was here
306 | static inline int typec_link_port(struct device *port)
| ^~~~~~~~~~~~~~~
>> drivers/usb/typec/port-mapper.c:215:6: error: redefinition of 'typec_unlink_port'
215 | void typec_unlink_port(struct device *port)
| ^~~~~~~~~~~~~~~~~
In file included from drivers/usb/typec/port-mapper.c:11:
include/linux/usb/typec.h:311:20: note: previous definition of 'typec_unlink_port' was here
311 | static inline void typec_unlink_port(struct device *port) { }
| ^~~~~~~~~~~~~~~~~
vim +/typec_unlink_port +215 drivers/usb/typec/port-mapper.c
146
147 /**
148 * typec_link_port - Link a port to its connector
149 * @port: The port device
150 *
151 * Find the connector of @port and create symlink named "connector" for it.
152 * Returns 0 on success, or errno in case of a failure.
153 *
154 * NOTE. The function increments the reference count of @port on success.
155 */
> 156 int typec_link_port(struct device *port)
157 {
158 struct device *connector;
159 struct port_node *node;
160 int ret = 0;
161
162 node = create_port_node(port);
163 if (IS_ERR(node))
164 return PTR_ERR(node);
165
166 connector = find_connector(node);
167 if (!connector)
168 goto remove_node;
169
170 ret = link_port(to_typec_port(connector), node);
171 if (ret)
172 goto put_connector;
173
174 return 0;
175
176 put_connector:
177 put_device(connector);
178 remove_node:
179 remove_port_node(node);
180
181 return ret;
182 }
183 EXPORT_SYMBOL_GPL(typec_link_port);
184
185 static int port_match_and_unlink(struct device *connector, void *port)
186 {
187 struct port_node *node;
188 struct port_node *tmp;
189 int ret = 0;
190
191 if (!is_typec_port(connector))
192 return 0;
193
194 mutex_lock(&to_typec_port(connector)->port_list_lock);
195 list_for_each_entry_safe(node, tmp, &to_typec_port(connector)->port_list, list) {
196 ret = node->dev == port;
197 if (ret) {
198 unlink_port(to_typec_port(connector), node);
199 remove_port_node(node);
200 put_device(connector);
201 break;
202 }
203 }
204 mutex_unlock(&to_typec_port(connector)->port_list_lock);
205
206 return ret;
207 }
208
209 /**
210 * typec_unlink_port - Unlink port from its connector
211 * @port: The port device
212 *
213 * Removes the symlink "connector" and decrements the reference count of @port.
214 */
> 215 void typec_unlink_port(struct device *port)
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
Hi Heikki,
I love your patch! Yet something to improve:
[auto build test ERROR on next-20210326]
[also build test ERROR on v5.12-rc5]
[cannot apply to usb/usb-testing chrome-platform-linux/for-next linus/master v5.12-rc5 v5.12-rc4 v5.12-rc3]
[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/20210329-164859
base: 931294922e65a23e1aad6398b9ae02df74044679
config: m68k-randconfig-c004-20210329 (attached as .config)
compiler: m68k-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/e86970d606657b7ce44bbdb80f4d25048bca710f
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/20210329-164859
git checkout e86970d606657b7ce44bbdb80f4d25048bca710f
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=m68k
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
>> drivers/usb/typec/port-mapper.c:156:5: error: redefinition of 'typec_link_port'
156 | int typec_link_port(struct device *port)
| ^~~~~~~~~~~~~~~
In file included from drivers/usb/typec/port-mapper.c:11:
include/linux/usb/typec.h:306:19: note: previous definition of 'typec_link_port' was here
306 | static inline int typec_link_port(struct device *port)
| ^~~~~~~~~~~~~~~
>> drivers/usb/typec/port-mapper.c:215:6: error: redefinition of 'typec_unlink_port'
215 | void typec_unlink_port(struct device *port)
| ^~~~~~~~~~~~~~~~~
In file included from drivers/usb/typec/port-mapper.c:11:
include/linux/usb/typec.h:311:20: note: previous definition of 'typec_unlink_port' was here
311 | static inline void typec_unlink_port(struct device *port) { }
| ^~~~~~~~~~~~~~~~~
vim +/typec_link_port +156 drivers/usb/typec/port-mapper.c
146
147 /**
148 * typec_link_port - Link a port to its connector
149 * @port: The port device
150 *
151 * Find the connector of @port and create symlink named "connector" for it.
152 * Returns 0 on success, or errno in case of a failure.
153 *
154 * NOTE. The function increments the reference count of @port on success.
155 */
> 156 int typec_link_port(struct device *port)
157 {
158 struct device *connector;
159 struct port_node *node;
160 int ret = 0;
161
162 node = create_port_node(port);
163 if (IS_ERR(node))
164 return PTR_ERR(node);
165
166 connector = find_connector(node);
167 if (!connector)
168 goto remove_node;
169
170 ret = link_port(to_typec_port(connector), node);
171 if (ret)
172 goto put_connector;
173
174 return 0;
175
176 put_connector:
177 put_device(connector);
178 remove_node:
179 remove_port_node(node);
180
181 return ret;
182 }
183 EXPORT_SYMBOL_GPL(typec_link_port);
184
185 static int port_match_and_unlink(struct device *connector, void *port)
186 {
187 struct port_node *node;
188 struct port_node *tmp;
189 int ret = 0;
190
191 if (!is_typec_port(connector))
192 return 0;
193
194 mutex_lock(&to_typec_port(connector)->port_list_lock);
195 list_for_each_entry_safe(node, tmp, &to_typec_port(connector)->port_list, list) {
196 ret = node->dev == port;
197 if (ret) {
198 unlink_port(to_typec_port(connector), node);
199 remove_port_node(node);
200 put_device(connector);
201 break;
202 }
203 }
204 mutex_unlock(&to_typec_port(connector)->port_list_lock);
205
206 return ret;
207 }
208
209 /**
210 * typec_unlink_port - Unlink port from its connector
211 * @port: The port device
212 *
213 * Removes the symlink "connector" and decrements the reference count of @port.
214 */
> 215 void typec_unlink_port(struct device *port)
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
On Mon, Mar 29, 2021 at 11:44:25AM +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 <[email protected]>
> ---
> drivers/usb/core/usb.c | 46 ++++++++++++++++++++++++++++++++++++++++++
> include/linux/usb.h | 1 +
> 2 files changed, 47 insertions(+)
>
> diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> index 2ce3667ec6fae..62368c4ed37af 100644
> --- a/drivers/usb/core/usb.c
> +++ b/drivers/usb/core/usb.c
> @@ -398,6 +398,52 @@ 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 usb_device *hdev, void *data)
> +{
> + struct each_hub_arg *arg = (struct each_hub_arg *)data;
> + struct usb_hub *hub;
> + int ret = 0;
> + int i;
> +
> + hub = usb_hub_to_struct_hub(hdev);
> + if (!hub)
> + return 0;
What happens if the hub is removed exactly now? Although hdev is
reference-counted (and the loop iterator does take a reference to it),
usb_hub_to_struct_hub doesn't take a reference to hub. And hub->ports
isn't refcounted at all.
> +
> + mutex_lock(&usb_port_peer_mutex);
> +
> + for (i = 0; i < hdev->maxchild; i++) {
> + ret = arg->fn(&hub->ports[i]->dev, arg->data);
> + if (ret)
> + break;
> + }
> +
> + mutex_unlock(&usb_port_peer_mutex);
I have a feeling that it would be better to take and release this mutex
in usb_for_each_port (or its caller), so that it is held over the whole
loop.
Alan Stern
> +
> + return ret;
> +}
> +
> +/**
> + * 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 usb_for_each_dev(&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 ddd2f5b2a2827..e4d2eb703cf89 100644
> --- a/include/linux/usb.h
> +++ b/include/linux/usb.h
> @@ -871,6 +871,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
>
On Mon, Mar 29, 2021 at 02:49:46PM -0400, Alan Stern wrote:
> On Mon, Mar 29, 2021 at 11:44:25AM +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 <[email protected]>
> > ---
> > drivers/usb/core/usb.c | 46 ++++++++++++++++++++++++++++++++++++++++++
> > include/linux/usb.h | 1 +
> > 2 files changed, 47 insertions(+)
> >
> > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> > index 2ce3667ec6fae..62368c4ed37af 100644
> > --- a/drivers/usb/core/usb.c
> > +++ b/drivers/usb/core/usb.c
> > @@ -398,6 +398,52 @@ 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 usb_device *hdev, void *data)
> > +{
> > + struct each_hub_arg *arg = (struct each_hub_arg *)data;
> > + struct usb_hub *hub;
> > + int ret = 0;
> > + int i;
> > +
> > + hub = usb_hub_to_struct_hub(hdev);
> > + if (!hub)
> > + return 0;
>
> What happens if the hub is removed exactly now? Although hdev is
> reference-counted (and the loop iterator does take a reference to it),
> usb_hub_to_struct_hub doesn't take a reference to hub. And hub->ports
> isn't refcounted at all.
If the hub is removed right now, and if hub_disconnect() also manages
to remove the ports before we have time to take the lock below, then
hdev->maxchild will be 0 by the time we can take the lock. In that
case nothing happens here.
If on the other hand we manage to acquire the usb_port_peer_mutex
before hub_disconnect(), then hub_disconnect() will simply have to
wait until we are done, and only after that remove the ports.
> > + mutex_lock(&usb_port_peer_mutex);
> > +
> > + for (i = 0; i < hdev->maxchild; i++) {
> > + ret = arg->fn(&hub->ports[i]->dev, arg->data);
> > + if (ret)
> > + break;
> > + }
> > +
> > + mutex_unlock(&usb_port_peer_mutex);
>
> I have a feeling that it would be better to take and release this mutex
> in usb_for_each_port (or its caller), so that it is held over the whole
> loop.
I disagree. The lock is for the ports, not the hubs. We should take
the lock when we are going through the ports of a hub, but release it
between the hubs. Otherwise we will be only keeping things on hold for
a long period of time for no good reason (I for example have to
evaluate the _PLD of every single port which takes a lot of time). We
don't need to prevent other things from happening to the hubs at the
same time.
thanks,
--
heikki
On Tue, Mar 30, 2021 at 12:07:35PM +0300, Heikki Krogerus wrote:
> On Mon, Mar 29, 2021 at 02:49:46PM -0400, Alan Stern wrote:
> > On Mon, Mar 29, 2021 at 11:44:25AM +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 <[email protected]>
> > > ---
> > > drivers/usb/core/usb.c | 46 ++++++++++++++++++++++++++++++++++++++++++
> > > include/linux/usb.h | 1 +
> > > 2 files changed, 47 insertions(+)
> > >
> > > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> > > index 2ce3667ec6fae..62368c4ed37af 100644
> > > --- a/drivers/usb/core/usb.c
> > > +++ b/drivers/usb/core/usb.c
> > > @@ -398,6 +398,52 @@ 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 usb_device *hdev, void *data)
> > > +{
> > > + struct each_hub_arg *arg = (struct each_hub_arg *)data;
> > > + struct usb_hub *hub;
> > > + int ret = 0;
> > > + int i;
> > > +
> > > + hub = usb_hub_to_struct_hub(hdev);
> > > + if (!hub)
> > > + return 0;
> >
> > What happens if the hub is removed exactly now? Although hdev is
> > reference-counted (and the loop iterator does take a reference to it),
> > usb_hub_to_struct_hub doesn't take a reference to hub. And hub->ports
> > isn't refcounted at all.
>
> If the hub is removed right now, and if hub_disconnect() also manages
> to remove the ports before we have time to take the lock below, then
> hdev->maxchild will be 0 by the time we can take the lock. In that
> case nothing happens here.
Okay, good.
> If on the other hand we manage to acquire the usb_port_peer_mutex
> before hub_disconnect(), then hub_disconnect() will simply have to
> wait until we are done, and only after that remove the ports.
>
> > > + mutex_lock(&usb_port_peer_mutex);
> > > +
> > > + for (i = 0; i < hdev->maxchild; i++) {
> > > + ret = arg->fn(&hub->ports[i]->dev, arg->data);
> > > + if (ret)
> > > + break;
> > > + }
> > > +
> > > + mutex_unlock(&usb_port_peer_mutex);
> >
> > I have a feeling that it would be better to take and release this mutex
> > in usb_for_each_port (or its caller), so that it is held over the whole
> > loop.
>
> I disagree. The lock is for the ports, not the hubs. We should take
> the lock when we are going through the ports of a hub, but release it
> between the hubs. Otherwise we will be only keeping things on hold for
> a long period of time for no good reason (I for example have to
> evaluate the _PLD of every single port which takes a lot of time). We
> don't need to prevent other things from happening to the hubs at the
> same time.
All right, you convinced me.
Acked-by: Alan Stern <[email protected]>
Alan Stern