Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1644617 > unrolled thread

[PATCH 07/24] thunderbolt: Convert switch to a device

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 07/24] thunderbolt: Convert switch to a device Mika Westerberg <mika.westerberg@linux.intel.com> - 2017-05-18 16:50 +0200
    Re: [PATCH 07/24] thunderbolt: Convert switch to a device Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-05-18 18:50 +0200
      Re: [PATCH 07/24] thunderbolt: Convert switch to a device Mika Westerberg <mika.westerberg@linux.intel.com> - 2017-05-19 10:30 +0200
    Re: [PATCH 07/24] thunderbolt: Convert switch to a device Lukas Wunner <lukas@wunner.de> - 2017-05-24 13:10 +0200
      Re: [PATCH 07/24] thunderbolt: Convert switch to a device Mika Westerberg <mika.westerberg@linux.intel.com> - 2017-05-24 13:50 +0200
        Re: [PATCH 07/24] thunderbolt: Convert switch to a device Lukas Wunner <lukas@wunner.de> - 2017-05-24 16:00 +0200

#1644617 — [PATCH 07/24] thunderbolt: Convert switch to a device

Thunderbolt domain consists of switches that are connected to each
other, forming a bus. This will convert each switch into a real Linux
device structure and adds them to the domain. The advantage here is
that we get all the goodies from the driver core, like reference
counting and sysfs hierarchy for free.

Also expose device identification information to the userspace via new
sysfs attributes.

In order to support internal connection manager (ICM) we separate switch
configuration into its own function (tb_switch_configure()) which is
only called by the existing native connection manager implementation
used on Macs.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Reviewed-by: Yehezkel Bernat <yehezkel.bernat@intel.com>
Reviewed-by: Michael Jamet <michael.jamet@intel.com>
---
 Documentation/ABI/testing/sysfs-bus-thunderbolt |  20 ++
 drivers/thunderbolt/eeprom.c                    |   2 +
 drivers/thunderbolt/switch.c                    | 242 +++++++++++++++++++-----
 drivers/thunderbolt/tb.c                        |  40 +++-
 drivers/thunderbolt/tb.h                        |  45 ++++-
 5 files changed, 285 insertions(+), 64 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-thunderbolt

diff --git a/Documentation/ABI/testing/sysfs-bus-thunderbolt b/Documentation/ABI/testing/sysfs-bus-thunderbolt
new file mode 100644
index 000000000000..a3dac3becd1e
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-thunderbolt
@@ -0,0 +1,20 @@
+What:		/sys/bus/thunderbolt/devices/.../device
+Date:		Sep 2017
+KernelVersion:	4.13
+Contact:	thunderbolt-software@lists.01.org
+Description:	This attribute contains id of this device extracted from
+		the device DROM.
+
+What:		/sys/bus/thunderbolt/devices/.../vendor
+Date:		Sep 2017
+KernelVersion:	4.13
+Contact:	thunderbolt-software@lists.01.org
+Description:	This attribute contains vendor id of this device extracted
+		from the device DROM.
+
+What:		/sys/bus/thunderbolt/devices/.../unique_id
+Date:		Sep 2017
+KernelVersion:	4.13
+Contact:	thunderbolt-software@lists.01.org
+Description:	This attribute contains unique id (UUID) of this device
+		extracted from the device DROM or hardware registers.
diff --git a/drivers/thunderbolt/eeprom.c b/drivers/thunderbolt/eeprom.c
index eb2179c98b09..7e485e3ef27e 100644
--- a/drivers/thunderbolt/eeprom.c
+++ b/drivers/thunderbolt/eeprom.c
@@ -479,6 +479,8 @@ int tb_drom_read(struct tb_switch *sw)
 		goto err;
 	}
 	sw->uid = header->uid;
+	sw->vendor = header->vendor_id;
+	sw->device = header->model_id;
 
 	crc = tb_crc32(sw->drom + TB_DROM_DATA_START, header->data_len);
 	if (crc != header->data_crc32) {
diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
index 5bae49d72d2c..a03548763180 100644
--- a/drivers/thunderbolt/switch.c
+++ b/drivers/thunderbolt/switch.c
@@ -281,6 +281,9 @@ static int tb_plug_events_active(struct tb_switch *sw, bool active)
 	u32 data;
 	int res;
 
+	if (!sw->config.enabled)
+		return 0;
+
 	sw->config.plug_events_delay = 0xff;
 	res = tb_sw_write(sw, ((u32 *) &sw->config) + 4, TB_CFG_SWITCH, 4, 1);
 	if (res)
@@ -307,36 +310,71 @@ static int tb_plug_events_active(struct tb_switch *sw, bool active)
 			   sw->cap_plug_events + 1, 1);
 }
 
+static ssize_t device_show(struct device *dev, struct device_attribute *attr,
+			   char *buf)
+{
+	struct tb_switch *sw = tb_to_switch(dev);
 
-/**
- * tb_switch_free() - free a tb_switch and all downstream switches
- */
-void tb_switch_free(struct tb_switch *sw)
+	return sprintf(buf, "%#x\n", sw->device);
+}
+static DEVICE_ATTR_RO(device);
+
+static ssize_t vendor_show(struct device *dev, struct device_attribute *attr,
+			   char *buf)
 {
-	int i;
-	/* port 0 is the switch itself and never has a remote */
-	for (i = 1; i <= sw->config.max_port_number; i++) {
-		if (tb_is_upstream_port(&sw->ports[i]))
-			continue;
-		if (sw->ports[i].remote)
-			tb_switch_free(sw->ports[i].remote->sw);
-		sw->ports[i].remote = NULL;
-	}
+	struct tb_switch *sw = tb_to_switch(dev);
 
-	if (!sw->is_unplugged)
-		tb_plug_events_active(sw, false);
+	return sprintf(buf, "%#x\n", sw->vendor);
+}
+static DEVICE_ATTR_RO(vendor);
+
+static ssize_t unique_id_show(struct device *dev, struct device_attribute *attr,
+			      char *buf)
+{
+	struct tb_switch *sw = tb_to_switch(dev);
 
+	return sprintf(buf, "%pUb\n", sw->uuid);
+}
+static DEVICE_ATTR_RO(unique_id);
+
+static struct attribute *switch_attrs[] = {
+	&dev_attr_device.attr,
+	&dev_attr_vendor.attr,
+	&dev_attr_unique_id.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(switch);
+
+static void tb_switch_release(struct device *dev)
+{
+	struct tb_switch *sw = tb_to_switch(dev);
+
+	kfree(sw->uuid);
 	kfree(sw->ports);
 	kfree(sw->drom);
 	kfree(sw);
 }
 
+struct device_type tb_switch_type = {
+	.name = "thunderbolt_device",
+	.release = tb_switch_release,
+};
+
 /**
- * tb_switch_alloc() - allocate and initialize a switch
+ * tb_switch_alloc() - allocate a switch
+ * @tb: Pointer to the owning domain
+ * @parent: Parent device for this switch
+ * @route: Route string for this switch
  *
- * Return: Returns a NULL on failure.
+ * Allocates and initializes a switch. Will not upload configuration to
+ * the switch. For that you need to call tb_switch_configure()
+ * separately. The returned switch should be released by calling
+ * tb_switch_put().
+ *
+ * Return: Pointer to the allocated switch or %NULL in case of failure
  */
-struct tb_switch *tb_switch_alloc(struct tb *tb, u64 route)
+struct tb_switch *tb_switch_alloc(struct tb *tb, struct device *parent,
+				  u64 route)
 {
 	int i;
 	int cap;
@@ -352,10 +390,8 @@ struct tb_switch *tb_switch_alloc(struct tb *tb, u64 route)
 	sw->tb = tb;
 	if (tb_cfg_read(tb->ctl, &sw->config, route, 0, TB_CFG_SWITCH, 0, 5))
 		goto err;
-	tb_info(tb,
-		"initializing Switch at %#llx (depth: %d, up port: %d)\n",
-		route, tb_route_length(route), upstream_port);
-	tb_info(tb, "old switch config:\n");
+
+	tb_info(tb, "current switch config:\n");
 	tb_dump_switch(tb, &sw->config);
 
 	/* configure switch */
@@ -363,24 +399,7 @@ struct tb_switch *tb_switch_alloc(struct tb *tb, u64 route)
 	sw->config.depth = tb_route_length(route);
 	sw->config.route_lo = route;
 	sw->config.route_hi = route >> 32;
-	sw->config.enabled = 1;
-	/* from here on we may use the tb_sw_* functions & macros */
-
-	if (sw->config.vendor_id != 0x8086)
-		tb_sw_warn(sw, "unknown switch vendor id %#x\n",
-			   sw->config.vendor_id);
-
-	if (sw->config.device_id != PCI_DEVICE_ID_INTEL_LIGHT_RIDGE &&
-	    sw->config.device_id != PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_4C &&
-	    sw->config.device_id != PCI_DEVICE_ID_INTEL_PORT_RIDGE &&
-	    sw->config.device_id != PCI_DEVICE_ID_INTEL_FALCON_RIDGE_2C_BRIDGE &&
-	    sw->config.device_id != PCI_DEVICE_ID_INTEL_FALCON_RIDGE_4C_BRIDGE)
-		tb_sw_warn(sw, "unsupported switch device id %#x\n",
-			   sw->config.device_id);
-
-	/* upload configuration */
-	if (tb_sw_write(sw, 1 + (u32 *) &sw->config, TB_CFG_SWITCH, 1, 3))
-		goto err;
+	sw->config.enabled = 0;
 
 	/* initialize ports */
 	sw->ports = kcalloc(sw->config.max_port_number + 1, sizeof(*sw->ports),
@@ -401,31 +420,152 @@ struct tb_switch *tb_switch_alloc(struct tb *tb, u64 route)
 	}
 	sw->cap_plug_events = cap;
 
+	device_initialize(&sw->dev);
+	sw->dev.parent = parent;
+	sw->dev.bus = &tb_bus_type;
+	sw->dev.type = &tb_switch_type;
+	sw->dev.groups = switch_groups;
+	dev_set_name(&sw->dev, "%u-%llx", tb->index, tb_route(sw));
+
+	return sw;
+err:
+	kfree(sw->ports);
+	kfree(sw);
+	return NULL;
+}
+
+/**
+ * tb_switch_configure() - Uploads configuration to the switch
+ * @sw: Switch to configure
+ *
+ * Call this function before the switch is added to the system. It will
+ * upload configuration to the switch and makes it available for the
+ * connection manager to use.
+ *
+ * Return: %0 in case of success and negative errno in case of failure
+ */
+int tb_switch_configure(struct tb_switch *sw)
+{
+	struct tb *tb = sw->tb;
+	u64 route;
+	int ret;
+
+	route = tb_route(sw);
+	tb_info(tb,
+		"initializing Switch at %#llx (depth: %d, up port: %d)\n",
+		route, tb_route_length(route), sw->config.upstream_port_number);
+
+	if (sw->config.vendor_id != PCI_VENDOR_ID_INTEL)
+		tb_sw_warn(sw, "unknown switch vendor id %#x\n",
+			   sw->config.vendor_id);
+
+	if (sw->config.device_id != PCI_DEVICE_ID_INTEL_LIGHT_RIDGE &&
+	    sw->config.device_id != PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_4C &&
+	    sw->config.device_id != PCI_DEVICE_ID_INTEL_PORT_RIDGE &&
+	    sw->config.device_id != PCI_DEVICE_ID_INTEL_FALCON_RIDGE_2C_BRIDGE &&
+	    sw->config.device_id != PCI_DEVICE_ID_INTEL_FALCON_RIDGE_4C_BRIDGE)
+		tb_sw_warn(sw, "unsupported switch device id %#x\n",
+			   sw->config.device_id);
+
+	sw->config.enabled = 1;
+
+	/* upload configuration */
+	ret = tb_sw_write(sw, 1 + (u32 *) &sw->config, TB_CFG_SWITCH, 1, 3);
+	if (ret)
+		return ret;
+
+	return tb_plug_events_active(sw, true);
+}
+
+static void tb_switch_set_uuid(struct tb_switch *sw)
+{
+	u32 uuid[4];
+	int cap;
+
+	if (sw->uuid)
+		return;
+
+	/*
+	 * By default the UUID will be based on UID where upper two
+	 * dwords are filled with ones.
+	 */
+	uuid[0] = sw->uid & 0xffffffff;
+	uuid[1] = (sw->uid >> 32) & 0xffffffff;
+	uuid[2] = 0xffffffff;
+	uuid[3] = 0xffffffff;
+
+	/*
+	 * The newer controllers include fused UUID as part of link
+	 * controller specific registers
+	 */
+	cap = tb_switch_find_vsec_cap(sw, TB_VSEC_CAP_LINK_CONTROLLER);
+	if (cap > 0)
+		tb_sw_read(sw, uuid, TB_CFG_SWITCH, cap + 3, 4);
+
+	sw->uuid = kmemdup(uuid, sizeof(uuid), GFP_KERNEL);
+}
+
+/**
+ * tb_switch_add() - Add a switch to the domain
+ * @sw: Switch to add
+ *
+ * This is the last step in adding switch to the domain. It will read
+ * identification information from DROM and initializes ports so that
+ * they can be used to connect other switches. The switch will be
+ * exposed to the userspace when this function successfully returns. To
+ * remove and release the switch, call tb_switch_remove().
+ *
+ * Return: %0 in case of success and negative errno in case of failure
+ */
+int tb_switch_add(struct tb_switch *sw)
+{
+	int i, ret;
+
 	/* read drom */
 	if (tb_drom_read(sw))
 		tb_sw_warn(sw, "tb_eeprom_read_rom failed, continuing\n");
 	tb_sw_info(sw, "uid: %#llx\n", sw->uid);
 
+	tb_switch_set_uuid(sw);
+
 	for (i = 0; i <= sw->config.max_port_number; i++) {
 		if (sw->ports[i].disabled) {
 			tb_port_info(&sw->ports[i], "disabled by eeprom\n");
 			continue;
 		}
-		if (tb_init_port(&sw->ports[i]))
-			goto err;
+		ret = tb_init_port(&sw->ports[i]);
+		if (ret)
+			return ret;
 	}
 
-	/* TODO: I2C, IECS, link controller */
+	return device_add(&sw->dev);
+}
 
-	if (tb_plug_events_active(sw, true))
-		goto err;
+/**
+ * tb_switch_remove() - Remove and release a switch
+ * @sw: Switch to remove
+ *
+ * This will remove the switch from the domain and release it after last
+ * reference count drops to zero. If there are switches connected below
+ * this switch, they will be removed as well.
+ */
+void tb_switch_remove(struct tb_switch *sw)
+{
+	int i;
 
-	return sw;
-err:
-	kfree(sw->ports);
-	kfree(sw->drom);
-	kfree(sw);
-	return NULL;
+	/* port 0 is the switch itself and never has a remote */
+	for (i = 1; i <= sw->config.max_port_number; i++) {
+		if (tb_is_upstream_port(&sw->ports[i]))
+			continue;
+		if (sw->ports[i].remote)
+			tb_switch_remove(sw->ports[i].remote->sw);
+		sw->ports[i].remote = NULL;
+	}
+
+	if (!sw->is_unplugged)
+		tb_plug_events_active(sw, false);
+
+	device_unregister(&sw->dev);
 }
 
 /**
diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
index 9f00a0f28d53..94ecac012428 100644
--- a/drivers/thunderbolt/tb.c
+++ b/drivers/thunderbolt/tb.c
@@ -61,9 +61,21 @@ static void tb_scan_port(struct tb_port *port)
 		tb_port_WARN(port, "port already has a remote!\n");
 		return;
 	}
-	sw = tb_switch_alloc(port->sw->tb, tb_downstream_route(port));
+	sw = tb_switch_alloc(port->sw->tb, &port->sw->dev,
+			     tb_downstream_route(port));
 	if (!sw)
 		return;
+
+	if (tb_switch_configure(sw)) {
+		tb_switch_put(sw);
+		return;
+	}
+
+	if (tb_switch_add(sw)) {
+		tb_switch_put(sw);
+		return;
+	}
+
 	port->remote = tb_upstream_port(sw);
 	tb_upstream_port(sw)->remote = port;
 	tb_scan_switch(sw);
@@ -100,7 +112,7 @@ static void tb_free_unplugged_children(struct tb_switch *sw)
 		if (!port->remote)
 			continue;
 		if (port->remote->sw->is_unplugged) {
-			tb_switch_free(port->remote->sw);
+			tb_switch_remove(port->remote->sw);
 			port->remote = NULL;
 		} else {
 			tb_free_unplugged_children(port->remote->sw);
@@ -266,7 +278,7 @@ static void tb_handle_hotplug(struct work_struct *work)
 			tb_port_info(port, "unplugged\n");
 			tb_sw_set_unplugged(port->remote->sw);
 			tb_free_invalid_tunnels(tb);
-			tb_switch_free(port->remote->sw);
+			tb_switch_remove(port->remote->sw);
 			port->remote = NULL;
 		} else {
 			tb_port_info(port,
@@ -325,22 +337,32 @@ static void tb_stop(struct tb *tb)
 		tb_pci_deactivate(tunnel);
 		tb_pci_free(tunnel);
 	}
-
-	if (tb->root_switch)
-		tb_switch_free(tb->root_switch);
-	tb->root_switch = NULL;
-
+	tb_switch_remove(tb->root_switch);
 	tcm->hotplug_active = false; /* signal tb_handle_hotplug to quit */
 }
 
 static int tb_start(struct tb *tb)
 {
 	struct tb_cm *tcm = tb_priv(tb);
+	int ret;
 
-	tb->root_switch = tb_switch_alloc(tb, 0);
+	tb->root_switch = tb_switch_alloc(tb, &tb->dev, 0);
 	if (!tb->root_switch)
 		return -ENOMEM;
 
+	ret = tb_switch_configure(tb->root_switch);
+	if (ret) {
+		tb_switch_put(tb->root_switch);
+		return ret;
+	}
+
+	/* Announce the switch to the world */
+	ret = tb_switch_add(tb->root_switch);
+	if (ret) {
+		tb_switch_put(tb->root_switch);
+		return ret;
+	}
+
 	/* Full scan to discover devices added before the driver was loaded. */
 	tb_scan_switch(tb->root_switch);
 	tb_activate_pcie_devices(tb);
diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
index ce9db64e3333..350c3f21924e 100644
--- a/drivers/thunderbolt/tb.h
+++ b/drivers/thunderbolt/tb.h
@@ -8,20 +8,36 @@
 #define TB_H_
 
 #include <linux/pci.h>
+#include <linux/uuid.h>
 
 #include "tb_regs.h"
 #include "ctl.h"
 
 /**
  * struct tb_switch - a thunderbolt switch
+ * @dev: Device for the switch
+ * @config: Switch configuration
+ * @ports: Ports in this switch
+ * @tb: Pointer to the domain the switch belongs to
+ * @uid: Unique ID of the switch
+ * @uuid: UUID of the switch (or %NULL if not supported)
+ * @vendor: Vendor ID of the switch
+ * @device: Device ID of the switch
+ * @cap_plug_events: Offset to the plug events capability (%0 if not found)
+ * @is_unplugged: The switch is going away
+ * @drom: DROM of the switch (%NULL if not found)
  */
 struct tb_switch {
+	struct device dev;
 	struct tb_regs_switch_header config;
 	struct tb_port *ports;
 	struct tb *tb;
 	u64 uid;
-	int cap_plug_events; /* offset, zero if not found */
-	bool is_unplugged; /* unplugged, will go away */
+	uuid_be *uuid;
+	u16 vendor;
+	u16 device;
+	int cap_plug_events;
+	bool is_unplugged;
 	u8 *drom;
 };
 
@@ -242,6 +258,7 @@ struct tb *tb_probe(struct tb_nhi *nhi);
 
 extern struct bus_type tb_bus_type;
 extern struct device_type tb_domain_type;
+extern struct device_type tb_switch_type;
 
 int tb_domain_init(void);
 void tb_domain_exit(void);
@@ -257,14 +274,34 @@ static inline void tb_domain_put(struct tb *tb)
 	put_device(&tb->dev);
 }
 
-struct tb_switch *tb_switch_alloc(struct tb *tb, u64 route);
-void tb_switch_free(struct tb_switch *sw);
+struct tb_switch *tb_switch_alloc(struct tb *tb, struct device *parent,
+				  u64 route);
+int tb_switch_configure(struct tb_switch *sw);
+int tb_switch_add(struct tb_switch *sw);
+void tb_switch_remove(struct tb_switch *sw);
 void tb_switch_suspend(struct tb_switch *sw);
 int tb_switch_resume(struct tb_switch *sw);
 int tb_switch_reset(struct tb *tb, u64 route);
 void tb_sw_set_unplugged(struct tb_switch *sw);
 struct tb_switch *get_switch_at_route(struct tb_switch *sw, u64 route);
 
+static inline void tb_switch_put(struct tb_switch *sw)
+{
+	put_device(&sw->dev);
+}
+
+static inline bool tb_is_switch(const struct device *dev)
+{
+	return dev->type == &tb_switch_type;
+}
+
+static inline struct tb_switch *tb_to_switch(struct device *dev)
+{
+	if (tb_is_switch(dev))
+		return container_of(dev, struct tb_switch, dev);
+	return NULL;
+}
+
 int tb_wait_for_port(struct tb_port *port, bool wait_if_unplugged);
 int tb_port_add_nfc_credits(struct tb_port *port, int credits);
 int tb_port_clear_counter(struct tb_port *port, int counter);
-- 
2.11.0

[toc] | [next] | [standalone]

#1644743

On Thu, May 18, 2017 at 5:38 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> Thunderbolt domain consists of switches that are connected to each
> other, forming a bus. This will convert each switch into a real Linux
> device structure and adds them to the domain. The advantage here is
> that we get all the goodies from the driver core, like reference
> counting and sysfs hierarchy for free.
>
> Also expose device identification information to the userspace via new
> sysfs attributes.
>
> In order to support internal connection manager (ICM) we separate switch
> configuration into its own function (tb_switch_configure()) which is
> only called by the existing native connection manager implementation
> used on Macs.
>

Couple of nits below.

> +
> +       return sw;
> +err:

Perhaps
err_free_sw_ports:
?

> +       kfree(sw->ports);
> +       kfree(sw);
> +       return NULL;
> +}

> +       /* upload configuration */
> +       ret = tb_sw_write(sw, 1 + (u32 *) &sw->config, TB_CFG_SWITCH, 1, 3);

Extra space before &.

> +       if (ret)
> +               return ret;

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1645338

On Thu, May 18, 2017 at 07:49:21PM +0300, Andy Shevchenko wrote:
> On Thu, May 18, 2017 at 5:38 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > Thunderbolt domain consists of switches that are connected to each
> > other, forming a bus. This will convert each switch into a real Linux
> > device structure and adds them to the domain. The advantage here is
> > that we get all the goodies from the driver core, like reference
> > counting and sysfs hierarchy for free.
> >
> > Also expose device identification information to the userspace via new
> > sysfs attributes.
> >
> > In order to support internal connection manager (ICM) we separate switch
> > configuration into its own function (tb_switch_configure()) which is
> > only called by the existing native connection manager implementation
> > used on Macs.
> >
> 
> Couple of nits below.
> 
> > +
> > +       return sw;
> > +err:
> 
> Perhaps
> err_free_sw_ports:
> ?

Works for me :)

> > +       kfree(sw->ports);
> > +       kfree(sw);
> > +       return NULL;
> > +}
> 
> > +       /* upload configuration */
> > +       ret = tb_sw_write(sw, 1 + (u32 *) &sw->config, TB_CFG_SWITCH, 1, 3);
> 
> Extra space before &.

This comes from the original code but I'll change it.

> > +       if (ret)
> > +               return ret;
> 
> -- 
> With Best Regards,
> Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1649469

On Thu, May 18, 2017 at 05:38:57PM +0300, Mika Westerberg wrote:
> Thunderbolt domain consists of switches that are connected to each
> other, forming a bus. This will convert each switch into a real Linux
> device structure and adds them to the domain. The advantage here is
> that we get all the goodies from the driver core, like reference
> counting and sysfs hierarchy for free.

I'm wondering, would it make sense to also model each port of a switch
as a device?

With your patches, the hierarchy looks something like this:
nhi_pci_dev/domain0/switch0/switch1/switch2

If each port is modeled as a device, we'd get something like this:
nhi_pci_dev/domain0/switch0/port1/switch1/port3/switch2

I think with the controllers that shipped, there can be up to two
child switches below a switch.  If ports are not modeled as devices,
you can't tell which port a switch is connected to.

Also, if ports are modeled as devices we'd be able to store attributes
such as port type in sysfs.  Of course we could also store those in
each switch directory as "port0", "port1", ... files.

I don't know if this makes sense, but a discussion about the pros
and cons is probably warranted.  IIUC once the patches go in, the
layout is set in stone because it's a user-space API.


> +What:		/sys/bus/thunderbolt/devices/.../device
> +Date:		Sep 2017
> +KernelVersion:	4.13
> +Contact:	thunderbolt-software@lists.01.org
> +Description:	This attribute contains id of this device extracted from
> +		the device DROM.

Hm, why did you choose to expose the DROM vendor + device ID instead of
the one in switch config space?

The vendor on my MacBookPro9,1 is just 0x0001 and the device is 0x000a.
Is this valid?  Does Intel assign the vendor IDs, i.e. 0x0001 = Apple?


> +	if (sw->config.vendor_id != PCI_VENDOR_ID_INTEL)
> +		tb_sw_warn(sw, "unknown switch vendor id %#x\n",
> +			   sw->config.vendor_id);
> +
> +	if (sw->config.device_id != PCI_DEVICE_ID_INTEL_LIGHT_RIDGE &&
> +	    sw->config.device_id != PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_4C &&
> +	    sw->config.device_id != PCI_DEVICE_ID_INTEL_PORT_RIDGE &&
> +	    sw->config.device_id != PCI_DEVICE_ID_INTEL_FALCON_RIDGE_2C_BRIDGE &&
> +	    sw->config.device_id != PCI_DEVICE_ID_INTEL_FALCON_RIDGE_4C_BRIDGE)
> +		tb_sw_warn(sw, "unsupported switch device id %#x\n",
> +			   sw->config.device_id);

I'm wondering if this makes sense anymore, as said we should try to avoid
having to amend device lists in multiple places in the driver for each
newly introduced controller.


> +static void tb_switch_set_uuid(struct tb_switch *sw)
> +{
> +	u32 uuid[4];
> +	int cap;
> +
> +	if (sw->uuid)
> +		return;

When can this be nonzero?


> +
> +	/*
> +	 * By default the UUID will be based on UID where upper two
> +	 * dwords are filled with ones.
> +	 */
> +	uuid[0] = sw->uid & 0xffffffff;
> +	uuid[1] = (sw->uid >> 32) & 0xffffffff;
> +	uuid[2] = 0xffffffff;
> +	uuid[3] = 0xffffffff;
> +
> +	/*
> +	 * The newer controllers include fused UUID as part of link
> +	 * controller specific registers
> +	 */
> +	cap = tb_switch_find_vsec_cap(sw, TB_VSEC_CAP_LINK_CONTROLLER);
> +	if (cap > 0)
> +		tb_sw_read(sw, uuid, TB_CFG_SWITCH, cap + 3, 4);
> +
> +	sw->uuid = kmemdup(uuid, sizeof(uuid), GFP_KERNEL);

Hm, so on newer controller the uuid is calculated and the result is
subsequently overwritten?  Meh... :-/

On Macs, the UUID is conveyed in an EFI device property.

How about putting the VSEC code path first, then fall back to calculating
the default UUID?  Apart from being prettier, this would allow me to
easily add the Mac-specific code path.

BTW, why is there a uid for each switch and a UUID on top?
IIUC, if the switch is the root switch, then the UUID is used to
uniquely identify the domain starting at that switch, is that correct?
Actually just using the 64-bit uid would still suffice for that use case.
That is something I've never really comprehended.

Thanks,

Lukas

[toc] | [prev] | [next] | [standalone]

#1649517

On Wed, May 24, 2017 at 01:09:08PM +0200, Lukas Wunner wrote:
> On Thu, May 18, 2017 at 05:38:57PM +0300, Mika Westerberg wrote:
> > Thunderbolt domain consists of switches that are connected to each
> > other, forming a bus. This will convert each switch into a real Linux
> > device structure and adds them to the domain. The advantage here is
> > that we get all the goodies from the driver core, like reference
> > counting and sysfs hierarchy for free.
> 
> I'm wondering, would it make sense to also model each port of a switch
> as a device?
> 
> With your patches, the hierarchy looks something like this:
> nhi_pci_dev/domain0/switch0/switch1/switch2
> 
> If each port is modeled as a device, we'd get something like this:
> nhi_pci_dev/domain0/switch0/port1/switch1/port3/switch2
> 
> I think with the controllers that shipped, there can be up to two
> child switches below a switch.  If ports are not modeled as devices,
> you can't tell which port a switch is connected to.

Yes you can - it is part of the route string that we use here as bus
address. Each "hop" there is the port number through the switch is
accessed. For example I have here 4 devices connected:

$ ls -1 /sys/bus/thunderbolt/devices/
0-0
0-1
0-1040301
0-301
0-40301
domain0

> Also, if ports are modeled as devices we'd be able to store attributes
> such as port type in sysfs.  Of course we could also store those in
> each switch directory as "port0", "port1", ... files.

Is there any reason adding these? It will not help the user to identify
the connected device and I don't see how that information could be
useful.

That kind of information could go to debugfs, though.

> I don't know if this makes sense, but a discussion about the pros
> and cons is probably warranted.  IIUC once the patches go in, the
> layout is set in stone because it's a user-space API.

Yes, that's right. Also that is one of the reasons why this series tries
to keep the attribute amout minimum and their contents simple.

> > +What:		/sys/bus/thunderbolt/devices/.../device
> > +Date:		Sep 2017
> > +KernelVersion:	4.13
> > +Contact:	thunderbolt-software@lists.01.org
> > +Description:	This attribute contains id of this device extracted from
> > +		the device DROM.
> 
> Hm, why did you choose to expose the DROM vendor + device ID instead of
> the one in switch config space?

That is the actual vendor/device who made the thing and can be used to
identify the actual device as well.

> The vendor on my MacBookPro9,1 is just 0x0001 and the device is 0x000a.
> Is this valid?  Does Intel assign the vendor IDs, i.e. 0x0001 = Apple?

Apple vendor ID in Thunderbolt realm is 0x0001. Yes, Intel assigns
those.

> > +	if (sw->config.vendor_id != PCI_VENDOR_ID_INTEL)
> > +		tb_sw_warn(sw, "unknown switch vendor id %#x\n",
> > +			   sw->config.vendor_id);
> > +
> > +	if (sw->config.device_id != PCI_DEVICE_ID_INTEL_LIGHT_RIDGE &&
> > +	    sw->config.device_id != PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_4C &&
> > +	    sw->config.device_id != PCI_DEVICE_ID_INTEL_PORT_RIDGE &&
> > +	    sw->config.device_id != PCI_DEVICE_ID_INTEL_FALCON_RIDGE_2C_BRIDGE &&
> > +	    sw->config.device_id != PCI_DEVICE_ID_INTEL_FALCON_RIDGE_4C_BRIDGE)
> > +		tb_sw_warn(sw, "unsupported switch device id %#x\n",
> > +			   sw->config.device_id);
> 
> I'm wondering if this makes sense anymore, as said we should try to avoid
> having to amend device lists in multiple places in the driver for each
> newly introduced controller.

I agree. In the next version I'm moving this to be part of the code
where we identify the generation. So at least it will be removed from
this place.

> > +static void tb_switch_set_uuid(struct tb_switch *sw)
> > +{
> > +	u32 uuid[4];
> > +	int cap;
> > +
> > +	if (sw->uuid)
> > +		return;
> 
> When can this be nonzero?

On Macs with older controller than AR (no ICM running).

> > +
> > +	/*
> > +	 * By default the UUID will be based on UID where upper two
> > +	 * dwords are filled with ones.
> > +	 */
> > +	uuid[0] = sw->uid & 0xffffffff;
> > +	uuid[1] = (sw->uid >> 32) & 0xffffffff;
> > +	uuid[2] = 0xffffffff;
> > +	uuid[3] = 0xffffffff;
> > +
> > +	/*
> > +	 * The newer controllers include fused UUID as part of link
> > +	 * controller specific registers
> > +	 */
> > +	cap = tb_switch_find_vsec_cap(sw, TB_VSEC_CAP_LINK_CONTROLLER);
> > +	if (cap > 0)
> > +		tb_sw_read(sw, uuid, TB_CFG_SWITCH, cap + 3, 4);
> > +
> > +	sw->uuid = kmemdup(uuid, sizeof(uuid), GFP_KERNEL);
> 
> Hm, so on newer controller the uuid is calculated and the result is
> subsequently overwritten?  Meh... :-/

ICM gives the UUID as part of the device connected message. This here to
make the UUID working also on systems withouth ICM (older Macs). The
special case comes from the older devices where there is no fused UUID
so we generate one based on UID instead following what ICM does.

This allows us to show "unique_id" attribute the same way on all
systems.

> On Macs, the UUID is conveyed in an EFI device property.

Yes, but only for the host and UUID is actually coming from the fuses
(hardware), not from EFI property.

> How about putting the VSEC code path first, then fall back to calculating
> the default UUID?  Apart from being prettier, this would allow me to
> easily add the Mac-specific code path.

I can change the ordering but you don't need to add Mac specific path
there - this code has been tested on Macs already and it should work
exactly the same there.

I have here Mac Mini (Light Ridge), Macbook (Cactus Ridge) and newer
Macbook with Alpine Ridge and I've tried to make sure it works the same.

Below is taken from the same devices connected to a Cactus Ridge based
Mac.

0-0/authorized:1
0-0/device:0xa
0-0/device_name:Macintosh
0-0/uevent:DEVTYPE=thunderbolt_device
0-0/unique_id:00000000-0000-0008-83b3-30099bc1194a
0-0/vendor:0x1
0-0/vendor_name:Apple, Inc.
0-1/authorized:1
0-1/device:0x5
0-1/device_name:34UC97
0-1/uevent:DEVTYPE=thunderbolt_device
0-1/unique_id:00000000-0000-0018-0022-c32682b35855
0-1/vendor:0x1e
0-1/vendor_name:LG Electronics
0-1030301/authorized:1
0-1030301/device:0x301
0-1030301/device_name:AKiTiO Thunder3 Duo Pro
0-1030301/nvm_authenticate:0x0
0-1030301/nvm_version:16.0
0-1030301/uevent:DEVTYPE=thunderbolt_device
0-1030301/unique_id:d3010000-0000-9518-a29c-9bc5cca35116
0-1030301/vendor:0x41
0-1030301/vendor_name:inXtron
0-301/authorized:1
0-301/device:0x3
0-301/device_name:eSata Hub
0-301/uevent:DEVTYPE=thunderbolt_device
0-301/unique_id:102f9d1e-0000-0300-ffff-ffffffffffff
0-301/vendor:0x3
0-301/vendor_name:LaCie
0-30301/authorized:1
0-30301/device:0x305
0-30301/device_name:AKiTiO Thunder3 PCIe Box
0-30301/nvm_authenticate:0x0
0-30301/nvm_version:19.0
0-30301/uevent:DEVTYPE=thunderbolt_device
0-30301/unique_id:dc010000-0000-8508-a22d-32ca6421cb16
0-30301/vendor:0x41
0-30301/vendor_name:inXtron
domain0/security:none
domain0/uevent:DEVTYPE=thunderbolt_domain

> BTW, why is there a uid for each switch and a UUID on top?

I think the UUID came with Redwood Ridge or so so before that there was
only UID.

> IIUC, if the switch is the root switch, then the UUID is used to
> uniquely identify the domain starting at that switch, is that correct?

Yes, that's correct.

> Actually just using the 64-bit uid would still suffice for that use case.
> That is something I've never really comprehended.

Well they decided to go with full UUID and I can't blame them ;-)

[toc] | [prev] | [next] | [standalone]

#1649644

On Wed, May 24, 2017 at 02:43:22PM +0300, Mika Westerberg wrote:
> On Wed, May 24, 2017 at 01:09:08PM +0200, Lukas Wunner wrote:
> > On Thu, May 18, 2017 at 05:38:57PM +0300, Mika Westerberg wrote:
> > > Thunderbolt domain consists of switches that are connected to each
> > > other, forming a bus. This will convert each switch into a real Linux
> > > device structure and adds them to the domain. The advantage here is
> > > that we get all the goodies from the driver core, like reference
> > > counting and sysfs hierarchy for free.
> > 
> > I'm wondering, would it make sense to also model each port of a switch
> > as a device?
> > 
> > With your patches, the hierarchy looks something like this:
> > nhi_pci_dev/domain0/switch0/switch1/switch2
> > 
> > If each port is modeled as a device, we'd get something like this:
> > nhi_pci_dev/domain0/switch0/port1/switch1/port3/switch2
> > 
> > I think with the controllers that shipped, there can be up to two
> > child switches below a switch.  If ports are not modeled as devices,
> > you can't tell which port a switch is connected to.
> 
> Yes you can - it is part of the route string that we use here as bus
> address. Each "hop" there is the port number through the switch is
> accessed. For example I have here 4 devices connected:
> 
> $ ls -1 /sys/bus/thunderbolt/devices/
> 0-0
> 0-1
> 0-1040301
> 0-301
> 0-40301
> domain0

I see.

Is it possible to determine for a given port of type PCIe to which
downstream bridge it belongs?

E.g. on my Light Ridge, the PCIe ports are numbered 7, 8, 9, 10.
The downstream bridges on the controller are numbered 03, 04, 05, 06.
But the ordering seems to be mixed up, e.g. port 7 corresponds to
downstream bridge 0000:06:04.0.  Not to 03, as one would expect.
                          ^^

Can this perhaps be determined from config space?

If we knew the correlation between Thunderbolt PCIe port and downstream
bridge, we could provide a symlink in sysfs from the Thunderbolt bus
to the PCI bus.  E.g. in the switch directory for a plugged in device,
there would be a symlink to the corresponding upstream bridge.  For the
root switch, this would be the upstream bridge of the host controller.


> > Also, if ports are modeled as devices we'd be able to store attributes
> > such as port type in sysfs.  Of course we could also store those in
> > each switch directory as "port0", "port1", ... files.
> 
> Is there any reason adding these? It will not help the user to identify
> the connected device and I don't see how that information could be
> useful.
>
> That kind of information could go to debugfs, though.

Well, currently we print that stuff to syslog and it's valuable to
understand what's going on.  Being able to check e.g. the type of
a port on a running system would be even better.  I'm not saying
we should model ports as devices, I just want to have a discussion
about the pros and cons.


> > On Macs, the UUID is conveyed in an EFI device property.
> 
> Yes, but only for the host and UUID is actually coming from the fuses
> (hardware), not from EFI property.

Nope, on Macs the UUID is calculated by sha1-hashing a constant, then
extending that by sha1-hashing the uid, then truncating the result to
16 bytes.

The uid in turn is calculated by combining a 16-bit constant with a
24-bit model-specific number and a 24-bit serial number.

This is done by the EFI NHI driver.  No fuses involved.

(Okay the serial number is coming from an EEPROM, but not that of
the Thunderbolt controller).


> > > +
> > > +	/*
> > > +	 * By default the UUID will be based on UID where upper two
> > > +	 * dwords are filled with ones.
> > > +	 */
> > > +	uuid[0] = sw->uid & 0xffffffff;
> > > +	uuid[1] = (sw->uid >> 32) & 0xffffffff;
> > > +	uuid[2] = 0xffffffff;
> > > +	uuid[3] = 0xffffffff;
> > > +
> > > +	/*
> > > +	 * The newer controllers include fused UUID as part of link
> > > +	 * controller specific registers
> > > +	 */
> > > +	cap = tb_switch_find_vsec_cap(sw, TB_VSEC_CAP_LINK_CONTROLLER);
> > > +	if (cap > 0)
> > > +		tb_sw_read(sw, uuid, TB_CFG_SWITCH, cap + 3, 4);
> > > +
> > > +	sw->uuid = kmemdup(uuid, sizeof(uuid), GFP_KERNEL);
> > 
> > Hm, so on newer controller the uuid is calculated and the result is
> > subsequently overwritten?  Meh... :-/
> 
> ICM gives the UUID as part of the device connected message. This here to
> make the UUID working also on systems withouth ICM (older Macs). The
> special case comes from the older devices where there is no fused UUID
> so we generate one based on UID instead following what ICM does.
> 
> This allows us to show "unique_id" attribute the same way on all
> systems.

I'll have to double check if the macOS NHI driver calculates a UUID for
each switch, and how it does that.  We should try to be compatible.


> > How about putting the VSEC code path first, then fall back to calculating
> > the default UUID?  Apart from being prettier, this would allow me to
> > easily add the Mac-specific code path.
> 
> I can change the ordering but you don't need to add Mac specific path
> there - this code has been tested on Macs already and it should work
> exactly the same there.

I don't doubt that it works, the problem is that the UUID should be
identical to what macOS uses.


> Below is taken from the same devices connected to a Cactus Ridge based
> Mac.
> 
> 0-0/authorized:1
> 0-0/device:0xa
> 0-0/device_name:Macintosh
> 0-0/uevent:DEVTYPE=thunderbolt_device
> 0-0/unique_id:00000000-0000-0008-83b3-30099bc1194a
> 0-0/vendor:0x1
> 0-0/vendor_name:Apple, Inc.

Interesting, however this means that the device ID is identical across
all Macs.  (0xa, same as on my MacBookPro9,1 w/ Light Ridge.)


> > IIUC, if the switch is the root switch, then the UUID is used to
> > uniquely identify the domain starting at that switch, is that correct?
> 
> Yes, that's correct.

Why is a UUID calculated for each switch on the chain even if only the
UUID on the root switch is needed to give the domain a unique ID?

Thanks,

Lukas

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web