Thunderbolt material for v4.20, comprising:
* A fix to skip disabled ports on tunnel establishment. I am not aware
that this has caused breakage in the real world so far, so no need to
apply to v4.19 or backport to stable.
* Obtain PCI slot number from DROM and use it to correlate PCI devices
with Thunderbolt ports.
* I am nominating myself as co-maintainer, see commit message for the
reasons.
Regarding the driver's verbosity which Stephen Hemminger and Mika
Westerberg have taken exception to:
* Patch [2/5] logs the PCI slot number as well as data whose purpose is
unknown at KERN_INFO severity. I am not opposed to log the former at
KERN_DEBUG severity, but the latter must stay at KERN_INFO to allow us
to collect the data of various Thunderbolt devices in the wild and
reverse-engineer its meaning. If Intel divulges the data's meaning then
it need no longer be dumped. This would be ideal for everyone involved.
* Patch [4/5] logs a message at KERN_INFO severity whenever a PCI device
correlates or no longer correlates with a Thunderbolt port. I am not
opposed to toning this down to KERN_DEBUG, subject to the introduction
of tb_port_dbg().
Thanks,
Lukas
Lukas Wunner (5):
thunderbolt: Skip disabled ports on tunnel establishment
thunderbolt: Obtain PCI slot number from DROM
thunderbolt: Move upstream_port to struct tb
thunderbolt: Correlate PCI devices with Thunderbolt ports
MAINTAINERS: Add Lukas Wunner as co-maintainer of thunderbolt
MAINTAINERS | 1 +
drivers/thunderbolt/Makefile | 2 +-
drivers/thunderbolt/adapter_pci.c | 166 ++++++++++++++++++++++++++++++
drivers/thunderbolt/adapter_pci.h | 19 ++++
drivers/thunderbolt/domain.c | 12 +++
drivers/thunderbolt/eeprom.c | 21 ++++
drivers/thunderbolt/icm.c | 65 +++---------
drivers/thunderbolt/switch.c | 14 ++-
drivers/thunderbolt/tb.c | 23 ++---
drivers/thunderbolt/tb.h | 45 ++++++++
include/linux/thunderbolt.h | 2 +
11 files changed, 305 insertions(+), 65 deletions(-)
create mode 100644 drivers/thunderbolt/adapter_pci.c
create mode 100644 drivers/thunderbolt/adapter_pci.h
--
2.18.0
The port numbers of PCIe adapters on a CIO switch and the slot numbers
of their corresponding PCI devices don't necessarily match up in
ascending order.
E.g. Light Ridge in host mode (MacBookPro9,1) mixes them up like this:
switch port 7 == pci slot 4 (PCIe downstream port)
switch port 8 == pci slot 5 (PCIe downstream port)
switch port 9 == pci slot 6 (PCIe downstream port)
switch port 10 == pci slot 3 (PCIe downstream port)
Light Ridge in endpoint mode (Sonnet Echo Express):
switch port 7 == pci slot 4 (PCIe downstream port)
switch port 8 == pci slot 5 (PCIe downstream port)
switch port 9 (PCIe downstream port, disabled in DROM)
switch port 10 == pci slot 0 (PCIe upstream port, pci slot 3 in DROM)
Obtain the slot number used by a switch port from the DROM and store it
in struct tb_port. This will allow us to correlate PCI devices with
Thunderbolt ports. For space efficiency, use a union in struct tb_port
to store attributes that are specific to certain port types.
Attributes specific to TB_TYPE_PORT could be moved into a separate
struct within the union.
The slot number occupies 3 bits in the third byte of PCI DROM entries.
The purpose of the remaining 5 bits is unknown (function number?).
Downstream port entries have 3 bytes, upstream port entries 11 bytes.
The slot number in upstream port entries is not that of the port itself
(which is always zero), but that of the PCI endpoint device built into
the Thunderbolt product. In the case of the Sonnet Echo Express, the
upstream port specifies slot 3, which is used by a PLX switch.
The purpose of the additional 8 bytes present on upstream ports is
unknown (number of PCIe lanes, priority requirements, direct tunnel to
the host desired?). Examples:
01 00 64 00 00 00 00 00 (Sonnet Echo Express, PLX switch)
01 40 0a 00 00 00 00 00 (Apple Gigabit Ethernet Adapter, BCM957762)
Signed-off-by: Lukas Wunner <[email protected]>
---
drivers/thunderbolt/eeprom.c | 21 +++++++++++++++++++++
drivers/thunderbolt/switch.c | 14 +++++++++++++-
drivers/thunderbolt/tb.h | 9 +++++++++
3 files changed, 43 insertions(+), 1 deletion(-)
diff --git a/drivers/thunderbolt/eeprom.c b/drivers/thunderbolt/eeprom.c
index 3e8caf22c294..ce6e037fdb39 100644
--- a/drivers/thunderbolt/eeprom.c
+++ b/drivers/thunderbolt/eeprom.c
@@ -236,6 +236,15 @@ struct tb_drom_entry_port {
u8 unknown4:2;
} __packed;
+struct tb_drom_entry_pci {
+ /* BYTES 0-1 */
+ struct tb_drom_entry_header header;
+ /* BYTE 2 */
+ u8 unknown:5;
+ u8 slot:3;
+ /* BYTES 3-10 are only present on PCIe upstream ports */
+} __packed;
+
/**
* tb_eeprom_get_drom_offset - get drom offset within eeprom
@@ -365,6 +374,18 @@ static int tb_drom_parse_entry_port(struct tb_switch *sw,
if (entry->has_dual_link_port)
port->dual_link_port =
&port->sw->ports[entry->dual_link_port_nr];
+ } else if (type == TB_TYPE_PCIE_UP || type == TB_TYPE_PCIE_DOWN) {
+ struct tb_drom_entry_pci *entry = (void *) header;
+ if (header->len < sizeof(*entry)) {
+ tb_sw_warn(sw,
+ "port entry has size %#x (expected %#zx+)\n",
+ header->len, sizeof(struct tb_drom_entry_pci));
+ return -EIO;
+ }
+ port->pci.devfn = PCI_DEVFN(entry->slot, 0);
+ memcpy(port->pci.unknown, header + 1,
+ min(header->len - sizeof(struct tb_drom_entry_header),
+ sizeof(port->pci.unknown)));
}
return 0;
}
diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
index 7442bc4c6433..54fd42250935 100644
--- a/drivers/thunderbolt/switch.c
+++ b/drivers/thunderbolt/switch.c
@@ -577,6 +577,7 @@ int tb_port_clear_counter(struct tb_port *port, int counter)
*/
static int tb_init_port(struct tb_port *port)
{
+ struct tb *tb = port->sw->tb;
int res;
int cap;
@@ -594,9 +595,20 @@ static int tb_init_port(struct tb_port *port)
tb_port_WARN(port, "non switch port without a PHY\n");
}
- tb_dump_port(port->sw->tb, &port->config);
+ tb_dump_port(tb, &port->config);
/* TODO: Read dual link port, DP port and more from EEPROM. */
+ switch (port->config.type) {
+ case TB_TYPE_PCIE_UP:
+ case TB_TYPE_PCIE_DOWN:
+ tb_info(tb, " PCI slot: %02x.0\n", PCI_SLOT(port->pci.devfn));
+ tb_info(tb, " PCI unknown data: %*ph\n",
+ (int)sizeof(port->pci.unknown), port->pci.unknown);
+ break;
+ default:
+ break;
+ }
+
return 0;
}
diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
index 5067d69d0501..755183f0b257 100644
--- a/drivers/thunderbolt/tb.h
+++ b/drivers/thunderbolt/tb.h
@@ -125,6 +125,9 @@ struct tb_switch {
* @dual_link_port: If the switch is connected using two ports, points
* to the other port.
* @link_nr: Is this primary or secondary port on the dual_link.
+ * @pci: Data specific to PCIe adapters
+ * @pci.devfn: PCI slot/function according to DROM
+ * @pci.unknown: Unknown data in DROM (to be reverse-engineered)
*/
struct tb_port {
struct tb_regs_port_header config;
@@ -136,6 +139,12 @@ struct tb_port {
bool disabled;
struct tb_port *dual_link_port;
u8 link_nr:1;
+ union {
+ struct {
+ u8 devfn;
+ u8 unknown[9];
+ } pci;
+ };
};
/**
--
2.18.0
If a PCIe downstream adapter is marked disabled in the DROM, that port
is ineligible for tunnel establishment, so skip over it when searching
for an unused port.
Signed-off-by: Lukas Wunner <[email protected]>
---
drivers/thunderbolt/tb.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
index 1424581fd9af..0da2e7a06ab5 100644
--- a/drivers/thunderbolt/tb.c
+++ b/drivers/thunderbolt/tb.c
@@ -147,6 +147,8 @@ static struct tb_port *tb_find_unused_down_port(struct tb_switch *sw)
int res;
int data;
for (i = 1; i <= sw->config.max_port_number; i++) {
+ if (sw->ports[i].disabled)
+ continue;
if (tb_is_upstream_port(&sw->ports[i]))
continue;
if (sw->ports[i].config.type != TB_TYPE_PCIE_DOWN)
--
2.18.0
Andreas Noever has let it be known off-list already a while ago that he
currently cannot spare as much time for Thunderbolt development as he'd
like. As a result the driver's development has become dominated by
Intel.
I would like to step up as co-maintainer to provide additional checks
and balances and prevent the driver from degenerating into an Intel-only
show. A number of things really irk me:
* I've been contributing to this driver as well as Thunderbolt-related
bits to the EFI, GPU and PCI subsystems for close to three years and
was explicitly asked by Intel to cc them on every Thunderbolt-related
patch. Yet Intel did not see fit to cc me on their changes that went
into 4.17. I literally only learned about their existence from
reading the news. In the 4.19 cycle I was only cc'ed on a subset of
their patches.
* Intel's efforts have been focussed exclusively on firmware-controlled
tunnel management (ICM). They made no contributions to OS-controlled
tunnel management. ICM cannot be used on Macs with Thunderbolt 1 + 2.
ICM requires trusting a firmware blob. ICM does not offer the same
versatility as OS-controlled tunnel management, e.g. using separate
tunnels to afford different QoS levels or correlation of Thunderbolt
ports with PCI devices. Apple chose OS-controlled tunnel management
for very valid technical reasons.
* Our OS-controlled tunnel management still lacks important features
such as daisy-chaining and DP tunnels. Each feature needs to be
reverse-engineered because there is no public spec. Intel issued a
press statement in May 2017 promising to make the specification public
"next year". More than a year has passed -- no spec. The company has
since changed leadership, who knows if they haven't silently canned
the plans for a public spec? I offered to sign an NDA and go through
a disclosure process for every patch -- no reaction.
* Reverse-engineering requires verbose logging so that we're able to
collect data on various systems and endpoint devices to deduce the
meaning of registers. Yet Intel now seeks to mute log output, thereby
curbing our reverse-engineering efforts. This exemplifies a worrying
tendency to ignore the needs of non-Intel stakeholders in the
developer community or even undermine them.
* Recent Intel contributions are maintainer self-commits without any
Reviewed-by tags, which is generally considered a bad practice.
Review comments offered by Intel-outsiders are not taken seriously.
For example the driver's initcall level has been fiddled with twice
now. A review comment pointing out the fragility of abusing initcall
levels to implement dependencies and suggesting the use of a notifier
chain instead was summarily dismissed as unnecessary unless it breaks
a third time.
Signed-off-by: Lukas Wunner <[email protected]>
---
MAINTAINERS | 1 +
1 file changed, 1 insertion(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index a5b256b25905..8815f4639e58 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14445,6 +14445,7 @@ F: drivers/platform/x86/thinkpad_acpi.c
THUNDERBOLT DRIVER
M: Andreas Noever <[email protected]>
+M: Lukas Wunner <[email protected]>
M: Michael Jamet <[email protected]>
M: Mika Westerberg <[email protected]>
M: Yehezkel Bernat <[email protected]>
--
2.18.0
The code for ICM-controlled tunnel management stores a pointer to the
upstream port in struct icm. We're going to need it in the code for
OS-controlled tunnel management as well, so move the pointer to
struct tb which is shared by both tunnel management methods.
Searching for the upstream port previously comprised walking upward from
the NHI until a PCIe upstream port is found. That seems needlessly
complicated since on all known Thunderbolt controllers, the upstream
port is the NHI's grandparent, i.e. exactly two levels above. Simplify
the search accordingly.
No functional change intended.
Signed-off-by: Lukas Wunner <[email protected]>
---
drivers/thunderbolt/domain.c | 12 +++++++
drivers/thunderbolt/icm.c | 65 +++++++++---------------------------
include/linux/thunderbolt.h | 2 ++
3 files changed, 29 insertions(+), 50 deletions(-)
diff --git a/drivers/thunderbolt/domain.c b/drivers/thunderbolt/domain.c
index 092381e2accf..ba9e4ed88bf2 100644
--- a/drivers/thunderbolt/domain.c
+++ b/drivers/thunderbolt/domain.c
@@ -326,6 +326,7 @@ struct device_type tb_domain_type = {
struct tb *tb_domain_alloc(struct tb_nhi *nhi, size_t privsize)
{
struct tb *tb;
+ int i;
/*
* Make sure the structure sizes map with that the hardware
@@ -350,6 +351,15 @@ struct tb *tb_domain_alloc(struct tb_nhi *nhi, size_t privsize)
if (!tb->wq)
goto err_remove_ida;
+ tb->upstream = nhi->pdev;
+ for (i = 0; i < 2; i++) {
+ tb->upstream = pci_upstream_bridge(tb->upstream);
+ if (!tb->upstream) {
+ pci_err(nhi->pdev, "cannot find upstream, aborting\n");
+ goto err_destroy_wq;
+ }
+ }
+
tb->dev.parent = &nhi->pdev->dev;
tb->dev.bus = &tb_bus_type;
tb->dev.type = &tb_domain_type;
@@ -359,6 +369,8 @@ struct tb *tb_domain_alloc(struct tb_nhi *nhi, size_t privsize)
return tb;
+err_destroy_wq:
+ destroy_workqueue(tb->wq);
err_remove_ida:
ida_simple_remove(&tb_domain_ida, tb->index);
err_free:
diff --git a/drivers/thunderbolt/icm.c b/drivers/thunderbolt/icm.c
index e1e264a9a4c7..b6ce3eea3de5 100644
--- a/drivers/thunderbolt/icm.c
+++ b/drivers/thunderbolt/icm.c
@@ -51,11 +51,8 @@
* struct icm - Internal connection manager private data
* @request_lock: Makes sure only one message is send to ICM at time
* @rescan_work: Work used to rescan the surviving switches after resume
- * @upstream_port: Pointer to the PCIe upstream port this host
- * controller is connected. This is only set for systems
- * where ICM needs to be started manually
* @vnd_cap: Vendor defined capability where PCIe2CIO mailbox resides
- * (only set when @upstream_port is not %NULL)
+ * (only set on Macs as they require a firmware reset to use ICM)
* @safe_mode: ICM is in safe mode
* @max_boot_acl: Maximum number of preboot ACL entries (%0 if not supported)
* @rpm: Does the controller support runtime PM (RTD3)
@@ -72,7 +69,6 @@
struct icm {
struct mutex request_lock;
struct delayed_work rescan_work;
- struct pci_dev *upstream_port;
size_t max_boot_acl;
int vnd_cap;
bool safe_mode;
@@ -1188,60 +1184,29 @@ icm_tr_xdomain_disconnected(struct tb *tb, const struct icm_pkg_header *hdr)
}
}
-static struct pci_dev *get_upstream_port(struct pci_dev *pdev)
-{
- struct pci_dev *parent;
-
- parent = pci_upstream_bridge(pdev);
- while (parent) {
- if (!pci_is_pcie(parent))
- return NULL;
- if (pci_pcie_type(parent) == PCI_EXP_TYPE_UPSTREAM)
- break;
- parent = pci_upstream_bridge(parent);
- }
-
- if (!parent)
- return NULL;
-
- switch (parent->device) {
- 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_LP_BRIDGE:
- case PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_4C_BRIDGE:
- case PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_2C_BRIDGE:
- return parent;
- }
-
- return NULL;
-}
-
static bool icm_ar_is_supported(struct tb *tb)
{
- struct pci_dev *upstream_port;
struct icm *icm = tb_priv(tb);
+ int cap;
/*
* Starting from Alpine Ridge we can use ICM on Apple machines
* as well. We just need to reset and re-enable it first.
+ * Reset is performed through the vendor specific capability.
*/
if (!x86_apple_machine)
return true;
- /*
- * Find the upstream PCIe port in case we need to do reset
- * through its vendor specific registers.
- */
- upstream_port = get_upstream_port(tb->nhi->pdev);
- if (upstream_port) {
- int cap;
-
- cap = pci_find_ext_capability(upstream_port,
+ switch (tb->upstream->device) {
+ 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_LP_BRIDGE:
+ case PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_4C_BRIDGE:
+ case PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_2C_BRIDGE:
+ cap = pci_find_ext_capability(tb->upstream,
PCI_EXT_CAP_ID_VNDR);
if (cap > 0) {
- icm->upstream_port = upstream_port;
icm->vnd_cap = cap;
-
return true;
}
}
@@ -1485,7 +1450,7 @@ static int pci2cio_wait_completion(struct icm *icm, unsigned long timeout_msec)
u32 cmd;
do {
- pci_read_config_dword(icm->upstream_port,
+ pci_read_config_dword(icm_to_tb(icm)->upstream,
icm->vnd_cap + PCIE2CIO_CMD, &cmd);
if (!(cmd & PCIE2CIO_CMD_START)) {
if (cmd & PCIE2CIO_CMD_TIMEOUT)
@@ -1502,7 +1467,7 @@ static int pci2cio_wait_completion(struct icm *icm, unsigned long timeout_msec)
static int pcie2cio_read(struct icm *icm, enum tb_cfg_space cs,
unsigned int port, unsigned int index, u32 *data)
{
- struct pci_dev *pdev = icm->upstream_port;
+ struct pci_dev *pdev = icm_to_tb(icm)->upstream;
int ret, vnd_cap = icm->vnd_cap;
u32 cmd;
@@ -1523,7 +1488,7 @@ static int pcie2cio_read(struct icm *icm, enum tb_cfg_space cs,
static int pcie2cio_write(struct icm *icm, enum tb_cfg_space cs,
unsigned int port, unsigned int index, u32 data)
{
- struct pci_dev *pdev = icm->upstream_port;
+ struct pci_dev *pdev = icm_to_tb(icm)->upstream;
int vnd_cap = icm->vnd_cap;
u32 cmd;
@@ -1543,7 +1508,7 @@ static int icm_firmware_reset(struct tb *tb, struct tb_nhi *nhi)
struct icm *icm = tb_priv(tb);
u32 val;
- if (!icm->upstream_port)
+ if (!icm->vnd_cap)
return -ENODEV;
/* Put ARC to wait for CIO reset event to happen */
@@ -1599,7 +1564,7 @@ static int icm_reset_phy_port(struct tb *tb, int phy_port)
u32 val0, val1;
int ret;
- if (!icm->upstream_port)
+ if (!icm->vnd_cap)
return 0;
if (phy_port) {
diff --git a/include/linux/thunderbolt.h b/include/linux/thunderbolt.h
index a3ed26082bc1..968fe1519f8f 100644
--- a/include/linux/thunderbolt.h
+++ b/include/linux/thunderbolt.h
@@ -70,6 +70,7 @@ enum tb_security_level {
* @index: Linux assigned domain number
* @security_level: Current security level
* @nboot_acl: Number of boot ACLs the domain supports
+ * @upstream: Pointer to the PCIe upstream port of this host controller
* @privdata: Private connection manager specific data
*/
struct tb {
@@ -83,6 +84,7 @@ struct tb {
int index;
enum tb_security_level security_level;
size_t nboot_acl;
+ struct pci_dev *upstream;
unsigned long privdata[0];
};
--
2.18.0
macOS correlates PCI devices with Thunderbolt ports and stores their
device paths in each other's I/O Registry entry:
IOThunderboltPort@7 {
"PCI Device" = 4
"PCI Function" = 0
"PCI Path" = "IOService:/AppleACPIPlatformExpert/PCI0@0/AppleACPIPCI/PEG1@1,1/IOPP/UPSB@0/IOPP/DSB2@4"
...
}
DSB2@4 {
"pcidebug" = "6:4:0(132:132)"
"Thunderbolt Path" = "IOService:/AppleACPIPlatformExpert/PCI0@0/AppleACPIPCI/PEG1@1,1/IOPP/UPSB@0/IOPP/DSB0@0/IOPP/NHI0@0/AppleThunderboltHAL/AppleThunderboltNHIType1/IOThunderboltController/IOThunderboltPort@6/IOThunderboltSwitchType1/IOThunderboltPort@7"
...
}
Do the same by finding the Thunderbolt port corresponding to a PCI
device from a bus notifier and storing a pointer to the PCI device in
struct tb_port. On initial switch scan, fill in the pointers for
already enumerated PCI devices.
This achieves a unidirectional mapping from tb_port to pci_dev. If an
inverse mapping is needed, it may be possible to use the sysdata pointer
in struct pci_dev.
To find the Thunderbolt port, the PCI slot numbers specified in the root
switch's DROM need to be available. On Macs with Thunderbolt 1 that's
not the case unless the kernel is booted by the EFI stub.
Moreover the driver needs to know which tunnels have been established,
which is not the case with ICM-controlled tunnel management, so the bus
notifier is only registered if tunnel management is under OS control.
Perhaps it is possible to retrieve a list of established tunnels from
the ICM firmware, or if all else fails, follow the hop entries in a
downstream port's config space to discover established tunnels.
Correlation would then also work for ICM-controlled tunnel management.
Ideas what we can do with correlation:
* Represent the relationship between PCI devices and Thunderbolt ports
with symlinks in sysfs.
* Thunderbolt controllers up to revision 1 of Cactus Ridge 4C have to
use INTx because MSI signaling is broken. This results in hotplug
ports sharing interrupts with other devices and, when daisy-chaining
multiple affected Thunderbolt controllers, can lead to extremely
unbalanced interrupt usage. To avoid this we could prefer downstream
ports for tunnel establishment which do not share interrupts (based
on the nr_actions field of the correlated PCI device's irq_desc).
* Alternatively, we could use non-working MSI signaling on affected
controllers and synthesize an interrupt whenever a tunnel is
established or goes down on unplug.
The shared interrupts issue is grave. This is /proc/interrupts on a
MacBookPro9,1 with a daisy-chain of two Light Ridge controllers and
one Port Ridge (all with broken MSI signaling):
16: IO-APIC 16-fasteoi pciehp
17: IO-APIC 17-fasteoi pciehp, pciehp, pciehp, mmc0, snd_hda_intel, b43
18: IO-APIC 18-fasteoi pciehp, pciehp, i801_smbus
19: IO-APIC 19-fasteoi pciehp
Signed-off-by: Lukas Wunner <[email protected]>
---
drivers/thunderbolt/Makefile | 2 +-
drivers/thunderbolt/adapter_pci.c | 166 ++++++++++++++++++++++++++++++
drivers/thunderbolt/adapter_pci.h | 19 ++++
drivers/thunderbolt/tb.c | 21 ++--
drivers/thunderbolt/tb.h | 36 +++++++
5 files changed, 230 insertions(+), 14 deletions(-)
create mode 100644 drivers/thunderbolt/adapter_pci.c
create mode 100644 drivers/thunderbolt/adapter_pci.h
diff --git a/drivers/thunderbolt/Makefile b/drivers/thunderbolt/Makefile
index f2f0de27252b..c91b9f7d4c0b 100644
--- a/drivers/thunderbolt/Makefile
+++ b/drivers/thunderbolt/Makefile
@@ -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 icm.o property.o xdomain.o
+thunderbolt-objs += adapter_pci.o domain.o dma_port.o icm.o property.o xdomain.o
diff --git a/drivers/thunderbolt/adapter_pci.c b/drivers/thunderbolt/adapter_pci.c
new file mode 100644
index 000000000000..e5abcbd6d064
--- /dev/null
+++ b/drivers/thunderbolt/adapter_pci.c
@@ -0,0 +1,166 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * PCIe adapters on a Thunderbolt switch serve as endpoints for PCI tunnels.
+ * Each may be attached to an upstream or downstream port of the PCIe switch
+ * integrated into a Thunderbolt controller.
+ *
+ * Copyright (C) 2018 Lukas Wunner <[email protected]>
+ */
+
+#include "tb.h"
+#include "tunnel_pci.h"
+#include "adapter_pci.h"
+
+/**
+ * tb_is_pci_adapter() - whether given PCI device is a Thunderbolt PCIe adapter
+ * @pdev: PCI device
+ *
+ * For simplicity this function returns a false positive in the following cases
+ * and callers need to make sure they can handle that:
+ * * Upstream port on a host controller
+ * * Downstream port to the XHCI on a host controller
+ * * Downstream port on non-chainable endpoint controllers such as Port Ridge
+ */
+static bool tb_is_pci_adapter(struct pci_dev *pdev)
+{
+ /* downstream ports with devfn 0 are reserved for the NHI */
+ return pdev->is_thunderbolt &&
+ (pci_pcie_type(pdev) == PCI_EXP_TYPE_UPSTREAM ||
+ (pci_pcie_type(pdev) == PCI_EXP_TYPE_DOWNSTREAM &&
+ pdev->devfn));
+}
+
+/**
+ * tb_pci_find_port() - locate Thunderbolt port for given PCI device
+ * @pdev: PCI device
+ *
+ * Walk up the PCI hierarchy from @pdev to discover the sequence of
+ * PCIe upstream and downstream ports leading to the host controller.
+ * Then walk down the Thunderbolt daisy-chain following the previously
+ * discovered sequence along the tunnels we've established.
+ *
+ * Return the port corresponding to @pdev, or %NULL if none was found.
+ *
+ * This function needs to be called under the global Thunderbolt lock
+ * to prevent tb_switch and tb_pci_tunnel structs from going away.
+ */
+static struct tb_port *tb_pci_find_port(struct tb *tb, struct pci_dev *pdev)
+{
+ struct tb_cm *tcm = tb_priv(tb);
+ struct tb_pci_tunnel *tunnel;
+ struct pci_dev *parent_pdev;
+ struct tb_port *parent_port;
+ struct tb_port *port;
+
+ if (!tb_is_pci_adapter(pdev))
+ return NULL;
+
+ /* base of the recursion: we've reached the host controller */
+ if (pdev->bus == tb->upstream->subordinate) {
+ tb_sw_for_each_port(tb->root_switch, port)
+ if (port->pci.devfn == pdev->devfn)
+ return port;
+
+ return NULL;
+ }
+
+ /* recurse up the PCI hierarchy */
+ parent_pdev = pci_upstream_bridge(pdev);
+ if (!parent_pdev)
+ return NULL;
+
+ parent_port = tb_pci_find_port(tb, parent_pdev);
+ if (!parent_port)
+ return NULL;
+
+ switch (parent_port->config.type) {
+ case TB_TYPE_PCIE_UP:
+ /*
+ * A PCIe upstream adapter is the parent of
+ * a PCIe downstream adapter on the same switch.
+ */
+ tb_sw_for_each_port(parent_port->sw, port)
+ if (port->config.type == TB_TYPE_PCIE_DOWN &&
+ port->pci.devfn == pdev->devfn)
+ return port;
+ return NULL;
+ case TB_TYPE_PCIE_DOWN:
+ /*
+ * A PCIe downstream adapter is the parent of
+ * a PCIe upstream adapter at the other end of a tunnel.
+ */
+ list_for_each_entry(tunnel, &tcm->tunnel_list, list)
+ if (tunnel->down_port == parent_port)
+ return tunnel->up_port;
+ return NULL;
+ default:
+ return NULL;
+ }
+}
+
+/**
+ * tb_pci_notifier_call() - Thunderbolt PCI bus notifier
+ * @nb: Notifier block embedded in struct tb_cm
+ * @action: Notifier action
+ * @data: PCI device
+ *
+ * On addition of PCI device @data, correlate it with a PCIe adapter on the
+ * Thunderbolt bus and store a pointer to the PCI device in struct tb_port.
+ * On deletion, reset the pointer to %NULL.
+ */
+int tb_pci_notifier_call(struct notifier_block *nb, unsigned long action,
+ void *data)
+{
+ struct tb_cm *tcm = container_of(nb, struct tb_cm, pci_notifier);
+ struct tb *tb = tb_from_priv(tcm);
+ struct device *dev = data;
+ struct pci_dev *pdev = to_pci_dev(dev);
+ struct tb_port *port;
+
+ if ((action != BUS_NOTIFY_ADD_DEVICE &&
+ action != BUS_NOTIFY_DEL_DEVICE) || !tb_is_pci_adapter(pdev))
+ return NOTIFY_DONE;
+
+ mutex_lock(&tb->lock);
+ port = tb_pci_find_port(tb, pdev);
+ if (!port)
+ goto out;
+
+ switch (action) {
+ case BUS_NOTIFY_ADD_DEVICE:
+ port->pci.dev = pdev;
+ tb_port_info(port, "correlates with %s\n", pci_name(pdev));
+ break;
+ case BUS_NOTIFY_DEL_DEVICE:
+ port->pci.dev = NULL;
+ tb_port_info(port, "no longer correlates with %s\n",
+ pci_name(pdev));
+ break;
+ }
+out:
+ mutex_unlock(&tb->lock);
+ return NOTIFY_DONE;
+}
+
+/**
+ * tb_pci_correlate() - Correlate given PCI device with a Thunderbolt port
+ * @pdev: PCI device
+ * @data: Thunderbolt bus
+ *
+ * Correlate @pdev with a PCIe adapter on Thunderbolt bus @data and store a
+ * pointer to the PCI device in struct tb_port. Intended to be used as a
+ * pci_walk_bus() callback.
+ */
+int tb_pci_correlate(struct pci_dev *pdev, void *data)
+{
+ struct tb *tb = data;
+ struct tb_port *port;
+
+ port = tb_pci_find_port(tb, pdev);
+ if (port) {
+ port->pci.dev = pdev;
+ tb_port_info(port, "correlates with %s\n", pci_name(pdev));
+ }
+
+ return 0;
+}
diff --git a/drivers/thunderbolt/adapter_pci.h b/drivers/thunderbolt/adapter_pci.h
new file mode 100644
index 000000000000..76de43cd0f17
--- /dev/null
+++ b/drivers/thunderbolt/adapter_pci.h
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * PCIe adapters on a Thunderbolt switch serve as endpoints for PCI tunnels.
+ * Each may be attached to an upstream or downstream port of the PCIe switch
+ * integrated into a Thunderbolt controller.
+ *
+ * Copyright (C) 2018 Lukas Wunner <[email protected]>
+ */
+
+#ifndef ADAPTER_PCI_H_
+#define ADAPTER_PCI_H_
+
+#include <linux/notifier.h>
+
+int tb_pci_notifier_call(struct notifier_block *nb, unsigned long action,
+ void *data);
+int tb_pci_correlate(struct pci_dev *pdev, void *data);
+
+#endif
diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
index 0da2e7a06ab5..3549a51b23d9 100644
--- a/drivers/thunderbolt/tb.c
+++ b/drivers/thunderbolt/tb.c
@@ -13,19 +13,7 @@
#include "tb.h"
#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;
-};
+#include "adapter_pci.h"
/* enumeration & hot plug handling */
@@ -355,6 +343,7 @@ static void tb_stop(struct tb *tb)
struct tb_pci_tunnel *tunnel;
struct tb_pci_tunnel *n;
+ bus_unregister_notifier(&pci_bus_type, &tcm->pci_notifier);
/* tunnels are only present after everything has been initialized */
list_for_each_entry_safe(tunnel, n, &tcm->tunnel_list, list) {
tb_pci_deactivate(tunnel);
@@ -395,6 +384,11 @@ static int tb_start(struct tb *tb)
/* Full scan to discover devices added before the driver was loaded. */
tb_scan_switch(tb->root_switch);
+
+ /* Correlate PCI devices with Thunderbolt ports */
+ bus_register_notifier(&pci_bus_type, &tcm->pci_notifier);
+ pci_walk_bus(tb->upstream->subordinate, tb_pci_correlate, tb);
+
tb_activate_pcie_devices(tb);
/* Allow tb_handle_hotplug to progress events */
@@ -469,6 +463,7 @@ struct tb *tb_probe(struct tb_nhi *nhi)
tcm = tb_priv(tb);
INIT_LIST_HEAD(&tcm->tunnel_list);
+ tcm->pci_notifier.notifier_call = tb_pci_notifier_call;
return tb;
}
diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
index 755183f0b257..603e6214b060 100644
--- a/drivers/thunderbolt/tb.h
+++ b/drivers/thunderbolt/tb.h
@@ -17,6 +17,20 @@
#include "ctl.h"
#include "dma_port.h"
+/**
+ * struct tb_cm - Native Thunderbolt connection manager
+ * @pci_notifier: Notifier to correlate PCI devices with Thunderbolt ports
+ * @tunnel_list: List of active tunnels
+ * @hotplug_active: tb_handle_hotplug() will stop processing 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 notifier_block pci_notifier;
+ struct list_head tunnel_list;
+ unsigned int hotplug_active:1;
+};
+
/**
* struct tb_switch_nvm - Structure holding switch NVM information
* @major: Major version number of the active NVM portion
@@ -128,6 +142,9 @@ struct tb_switch {
* @pci: Data specific to PCIe adapters
* @pci.devfn: PCI slot/function according to DROM
* @pci.unknown: Unknown data in DROM (to be reverse-engineered)
+ * @pci.dev: PCI device of corresponding PCIe upstream or downstream port
+ * (%NULL if not found or if removed by PCI core). To access,
+ * acquire Thunderbolt lock, call pci_dev_get(), release the lock.
*/
struct tb_port {
struct tb_regs_port_header config;
@@ -143,6 +160,7 @@ struct tb_port {
struct {
u8 devfn;
u8 unknown[9];
+ struct pci_dev *dev;
} pci;
};
};
@@ -250,6 +268,11 @@ static inline void *tb_priv(struct tb *tb)
return (void *)tb->privdata;
}
+static inline struct tb *tb_from_priv(void *priv)
+{
+ return priv - offsetof(struct tb, privdata);
+}
+
#define TB_AUTOSUSPEND_DELAY 15000 /* ms */
/* helper functions & macros */
@@ -332,6 +355,19 @@ static inline int tb_port_write(struct tb_port *port, const void *buffer,
length);
}
+/**
+ * tb_sw_for_each_port() - iterate over ports on a switch
+ * @switch: pointer to struct tb_switch over whose ports shall be iterated
+ * @port: pointer to struct tb_port which shall be used as the iterator
+ *
+ * Excludes port 0, which is the switch itself and therefore irrelevant for
+ * most iterations.
+ */
+#define tb_sw_for_each_port(switch, port) \
+ for (port = &(switch)->ports[1]; \
+ port <= &(switch)->ports[(switch)->config.max_port_number]; \
+ port++)
+
#define tb_err(tb, fmt, arg...) dev_err(&(tb)->nhi->pdev->dev, fmt, ## arg)
#define tb_WARN(tb, fmt, arg...) dev_WARN(&(tb)->nhi->pdev->dev, fmt, ## arg)
#define tb_warn(tb, fmt, arg...) dev_warn(&(tb)->nhi->pdev->dev, fmt, ## arg)
--
2.18.0
Hi Lukas,
I'm including Greg here in case I've done something wrong as a maintainer.
Since I've only maintained Thunderbolt quite short time, it may be that
I've done mistakes but certainly I did not deliberately try to make life of
people developing this for older Apple systems harder.
On Sun, Sep 09, 2018 at 11:42:01PM +0200, Lukas Wunner wrote:
> Andreas Noever has let it be known off-list already a while ago that he
> currently cannot spare as much time for Thunderbolt development as he'd
> like. As a result the driver's development has become dominated by
> Intel.
I was not aware of this. Althought Andreas has not commented much
lately, I thought he is still looking after our changes. I hope he still
is :)
> I would like to step up as co-maintainer to provide additional checks
> and balances and prevent the driver from degenerating into an Intel-only
> show. A number of things really irk me:
I don't have anything against this but at the same time I'm afraid it
might lead to a situation where the Thunderbolt driver evolution gets
stopped into its tracks because of unnecessary fighting over each patch
and change which does not benefit Linux kernel in general.
> * I've been contributing to this driver as well as Thunderbolt-related
> bits to the EFI, GPU and PCI subsystems for close to three years and
> was explicitly asked by Intel to cc them on every Thunderbolt-related
> patch. Yet Intel did not see fit to cc me on their changes that went
> into 4.17. I literally only learned about their existence from
> reading the news. In the 4.19 cycle I was only cc'ed on a subset of
> their patches.
>
> * Intel's efforts have been focussed exclusively on firmware-controlled
> tunnel management (ICM). They made no contributions to OS-controlled
> tunnel management. ICM cannot be used on Macs with Thunderbolt 1 + 2.
> ICM requires trusting a firmware blob. ICM does not offer the same
> versatility as OS-controlled tunnel management, e.g. using separate
> tunnels to afford different QoS levels or correlation of Thunderbolt
> ports with PCI devices. Apple chose OS-controlled tunnel management
> for very valid technical reasons.
>
> * Our OS-controlled tunnel management still lacks important features
> such as daisy-chaining and DP tunnels. Each feature needs to be
> reverse-engineered because there is no public spec. Intel issued a
> press statement in May 2017 promising to make the specification public
> "next year". More than a year has passed -- no spec. The company has
> since changed leadership, who knows if they haven't silently canned
> the plans for a public spec? I offered to sign an NDA and go through
> a disclosure process for every patch -- no reaction.
>
> * Reverse-engineering requires verbose logging so that we're able to
> collect data on various systems and endpoint devices to deduce the
> meaning of registers. Yet Intel now seeks to mute log output, thereby
> curbing our reverse-engineering efforts. This exemplifies a worrying
> tendency to ignore the needs of non-Intel stakeholders in the
> developer community or even undermine them.
The reason for making the driver less verbose comes from direct feedback
from the community. For example:
https://lkml.org/lkml/2017/10/31/864
The goal is certainly not to prevent reverse-engineering. It simply logs
too much with information that is not usable to the end user. For an OS
such as Linux where drivers tend to be high-quality professional work,
logging like this is simply not acceptable.
I also concluded that all the possible logs you are interested (this
includes Thunderbolt generation 1 and 2 devices) are pretty much already
available to you. Those devices do not appear in any modern designs,
including Apple platforms who use generation 3 devices and those are
already supported by the current driver making the logs irrelevant.
Andreas who did the hard work of reverse-engineering the initial driver and
who I still consider the authorative maintainer was fine at the time to
silence the driver.
> * Recent Intel contributions are maintainer self-commits without any
> Reviewed-by tags, which is generally considered a bad practice.
> Review comments offered by Intel-outsiders are not taken seriously.
> For example the driver's initcall level has been fiddled with twice
> now. A review comment pointing out the fragility of abusing initcall
> levels to implement dependencies and suggesting the use of a notifier
> chain instead was summarily dismissed as unnecessary unless it breaks
> a third time.
[Adding more context for Greg first]
I noticed that if the driver is built into the kernel image, it may
start DMA before IOMMUs get initialized. This makes it to fail once
IOMMUs are up and running. I originally moved the driver to load
before the networking driver (they were using both module_initcall())
but that turned out causing problems with IOMMUs so I moved the
initcall level so that initialization happens after IOMMUs but before
network, and hopefully in future other drivers. Because this is
problem of ordering of module init code, we cannot use -EPROBE_DEFER
or device_links. The patch in question is here:
https://lkml.org/lkml/2018/9/5/248
I mentioned during the review that the proposal you are suggesting
(adding blocking notifiers) is simply too complex solution. Other
drivers more or less use initcall levels to make sure the bus core gets
initialized before any drivers using that bus can be loaded. With
notifiers you would need to implement a proper API that drivers can hook
into which becomes complex and adds many lines of additional code that
needs to be maintained. As the guy who maintains this, I prefer the
simpler solution.
> Signed-off-by: Lukas Wunner <[email protected]>
> ---
> MAINTAINERS | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a5b256b25905..8815f4639e58 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -14445,6 +14445,7 @@ F: drivers/platform/x86/thinkpad_acpi.c
>
> THUNDERBOLT DRIVER
> M: Andreas Noever <[email protected]>
> +M: Lukas Wunner <[email protected]>
> M: Michael Jamet <[email protected]>
> M: Mika Westerberg <[email protected]>
> M: Yehezkel Bernat <[email protected]>
> --
> 2.18.0
Hi Lukas,
On Sun, Sep 09, 2018 at 11:42:01PM +0200, Lukas Wunner wrote:
> Ideas what we can do with correlation:
>
> * Represent the relationship between PCI devices and Thunderbolt ports
> with symlinks in sysfs.
I wonder is that really useful? I don't think we should be adding sysfs
entries without any real reason why it would be needed and who would be
using them.
> * Thunderbolt controllers up to revision 1 of Cactus Ridge 4C have to
> use INTx because MSI signaling is broken. This results in hotplug
> ports sharing interrupts with other devices and, when daisy-chaining
> multiple affected Thunderbolt controllers, can lead to extremely
> unbalanced interrupt usage. To avoid this we could prefer downstream
> ports for tunnel establishment which do not share interrupts (based
> on the nr_actions field of the correlated PCI device's irq_desc).
>
> * Alternatively, we could use non-working MSI signaling on affected
> controllers and synthesize an interrupt whenever a tunnel is
> established or goes down on unplug.
Problem I see with this patch as it stands is that you add 200+ lines of
code into the driver that is not being used by anything as far as I
understand it.
On Sun, Sep 09, 2018 at 11:42:01PM +0200, Lukas Wunner wrote:
> +struct tb_drom_entry_pci {
> + /* BYTES 0-1 */
> + struct tb_drom_entry_header header;
> + /* BYTE 2 */
> + u8 unknown:5;
> + u8 slot:3;
> + /* BYTES 3-10 are only present on PCIe upstream ports */
> +} __packed;
No need for __packed unless you absolutely are certain the compiler does
not do the right thing. When I submitted the network driver, David
Miller explained this to me and I ended up dropping those.
Also use of bitfields is something we should avoid when touching
hardware/firmware records because compiler here can do all sorts of
tricks.
I know the driver is currently using quite many of them but I think it
is good for the new code not to include them anymore.
On Mon, Sep 10, 2018 at 12:33:33PM +0300, Mika Westerberg wrote:
> The reason for making the driver less verbose comes from direct feedback
> from the community. For example:
>
> https://lkml.org/lkml/2017/10/31/864
I am not opposed to muting messages to KERN_DEBUG severity which merely
report what the driver is doing such as:
"control channel created"
"control channel starting..."
However messages should NOT be muted which report register contents
or register changes unless those registers are *fully* documented and
register changes are known to work *reliably*. The URL you're referring
to above provides an example where that's not the case:
"disabling interrupt at register 0x38200 bit 12 (0xffffffff -> 0xffffefff)"
Something is broken here, the register was read as "all ones".
This doesn't seem to work as reliable as it should and in that case
please don't mute the message until we know it's fixed and always
works.
Also, it is quite customary and serves a useful purpose to report
devices at KERN_INFO severity as they're enumerated. E.g. the PCI
bus logs messages for each enumerated device, pciehp logs the
port's capabilities on probe, and so on. Therefore please do not
mute the enumeration of switches and their ports. If you find the
messages too noisy, feel free to condense the data reported for each
port to 1 or 2 lines.
We currently print "Port ..." on enumeration, but use the syntax
"<route string>:<port number>" for other port-related messages
printed with tb_port_*(). It might be beneficial to use a single
syntax consistently.
In one of the patches I've posted yesterday, I'm logging unknown
data that is stashed in the DROM for PCI ports. To make sense of
that data, at least one line for each port needs to be logged.
(Such that we know which data pertains to which port.)
I don't really like spamming the log with this unknown data that
needs to be reverse-engineered. I hate it. You would do everyone
a favor if you could divulge the meaning of such unknown registers.
Then we don't need to log it in the first place.
Thanks,
Lukas
On Mon, Sep 10, 2018 at 12:25:14PM +0200, Lukas Wunner wrote:
> On Mon, Sep 10, 2018 at 12:33:33PM +0300, Mika Westerberg wrote:
> > The reason for making the driver less verbose comes from direct feedback
> > from the community. For example:
> >
> > https://lkml.org/lkml/2017/10/31/864
>
> I am not opposed to muting messages to KERN_DEBUG severity which merely
> report what the driver is doing such as:
>
> "control channel created"
> "control channel starting..."
>
> However messages should NOT be muted which report register contents
> or register changes unless those registers are *fully* documented and
> register changes are known to work *reliably*. The URL you're referring
> to above provides an example where that's not the case:
>
> "disabling interrupt at register 0x38200 bit 12 (0xffffffff -> 0xffffefff)"
>
> Something is broken here, the register was read as "all ones".
> This doesn't seem to work as reliable as it should and in that case
> please don't mute the message until we know it's fixed and always
> works.
That is actually already on my todo list. What happens here is that on
PCs the controller is hot-removed and the driver is removed but it tries
to touch hardware after it is already gone. We have a flag for that
already so I'm hoping to use that and skip touching hardware if it is
set.
> Also, it is quite customary and serves a useful purpose to report
> devices at KERN_INFO severity as they're enumerated. E.g. the PCI
> bus logs messages for each enumerated device, pciehp logs the
> port's capabilities on probe, and so on. Therefore please do not
> mute the enumeration of switches and their ports. If you find the
> messages too noisy, feel free to condense the data reported for each
> port to 1 or 2 lines.
That I realized also myself after testing the patch bit more. We could
print some information that is useful upon device connect/disconnect.
But I don't think dumping the switch structure and ports are useful. I
would rather make it to follow USB and print the actual device that was
connected (this is the information read from DROM and which is exported
in sysfs as well).
So I'm experimenting patch that does something like this when device is
connected:
thunderbolt 0-1: device 1:8003 connected
thunderbolt 0-1: Apple, Inc Thunderbolt to FireWire Adapter
and when it is disconnected
thunderbolt 0-1: device disconnected
> We currently print "Port ..." on enumeration, but use the syntax
> "<route string>:<port number>" for other port-related messages
> printed with tb_port_*(). It might be beneficial to use a single
> syntax consistently.
Since thunderbolt is now bus and all the devices include embedded struct
device we can use dev_*() just like everybody else is doing.
On Mon, Sep 10, 2018 at 12:33:33PM +0300, Mika Westerberg wrote:
> Hi Lukas,
>
> I'm including Greg here in case I've done something wrong as a maintainer.
> Since I've only maintained Thunderbolt quite short time, it may be that
> I've done mistakes but certainly I did not deliberately try to make life of
> people developing this for older Apple systems harder.
Greg did not yell at me (yet) so I guess I'm doing OK :)
> On Sun, Sep 09, 2018 at 11:42:01PM +0200, Lukas Wunner wrote:
> > Andreas Noever has let it be known off-list already a while ago that he
> > currently cannot spare as much time for Thunderbolt development as he'd
> > like. As a result the driver's development has become dominated by
> > Intel.
>
> I was not aware of this. Althought Andreas has not commented much
> lately, I thought he is still looking after our changes. I hope he still
> is :)
>
> > I would like to step up as co-maintainer to provide additional checks
> > and balances and prevent the driver from degenerating into an Intel-only
> > show. A number of things really irk me:
>
> I don't have anything against this but at the same time I'm afraid it
> might lead to a situation where the Thunderbolt driver evolution gets
> stopped into its tracks because of unnecessary fighting over each patch
> and change which does not benefit Linux kernel in general.
I think we have enough maintainers in this subsystem:
Andreas - Apple hardware
Michael and me - Intel
Yehezkel - Microsoft
But I think we can make you a dedicated reviewer. This should make sure
you get to review all the patches touching this subsystem.
However, first I would like to get confirmation from Andreas that he
approves this.
I also would like that anyone submitting patches to this subsystem do
not get bad feelings during the review but instead possible issues and
improvement suggestions are written in such way that the submitter feels
his work is valued (even if not always correct).
This is especially important when a random Intel (well, or Apple)
engineer submits a patch, say fixing a typo in a comment of some data
structure. There is no point starting to demand that the specific
register meaning needs to be disclosed. I've said this before but I or
any other Intel engineer do not have any power over when the spec is
released or any other related matter (like disclosing registers) and I
really don't want that every single patch review starts with demanding
people to disclose something extra. After all they are just trying to
improve the driver which is good for Linux.
If Andreas approves this, please send a patch adding you as a reviewer
and I'll apply it. Just please write the changelog in good will without
any personal frustration as those will be recorded forever in the kernel
history and you never know in future who you are working for.
From my point of view, I welcome you on board as good reviewer :)
On Mon, Sep 10, 2018 at 12:45 PM Mika Westerberg
<[email protected]> wrote:
>
> Hi Lukas,
>
> On Sun, Sep 09, 2018 at 11:42:01PM +0200, Lukas Wunner wrote:
> > Ideas what we can do with correlation:
> >
> > * Represent the relationship between PCI devices and Thunderbolt ports
> > with symlinks in sysfs.
>
> I wonder is that really useful? I don't think we should be adding sysfs
> entries without any real reason why it would be needed and who would be
> using them.
I think Lukas mentioned where it can be useful, even if it isn't used right now.
We also know this can be useful for some QoS configurations (even if we didn't
found it useful enough for now).
Lukas,
What I'd like to ask is: if your point is mainly for OS-controlled tunneling
management, I'd hope this future Connection Manager will have better ways to
determine the assigned PCI bus number of the PCI devices it tunnels than the
walk over the PCI tree the current patch does. Isn't it?
On Thu, Sep 13, 2018 at 11:58:26AM +0300, Mika Westerberg wrote:
> On Mon, Sep 10, 2018 at 12:33:33PM +0300, Mika Westerberg wrote:
> > Hi Lukas,
> >
> > I'm including Greg here in case I've done something wrong as a maintainer.
> > Since I've only maintained Thunderbolt quite short time, it may be that
> > I've done mistakes but certainly I did not deliberately try to make life of
> > people developing this for older Apple systems harder.
>
> Greg did not yell at me (yet) so I guess I'm doing OK :)
I have not seen anything to complain about here, so why would I? :)
> > On Sun, Sep 09, 2018 at 11:42:01PM +0200, Lukas Wunner wrote:
> > > Andreas Noever has let it be known off-list already a while ago that he
> > > currently cannot spare as much time for Thunderbolt development as he'd
> > > like. As a result the driver's development has become dominated by
> > > Intel.
> >
> > I was not aware of this. Althought Andreas has not commented much
> > lately, I thought he is still looking after our changes. I hope he still
> > is :)
> >
> > > I would like to step up as co-maintainer to provide additional checks
> > > and balances and prevent the driver from degenerating into an Intel-only
> > > show. A number of things really irk me:
> >
> > I don't have anything against this but at the same time I'm afraid it
> > might lead to a situation where the Thunderbolt driver evolution gets
> > stopped into its tracks because of unnecessary fighting over each patch
> > and change which does not benefit Linux kernel in general.
>
> I think we have enough maintainers in this subsystem:
>
> Andreas - Apple hardware
> Michael and me - Intel
> Yehezkel - Microsoft
>
> But I think we can make you a dedicated reviewer. This should make sure
> you get to review all the patches touching this subsystem.
That would be good. But always remember, no one, not even a maintainer,
is there as a "gatekeeper". Everyone can be routed around, or
accidentally forgotten to be cc:ed or patches can come in from other
subsystems as needed.
So adding more reviewers is always great, but it's never a "requirement"
that all people have to review everything before things are merged.
> This is especially important when a random Intel (well, or Apple)
> engineer submits a patch, say fixing a typo in a comment of some data
> structure. There is no point starting to demand that the specific
> register meaning needs to be disclosed. I've said this before but I or
> any other Intel engineer do not have any power over when the spec is
> released or any other related matter (like disclosing registers) and I
> really don't want that every single patch review starts with demanding
> people to disclose something extra. After all they are just trying to
> improve the driver which is good for Linux.
Well said, this is a tough area when dealing with both undocumented
hardware combined with a spec that is not public. Everyone's trying to
do their best so we should always assume that first.
thanks,
greg k-h
On Thu, Sep 13, 2018 at 12:43:03PM +0300, Yehezkel Bernat wrote:
> On Mon, Sep 10, 2018 at 12:45 PM Mika Westerberg
> <[email protected]> wrote:
> >
> > Hi Lukas,
> >
> > On Sun, Sep 09, 2018 at 11:42:01PM +0200, Lukas Wunner wrote:
> > > Ideas what we can do with correlation:
> > >
> > > * Represent the relationship between PCI devices and Thunderbolt ports
> > > with symlinks in sysfs.
> >
> > I wonder is that really useful? I don't think we should be adding sysfs
> > entries without any real reason why it would be needed and who would be
> > using them.
>
> I think Lukas mentioned where it can be useful, even if it isn't used right now.
> We also know this can be useful for some QoS configurations (even if we didn't
> found it useful enough for now).
I have no doubts that is not useful :) The issue is that currently it
does not do anything except adds this functionality to the driver but
nobody uses it. Like in the other parts of the kernel, let's merge this
at the same time when there is a legitimate user for the feature.
On Thu, Sep 13, 2018 at 11:00 AM Mika Westerberg
<[email protected]> wrote:
>
> On Mon, Sep 10, 2018 at 12:33:33PM +0300, Mika Westerberg wrote:
> > Hi Lukas,
> >
> > I'm including Greg here in case I've done something wrong as a maintainer.
> > Since I've only maintained Thunderbolt quite short time, it may be that
> > I've done mistakes but certainly I did not deliberately try to make life of
> > people developing this for older Apple systems harder.
>
> Greg did not yell at me (yet) so I guess I'm doing OK :)
>
> > On Sun, Sep 09, 2018 at 11:42:01PM +0200, Lukas Wunner wrote:
> > > Andreas Noever has let it be known off-list already a while ago that he
> > > currently cannot spare as much time for Thunderbolt development as he'd
> > > like. As a result the driver's development has become dominated by
> > > Intel.
> >
> > I was not aware of this. Althought Andreas has not commented much
> > lately, I thought he is still looking after our changes. I hope he still
> > is :)
> >
> > > I would like to step up as co-maintainer to provide additional checks
> > > and balances and prevent the driver from degenerating into an Intel-only
> > > show. A number of things really irk me:
> >
> > I don't have anything against this but at the same time I'm afraid it
> > might lead to a situation where the Thunderbolt driver evolution gets
> > stopped into its tracks because of unnecessary fighting over each patch
> > and change which does not benefit Linux kernel in general.
>
> I think we have enough maintainers in this subsystem:
As mentioned by Lukas I'm currently quite preoccupied with other commitments
and have not done much more than skimming the incoming patches (especially
those touching the Intel part - as I trust you know what you are doing there :))
If there is concern over too many maintainers then please give my spot to Lukas.
Since I no longer use my MacBook regularly my ability to effectively
test stuff is
quite limited anyways.
Thanks,
Andreas
> Andreas - Apple hardware
> Michael and me - Intel
> Yehezkel - Microsoft
>
> But I think we can make you a dedicated reviewer. This should make sure
> you get to review all the patches touching this subsystem.
>
> However, first I would like to get confirmation from Andreas that he
> approves this.
Signed-off-by: Andreas Noever <[email protected]>
>
> I also would like that anyone submitting patches to this subsystem do
> not get bad feelings during the review but instead possible issues and
> improvement suggestions are written in such way that the submitter feels
> his work is valued (even if not always correct).
>
> This is especially important when a random Intel (well, or Apple)
> engineer submits a patch, say fixing a typo in a comment of some data
> structure. There is no point starting to demand that the specific
> register meaning needs to be disclosed. I've said this before but I or
> any other Intel engineer do not have any power over when the spec is
> released or any other related matter (like disclosing registers) and I
> really don't want that every single patch review starts with demanding
> people to disclose something extra. After all they are just trying to
> improve the driver which is good for Linux.
>
> If Andreas approves this, please send a patch adding you as a reviewer
> and I'll apply it. Just please write the changelog in good will without
> any personal frustration as those will be recorded forever in the kernel
> history and you never know in future who you are working for.
>
> From my point of view, I welcome you on board as good reviewer :)
On Tue, Sep 18, 2018 at 12:34:16AM +0200, Andreas Noever wrote:
> On Thu, Sep 13, 2018 at 11:00 AM Mika Westerberg
> <[email protected]> wrote:
> >
> > On Mon, Sep 10, 2018 at 12:33:33PM +0300, Mika Westerberg wrote:
> > > Hi Lukas,
> > >
> > > I'm including Greg here in case I've done something wrong as a maintainer.
> > > Since I've only maintained Thunderbolt quite short time, it may be that
> > > I've done mistakes but certainly I did not deliberately try to make life of
> > > people developing this for older Apple systems harder.
> >
> > Greg did not yell at me (yet) so I guess I'm doing OK :)
> >
> > > On Sun, Sep 09, 2018 at 11:42:01PM +0200, Lukas Wunner wrote:
> > > > Andreas Noever has let it be known off-list already a while ago that he
> > > > currently cannot spare as much time for Thunderbolt development as he'd
> > > > like. As a result the driver's development has become dominated by
> > > > Intel.
> > >
> > > I was not aware of this. Althought Andreas has not commented much
> > > lately, I thought he is still looking after our changes. I hope he still
> > > is :)
> > >
> > > > I would like to step up as co-maintainer to provide additional checks
> > > > and balances and prevent the driver from degenerating into an Intel-only
> > > > show. A number of things really irk me:
> > >
> > > I don't have anything against this but at the same time I'm afraid it
> > > might lead to a situation where the Thunderbolt driver evolution gets
> > > stopped into its tracks because of unnecessary fighting over each patch
> > > and change which does not benefit Linux kernel in general.
> >
> > I think we have enough maintainers in this subsystem:
> As mentioned by Lukas I'm currently quite preoccupied with other commitments
> and have not done much more than skimming the incoming patches (especially
> those touching the Intel part - as I trust you know what you are doing there :))
> If there is concern over too many maintainers then please give my spot to Lukas.
> Since I no longer use my MacBook regularly my ability to effectively
> test stuff is
> quite limited anyways.
>
> Thanks,
> Andreas
>
>
> > Andreas - Apple hardware
> > Michael and me - Intel
> > Yehezkel - Microsoft
> >
> > But I think we can make you a dedicated reviewer. This should make sure
> > you get to review all the patches touching this subsystem.
> >
> > However, first I would like to get confirmation from Andreas that he
> > approves this.
> Signed-off-by: Andreas Noever <[email protected]>
Thanks for the comments Andreas! Then either way is fine by me :)