LinuxLists.cc - [PATCH 0/2] thunderbolt: Fix issues with more complex endpoints.

2014-06-12 21:12:08

Subject: [PATCH 0/2] thunderbolt: Fix issues with more complex endpoints.

Hi,

I have got my hand on a slightly more complex Thunderbolt device (a Startech
docking station). This series fixes the following issues that I have observed
with this device:

- Some ports on this device are disabled. Accessing them results in an error.
- This device uses both Thunderbolt links which are represented as two ports on
the switch. We have to ensure that we only scan one of these ports.

Part 1 of the series reads the switch configuration (disabled ports and which
port pairs are "dual link ports") from eeprom. Part 2 uses this information to
avoid scanning these ports.

Andreas Noever (2):
thunderbolt: Read port configuration from eeprom.
thunderbolt: Fix nontrivial endpoint devices.

drivers/thunderbolt/ctl.c | 2 +-
drivers/thunderbolt/eeprom.c | 266 ++++++++++++++++++++++++++++++++++++++++++-
drivers/thunderbolt/switch.c | 44 ++++---
drivers/thunderbolt/tb.c | 5 +
drivers/thunderbolt/tb.h | 7 +-
5 files changed, 300 insertions(+), 24 deletions(-)

--
2.0.0

2014-06-12 21:12:09

by Andreas Noever

[permalink] [raw]

Subject: [PATCH 1/2] thunderbolt: Read port configuration from eeprom.

All Thunderbolt switches (except the root switch) contain a drom which
contains information about the device. Right now we only read the UID.

Add code to read and parse this drom. For now we are only interested in
which ports are disabled and which ports are "dual link ports" (a
physical thunderbolt port/socket contains two such ports).

Signed-off-by: Andreas Noever <[email protected]>
---
drivers/thunderbolt/eeprom.c | 266 ++++++++++++++++++++++++++++++++++++++++++-
drivers/thunderbolt/switch.c | 4 +-
drivers/thunderbolt/tb.h | 7 +-
3 files changed, 270 insertions(+), 7 deletions(-)

diff --git a/drivers/thunderbolt/eeprom.c b/drivers/thunderbolt/eeprom.c
index f28e402..0d5a80b 100644
--- a/drivers/thunderbolt/eeprom.c
+++ b/drivers/thunderbolt/eeprom.c
@@ -4,6 +4,7 @@
* Copyright (c) 2014 Andreas Noever <[email protected]>
*/

+#include <linux/crc32.h>
#include "tb.h"

/**
@@ -152,9 +153,86 @@ static int tb_eeprom_read_n(struct tb_switch *sw, u16 offset, u8 *val,
return tb_eeprom_active(sw, false);
}

-int tb_eeprom_read_uid(struct tb_switch *sw, u64 *uid)
+static u8 tb_crc8(u8 *data, int len)
+{
+ int i, j;
+ u8 val = 0xff;
+ for (i = 0; i < len; i++) {
+ val ^= data[i];
+ for (j = 0; j < 8; j++)
+ val = (val << 1) ^ ((val & 0x80) ? 7 : 0);
+ }
+ return val;
+}
+
+static u32 tb_crc32(void *data, size_t len)
+{
+ return ~__crc32c_le(~0, data, len);
+}
+
+#define TB_DROM_DATA_START 13
+struct tb_drom_header {
+ /* BYTE 0 */
+ u8 uid_crc8; /* checksum for uid */
+ /* BYTES 1-8 */
+ u64 uid;
+ /* BYTES 9-12 */
+ u32 data_crc32; /* checksum for data_len bytes starting at byte 13 */
+ /* BYTE 13 */
+ u8 device_rom_revision; /* should be <= 1 */
+ u16 data_len:10;
+ u8 __unknown1:6;
+ /* BYTES 16-21 */
+ u16 vendor_id;
+ u16 model_id;
+ u8 model_rev;
+ u8 eeprom_rev;
+} __packed;
+
+enum tb_drom_entry_type {
+ TB_DROM_ENTRY_GENERIC,
+ TB_DROM_ENTRY_PORT,
+};
+
+struct tb_drom_entry_header {
+ u8 len;
+ u8 index:6;
+ bool port_disabled:1; /* only valid if type is TB_DROM_ENTRY_PORT */
+ enum tb_drom_entry_type type:1;
+} __packed;
+
+struct tb_drom_entry_port {
+ /* BYTES 0-1 */
+ struct tb_drom_entry_header header;
+ /* BYTE 2 */
+ u8 dual_link_port_rid:4;
+ u8 link_nr:1;
+ u8 unknown1:2;
+ bool has_dual_link_port:1;
+
+ /* BYTE 3 */
+ u8 dual_link_port_nr:6;
+ u8 unknown2:2;
+
+ /* BYTES 4 - 5 TODO decode */
+ u8 micro2:4;
+ u8 micro1:4;
+ u8 micro3;
+
+ /* BYTES 5-6, TODO: verify (find hardware that has these set) */
+ u8 peer_port_rid:4;
+ u8 unknown3:3;
+ bool has_peer_port:1;
+ u8 peer_port_nr:6;
+ u8 unknown4:2;
+} __packed;
+
+
+/**
+ * tb_eeprom_get_drom_offset - get drom offset within eeprom
+ */
+int tb_eeprom_get_drom_offset(struct tb_switch *sw, u16 *offset)
{
- u8 data[9];
struct tb_cap_plug_events cap;
int res;
if (!sw->cap_plug_events) {
@@ -165,6 +243,7 @@ int tb_eeprom_read_uid(struct tb_switch *sw, u64 *uid)
sizeof(cap) / 4);
if (res)
return res;
+
if (!cap.eeprom_ctl.present || cap.eeprom_ctl.not_present) {
tb_sw_warn(sw, "no NVM\n");
return -ENOSYS;
@@ -175,15 +254,194 @@ int tb_eeprom_read_uid(struct tb_switch *sw, u64 *uid)
cap.drom_offset);
return -ENXIO;
}
+ *offset = cap.drom_offset;
+ return 0;
+}
+
+/**
+ * tb_drom_read_uid_only - read uid directly from drom
+ *
+ * Does not use the cached copy in sw->drom. Used during resume to check switch
+ * identity.
+ */
+int tb_drom_read_uid_only(struct tb_switch *sw, u64 *uid)
+{
+ u8 data[9];
+ u16 drom_offset;
+ u8 crc;
+ int res = tb_eeprom_get_drom_offset(sw, &drom_offset);
+ if (res)
+ return res;

/* read uid */
- res = tb_eeprom_read_n(sw, cap.drom_offset, data, 9);
+ res = tb_eeprom_read_n(sw, drom_offset, data, 9);
if (res)
return res;
- /* TODO: check checksum in data[0] */
+
+ crc = tb_crc8(data + 1, 8);
+ if (crc != data[0]) {
+ tb_sw_warn(sw, "uid crc8 missmatch (expected: %#x, got: %#x)\n",
+ data[0], crc);
+ return -EIO;
+ }
+
*uid = *(u64 *)(data+1);
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)
+{
+ 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)
+ return 0;
+
+ res = tb_port_read(port, &type, TB_CFG_PORT, 2, 1);
+ if (res)
+ return res;
+ type &= 0xffffff;
+
+ if (type == TB_TYPE_PORT) {
+ struct tb_drom_entry_port *entry = (void *) header;
+ if (header->len != sizeof(*entry)) {
+ tb_sw_warn(sw,
+ "port entry has size %#x (expected %#lx)\n",
+ header->len, sizeof(struct tb_drom_entry_port));
+ return -EIO;
+ }
+ tb_drom_parse_port_entry(port, entry);
+ }
+ return 0;
+}
+
+/**
+ * tb_drom_parse_entries - parse the linked list of drom entries
+ *
+ * Drom must have been copied to sw->drom.
+ */
+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;
+
+ while (pos < drom_size) {
+ struct tb_drom_entry_header *entry = (void *) (sw->drom + pos);
+ if (pos + 1 == drom_size || pos + entry->len > drom_size
+ || !entry->len) {
+ tb_sw_warn(sw, "drom buffer overrun, aborting\n");
+ return -EIO;
+ }
+
+ tb_drom_parse_entry(sw, entry);
+
+ pos += entry->len;
+ }
+ return 0;
+}
+
+/**
+ * tb_drom_read - copy drom to sw->drom and parse it
+ */
+int tb_drom_read(struct tb_switch *sw)
+{
+ u16 drom_offset;
+ u16 size;
+ u32 crc;
+ struct tb_drom_header *header;
+ int res;
+ if (sw->drom)
+ return 0;
+
+ if (tb_route(sw) == 0) {
+ /*
+ * The root switch contains only a dummy drom (header only,
+ * no entries). Hardcode the configuration here.
+ */
+ tb_drom_read_uid_only(sw, &sw->uid);
+
+ sw->ports[1].link_nr = 0;
+ sw->ports[2].link_nr = 1;
+ sw->ports[1].dual_link_port = &sw->ports[2];
+ sw->ports[2].dual_link_port = &sw->ports[1];
+
+ sw->ports[3].link_nr = 0;
+ sw->ports[4].link_nr = 1;
+ sw->ports[3].dual_link_port = &sw->ports[4];
+ sw->ports[4].dual_link_port = &sw->ports[3];
+ return 0;
+ }
+
+ res = tb_eeprom_get_drom_offset(sw, &drom_offset);
+ if (res)
+ return res;
+
+ res = tb_eeprom_read_n(sw, drom_offset + 14, (u8 *) &size, 2);
+ if (res)
+ return res;
+ size &= 0x3ff;
+ size += TB_DROM_DATA_START;
+ tb_sw_info(sw, "reading drom (length: %#x)\n", size);
+ if (size < sizeof(*header)) {
+ tb_sw_warn(sw, "drom too small, aborting\n");
+ return -EIO;
+ }
+
+ sw->drom = kzalloc(size, GFP_KERNEL);
+ if (!sw->drom)
+ return -ENOMEM;
+ res = tb_eeprom_read_n(sw, drom_offset, sw->drom, size);
+ if (res)
+ goto err;
+
+ header = (void *) sw->drom;
+
+ if (header->data_len + TB_DROM_DATA_START != size) {
+ tb_sw_warn(sw, "drom size mismatch, aborting\n");
+ goto err;
+ }
+
+ crc = tb_crc8((u8 *) &header->uid, 8);
+ if (crc != header->uid_crc8) {
+ tb_sw_warn(sw,
+ "drom uid crc8 mismatch (expected: %#x, got: %#x), aborting\n",
+ header->uid_crc8, crc);
+ goto err;
+ }
+ sw->uid = header->uid;
+
+ 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",
+ header->data_crc32, crc);
+ goto err;
+ }
+
+ if (header->device_rom_revision > 1)
+ tb_sw_warn(sw, "drom device_rom_revision %#x unknown\n",
+ header->device_rom_revision);
+
+ return tb_drom_parse_entries(sw);
+err:
+ kfree(sw->drom);
+ return -EIO;
+
+}
diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
index c2a24b6..9dfb8e1 100644
--- a/drivers/thunderbolt/switch.c
+++ b/drivers/thunderbolt/switch.c
@@ -400,7 +400,7 @@ struct tb_switch *tb_switch_alloc(struct tb *tb, u64 route)
}
sw->cap_plug_events = cap;

- if (tb_eeprom_read_uid(sw, &sw->uid))
+ if (tb_drom_read_uid_only(sw, &sw->uid))
tb_sw_warn(sw, "could not read uid from eeprom\n");
else
tb_sw_info(sw, "uid: %#llx\n", sw->uid);
@@ -442,7 +442,7 @@ int tb_switch_resume(struct tb_switch *sw)
u64 uid;
tb_sw_info(sw, "resuming switch\n");

- err = tb_eeprom_read_uid(sw, &uid);
+ err = tb_drom_read_uid_only(sw, &uid);
if (err) {
tb_sw_warn(sw, "uid read failed\n");
return err;
diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
index 63e89d0..18ade5e 100644
--- a/drivers/thunderbolt/tb.h
+++ b/drivers/thunderbolt/tb.h
@@ -22,6 +22,7 @@ struct tb_switch {
u64 uid;
int cap_plug_events; /* offset, zero if not found */
bool is_unplugged; /* unplugged, will go away */
+ u8 *drom;
};

/**
@@ -33,6 +34,9 @@ struct tb_port {
struct tb_port *remote; /* remote port, NULL if not connected */
int cap_phy; /* offset, zero if not found */
u8 port; /* port number on switch */
+ bool disabled; /* disabled by eeprom */
+ struct tb_port *dual_link_port;
+ u8 link_nr:1;
};

/**
@@ -237,7 +241,8 @@ int tb_path_activate(struct tb_path *path);
void tb_path_deactivate(struct tb_path *path);
bool tb_path_is_invalid(struct tb_path *path);

-int tb_eeprom_read_uid(struct tb_switch *sw, u64 *uid);
+int tb_drom_read(struct tb_switch *sw);
+int tb_drom_read_uid_only(struct tb_switch *sw, u64 *uid);

static inline int tb_route_length(u64 route)
--
2.0.0

2014-06-12 21:12:11

by Andreas Noever

[permalink] [raw]

Subject: [PATCH 2/2] thunderbolt: Fix nontrivial endpoint devices.

Fix issues observed with the Startech docking station:

Fix the type of the route parameter in tb_ctl_rx. It should be u64 and not
u8 (which only worked for short routes).

A thunderbolt cable contains two lanes. If both endpoints support it a
connection will be established on both lanes. Previously we tried to
scan below both "dual link ports". Use the information extracted from
the drom to only scan behind ports with lane_nr == 0.

Endpoints with more complex thunderbolt controllers have some of their
ports disabled (for example the NHI port or one of the HDMI/DP ports).
Accessing them results in an error so we now ignore ports which are
marked as disabled in the drom.

Signed-off-by: Andreas Noever <[email protected]>
---
drivers/thunderbolt/ctl.c | 2 +-
drivers/thunderbolt/switch.c | 42 +++++++++++++++++++++++++-----------------
drivers/thunderbolt/tb.c | 5 +++++
3 files changed, 31 insertions(+), 18 deletions(-)

diff --git a/drivers/thunderbolt/ctl.c b/drivers/thunderbolt/ctl.c
index 9b0120b..d04fee4 100644
--- a/drivers/thunderbolt/ctl.c
+++ b/drivers/thunderbolt/ctl.c
@@ -439,7 +439,7 @@ rx:
*/
static struct tb_cfg_result tb_ctl_rx(struct tb_ctl *ctl, void *buffer,
size_t length, int timeout_msec,
- u8 route, enum tb_cfg_pkg_type type)
+ u64 route, enum tb_cfg_pkg_type type)
{
struct tb_cfg_result res;
struct ctl_pkg *pkg;
diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
index 9dfb8e1..0d50e7e 100644
--- a/drivers/thunderbolt/switch.c
+++ b/drivers/thunderbolt/switch.c
@@ -180,20 +180,17 @@ int tb_port_clear_counter(struct tb_port *port, int counter)
*
* Return: Returns 0 on success or an error code on failure.
*/
-static int tb_init_port(struct tb_switch *sw, u8 port_nr)
+static int tb_init_port(struct tb_port *port)
{
int res;
int cap;
- struct tb_port *port = &sw->ports[port_nr];
- port->sw = sw;
- port->port = port_nr;
- port->remote = NULL;
+
res = tb_port_read(port, &port->config, TB_CFG_PORT, 0, 8);
if (res)
return res;

/* Port 0 is the switch itself and has no PHY. */
- if (port->config.type == TB_TYPE_PORT && port_nr != 0) {
+ if (port->config.type == TB_TYPE_PORT && port->port != 0) {
cap = tb_find_cap(port, TB_CFG_PORT, TB_CAP_PHY);

if (cap > 0)
@@ -202,7 +199,7 @@ static int tb_init_port(struct tb_switch *sw, u8 port_nr)
tb_port_WARN(port, "non switch port without a PHY\n");
}

- tb_dump_port(sw->tb, &port->config);
+ tb_dump_port(port->sw->tb, &port->config);

/* TODO: Read dual link port, DP port and more from EEPROM. */
return 0;
@@ -329,6 +326,7 @@ void tb_switch_free(struct tb_switch *sw)
tb_plug_events_active(sw, false);

kfree(sw->ports);
+ kfree(sw->drom);
kfree(sw);
}

@@ -381,18 +379,16 @@ struct tb_switch *tb_switch_alloc(struct tb *tb, u64 route)

/* initialize ports */
sw->ports = kcalloc(sw->config.max_port_number + 1, sizeof(*sw->ports),
- GFP_KERNEL);
+ GFP_KERNEL);
if (!sw->ports)
goto err;

for (i = 0; i <= sw->config.max_port_number; i++) {
- if (tb_init_port(sw, i))
- goto err;
- /* TODO: check if port is disabled (EEPROM) */
+ /* minimum setup for tb_find_cap and tb_drom_read to work */
+ sw->ports[i].sw = sw;
+ sw->ports[i].port = i;
}

- /* TODO: I2C, IECS, EEPROM, link controller */
-
cap = tb_find_cap(&sw->ports[0], TB_CFG_SWITCH, TB_CAP_PLUG_EVENTS);
if (cap < 0) {
tb_sw_warn(sw, "cannot find TB_CAP_PLUG_EVENTS aborting\n");
@@ -400,10 +396,21 @@ struct tb_switch *tb_switch_alloc(struct tb *tb, u64 route)
}
sw->cap_plug_events = cap;

- if (tb_drom_read_uid_only(sw, &sw->uid))
- tb_sw_warn(sw, "could not read uid from eeprom\n");
- else
- tb_sw_info(sw, "uid: %#llx\n", sw->uid);
+ /* read drom */
+ if (tb_drom_read(sw))
+ tb_sw_warn(sw, "tb_eeprom_read_rom failed, continuing\n");
+ tb_sw_info(sw, "uid: %#llx\n", sw->uid);
+
+ for (i = 0; i <= sw->config.max_port_number; i++) {
+ if (sw->ports[i].disabled) {
+ tb_port_info(&sw->ports[i], "disabled by eeprom\n");
+ continue;
+ }
+ if (tb_init_port(&sw->ports[i]))
+ goto err;
+ }
+
+ /* TODO: I2C, IECS, link controller */

if (tb_plug_events_active(sw, true))
goto err;
@@ -411,6 +418,7 @@ struct tb_switch *tb_switch_alloc(struct tb *tb, u64 route)
return sw;
err:
kfree(sw->ports);
+ kfree(sw->drom);
kfree(sw);
return NULL;
}
diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
index 1aa6dd7..d2c3fe3 100644
--- a/drivers/thunderbolt/tb.c
+++ b/drivers/thunderbolt/tb.c
@@ -38,6 +38,11 @@ static void tb_scan_port(struct tb_port *port)
return;
if (port->config.type != TB_TYPE_PORT)
return;
+ if (port->dual_link_port && port->link_nr)
+ return; /*
+ * Downstream switch is reachable through two ports.
+ * Only scan on the primary port (link_nr == 0).
+ */
if (tb_wait_for_port(port, false) <= 0)
return;
if (port->remote) {
--
2.0.0

2014-06-12 22:02:59

by Greg KH

[permalink] [raw]

Subject: Re: [PATCH 0/2] thunderbolt: Fix issues with more complex endpoints.

On Thu, Jun 12, 2014 at 11:11:45PM +0200, Andreas Noever wrote:
> Hi,
>
> I have got my hand on a slightly more complex Thunderbolt device (a Startech
> docking station). This series fixes the following issues that I have observed
> with this device:
>
> - Some ports on this device are disabled. Accessing them results in an error.
> - This device uses both Thunderbolt links which are represented as two ports on
> the switch. We have to ensure that we only scan one of these ports.
>
> Part 1 of the series reads the switch configuration (disabled ports and which
> port pairs are "dual link ports") from eeprom. Part 2 uses this information to
> avoid scanning these ports.
>
> Andreas Noever (2):
> thunderbolt: Read port configuration from eeprom.
> thunderbolt: Fix nontrivial endpoint devices.
>
> drivers/thunderbolt/ctl.c | 2 +-
> drivers/thunderbolt/eeprom.c | 266 ++++++++++++++++++++++++++++++++++++++++++-
> drivers/thunderbolt/switch.c | 44 ++++---
> drivers/thunderbolt/tb.c | 5 +
> drivers/thunderbolt/tb.h | 7 +-
> 5 files changed, 300 insertions(+), 24 deletions(-)
>
> --

Thanks, I'll queue these up on top of your other patches once 3.16-rc1
is out.

greg k-h

2014-06-19 21:11:24

by Greg KH

[permalink] [raw]