Skip to contentSkip to conversation

Thunderbolt security levels and NVM firmware upgrade

PATCH Superseded Mika Westerberg 7 participants 56 messages Jun 2, 2017 – Jun 6, 2017

Subsystems (2)

v3 of 4 Superseded Latest is v4 →

lore
Share & tools
lore
#0 lore raw

v3 cover Thunderbolt security levels and NVM firmware upgrade

Hi,

This is a third version of the patch series adding support for Thunderbolt security levels and NVM firmware upgrade. PCs running Intel Falcon Ridge or newer need these in order to connect devices if the security level is set to "user(SL1) or secure(SL2)" from BIOS.

The security levels were added to to prevent DMA attacks when PCIe is tunneled over Thunderbolt fabric where IOMMU is not available or cannot be enabled for different reasons.

This series converts the Thunderbolt driver to a Linux bus and makes each Thunderbolt device (aka Thunderbolt switch or router or endpoint) a Linux device which is connected to that bus addressed by their routing string (this is the addressing Thunderbolt uses).

With these patches the user can authorize devices using sysfs attributes like (following USB):

  # echo 1 > /sys/bus/thunderbolt/devices/0-1/authorized

If the BIOS security level is set to "secure" and the device supports secure connect on the first connect the device is uploaded a random key:

# key=$(openssl rand -hex 32)

  # echo $key > /sys/bus/thunderbolt/devices/0-1/key
  # echo 1 > /sys/bus/thunderbolt/devices/0-1/authorized

Next time the device is plugged in the user can verify (challenge) the device using the same key:

  # echo $key > /sys/bus/thunderbolt/devices/0-1/key
  # echo 2 > /sys/bus/thunderbolt/devices/0-1/authorized

The device identification information is found below each device sysfs node and includes following attributes:

device - Device ID
device_name - Name of the device
vendor - Vendor ID
vendor_name - Name of the vendor
unique_id - Unique ID string identifying this particular device

In addition these patches add support for upgrading NVM firmware running on a host or device by running something like:

  # dd if=KYK_TBT_FW_0018.bin of=/sys/bus/thunderbolt/devices/0-0/nvm_non_active0/nvmem
  # echo 1 > /sys/bus/thunderbolt/devices/0-0/nvm_authenticate

More information how to use the Thunderbolt bus can be found in patch [26/27].

Where Internal Connection Manager (ICM) firmware is available and usable, we use it in the driver. This also includes newer Apple Macbooks with Alpine Ridge. For older Macbooks the driver works as before but in addition the Thunderbolt bus is available there as well (including possibility to upgrade NVM firmware of connected devices).

Note for Macs the Linux native PCIe hotplug support does not work well with the Thunderbolt PCIe topologies where there is need to put all available resources to the PCIe downstream port where the PCIe chain is extended. This is something we need to fix. In the mean time is a way to work it around by passing "pci=hpbussize=10,hpmemsize=2M" or so to the kernel command line.

Regarding Christoph's UUID series here [1], they now include the existing types for backwards compatibility so this series compiles and works fine with them. We will convert the Thunderbolt driver to use these new types once that series hits mainline.

Andreas and Lukas, do you have any comments or concerns regarding this series?

We are currently porting Amir's network driver on top of the Thunderbolt bus.

The previous version (v2) can be found in [2].

[1] http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/uuid-types
[2] https://lwn.net/Articles/723935/

Changes from the previos version (v2):

[PATCH v2 03/27] thunderbolt: Do not try to read UID if DROM offset is read as 0
- Add Suggested-by Andreas

[PATCH v2 07/27] thunderbolt: Allow passing NULL to tb_ctl_free()
- Add Suggested-by Andy

[PATCH v2 09/27] thunderbolt: Convert switch to a device
- Rework comment regarding UUID to mention that it is not strictly following rules of UUID format.
- Update Documentation/ABI/testing/sysfs-bus-thunderbolt unique_id attribute description accordingly.

[PATCH v2 12/27] thunderbolt: Refactor and fix parsing of port drom entries
- Add Reviewed-by Andy

[PATCH v2 13/27] thunderbolt: Read vendor and device name from DROM
- Use ksrtndup() instead and drop parse_name()

[PATCH v2 21/27] thunderbolt: Store Thunderbolt generation in the switch structure
- Make the unknown switch defaulting to generation 1 more explicit
- Add Reviewed-by Andy

[PATCH v2 24/27] thunderbolt: Add support for Internal Connection Manager (ICM)
- Allow secure connect only when BIOS setting is in "secure connect/SL2" to follow what Windows does.

[PATCH v2 25/27] thunderbolt: Add support for host and device NVM firmware upgrade
- Add check for sw->nvm in nvm_authenticate_store() to prevent NULL pointer dereference when NVMem devices are not yet added.

[PATCH v2 26/27] thunderbolt: Add documentation how Thunderbolt bus can be used
- Update documentation to mention that secure connect is only available when the BIOS setting is in "secure connect/SL2".

Lukas Wunner (1):
thunderbolt: Refactor and fix parsing of port drom entries

Mika Westerberg (26):
thunderbolt: Use const buffer pointer in write operations
thunderbolt: No need to read UID of the root switch on resume
thunderbolt: Do not try to read UID if DROM offset is read as 0
thunderbolt: Do not warn about newer DROM versions
thunderbolt: Add MSI-X support
thunderbolt: Rework capability handling
thunderbolt: Allow passing NULL to tb_ctl_free()
thunderbolt: Introduce thunderbolt bus and connection manager
thunderbolt: Convert switch to a device
thunderbolt: Fail switch adding operation if reading DROM fails
thunderbolt: Do not fail if DROM data CRC32 is invalid
thunderbolt: Read vendor and device name from DROM
thunderbolt: Move control channel messages to tb_msgs.h
thunderbolt: Expose get_route() to other files
thunderbolt: Expose make_header() to other files
thunderbolt: Let the connection manager handle all notifications
thunderbolt: Rework control channel to be more reliable
thunderbolt: Add new Thunderbolt PCI IDs
thunderbolt: Add support for NHI mailbox
thunderbolt: Store Thunderbolt generation in the switch structure
thunderbolt: Add support for DMA configuration based mailbox
thunderbolt: Do not touch the hardware if the NHI is gone on resume
thunderbolt: Add support for Internal Connection Manager (ICM)
thunderbolt: Add support for host and device NVM firmware upgrade
thunderbolt: Add documentation how Thunderbolt bus can be used
MAINTAINERS: Add maintainers for Thunderbolt driver

 Documentation/ABI/testing/sysfs-bus-thunderbolt |  110 +++
 Documentation/admin-guide/index.rst             |    1 +
 Documentation/admin-guide/thunderbolt.rst       |  199 ++++
 MAINTAINERS                                     |    3 +
 drivers/thunderbolt/Kconfig                     |   13 +-
 drivers/thunderbolt/Makefile                    |    2 +-
 drivers/thunderbolt/cap.c                       |  169 ++--
 drivers/thunderbolt/ctl.c                       |  665 +++++++++----
 drivers/thunderbolt/ctl.h                       |  105 +-
 drivers/thunderbolt/dma_port.c                  |  524 ++++++++++
 drivers/thunderbolt/dma_port.h                  |   34 +
 drivers/thunderbolt/domain.c                    |  455 +++++++++
 drivers/thunderbolt/eeprom.c                    |  119 ++-
 drivers/thunderbolt/icm.c                       | 1089 +++++++++++++++++++++
 drivers/thunderbolt/nhi.c                       |  306 +++++-
 drivers/thunderbolt/nhi.h                       |   93 +-
 drivers/thunderbolt/nhi_regs.h                  |   27 +
 drivers/thunderbolt/switch.c                    | 1178 +++++++++++++++++++++--
 drivers/thunderbolt/tb.c                        |  237 +++--
 drivers/thunderbolt/tb.h                        |  251 ++++-
 drivers/thunderbolt/tb_msgs.h                   |  260 +++++
 drivers/thunderbolt/tb_regs.h                   |   50 +-
 drivers/thunderbolt/tunnel_pci.c                |   17 +-
 23 files changed, 5323 insertions(+), 584 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-thunderbolt
 create mode 100644 Documentation/admin-guide/thunderbolt.rst
 create mode 100644 drivers/thunderbolt/dma_port.c
 create mode 100644 drivers/thunderbolt/dma_port.h
 create mode 100644 drivers/thunderbolt/domain.c
 create mode 100644 drivers/thunderbolt/icm.c
 create mode 100644 drivers/thunderbolt/tb_msgs.h
Signature · 1 line
2.11.0
#1 lore raw

PATCH v3 1/27 thunderbolt: Use const buffer pointer in write operations

These functions should not (and do not) modify the argument in any way so make it const.

---
 drivers/thunderbolt/ctl.c | 8 ++++----
 drivers/thunderbolt/ctl.h | 4 ++--
 drivers/thunderbolt/tb.h  | 2 +-
 3 files changed, 7 insertions(+), 7 deletions(-)
drivers/thunderbolt/ctl.c +4 -4
index 1146ff4210a9..1031d97407a8 100644
@@ -273,7 +273,7 @@ static void tb_cfg_print_error(struct tb_ctl *ctl,
 	}
 }
 
-static void cpu_to_be32_array(__be32 *dst, u32 *src, size_t len)
+static void cpu_to_be32_array(__be32 *dst, const u32 *src, size_t len)
 {
 	int i;
 	for (i = 0; i < len; i++)
@@ -333,7 +333,7 @@ static void tb_ctl_tx_callback(struct tb_ring *ring, struct ring_frame *frame,
  *
  * Return: Returns 0 on success or an error code on failure.
  */
-static int tb_ctl_tx(struct tb_ctl *ctl, void *data, size_t len,
+static int tb_ctl_tx(struct tb_ctl *ctl, const void *data, size_t len,
 		     enum tb_cfg_pkg_type type)
 {
 	int res;
@@ -650,7 +650,7 @@ struct tb_cfg_result tb_cfg_read_raw(struct tb_ctl *ctl, void *buffer,
  *
  * Offset and length are in dwords.
  */
-struct tb_cfg_result tb_cfg_write_raw(struct tb_ctl *ctl, void *buffer,
+struct tb_cfg_result tb_cfg_write_raw(struct tb_ctl *ctl, const void *buffer,
 		u64 route, u32 port, enum tb_cfg_space space,
 		u32 offset, u32 length, int timeout_msec)
 {
@@ -695,7 +695,7 @@ int tb_cfg_read(struct tb_ctl *ctl, void *buffer, u64 route, u32 port,
 	return res.err;
 }
 
-int tb_cfg_write(struct tb_ctl *ctl, void *buffer, u64 route, u32 port,
+int tb_cfg_write(struct tb_ctl *ctl, const void *buffer, u64 route, u32 port,
 		 enum tb_cfg_space space, u32 offset, u32 length)
 {
 	struct tb_cfg_result res = tb_cfg_write_raw(ctl, buffer, route, port,
drivers/thunderbolt/ctl.h +2 -2
index ba87d6e731dd..83ae54947082 100644
@@ -61,13 +61,13 @@ struct tb_cfg_result tb_cfg_read_raw(struct tb_ctl *ctl, void *buffer,
 				     u64 route, u32 port,
 				     enum tb_cfg_space space, u32 offset,
 				     u32 length, int timeout_msec);
-struct tb_cfg_result tb_cfg_write_raw(struct tb_ctl *ctl, void *buffer,
+struct tb_cfg_result tb_cfg_write_raw(struct tb_ctl *ctl, const void *buffer,
 				      u64 route, u32 port,
 				      enum tb_cfg_space space, u32 offset,
 				      u32 length, int timeout_msec);
 int tb_cfg_read(struct tb_ctl *ctl, void *buffer, u64 route, u32 port,
 		enum tb_cfg_space space, u32 offset, u32 length);
-int tb_cfg_write(struct tb_ctl *ctl, void *buffer, u64 route, u32 port,
+int tb_cfg_write(struct tb_ctl *ctl, const void *buffer, u64 route, u32 port,
 		 enum tb_cfg_space space, u32 offset, u32 length);
 int tb_cfg_get_upstream_port(struct tb_ctl *ctl, u64 route);
 
drivers/thunderbolt/tb.h +1 -1
index 61d57ba64035..ba2b85750335 100644
@@ -173,7 +173,7 @@ static inline int tb_port_read(struct tb_port *port, void *buffer,
 			   length);
 }
 
-static inline int tb_port_write(struct tb_port *port, void *buffer,
+static inline int tb_port_write(struct tb_port *port, const void *buffer,
 				enum tb_cfg_space space, u32 offset, u32 length)
 {
 	return tb_cfg_write(port->sw->tb->ctl,
Signature · 1 line
2.11.0
#2 lore raw

PATCH v3 2/27 thunderbolt: No need to read UID of the root switch on resume

In reply to Mika Westerberg

The root switch is part of the host controller and cannot be physically removed, so there is no point of reading UID again on resume in order to check if the root switch is still the same.

---
 drivers/thunderbolt/switch.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)
drivers/thunderbolt/switch.c +18 -11
index c6f30b1695a9..81f5164a6364 100644
@@ -452,19 +452,26 @@ void tb_sw_set_unplugged(struct tb_switch *sw)
 int tb_switch_resume(struct tb_switch *sw)
 {
 	int i, err;
-	u64 uid;
 	tb_sw_info(sw, "resuming switch\n");
 
-	err = tb_drom_read_uid_only(sw, &uid);
-	if (err) {
-		tb_sw_warn(sw, "uid read failed\n");
-		return err;
-	}
-	if (sw != sw->tb->root_switch && sw->uid != uid) {
-		tb_sw_info(sw,
-			"changed while suspended (uid %#llx -> %#llx)\n",
-			sw->uid, uid);
-		return -ENODEV;
+	/*
+	 * Check for UID of the connected switches except for root
+	 * switch which we assume cannot be removed.
+	 */
+	if (tb_route(sw)) {
+		u64 uid;
+
+		err = tb_drom_read_uid_only(sw, &uid);
+		if (err) {
+			tb_sw_warn(sw, "uid read failed\n");
+			return err;
+		}
+		if (sw->uid != uid) {
+			tb_sw_info(sw,
+				"changed while suspended (uid %#llx -> %#llx)\n",
+				sw->uid, uid);
+			return -ENODEV;
+		}
 	}
 
 	/* upload configuration */
Signature · 1 line
2.11.0
#3 lore raw

PATCH v3 3/27 thunderbolt: Do not try to read UID if DROM offset is read as 0

In reply to Mika Westerberg

At least Falcon Ridge when in host mode does not have any kind of DROM available and reading DROM offset returns 0 for these. Do not try to read DROM any further in that case.

---
 drivers/thunderbolt/eeprom.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/thunderbolt/eeprom.c +3 -0
index 6392990c984d..e4e64b130514 100644
@@ -276,6 +276,9 @@ int tb_drom_read_uid_only(struct tb_switch *sw, u64 *uid)
 	if (res)
 		return res;
 
+	if (drom_offset == 0)
+		return -ENODEV;
+
 	/* read uid */
 	res = tb_eeprom_read_n(sw, drom_offset, data, 9);
 	if (res)
Signature · 1 line
2.11.0
#4 lore raw

PATCH v3 4/27 thunderbolt: Do not warn about newer DROM versions

In reply to Mika Westerberg

DROM version 2 is compatible with the previous generation so no need to warn about that.

---
 drivers/thunderbolt/eeprom.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/thunderbolt/eeprom.c +1 -1
index e4e64b130514..eb2179c98b09 100644
@@ -488,7 +488,7 @@ int tb_drom_read(struct tb_switch *sw)
 		goto err;
 	}
 
-	if (header->device_rom_revision > 1)
+	if (header->device_rom_revision > 2)
 		tb_sw_warn(sw, "drom device_rom_revision %#x unknown\n",
 			header->device_rom_revision);
 
Signature · 1 line
2.11.0
#5 lore raw

PATCH v3 5/27 thunderbolt: Add MSI-X support

In reply to Mika Westerberg

Intel Thunderbolt controllers support up to 16 MSI-X vectors. Using MSI-X is preferred over MSI or legacy interrupt and may bring additional performance because there is no need to check the status registers which interrupt was triggered.

While there we convert comments in structs tb_ring and tb_nhi to follow kernel-doc format more closely.

This code is based on the work done by Amir Levy and Michael Jamet.

---
 drivers/thunderbolt/ctl.c      |   4 +-
 drivers/thunderbolt/nhi.c      | 165 +++++++++++++++++++++++++++++++++++------
 drivers/thunderbolt/nhi.h      |  56 +++++++++++---
 drivers/thunderbolt/nhi_regs.h |   9 +++
 4 files changed, 198 insertions(+), 36 deletions(-)
drivers/thunderbolt/ctl.c +2 -2
index 1031d97407a8..889a32dd21e7 100644
@@ -488,11 +488,11 @@ struct tb_ctl *tb_ctl_alloc(struct tb_nhi *nhi, hotplug_cb cb, void *cb_data)
 	if (!ctl->frame_pool)
 		goto err;
 
-	ctl->tx = ring_alloc_tx(nhi, 0, 10);
+	ctl->tx = ring_alloc_tx(nhi, 0, 10, RING_FLAG_NO_SUSPEND);
 	if (!ctl->tx)
 		goto err;
 
-	ctl->rx = ring_alloc_rx(nhi, 0, 10);
+	ctl->rx = ring_alloc_rx(nhi, 0, 10, RING_FLAG_NO_SUSPEND);
 	if (!ctl->rx)
 		goto err;
 
drivers/thunderbolt/nhi.c +143 -22

241 lines not on this page · raw message

drivers/thunderbolt/nhi.h +44 -12

97 lines not on this page · raw message

drivers/thunderbolt/nhi_regs.h +9 -0
index 75cf0691e6c5..48b98d3c7e6a 100644
@@ -95,7 +95,16 @@ struct ring_desc {
 #define REG_RING_INTERRUPT_BASE	0x38200
 #define RING_INTERRUPT_REG_COUNT(nhi) ((31 + 2 * nhi->hop_count) / 32)
 
+/* Interrupt Vector Allocation */
+#define REG_INT_VEC_ALLOC_BASE	0x38c40
+#define REG_INT_VEC_ALLOC_BITS	4
+#define REG_INT_VEC_ALLOC_MASK	GENMASK(3, 0)
+#define REG_INT_VEC_ALLOC_REGS	(32 / REG_INT_VEC_ALLOC_BITS)
+
 /* The last 11 bits contain the number of hops supported by the NHI port. */
 #define REG_HOP_COUNT		0x39640
 
+#define REG_DMA_MISC			0x39864
+#define REG_DMA_MISC_INT_AUTO_CLEAR     BIT(2)
+
 #endif
Signature · 1 line
2.11.0
#6 lore raw

PATCH v3 6/27 thunderbolt: Rework capability handling

In reply to Mika Westerberg

Organization of the capabilities in switches and ports is not so random after all. Rework the capability handling functionality so that it follows how capabilities are organized and provide two new functions (tb_switch_find_vsec_cap() and tb_port_find_cap()) which can be used to extract capabilities for ports and switches. Then convert the current users over these.

---
 drivers/thunderbolt/cap.c        | 169 +++++++++++++++++++++------------------
 drivers/thunderbolt/switch.c     |   6 +-
 drivers/thunderbolt/tb.c         |   8 +-
 drivers/thunderbolt/tb.h         |   3 +-
 drivers/thunderbolt/tb_regs.h    |  50 +++++++++---
 drivers/thunderbolt/tunnel_pci.c |   8 +-
 6 files changed, 142 insertions(+), 102 deletions(-)
drivers/thunderbolt/cap.c +91 -78
index a7b47e7cddbd..7260971acc4f 100644
@@ -9,6 +9,8 @@
 
 #include "tb.h"
 
+#define CAP_OFFSET_MAX		0xff
+#define VSEC_CAP_OFFSET_MAX	0xffff
 
 struct tb_cap_any {
 	union {
@@ -18,99 +20,110 @@ struct tb_cap_any {
 	};
 } __packed;
 
-static bool tb_cap_is_basic(struct tb_cap_any *cap)
-{
-	/* basic.cap is u8. This checks only the lower 8 bit of cap. */
-	return cap->basic.cap != 5;
-}
-
-static bool tb_cap_is_long(struct tb_cap_any *cap)
+/**
+ * tb_port_find_cap() - Find port capability
+ * @port: Port to find the capability for
+ * @cap: Capability to look
+ *
+ * Returns offset to start of capability or %-ENOENT if no such
+ * capability was found. Negative errno is returned if there was an
+ * error.
+ */
+int tb_port_find_cap(struct tb_port *port, enum tb_port_cap cap)
 {
-	return !tb_cap_is_basic(cap)
-	       && cap->extended_short.next == 0
-	       && cap->extended_short.length == 0;
-}
+	u32 offset;
 
-static enum tb_cap tb_cap(struct tb_cap_any *cap)
-{
-	if (tb_cap_is_basic(cap))
-		return cap->basic.cap;
+	/*
+	 * DP out adapters claim to implement TMU capability but in
+	 * reality they do not so we hard code the adapter specific
+	 * capability offset here.
+	 */
+	if (port->config.type == TB_TYPE_DP_HDMI_OUT)
+		offset = 0x39;
 	else
-		/* extended_short/long have cap at the same offset. */
-		return cap->extended_short.cap;
+		offset = 0x1;
+
+	do {
+		struct tb_cap_any header;
+		int ret;
+
+		ret = tb_port_read(port, &header, TB_CFG_PORT, offset, 1);
+		if (ret)
+			return ret;
+
+		if (header.basic.cap == cap)
+			return offset;
+
+		offset = header.basic.next;
+	} while (offset);
+
+	return -ENOENT;
 }
 
-static u32 tb_cap_next(struct tb_cap_any *cap, u32 offset)
+static int tb_switch_find_cap(struct tb_switch *sw, enum tb_switch_cap cap)
 {
-	int next;
-	if (offset == 1) {
-		/*
-		 * The first pointer is part of the switch header and always
-		 * a simple pointer.
-		 */
-		next = cap->basic.next;
-	} else {
-		/*
-		 * Somehow Intel decided to use 3 different types of capability
-		 * headers. It is not like anyone could have predicted that
-		 * single byte offsets are not enough...
-		 */
-		if (tb_cap_is_basic(cap))
-			next = cap->basic.next;
-		else if (!tb_cap_is_long(cap))
-			next = cap->extended_short.next;
-		else
-			next = cap->extended_long.next;
+	int offset = sw->config.first_cap_offset;
+
+	while (offset > 0 && offset < CAP_OFFSET_MAX) {
+		struct tb_cap_any header;
+		int ret;
+
+		ret = tb_sw_read(sw, &header, TB_CFG_SWITCH, offset, 1);
+		if (ret)
+			return ret;
+
+		if (header.basic.cap == cap)
+			return offset;
+
+		offset = header.basic.next;
 	}
-	/*
-	 * "Hey, we could terminate some capability lists with a null offset
-	 *  and others with a pointer to the last element." - "Great idea!"
-	 */
-	if (next == offset)
-		return 0;
-	return next;
+
+	return -ENOENT;
 }
 
 /**
- * tb_find_cap() - find a capability
+ * tb_switch_find_vsec_cap() - Find switch vendor specific capability
+ * @sw: Switch to find the capability for
+ * @vsec: Vendor specific capability to look
  *
- * Return: Returns a positive offset if the capability was found and 0 if not.
- * Returns an error code on failure.
+ * Functions enumerates vendor specific (VSEC) capabilities of a switch
+ * and returns offset when capability matching @vsec is found. If no
+ * such capability is found returns %-ENOENT. In case of error returns
+ * negative errno.
  */
-int tb_find_cap(struct tb_port *port, enum tb_cfg_space space, enum tb_cap cap)
+int tb_switch_find_vsec_cap(struct tb_switch *sw, enum tb_switch_vsec_cap vsec)
 {
-	u32 offset = 1;
 	struct tb_cap_any header;
-	int res;
-	int retries = 10;
-	while (retries--) {
-		res = tb_port_read(port, &header, space, offset, 1);
-		if (res) {
-			/* Intel needs some help with linked lists. */
-			if (space == TB_CFG_PORT && offset == 0xa
-			    && port->config.type == TB_TYPE_DP_HDMI_OUT) {
-				offset = 0x39;
-				continue;
-			}
-			return res;
-		}
-		if (offset != 1) {
-			if (tb_cap(&header) == cap)
+	int offset;
+
+	offset = tb_switch_find_cap(sw, TB_SWITCH_CAP_VSEC);
+	if (offset < 0)
+		return offset;
+
+	while (offset > 0 && offset < VSEC_CAP_OFFSET_MAX) {
+		int ret;
+
+		ret = tb_sw_read(sw, &header, TB_CFG_SWITCH, offset, 2);
+		if (ret)
+			return ret;
+
+		/*
+		 * Extended vendor specific capabilities come in two
+		 * flavors: short and long. The latter is used when
+		 * offset is over 0xff.
+		 */
+		if (offset >= CAP_OFFSET_MAX) {
+			if (header.extended_long.vsec_id == vsec)
 				return offset;
-			if (tb_cap_is_long(&header)) {
-				/* tb_cap_extended_long is 2 dwords */
-				res = tb_port_read(port, &header, space,
-						   offset, 2);
-				if (res)
-					return res;
-			}
+			offset = header.extended_long.next;
+		} else {
+			if (header.extended_short.vsec_id == vsec)
+				return offset;
+			if (!header.extended_short.length)
+				return -ENOENT;
+			offset = header.extended_short.next;
 		}
-		offset = tb_cap_next(&header, offset);
-		if (!offset)
-			return 0;
 	}
-	tb_port_WARN(port,
-		     "run out of retries while looking for cap %#x in config space %d, last offset: %#x\n",
-		     cap, space, offset);
-	return -EIO;
+
+	return -ENOENT;
 }
drivers/thunderbolt/switch.c +3 -3
index 81f5164a6364..c02079424d0e 100644
@@ -192,7 +192,7 @@ static int tb_init_port(struct tb_port *port)
 
 	/* Port 0 is the switch itself and has no PHY. */
 	if (port->config.type == TB_TYPE_PORT && port->port != 0) {
-		cap = tb_find_cap(port, TB_CFG_PORT, TB_CAP_PHY);
+		cap = tb_port_find_cap(port, TB_PORT_CAP_PHY);
 
 		if (cap > 0)
 			port->cap_phy = cap;
@@ -394,9 +394,9 @@ struct tb_switch *tb_switch_alloc(struct tb *tb, u64 route)
 		sw->ports[i].port = i;
 	}
 
-	cap = tb_find_cap(&sw->ports[0], TB_CFG_SWITCH, TB_CAP_PLUG_EVENTS);
+	cap = tb_switch_find_vsec_cap(sw, TB_VSEC_CAP_PLUG_EVENTS);
 	if (cap < 0) {
-		tb_sw_warn(sw, "cannot find TB_CAP_PLUG_EVENTS aborting\n");
+		tb_sw_warn(sw, "cannot find TB_VSEC_CAP_PLUG_EVENTS aborting\n");
 		goto err;
 	}
 	sw->cap_plug_events = cap;
drivers/thunderbolt/tb.c +4 -4
index 24b6d30c3c86..6b44076e1380 100644
@@ -121,8 +121,8 @@ static struct tb_port *tb_find_unused_down_port(struct tb_switch *sw)
 			continue;
 		if (sw->ports[i].config.type != TB_TYPE_PCIE_DOWN)
 			continue;
-		cap = tb_find_cap(&sw->ports[i], TB_CFG_PORT, TB_CAP_PCIE);
-		if (cap <= 0)
+		cap = tb_port_find_cap(&sw->ports[i], TB_PORT_CAP_ADAP);
+		if (cap < 0)
 			continue;
 		res = tb_port_read(&sw->ports[i], &data, TB_CFG_PORT, cap, 1);
 		if (res < 0)
@@ -165,8 +165,8 @@ static void tb_activate_pcie_devices(struct tb *tb)
 		}
 
 		/* check whether port is already activated */
-		cap = tb_find_cap(up_port, TB_CFG_PORT, TB_CAP_PCIE);
-		if (cap <= 0)
+		cap = tb_port_find_cap(up_port, TB_PORT_CAP_ADAP);
+		if (cap < 0)
 			continue;
 		if (tb_port_read(up_port, &data, TB_CFG_PORT, cap, 1))
 			continue;
drivers/thunderbolt/tb.h +2 -1
index ba2b85750335..9b60e71562dc 100644
@@ -233,7 +233,8 @@ 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);
 
-int tb_find_cap(struct tb_port *port, enum tb_cfg_space space, enum tb_cap cap);
+int tb_switch_find_vsec_cap(struct tb_switch *sw, enum tb_switch_vsec_cap vsec);
+int tb_port_find_cap(struct tb_port *port, enum tb_port_cap cap);
 
 struct tb_path *tb_path_alloc(struct tb *tb, int num_hops);
 void tb_path_free(struct tb_path *path);
drivers/thunderbolt/tb_regs.h +38 -12

68 lines not on this page · raw message

drivers/thunderbolt/tunnel_pci.c +4 -4
index baf1cd370446..f4ce9845e42a 100644
@@ -147,10 +147,10 @@ bool tb_pci_is_invalid(struct tb_pci_tunnel *tunnel)
 static int tb_pci_port_active(struct tb_port *port, bool active)
 {
 	u32 word = active ? 0x80000000 : 0x0;
-	int cap = tb_find_cap(port, TB_CFG_PORT, TB_CAP_PCIE);
-	if (cap <= 0) {
-		tb_port_warn(port, "TB_CAP_PCIE not found: %d\n", cap);
-		return cap ? cap : -ENXIO;
+	int cap = tb_port_find_cap(port, TB_PORT_CAP_ADAP);
+	if (cap < 0) {
+		tb_port_warn(port, "TB_PORT_CAP_ADAP not found: %d\n", cap);
+		return cap;
 	}
 	return tb_port_write(port, &word, TB_CFG_PORT, cap, 1);
 }
Signature · 1 line
2.11.0
#7 lore raw

Re: [PATCH v3 06/27] thunderbolt: Rework capability handling

Quoted 4 lines · Mika Westerberg

Organization of the capabilities in switches and ports is not so random after all. Rework the capability handling functionality so that it follows how capabilities are organized and provide two new functions (tb_switch_find_vsec_cap() and tb_port_find_cap()) which can be used to

                  ^^^^^^^^

I assume VSEC is the same acronym as in the PCIe spec, so this naming scheme results in "vendor specific extended capability capability", which is maybe a bit odd.

Thanks,

Lukas

#8 lore raw

Re: [PATCH v3 06/27] thunderbolt: Rework capability handling

Quoted 10 lines · Lukas Wunner
Quoted 4 lines · Mika Westerberg

Organization of the capabilities in switches and ports is not so random after all. Rework the capability handling functionality so that it follows how capabilities are organized and provide two new functions (tb_switch_find_vsec_cap() and tb_port_find_cap()) which can be used to

                  ^^^^^^^^

I assume VSEC is the same acronym as in the PCIe spec, so this naming scheme results in "vendor specific extended capability capability", which is maybe a bit odd.

AFAIK it comes from Vendor SpEcifiC but I'm not 100% sure ;-) The Alpine Ridge datasheet calls it also VSEC capability which is why we chose the naming accordingly.

#9 lore raw

Re: [PATCH v3 06/27] thunderbolt: Rework capability handling

Quoted 8 lines · Mika Westerberg
Quoted 7 lines · Lukas Wunner
Quoted 4 lines · Mika Westerberg

Organization of the capabilities in switches and ports is not so random after all. Rework the capability handling functionality so that it follows how capabilities are organized and provide two new functions (tb_switch_find_vsec_cap() and tb_port_find_cap()) which can be used to

                  ^^^^^^^^
Quoted 7 lines
Quoted 3 lines

I assume VSEC is the same acronym as in the PCIe spec, so this naming scheme results in "vendor specific extended capability capability", which is maybe a bit odd.

AFAIK it comes from Vendor SpEcifiC but I'm not 100% sure ;-) The Alpine Ridge datasheet calls it also VSEC capability which is why we chose the naming accordingly.

That said, it could also come from Vendor SpEcific Capability. I can change it to tb_switch_find_vsec() if that works better.

#10 lore raw

Re: [PATCH v3 06/27] thunderbolt: Rework capability handling

Quoted 13 lines · Mika Westerberg
Quoted 12 lines · Mika Westerberg
Quoted 10 lines · Lukas Wunner
Quoted 4 lines · Mika Westerberg

Organization of the capabilities in switches and ports is not so random after all. Rework the capability handling functionality so that it follows how capabilities are organized and provide two new functions (tb_switch_find_vsec_cap() and tb_port_find_cap()) which can be used to

                  ^^^^^^^^

I assume VSEC is the same acronym as in the PCIe spec, so this naming scheme results in "vendor specific extended capability capability", which is maybe a bit odd.

Quoted 6 lines
Quoted 3 lines

AFAIK it comes from Vendor SpEcifiC but I'm not 100% sure ;-) The Alpine Ridge datasheet calls it also VSEC capability which is why we chose the naming accordingly.

That said, it could also come from Vendor SpEcific Capability. I can change it to tb_switch_find_vsec() if that works better.

I would folllow PCI existing namings, i.e.
_vse_cap()

_vse_capability() also possible, but too long to me.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#11 lore raw

Re: [PATCH v3 06/27] thunderbolt: Rework capability handling

Quoted 19 lines · Andy Shevchenko
Quoted 17 lines · Mika Westerberg
Quoted 15 lines · Mika Westerberg
Quoted 10 lines · Lukas Wunner
Quoted 4 lines · Mika Westerberg

Organization of the capabilities in switches and ports is not so random after all. Rework the capability handling functionality so that it follows how capabilities are organized and provide two new functions (tb_switch_find_vsec_cap() and tb_port_find_cap()) which can be used to

                  ^^^^^^^^

I assume VSEC is the same acronym as in the PCIe spec, so this naming scheme results in "vendor specific extended capability capability", which is maybe a bit odd.

AFAIK it comes from Vendor SpEcifiC but I'm not 100% sure ;-) The Alpine Ridge datasheet calls it also VSEC capability which is why we chose the naming accordingly.

Quoted 5 lines
Quoted 2 lines

That said, it could also come from Vendor SpEcific Capability. I can change it to tb_switch_find_vsec() if that works better.

I would folllow PCI existing namings, i.e.
_vse_cap()

Indeed this sounds better.

I looked at the TBT specs again and they just use VSEC but do not open the acronym any further. I guess the "Vendor Specific Extended Capability" as Lukas pointed out is the correct one.

I'll change the patch to use tb_switch_find_vse_cap() and rename TB_VSEC_CAP_* to TB_VSE_CAP_* if there are no objections.

#12 lore raw

PATCH v3 7/27 thunderbolt: Allow passing NULL to tb_ctl_free()

In reply to Mika Westerberg

Following the usual pattern used in many places, we allow passing NULL pointer to tb_ctl_free(). Then the user can call the function regardless if it has allocated control channel or not making the code bit simpler.

---
 drivers/thunderbolt/ctl.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/thunderbolt/ctl.c +4 -0
index 889a32dd21e7..f8290a577b2b 100644
@@ -520,6 +520,10 @@ struct tb_ctl *tb_ctl_alloc(struct tb_nhi *nhi, hotplug_cb cb, void *cb_data)
 void tb_ctl_free(struct tb_ctl *ctl)
 {
 	int i;
+
+	if (!ctl)
+		return;
+
 	if (ctl->rx)
 		ring_free(ctl->rx);
 	if (ctl->tx)
Signature · 1 line
2.11.0
#13 lore raw

PATCH v3 8/27 thunderbolt: Introduce thunderbolt bus and connection manager

In reply to Mika Westerberg

Thunderbolt fabric consists of one or more switches. This fabric is called domain and it is controlled by an entity called connection manager. The connection manager can be either internal (driven by a firmware running on the host controller) or external (software driver). This driver currently implements support for the latter.

In order to manage switches and their properties more easily we model this domain structure as a Linux bus. Each host controller adds a domain device to this bus, and these devices are named as domainN where N stands for index or id of the current domain.

We then abstract connection manager specific operations into a new structure tb_cm_ops and convert the existing tb.c to fill those accordingly. This makes it easier to add support for the internal connection manager in subsequent patches.

---
 drivers/thunderbolt/Makefile     |   2 +-
 drivers/thunderbolt/domain.c     | 229 +++++++++++++++++++++++++++++++++++++++
 drivers/thunderbolt/nhi.c        |  31 ++++--
 drivers/thunderbolt/tb.c         | 156 ++++++++++++--------------
 drivers/thunderbolt/tb.h         |  70 +++++++++---
 drivers/thunderbolt/tunnel_pci.c |   9 +-
 6 files changed, 376 insertions(+), 121 deletions(-)
 create mode 100644 drivers/thunderbolt/domain.c
drivers/thunderbolt/Makefile +1 -1
index 5d1053cdfa54..e276a9a62261 100644
@@ -1,3 +1,3 @@
 obj-${CONFIG_THUNDERBOLT} := thunderbolt.o
 thunderbolt-objs := nhi.o ctl.o tb.o switch.o cap.o path.o tunnel_pci.o eeprom.o
-
+thunderbolt-objs += domain.o
drivers/thunderbolt/domain.c new file +229 -0

229 lines not on this page · raw message

drivers/thunderbolt/nhi.c +23 -8
index ed75c49748f5..c1113a3c4128 100644
@@ -586,16 +586,16 @@ static int nhi_suspend_noirq(struct device *dev)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
 	struct tb *tb = pci_get_drvdata(pdev);
-	thunderbolt_suspend(tb);
-	return 0;
+
+	return tb_domain_suspend_noirq(tb);
 }
 
 static int nhi_resume_noirq(struct device *dev)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
 	struct tb *tb = pci_get_drvdata(pdev);
-	thunderbolt_resume(tb);
-	return 0;
+
+	return tb_domain_resume_noirq(tb);
 }
 
 static void nhi_shutdown(struct tb_nhi *nhi)
@@ -715,12 +715,17 @@ static int nhi_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	iowrite32(3906250 / 10000, nhi->iobase + 0x38c00);
 
 	dev_info(&nhi->pdev->dev, "NHI initialized, starting thunderbolt\n");
-	tb = thunderbolt_alloc_and_start(nhi);
-	if (!tb) {
+	tb = tb_probe(nhi);
+	if (!tb)
+		return -ENODEV;
+
+	res = tb_domain_add(tb);
+	if (res) {
 		/*
 		 * At this point the RX/TX rings might already have been
 		 * activated. Do a proper shutdown.
 		 */
+		tb_domain_put(tb);
 		nhi_shutdown(nhi);
 		return -EIO;
 	}
@@ -733,7 +738,8 @@ static void nhi_remove(struct pci_dev *pdev)
 {
 	struct tb *tb = pci_get_drvdata(pdev);
 	struct tb_nhi *nhi = tb->nhi;
-	thunderbolt_shutdown_and_free(tb);
+
+	tb_domain_remove(tb);
 	nhi_shutdown(nhi);
 }
 
@@ -797,14 +803,23 @@ static struct pci_driver nhi_driver = {
 
 static int __init nhi_init(void)
 {
+	int ret;
+
 	if (!dmi_match(DMI_BOARD_VENDOR, "Apple Inc."))
 		return -ENOSYS;
-	return pci_register_driver(&nhi_driver);
+	ret = tb_domain_init();
+	if (ret)
+		return ret;
+	ret = pci_register_driver(&nhi_driver);
+	if (ret)
+		tb_domain_exit();
+	return ret;
 }
 
 static void __exit nhi_unload(void)
 {
 	pci_unregister_driver(&nhi_driver);
+	tb_domain_exit();
 }
 
 module_init(nhi_init);
drivers/thunderbolt/tb.c +69 -87

245 lines not on this page · raw message

drivers/thunderbolt/tb.h +53 -17

93 lines not on this page · raw message

drivers/thunderbolt/tunnel_pci.c +1 -8
index f4ce9845e42a..ca4475907d7a 100644
@@ -194,19 +194,13 @@ int tb_pci_restart(struct tb_pci_tunnel *tunnel)
  */
 int tb_pci_activate(struct tb_pci_tunnel *tunnel)
 {
-	int res;
 	if (tunnel->path_to_up->activated || tunnel->path_to_down->activated) {
 		tb_tunnel_WARN(tunnel,
 			       "trying to activate an already activated tunnel\n");
 		return -EINVAL;
 	}
 
-	res = tb_pci_restart(tunnel);
-	if (res)
-		return res;
-
-	list_add(&tunnel->list, &tunnel->tb->tunnel_list);
-	return 0;
+	return tb_pci_restart(tunnel);
 }
 
 
@@ -227,6 +221,5 @@ void tb_pci_deactivate(struct tb_pci_tunnel *tunnel)
 		tb_path_deactivate(tunnel->path_to_down);
 	if (tunnel->path_to_up->activated)
 		tb_path_deactivate(tunnel->path_to_up);
-	list_del_init(&tunnel->list);
 }
 
Signature · 1 line
2.11.0
maintainer
#14 lore raw

Re: [PATCH v3 08/27] thunderbolt: Introduce thunderbolt bus and connection manager

Quoted 459 lines · Mika Westerberg

Thunderbolt fabric consists of one or more switches. This fabric is called domain and it is controlled by an entity called connection manager. The connection manager can be either internal (driven by a firmware running on the host controller) or external (software driver). This driver currently implements support for the latter.

In order to manage switches and their properties more easily we model this domain structure as a Linux bus. Each host controller adds a domain device to this bus, and these devices are named as domainN where N stands for index or id of the current domain.

We then abstract connection manager specific operations into a new structure tb_cm_ops and convert the existing tb.c to fill those accordingly. This makes it easier to add support for the internal connection manager in subsequent patches.

---
 drivers/thunderbolt/Makefile     |   2 +-
 drivers/thunderbolt/domain.c     | 229 +++++++++++++++++++++++++++++++++++++++
 drivers/thunderbolt/nhi.c        |  31 ++++--
 drivers/thunderbolt/tb.c         | 156 ++++++++++++--------------
 drivers/thunderbolt/tb.h         |  70 +++++++++---
 drivers/thunderbolt/tunnel_pci.c |   9 +-
 6 files changed, 376 insertions(+), 121 deletions(-)
 create mode 100644 drivers/thunderbolt/domain.c
drivers/thunderbolt/Makefile +1 -1
index 5d1053cdfa54..e276a9a62261 100644
@@ -1,3 +1,3 @@
 obj-${CONFIG_THUNDERBOLT} := thunderbolt.o
 thunderbolt-objs := nhi.o ctl.o tb.o switch.o cap.o path.o tunnel_pci.o eeprom.o
-
+thunderbolt-objs += domain.o
drivers/thunderbolt/domain.c new file +229 -0
new file mode 100644
index 000000000000..e2f3777edee6
@@ -0,0 +1,229 @@
+/*
+ * Thunderbolt bus support
+ *
+ * Copyright (C) 2017, Intel Corporation
+ * Author:  Mika Westerberg <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/device.h>
+#include <linux/idr.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+
+#include "tb.h"
+
+static DEFINE_IDA(tb_domain_ida);
+
+struct bus_type tb_bus_type = {
+       .name = "thunderbolt",
+};
+
+static void tb_domain_release(struct device *dev)
+{
+       struct tb *tb = container_of(dev, struct tb, dev);
+
+       tb_ctl_free(tb->ctl);
+       destroy_workqueue(tb->wq);
+       ida_simple_remove(&tb_domain_ida, tb->index);
+       kfree(tb);
+}
+
+struct device_type tb_domain_type = {
+       .name = "thunderbolt_domain",
+       .release = tb_domain_release,
+};
+
+/**
+ * tb_domain_alloc() - Allocate a domain
+ * @nhi: Pointer to the host controller
+ * @privsize: Size of the connection manager private data
+ *
+ * Allocates and initializes a new Thunderbolt domain. Connection
+ * managers are expected to call this and then fill in @cm_ops
+ * accordingly.
+ *
+ * Call tb_domain_put() to release the domain before it has been added
+ * to the system.
+ *
+ * Return: allocated domain structure on %NULL in case of error
+ */
+struct tb *tb_domain_alloc(struct tb_nhi *nhi, size_t privsize)
+{
+       struct tb *tb;
+
+       /*
+        * Make sure the structure sizes map with that the hardware
+        * expects because bit-fields are being used.
+        */
+       BUILD_BUG_ON(sizeof(struct tb_regs_switch_header) != 5 * 4);
+       BUILD_BUG_ON(sizeof(struct tb_regs_port_header) != 8 * 4);
+       BUILD_BUG_ON(sizeof(struct tb_regs_hop) != 2 * 4);
+
+       tb = kzalloc(sizeof(*tb) + privsize, GFP_KERNEL);
+       if (!tb)
+               return NULL;
+
+       tb->nhi = nhi;
+       mutex_init(&tb->lock);
+
+       tb->index = ida_simple_get(&tb_domain_ida, 0, 0, GFP_KERNEL);
+       if (tb->index < 0)
+               goto err_free;
+
+       tb->wq = alloc_ordered_workqueue("thunderbolt%d", 0, tb->index);
+       if (!tb->wq)
+               goto err_remove_ida;
+
+       tb->dev.parent = &nhi->pdev->dev;
+       tb->dev.bus = &tb_bus_type;
+       tb->dev.type = &tb_domain_type;
+       dev_set_name(&tb->dev, "domain%d", tb->index);
+       device_initialize(&tb->dev);
+
+       return tb;
+
+err_remove_ida:
+       ida_simple_remove(&tb_domain_ida, tb->index);
+err_free:
+       kfree(tb);
+
+       return NULL;
+}
+
+/**
+ * tb_domain_add() - Add domain to the system
+ * @tb: Domain to add
+ *
+ * Starts the domain and adds it to the system. Hotplugging devices will
+ * work after this has been returned successfully. In order to remove
+ * and release the domain after this function has been called, call
+ * tb_domain_remove().
+ *
+ * Return: %0 in case of success and negative errno in case of error
+ */
+int tb_domain_add(struct tb *tb)
+{
+       int ret;
+
+       if (WARN_ON(!tb->cm_ops))
+               return -EINVAL;
+
+       mutex_lock(&tb->lock);
+
+       tb->ctl = tb_ctl_alloc(tb->nhi, tb->cm_ops->hotplug, tb);
+       if (!tb->ctl) {
+               ret = -ENOMEM;
+               goto err_unlock;
+       }
+
+       /*
+        * tb_schedule_hotplug_handler may be called as soon as the config
+        * channel is started. Thats why we have to hold the lock here.
+        */
+       tb_ctl_start(tb->ctl);
+
+       ret = device_add(&tb->dev);
+       if (ret)
+               goto err_ctl_stop;
+
+       /* Start the domain */
+       if (tb->cm_ops->start) {
+               ret = tb->cm_ops->start(tb);
+               if (ret)
+                       goto err_domain_del;
+       }
+
+       /* This starts event processing */
+       mutex_unlock(&tb->lock);
+
+       return 0;
+
+err_domain_del:
+       device_del(&tb->dev);
+err_ctl_stop:
+       tb_ctl_stop(tb->ctl);
+err_unlock:
+       mutex_unlock(&tb->lock);
+
+       return ret;
+}
+
+/**
+ * tb_domain_remove() - Removes and releases a domain
+ * @tb: Domain to remove
+ *
+ * Stops the domain, removes it from the system and releases all
+ * resources once the last reference has been released.
+ */
+void tb_domain_remove(struct tb *tb)
+{
+       mutex_lock(&tb->lock);
+       if (tb->cm_ops->stop)
+               tb->cm_ops->stop(tb);
+       /* Stop the domain control traffic */
+       tb_ctl_stop(tb->ctl);
+       mutex_unlock(&tb->lock);
+
+       flush_workqueue(tb->wq);
+       device_unregister(&tb->dev);
+}
+
+/**
+ * tb_domain_suspend_noirq() - Suspend a domain
+ * @tb: Domain to suspend
+ *
+ * Suspends all devices in the domain and stops the control channel.
+ */
+int tb_domain_suspend_noirq(struct tb *tb)
+{
+       int ret = 0;
+
+       /*
+        * The control channel interrupt is left enabled during suspend
+        * and taking the lock here prevents any events happening before
+        * we actually have stopped the domain and the control channel.
+        */
+       mutex_lock(&tb->lock);
+       if (tb->cm_ops->suspend_noirq)
+               ret = tb->cm_ops->suspend_noirq(tb);
+       if (!ret)
+               tb_ctl_stop(tb->ctl);
+       mutex_unlock(&tb->lock);
+
+       return ret;
+}
+
+/**
+ * tb_domain_resume_noirq() - Resume a domain
+ * @tb: Domain to resume
+ *
+ * Re-starts the control channel, and resumes all devices connected to
+ * the domain.
+ */
+int tb_domain_resume_noirq(struct tb *tb)
+{
+       int ret = 0;
+
+       mutex_lock(&tb->lock);
+       tb_ctl_start(tb->ctl);
+       if (tb->cm_ops->resume_noirq)
+               ret = tb->cm_ops->resume_noirq(tb);
+       mutex_unlock(&tb->lock);
+
+       return ret;
+}
+
+int tb_domain_init(void)
+{
+       return bus_register(&tb_bus_type);
+}
+
+void tb_domain_exit(void)
+{
+       bus_unregister(&tb_bus_type);
+       ida_destroy(&tb_domain_ida);
+}
drivers/thunderbolt/nhi.c +23 -8
index ed75c49748f5..c1113a3c4128 100644
@@ -586,16 +586,16 @@ static int nhi_suspend_noirq(struct device *dev)
 {
        struct pci_dev *pdev = to_pci_dev(dev);
        struct tb *tb = pci_get_drvdata(pdev);
-       thunderbolt_suspend(tb);
-       return 0;
+
+       return tb_domain_suspend_noirq(tb);
 }
 
 static int nhi_resume_noirq(struct device *dev)
 {
        struct pci_dev *pdev = to_pci_dev(dev);
        struct tb *tb = pci_get_drvdata(pdev);
-       thunderbolt_resume(tb);
-       return 0;
+
+       return tb_domain_resume_noirq(tb);
 }
 
 static void nhi_shutdown(struct tb_nhi *nhi)
@@ -715,12 +715,17 @@ static int nhi_probe(struct pci_dev *pdev, const struct pci_device_id *id)
        iowrite32(3906250 / 10000, nhi->iobase + 0x38c00);
 
        dev_info(&nhi->pdev->dev, "NHI initialized, starting thunderbolt\n");
-       tb = thunderbolt_alloc_and_start(nhi);
-       if (!tb) {
+       tb = tb_probe(nhi);
+       if (!tb)
+               return -ENODEV;
+
+       res = tb_domain_add(tb);
+       if (res) {
                /*
                 * At this point the RX/TX rings might already have been
                 * activated. Do a proper shutdown.
                 */
+               tb_domain_put(tb);
                nhi_shutdown(nhi);
                return -EIO;
        }
@@ -733,7 +738,8 @@ static void nhi_remove(struct pci_dev *pdev)
 {
        struct tb *tb = pci_get_drvdata(pdev);
        struct tb_nhi *nhi = tb->nhi;
-       thunderbolt_shutdown_and_free(tb);
+
+       tb_domain_remove(tb);
        nhi_shutdown(nhi);
 }
 
@@ -797,14 +803,23 @@ static struct pci_driver nhi_driver = {
 
 static int __init nhi_init(void)
 {
+       int ret;
+
        if (!dmi_match(DMI_BOARD_VENDOR, "Apple Inc."))
                return -ENOSYS;
-       return pci_register_driver(&nhi_driver);
+       ret = tb_domain_init();
+       if (ret)
+               return ret;
+       ret = pci_register_driver(&nhi_driver);
+       if (ret)
+               tb_domain_exit();
+       return ret;
 }
 
 static void __exit nhi_unload(void)
 {
        pci_unregister_driver(&nhi_driver);
+       tb_domain_exit();
 }
 
 module_init(nhi_init);
drivers/thunderbolt/tb.c +24 -19
index 6b44076e1380..9f00a0f28d53 100644
@@ -12,6 +12,18 @@
 #include "tb_regs.h"
 #include "tunnel_pci.h"
 
+/**
+ * struct tb_cm - Simple Thunderbolt connection manager
+ * @tunnel_list: List of active tunnels
+ * @hotplug_active: tb_handle_hotplug will stop progressing plug
+ *                 events and exit if this is not set (it needs to
+ *                 acquire the lock one more time). Used to drain wq
+ *                 after cfg has been paused.
+ */
+struct tb_cm {
+       struct list_head tunnel_list;
+       bool hotplug_active;
+};
 
 /* enumeration & hot plug handling */
 
@@ -62,12 +74,14 @@ static void tb_scan_port(struct tb_port *port)
  */
 static void tb_free_invalid_tunnels(struct tb *tb)
 {
+       struct tb_cm *tcm = tb_priv(tb);
        struct tb_pci_tunnel *tunnel;
        struct tb_pci_tunnel *n;
-       list_for_each_entry_safe(tunnel, n, &tb->tunnel_list, list)
-       {
+
+       list_for_each_entry_safe(tunnel, n, &tcm->tunnel_list, list) {
                if (tb_pci_is_invalid(tunnel)) {
                        tb_pci_deactivate(tunnel);
+                       list_del(&tunnel->list);
                        tb_pci_free(tunnel);
                }
        }
@@ -149,6 +163,8 @@ static void tb_activate_pcie_devices(struct tb *tb)
        struct tb_port *up_port;
        struct tb_port *down_port;
        struct tb_pci_tunnel *tunnel;
+       struct tb_cm *tcm = tb_priv(tb);
+
        /* scan for pcie devices at depth 1*/
        for (i = 1; i <= tb->root_switch->config.max_port_number; i++) {
                if (tb_is_upstream_port(&tb->root_switch->ports[i]))
@@ -195,6 +211,7 @@ static void tb_activate_pcie_devices(struct tb *tb)
                        tb_pci_free(tunnel);
                }
 
+               list_add(&tunnel->list, &tcm->tunnel_list);
        }
 }
 
@@ -217,10 +234,11 @@ static void tb_handle_hotplug(struct work_struct *work)
 {
        struct tb_hotplug_event *ev = container_of(work, typeof(*ev), work);
        struct tb *tb = ev->tb;
+       struct tb_cm *tcm = tb_priv(tb);
        struct tb_switch *sw;
        struct tb_port *port;
        mutex_lock(&tb->lock);
-       if (!tb->hotplug_active)
+       if (!tcm->hotplug_active)
                goto out; /* during init, suspend or shutdown */
 
        sw = get_switch_at_route(tb->root_switch, ev->route);
@@ -296,22 +314,14 @@ static void tb_schedule_hotplug_handler(void *data, u64 route, u8 port,
        queue_work(tb->wq, &ev->work);
 }
 
-/**
- * thunderbolt_shutdown_and_free() - shutdown everything
- *
- * Free all switches and the config channel.
- *
- * Used in the error path of thunderbolt_alloc_and_start.
- */
-void thunderbolt_shutdown_and_free(struct tb *tb)
+static void tb_stop(struct tb *tb)
 {
+       struct tb_cm *tcm = tb_priv(tb);
        struct tb_pci_tunnel *tunnel;
        struct tb_pci_tunnel *n;
 
-       mutex_lock(&tb->lock);
-
        /* tunnels are only present after everything has been initialized */
-       list_for_each_entry_safe(tunnel, n, &tb->tunnel_list, list) {
+       list_for_each_entry_safe(tunnel, n, &tcm->tunnel_list, list) {
                tb_pci_deactivate(tunnel);
                tb_pci_free(tunnel);
        }
@@ -320,98 +330,44 @@ void thunderbolt_shutdown_and_free(struct tb *tb)
                tb_switch_free(tb->root_switch);
        tb->root_switch = NULL;
 
-       if (tb->ctl) {
-               tb_ctl_stop(tb->ctl);
-               tb_ctl_free(tb->ctl);
-       }
-       tb->ctl = NULL;

drivers/thunderbolt/tb.c void thunderbolt_shutdown_and_free(struct tb *tb) · line 337

Quoted 10 lines
drivers/thunderbolt/tb.c +0 -10
-       tb->hotplug_active = false; /* signal tb_handle_hotplug to quit */
-
-       /* allow tb_handle_hotplug to acquire the lock */
-       mutex_unlock(&tb->lock);
-       if (tb->wq) {
-               flush_workqueue(tb->wq);
-               destroy_workqueue(tb->wq);
-               tb->wq = NULL;
-       }
-       mutex_destroy(&tb->lock);

This seems to have been lost.

Quoted 277 lines
drivers/thunderbolt/tb.c +45 -58
-       kfree(tb);
+       tcm->hotplug_active = false; /* signal tb_handle_hotplug to quit */
 }
 
-/**
- * thunderbolt_alloc_and_start() - setup the thunderbolt bus
- *
- * Allocates a tb_cfg control channel, initializes the root switch, enables
- * plug events and activates pci devices.
- *
- * Return: Returns NULL on error.
- */
-struct tb *thunderbolt_alloc_and_start(struct tb_nhi *nhi)
+static int tb_start(struct tb *tb)
 {
-       struct tb *tb;
-
-       BUILD_BUG_ON(sizeof(struct tb_regs_switch_header) != 5 * 4);
-       BUILD_BUG_ON(sizeof(struct tb_regs_port_header) != 8 * 4);
-       BUILD_BUG_ON(sizeof(struct tb_regs_hop) != 2 * 4);
-
-       tb = kzalloc(sizeof(*tb), GFP_KERNEL);
-       if (!tb)
-               return NULL;
-
-       tb->nhi = nhi;
-       mutex_init(&tb->lock);
-       mutex_lock(&tb->lock);
-       INIT_LIST_HEAD(&tb->tunnel_list);
-
-       tb->wq = alloc_ordered_workqueue("thunderbolt", 0);
-       if (!tb->wq)
-               goto err_locked;
-
-       tb->ctl = tb_ctl_alloc(tb->nhi, tb_schedule_hotplug_handler, tb);
-       if (!tb->ctl)
-               goto err_locked;
-       /*
-        * tb_schedule_hotplug_handler may be called as soon as the config
-        * channel is started. Thats why we have to hold the lock here.
-        */
-       tb_ctl_start(tb->ctl);
+       struct tb_cm *tcm = tb_priv(tb);
 
        tb->root_switch = tb_switch_alloc(tb, 0);
        if (!tb->root_switch)
-               goto err_locked;
+               return -ENOMEM;
 
        /* Full scan to discover devices added before the driver was loaded. */
        tb_scan_switch(tb->root_switch);
        tb_activate_pcie_devices(tb);
 
        /* Allow tb_handle_hotplug to progress events */
-       tb->hotplug_active = true;
-       mutex_unlock(&tb->lock);
-       return tb;
-
-err_locked:
-       mutex_unlock(&tb->lock);
-       thunderbolt_shutdown_and_free(tb);
-       return NULL;
+       tcm->hotplug_active = true;
+       return 0;
 }
 
-void thunderbolt_suspend(struct tb *tb)
+static int tb_suspend_noirq(struct tb *tb)
 {
+       struct tb_cm *tcm = tb_priv(tb);
+
        tb_info(tb, "suspending...\n");
-       mutex_lock(&tb->lock);
        tb_switch_suspend(tb->root_switch);
-       tb_ctl_stop(tb->ctl);
-       tb->hotplug_active = false; /* signal tb_handle_hotplug to quit */
-       mutex_unlock(&tb->lock);
+       tcm->hotplug_active = false; /* signal tb_handle_hotplug to quit */
        tb_info(tb, "suspend finished\n");
+
+       return 0;
 }
 
-void thunderbolt_resume(struct tb *tb)
+static int tb_resume_noirq(struct tb *tb)
 {
+       struct tb_cm *tcm = tb_priv(tb);
        struct tb_pci_tunnel *tunnel, *n;
+
        tb_info(tb, "resuming...\n");
-       mutex_lock(&tb->lock);
-       tb_ctl_start(tb->ctl);
 
        /* remove any pci devices the firmware might have setup */
        tb_switch_reset(tb, 0);
@@ -419,9 +375,9 @@ void thunderbolt_resume(struct tb *tb)
        tb_switch_resume(tb->root_switch);
        tb_free_invalid_tunnels(tb);
        tb_free_unplugged_children(tb->root_switch);
-       list_for_each_entry_safe(tunnel, n, &tb->tunnel_list, list)
+       list_for_each_entry_safe(tunnel, n, &tcm->tunnel_list, list)
                tb_pci_restart(tunnel);
-       if (!list_empty(&tb->tunnel_list)) {
+       if (!list_empty(&tcm->tunnel_list)) {
                /*
                 * the pcie links need some time to get going.
                 * 100ms works for me...
@@ -430,7 +386,33 @@ void thunderbolt_resume(struct tb *tb)
                msleep(100);
        }
         /* Allow tb_handle_hotplug to progress events */
-       tb->hotplug_active = true;
-       mutex_unlock(&tb->lock);
+       tcm->hotplug_active = true;
        tb_info(tb, "resume finished\n");
+
+       return 0;
+}
+
+static const struct tb_cm_ops tb_cm_ops = {
+       .start = tb_start,
+       .stop = tb_stop,
+       .suspend_noirq = tb_suspend_noirq,
+       .resume_noirq = tb_resume_noirq,
+       .hotplug = tb_schedule_hotplug_handler,
+};
+
+struct tb *tb_probe(struct tb_nhi *nhi)
+{
+       struct tb_cm *tcm;
+       struct tb *tb;
+
+       tb = tb_domain_alloc(nhi, sizeof(*tcm));
+       if (!tb)
+               return NULL;
+
+       tb->cm_ops = &tb_cm_ops;
+
+       tcm = tb_priv(tb);
+       INIT_LIST_HEAD(&tcm->tunnel_list);
+
+       return tb;
 }
drivers/thunderbolt/tb.h +53 -17
index 9b60e71562dc..ce9db64e3333 100644
@@ -92,29 +92,52 @@ struct tb_path {
        int path_length; /* number of hops */
 };
 
+/**
+ * struct tb_cm_ops - Connection manager specific operations vector
+ * @start: Starts the domain
+ * @stop: Stops the domain
+ * @suspend_noirq: Connection manager specific suspend_noirq
+ * @resume_noirq: Connection manager specific resume_noirq
+ * @hotplug: Handle hotplug event
+ */
+struct tb_cm_ops {
+       int (*start)(struct tb *tb);
+       void (*stop)(struct tb *tb);
+       int (*suspend_noirq)(struct tb *tb);
+       int (*resume_noirq)(struct tb *tb);
+       hotplug_cb hotplug;
+};
 
 /**
  * struct tb - main thunderbolt bus structure
+ * @dev: Domain device
+ * @lock: Big lock. Must be held when accessing cfg or any struct
+ *       tb_switch / struct tb_port.
+ * @nhi: Pointer to the NHI structure
+ * @ctl: Control channel for this domain
+ * @wq: Ordered workqueue for all domain specific work
+ * @root_switch: Root switch of this domain
+ * @cm_ops: Connection manager specific operations vector
+ * @index: Linux assigned domain number
+ * @privdata: Private connection manager specific data
  */
 struct tb {
-       struct mutex lock;      /*
-                                * Big lock. Must be held when accessing cfg or
-                                * any struct tb_switch / struct tb_port.
-                                */
+       struct device dev;
+       struct mutex lock;
        struct tb_nhi *nhi;
        struct tb_ctl *ctl;
-       struct workqueue_struct *wq; /* ordered workqueue for plug events */
+       struct workqueue_struct *wq;
        struct tb_switch *root_switch;
-       struct list_head tunnel_list; /* list of active PCIe tunnels */
-       bool hotplug_active; /*
-                             * tb_handle_hotplug will stop progressing plug
-                             * events and exit if this is not set (it needs to
-                             * acquire the lock one more time). Used to drain
-                             * wq after cfg has been paused.
-                             */
-
+       const struct tb_cm_ops *cm_ops;
+       int index;
+       unsigned long privdata[0];
 };
 
+static inline void *tb_priv(struct tb *tb)
+{
+       return (void *)tb->privdata;
+}
+
 /* helper functions & macros */
 
 /**
@@ -215,11 +238,24 @@ static inline int tb_port_write(struct tb_port *port, const void *buffer,
 #define tb_port_info(port, fmt, arg...) \
        __TB_PORT_PRINT(tb_info, port, fmt, ##arg)
 
+struct tb *tb_probe(struct tb_nhi *nhi);
+
+extern struct bus_type tb_bus_type;
+extern struct device_type tb_domain_type;
+
+int tb_domain_init(void);
+void tb_domain_exit(void);
 
-struct tb *thunderbolt_alloc_and_start(struct tb_nhi *nhi);
-void thunderbolt_shutdown_and_free(struct tb *tb);
-void thunderbolt_suspend(struct tb *tb);
-void thunderbolt_resume(struct tb *tb);
+struct tb *tb_domain_alloc(struct tb_nhi *nhi, size_t privsize);
+int tb_domain_add(struct tb *tb);
+void tb_domain_remove(struct tb *tb);
+int tb_domain_suspend_noirq(struct tb *tb);
+int tb_domain_resume_noirq(struct tb *tb);
+
+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);
drivers/thunderbolt/tunnel_pci.c +1 -9
index f4ce9845e42a..ca4475907d7a 100644
@@ -194,19 +194,13 @@ int tb_pci_restart(struct tb_pci_tunnel *tunnel)
  */
 int tb_pci_activate(struct tb_pci_tunnel *tunnel)
 {
-       int res;
        if (tunnel->path_to_up->activated || tunnel->path_to_down->activated) {
                tb_tunnel_WARN(tunnel,
                               "trying to activate an already activated tunnel\n");
                return -EINVAL;
        }
 
-       res = tb_pci_restart(tunnel);
-       if (res)
-               return res;
-
-       list_add(&tunnel->list, &tunnel->tb->tunnel_list);
-       return 0;
+       return tb_pci_restart(tunnel);
 }
 
 
@@ -227,6 +221,5 @@ void tb_pci_deactivate(struct tb_pci_tunnel *tunnel)
                tb_path_deactivate(tunnel->path_to_down);
        if (tunnel->path_to_up->activated)
                tb_path_deactivate(tunnel->path_to_up);
-       list_del_init(&tunnel->list);
 }
 
--

2.11.0

#15 lore raw

PATCH v3 9/27 thunderbolt: Convert switch to a device

In reply to Mika Westerberg

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.

---
 Documentation/ABI/testing/sysfs-bus-thunderbolt |  22 ++
 drivers/thunderbolt/eeprom.c                    |   2 +
 drivers/thunderbolt/switch.c                    | 261 +++++++++++++++++++-----
 drivers/thunderbolt/tb.c                        |  40 +++-
 drivers/thunderbolt/tb.h                        |  45 +++-
 5 files changed, 303 insertions(+), 67 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-thunderbolt
Documentation/ABI/testing/sysfs-bus-thunderbolt new file +22 -0
new file mode 100644
index 000000000000..9f1bd0086938
@@ -0,0 +1,22 @@
+What:		/sys/bus/thunderbolt/devices/.../device
+Date:		Sep 2017
+KernelVersion:	4.13
+Contact:	[email protected]
+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:	[email protected]
+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:	[email protected]
+Description:	This attribute contains unique_id string of this device.
+		This is either read from hardware registers (UUID on
+		newer hardware) or based on UID from the device DROM.
+		Can be used to uniquely identify particular device.
drivers/thunderbolt/eeprom.c +2 -0
index eb2179c98b09..7e485e3ef27e 100644
@@ -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) {
drivers/thunderbolt/switch.c +207 -54

325 lines not on this page · raw message

drivers/thunderbolt/tb.c +31 -9
index 9f00a0f28d53..94ecac012428 100644
@@ -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);
drivers/thunderbolt/tb.h +41 -4

81 lines not on this page · raw message

Signature · 1 line
2.11.0
Acked-by on v3 9/27
#16 lore raw

Re: [PATCH v3 09/27] thunderbolt: Convert switch to a device

Quoted 18 lines · Mika Westerberg

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.

Quoted 8 lines
---
 Documentation/ABI/testing/sysfs-bus-thunderbolt |  22 ++
 drivers/thunderbolt/eeprom.c                    |   2 +
 drivers/thunderbolt/switch.c                    | 261 +++++++++++++++++++-----
 drivers/thunderbolt/tb.c                        |  40 +++-
 drivers/thunderbolt/tb.h                        |  45 +++-
 5 files changed, 303 insertions(+), 67 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-thunderbolt

For the driver model type changes, looks good to me, nice job:

#17 lore raw

PATCH v3 10/27 thunderbolt: Fail switch adding operation if reading DROM fails

In reply to Mika Westerberg

All non-root switches are expected to have DROM so if the operation fails, it might be due the user unlugging the device. There is no point continuing adding the switch further in that case. Just bail out.

For root switches (hosts) the DROM is either retrieved from a EFI variable, NVM or hard-coded.

---
 drivers/thunderbolt/switch.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
drivers/thunderbolt/switch.c +5 -2
index 5cd854dedeea..0ce8b600a23f 100644
@@ -535,8 +535,11 @@ 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");
+	ret = tb_drom_read(sw);
+	if (ret) {
+		tb_sw_warn(sw, "tb_eeprom_read_rom failed\n");
+		return ret;
+	}
 	tb_sw_info(sw, "uid: %#llx\n", sw->uid);
 
 	tb_switch_set_uuid(sw);
Signature · 1 line
2.11.0
#18 lore raw

PATCH v3 11/27 thunderbolt: Do not fail if DROM data CRC32 is invalid

In reply to Mika Westerberg

There are devices out there where CRC32 of the DROM is not correct. One reason for this is that the ICM firmware does not validate it and it seems that neither does the Apple driver. To be able to support such devices we continue parsing the DROM contents regardless of whether CRC32 failed or not. We still keep the warning there.

---
 drivers/thunderbolt/eeprom.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/thunderbolt/eeprom.c +1 -2
index 7e485e3ef27e..e2c1f8a45522 100644
@@ -485,9 +485,8 @@ int tb_drom_read(struct tb_switch *sw)
 	crc = tb_crc32(sw->drom + TB_DROM_DATA_START, header->data_len);
 	if (crc != header->data_crc32) {
 		tb_sw_warn(sw,
-			"drom data crc32 mismatch (expected: %#x, got: %#x), aborting\n",
+			"drom data crc32 mismatch (expected: %#x, got: %#x), continuing\n",
 			header->data_crc32, crc);
-		goto err;
 	}
 
 	if (header->device_rom_revision > 2)
Signature · 1 line
2.11.0
#19 lore raw

PATCH v3 12/27 thunderbolt: Refactor and fix parsing of port drom entries

In reply to Mika Westerberg

From: Lukas Wunner <[email protected]>

Currently tb_drom_parse_entry() is only able to parse drom entries of type TB_DROM_ENTRY_PORT. Rename it to tb_drom_parse_entry_port(). Fold tb_drom_parse_port_entry() into it.

Its return value is currently ignored. Evaluate it and abort parsing on error.

Change tb_drom_parse_entries() to accommodate for parsing of other entry types than TB_DROM_ENTRY_PORT.

---
 drivers/thunderbolt/eeprom.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)
drivers/thunderbolt/eeprom.c +16 -16
index e2c1f8a45522..5c7d80a109b1 100644
@@ -295,25 +295,13 @@ int tb_drom_read_uid_only(struct tb_switch *sw, u64 *uid)
 	return 0;
 }
 
-static void tb_drom_parse_port_entry(struct tb_port *port,
-		struct tb_drom_entry_port *entry)
-{
-	port->link_nr = entry->link_nr;
-	if (entry->has_dual_link_port)
-		port->dual_link_port =
-				&port->sw->ports[entry->dual_link_port_nr];
-}
-
-static int tb_drom_parse_entry(struct tb_switch *sw,
-		struct tb_drom_entry_header *header)
+static int tb_drom_parse_entry_port(struct tb_switch *sw,
+				    struct tb_drom_entry_header *header)
 {
 	struct tb_port *port;
 	int res;
 	enum tb_port_type type;
 
-	if (header->type != TB_DROM_ENTRY_PORT)
-		return 0;
-
 	port = &sw->ports[header->index];
 	port->disabled = header->port_disabled;
 	if (port->disabled)
@@ -332,7 +320,10 @@ static int tb_drom_parse_entry(struct tb_switch *sw,
 				header->len, sizeof(struct tb_drom_entry_port));
 			return -EIO;
 		}
-		tb_drom_parse_port_entry(port, entry);
+		port->link_nr = entry->link_nr;
+		if (entry->has_dual_link_port)
+			port->dual_link_port =
+				&port->sw->ports[entry->dual_link_port_nr];
 	}
 	return 0;
 }
@@ -347,6 +338,7 @@ static int tb_drom_parse_entries(struct tb_switch *sw)
 	struct tb_drom_header *header = (void *) sw->drom;
 	u16 pos = sizeof(*header);
 	u16 drom_size = header->data_len + TB_DROM_DATA_START;
+	int res;
 
 	while (pos < drom_size) {
 		struct tb_drom_entry_header *entry = (void *) (sw->drom + pos);
@@ -356,7 +348,15 @@ static int tb_drom_parse_entries(struct tb_switch *sw)
 			return -EIO;
 		}
 
-		tb_drom_parse_entry(sw, entry);
+		switch (entry->type) {
+		case TB_DROM_ENTRY_GENERIC:
+			break;
+		case TB_DROM_ENTRY_PORT:
+			res = tb_drom_parse_entry_port(sw, entry);
+			break;
+		}
+		if (res)
+			return res;
 
 		pos += entry->len;
 	}
Signature · 1 line
2.11.0
#20 lore raw

PATCH v3 13/27 thunderbolt: Read vendor and device name from DROM

In reply to Mika Westerberg

The device DROM contains name of the vendor and device among other things. Extract this information and expose it to the userspace via two new attributes.

---
 Documentation/ABI/testing/sysfs-bus-thunderbolt | 14 +++++++++++
 drivers/thunderbolt/eeprom.c                    | 32 +++++++++++++++++++++++++
 drivers/thunderbolt/switch.c                    | 22 +++++++++++++++++
 drivers/thunderbolt/tb.h                        |  4 ++++
 4 files changed, 72 insertions(+)
Documentation/ABI/testing/sysfs-bus-thunderbolt +14 -0
index 9f1bd0086938..29a516f53d2c 100644
@@ -5,6 +5,13 @@ Contact:	[email protected]
 Description:	This attribute contains id of this device extracted from
 		the device DROM.
 
+What:		/sys/bus/thunderbolt/devices/.../device_name
+Date:		Sep 2017
+KernelVersion:	4.13
+Contact:	[email protected]
+Description:	This attribute contains name of this device extracted from
+		the device DROM.
+
 What:		/sys/bus/thunderbolt/devices/.../vendor
 Date:		Sep 2017
 KernelVersion:	4.13
@@ -12,6 +19,13 @@ Contact:	[email protected]
 Description:	This attribute contains vendor id of this device extracted
 		from the device DROM.
 
+What:		/sys/bus/thunderbolt/devices/.../vendor_name
+Date:		Sep 2017
+KernelVersion:	4.13
+Contact:	[email protected]
+Description:	This attribute contains vendor name of this device extracted
+		from the device DROM.
+
 What:		/sys/bus/thunderbolt/devices/.../unique_id
 Date:		Sep 2017
 KernelVersion:	4.13
drivers/thunderbolt/eeprom.c +32 -0
index 5c7d80a109b1..d40a5f07fc4c 100644
@@ -204,6 +204,11 @@ struct tb_drom_entry_header {
 	enum tb_drom_entry_type type:1;
 } __packed;
 
+struct tb_drom_entry_generic {
+	struct tb_drom_entry_header header;
+	u8 data[0];
+} __packed;
+
 struct tb_drom_entry_port {
 	/* BYTES 0-1 */
 	struct tb_drom_entry_header header;
@@ -295,6 +300,32 @@ int tb_drom_read_uid_only(struct tb_switch *sw, u64 *uid)
 	return 0;
 }
 
+static int tb_drom_parse_entry_generic(struct tb_switch *sw,
+		struct tb_drom_entry_header *header)
+{
+	const struct tb_drom_entry_generic *entry =
+		(const struct tb_drom_entry_generic *)header;
+
+	switch (header->index) {
+	case 1:
+		/* Length includes 2 bytes header so remove it before copy */
+		sw->vendor_name = kstrndup(entry->data,
+			header->len - sizeof(*header), GFP_KERNEL);
+		if (!sw->vendor_name)
+			return -ENOMEM;
+		break;
+
+	case 2:
+		sw->device_name = kstrndup(entry->data,
+			header->len - sizeof(*header), GFP_KERNEL);
+		if (!sw->device_name)
+			return -ENOMEM;
+		break;
+	}
+
+	return 0;
+}
+
 static int tb_drom_parse_entry_port(struct tb_switch *sw,
 				    struct tb_drom_entry_header *header)
 {
@@ -350,6 +381,7 @@ static int tb_drom_parse_entries(struct tb_switch *sw)
 
 		switch (entry->type) {
 		case TB_DROM_ENTRY_GENERIC:
+			res = tb_drom_parse_entry_generic(sw, entry);
 			break;
 		case TB_DROM_ENTRY_PORT:
 			res = tb_drom_parse_entry_port(sw, entry);
drivers/thunderbolt/switch.c +22 -0
index 0ce8b600a23f..0a592ccb7923 100644
@@ -319,6 +319,15 @@ static ssize_t device_show(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RO(device);
 
+static ssize_t
+device_name_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct tb_switch *sw = tb_to_switch(dev);
+
+	return sprintf(buf, "%s\n", sw->device_name ? sw->device_name : "");
+}
+static DEVICE_ATTR_RO(device_name);
+
 static ssize_t vendor_show(struct device *dev, struct device_attribute *attr,
 			   char *buf)
 {
@@ -328,6 +337,15 @@ static ssize_t vendor_show(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RO(vendor);
 
+static ssize_t
+vendor_name_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct tb_switch *sw = tb_to_switch(dev);
+
+	return sprintf(buf, "%s\n", sw->vendor_name ? sw->vendor_name : "");
+}
+static DEVICE_ATTR_RO(vendor_name);
+
 static ssize_t unique_id_show(struct device *dev, struct device_attribute *attr,
 			      char *buf)
 {
@@ -339,7 +357,9 @@ static DEVICE_ATTR_RO(unique_id);
 
 static struct attribute *switch_attrs[] = {
 	&dev_attr_device.attr,
+	&dev_attr_device_name.attr,
 	&dev_attr_vendor.attr,
+	&dev_attr_vendor_name.attr,
 	&dev_attr_unique_id.attr,
 	NULL,
 };
@@ -358,6 +378,8 @@ static void tb_switch_release(struct device *dev)
 	struct tb_switch *sw = tb_to_switch(dev);
 
 	kfree(sw->uuid);
+	kfree(sw->device_name);
+	kfree(sw->vendor_name);
 	kfree(sw->ports);
 	kfree(sw->drom);
 	kfree(sw);
drivers/thunderbolt/tb.h +4 -0
index 350c3f21924e..5e66dce53c65 100644
@@ -23,6 +23,8 @@
  * @uuid: UUID of the switch (or %NULL if not supported)
  * @vendor: Vendor ID of the switch
  * @device: Device ID of the switch
+ * @vendor_name: Name of the vendor (or %NULL if not known)
+ * @device_name: Name of the device (or %NULL if not known)
  * @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)
@@ -36,6 +38,8 @@ struct tb_switch {
 	uuid_be *uuid;
 	u16 vendor;
 	u16 device;
+	const char *vendor_name;
+	const char *device_name;
 	int cap_plug_events;
 	bool is_unplugged;
 	u8 *drom;
Signature · 1 line
2.11.0
#21 lore raw

PATCH v3 14/27 thunderbolt: Move control channel messages to tb_msgs.h

In reply to Mika Westerberg

We will be forwarding notifications received from the control channel to the connection manager implementations. This way they can decide what to do if anything when a notification is received.

To be able to use control channel messages from other files, move them to tb_msgs.h.

No functional changes intended.

---
 drivers/thunderbolt/ctl.c     |  76 -----------------------------
 drivers/thunderbolt/ctl.h     |  16 +------
 drivers/thunderbolt/tb_msgs.h | 108 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 109 insertions(+), 91 deletions(-)
 create mode 100644 drivers/thunderbolt/tb_msgs.h
drivers/thunderbolt/ctl.c +0 -76
index f8290a577b2b..24118c60b062 100644
@@ -52,82 +52,6 @@ struct tb_ctl {
 #define tb_ctl_info(ctl, format, arg...) \
 	dev_info(&(ctl)->nhi->pdev->dev, format, ## arg)
 
-
-/* configuration packets definitions */
-
-enum tb_cfg_pkg_type {
-	TB_CFG_PKG_READ = 1,
-	TB_CFG_PKG_WRITE = 2,
-	TB_CFG_PKG_ERROR = 3,
-	TB_CFG_PKG_NOTIFY_ACK = 4,
-	TB_CFG_PKG_EVENT = 5,
-	TB_CFG_PKG_XDOMAIN_REQ = 6,
-	TB_CFG_PKG_XDOMAIN_RESP = 7,
-	TB_CFG_PKG_OVERRIDE = 8,
-	TB_CFG_PKG_RESET = 9,
-	TB_CFG_PKG_PREPARE_TO_SLEEP = 0xd,
-};
-
-/* common header */
-struct tb_cfg_header {
-	u32 route_hi:22;
-	u32 unknown:10; /* highest order bit is set on replies */
-	u32 route_lo;
-} __packed;
-
-/* additional header for read/write packets */
-struct tb_cfg_address {
-	u32 offset:13; /* in dwords */
-	u32 length:6; /* in dwords */
-	u32 port:6;
-	enum tb_cfg_space space:2;
-	u32 seq:2; /* sequence number  */
-	u32 zero:3;
-} __packed;
-
-/* TB_CFG_PKG_READ, response for TB_CFG_PKG_WRITE */
-struct cfg_read_pkg {
-	struct tb_cfg_header header;
-	struct tb_cfg_address addr;
-} __packed;
-
-/* TB_CFG_PKG_WRITE, response for TB_CFG_PKG_READ */
-struct cfg_write_pkg {
-	struct tb_cfg_header header;
-	struct tb_cfg_address addr;
-	u32 data[64]; /* maximum size, tb_cfg_address.length has 6 bits */
-} __packed;
-
-/* TB_CFG_PKG_ERROR */
-struct cfg_error_pkg {
-	struct tb_cfg_header header;
-	enum tb_cfg_error error:4;
-	u32 zero1:4;
-	u32 port:6;
-	u32 zero2:2; /* Both should be zero, still they are different fields. */
-	u32 zero3:16;
-} __packed;
-
-/* TB_CFG_PKG_EVENT */
-struct cfg_event_pkg {
-	struct tb_cfg_header header;
-	u32 port:6;
-	u32 zero:25;
-	bool unplug:1;
-} __packed;
-
-/* TB_CFG_PKG_RESET */
-struct cfg_reset_pkg {
-	struct tb_cfg_header header;
-} __packed;
-
-/* TB_CFG_PKG_PREPARE_TO_SLEEP */
-struct cfg_pts_pkg {
-	struct tb_cfg_header header;
-	u32 data;
-} __packed;
-
-
 /* utility functions */
 
 static u64 get_route(struct tb_cfg_header header)
drivers/thunderbolt/ctl.h +1 -15
index 83ae54947082..610980e3232f 100644
@@ -8,6 +8,7 @@
 #define _TB_CFG
 
 #include "nhi.h"
+#include "tb_msgs.h"
 
 /* control channel */
 struct tb_ctl;
@@ -23,21 +24,6 @@ void tb_ctl_free(struct tb_ctl *ctl);
 
 #define TB_CFG_DEFAULT_TIMEOUT 5000 /* msec */
 
-enum tb_cfg_space {
-	TB_CFG_HOPS = 0,
-	TB_CFG_PORT = 1,
-	TB_CFG_SWITCH = 2,
-	TB_CFG_COUNTERS = 3,
-};
-
-enum tb_cfg_error {
-	TB_CFG_ERROR_PORT_NOT_CONNECTED = 0,
-	TB_CFG_ERROR_INVALID_CONFIG_SPACE = 2,
-	TB_CFG_ERROR_NO_SUCH_PORT = 4,
-	TB_CFG_ERROR_ACK_PLUG_EVENT = 7, /* send as reply to TB_CFG_PKG_EVENT */
-	TB_CFG_ERROR_LOOP = 8,
-};
-
 struct tb_cfg_result {
 	u64 response_route;
 	u32 response_port; /*
drivers/thunderbolt/tb_msgs.h new file +108 -0

108 lines not on this page · raw message

Signature · 1 line
2.11.0
#22 lore raw

PATCH v3 15/27 thunderbolt: Expose get_route() to other files

In reply to Mika Westerberg

We are going to use it when we change the connection manager to handle events itself. Also rename it to follow naming convention used in functions exposed in ctl.h.

---
 drivers/thunderbolt/ctl.c | 19 +++++++------------
 drivers/thunderbolt/ctl.h |  4 ++++
 2 files changed, 11 insertions(+), 12 deletions(-)
drivers/thunderbolt/ctl.c +7 -12
index 24118c60b062..8352ee8662aa 100644
@@ -54,11 +54,6 @@ struct tb_ctl {
 
 /* utility functions */
 
-static u64 get_route(struct tb_cfg_header header)
-{
-	return (u64) header.route_hi << 32 | header.route_lo;
-}
-
 static struct tb_cfg_header make_header(u64 route)
 {
 	struct tb_cfg_header header = {
@@ -66,7 +61,7 @@ static struct tb_cfg_header make_header(u64 route)
 		.route_lo = route,
 	};
 	/* check for overflow, route_hi is not 32 bits! */
-	WARN_ON(get_route(header) != route);
+	WARN_ON(tb_cfg_get_route(&header) != route);
 	return header;
 }
 
@@ -91,9 +86,9 @@ static int check_header(struct ctl_pkg *pkg, u32 len, enum tb_cfg_pkg_type type,
 	if (WARN(header->unknown != 1 << 9,
 			"header->unknown is %#x\n", header->unknown))
 		return -EIO;
-	if (WARN(route != get_route(*header),
+	if (WARN(route != tb_cfg_get_route(header),
 			"wrong route (expected %llx, got %llx)",
-			route, get_route(*header)))
+			route, tb_cfg_get_route(header)))
 		return -EIO;
 	return 0;
 }
@@ -126,10 +121,10 @@ static struct tb_cfg_result decode_error(struct ctl_pkg *response)
 {
 	struct cfg_error_pkg *pkg = response->buffer;
 	struct tb_cfg_result res = { 0 };
-	res.response_route = get_route(pkg->header);
+	res.response_route = tb_cfg_get_route(&pkg->header);
 	res.response_port = 0;
 	res.err = check_header(response, sizeof(*pkg), TB_CFG_PKG_ERROR,
-			       get_route(pkg->header));
+			       tb_cfg_get_route(&pkg->header));
 	if (res.err)
 		return res;
 
@@ -153,7 +148,7 @@ static struct tb_cfg_result parse_header(struct ctl_pkg *pkg, u32 len,
 		return decode_error(pkg);
 
 	res.response_port = 0; /* will be updated later for cfg_read/write */
-	res.response_route = get_route(*header);
+	res.response_route = tb_cfg_get_route(header);
 	res.err = check_header(pkg, len, type, route);
 	return res;
 }
@@ -294,7 +289,7 @@ static void tb_ctl_handle_plug_event(struct tb_ctl *ctl,
 				     struct ctl_pkg *response)
 {
 	struct cfg_event_pkg *pkg = response->buffer;
-	u64 route = get_route(pkg->header);
+	u64 route = tb_cfg_get_route(&pkg->header);
 
 	if (check_header(response, sizeof(*pkg), TB_CFG_PKG_EVENT, route)) {
 		tb_ctl_warn(ctl, "malformed TB_CFG_PKG_EVENT\n");
drivers/thunderbolt/ctl.h +4 -0
index 610980e3232f..9812b1c86d4f 100644
@@ -38,6 +38,10 @@ struct tb_cfg_result {
 	enum tb_cfg_error tb_error; /* valid if err == 1 */
 };
 
+static inline u64 tb_cfg_get_route(const struct tb_cfg_header *header)
+{
+	return (u64) header->route_hi << 32 | header->route_lo;
+}
 
 int tb_cfg_error(struct tb_ctl *ctl, u64 route, u32 port,
 		 enum tb_cfg_error error);
Signature · 1 line
2.11.0
#23 lore raw

PATCH v3 16/27 thunderbolt: Expose make_header() to other files

In reply to Mika Westerberg

We will be using this function in files introduced in subsequent patches. While there the function is renamed to tb_cfg_make_header() following tb_cfg_get_route().

---
 drivers/thunderbolt/ctl.c | 19 ++++---------------
 drivers/thunderbolt/ctl.h | 11 +++++++++++
 2 files changed, 15 insertions(+), 15 deletions(-)
drivers/thunderbolt/ctl.c +4 -15
index 8352ee8662aa..c6633da582b8 100644
@@ -54,17 +54,6 @@ struct tb_ctl {
 
 /* utility functions */
 
-static struct tb_cfg_header make_header(u64 route)
-{
-	struct tb_cfg_header header = {
-		.route_hi = route >> 32,
-		.route_lo = route,
-	};
-	/* check for overflow, route_hi is not 32 bits! */
-	WARN_ON(tb_cfg_get_route(&header) != route);
-	return header;
-}
-
 static int check_header(struct ctl_pkg *pkg, u32 len, enum tb_cfg_pkg_type type,
 			u64 route)
 {
@@ -501,7 +490,7 @@ int tb_cfg_error(struct tb_ctl *ctl, u64 route, u32 port,
 		 enum tb_cfg_error error)
 {
 	struct cfg_error_pkg pkg = {
-		.header = make_header(route),
+		.header = tb_cfg_make_header(route),
 		.port = port,
 		.error = error,
 	};
@@ -520,7 +509,7 @@ struct tb_cfg_result tb_cfg_reset(struct tb_ctl *ctl, u64 route,
 				  int timeout_msec)
 {
 	int err;
-	struct cfg_reset_pkg request = { .header = make_header(route) };
+	struct cfg_reset_pkg request = { .header = tb_cfg_make_header(route) };
 	struct tb_cfg_header reply;
 
 	err = tb_ctl_tx(ctl, &request, sizeof(request), TB_CFG_PKG_RESET);
@@ -542,7 +531,7 @@ struct tb_cfg_result tb_cfg_read_raw(struct tb_ctl *ctl, void *buffer,
 {
 	struct tb_cfg_result res = { 0 };
 	struct cfg_read_pkg request = {
-		.header = make_header(route),
+		.header = tb_cfg_make_header(route),
 		.addr = {
 			.port = port,
 			.space = space,
@@ -579,7 +568,7 @@ struct tb_cfg_result tb_cfg_write_raw(struct tb_ctl *ctl, const void *buffer,
 {
 	struct tb_cfg_result res = { 0 };
 	struct cfg_write_pkg request = {
-		.header = make_header(route),
+		.header = tb_cfg_make_header(route),
 		.addr = {
 			.port = port,
 			.space = space,
drivers/thunderbolt/ctl.h +11 -0
index 9812b1c86d4f..914da86ec77d 100644
@@ -43,6 +43,17 @@ static inline u64 tb_cfg_get_route(const struct tb_cfg_header *header)
 	return (u64) header->route_hi << 32 | header->route_lo;
 }
 
+static inline struct tb_cfg_header tb_cfg_make_header(u64 route)
+{
+	struct tb_cfg_header header = {
+		.route_hi = route >> 32,
+		.route_lo = route,
+	};
+	/* check for overflow, route_hi is not 32 bits! */
+	WARN_ON(tb_cfg_get_route(&header) != route);
+	return header;
+}
+
 int tb_cfg_error(struct tb_ctl *ctl, u64 route, u32 port,
 		 enum tb_cfg_error error);
 struct tb_cfg_result tb_cfg_reset(struct tb_ctl *ctl, u64 route,
Signature · 1 line
2.11.0
#24 lore raw

PATCH v3 17/27 thunderbolt: Let the connection manager handle all notifications

In reply to Mika Westerberg

Currently the control channel (ctl.c) handles the one supported notification (PLUG_EVENT) and sends back ACK accordingly. However, we are going to add support for the internal connection manager (ICM) that needs to handle a different notifications. So instead of dealing everything in the control channel, we change the callback to take an arbitrary thunderbolt packet and convert the native connection manager to handle the event itself.

In addition we only push replies we know of to the response FIFO. Everything else is treated as notification (or request) and is expected to be dealt by the connection manager implementation.

---
 drivers/thunderbolt/ctl.c    | 86 ++++++++++++++++++++++++++++++--------------
 drivers/thunderbolt/ctl.h    |  5 +--
 drivers/thunderbolt/domain.c | 15 +++++++-
 drivers/thunderbolt/tb.c     | 30 ++++++++++++----
 drivers/thunderbolt/tb.h     |  5 +--
 5 files changed, 103 insertions(+), 38 deletions(-)
drivers/thunderbolt/ctl.c +60 -26
index c6633da582b8..5417ed244edc 100644
@@ -35,7 +35,7 @@ struct tb_ctl {
 	DECLARE_KFIFO(response_fifo, struct ctl_pkg*, 16);
 	struct completion response_ready;
 
-	hotplug_cb callback;
+	event_cb callback;
 	void *callback_data;
 };
 
@@ -52,6 +52,9 @@ struct tb_ctl {
 #define tb_ctl_info(ctl, format, arg...) \
 	dev_info(&(ctl)->nhi->pdev->dev, format, ## arg)
 
+#define tb_ctl_dbg(ctl, format, arg...) \
+	dev_dbg(&(ctl)->nhi->pdev->dev, format, ## arg)
+
 /* utility functions */
 
 static int check_header(struct ctl_pkg *pkg, u32 len, enum tb_cfg_pkg_type type,
@@ -272,24 +275,12 @@ static int tb_ctl_tx(struct tb_ctl *ctl, const void *data, size_t len,
 }
 
 /**
- * tb_ctl_handle_plug_event() - acknowledge a plug event, invoke ctl->callback
+ * tb_ctl_handle_event() - acknowledge a plug event, invoke ctl->callback
  */
-static void tb_ctl_handle_plug_event(struct tb_ctl *ctl,
-				     struct ctl_pkg *response)
+static void tb_ctl_handle_event(struct tb_ctl *ctl, enum tb_cfg_pkg_type type,
+				struct ctl_pkg *pkg, size_t size)
 {
-	struct cfg_event_pkg *pkg = response->buffer;
-	u64 route = tb_cfg_get_route(&pkg->header);
-
-	if (check_header(response, sizeof(*pkg), TB_CFG_PKG_EVENT, route)) {
-		tb_ctl_warn(ctl, "malformed TB_CFG_PKG_EVENT\n");
-		return;
-	}
-
-	if (tb_cfg_error(ctl, route, pkg->port, TB_CFG_ERROR_ACK_PLUG_EVENT))
-		tb_ctl_warn(ctl, "could not ack plug event on %llx:%x\n",
-			    route, pkg->port);
-	WARN(pkg->zero, "pkg->zero is %#x\n", pkg->zero);
-	ctl->callback(ctl->callback_data, route, pkg->port, pkg->unplug);
+	ctl->callback(ctl->callback_data, type, pkg->buffer, size);
 }
 
 static void tb_ctl_rx_submit(struct ctl_pkg *pkg)
@@ -302,10 +293,29 @@ static void tb_ctl_rx_submit(struct ctl_pkg *pkg)
 					     */
 }
 
+static int tb_async_error(const struct ctl_pkg *pkg)
+{
+	const struct cfg_error_pkg *error = (const struct cfg_error_pkg *)pkg;
+
+	if (pkg->frame.eof != TB_CFG_PKG_ERROR)
+		return false;
+
+	switch (error->error) {
+	case TB_CFG_ERROR_LINK_ERROR:
+	case TB_CFG_ERROR_HEC_ERROR_DETECTED:
+	case TB_CFG_ERROR_FLOW_CONTROL_ERROR:
+		return true;
+
+	default:
+		return false;
+	}
+}
+
 static void tb_ctl_rx_callback(struct tb_ring *ring, struct ring_frame *frame,
 			       bool canceled)
 {
 	struct ctl_pkg *pkg = container_of(frame, typeof(*pkg), frame);
+	__be32 crc32;
 
 	if (canceled)
 		return; /*
@@ -320,18 +330,42 @@ static void tb_ctl_rx_callback(struct tb_ring *ring, struct ring_frame *frame,
 	}
 
 	frame->size -= 4; /* remove checksum */
-	if (*(__be32 *) (pkg->buffer + frame->size)
-			!= tb_crc(pkg->buffer, frame->size)) {
-		tb_ctl_err(pkg->ctl,
-			   "RX: checksum mismatch, dropping packet\n");
-		goto rx;
-	}
+	crc32 = tb_crc(pkg->buffer, frame->size);
 	be32_to_cpu_array(pkg->buffer, pkg->buffer, frame->size / 4);
 
-	if (frame->eof == TB_CFG_PKG_EVENT) {
-		tb_ctl_handle_plug_event(pkg->ctl, pkg);
+	switch (frame->eof) {
+	case TB_CFG_PKG_READ:
+	case TB_CFG_PKG_WRITE:
+	case TB_CFG_PKG_ERROR:
+	case TB_CFG_PKG_OVERRIDE:
+	case TB_CFG_PKG_RESET:
+		if (*(__be32 *)(pkg->buffer + frame->size) != crc32) {
+			tb_ctl_err(pkg->ctl,
+				   "RX: checksum mismatch, dropping packet\n");
+			goto rx;
+		}
+		if (tb_async_error(pkg)) {
+			tb_ctl_handle_event(pkg->ctl, frame->eof,
+					    pkg, frame->size);
+			goto rx;
+		}
+		break;
+
+	case TB_CFG_PKG_EVENT:
+		if (*(__be32 *)(pkg->buffer + frame->size) != crc32) {
+			tb_ctl_err(pkg->ctl,
+				   "RX: checksum mismatch, dropping packet\n");
+			goto rx;
+		}
+		tb_ctl_handle_event(pkg->ctl, frame->eof, pkg, frame->size);
+		goto rx;
+
+	default:
+		tb_ctl_dbg(pkg->ctl, "RX: unknown package %#x, dropping\n",
+			   frame->eof);
 		goto rx;
 	}
+
 	if (!kfifo_put(&pkg->ctl->response_fifo, pkg)) {
 		tb_ctl_err(pkg->ctl, "RX: fifo is full\n");
 		goto rx;
@@ -379,7 +413,7 @@ static struct tb_cfg_result tb_ctl_rx(struct tb_ctl *ctl, void *buffer,
  *
  * Return: Returns a pointer on success or NULL on failure.
  */
-struct tb_ctl *tb_ctl_alloc(struct tb_nhi *nhi, hotplug_cb cb, void *cb_data)
+struct tb_ctl *tb_ctl_alloc(struct tb_nhi *nhi, event_cb cb, void *cb_data)
 {
 	int i;
 	struct tb_ctl *ctl = kzalloc(sizeof(*ctl), GFP_KERNEL);
drivers/thunderbolt/ctl.h +3 -2
index 914da86ec77d..2b23e030a85b 100644
@@ -13,9 +13,10 @@
 /* control channel */
 struct tb_ctl;
 
-typedef void (*hotplug_cb)(void *data, u64 route, u8 port, bool unplug);
+typedef void (*event_cb)(void *data, enum tb_cfg_pkg_type type,
+			 const void *buf, size_t size);
 
-struct tb_ctl *tb_ctl_alloc(struct tb_nhi *nhi, hotplug_cb cb, void *cb_data);
+struct tb_ctl *tb_ctl_alloc(struct tb_nhi *nhi, event_cb cb, void *cb_data);
 void tb_ctl_start(struct tb_ctl *ctl);
 void tb_ctl_stop(struct tb_ctl *ctl);
 void tb_ctl_free(struct tb_ctl *ctl);
drivers/thunderbolt/domain.c +14 -1
index e2f3777edee6..8ba6999c3165 100644
@@ -94,6 +94,19 @@ struct tb *tb_domain_alloc(struct tb_nhi *nhi, size_t privsize)
 	return NULL;
 }
 
+static void tb_domain_event_cb(void *data, enum tb_cfg_pkg_type type,
+			       const void *buf, size_t size)
+{
+	struct tb *tb = data;
+
+	if (!tb->cm_ops->handle_event) {
+		tb_warn(tb, "domain does not have event handler\n");
+		return;
+	}
+
+	tb->cm_ops->handle_event(tb, type, buf, size);
+}
+
 /**
  * tb_domain_add() - Add domain to the system
  * @tb: Domain to add
@@ -114,7 +127,7 @@ int tb_domain_add(struct tb *tb)
 
 	mutex_lock(&tb->lock);
 
-	tb->ctl = tb_ctl_alloc(tb->nhi, tb->cm_ops->hotplug, tb);
+	tb->ctl = tb_ctl_alloc(tb->nhi, tb_domain_event_cb, tb);
 	if (!tb->ctl) {
 		ret = -ENOMEM;
 		goto err_unlock;
drivers/thunderbolt/tb.c +23 -7
index 94ecac012428..ea9de49b5e10 100644
@@ -311,18 +311,34 @@ static void tb_handle_hotplug(struct work_struct *work)
  *
  * Delegates to tb_handle_hotplug.
  */
-static void tb_schedule_hotplug_handler(void *data, u64 route, u8 port,
-					bool unplug)
+static void tb_handle_event(struct tb *tb, enum tb_cfg_pkg_type type,
+			    const void *buf, size_t size)
 {
-	struct tb *tb = data;
-	struct tb_hotplug_event *ev = kmalloc(sizeof(*ev), GFP_KERNEL);
+	const struct cfg_event_pkg *pkg = buf;
+	struct tb_hotplug_event *ev;
+	u64 route;
+
+	if (type != TB_CFG_PKG_EVENT) {
+		tb_warn(tb, "unexpected event %#x, ignoring\n", type);
+		return;
+	}
+
+	route = tb_cfg_get_route(&pkg->header);
+
+	if (tb_cfg_error(tb->ctl, route, pkg->port,
+			 TB_CFG_ERROR_ACK_PLUG_EVENT)) {
+		tb_warn(tb, "could not ack plug event on %llx:%x\n", route,
+			pkg->port);
+	}
+
+	ev = kmalloc(sizeof(*ev), GFP_KERNEL);
 	if (!ev)
 		return;
 	INIT_WORK(&ev->work, tb_handle_hotplug);
 	ev->tb = tb;
 	ev->route = route;
-	ev->port = port;
-	ev->unplug = unplug;
+	ev->port = pkg->port;
+	ev->unplug = pkg->unplug;
 	queue_work(tb->wq, &ev->work);
 }
 
@@ -419,7 +435,7 @@ static const struct tb_cm_ops tb_cm_ops = {
 	.stop = tb_stop,
 	.suspend_noirq = tb_suspend_noirq,
 	.resume_noirq = tb_resume_noirq,
-	.hotplug = tb_schedule_hotplug_handler,
+	.handle_event = tb_handle_event,
 };
 
 struct tb *tb_probe(struct tb_nhi *nhi)
drivers/thunderbolt/tb.h +3 -2
index 5e66dce53c65..73db614fa913 100644
@@ -118,14 +118,15 @@ struct tb_path {
  * @stop: Stops the domain
  * @suspend_noirq: Connection manager specific suspend_noirq
  * @resume_noirq: Connection manager specific resume_noirq
- * @hotplug: Handle hotplug event
+ * @handle_event: Handle thunderbolt event
  */
 struct tb_cm_ops {
 	int (*start)(struct tb *tb);
 	void (*stop)(struct tb *tb);
 	int (*suspend_noirq)(struct tb *tb);
 	int (*resume_noirq)(struct tb *tb);
-	hotplug_cb hotplug;
+	void (*handle_event)(struct tb *tb, enum tb_cfg_pkg_type,
+			     const void *buf, size_t size);
 };
 
 /**
Signature · 1 line
2.11.0
#25 lore raw

PATCH v3 18/27 thunderbolt: Rework control channel to be more reliable

In reply to Mika Westerberg

If a request times out the response might arrive right after the request is failed. This response is pushed to the kfifo and next request will read it instead. Since it most likely will not pass our validation checks in parse_header() the next request will fail as well, and response to that request will be pushed to the kfifo, ad infinitum.

We end up in a situation where all requests fail and no devices can be added anymore until the driver is unloaded and reloaded again.

To overcome this, rework the control channel so that we will have a queue of outstanding requests. Each request will be handled in turn and the response is validated against what is expected. Unexpected packets (for example responses for requests that have been timed out) are dropped. This model is copied from Greybus implementation with small changes here and there to get it cope with Thunderbolt control packets.

In addition the configuration packets support sequence number which the switch is supposed to copy from the request to response. We use this to drop responses that are already timed out. Taking advantage of the sequence number, we automatically retry configuration read/write 4 times before giving up.

Also timeout is not a programming error so there is no need to trigger a scary backtrace (WARN), instead we just log a warning. After all Thunderbolt devices are hot-pluggable by definition which means user can unplug a device any time and that is totally acceptable.

With this change there is no need to take the global domain lock when sending configuration packets anymore. This is useful when we add support for cross-domain (XDomain) communication later on.

---
 drivers/thunderbolt/ctl.c | 477 +++++++++++++++++++++++++++++++++++++++-------
 drivers/thunderbolt/ctl.h |  65 +++++++
 drivers/thunderbolt/tb.h  |   2 +-
 3 files changed, 473 insertions(+), 71 deletions(-)
drivers/thunderbolt/ctl.c +407 -70
index 5417ed244edc..27c30ff79a84 100644
@@ -5,22 +5,17 @@
  */
 
 #include <linux/crc32.h>
+#include <linux/delay.h>
 #include <linux/slab.h>
 #include <linux/pci.h>
 #include <linux/dmapool.h>
 #include <linux/workqueue.h>
-#include <linux/kfifo.h>
 
 #include "ctl.h"
 
 
-struct ctl_pkg {
-	struct tb_ctl *ctl;
-	void *buffer;
-	struct ring_frame frame;
-};
-
-#define TB_CTL_RX_PKG_COUNT 10
+#define TB_CTL_RX_PKG_COUNT	10
+#define TB_CTL_RETRIES		4
 
 /**
  * struct tb_cfg - thunderbolt control channel
@@ -32,8 +27,9 @@ struct tb_ctl {
 
 	struct dma_pool *frame_pool;
 	struct ctl_pkg *rx_packets[TB_CTL_RX_PKG_COUNT];
-	DECLARE_KFIFO(response_fifo, struct ctl_pkg*, 16);
-	struct completion response_ready;
+	struct mutex request_queue_lock;
+	struct list_head request_queue;
+	bool running;
 
 	event_cb callback;
 	void *callback_data;
@@ -55,10 +51,121 @@ struct tb_ctl {
 #define tb_ctl_dbg(ctl, format, arg...) \
 	dev_dbg(&(ctl)->nhi->pdev->dev, format, ## arg)
 
+static DECLARE_WAIT_QUEUE_HEAD(tb_cfg_request_cancel_queue);
+/* Serializes access to request kref_get/put */
+static DEFINE_MUTEX(tb_cfg_request_lock);
+
+/**
+ * tb_cfg_request_alloc() - Allocates a new config request
+ *
+ * This is refcounted object so when you are done with this, call
+ * tb_cfg_request_put() to it.
+ */
+struct tb_cfg_request *tb_cfg_request_alloc(void)
+{
+	struct tb_cfg_request *req;
+
+	req = kzalloc(sizeof(*req), GFP_KERNEL);
+	if (!req)
+		return NULL;
+
+	kref_init(&req->kref);
+
+	return req;
+}
+
+/**
+ * tb_cfg_request_get() - Increase refcount of a request
+ * @req: Request whose refcount is increased
+ */
+void tb_cfg_request_get(struct tb_cfg_request *req)
+{
+	mutex_lock(&tb_cfg_request_lock);
+	kref_get(&req->kref);
+	mutex_unlock(&tb_cfg_request_lock);
+}
+
+static void tb_cfg_request_destroy(struct kref *kref)
+{
+	struct tb_cfg_request *req = container_of(kref, typeof(*req), kref);
+
+	kfree(req);
+}
+
+/**
+ * tb_cfg_request_put() - Decrease refcount and possibly release the request
+ * @req: Request whose refcount is decreased
+ *
+ * Call this function when you are done with the request. When refcount
+ * goes to %0 the object is released.
+ */
+void tb_cfg_request_put(struct tb_cfg_request *req)
+{
+	mutex_lock(&tb_cfg_request_lock);
+	kref_put(&req->kref, tb_cfg_request_destroy);
+	mutex_unlock(&tb_cfg_request_lock);
+}
+
+static int tb_cfg_request_enqueue(struct tb_ctl *ctl,
+				  struct tb_cfg_request *req)
+{
+	WARN_ON(test_bit(TB_CFG_REQUEST_ACTIVE, &req->flags));
+	WARN_ON(req->ctl);
+
+	mutex_lock(&ctl->request_queue_lock);
+	if (!ctl->running) {
+		mutex_unlock(&ctl->request_queue_lock);
+		return -ENOTCONN;
+	}
+	req->ctl = ctl;
+	list_add_tail(&req->list, &ctl->request_queue);
+	set_bit(TB_CFG_REQUEST_ACTIVE, &req->flags);
+	mutex_unlock(&ctl->request_queue_lock);
+	return 0;
+}
+
+static void tb_cfg_request_dequeue(struct tb_cfg_request *req)
+{
+	struct tb_ctl *ctl = req->ctl;
+
+	mutex_lock(&ctl->request_queue_lock);
+	list_del(&req->list);
+	clear_bit(TB_CFG_REQUEST_ACTIVE, &req->flags);
+	if (test_bit(TB_CFG_REQUEST_CANCELED, &req->flags))
+		wake_up(&tb_cfg_request_cancel_queue);
+	mutex_unlock(&ctl->request_queue_lock);
+}
+
+static bool tb_cfg_request_is_active(struct tb_cfg_request *req)
+{
+	return test_bit(TB_CFG_REQUEST_ACTIVE, &req->flags);
+}
+
+static struct tb_cfg_request *
+tb_cfg_request_find(struct tb_ctl *ctl, struct ctl_pkg *pkg)
+{
+	struct tb_cfg_request *req;
+	bool found = false;
+
+	mutex_lock(&pkg->ctl->request_queue_lock);
+	list_for_each_entry(req, &pkg->ctl->request_queue, list) {
+		tb_cfg_request_get(req);
+		if (req->match(req, pkg)) {
+			found = true;
+			break;
+		}
+		tb_cfg_request_put(req);
+	}
+	mutex_unlock(&pkg->ctl->request_queue_lock);
+
+	return found ? req : NULL;
+}
+
 /* utility functions */
 
-static int check_header(struct ctl_pkg *pkg, u32 len, enum tb_cfg_pkg_type type,
-			u64 route)
+
+static int check_header(const struct ctl_pkg *pkg, u32 len,
+			enum tb_cfg_pkg_type type, u64 route)
 {
 	struct tb_cfg_header *header = pkg->buffer;
 
@@ -100,8 +207,6 @@ static int check_config_address(struct tb_cfg_address addr,
 	if (WARN(length != addr.length, "wrong space (expected %x, got %x\n)",
 			length, addr.length))
 		return -EIO;
-	if (WARN(addr.seq, "addr.seq is %#x\n", addr.seq))
-		return -EIO;
 	/*
 	 * We cannot check addr->port as it is set to the upstream port of the
 	 * sender.
@@ -109,7 +214,7 @@ static int check_config_address(struct tb_cfg_address addr,
 	return 0;
 }
 
-static struct tb_cfg_result decode_error(struct ctl_pkg *response)
+static struct tb_cfg_result decode_error(const struct ctl_pkg *response)
 {
 	struct cfg_error_pkg *pkg = response->buffer;
 	struct tb_cfg_result res = { 0 };
@@ -130,7 +235,7 @@ static struct tb_cfg_result decode_error(struct ctl_pkg *response)
 
 }
 
-static struct tb_cfg_result parse_header(struct ctl_pkg *pkg, u32 len,
+static struct tb_cfg_result parse_header(const struct ctl_pkg *pkg, u32 len,
 					 enum tb_cfg_pkg_type type, u64 route)
 {
 	struct tb_cfg_header *header = pkg->buffer;
@@ -198,7 +303,7 @@ static void be32_to_cpu_array(u32 *dst, __be32 *src, size_t len)
 		dst[i] = be32_to_cpu(src[i]);
 }
 
-static __be32 tb_crc(void *data, size_t len)
+static __be32 tb_crc(const void *data, size_t len)
 {
 	return cpu_to_be32(~__crc32c_le(~0, data, len));
 }
@@ -315,6 +420,7 @@ static void tb_ctl_rx_callback(struct tb_ring *ring, struct ring_frame *frame,
 			       bool canceled)
 {
 	struct ctl_pkg *pkg = container_of(frame, typeof(*pkg), frame);
+	struct tb_cfg_request *req;
 	__be32 crc32;
 
 	if (canceled)
@@ -361,48 +467,135 @@ static void tb_ctl_rx_callback(struct tb_ring *ring, struct ring_frame *frame,
 		goto rx;
 
 	default:
-		tb_ctl_dbg(pkg->ctl, "RX: unknown package %#x, dropping\n",
-			   frame->eof);
-		goto rx;
+		break;
 	}
 
-	if (!kfifo_put(&pkg->ctl->response_fifo, pkg)) {
-		tb_ctl_err(pkg->ctl, "RX: fifo is full\n");
-		goto rx;
+	/*
+	 * The received packet will be processed only if there is an
+	 * active request and that the packet is what is expected. This
+	 * prevents packets such as replies coming after timeout has
+	 * triggered from messing with the active requests.
+	 */
+	req = tb_cfg_request_find(pkg->ctl, pkg);
+	if (req) {
+		if (req->copy(req, pkg))
+			schedule_work(&req->work);
+		tb_cfg_request_put(req);
 	}
-	complete(&pkg->ctl->response_ready);
-	return;
+
 rx:
 	tb_ctl_rx_submit(pkg);
 }
 
+static void tb_cfg_request_work(struct work_struct *work)
+{
+	struct tb_cfg_request *req = container_of(work, typeof(*req), work);
+
+	if (!test_bit(TB_CFG_REQUEST_CANCELED, &req->flags))
+		req->callback(req->callback_data);
+
+	tb_cfg_request_dequeue(req);
+	tb_cfg_request_put(req);
+}
+
 /**
- * tb_ctl_rx() - receive a packet from the control channel
+ * tb_cfg_request() - Start control request not waiting for it to complete
+ * @ctl: Control channel to use
+ * @req: Request to start
+ * @callback: Callback called when the request is completed
+ * @callback_data: Data to be passed to @callback
+ *
+ * This queues @req on the given control channel without waiting for it
+ * to complete. When the request completes @callback is called.
  */
-static struct tb_cfg_result tb_ctl_rx(struct tb_ctl *ctl, void *buffer,
-				      size_t length, int timeout_msec,
-				      u64 route, enum tb_cfg_pkg_type type)
+int tb_cfg_request(struct tb_ctl *ctl, struct tb_cfg_request *req,
+		   void (*callback)(void *), void *callback_data)
 {
-	struct tb_cfg_result res;
-	struct ctl_pkg *pkg;
+	int ret;
 
-	if (!wait_for_completion_timeout(&ctl->response_ready,
-					 msecs_to_jiffies(timeout_msec))) {
-		tb_ctl_WARN(ctl, "RX: timeout\n");
-		return (struct tb_cfg_result) { .err = -ETIMEDOUT };
-	}
-	if (!kfifo_get(&ctl->response_fifo, &pkg)) {
-		tb_ctl_WARN(ctl, "empty kfifo\n");
-		return (struct tb_cfg_result) { .err = -EIO };
-	}
+	req->flags = 0;
+	req->callback = callback;
+	req->callback_data = callback_data;
+	INIT_WORK(&req->work, tb_cfg_request_work);
+	INIT_LIST_HEAD(&req->list);
 
-	res = parse_header(pkg, length, type, route);
-	if (!res.err)
-		memcpy(buffer, pkg->buffer, length);
-	tb_ctl_rx_submit(pkg);
-	return res;
+	tb_cfg_request_get(req);
+	ret = tb_cfg_request_enqueue(ctl, req);
+	if (ret)
+		goto err_put;
+
+	ret = tb_ctl_tx(ctl, req->request, req->request_size,
+			req->request_type);
+	if (ret)
+		goto err_dequeue;
+
+	if (!req->response)
+		schedule_work(&req->work);
+
+	return 0;
+
+err_dequeue:
+	tb_cfg_request_dequeue(req);
+err_put:
+	tb_cfg_request_put(req);
+
+	return ret;
+}
+
+/**
+ * tb_cfg_request_cancel() - Cancel a control request
+ * @req: Request to cancel
+ * @err: Error to assign to the request
+ *
+ * This function can be used to cancel ongoing request. It will wait
+ * until the request is not active anymore.
+ */
+void tb_cfg_request_cancel(struct tb_cfg_request *req, int err)
+{
+	set_bit(TB_CFG_REQUEST_CANCELED, &req->flags);
+	schedule_work(&req->work);
+	wait_event(tb_cfg_request_cancel_queue, !tb_cfg_request_is_active(req));
+	req->result.err = err;
 }
 
+static void tb_cfg_request_complete(void *data)
+{
+	complete(data);
+}
+
+/**
+ * tb_cfg_request_sync() - Start control request and wait until it completes
+ * @ctl: Control channel to use
+ * @req: Request to start
+ * @timeout_msec: Timeout how long to wait @req to complete
+ *
+ * Starts a control request and waits until it completes. If timeout
+ * triggers the request is canceled before function returns. Note the
+ * caller needs to make sure only one message for given switch is active
+ * at a time.
+ */
+struct tb_cfg_result tb_cfg_request_sync(struct tb_ctl *ctl,
+					 struct tb_cfg_request *req,
+					 int timeout_msec)
+{
+	unsigned long timeout = msecs_to_jiffies(timeout_msec);
+	struct tb_cfg_result res = { 0 };
+	DECLARE_COMPLETION_ONSTACK(done);
+	int ret;
+
+	ret = tb_cfg_request(ctl, req, tb_cfg_request_complete, &done);
+	if (ret) {
+		res.err = ret;
+		return res;
+	}
+
+	if (!wait_for_completion_timeout(&done, timeout))
+		tb_cfg_request_cancel(req, -ETIMEDOUT);
+
+	flush_work(&req->work);
+
+	return req->result;
+}
 
 /* public interface, alloc/start/stop/free */
 
@@ -423,8 +616,8 @@ struct tb_ctl *tb_ctl_alloc(struct tb_nhi *nhi, event_cb cb, void *cb_data)
 	ctl->callback = cb;
 	ctl->callback_data = cb_data;
 
-	init_completion(&ctl->response_ready);
-	INIT_KFIFO(ctl->response_fifo);
+	mutex_init(&ctl->request_queue_lock);
+	INIT_LIST_HEAD(&ctl->request_queue);
 	ctl->frame_pool = dma_pool_create("thunderbolt_ctl", &nhi->pdev->dev,
 					 TB_FRAME_SIZE, 4, 0);
 	if (!ctl->frame_pool)
@@ -492,6 +685,8 @@ void tb_ctl_start(struct tb_ctl *ctl)
 	ring_start(ctl->rx);
 	for (i = 0; i < TB_CTL_RX_PKG_COUNT; i++)
 		tb_ctl_rx_submit(ctl->rx_packets[i]);
+
+	ctl->running = true;
 }
 
 /**
@@ -504,12 +699,16 @@ void tb_ctl_start(struct tb_ctl *ctl)
  */
 void tb_ctl_stop(struct tb_ctl *ctl)
 {
+	mutex_lock(&ctl->request_queue_lock);
+	ctl->running = false;
+	mutex_unlock(&ctl->request_queue_lock);
+
 	ring_stop(ctl->rx);
 	ring_stop(ctl->tx);
 
-	if (!kfifo_is_empty(&ctl->response_fifo))
-		tb_ctl_WARN(ctl, "dangling response in response_fifo\n");
-	kfifo_reset(&ctl->response_fifo);
+	if (!list_empty(&ctl->request_queue))
+		tb_ctl_WARN(ctl, "dangling request in request_queue\n");
+	INIT_LIST_HEAD(&ctl->request_queue);
 	tb_ctl_info(ctl, "control channel stopped\n");
 }
 
@@ -532,6 +731,49 @@ int tb_cfg_error(struct tb_ctl *ctl, u64 route, u32 port,
 	return tb_ctl_tx(ctl, &pkg, sizeof(pkg), TB_CFG_PKG_ERROR);
 }
 
+static bool tb_cfg_match(const struct tb_cfg_request *req,
+			 const struct ctl_pkg *pkg)
+{
+	u64 route = tb_cfg_get_route(pkg->buffer) & ~BIT_ULL(63);
+
+	if (pkg->frame.eof == TB_CFG_PKG_ERROR)
+		return true;
+
+	if (pkg->frame.eof != req->response_type)
+		return false;
+	if (route != tb_cfg_get_route(req->request))
+		return false;
+	if (pkg->frame.size != req->response_size)
+		return false;
+
+	if (pkg->frame.eof == TB_CFG_PKG_READ ||
+	    pkg->frame.eof == TB_CFG_PKG_WRITE) {
+		const struct cfg_read_pkg *req_hdr = req->request;
+		const struct cfg_read_pkg *res_hdr = pkg->buffer;
+
+		if (req_hdr->addr.seq != res_hdr->addr.seq)
+			return false;
+	}
+
+	return true;
+}
+
+static bool tb_cfg_copy(struct tb_cfg_request *req, const struct ctl_pkg *pkg)
+{
+	struct tb_cfg_result res;
+
+	/* Now make sure it is in expected format */
+	res = parse_header(pkg, req->response_size, req->response_type,
+			   tb_cfg_get_route(req->request));
+	if (!res.err)
+		memcpy(req->response, pkg->buffer, req->response_size);
+
+	req->result = res;
+
+	/* Always complete when first response is received */
+	return true;
+}
+
 /**
  * tb_cfg_reset() - send a reset packet and wait for a response
  *
@@ -542,16 +784,31 @@ int tb_cfg_error(struct tb_ctl *ctl, u64 route, u32 port,
 struct tb_cfg_result tb_cfg_reset(struct tb_ctl *ctl, u64 route,
 				  int timeout_msec)
 {
-	int err;
 	struct cfg_reset_pkg request = { .header = tb_cfg_make_header(route) };
+	struct tb_cfg_result res = { 0 };
 	struct tb_cfg_header reply;
+	struct tb_cfg_request *req;
+
+	req = tb_cfg_request_alloc();
+	if (!req) {
+		res.err = -ENOMEM;
+		return res;
+	}
+
+	req->match = tb_cfg_match;
+	req->copy = tb_cfg_copy;
+	req->request = &request;
+	req->request_size = sizeof(request);
+	req->request_type = TB_CFG_PKG_RESET;
+	req->response = &reply;
+	req->response_size = sizeof(reply);
+	req->response_type = sizeof(TB_CFG_PKG_RESET);
+
+	res = tb_cfg_request_sync(ctl, req, timeout_msec);
 
-	err = tb_ctl_tx(ctl, &request, sizeof(request), TB_CFG_PKG_RESET);
-	if (err)
-		return (struct tb_cfg_result) { .err = err };
+	tb_cfg_request_put(req);
 
-	return tb_ctl_rx(ctl, &reply, sizeof(reply), timeout_msec, route,
-			 TB_CFG_PKG_RESET);
+	return res;
 }
 
 /**
@@ -574,13 +831,39 @@ struct tb_cfg_result tb_cfg_read_raw(struct tb_ctl *ctl, void *buffer,
 		},
 	};
 	struct cfg_write_pkg reply;
+	int retries = 0;
 
-	res.err = tb_ctl_tx(ctl, &request, sizeof(request), TB_CFG_PKG_READ);
-	if (res.err)
-		return res;
+	while (retries < TB_CTL_RETRIES) {
+		struct tb_cfg_request *req;
+
+		req = tb_cfg_request_alloc();
+		if (!req) {
+			res.err = -ENOMEM;
+			return res;
+		}
+
+		request.addr.seq = retries++;
+
+		req->match = tb_cfg_match;
+		req->copy = tb_cfg_copy;
+		req->request = &request;
+		req->request_size = sizeof(request);
+		req->request_type = TB_CFG_PKG_READ;
+		req->response = &reply;
+		req->response_size = 12 + 4 * length;
+		req->response_type = TB_CFG_PKG_READ;
+
+		res = tb_cfg_request_sync(ctl, req, timeout_msec);
+
+		tb_cfg_request_put(req);
+
+		if (res.err != -ETIMEDOUT)
+			break;
+
+		/* Wait a bit (arbitrary time) until we send a retry */
+		usleep_range(10, 100);
+	}
 
-	res = tb_ctl_rx(ctl, &reply, 12 + 4 * length, timeout_msec, route,
-			TB_CFG_PKG_READ);
 	if (res.err)
 		return res;
 
@@ -611,15 +894,41 @@ struct tb_cfg_result tb_cfg_write_raw(struct tb_ctl *ctl, const void *buffer,
 		},
 	};
 	struct cfg_read_pkg reply;
+	int retries = 0;
 
 	memcpy(&request.data, buffer, length * 4);
 
-	res.err = tb_ctl_tx(ctl, &request, 12 + 4 * length, TB_CFG_PKG_WRITE);
-	if (res.err)
-		return res;
+	while (retries < TB_CTL_RETRIES) {
+		struct tb_cfg_request *req;
+
+		req = tb_cfg_request_alloc();
+		if (!req) {
+			res.err = -ENOMEM;
+			return res;
+		}
+
+		request.addr.seq = retries++;
+
+		req->match = tb_cfg_match;
+		req->copy = tb_cfg_copy;
+		req->request = &request;
+		req->request_size = 12 + 4 * length;
+		req->request_type = TB_CFG_PKG_WRITE;
+		req->response = &reply;
+		req->response_size = sizeof(reply);
+		req->response_type = TB_CFG_PKG_WRITE;
+
+		res = tb_cfg_request_sync(ctl, req, timeout_msec);
+
+		tb_cfg_request_put(req);
+
+		if (res.err != -ETIMEDOUT)
+			break;
+
+		/* Wait a bit (arbitrary time) until we send a retry */
+		usleep_range(10, 100);
+	}
 
-	res = tb_ctl_rx(ctl, &reply, sizeof(reply), timeout_msec, route,
-			TB_CFG_PKG_WRITE);
 	if (res.err)
 		return res;
 
@@ -633,11 +942,25 @@ int tb_cfg_read(struct tb_ctl *ctl, void *buffer, u64 route, u32 port,
 {
 	struct tb_cfg_result res = tb_cfg_read_raw(ctl, buffer, route, port,
 			space, offset, length, TB_CFG_DEFAULT_TIMEOUT);
-	if (res.err == 1) {
+	switch (res.err) {
+	case 0:
+		/* Success */
+		break;
+
+	case 1:
+		/* Thunderbolt error, tb_error holds the actual number */
 		tb_cfg_print_error(ctl, &res);
 		return -EIO;
+
+	case -ETIMEDOUT:
+		tb_ctl_warn(ctl, "timeout reading config space %u from %#x\n",
+			    space, offset);
+		break;
+
+	default:
+		WARN(1, "tb_cfg_read: %d\n", res.err);
+		break;
 	}
-	WARN(res.err, "tb_cfg_read: %d\n", res.err);
 	return res.err;
 }
 
@@ -646,11 +969,25 @@ int tb_cfg_write(struct tb_ctl *ctl, const void *buffer, u64 route, u32 port,
 {
 	struct tb_cfg_result res = tb_cfg_write_raw(ctl, buffer, route, port,
 			space, offset, length, TB_CFG_DEFAULT_TIMEOUT);
-	if (res.err == 1) {
+	switch (res.err) {
+	case 0:
+		/* Success */
+		break;
+
+	case 1:
+		/* Thunderbolt error, tb_error holds the actual number */
 		tb_cfg_print_error(ctl, &res);
 		return -EIO;
+
+	case -ETIMEDOUT:
+		tb_ctl_warn(ctl, "timeout writing config space %u to %#x\n",
+			    space, offset);
+		break;
+
+	default:
+		WARN(1, "tb_cfg_write: %d\n", res.err);
+		break;
 	}
-	WARN(res.err, "tb_cfg_write: %d\n", res.err);
 	return res.err;
 }
 
drivers/thunderbolt/ctl.h +65 -0

77 lines not on this page · raw message

drivers/thunderbolt/tb.h +1 -1
index 73db614fa913..0be989069941 100644
@@ -132,7 +132,7 @@ struct tb_cm_ops {
 /**
  * struct tb - main thunderbolt bus structure
  * @dev: Domain device
- * @lock: Big lock. Must be held when accessing cfg or any struct
+ * @lock: Big lock. Must be held when accessing any struct
  *	  tb_switch / struct tb_port.
  * @nhi: Pointer to the NHI structure
  * @ctl: Control channel for this domain
Signature · 1 line
2.11.0
#26 lore raw

PATCH v3 19/27 thunderbolt: Add new Thunderbolt PCI IDs

In reply to Mika Westerberg

Add Intel Win Ridge (Thunderbolt 2) and Alpine Ridge (Thunderbolt 3) controller PCI IDs to the list of supported devices.

This code is based on the work done by Amir Levy and Michael Jamet.

---
 drivers/thunderbolt/nhi.c    | 11 +++++++++++
 drivers/thunderbolt/nhi.h    | 17 +++++++++++++++++
 drivers/thunderbolt/switch.c | 19 ++++++++++++++-----
 3 files changed, 42 insertions(+), 5 deletions(-)
drivers/thunderbolt/nhi.c +11 -0
index c1113a3c4128..fa4c2745dba2 100644
@@ -787,6 +787,17 @@ static struct pci_device_id nhi_ids[] = {
 		.device = PCI_DEVICE_ID_INTEL_FALCON_RIDGE_4C_NHI,
 		.subvendor = PCI_ANY_ID, .subdevice = PCI_ANY_ID,
 	},
+
+	/* Thunderbolt 3 */
+	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_2C_NHI) },
+	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_4C_NHI) },
+	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_USBONLY_NHI) },
+	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_LP_NHI) },
+	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_LP_USBONLY_NHI) },
+	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_2C_NHI) },
+	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_4C_NHI) },
+	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_USBONLY_NHI) },
+
 	{ 0,}
 };
 
drivers/thunderbolt/nhi.h +17 -0
index 630f44140530..8bd9b4e5a0b1 100644
@@ -143,4 +143,21 @@ static inline int ring_tx(struct tb_ring *ring, struct ring_frame *frame)
 	return __ring_enqueue(ring, frame);
 }
 
+/*
+ * PCI IDs used in this driver from Win Ridge forward. There is no
+ * need for the PCI quirk anymore as we will use ICM also on Apple
+ * hardware.
+ */
+#define PCI_DEVICE_ID_INTEL_WIN_RIDGE_2C_NHI            0x157d
+#define PCI_DEVICE_ID_INTEL_WIN_RIDGE_2C_BRIDGE         0x157e
+#define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_LP_NHI		0x15bf
+#define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_LP_BRIDGE	0x15c0
+#define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_4C_NHI	0x15d2
+#define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_4C_BRIDGE	0x15d3
+#define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_2C_NHI	0x15d9
+#define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_2C_BRIDGE	0x15da
+#define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_LP_USBONLY_NHI	0x15dc
+#define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_USBONLY_NHI	0x15dd
+#define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_USBONLY_NHI	0x15de
+
 #endif
drivers/thunderbolt/switch.c +14 -5
index 0a592ccb7923..8006577590ed 100644
@@ -491,13 +491,22 @@ int tb_switch_configure(struct tb_switch *sw)
 		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)
+	switch (sw->config.device_id) {
+	case PCI_DEVICE_ID_INTEL_LIGHT_RIDGE:
+	case PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_4C:
+	case PCI_DEVICE_ID_INTEL_PORT_RIDGE:
+	case PCI_DEVICE_ID_INTEL_FALCON_RIDGE_2C_BRIDGE:
+	case PCI_DEVICE_ID_INTEL_FALCON_RIDGE_4C_BRIDGE:
+	case PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_2C_BRIDGE:
+	case PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_4C_BRIDGE:
+	case PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_2C_BRIDGE:
+	case PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_4C_BRIDGE:
+		break;
+
+	default:
 		tb_sw_warn(sw, "unsupported switch device id %#x\n",
 			   sw->config.device_id);
+	}
 
 	sw->config.enabled = 1;
 
Signature · 1 line
2.11.0
#27 lore raw

Re: [PATCH v3 19/27] thunderbolt: Add new Thunderbolt PCI IDs

drivers/thunderbolt/nhi.h static inline int ring_tx(struct tb_ring *ring, struct ring_frame *frame) · line 150

Quoted 11 lines · Mika Westerberg
drivers/thunderbolt/nhi.h +5 -0
@@ -143,4 +143,21 @@ static inline int ring_tx(struct tb_ring *ring, struct ring_frame *frame)
 	return __ring_enqueue(ring, frame);
 }
 
+/*
+ * PCI IDs used in this driver from Win Ridge forward. There is no
+ * need for the PCI quirk anymore as we will use ICM also on Apple
+ * hardware.
+ */

I wrote a patch a few months ago which replaces the PCI quirk with device links:

https://github.com/l1k/linux/commit/8e2e7eaa1163

I was going to upstream this sometime this year, it probably would have been better if I had done that already but I wasn't expecting your series.

In any case, all Thunderbolt PCI device IDs can then be moved to a header in drivers/thunderbolt/, except for a few TB1 devices which are referenced by quirk_thunderbolt_hotplug_msi() and quirk_apple_poweroff_thunderbolt().

As to using ICM on Apple hardware, I've heard from people with Alpine Ridge MacBook Pros that the native (i.e. non-ICM) driver at least probes fine. I've yet to hear from folks who have actually tested it with any attached TB3 devices, but my expectation would be that it should work fine in native mode since the protocol seems to be the same.

I think I saw somewhere that you reset the root switch when reconfiguring the chip to run in ICM mode. That's a problem because on Apple hardware, there's a (native, non-ICM) EFI NHI driver which sets up tunnels to any devices which are already attached on boot. This can be used to boot from Thunderbolt-attached harddisks. I imagine that resetting the chip to switch to ICM-mode will cause those already established tunnels to go away, essentially breaking booting from TB-attached disks. That's a regression which we should try to avoid.

Ideally, the user should be able to choose whether to use ICM-mode or native mode. This could be implemented via a module parameter. I imagine it might even be possible to use native mode on non-Macs and this should be allowed as well.

Thanks,

Lukas

#28 lore raw

Re: [PATCH v3 19/27] thunderbolt: Add new Thunderbolt PCI IDs

Quoted 18 lines · Lukas Wunner

drivers/thunderbolt/nhi.h static inline int ring_tx(struct tb_ring *ring, struct ring_frame *frame) · line 150

Quoted 11 lines · Mika Westerberg
drivers/thunderbolt/nhi.h +5 -0
@@ -143,4 +143,21 @@ static inline int ring_tx(struct tb_ring *ring, struct ring_frame *frame)
 	return __ring_enqueue(ring, frame);
 }
 
+/*
+ * PCI IDs used in this driver from Win Ridge forward. There is no
+ * need for the PCI quirk anymore as we will use ICM also on Apple
+ * hardware.
+ */

I wrote a patch a few months ago which replaces the PCI quirk with device links:

https://github.com/l1k/linux/commit/8e2e7eaa1163

Quoted 8 lines

I was going to upstream this sometime this year, it probably would have been better if I had done that already but I wasn't expecting your series.

In any case, all Thunderbolt PCI device IDs can then be moved to a header in drivers/thunderbolt/, except for a few TB1 devices which are referenced by quirk_thunderbolt_hotplug_msi() and quirk_apple_poweroff_thunderbolt().

OK, but that should be a separate patch, right? On top of your device links patch.

Quoted 6 lines

As to using ICM on Apple hardware, I've heard from people with Alpine Ridge MacBook Pros that the native (i.e. non-ICM) driver at least probes fine. I've yet to hear from folks who have actually tested it with any attached TB3 devices, but my expectation would be that it should work fine in native mode since the protocol seems to be the same.

I have one Mac with Alpine Ridge (well 4 Thunderbolt ports, 2 Alpine Ridges) and it indeed works in native mode but it can tunnel only one device, no display port.

However, starting ICM on them allows you to connect up to 6 devices including display port. It also allows cross-domain connections where we can implement things like Amir's networking driver.

That's the reason we did it this way - to get Thunderbolt working the same way in Linux than it works on Macs running OS X :)

Quoted 8 lines

I think I saw somewhere that you reset the root switch when reconfiguring the chip to run in ICM mode. That's a problem because on Apple hardware, there's a (native, non-ICM) EFI NHI driver which sets up tunnels to any devices which are already attached on boot. This can be used to boot from Thunderbolt-attached harddisks. I imagine that resetting the chip to switch to ICM-mode will cause those already established tunnels to go away, essentially breaking booting from TB-attached disks. That's a regression which we should try to avoid.

Yes, we reset the links and ICM re-establishes them the but during that time the PCIe tunnel is down. However, it should not affect booting as that is done in the EFI firmware as far as I can tell. If you have rootfs on a TBT harddisk, it should not be problem either if your initrd includes drivers for Thunderbolt and the disks you are using.

Unless I'm missing something.

Quoted 2 lines

Ideally, the user should be able to choose whether to use ICM-mode or native mode. This could be implemented via a module parameter.

I have nothing against that in principle but at least we should default to the most functioning connection manager then, and I'm not sure it is worth adding right now until we get some feedback from people actually running ICM on Macs.

Quoted 2 lines

I imagine it might even be possible to use native mode on non-Macs and this should be allowed as well.

While I guess it is doable, it is a path that is not tested at all so you will get more problems than you are trying to solve, IMHO.

#29 lore raw

Re: [PATCH v3 19/27] thunderbolt: Add new Thunderbolt PCI IDs

Quoted 23 lines · Mika Westerberg
Quoted 22 lines · Lukas Wunner

drivers/thunderbolt/nhi.h static inline int ring_tx(struct tb_ring *ring, struct ring_frame *frame) · line 150

Quoted 11 lines · Mika Westerberg
drivers/thunderbolt/nhi.h +5 -0
@@ -143,4 +143,21 @@ static inline int ring_tx(struct tb_ring *ring, struct ring_frame *frame)
 	return __ring_enqueue(ring, frame);
 }
 
+/*
+ * PCI IDs used in this driver from Win Ridge forward. There is no
+ * need for the PCI quirk anymore as we will use ICM also on Apple
+ * hardware.
+ */

I wrote a patch a few months ago which replaces the PCI quirk with device links:

https://github.com/l1k/linux/commit/8e2e7eaa1163

I was going to upstream this sometime this year, it probably would have been better if I had done that already but I wasn't expecting your series.

Quoted 7 lines
Quoted 4 lines

In any case, all Thunderbolt PCI device IDs can then be moved to a header in drivers/thunderbolt/, except for a few TB1 devices which are referenced by quirk_thunderbolt_hotplug_msi() and quirk_apple_poweroff_thunderbolt().

OK, but that should be a separate patch, right? On top of your device links patch.

Yes. If you like the device links patch, feel free to include it in your series or modify it as you see fit.

I forgot to mention, on Alpine Ridge there's an additional downstream bridge with an XHCI controller below it. A device link will also be established from that downstream bridge to the NHI. I'm not sure if that is actually necessary, perhaps it's even undesirable.

Quoted 10 lines
Quoted 6 lines

As to using ICM on Apple hardware, I've heard from people with Alpine Ridge MacBook Pros that the native (i.e. non-ICM) driver at least probes fine. I've yet to hear from folks who have actually tested it with any attached TB3 devices, but my expectation would be that it should work fine in native mode since the protocol seems to be the same.

I have one Mac with Alpine Ridge (well 4 Thunderbolt ports, 2 Alpine Ridges) and it indeed works in native mode but it can tunnel only one device, no display port.

Yes. Those are limitations of the native driver. It doesn't support chaining and DP tunnels yet. If chained devices and DP devices are present on boot, then the EFI NHI driver (on Macs) will establish the tunnels and thunderbolt.ko will inherit them. Once the devices are unplugged and replugged, the tunnels are gone and cannot be re-established until the machine is rebooted.

Quoted 6 lines

However, starting ICM on them allows you to connect up to 6 devices including display port. It also allows cross-domain connections where we can implement things like Amir's networking driver.

That's the reason we did it this way - to get Thunderbolt working the same way in Linux than it works on Macs running OS X :)

Yes, it's useful to have ICM-based Thunderbolt on Macs and personally I'm totally fine with making it the default for Macs which support it. (I can't speak for Andreas, obviously.)

However it would be great to give people the *choice* between ICM versus native mode, for at least two reasons:

(1) Native mode uses free software. (I assume the ICM firmware remains closed source.)

(2) Native mode allows more versatility, e.g. how PCI tunnels are set up to chained devices: PCI fanout or PCI direct routing, see:
https://developer.apple.com/library/content/documentation/HardwareDrivers/Conceptual/ThunderboltDevGuide/Basics/Basics.html

Apple supports traffic prioritization to enable audio over Thunderbolt with higher accuracy / minimal skew, I assume their choice to use native mode was largely motivated by being able to support specialized applications like that which are difficult or perhaps impossible to implement in firmware:
http://pdfpiw.uspto.gov/.piw?Docid=09015384

By the way, you wrote in one of the commit messages that ICM is "a firmware running on the host controller". Is it really running on the *controller* (i.e. Thunderbolt chip)? My understanding was that ICM is part of the BIOS and executed on the host CPU in System Management Mode.

Thanks,

Lukas

#30 lore raw

Re: [PATCH v3 19/27] thunderbolt: Add new Thunderbolt PCI IDs

Quoted 29 lines · Lukas Wunner
Quoted 28 lines · Mika Westerberg
Quoted 26 lines · Lukas Wunner

drivers/thunderbolt/nhi.h static inline int ring_tx(struct tb_ring *ring, struct ring_frame *frame) · line 150

Quoted 11 lines · Mika Westerberg
drivers/thunderbolt/nhi.h +5 -0
@@ -143,4 +143,21 @@ static inline int ring_tx(struct tb_ring *ring, struct ring_frame *frame)
 	return __ring_enqueue(ring, frame);
 }
 
+/*
+ * PCI IDs used in this driver from Win Ridge forward. There is no
+ * need for the PCI quirk anymore as we will use ICM also on Apple
+ * hardware.
+ */

I wrote a patch a few months ago which replaces the PCI quirk with device links:

https://github.com/l1k/linux/commit/8e2e7eaa1163

I was going to upstream this sometime this year, it probably would have been better if I had done that already but I wasn't expecting your series.

In any case, all Thunderbolt PCI device IDs can then be moved to a header in drivers/thunderbolt/, except for a few TB1 devices which are referenced by quirk_thunderbolt_hotplug_msi() and quirk_apple_poweroff_thunderbolt().

Quoted 5 lines
Quoted 2 lines

OK, but that should be a separate patch, right? On top of your device links patch.

Yes. If you like the device links patch, feel free to include it in your series or modify it as you see fit.

It is kind of separate thing - here we add the the missing functionality so that PC users can finally get their TBT devices connected.

We can do those things later when the basic functionality is in place IMHO.

Quoted 4 lines

I forgot to mention, on Alpine Ridge there's an additional downstream bridge with an XHCI controller below it. A device link will also be established from that downstream bridge to the NHI. I'm not sure if that is actually necessary, perhaps it's even undesirable.

OK

Quoted 39 lines
Quoted 10 lines
Quoted 6 lines

As to using ICM on Apple hardware, I've heard from people with Alpine Ridge MacBook Pros that the native (i.e. non-ICM) driver at least probes fine. I've yet to hear from folks who have actually tested it with any attached TB3 devices, but my expectation would be that it should work fine in native mode since the protocol seems to be the same.

I have one Mac with Alpine Ridge (well 4 Thunderbolt ports, 2 Alpine Ridges) and it indeed works in native mode but it can tunnel only one device, no display port.

Yes. Those are limitations of the native driver. It doesn't support chaining and DP tunnels yet. If chained devices and DP devices are present on boot, then the EFI NHI driver (on Macs) will establish the tunnels and thunderbolt.ko will inherit them. Once the devices are unplugged and replugged, the tunnels are gone and cannot be re-established until the machine is rebooted.

Quoted 6 lines

However, starting ICM on them allows you to connect up to 6 devices including display port. It also allows cross-domain connections where we can implement things like Amir's networking driver.

That's the reason we did it this way - to get Thunderbolt working the same way in Linux than it works on Macs running OS X :)

Yes, it's useful to have ICM-based Thunderbolt on Macs and personally I'm totally fine with making it the default for Macs which support it. (I can't speak for Andreas, obviously.)

However it would be great to give people the *choice* between ICM versus native mode, for at least two reasons:

(1) Native mode uses free software. (I assume the ICM firmware remains closed source.)

(2) Native mode allows more versatility, e.g. how PCI tunnels are set up to chained devices: PCI fanout or PCI direct routing, see:
https://developer.apple.com/library/content/documentation/HardwareDrivers/Conceptual/ThunderboltDevGuide/Basics/Basics.html

Quoted 6 lines

Apple supports traffic prioritization to enable audio over Thunderbolt with higher accuracy / minimal skew, I assume their choice to use native mode was largely motivated by being able to support specialized applications like that which are difficult or perhaps impossible to implement in firmware:
http://pdfpiw.uspto.gov/.piw?Docid=09015384

Right, but do we need to do it now before we have got any feedback from users using Macs with Alpine Ridge and ICM? We can always add the module parameter later if really needed.

Quoted 5 lines

By the way, you wrote in one of the commit messages that ICM is "a firmware running on the host controller". Is it really running on the *controller* (i.e. Thunderbolt chip)? My understanding was that ICM is part of the BIOS and executed on the host CPU in System Management Mode.

Yes it is. The TBT3 devices (well even TBT2 devices) supporting secure connect for example implement it in ICM firmware.

With these patches you can even upgrade that ICM firmware on devices and that also works if you are using the native connection manager.

#31 lore raw

Re: [PATCH v3 19/27] thunderbolt: Add new Thunderbolt PCI IDs

Quoted 21 lines · Mika Westerberg
Quoted 14 lines

Yes, it's useful to have ICM-based Thunderbolt on Macs and personally I'm totally fine with making it the default for Macs which support it. (I can't speak for Andreas, obviously.)

However it would be great to give people the *choice* between ICM versus native mode, for at least two reasons:

(1) Native mode uses free software. (I assume the ICM firmware remains closed source.)

(2) Native mode allows more versatility, e.g. how PCI tunnels are set up to chained devices: PCI fanout or PCI direct routing, see:
https://developer.apple.com/library/content/documentation/HardwareDrivers/Conceptual/ThunderboltDevGuide/Basics/Basics.html

Quoted 6 lines

Apple supports traffic prioritization to enable audio over Thunderbolt with higher accuracy / minimal skew, I assume their choice to use native mode was largely motivated by being able to support specialized applications like that which are difficult or perhaps impossible to implement in firmware:
http://pdfpiw.uspto.gov/.piw?Docid=09015384

Quoted 3 lines

Right, but do we need to do it now before we have got any feedback from users using Macs with Alpine Ridge and ICM? We can always add the module parameter later if really needed.

It turns out that this is not too big change after all so I think it can be included in the ICM patch if you want.

Basically we would have module parameter:

cm=auto|native|icm

where auto is the default.

Does that work?

#32 lore raw

Re: [PATCH v3 19/27] thunderbolt: Add new Thunderbolt PCI IDs

Quoted 29 lines · Mika Westerberg
Quoted 21 lines · Mika Westerberg
Quoted 14 lines

Yes, it's useful to have ICM-based Thunderbolt on Macs and personally I'm totally fine with making it the default for Macs which support it. (I can't speak for Andreas, obviously.)

However it would be great to give people the *choice* between ICM versus native mode, for at least two reasons:

(1) Native mode uses free software. (I assume the ICM firmware remains closed source.)

(2) Native mode allows more versatility, e.g. how PCI tunnels are set up to chained devices: PCI fanout or PCI direct routing, see:
https://developer.apple.com/library/content/documentation/HardwareDrivers/Conceptual/ThunderboltDevGuide/Basics/Basics.html

Quoted 6 lines

Apple supports traffic prioritization to enable audio over Thunderbolt with higher accuracy / minimal skew, I assume their choice to use native mode was largely motivated by being able to support specialized applications like that which are difficult or perhaps impossible to implement in firmware:
http://pdfpiw.uspto.gov/.piw?Docid=09015384

Quoted 3 lines

Right, but do we need to do it now before we have got any feedback from users using Macs with Alpine Ridge and ICM? We can always add the module parameter later if really needed.

It turns out that this is not too big change after all so I think it can be included in the ICM patch if you want.

Quoted 7 lines

Basically we would have module parameter:

cm=auto|native|icm

where auto is the default.

Does that work?

Yes, great, thanks!

And in "auto" mode, it's ICM on non-Macs and TBT3-Macs, and native for TBT1/TBT2-Macs, right?

Some Falcon Ridge Macs support ICM as well (e.g. the MacBookPro11,4 and MacBookPro11,5 introduced 2015), but maybe not all of them. This will have to be tested. If all of them support it, we could later default to ICM for those as well, or use DMI data to constrain it to specific models.

Best regards,

Lukas

#33 lore raw

Re: [PATCH v3 19/27] thunderbolt: Add new Thunderbolt PCI IDs

Quoted 34 lines · Lukas Wunner
Quoted 33 lines · Mika Westerberg
Quoted 21 lines · Mika Westerberg
Quoted 14 lines

Yes, it's useful to have ICM-based Thunderbolt on Macs and personally I'm totally fine with making it the default for Macs which support it. (I can't speak for Andreas, obviously.)

However it would be great to give people the *choice* between ICM versus native mode, for at least two reasons:

(1) Native mode uses free software. (I assume the ICM firmware remains closed source.)

(2) Native mode allows more versatility, e.g. how PCI tunnels are set up to chained devices: PCI fanout or PCI direct routing, see:
https://developer.apple.com/library/content/documentation/HardwareDrivers/Conceptual/ThunderboltDevGuide/Basics/Basics.html

Quoted 6 lines

Apple supports traffic prioritization to enable audio over Thunderbolt with higher accuracy / minimal skew, I assume their choice to use native mode was largely motivated by being able to support specialized applications like that which are difficult or perhaps impossible to implement in firmware:
http://pdfpiw.uspto.gov/.piw?Docid=09015384

Quoted 3 lines

Right, but do we need to do it now before we have got any feedback from users using Macs with Alpine Ridge and ICM? We can always add the module parameter later if really needed.

It turns out that this is not too big change after all so I think it can be included in the ICM patch if you want.

Basically we would have module parameter:

cm=auto|native|icm

Quoted 8 lines
Quoted 3 lines

where auto is the default.

Does that work?

Yes, great, thanks!

And in "auto" mode, it's ICM on non-Macs and TBT3-Macs, and native for TBT1/TBT2-Macs, right?

Yes, that's correct.

Quoted 5 lines

Some Falcon Ridge Macs support ICM as well (e.g. the MacBookPro11,4 and MacBookPro11,5 introduced 2015), but maybe not all of them. This will have to be tested. If all of them support it, we could later default to ICM for those as well, or use DMI data to constrain it to specific models.

Unfortunately starting ICM on Falcon Ridge Macs does not work (I tried that already) so there we need to stick with the native implementation.

#34 lore raw

Re: [PATCH v3 19/27] thunderbolt: Add new Thunderbolt PCI IDs

In reply to Mika Westerberg
Quoted 7 lines · Mika Westerberg
Quoted 1 line · Mika Westerberg
Quoted 1 line

It turns out that this is not too big change after all so I think it can
be included in the ICM patch if you want.

Quoted 7 lines

Basically we would have module parameter:

cm=auto|native|icm

where auto is the default.

Does that work?

I remember Greg is opposite to being fan of module parameters in new code, so, please, if you are still going to do that, do it a separate patch which will be last in the series and can be discussed separately w.o. preventing everything else being applied.

Signature · 2 lines
Andy Shevchenko <[email protected]>
Intel Finland Oy
#35 lore raw

Re: [PATCH v3 19/27] thunderbolt: Add new Thunderbolt PCI IDs

Quoted 14 lines · Andy Shevchenko
Quoted 13 lines · Mika Westerberg
Quoted 1 line · Mika Westerberg
Quoted 1 line

It turns out that this is not too big change after all so I think it can
be included in the ICM patch if you want.

Basically we would have module parameter:

cm=auto|native|icm

where auto is the default.

Quoted 7 lines
Quoted 1 line

Does that work?

I remember Greg is opposite to being fan of module parameters in new code, so, please, if you are still going to do that, do it a separate patch which will be last in the series and can be discussed separately w.o. preventing everything else being applied.

Good point, thanks!

I'll add it as a separate patch then.

#36 lore raw

Re: [PATCH v3 19/27] thunderbolt: Add new Thunderbolt PCI IDs

In reply to Andy Shevchenko
Quoted 14 lines · Andy Shevchenko
Quoted 13 lines · Mika Westerberg
Quoted 1 line · Mika Westerberg
Quoted 1 line

It turns out that this is not too big change after all so I think it can
be included in the ICM patch if you want.

Basically we would have module parameter:

cm=auto|native|icm

where auto is the default.

Quoted 7 lines
Quoted 1 line

Does that work?

I remember Greg is opposite to being fan of module parameters in new code, so, please, if you are still going to do that, do it a separate patch which will be last in the series and can be discussed separately w.o. preventing everything else being applied.

Yes, you should never need module parameters in "modern" code. As no one will ever set them, or know what to do here. Make it "automatic" please.

thanks,

greg k-h

#37 lore raw

Re: [PATCH v3 19/27] thunderbolt: Add new Thunderbolt PCI IDs

Quoted 18 lines · Greg Kroah-Hartman
Quoted 17 lines · Andy Shevchenko
Quoted 14 lines · Mika Westerberg
Quoted 1 line · Mika Westerberg
Quoted 1 line

It turns out that this is not too big change after all so I think it can
be included in the ICM patch if you want.

Basically we would have module parameter:

cm=auto|native|icm

where auto is the default.

Does that work?

Quoted 8 lines
Quoted 4 lines

I remember Greg is opposite to being fan of module parameters in new code, so, please, if you are still going to do that, do it a separate patch which will be last in the series and can be discussed separately w.o. preventing everything else being applied.

Yes, you should never need module parameters in "modern" code. As no one will ever set them, or know what to do here. Make it "automatic" please.

Well in current series it is already automatic :-)

What Lukas wants here is a way to force a certain connection manager implementation to be used instead.

#38 lore raw

RE: [PATCH v3 19/27] thunderbolt: Add new Thunderbolt PCI IDs

Quoted 36 lines

-----Original Message-----
From: Mika Westerberg [mailto:[email protected]]
Sent: Monday, June 5, 2017 10:41 AM
To: Greg Kroah-Hartman <[email protected]>
Cc: Andy Shevchenko <[email protected]>; Lukas Wunner <[email protected]>; Andreas Noever <[email protected]>; Michael Jamet <[email protected]>; Yehezkel Bernat <[email protected]>; Amir Levy <[email protected]>; Andy Lutomirski <[email protected]>; Limonciello, Mario <[email protected]>; Dominguez, Jared <[email protected]>; [email protected]
Subject: Re: [PATCH v3 19/27] thunderbolt: Add new Thunderbolt PCI IDs

Quoted 23 lines · Greg Kroah-Hartman
Quoted 21 lines · Andy Shevchenko
Quoted 7 lines · Mika Westerberg
Quoted 1 line · Mika Westerberg
Quoted 1 line

It turns out that this is not too big change after all so I think it can
be included in the ICM patch if you want.

Quoted 7 lines

Basically we would have module parameter:

cm=auto|native|icm

where auto is the default.

Does that work?

I remember Greg is opposite to being fan of module parameters in new code, so, please, if you are still going to do that, do it a separate patch which will be last in the series and can be discussed separately w.o. preventing everything else being applied.

Quoted 8 lines
Quoted 3 lines

Yes, you should never need module parameters in "modern" code. As no one will ever set them, or know what to do here. Make it "automatic" please.

Well in current series it is already automatic :-)

What Lukas wants here is a way to force a certain connection manager implementation to be used instead.

And presumably that's mostly for debugging purposes and will lead to additional quirks to get "automatic" to do the right thing on those machines that's it's found to be needed or more beneficial.

#39 lore raw

Re: [PATCH v3 19/27] thunderbolt: Add new Thunderbolt PCI IDs

Quoted 3 lines · On Mon, Jun 05, 2017 at 03:50:24PM +0000, [email protected] wrote:

And presumably that's mostly for debugging purposes and will lead to additional quirks to get "automatic" to do the right thing on those machines that's it's found to be needed or more beneficial.

I would say that this is only for kernel hackers, like Lukas, who know what they are doing. But then again people needing that could just comment out one line in nhi.c to pick the implementation they want without any module parameter ;-)

I think I'm not going to add that patch to this series after all - we can always do it later if really really needed.

#40 lore raw

PATCH v3 20/27 thunderbolt: Add support for NHI mailbox

In reply to Mika Westerberg

The host controller includes two sets of registers that are used to communicate with the firmware. Add functions that can be used to access these registers.

This code is based on the work done by Amir Levy and Michael Jamet.

---
 drivers/thunderbolt/nhi.c      | 58 ++++++++++++++++++++++++++++++++++++++++++
 drivers/thunderbolt/nhi.h      | 16 ++++++++++++
 drivers/thunderbolt/nhi_regs.h | 11 ++++++++
 3 files changed, 85 insertions(+)
drivers/thunderbolt/nhi.c +58 -0
index fa4c2745dba2..c358c074f925 100644
@@ -14,6 +14,7 @@
 #include <linux/interrupt.h>
 #include <linux/module.h>
 #include <linux/dmi.h>
+#include <linux/delay.h>
 
 #include "nhi.h"
 #include "nhi_regs.h"
@@ -28,6 +29,8 @@
 #define MSIX_MIN_VECS		6
 #define MSIX_MAX_VECS		16
 
+#define NHI_MAILBOX_TIMEOUT	500 /* ms */
+
 static int ring_interrupt_index(struct tb_ring *ring)
 {
 	int bit = ring->hop;
@@ -525,6 +528,61 @@ void ring_free(struct tb_ring *ring)
 	kfree(ring);
 }
 
+/**
+ * nhi_mailbox_cmd() - Send a command through NHI mailbox
+ * @nhi: Pointer to the NHI structure
+ * @cmd: Command to send
+ * @data: Data to be send with the command
+ *
+ * Sends mailbox command to the firmware running on NHI. Returns %0 in
+ * case of success and negative errno in case of failure.
+ */
+int nhi_mailbox_cmd(struct tb_nhi *nhi, enum nhi_mailbox_cmd cmd, u32 data)
+{
+	ktime_t timeout;
+	u32 val;
+
+	iowrite32(data, nhi->iobase + REG_INMAIL_DATA);
+
+	val = ioread32(nhi->iobase + REG_INMAIL_CMD);
+	val &= ~(REG_INMAIL_CMD_MASK | REG_INMAIL_ERROR);
+	val |= REG_INMAIL_OP_REQUEST | cmd;
+	iowrite32(val, nhi->iobase + REG_INMAIL_CMD);
+
+	timeout = ktime_add_ms(ktime_get(), NHI_MAILBOX_TIMEOUT);
+	do {
+		val = ioread32(nhi->iobase + REG_INMAIL_CMD);
+		if (!(val & REG_INMAIL_OP_REQUEST))
+			break;
+		usleep_range(10, 20);
+	} while (ktime_before(ktime_get(), timeout));
+
+	if (val & REG_INMAIL_OP_REQUEST)
+		return -ETIMEDOUT;
+	if (val & REG_INMAIL_ERROR)
+		return -EIO;
+
+	return 0;
+}
+
+/**
+ * nhi_mailbox_mode() - Return current firmware operation mode
+ * @nhi: Pointer to the NHI structure
+ *
+ * The function reads current firmware operation mode using NHI mailbox
+ * registers and returns it to the caller.
+ */
+enum nhi_fw_mode nhi_mailbox_mode(struct tb_nhi *nhi)
+{
+	u32 val;
+
+	val = ioread32(nhi->iobase + REG_OUTMAIL_CMD);
+	val &= REG_OUTMAIL_CMD_OPMODE_MASK;
+	val >>= REG_OUTMAIL_CMD_OPMODE_SHIFT;
+
+	return (enum nhi_fw_mode)val;
+}
+
 static void nhi_interrupt_work(struct work_struct *work)
 {
 	struct tb_nhi *nhi = container_of(work, typeof(*nhi), interrupt_work);
drivers/thunderbolt/nhi.h +16 -0
index 8bd9b4e5a0b1..446ff6dac91d 100644
@@ -143,6 +143,22 @@ static inline int ring_tx(struct tb_ring *ring, struct ring_frame *frame)
 	return __ring_enqueue(ring, frame);
 }
 
+enum nhi_fw_mode {
+	NHI_FW_SAFE_MODE,
+	NHI_FW_AUTH_MODE,
+	NHI_FW_EP_MODE,
+	NHI_FW_CM_MODE,
+};
+
+enum nhi_mailbox_cmd {
+	NHI_MAILBOX_SAVE_DEVS = 0x05,
+	NHI_MAILBOX_DRV_UNLOADS = 0x07,
+	NHI_MAILBOX_ALLOW_ALL_DEVS = 0x23,
+};
+
+int nhi_mailbox_cmd(struct tb_nhi *nhi, enum nhi_mailbox_cmd cmd, u32 data);
+enum nhi_fw_mode nhi_mailbox_mode(struct tb_nhi *nhi);
+
 /*
  * PCI IDs used in this driver from Win Ridge forward. There is no
  * need for the PCI quirk anymore as we will use ICM also on Apple
drivers/thunderbolt/nhi_regs.h +11 -0
index 48b98d3c7e6a..322fe1fa3a3c 100644
@@ -107,4 +107,15 @@ struct ring_desc {
 #define REG_DMA_MISC			0x39864
 #define REG_DMA_MISC_INT_AUTO_CLEAR     BIT(2)
 
+#define REG_INMAIL_DATA			0x39900
+
+#define REG_INMAIL_CMD			0x39904
+#define REG_INMAIL_CMD_MASK		GENMASK(7, 0)
+#define REG_INMAIL_ERROR		BIT(30)
+#define REG_INMAIL_OP_REQUEST		BIT(31)
+
+#define REG_OUTMAIL_CMD			0x3990c
+#define REG_OUTMAIL_CMD_OPMODE_SHIFT	8
+#define REG_OUTMAIL_CMD_OPMODE_MASK	GENMASK(11, 8)
+
 #endif
Signature · 1 line
2.11.0
#41 lore raw

PATCH v3 21/27 thunderbolt: Store Thunderbolt generation in the switch structure

In reply to Mika Westerberg

In some cases it is useful to know what is the Thunderbolt generation the switch supports. This introduces a new field to struct switch that stores the generation of the switch based on the device ID. Unknown switches (there should be none) are assumed to be first generation to be on the safe side.

---
 drivers/thunderbolt/switch.c | 55 ++++++++++++++++++++++++++++++--------------
 drivers/thunderbolt/tb.h     |  2 ++
 2 files changed, 40 insertions(+), 17 deletions(-)
drivers/thunderbolt/switch.c +38 -17
index 8006577590ed..56b99ad2099d 100644
@@ -390,6 +390,42 @@ struct device_type tb_switch_type = {
 	.release = tb_switch_release,
 };
 
+static int tb_switch_get_generation(struct tb_switch *sw)
+{
+	switch (sw->config.device_id) {
+	case PCI_DEVICE_ID_INTEL_LIGHT_RIDGE:
+	case PCI_DEVICE_ID_INTEL_EAGLE_RIDGE:
+	case PCI_DEVICE_ID_INTEL_LIGHT_PEAK:
+	case PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_2C:
+	case PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_4C:
+	case PCI_DEVICE_ID_INTEL_PORT_RIDGE:
+	case PCI_DEVICE_ID_INTEL_REDWOOD_RIDGE_2C_BRIDGE:
+	case PCI_DEVICE_ID_INTEL_REDWOOD_RIDGE_4C_BRIDGE:
+		return 1;
+
+	case PCI_DEVICE_ID_INTEL_WIN_RIDGE_2C_BRIDGE:
+	case PCI_DEVICE_ID_INTEL_FALCON_RIDGE_2C_BRIDGE:
+	case PCI_DEVICE_ID_INTEL_FALCON_RIDGE_4C_BRIDGE:
+		return 2;
+
+	case PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_LP_BRIDGE:
+	case PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_2C_BRIDGE:
+	case PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_4C_BRIDGE:
+	case PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_2C_BRIDGE:
+	case PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_4C_BRIDGE:
+		return 3;
+
+	default:
+		/*
+		 * For unknown switches assume generation to be 1 to be
+		 * on the safe side.
+		 */
+		tb_sw_warn(sw, "unsupported switch device id %#x\n",
+			   sw->config.device_id);
+		return 1;
+	}
+}
+
 /**
  * tb_switch_alloc() - allocate a switch
  * @tb: Pointer to the owning domain
@@ -443,6 +479,8 @@ struct tb_switch *tb_switch_alloc(struct tb *tb, struct device *parent,
 		sw->ports[i].port = i;
 	}
 
+	sw->generation = tb_switch_get_generation(sw);
+
 	cap = tb_switch_find_vsec_cap(sw, TB_VSEC_CAP_PLUG_EVENTS);
 	if (cap < 0) {
 		tb_sw_warn(sw, "cannot find TB_VSEC_CAP_PLUG_EVENTS aborting\n");
@@ -491,23 +529,6 @@ int tb_switch_configure(struct tb_switch *sw)
 		tb_sw_warn(sw, "unknown switch vendor id %#x\n",
 			   sw->config.vendor_id);
 
-	switch (sw->config.device_id) {
-	case PCI_DEVICE_ID_INTEL_LIGHT_RIDGE:
-	case PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_4C:
-	case PCI_DEVICE_ID_INTEL_PORT_RIDGE:
-	case PCI_DEVICE_ID_INTEL_FALCON_RIDGE_2C_BRIDGE:
-	case PCI_DEVICE_ID_INTEL_FALCON_RIDGE_4C_BRIDGE:
-	case PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_2C_BRIDGE:
-	case PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_4C_BRIDGE:
-	case PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_2C_BRIDGE:
-	case PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_4C_BRIDGE:
-		break;
-
-	default:
-		tb_sw_warn(sw, "unsupported switch device id %#x\n",
-			   sw->config.device_id);
-	}
-
 	sw->config.enabled = 1;
 
 	/* upload configuration */
drivers/thunderbolt/tb.h +2 -0
index 0be989069941..b3cda7605619 100644
@@ -25,6 +25,7 @@
  * @device: Device ID of the switch
  * @vendor_name: Name of the vendor (or %NULL if not known)
  * @device_name: Name of the device (or %NULL if not known)
+ * @generation: Switch Thunderbolt generation
  * @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)
@@ -40,6 +41,7 @@ struct tb_switch {
 	u16 device;
 	const char *vendor_name;
 	const char *device_name;
+	unsigned int generation;
 	int cap_plug_events;
 	bool is_unplugged;
 	u8 *drom;
Signature · 1 line
2.11.0
#42 lore raw

PATCH v3 22/27 thunderbolt: Add support for DMA configuration based mailbox

In reply to Mika Westerberg

The DMA (NHI) port of a switch provides access to the NVM of the host controller (and devices starting from Intel Alpine Ridge). The NVM contains also more complete DROM for the root switch including vendor and device identification strings.

This will look for the DMA port capability for each switch and if found populates sw->dma_port. We then teach tb_drom_read() to read the DROM information from NVM if available for the root switch.

The DMA port capability also supports upgrading the NVM for both host controller and devices which will be added in subsequent patches.

This code is based on the work done by Amir Levy and Michael Jamet.

---
 drivers/thunderbolt/Makefile   |   2 +-
 drivers/thunderbolt/dma_port.c | 524 +++++++++++++++++++++++++++++++++++++++++
 drivers/thunderbolt/dma_port.h |  34 +++
 drivers/thunderbolt/eeprom.c   |  51 +++-
 drivers/thunderbolt/switch.c   |  30 +++
 drivers/thunderbolt/tb.h       |   5 +
 6 files changed, 644 insertions(+), 2 deletions(-)
 create mode 100644 drivers/thunderbolt/dma_port.c
 create mode 100644 drivers/thunderbolt/dma_port.h
drivers/thunderbolt/Makefile +1 -1
index e276a9a62261..9828e862dd35 100644
@@ -1,3 +1,3 @@
 obj-${CONFIG_THUNDERBOLT} := thunderbolt.o
 thunderbolt-objs := nhi.o ctl.o tb.o switch.o cap.o path.o tunnel_pci.o eeprom.o
-thunderbolt-objs += domain.o
+thunderbolt-objs += domain.o dma_port.o
drivers/thunderbolt/dma_port.c new file +524 -0

524 lines not on this page · raw message

drivers/thunderbolt/dma_port.h new file +34 -0
new file mode 100644
index 000000000000..c4a69e0fbff7
@@ -0,0 +1,34 @@
+/*
+ * Thunderbolt DMA configuration based mailbox support
+ *
+ * Copyright (C) 2017, Intel Corporation
+ * Authors: Michael Jamet <[email protected]>
+ *          Mika Westerberg <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef DMA_PORT_H_
+#define DMA_PORT_H_
+
+#include "tb.h"
+
+struct tb_switch;
+struct tb_dma_port;
+
+#define DMA_PORT_CSS_ADDRESS		0x3fffff
+#define DMA_PORT_CSS_MAX_SIZE		SZ_128
+
+struct tb_dma_port *dma_port_alloc(struct tb_switch *sw);
+void dma_port_free(struct tb_dma_port *dma);
+int dma_port_flash_read(struct tb_dma_port *dma, unsigned int address,
+			void *buf, size_t size);
+int dma_port_flash_update_auth(struct tb_dma_port *dma);
+int dma_port_flash_update_auth_status(struct tb_dma_port *dma, u32 *status);
+int dma_port_flash_write(struct tb_dma_port *dma, unsigned int address,
+			 const void *buf, size_t size);
+int dma_port_power_cycle(struct tb_dma_port *dma);
+
+#endif
drivers/thunderbolt/eeprom.c +50 -1

69 lines not on this page · raw message

drivers/thunderbolt/switch.c +30 -0
index 56b99ad2099d..2c99fa5884a2 100644
@@ -377,6 +377,8 @@ static void tb_switch_release(struct device *dev)
 {
 	struct tb_switch *sw = tb_to_switch(dev);
 
+	dma_port_free(sw->dma_port);
+
 	kfree(sw->uuid);
 	kfree(sw->device_name);
 	kfree(sw->vendor_name);
@@ -570,6 +572,25 @@ static void tb_switch_set_uuid(struct tb_switch *sw)
 	sw->uuid = kmemdup(uuid, sizeof(uuid), GFP_KERNEL);
 }
 
+static void tb_switch_add_dma_port(struct tb_switch *sw)
+{
+	switch (sw->generation) {
+	case 3:
+		break;
+
+	case 2:
+		/* Only root switch can be upgraded */
+		if (tb_route(sw))
+			return;
+		break;
+
+	default:
+		return;
+	}
+
+	sw->dma_port = dma_port_alloc(sw);
+}
+
 /**
  * tb_switch_add() - Add a switch to the domain
  * @sw: Switch to add
@@ -586,6 +607,15 @@ int tb_switch_add(struct tb_switch *sw)
 {
 	int i, ret;
 
+	/*
+	 * Initialize DMA control port now before we read DROM. Recent
+	 * host controllers have more complete DROM on NVM that includes
+	 * vendor and model identification strings which we then expose
+	 * to the userspace. NVM can be accessed through DMA
+	 * configuration based mailbox.
+	 */
+	tb_switch_add_dma_port(sw);
+
 	/* read drom */
 	ret = tb_drom_read(sw);
 	if (ret) {
drivers/thunderbolt/tb.h +5 -0
index b3cda7605619..1facf9e4b827 100644
@@ -12,12 +12,16 @@
 
 #include "tb_regs.h"
 #include "ctl.h"
+#include "dma_port.h"
 
 /**
  * struct tb_switch - a thunderbolt switch
  * @dev: Device for the switch
  * @config: Switch configuration
  * @ports: Ports in this switch
+ * @dma_port: If the switch has port supporting DMA configuration based
+ *	      mailbox this will hold the pointer to that (%NULL
+ *	      otherwise).
  * @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)
@@ -34,6 +38,7 @@ struct tb_switch {
 	struct device dev;
 	struct tb_regs_switch_header config;
 	struct tb_port *ports;
+	struct tb_dma_port *dma_port;
 	struct tb *tb;
 	u64 uid;
 	uuid_be *uuid;
Signature · 1 line
2.11.0
#43 lore raw

PATCH v3 23/27 thunderbolt: Do not touch the hardware if the NHI is gone on resume

In reply to Mika Westerberg

On PCs the NHI host controller is only present when there is a device connected. When the last device is disconnected the host controller will dissappear shortly (within 10s). Now if that happens when we are suspended we should not try to touch the hardware anymore, so add a flag for this and check it before we re-enable rings.

---
 drivers/thunderbolt/nhi.c | 12 ++++++++++++
 drivers/thunderbolt/nhi.h |  3 +++
 2 files changed, 15 insertions(+)
drivers/thunderbolt/nhi.c +12 -0
index c358c074f925..14311535661d 100644
@@ -403,6 +403,8 @@ void ring_start(struct tb_ring *ring)
 {
 	mutex_lock(&ring->nhi->lock);
 	mutex_lock(&ring->lock);
+	if (ring->nhi->going_away)
+		goto err;
 	if (ring->running) {
 		dev_WARN(&ring->nhi->pdev->dev, "ring already started\n");
 		goto err;
@@ -449,6 +451,8 @@ void ring_stop(struct tb_ring *ring)
 	mutex_lock(&ring->lock);
 	dev_info(&ring->nhi->pdev->dev, "stopping %s %d\n",
 		 RING_TYPE(ring), ring->hop);
+	if (ring->nhi->going_away)
+		goto err;
 	if (!ring->running) {
 		dev_WARN(&ring->nhi->pdev->dev, "%s %d already stopped\n",
 			 RING_TYPE(ring), ring->hop);
@@ -653,6 +657,14 @@ static int nhi_resume_noirq(struct device *dev)
 	struct pci_dev *pdev = to_pci_dev(dev);
 	struct tb *tb = pci_get_drvdata(pdev);
 
+	/*
+	 * Check that the device is still there. It may be that the user
+	 * unplugged last device which causes the host controller to go
+	 * away on PCs.
+	 */
+	if (!pci_device_is_present(pdev))
+		tb->nhi->going_away = true;
+
 	return tb_domain_resume_noirq(tb);
 }
 
drivers/thunderbolt/nhi.h +3 -0
index 446ff6dac91d..953864ae0ab3 100644
@@ -20,6 +20,8 @@
  * @tx_rings: All Tx rings available on this host controller
  * @rx_rings: All Rx rings available on this host controller
  * @msix_ida: Used to allocate MSI-X vectors for rings
+ * @going_away: The host controller device is about to disappear so when
+ *		this flag is set, avoid touching the hardware anymore.
  * @interrupt_work: Work scheduled to handle ring interrupt when no
  *		    MSI-X is used.
  * @hop_count: Number of rings (end point hops) supported by NHI.
@@ -31,6 +33,7 @@ struct tb_nhi {
 	struct tb_ring **tx_rings;
 	struct tb_ring **rx_rings;
 	struct ida msix_ida;
+	bool going_away;
 	struct work_struct interrupt_work;
 	u32 hop_count;
 };
Signature · 1 line
2.11.0
#44 lore raw

PATCH v3 24/27 thunderbolt: Add support for Internal Connection Manager (ICM)

In reply to Mika Westerberg

Starting from Intel Falcon Ridge the internal connection manager running on the Thunderbolt host controller has been supporting 4 security levels. One reason for this is to prevent DMA attacks and only allow connecting devices the user trusts.

The internal connection manager (ICM) is the preferred way of connecting Thunderbolt devices over software only implementation typically used on Macs. The driver communicates with ICM using special Thunderbolt ring 0 (control channel) messages. In order to handle these messages we add support for the ICM messages to the control channel.

The security levels are as follows:

none - No security, all tunnels are created automatically user - User needs to approve the device before tunnels are created secure - User need to approve the device before tunnels are created.
The device is sent a challenge on future connects to be able to verify it is actually the approved device.
dponly - Only Display Port and USB tunnels can be created and those
are created automatically.

The security levels are typically configurable from the system BIOS and by default it is set to "user" on many systems.

In this patch each Thunderbolt device will have either one or two new sysfs attributes: authorized and key. The latter appears for devices that support secure connect.

In order to identify the device the user can read identication information, including UUID and name of the device from sysfs and based on that make a decision to authorize the device. The device is authorized by simply writing 1 to the "authorized" sysfs attribute. This is following the USB bus device authorization mechanism. The secure connect requires an additional challenge step (writing 2 to the "authorized" attribute) in future connects when the key has already been stored to the NVM of the device.

Non-ICM systems (before Alpine Ridge) continue to use the existing functionality and the security level is set to none. For systems with Alpine Ridge, even on Apple hardware, we will use ICM.

This code is based on the work done by Amir Levy and Michael Jamet.

---
 Documentation/ABI/testing/sysfs-bus-thunderbolt |   48 +
 drivers/thunderbolt/Kconfig                     |   12 +-
 drivers/thunderbolt/Makefile                    |    2 +-
 drivers/thunderbolt/ctl.c                       |    2 +
 drivers/thunderbolt/domain.c                    |  195 +++++
 drivers/thunderbolt/icm.c                       | 1058 +++++++++++++++++++++++
 drivers/thunderbolt/nhi.c                       |   33 +-
 drivers/thunderbolt/nhi_regs.h                  |    7 +
 drivers/thunderbolt/switch.c                    |  222 +++++
 drivers/thunderbolt/tb.c                        |    7 +
 drivers/thunderbolt/tb.h                        |   79 ++
 drivers/thunderbolt/tb_msgs.h                   |  152 ++++
 12 files changed, 1805 insertions(+), 12 deletions(-)
 create mode 100644 drivers/thunderbolt/icm.c
Documentation/ABI/testing/sysfs-bus-thunderbolt +48 -0
index 29a516f53d2c..05b7f9a6431f 100644
@@ -1,3 +1,51 @@
+What: /sys/bus/thunderbolt/devices/.../domainX/security
+Date:		Sep 2017
+KernelVersion:	4.13
+Contact:	[email protected]
+Description:	This attribute holds current Thunderbolt security level
+		set by the system BIOS. Possible values are:
+
+		none: All devices are automatically authorized
+		user: Devices are only authorized based on writing
+		      appropriate value to the authorized attribute
+		secure: Require devices that support secure connect at
+			minimum. User needs to authorize each device.
+		dponly: Automatically tunnel Display port (and USB). No
+			PCIe tunnels are created.
+
+What: /sys/bus/thunderbolt/devices/.../authorized
+Date:		Sep 2017
+KernelVersion:	4.13
+Contact:	[email protected]
+Description:	This attribute is used to authorize Thunderbolt devices
+		after they have been connected. If the device is not
+		authorized, no devices such as PCIe and Display port are
+		available to the system.
+
+		Contents of this attribute will be 0 when the device is not
+		yet authorized.
+
+		Possible values are supported:
+		1: The device will be authorized and connected
+
+		When key attribute contains 32 byte hex string the possible
+		values are:
+		1: The 32 byte hex string is added to the device NVM and
+		   the device is authorized.
+		2: Send a challenge based on the 32 byte hex string. If the
+		   challenge response from device is valid, the device is
+		   authorized. In case of failure errno will be ENOKEY if
+		   the device did not contain a key at all, and
+		   EKEYREJECTED if the challenge response did not match.
+
+What: /sys/bus/thunderbolt/devices/.../key
+Date:		Sep 2017
+KernelVersion:	4.13
+Contact:	[email protected]
+Description:	When a devices supports Thunderbolt secure connect it will
+		have this attribute. Writing 32 byte hex string changes
+		authorization to use the secure connection method instead.
+
 What:		/sys/bus/thunderbolt/devices/.../device
 Date:		Sep 2017
 KernelVersion:	4.13
drivers/thunderbolt/Kconfig +6 -6
index d35db16aa43f..a9cc724985ad 100644
@@ -1,15 +1,15 @@
 menuconfig THUNDERBOLT
-	tristate "Thunderbolt support for Apple devices"
+	tristate "Thunderbolt support"
 	depends on PCI
 	depends on X86 || COMPILE_TEST
 	select APPLE_PROPERTIES if EFI_STUB && X86
 	select CRC32
+	select CRYPTO
+	select CRYPTO_HASH
 	help
-	  Cactus Ridge Thunderbolt Controller driver
-	  This driver is required if you want to hotplug Thunderbolt devices on
-	  Apple hardware.
-
-	  Device chaining is currently not supported.
+	  Thunderbolt Controller driver. This driver is required if you
+	  want to hotplug Thunderbolt devices on Apple hardware or on PCs
+	  with Intel Falcon Ridge or newer.
 
 	  To compile this driver a module, choose M here. The module will be
 	  called thunderbolt.
drivers/thunderbolt/Makefile +1 -1
index 9828e862dd35..4900febc6c8a 100644
@@ -1,3 +1,3 @@
 obj-${CONFIG_THUNDERBOLT} := thunderbolt.o
 thunderbolt-objs := nhi.o ctl.o tb.o switch.o cap.o path.o tunnel_pci.o eeprom.o
-thunderbolt-objs += domain.o dma_port.o
+thunderbolt-objs += domain.o dma_port.o icm.o
drivers/thunderbolt/ctl.c +2 -0
index 27c30ff79a84..69c0232a22f8 100644
@@ -463,6 +463,8 @@ static void tb_ctl_rx_callback(struct tb_ring *ring, struct ring_frame *frame,
 				   "RX: checksum mismatch, dropping packet\n");
 			goto rx;
 		}
+		/* Fall through */
+	case TB_CFG_PKG_ICM_EVENT:
 		tb_ctl_handle_event(pkg->ctl, frame->eof, pkg, frame->size);
 		goto rx;
 
drivers/thunderbolt/domain.c +195 -0

224 lines not on this page · raw message

drivers/thunderbolt/icm.c new file +1058 -0

1,058 lines not on this page · raw message

drivers/thunderbolt/nhi.c +28 -5
index 14311535661d..05af126a2435 100644
@@ -13,7 +13,6 @@
 #include <linux/pci.h>
 #include <linux/interrupt.h>
 #include <linux/module.h>
-#include <linux/dmi.h>
 #include <linux/delay.h>
 
 #include "nhi.h"
@@ -668,6 +667,22 @@ static int nhi_resume_noirq(struct device *dev)
 	return tb_domain_resume_noirq(tb);
 }
 
+static int nhi_suspend(struct device *dev)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	struct tb *tb = pci_get_drvdata(pdev);
+
+	return tb_domain_suspend(tb);
+}
+
+static void nhi_complete(struct device *dev)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	struct tb *tb = pci_get_drvdata(pdev);
+
+	tb_domain_complete(tb);
+}
+
 static void nhi_shutdown(struct tb_nhi *nhi)
 {
 	int i;
@@ -784,10 +799,16 @@ static int nhi_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	/* magic value - clock related? */
 	iowrite32(3906250 / 10000, nhi->iobase + 0x38c00);
 
-	dev_info(&nhi->pdev->dev, "NHI initialized, starting thunderbolt\n");
-	tb = tb_probe(nhi);
+	tb = icm_probe(nhi);
 	if (!tb)
+		tb = tb_probe(nhi);
+	if (!tb) {
+		dev_err(&nhi->pdev->dev,
+			"failed to determine connection manager, aborting\n");
 		return -ENODEV;
+	}
+
+	dev_info(&nhi->pdev->dev, "NHI initialized, starting thunderbolt\n");
 
 	res = tb_domain_add(tb);
 	if (res) {
@@ -826,6 +847,10 @@ static const struct dev_pm_ops nhi_pm_ops = {
 					    * pci-tunnels stay alive.
 					    */
 	.restore_noirq = nhi_resume_noirq,
+	.suspend = nhi_suspend,
+	.freeze = nhi_suspend,
+	.poweroff = nhi_suspend,
+	.complete = nhi_complete,
 };
 
 static struct pci_device_id nhi_ids[] = {
@@ -886,8 +911,6 @@ static int __init nhi_init(void)
 {
 	int ret;
 
-	if (!dmi_match(DMI_BOARD_VENDOR, "Apple Inc."))
-		return -ENOSYS;
 	ret = tb_domain_init();
 	if (ret)
 		return ret;
drivers/thunderbolt/nhi_regs.h +7 -0
index 322fe1fa3a3c..09ed574e92ff 100644
@@ -118,4 +118,11 @@ struct ring_desc {
 #define REG_OUTMAIL_CMD_OPMODE_SHIFT	8
 #define REG_OUTMAIL_CMD_OPMODE_MASK	GENMASK(11, 8)
 
+#define REG_FW_STS			0x39944
+#define REG_FW_STS_NVM_AUTH_DONE	BIT(31)
+#define REG_FW_STS_CIO_RESET_REQ	BIT(30)
+#define REG_FW_STS_ICM_EN_CPU		BIT(2)
+#define REG_FW_STS_ICM_EN_INVERT	BIT(1)
+#define REG_FW_STS_ICM_EN		BIT(0)
+
 #endif
drivers/thunderbolt/switch.c +222 -0

270 lines not on this page · raw message

drivers/thunderbolt/tb.c +7 -0
index ea9de49b5e10..ad2304bad592 100644
@@ -7,6 +7,7 @@
 #include <linux/slab.h>
 #include <linux/errno.h>
 #include <linux/delay.h>
+#include <linux/dmi.h>
 
 #include "tb.h"
 #include "tb_regs.h"
@@ -71,6 +72,8 @@ static void tb_scan_port(struct tb_port *port)
 		return;
 	}
 
+	sw->authorized = true;
+
 	if (tb_switch_add(sw)) {
 		tb_switch_put(sw);
 		return;
@@ -443,10 +446,14 @@ struct tb *tb_probe(struct tb_nhi *nhi)
 	struct tb_cm *tcm;
 	struct tb *tb;
 
+	if (!dmi_match(DMI_BOARD_VENDOR, "Apple Inc."))
+		return NULL;
+
 	tb = tb_domain_alloc(nhi, sizeof(*tcm));
 	if (!tb)
 		return NULL;
 
+	tb->security_level = TB_SECURITY_NONE;
 	tb->cm_ops = &tb_cm_ops;
 
 	tcm = tb_priv(tb);
drivers/thunderbolt/tb.h +79 -0

152 lines not on this page · raw message

drivers/thunderbolt/tb_msgs.h +152 -0

168 lines not on this page · raw message

Signature · 1 line
2.11.0
#45 lore raw

PATCH v3 25/27 thunderbolt: Add support for host and device NVM firmware upgrade

In reply to Mika Westerberg

Starting from Intel Falcon Ridge the NVM firmware can be upgraded by using DMA configuration based mailbox commands. If we detect that the host or device (device support starts from Intel Alpine Ridge) has the DMA configuration based mailbox we expose NVM information to the userspace as two separate Linux NVMem devices: nvm_active and nvm_non_active. The former is read-only portion of the active NVM which firmware upgrade tools can be use to find out suitable NVM image if the device identification strings are not enough.

The latter is write-only portion where the new NVM image is to be written by the userspace. It is up to the userspace to find out right NVM image (the kernel does very minimal validation). The ICM firmware itself authenticates the new NVM firmware and fails the operation if it is not what is expected.

We also expose two new sysfs files per each switch: nvm_version and nvm_authenticate which can be used to read the active NVM version and start the upgrade process.

We also introduce safe mode which is the mode a switch goes when it does not have properly authenticated firmware. In this mode the switch only accepts a couple of commands including flashing a new NVM firmware image and triggering power cycle.

This code is based on the work done by Amir Levy and Michael Jamet.

---
 Documentation/ABI/testing/sysfs-bus-thunderbolt |  26 +
 drivers/thunderbolt/Kconfig                     |   1 +
 drivers/thunderbolt/domain.c                    |  18 +
 drivers/thunderbolt/icm.c                       |  33 +-
 drivers/thunderbolt/nhi.h                       |   1 +
 drivers/thunderbolt/switch.c                    | 603 +++++++++++++++++++++++-
 drivers/thunderbolt/tb.h                        |  40 +-
 7 files changed, 699 insertions(+), 23 deletions(-)
Documentation/ABI/testing/sysfs-bus-thunderbolt +26 -0
index 05b7f9a6431f..2a98149943ea 100644
@@ -82,3 +82,29 @@ Description:	This attribute contains unique_id string of this device.
 		This is either read from hardware registers (UUID on
 		newer hardware) or based on UID from the device DROM.
 		Can be used to uniquely identify particular device.
+
+What:		/sys/bus/thunderbolt/devices/.../nvm_version
+Date:		Sep 2017
+KernelVersion:	4.13
+Contact:	[email protected]
+Description:	If the device has upgradeable firmware the version
+		number is available here. Format: %x.%x, major.minor.
+		If the device is in safe mode reading the file returns
+		-ENODATA instead as the NVM version is not available.
+
+What:		/sys/bus/thunderbolt/devices/.../nvm_authenticate
+Date:		Sep 2017
+KernelVersion:	4.13
+Contact:	[email protected]
+Description:	When new NVM image is written to the non-active NVM
+		area (through non_activeX NVMem device), the
+		authentication procedure is started by writing 1 to
+		this file. If everything goes well, the device is
+		restarted with the new NVM firmware. If the image
+		verification fails an error code is returned instead.
+
+		When read holds status of the last authentication
+		operation if an error occurred during the process. This
+		is directly the status value from the DMA configuration
+		based mailbox before the device is power cycled. Writing
+		0 here clears the status.
drivers/thunderbolt/Kconfig +1 -0
index a9cc724985ad..f4869c38c7e4 100644
@@ -6,6 +6,7 @@ menuconfig THUNDERBOLT
 	select CRC32
 	select CRYPTO
 	select CRYPTO_HASH
+	select NVMEM
 	help
 	  Thunderbolt Controller driver. This driver is required if you
 	  want to hotplug Thunderbolt devices on Apple hardware or on PCs
drivers/thunderbolt/domain.c +18 -0
index bd491d483f7a..4544103b2cb2 100644
@@ -425,6 +425,23 @@ int tb_domain_challenge_switch_key(struct tb *tb, struct tb_switch *sw)
 	return ret;
 }
 
+/**
+ * tb_domain_disconnect_pcie_paths() - Disconnect all PCIe paths
+ * @tb: Domain whose PCIe paths to disconnect
+ *
+ * This needs to be called in preparation for NVM upgrade of the host
+ * controller. Makes sure all PCIe paths are disconnected.
+ *
+ * Return %0 on success and negative errno in case of error.
+ */
+int tb_domain_disconnect_pcie_paths(struct tb *tb)
+{
+	if (!tb->cm_ops->disconnect_pcie_paths)
+		return -EPERM;
+
+	return tb->cm_ops->disconnect_pcie_paths(tb);
+}
+
 int tb_domain_init(void)
 {
 	return bus_register(&tb_bus_type);
@@ -434,4 +451,5 @@ void tb_domain_exit(void)
 {
 	bus_unregister(&tb_bus_type);
 	ida_destroy(&tb_domain_ida);
+	tb_switch_exit();
 }
drivers/thunderbolt/icm.c +32 -1
index 0ffa4ec249ac..8ee340290219 100644
@@ -54,6 +54,7 @@
  *		   where ICM needs to be started manually
  * @vnd_cap: Vendor defined capability where PCIe2CIO mailbox resides
  *	     (only set when @upstream_port is not %NULL)
+ * @safe_mode: ICM is in safe mode
  * @is_supported: Checks if we can support ICM on this controller
  * @get_mode: Read and return the ICM firmware mode (optional)
  * @get_route: Find a route string for given switch
@@ -65,6 +66,7 @@ struct icm {
 	struct delayed_work rescan_work;
 	struct pci_dev *upstream_port;
 	int vnd_cap;
+	bool safe_mode;
 	bool (*is_supported)(struct tb *tb);
 	int (*get_mode)(struct tb *tb);
 	int (*get_route)(struct tb *tb, u8 link, u8 depth, u64 *route);
@@ -852,6 +854,10 @@ static int icm_firmware_init(struct tb *tb)
 		ret = icm->get_mode(tb);
 
 		switch (ret) {
+		case NHI_FW_SAFE_MODE:
+			icm->safe_mode = true;
+			break;
+
 		case NHI_FW_CM_MODE:
 			/* Ask ICM to accept all Thunderbolt devices */
 			nhi_mailbox_cmd(nhi, NHI_MAILBOX_ALLOW_ALL_DEVS, 0);
@@ -879,12 +885,20 @@ static int icm_firmware_init(struct tb *tb)
 
 static int icm_driver_ready(struct tb *tb)
 {
+	struct icm *icm = tb_priv(tb);
 	int ret;
 
 	ret = icm_firmware_init(tb);
 	if (ret)
 		return ret;
 
+	if (icm->safe_mode) {
+		tb_info(tb, "Thunderbolt host controller is in safe mode.\n");
+		tb_info(tb, "You need to update NVM firmware of the controller before it can be used.\n");
+		tb_info(tb, "For latest updates check https://thunderbolttechnology.net/updates.\n");
+		return 0;
+	}
+
 	return __icm_driver_ready(tb, &tb->security_level);
 }
 
@@ -975,12 +989,23 @@ static void icm_complete(struct tb *tb)
 
 static int icm_start(struct tb *tb)
 {
+	struct icm *icm = tb_priv(tb);
 	int ret;
 
-	tb->root_switch = tb_switch_alloc(tb, &tb->dev, 0);
+	if (icm->safe_mode)
+		tb->root_switch = tb_switch_alloc_safe_mode(tb, &tb->dev, 0);
+	else
+		tb->root_switch = tb_switch_alloc(tb, &tb->dev, 0);
 	if (!tb->root_switch)
 		return -ENODEV;
 
+	/*
+	 * NVM upgrade has not been tested on Apple systems and they
+	 * don't provide images publicly either. To be on the safe side
+	 * prevent root switch NVM upgrade on Macs for now.
+	 */
+	tb->root_switch->no_nvm_upgrade = is_apple();
+
 	ret = tb_switch_add(tb->root_switch);
 	if (ret)
 		tb_switch_put(tb->root_switch);
@@ -998,6 +1023,11 @@ static void icm_stop(struct tb *tb)
 	nhi_mailbox_cmd(tb->nhi, NHI_MAILBOX_DRV_UNLOADS, 0);
 }
 
+static int icm_disconnect_pcie_paths(struct tb *tb)
+{
+	return nhi_mailbox_cmd(tb->nhi, NHI_MAILBOX_DISCONNECT_PCIE_PATHS, 0);
+}
+
 /* Falcon Ridge and Alpine Ridge */
 static const struct tb_cm_ops icm_fr_ops = {
 	.driver_ready = icm_driver_ready,
@@ -1009,6 +1039,7 @@ static const struct tb_cm_ops icm_fr_ops = {
 	.approve_switch = icm_fr_approve_switch,
 	.add_switch_key = icm_fr_add_switch_key,
 	.challenge_switch_key = icm_fr_challenge_switch_key,
+	.disconnect_pcie_paths = icm_disconnect_pcie_paths,
 };
 
 struct tb *icm_probe(struct tb_nhi *nhi)
drivers/thunderbolt/nhi.h +1 -0
index 953864ae0ab3..5b5bb2c436be 100644
@@ -155,6 +155,7 @@ enum nhi_fw_mode {
 
 enum nhi_mailbox_cmd {
 	NHI_MAILBOX_SAVE_DEVS = 0x05,
+	NHI_MAILBOX_DISCONNECT_PCIE_PATHS = 0x06,
 	NHI_MAILBOX_DRV_UNLOADS = 0x07,
 	NHI_MAILBOX_ALLOW_ALL_DEVS = 0x23,
 };
drivers/thunderbolt/switch.c +582 -21

682 lines not on this page · raw message

drivers/thunderbolt/tb.h +39 -1
index e02f4e0b2b4d..cbe3e91a3167 100644
@@ -7,6 +7,7 @@
 #ifndef TB_H_
 #define TB_H_
 
+#include <linux/nvmem-provider.h>
 #include <linux/pci.h>
 #include <linux/uuid.h>
 
@@ -15,6 +16,30 @@
 #include "dma_port.h"
 
 /**
+ * struct tb_switch_nvm - Structure holding switch NVM information
+ * @major: Major version number of the active NVM portion
+ * @minor: Minor version number of the active NVM portion
+ * @id: Identifier used with both NVM portions
+ * @active: Active portion NVMem device
+ * @non_active: Non-active portion NVMem device
+ * @buf: Buffer where the NVM image is stored before it is written to
+ *	 the actual NVM flash device
+ * @buf_data_size: Number of bytes actually consumed by the new NVM
+ *		   image
+ * @authenticating: The switch is authenticating the new NVM
+ */
+struct tb_switch_nvm {
+	u8 major;
+	u8 minor;
+	int id;
+	struct nvmem_device *active;
+	struct nvmem_device *non_active;
+	void *buf;
+	size_t buf_data_size;
+	bool authenticating;
+};
+
+/**
  * enum tb_security_level - Thunderbolt security level
  * @TB_SECURITY_NONE: No security, legacy mode
  * @TB_SECURITY_USER: User approval required at minimum
@@ -39,7 +64,8 @@ enum tb_security_level {
  * @ports: Ports in this switch
  * @dma_port: If the switch has port supporting DMA configuration based
  *	      mailbox this will hold the pointer to that (%NULL
- *	      otherwise).
+ *	      otherwise). If set it also means the switch has
+ *	      upgradeable NVM.
  * @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)
@@ -51,6 +77,9 @@ enum tb_security_level {
  * @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)
+ * @nvm: Pointer to the NVM if the switch has one (%NULL otherwise)
+ * @no_nvm_upgrade: Prevent NVM upgrade of this switch
+ * @safe_mode: The switch is in safe-mode
  * @authorized: Whether the switch is authorized by user or policy
  * @work: Work used to automatically authorize a switch
  * @security_level: Switch supported security level
@@ -81,6 +110,9 @@ struct tb_switch {
 	int cap_plug_events;
 	bool is_unplugged;
 	u8 *drom;
+	struct tb_switch_nvm *nvm;
+	bool no_nvm_upgrade;
+	bool safe_mode;
 	unsigned int authorized;
 	struct work_struct work;
 	enum tb_security_level security_level;
@@ -172,6 +204,7 @@ struct tb_path {
  * @approve_switch: Approve switch
  * @add_switch_key: Add key to switch
  * @challenge_switch_key: Challenge switch using key
+ * @disconnect_pcie_paths: Disconnects PCIe paths before NVM update
  */
 struct tb_cm_ops {
 	int (*driver_ready)(struct tb *tb);
@@ -187,6 +220,7 @@ struct tb_cm_ops {
 	int (*add_switch_key)(struct tb *tb, struct tb_switch *sw);
 	int (*challenge_switch_key)(struct tb *tb, struct tb_switch *sw,
 				    const u8 *challenge, u8 *response);
+	int (*disconnect_pcie_paths)(struct tb *tb);
 };
 
 /**
@@ -340,6 +374,7 @@ extern struct device_type tb_switch_type;
 
 int tb_domain_init(void);
 void tb_domain_exit(void);
+void tb_switch_exit(void);
 
 struct tb *tb_domain_alloc(struct tb_nhi *nhi, size_t privsize);
 int tb_domain_add(struct tb *tb);
@@ -351,6 +386,7 @@ void tb_domain_complete(struct tb *tb);
 int tb_domain_approve_switch(struct tb *tb, struct tb_switch *sw);
 int tb_domain_approve_switch_key(struct tb *tb, struct tb_switch *sw);
 int tb_domain_challenge_switch_key(struct tb *tb, struct tb_switch *sw);
+int tb_domain_disconnect_pcie_paths(struct tb *tb);
 
 static inline void tb_domain_put(struct tb *tb)
 {
@@ -359,6 +395,8 @@ static inline void tb_domain_put(struct tb *tb)
 
 struct tb_switch *tb_switch_alloc(struct tb *tb, struct device *parent,
 				  u64 route);
+struct tb_switch *tb_switch_alloc_safe_mode(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);
Signature · 1 line
2.11.0
#46 lore raw

PATCH v3 26/27 thunderbolt: Add documentation how Thunderbolt bus can be used

In reply to Mika Westerberg

Since there are no such tool yet that handles all the low-level details of connecting devices and upgrading their firmware, add a small document that shows how the Thunderbolt bus can be used directly from command line.

---
 Documentation/admin-guide/index.rst       |   1 +
 Documentation/admin-guide/thunderbolt.rst | 199 ++++++++++++++++++++++++++++++
 2 files changed, 200 insertions(+)
 create mode 100644 Documentation/admin-guide/thunderbolt.rst
Documentation/admin-guide/index.rst +1 -0
index 8c60a8a32a1a..6d99a7ce6e21 100644
@@ -61,6 +61,7 @@ configure specific aspects of kernel behavior to your liking.
    java
    ras
    pm/index
+   thunderbolt
 
 .. only::  subproject and html
 
Documentation/admin-guide/thunderbolt.rst new file +199 -0

199 lines not on this page · raw message

Signature · 1 line
2.11.0
Acked-by on v3 26/27
#47 lore raw

Re: [PATCH v3 26/27] thunderbolt: Add documentation how Thunderbolt bus can be used

Quoted 9 lines · Mika Westerberg

Since there are no such tool yet that handles all the low-level details of connecting devices and upgrading their firmware, add a small document that shows how the Thunderbolt bus can be used directly from command line.

Very nice, thanks for documenting all of this.

#48 lore raw

PATCH v3 27/27 MAINTAINERS: Add maintainers for Thunderbolt driver

In reply to Mika Westerberg

We will be helping Andreas to maintain the Thunderbolt driver.

---
 MAINTAINERS | 3 +++
 1 file changed, 3 insertions(+)
MAINTAINERS +3 -0
index 053c3bdd1fe5..f1504e283ae2 100644
@@ -11328,6 +11328,9 @@ F:	Documentation/tee.txt
 
 THUNDERBOLT DRIVER
 M:	Andreas Noever <[email protected]>
+M:	Michael Jamet <[email protected]>
+M:	Mika Westerberg <[email protected]>
+M:	Yehezkel Bernat <[email protected]>
 S:	Maintained
 F:	drivers/thunderbolt/
 
Signature · 1 line
2.11.0
#49 lore raw
In reply to Mika Westerberg
Quoted 98 lines · Mika Westerberg

Hi,

This is a third version of the patch series adding support for Thunderbolt
security levels and NVM firmware upgrade. PCs running Intel Falcon Ridge or
newer need these in order to connect devices if the security level is set
to "user(SL1) or secure(SL2)" from BIOS.

The security levels were added to to prevent DMA attacks when PCIe is tunneled over Thunderbolt fabric where IOMMU is not available or cannot be
enabled for different reasons.

This series converts the Thunderbolt driver to a Linux bus and makes each
Thunderbolt device (aka Thunderbolt switch or router or endpoint) a Linux
device which is connected to that bus addressed by their routing string
(this is the addressing Thunderbolt uses).

With these patches the user can authorize devices using sysfs attributes
like (following USB):

  # echo 1 > /sys/bus/thunderbolt/devices/0-1/authorized

If the BIOS security level is set to "secure" and the device supports secure connect on the first connect the device is uploaded a random key:

  # key=$(openssl rand -hex 32)

  # echo $key > /sys/bus/thunderbolt/devices/0-1/key
  # echo 1 > /sys/bus/thunderbolt/devices/0-1/authorized

Next time the device is plugged in the user can verify (challenge) the device using the same key:

  # echo $key > /sys/bus/thunderbolt/devices/0-1/key
  # echo 2 > /sys/bus/thunderbolt/devices/0-1/authorized

The device identification information is found below each device sysfs node and includes following attributes:

  device - Device ID
  device_name - Name of the device
  vendor - Vendor ID
  vendor_name - Name of the vendor
  unique_id - Unique ID string identifying this particular
device

In addition these patches add support for upgrading NVM firmware running on
a host or device by running something like:

  # dd if=KYK_TBT_FW_0018.bin of=/sys/bus/thunderbolt/devices/0-
0/nvm_non_active0/nvmem
  # echo 1 > /sys/bus/thunderbolt/devices/0-0/nvm_authenticate

More information how to use the Thunderbolt bus can be found in patch [26/27].

Where Internal Connection Manager (ICM) firmware is available and usable,
we use it in the driver. This also includes newer Apple Macbooks with Alpine Ridge. For older Macbooks the driver works as before but in addition
the Thunderbolt bus is available there as well (including possibility to
upgrade NVM firmware of connected devices).

Note for Macs the Linux native PCIe hotplug support does not work well with
the Thunderbolt PCIe topologies where there is need to put all available
resources to the PCIe downstream port where the PCIe chain is extended.
This is something we need to fix. In the mean time is a way to work it around by passing "pci=hpbussize=10,hpmemsize=2M" or so to the kernel command line.

Regarding Christoph's UUID series here [1], they now include the existing
types for backwards compatibility so this series compiles and works fine
with them. We will convert the Thunderbolt driver to use these new types
once that series hits mainline.

Andreas and Lukas, do you have any comments or concerns regarding this series?

We are currently porting Amir's network driver on top of the Thunderbolt
bus.

Quoted 6 lines

The previous version (v2) can be found in [2].

[1] http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/uu
id-types
[2] https://lwn.net/Articles/723935/

Awesome work, Mika!

Have no more to comment.

Quoted 107 lines

Changes from the previos version (v2):

[PATCH v2 03/27] thunderbolt: Do not try to read UID if DROM offset is read as 0
  - Add Suggested-by Andreas

[PATCH v2 07/27] thunderbolt: Allow passing NULL to tb_ctl_free()
  - Add Suggested-by Andy

[PATCH v2 09/27] thunderbolt: Convert switch to a device
  - Rework comment regarding UUID to mention that it is not strictly following rules of UUID format.
  - Update Documentation/ABI/testing/sysfs-bus-thunderbolt unique_id attribute description accordingly.

[PATCH v2 12/27] thunderbolt: Refactor and fix parsing of port drom entries
  - Add Reviewed-by Andy

[PATCH v2 13/27] thunderbolt: Read vendor and device name from DROM
  - Use ksrtndup() instead and drop parse_name()

[PATCH v2 21/27] thunderbolt: Store Thunderbolt generation in the switch structure
  - Make the unknown switch defaulting to generation 1 more explicit
  - Add Reviewed-by Andy

[PATCH v2 24/27] thunderbolt: Add support for Internal Connection Manager (ICM)
  - Allow secure connect only when BIOS setting is in "secure connect/SL2" to follow what Windows does.

[PATCH v2 25/27] thunderbolt: Add support for host and device NVM firmware upgrade
  - Add check for sw->nvm in nvm_authenticate_store() to prevent NULL pointer dereference when NVMem devices are not yet added.

[PATCH v2 26/27] thunderbolt: Add documentation how Thunderbolt bus can be used
  - Update documentation to mention that secure connect is only available when the BIOS setting is in "secure connect/SL2".

Lukas Wunner (1):
  thunderbolt: Refactor and fix parsing of port drom entries

Mika Westerberg (26):
  thunderbolt: Use const buffer pointer in write operations
  thunderbolt: No need to read UID of the root switch on resume
  thunderbolt: Do not try to read UID if DROM offset is read as 0
  thunderbolt: Do not warn about newer DROM versions
  thunderbolt: Add MSI-X support
  thunderbolt: Rework capability handling
  thunderbolt: Allow passing NULL to tb_ctl_free()
  thunderbolt: Introduce thunderbolt bus and connection manager
  thunderbolt: Convert switch to a device
  thunderbolt: Fail switch adding operation if reading DROM fails
  thunderbolt: Do not fail if DROM data CRC32 is invalid
  thunderbolt: Read vendor and device name from DROM
  thunderbolt: Move control channel messages to tb_msgs.h
  thunderbolt: Expose get_route() to other files
  thunderbolt: Expose make_header() to other files
  thunderbolt: Let the connection manager handle all notifications
  thunderbolt: Rework control channel to be more reliable
  thunderbolt: Add new Thunderbolt PCI IDs
  thunderbolt: Add support for NHI mailbox
  thunderbolt: Store Thunderbolt generation in the switch structure
  thunderbolt: Add support for DMA configuration based mailbox
  thunderbolt: Do not touch the hardware if the NHI is gone on resume
  thunderbolt: Add support for Internal Connection Manager (ICM)
  thunderbolt: Add support for host and device NVM firmware upgrade
  thunderbolt: Add documentation how Thunderbolt bus can be used
  MAINTAINERS: Add maintainers for Thunderbolt driver

 Documentation/ABI/testing/sysfs-bus-thunderbolt |  110 +++ Documentation/admin-guide/index.rst             |    1 + Documentation/admin-guide/thunderbolt.rst       |  199 ++++ MAINTAINERS                                     |    3 + drivers/thunderbolt/Kconfig                     |   13 +- drivers/thunderbolt/Makefile                    |    2 +-

 drivers/thunderbolt/cap.c                       |  169 ++--
 drivers/thunderbolt/ctl.c                       |  665 +++++++++----
 drivers/thunderbolt/ctl.h                       |  105 +-

 drivers/thunderbolt/dma_port.c                  |  524 ++++++++++ drivers/thunderbolt/dma_port.h                  |   34 + drivers/thunderbolt/domain.c                    |  455 +++++++++ drivers/thunderbolt/eeprom.c                    |  119 ++- drivers/thunderbolt/icm.c                       | 1089
+++++++++++++++++++++
 drivers/thunderbolt/nhi.c                       |  306 +++++- drivers/thunderbolt/nhi.h                       |   93 +- drivers/thunderbolt/nhi_regs.h                  |   27 + drivers/thunderbolt/switch.c                    | 1178
+++++++++++++++++++++--
 drivers/thunderbolt/tb.c                        |  237 +++-- drivers/thunderbolt/tb.h                        |  251 ++++- drivers/thunderbolt/tb_msgs.h                   |  260 +++++ drivers/thunderbolt/tb_regs.h                   |   50 +- drivers/thunderbolt/tunnel_pci.c                |   17 +-

 23 files changed, 5323 insertions(+), 584 deletions(-)

 create mode 100644 Documentation/ABI/testing/sysfs-bus-thunderbolt create mode 100644 Documentation/admin-guide/thunderbolt.rst create mode 100644 drivers/thunderbolt/dma_port.c create mode 100644 drivers/thunderbolt/dma_port.h create mode 100644 drivers/thunderbolt/domain.c
 create mode 100644 drivers/thunderbolt/icm.c
 create mode 100644 drivers/thunderbolt/tb_msgs.h

Signature · 2 lines
Andy Shevchenko <[email protected]>
Intel Finland Oy
#50 lore raw
In reply to Mika Westerberg
Quoted 6 lines · Mika Westerberg

Hi,

This is a third version of the patch series adding support for Thunderbolt security levels and NVM firmware upgrade. PCs running Intel Falcon Ridge or newer need these in order to connect devices if the security level is set to "user(SL1) or secure(SL2)" from BIOS.

All looks good to me, very nice work.

I don't know what tree it should go in through, but if Andreas wants me to take it, I will if I can get his signed-off-by.

thanks,

greg k-h

#51 lore raw
Quoted 9 lines · Greg Kroah-Hartman
Quoted 6 lines · Mika Westerberg

Hi,

This is a third version of the patch series adding support for Thunderbolt security levels and NVM firmware upgrade. PCs running Intel Falcon Ridge or newer need these in order to connect devices if the security level is set to "user(SL1) or secure(SL2)" from BIOS.

All looks good to me, very nice work.

Thanks!

Quoted 2 lines

I don't know what tree it should go in through, but if Andreas wants me to take it, I will if I can get his signed-off-by.

That would be perfect.

Andreas, do you have any objections?

I will prepare another version where I've fixed the VSEC vs. VSE thing in the capability rework patch.

maintainer
#52 lore raw
Quoted 11 lines · Mika Westerberg
Quoted 9 lines · Greg Kroah-Hartman
Quoted 6 lines · Mika Westerberg

Hi,

This is a third version of the patch series adding support for Thunderbolt security levels and NVM firmware upgrade. PCs running Intel Falcon Ridge or newer need these in order to connect devices if the security level is set to "user(SL1) or secure(SL2)" from BIOS.

All looks good to me, very nice work.

Quoted 8 lines

Thanks!

Quoted 2 lines

I don't know what tree it should go in through, but if Andreas wants me to take it, I will if I can get his signed-off-by.

That would be perfect.

Andreas, do you have any objections?

No, Thanks a lot.

Greg, can you take this through your tree?

Mika, I have a quick question regarding the pci side of things (your "pci=hpbussize=10,hpmemsize=2M" workaround). Does that work for nested hotplug or just on the first level? Back when I was having a look at enabling chaining in the native driver I could not get pci to properly assign bus numbers to nested bridges. It always ran out of bus number after one level (irregardless of hpbussize). Has the pci behaviour changed or does the ICM somehow preconfigure the bridges before handing them of to linux?

Cheers
Andreas

Quoted 2 lines

I will prepare another version where I've fixed the VSEC vs. VSE thing in the capability rework patch.

#53 lore raw
Quoted 18 lines · Andreas Noever
Quoted 16 lines · Mika Westerberg
Quoted 9 lines · Greg Kroah-Hartman
Quoted 6 lines · Mika Westerberg

Hi,

This is a third version of the patch series adding support for Thunderbolt security levels and NVM firmware upgrade. PCs running Intel Falcon Ridge or newer need these in order to connect devices if the security level is set to "user(SL1) or secure(SL2)" from BIOS.

All looks good to me, very nice work.

Thanks!

Quoted 2 lines

I don't know what tree it should go in through, but if Andreas wants me to take it, I will if I can get his signed-off-by.

Quoted 6 lines
Quoted 3 lines

That would be perfect.

Andreas, do you have any objections?

No, Thanks a lot.

Thanks!

Quoted 4 lines

Greg, can you take this through your tree?

Quoted 8 lines

Mika, I have a quick question regarding the pci side of things (your "pci=hpbussize=10,hpmemsize=2M" workaround). Does that work for nested hotplug or just on the first level? Back when I was having a look at enabling chaining in the native driver I could not get pci to properly assign bus numbers to nested bridges. It always ran out of bus number after one level (irregardless of hpbussize). Has the pci behaviour changed or does the ICM somehow preconfigure the bridges before handing them of to linux?

I don't think ICM does any preconfiguration when native PCIe hotplug is used. For PCs we typically use BIOS assisted ACPI hotplug and the SMI handler configures the bridges before it notifies the OS.

In case of native PCIe hotplug, it still runs out of bus space, that's why it is workaround. It works to some depth though, because it adds 10 to all bridges that it finds when the scan is done. When next device is added you have that 10 which it then allocates to bridges downstream.

Anyway it should be fixed properly and it is on my todo list once I get still missing parts finished. Unless someone else fixes it first, that is ;-)

One solution that I've been thinking is to introduce some sort of resource allocation policies to root ports depending what is connected to them, and in case of Thunderbolt we follow the BIOS assisted hotplug way so that we assign the remaining resources to the downstream port where the chain is extended (this can be figured out from the registers, I think).

There is also another PCI/ACPI related issue that Mario reported where we execute _INI() methods before the initial PCI scan on boot when Thunderbolt device is connected causing Linux to accidentally find the upstream port of the Thunderbolt host controller before it is configured properly by the BIOS ACPI hotplug handler. I've discussed this with Rafael and he has an idea how we could fix it but it probably requires some changes to ACPICA first. Also on my todo list :)

#54 lore raw
In reply to Andreas Noever
Quoted 18 lines · Andreas Noever
Quoted 16 lines · Mika Westerberg
Quoted 9 lines · Greg Kroah-Hartman
Quoted 6 lines · Mika Westerberg

Hi,

This is a third version of the patch series adding support for Thunderbolt security levels and NVM firmware upgrade. PCs running Intel Falcon Ridge or newer need these in order to connect devices if the security level is set to "user(SL1) or secure(SL2)" from BIOS.

All looks good to me, very nice work.

Thanks!

Quoted 2 lines

I don't know what tree it should go in through, but if Andreas wants me to take it, I will if I can get his signed-off-by.

Quoted 8 lines
Quoted 3 lines

That would be perfect.

Andreas, do you have any objections?

No, Thanks a lot.

Greg, can you take this through your tree?

Sure, but wasn't there going to be a new version of this series, or should I just take this one as-is and follow-up patches will happen? It's up to both of you, either is fine with me.

thanks,

greg k-h

#55 lore raw
Quoted 3 lines · Greg Kroah-Hartman

Sure, but wasn't there going to be a new version of this series, or should I just take this one as-is and follow-up patches will happen? It's up to both of you, either is fine with me.

I will send an updated version (v4) of the series later today.