* [PATCH 0/3] thunderbolt: Miscellaneous cleanups
@ 2018-09-03 13:33 Mika Westerberg
2018-09-03 13:33 ` [PATCH 1/3] thunderbolt: Make the driver less verbose Mika Westerberg
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Mika Westerberg @ 2018-09-03 13:33 UTC (permalink / raw)
To: linux-kernel
Cc: Andreas Noever, Michael Jamet, Mika Westerberg, Yehezkel Bernat,
Lukas Wunner
Hi,
Here are a couple of changes which have been in my TODO list some time
already but have not had time to make a proper patch out of them until now.
Mika Westerberg (3):
thunderbolt: Make the driver less verbose
thunderbolt: Convert rest of the driver files to use SPDX identifier
thunderbolt: Add Intel as copyright holder
drivers/thunderbolt/cap.c | 3 +-
drivers/thunderbolt/ctl.c | 9 ++---
drivers/thunderbolt/ctl.h | 3 +-
drivers/thunderbolt/dma_port.c | 5 +--
drivers/thunderbolt/dma_port.h | 5 +--
drivers/thunderbolt/domain.c | 7 ++--
drivers/thunderbolt/eeprom.c | 5 +--
drivers/thunderbolt/icm.c | 5 +--
drivers/thunderbolt/nhi.c | 33 ++++++++++---------
drivers/thunderbolt/nhi.h | 3 +-
drivers/thunderbolt/nhi_regs.h | 1 +
drivers/thunderbolt/path.c | 26 +++++++--------
drivers/thunderbolt/property.c | 5 +--
drivers/thunderbolt/switch.c | 60 ++++++++++++++++------------------
drivers/thunderbolt/tb.c | 10 +++---
drivers/thunderbolt/tb.h | 9 +++--
drivers/thunderbolt/tb_msgs.h | 5 +--
drivers/thunderbolt/tb_regs.h | 3 +-
drivers/thunderbolt/xdomain.c | 5 +--
include/linux/thunderbolt.h | 5 +--
20 files changed, 96 insertions(+), 111 deletions(-)
--
2.18.0
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH 1/3] thunderbolt: Make the driver less verbose 2018-09-03 13:33 [PATCH 0/3] thunderbolt: Miscellaneous cleanups Mika Westerberg @ 2018-09-03 13:33 ` Mika Westerberg 2018-09-05 9:05 ` Lukas Wunner 2018-09-03 13:33 ` [PATCH 2/3] thunderbolt: Convert rest of the driver files to use SPDX identifier Mika Westerberg 2018-09-03 13:33 ` [PATCH 3/3] thunderbolt: Add Intel as copyright holder Mika Westerberg 2 siblings, 1 reply; 10+ messages in thread From: Mika Westerberg @ 2018-09-03 13:33 UTC (permalink / raw) To: linux-kernel Cc: Andreas Noever, Michael Jamet, Mika Westerberg, Yehezkel Bernat, Lukas Wunner Currently the driver logs quite a lot to the system message buffer even when doing normal operations. This information is not useful for ordinary users and might even annoy some. For this reason convert most of the logs at info level to happen at debug level instead. The nice output formatting is untouched. Logging can be easily re-enabled by passing "thunderbolt.dyndbg" in the kernel command line (or using the corresponding control file runtime). Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> --- drivers/thunderbolt/ctl.c | 6 ++-- drivers/thunderbolt/eeprom.c | 2 +- drivers/thunderbolt/nhi.c | 30 +++++++++---------- drivers/thunderbolt/path.c | 26 ++++++++-------- drivers/thunderbolt/switch.c | 57 +++++++++++++++++------------------- drivers/thunderbolt/tb.c | 10 +++---- drivers/thunderbolt/tb.h | 6 ++-- 7 files changed, 68 insertions(+), 69 deletions(-) diff --git a/drivers/thunderbolt/ctl.c b/drivers/thunderbolt/ctl.c index 37a7f4c735d0..2ab6033a0a3d 100644 --- a/drivers/thunderbolt/ctl.c +++ b/drivers/thunderbolt/ctl.c @@ -631,7 +631,7 @@ struct tb_ctl *tb_ctl_alloc(struct tb_nhi *nhi, event_cb cb, void *cb_data) ctl->rx_packets[i]->frame.callback = tb_ctl_rx_callback; } - tb_ctl_info(ctl, "control channel created\n"); + tb_ctl_dbg(ctl, "control channel created\n"); return ctl; err: tb_ctl_free(ctl); @@ -673,7 +673,7 @@ void tb_ctl_free(struct tb_ctl *ctl) void tb_ctl_start(struct tb_ctl *ctl) { int i; - tb_ctl_info(ctl, "control channel starting...\n"); + tb_ctl_dbg(ctl, "control channel starting...\n"); tb_ring_start(ctl->tx); /* is used to ack hotplug packets, start first */ tb_ring_start(ctl->rx); for (i = 0; i < TB_CTL_RX_PKG_COUNT; i++) @@ -702,7 +702,7 @@ void tb_ctl_stop(struct tb_ctl *ctl) 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"); + tb_ctl_dbg(ctl, "control channel stopped\n"); } /* public interface, commands */ diff --git a/drivers/thunderbolt/eeprom.c b/drivers/thunderbolt/eeprom.c index 3e8caf22c294..d2dd40390783 100644 --- a/drivers/thunderbolt/eeprom.c +++ b/drivers/thunderbolt/eeprom.c @@ -540,7 +540,7 @@ int tb_drom_read(struct tb_switch *sw) return res; size &= 0x3ff; size += TB_DROM_DATA_START; - tb_sw_info(sw, "reading drom (length: %#x)\n", size); + tb_sw_dbg(sw, "reading drom (length: %#x)\n", size); if (size < sizeof(*header)) { tb_sw_warn(sw, "drom too small, aborting\n"); return -EIO; diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c index 5cd6bdfa068f..26d34b76a88f 100644 --- a/drivers/thunderbolt/nhi.c +++ b/drivers/thunderbolt/nhi.c @@ -95,9 +95,9 @@ static void ring_interrupt_active(struct tb_ring *ring, bool active) else new = old & ~mask; - dev_info(&ring->nhi->pdev->dev, - "%s interrupt at register %#x bit %d (%#x -> %#x)\n", - active ? "enabling" : "disabling", reg, bit, old, new); + dev_dbg(&ring->nhi->pdev->dev, + "%s interrupt at register %#x bit %d (%#x -> %#x)\n", + active ? "enabling" : "disabling", reg, bit, old, new); if (new == old) dev_WARN(&ring->nhi->pdev->dev, @@ -476,8 +476,9 @@ static struct tb_ring *tb_ring_alloc(struct tb_nhi *nhi, u32 hop, int size, void *poll_data) { struct tb_ring *ring = NULL; - dev_info(&nhi->pdev->dev, "allocating %s ring %d of size %d\n", - transmit ? "TX" : "RX", hop, size); + + dev_dbg(&nhi->pdev->dev, "allocating %s ring %d of size %d\n", + transmit ? "TX" : "RX", hop, size); /* Tx Ring 2 is reserved for E2E workaround */ if (transmit && hop == RING_E2E_UNUSED_HOPID) @@ -585,8 +586,8 @@ void tb_ring_start(struct tb_ring *ring) dev_WARN(&ring->nhi->pdev->dev, "ring already started\n"); goto err; } - dev_info(&ring->nhi->pdev->dev, "starting %s %d\n", - RING_TYPE(ring), ring->hop); + dev_dbg(&ring->nhi->pdev->dev, "starting %s %d\n", + RING_TYPE(ring), ring->hop); if (ring->flags & RING_FLAG_FRAME) { /* Means 4096 */ @@ -647,8 +648,8 @@ void tb_ring_stop(struct tb_ring *ring) { spin_lock_irq(&ring->nhi->lock); spin_lock(&ring->lock); - dev_info(&ring->nhi->pdev->dev, "stopping %s %d\n", - RING_TYPE(ring), ring->hop); + dev_dbg(&ring->nhi->pdev->dev, "stopping %s %d\n", + RING_TYPE(ring), ring->hop); if (ring->nhi->going_away) goto err; if (!ring->running) { @@ -716,10 +717,8 @@ void tb_ring_free(struct tb_ring *ring) ring->descriptors_dma = 0; - dev_info(&ring->nhi->pdev->dev, - "freeing %s %d\n", - RING_TYPE(ring), - ring->hop); + dev_dbg(&ring->nhi->pdev->dev, "freeing %s %d\n", RING_TYPE(ring), + ring->hop); /** * ring->work can no longer be scheduled (it is scheduled only @@ -931,7 +930,8 @@ static int nhi_runtime_resume(struct device *dev) static void nhi_shutdown(struct tb_nhi *nhi) { int i; - dev_info(&nhi->pdev->dev, "shutdown\n"); + + dev_dbg(&nhi->pdev->dev, "shutdown\n"); for (i = 0; i < nhi->hop_count; i++) { if (nhi->tx_rings[i]) @@ -1059,7 +1059,7 @@ static int nhi_probe(struct pci_dev *pdev, const struct pci_device_id *id) return -ENODEV; } - dev_info(&nhi->pdev->dev, "NHI initialized, starting thunderbolt\n"); + dev_dbg(&nhi->pdev->dev, "NHI initialized, starting thunderbolt\n"); res = tb_domain_add(tb); if (res) { diff --git a/drivers/thunderbolt/path.c b/drivers/thunderbolt/path.c index ff49ad880bfd..a11956522bac 100644 --- a/drivers/thunderbolt/path.c +++ b/drivers/thunderbolt/path.c @@ -13,19 +13,19 @@ static void tb_dump_hop(struct tb_port *port, struct tb_regs_hop *hop) { - tb_port_info(port, " Hop through port %d to hop %d (%s)\n", - hop->out_port, hop->next_hop, - hop->enable ? "enabled" : "disabled"); - tb_port_info(port, " Weight: %d Priority: %d Credits: %d Drop: %d\n", - hop->weight, hop->priority, - hop->initial_credits, hop->drop_packages); - tb_port_info(port, " Counter enabled: %d Counter index: %d\n", - hop->counter_enable, hop->counter); - tb_port_info(port, " Flow Control (In/Eg): %d/%d Shared Buffer (In/Eg): %d/%d\n", - hop->ingress_fc, hop->egress_fc, - hop->ingress_shared_buffer, hop->egress_shared_buffer); - tb_port_info(port, " Unknown1: %#x Unknown2: %#x Unknown3: %#x\n", - hop->unknown1, hop->unknown2, hop->unknown3); + tb_port_dbg(port, " Hop through port %d to hop %d (%s)\n", + hop->out_port, hop->next_hop, + hop->enable ? "enabled" : "disabled"); + tb_port_dbg(port, " Weight: %d Priority: %d Credits: %d Drop: %d\n", + hop->weight, hop->priority, + hop->initial_credits, hop->drop_packages); + tb_port_dbg(port, " Counter enabled: %d Counter index: %d\n", + hop->counter_enable, hop->counter); + tb_port_dbg(port, " Flow Control (In/Eg): %d/%d Shared Buffer (In/Eg): %d/%d\n", + hop->ingress_fc, hop->egress_fc, + hop->ingress_shared_buffer, hop->egress_shared_buffer); + tb_port_dbg(port, " Unknown1: %#x Unknown2: %#x Unknown3: %#x\n", + hop->unknown1, hop->unknown2, hop->unknown3); } /** diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c index 7442bc4c6433..81134ee29578 100644 --- a/drivers/thunderbolt/switch.c +++ b/drivers/thunderbolt/switch.c @@ -436,15 +436,15 @@ static const char *tb_port_type(struct tb_regs_port_header *port) static void tb_dump_port(struct tb *tb, struct tb_regs_port_header *port) { - tb_info(tb, - " Port %d: %x:%x (Revision: %d, TB Version: %d, Type: %s (%#x))\n", - port->port_number, port->vendor_id, port->device_id, - port->revision, port->thunderbolt_version, tb_port_type(port), - port->type); - tb_info(tb, " Max hop id (in/out): %d/%d\n", - port->max_in_hop_id, port->max_out_hop_id); - tb_info(tb, " Max counters: %d\n", port->max_counters); - tb_info(tb, " NFC Credits: %#x\n", port->nfc_credits); + tb_dbg(tb, + " Port %d: %x:%x (Revision: %d, TB Version: %d, Type: %s (%#x))\n", + port->port_number, port->vendor_id, port->device_id, + port->revision, port->thunderbolt_version, tb_port_type(port), + port->type); + tb_dbg(tb, " Max hop id (in/out): %d/%d\n", + port->max_in_hop_id, port->max_out_hop_id); + tb_dbg(tb, " Max counters: %d\n", port->max_counters); + tb_dbg(tb, " NFC Credits: %#x\n", port->nfc_credits); } /** @@ -605,20 +605,18 @@ static int tb_init_port(struct tb_port *port) static void tb_dump_switch(struct tb *tb, struct tb_regs_switch_header *sw) { - tb_info(tb, - " Switch: %x:%x (Revision: %d, TB Version: %d)\n", - sw->vendor_id, sw->device_id, sw->revision, - sw->thunderbolt_version); - tb_info(tb, " Max Port Number: %d\n", sw->max_port_number); - tb_info(tb, " Config:\n"); - tb_info(tb, + tb_dbg(tb, " Switch: %x:%x (Revision: %d, TB Version: %d)\n", + sw->vendor_id, sw->device_id, sw->revision, + sw->thunderbolt_version); + tb_dbg(tb, " Max Port Number: %d\n", sw->max_port_number); + tb_dbg(tb, " Config:\n"); + tb_dbg(tb, " Upstream Port Number: %d Depth: %d Route String: %#llx Enabled: %d, PlugEventsDelay: %dms\n", - sw->upstream_port_number, sw->depth, - (((u64) sw->route_hi) << 32) | sw->route_lo, - sw->enabled, sw->plug_events_delay); - tb_info(tb, - " unknown1: %#x unknown4: %#x\n", - sw->__unknown1, sw->__unknown4); + sw->upstream_port_number, sw->depth, + (((u64) sw->route_hi) << 32) | sw->route_lo, + sw->enabled, sw->plug_events_delay); + tb_dbg(tb, " unknown1: %#x unknown4: %#x\n", + sw->__unknown1, sw->__unknown4); } /** @@ -634,7 +632,7 @@ int tb_switch_reset(struct tb *tb, u64 route) header.route_lo = route, header.enabled = true, }; - tb_info(tb, "resetting switch at %llx\n", route); + tb_dbg(tb, "resetting switch at %llx\n", route); res.err = tb_cfg_write(tb->ctl, ((u32 *) &header) + 2, route, 0, 2, 2, 2); if (res.err) @@ -1139,7 +1137,7 @@ struct tb_switch *tb_switch_alloc(struct tb *tb, struct device *parent, if (tb_cfg_read(tb->ctl, &sw->config, route, 0, TB_CFG_SWITCH, 0, 5)) goto err_free_sw_ports; - tb_info(tb, "current switch config:\n"); + tb_dbg(tb, "current switch config:\n"); tb_dump_switch(tb, &sw->config); /* configure switch */ @@ -1246,9 +1244,8 @@ int tb_switch_configure(struct tb_switch *sw) int ret; route = tb_route(sw); - tb_info(tb, - "initializing Switch at %#llx (depth: %d, up port: %d)\n", - route, tb_route_length(route), sw->config.upstream_port_number); + tb_dbg(tb, "initializing Switch at %#llx (depth: %d, up port: %d)\n", + route, tb_route_length(route), sw->config.upstream_port_number); if (sw->config.vendor_id != PCI_VENDOR_ID_INTEL) tb_sw_warn(sw, "unknown switch vendor id %#x\n", @@ -1386,13 +1383,13 @@ int tb_switch_add(struct tb_switch *sw) tb_sw_warn(sw, "tb_eeprom_read_rom failed\n"); return ret; } - tb_sw_info(sw, "uid: %#llx\n", sw->uid); + tb_sw_dbg(sw, "uid: %#llx\n", sw->uid); tb_switch_set_uuid(sw); for (i = 0; i <= sw->config.max_port_number; i++) { if (sw->ports[i].disabled) { - tb_port_info(&sw->ports[i], "disabled by eeprom\n"); + tb_port_dbg(&sw->ports[i], "disabled by eeprom\n"); continue; } ret = tb_init_port(&sw->ports[i]); @@ -1483,7 +1480,7 @@ void tb_sw_set_unplugged(struct tb_switch *sw) int tb_switch_resume(struct tb_switch *sw) { int i, err; - tb_sw_info(sw, "resuming switch\n"); + tb_sw_dbg(sw, "resuming switch\n"); /* * Check for UID of the connected switches except for root diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c index 1424581fd9af..30e02c716f6c 100644 --- a/drivers/thunderbolt/tb.c +++ b/drivers/thunderbolt/tb.c @@ -404,10 +404,10 @@ static int tb_suspend_noirq(struct tb *tb) { struct tb_cm *tcm = tb_priv(tb); - tb_info(tb, "suspending...\n"); + tb_dbg(tb, "suspending...\n"); tb_switch_suspend(tb->root_switch); tcm->hotplug_active = false; /* signal tb_handle_hotplug to quit */ - tb_info(tb, "suspend finished\n"); + tb_dbg(tb, "suspend finished\n"); return 0; } @@ -417,7 +417,7 @@ 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"); + tb_dbg(tb, "resuming...\n"); /* remove any pci devices the firmware might have setup */ tb_switch_reset(tb, 0); @@ -432,12 +432,12 @@ static int tb_resume_noirq(struct tb *tb) * the pcie links need some time to get going. * 100ms works for me... */ - tb_info(tb, "tunnels restarted, sleeping for 100ms\n"); + tb_dbg(tb, "tunnels restarted, sleeping for 100ms\n"); msleep(100); } /* Allow tb_handle_hotplug to progress events */ tcm->hotplug_active = true; - tb_info(tb, "resume finished\n"); + tb_dbg(tb, "resume finished\n"); return 0; } diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h index 5067d69d0501..5a99245573c0 100644 --- a/drivers/thunderbolt/tb.h +++ b/drivers/thunderbolt/tb.h @@ -327,7 +327,7 @@ static inline int tb_port_write(struct tb_port *port, const void *buffer, #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) #define tb_info(tb, fmt, arg...) dev_info(&(tb)->nhi->pdev->dev, fmt, ## arg) - +#define tb_dbg(tb, fmt, arg...) dev_dbg(&(tb)->nhi->pdev->dev, fmt, ## arg) #define __TB_SW_PRINT(level, sw, fmt, arg...) \ do { \ @@ -338,7 +338,7 @@ static inline int tb_port_write(struct tb_port *port, const void *buffer, #define tb_sw_WARN(sw, fmt, arg...) __TB_SW_PRINT(tb_WARN, sw, fmt, ##arg) #define tb_sw_warn(sw, fmt, arg...) __TB_SW_PRINT(tb_warn, sw, fmt, ##arg) #define tb_sw_info(sw, fmt, arg...) __TB_SW_PRINT(tb_info, sw, fmt, ##arg) - +#define tb_sw_dbg(sw, fmt, arg...) __TB_SW_PRINT(tb_dbg, sw, fmt, ##arg) #define __TB_PORT_PRINT(level, _port, fmt, arg...) \ do { \ @@ -352,6 +352,8 @@ static inline int tb_port_write(struct tb_port *port, const void *buffer, __TB_PORT_PRINT(tb_warn, port, fmt, ##arg) #define tb_port_info(port, fmt, arg...) \ __TB_PORT_PRINT(tb_info, port, fmt, ##arg) +#define tb_port_dbg(port, fmt, arg...) \ + __TB_PORT_PRINT(tb_dbg, port, fmt, ##arg) struct tb *icm_probe(struct tb_nhi *nhi); struct tb *tb_probe(struct tb_nhi *nhi); -- 2.18.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] thunderbolt: Make the driver less verbose 2018-09-03 13:33 ` [PATCH 1/3] thunderbolt: Make the driver less verbose Mika Westerberg @ 2018-09-05 9:05 ` Lukas Wunner 2018-09-05 9:54 ` Mika Westerberg 0 siblings, 1 reply; 10+ messages in thread From: Lukas Wunner @ 2018-09-05 9:05 UTC (permalink / raw) To: Mika Westerberg Cc: linux-kernel, Andreas Noever, Michael Jamet, Yehezkel Bernat On Mon, Sep 03, 2018 at 04:33:02PM +0300, Mika Westerberg wrote: > Currently the driver logs quite a lot to the system message buffer even > when doing normal operations. This information is not useful for > ordinary users and might even annoy some. No, the verbose logging is done on purpose to aid us in reverse-engineering the protocol. For example ... > - tb_port_info(port, " Unknown1: %#x Unknown2: %#x Unknown3: %#x\n", > - hop->unknown1, hop->unknown2, hop->unknown3); ... why do you think we're logging these seemingly stupid unknown bitfields? Because whenever someone posts dmesg output they inadvertantly post the contents of those unknown fields and we can then google the value of those fields on various controllers and machines and deduce their possible meaning. By muting those messages, you're taking away our reverse enginering aids without having released the spec, which would indeed obviate the need for them. Please don't do that. Release the spec, *then* you can mute the messages. Not the other way round. Thanks, Lukas ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] thunderbolt: Make the driver less verbose 2018-09-05 9:05 ` Lukas Wunner @ 2018-09-05 9:54 ` Mika Westerberg 2018-09-06 8:41 ` Lukas Wunner 0 siblings, 1 reply; 10+ messages in thread From: Mika Westerberg @ 2018-09-05 9:54 UTC (permalink / raw) To: Lukas Wunner; +Cc: linux-kernel, Andreas Noever, Michael Jamet, Yehezkel Bernat On Wed, Sep 05, 2018 at 11:05:10AM +0200, Lukas Wunner wrote: > On Mon, Sep 03, 2018 at 04:33:02PM +0300, Mika Westerberg wrote: > > Currently the driver logs quite a lot to the system message buffer even > > when doing normal operations. This information is not useful for > > ordinary users and might even annoy some. > > No, the verbose logging is done on purpose to aid us in reverse-engineering > the protocol. For example ... > > > - tb_port_info(port, " Unknown1: %#x Unknown2: %#x Unknown3: %#x\n", > > - hop->unknown1, hop->unknown2, hop->unknown3); > > ... why do you think we're logging these seemingly stupid unknown > bitfields? Because whenever someone posts dmesg output they > inadvertantly post the contents of those unknown fields and we can > then google the value of those fields on various controllers and > machines and deduce their possible meaning. And the majority of people get tons of completely useless messages filling their dmesgs? No, I don't think that's a good thing. > By muting those messages, you're taking away our reverse enginering aids > without having released the spec, which would indeed obviate the need > for them. Please don't do that. Release the spec, *then* you can > mute the messages. Not the other way round. All the possible messages are most likely logged already and available by Googling so even if we mute the driver now, you still can find those messages in the wild. Anything running on Alpine Ridge and higher does not require reverse-engineering (even on Apple systems) because those are already supported in the driver. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] thunderbolt: Make the driver less verbose 2018-09-05 9:54 ` Mika Westerberg @ 2018-09-06 8:41 ` Lukas Wunner 2018-09-06 10:47 ` Mika Westerberg 0 siblings, 1 reply; 10+ messages in thread From: Lukas Wunner @ 2018-09-06 8:41 UTC (permalink / raw) To: Mika Westerberg Cc: linux-kernel, Andreas Noever, Michael Jamet, Yehezkel Bernat On Wed, Sep 05, 2018 at 12:54:51PM +0300, Mika Westerberg wrote: > On Wed, Sep 05, 2018 at 11:05:10AM +0200, Lukas Wunner wrote: > > On Mon, Sep 03, 2018 at 04:33:02PM +0300, Mika Westerberg wrote: > > > Currently the driver logs quite a lot to the system message buffer even > > > when doing normal operations. This information is not useful for > > > ordinary users and might even annoy some. > > > > No, the verbose logging is done on purpose to aid us in reverse-engineering > > the protocol. For example ... > > > > > - tb_port_info(port, " Unknown1: %#x Unknown2: %#x Unknown3: %#x\n", > > > - hop->unknown1, hop->unknown2, hop->unknown3); > > > > ... why do you think we're logging these seemingly stupid unknown > > bitfields? Because whenever someone posts dmesg output they > > inadvertantly post the contents of those unknown fields and we can > > then google the value of those fields on various controllers and > > machines and deduce their possible meaning. > > And the majority of people get tons of completely useless messages > filling their dmesgs? No, I don't think that's a good thing. As you know the OS-controlled tunnel manager is far from feature complete. I think the "majority of people" are perfectly willing to accept verbose logging if it helps us fill in those gaps. You make it sound like logfiles are spammed all the time, but messages are in fact only logged on driver initialization and hotplug. We wouldn't be in this situation if we had a proper Thunderbolt spec. It wasn't *my* idea to keep it under wraps. So please leave the messages at info level so as not to hinder our work. As for your claim that the "majority of people" find the messages useless", I rather suspect that you, personally, find them useless because you complained about noisiness of the driver before: "I don't think we want to log anything with info level to be honest. The driver currently already is pretty noisy so adding even more information there just makes it worse ;-) I would rather convert debugging information to use tracepoints and get rid of the tb_*_info() things completely." https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1401181.html And guess what I responded: "The noisiness has value in that it helps with reverse-engineering: Just google for dmesg output and check what other machines are reporting for unknown registers. :-) If there was public documentation available or Intel would be okay with answering specific questions (as you've done with the 0x30 attribute id), then the value obviously diminishes." > Anything running on Alpine Ridge and higher does > not require reverse-engineering (even on Apple systems) because those > are already supported in the driver. Long term it may be worthwhile to move to OS-controlled tunnel management even when ICM-controlled tunnel management is available as the latter might not support features of the former, e.g. correlation of PCI devices with Thunderbolt ports: https://github.com/l1k/linux/commit/d024c6e7e80e Thanks, Lukas ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] thunderbolt: Make the driver less verbose 2018-09-06 8:41 ` Lukas Wunner @ 2018-09-06 10:47 ` Mika Westerberg 0 siblings, 0 replies; 10+ messages in thread From: Mika Westerberg @ 2018-09-06 10:47 UTC (permalink / raw) To: Lukas Wunner; +Cc: linux-kernel, Andreas Noever, Michael Jamet, Yehezkel Bernat On Thu, Sep 06, 2018 at 10:41:43AM +0200, Lukas Wunner wrote: > On Wed, Sep 05, 2018 at 12:54:51PM +0300, Mika Westerberg wrote: > > On Wed, Sep 05, 2018 at 11:05:10AM +0200, Lukas Wunner wrote: > > > On Mon, Sep 03, 2018 at 04:33:02PM +0300, Mika Westerberg wrote: > > > > Currently the driver logs quite a lot to the system message buffer even > > > > when doing normal operations. This information is not useful for > > > > ordinary users and might even annoy some. > > > > > > No, the verbose logging is done on purpose to aid us in reverse-engineering > > > the protocol. For example ... > > > > > > > - tb_port_info(port, " Unknown1: %#x Unknown2: %#x Unknown3: %#x\n", > > > > - hop->unknown1, hop->unknown2, hop->unknown3); > > > > > > ... why do you think we're logging these seemingly stupid unknown > > > bitfields? Because whenever someone posts dmesg output they > > > inadvertantly post the contents of those unknown fields and we can > > > then google the value of those fields on various controllers and > > > machines and deduce their possible meaning. > > > > And the majority of people get tons of completely useless messages > > filling their dmesgs? No, I don't think that's a good thing. > > As you know the OS-controlled tunnel manager is far from feature > complete. I think the "majority of people" are perfectly willing > to accept verbose logging if it helps us fill in those gaps. > > You make it sound like logfiles are spammed all the time, but > messages are in fact only logged on driver initialization and hotplug. And every time the controller runtime suspends/resumes. Every suspend/resume. Every time ring gets initialized/teared when you connect cable between computers. It is way too much for a driver that is part of production operating system. > We wouldn't be in this situation if we had a proper Thunderbolt spec. > It wasn't *my* idea to keep it under wraps. Please don't start that again. I can't affect when the spec is released, really. I'm just a poor driver guy trying to get this working as good as possible. > So please leave the messages at info level so as not to hinder our work. > > As for your claim that the "majority of people" find the messages > useless", I rather suspect that you, personally, find them useless > because you complained about noisiness of the driver before: > > "I don't think we want to log anything with info level to be honest. > The driver currently already is pretty noisy so adding even more > information there just makes it worse ;-) > I would rather convert debugging information to use tracepoints and > get rid of the tb_*_info() things completely." > https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1401181.html > > And guess what I responded: > > "The noisiness has value in that it helps with reverse-engineering: > Just google for dmesg output and check what other machines are > reporting for unknown registers. :-) > If there was public documentation available or Intel would be okay > with answering specific questions (as you've done with the 0x30 > attribute id), then the value obviously diminishes." It is not just me, see here: https://lkml.org/lkml/2017/10/31/864 And I fully agree. Andreas was fine at the time to silence the driver. I just did not have time to do that until now. Andreas, let me know if you have objections about this patch. ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/3] thunderbolt: Convert rest of the driver files to use SPDX identifier 2018-09-03 13:33 [PATCH 0/3] thunderbolt: Miscellaneous cleanups Mika Westerberg 2018-09-03 13:33 ` [PATCH 1/3] thunderbolt: Make the driver less verbose Mika Westerberg @ 2018-09-03 13:33 ` Mika Westerberg 2018-09-03 13:33 ` [PATCH 3/3] thunderbolt: Add Intel as copyright holder Mika Westerberg 2 siblings, 0 replies; 10+ messages in thread From: Mika Westerberg @ 2018-09-03 13:33 UTC (permalink / raw) To: linux-kernel Cc: Andreas Noever, Michael Jamet, Mika Westerberg, Yehezkel Bernat, Lukas Wunner This gets rid of the licence boilerplate duplicated in each file. While there fix doubled space in domain.c author line. No functional changes intended. Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> --- drivers/thunderbolt/dma_port.c | 5 +---- drivers/thunderbolt/dma_port.h | 5 +---- drivers/thunderbolt/domain.c | 7 ++----- drivers/thunderbolt/icm.c | 5 +---- drivers/thunderbolt/property.c | 5 +---- drivers/thunderbolt/tb_msgs.h | 5 +---- drivers/thunderbolt/xdomain.c | 5 +---- include/linux/thunderbolt.h | 5 +---- 8 files changed, 9 insertions(+), 33 deletions(-) diff --git a/drivers/thunderbolt/dma_port.c b/drivers/thunderbolt/dma_port.c index f2701194f810..847dd07a7b17 100644 --- a/drivers/thunderbolt/dma_port.c +++ b/drivers/thunderbolt/dma_port.c @@ -1,13 +1,10 @@ +// SPDX-License-Identifier: GPL-2.0 /* * Thunderbolt DMA configuration based mailbox support * * Copyright (C) 2017, Intel Corporation * Authors: Michael Jamet <michael.jamet@intel.com> * Mika Westerberg <mika.westerberg@linux.intel.com> - * - * 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/delay.h> diff --git a/drivers/thunderbolt/dma_port.h b/drivers/thunderbolt/dma_port.h index c4a69e0fbff7..7deadd97ce31 100644 --- a/drivers/thunderbolt/dma_port.h +++ b/drivers/thunderbolt/dma_port.h @@ -1,13 +1,10 @@ +/* SPDX-License-Identifier: GPL-2.0 */ /* * Thunderbolt DMA configuration based mailbox support * * Copyright (C) 2017, Intel Corporation * Authors: Michael Jamet <michael.jamet@intel.com> * Mika Westerberg <mika.westerberg@linux.intel.com> - * - * 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_ diff --git a/drivers/thunderbolt/domain.c b/drivers/thunderbolt/domain.c index 092381e2accf..93e562f18d40 100644 --- a/drivers/thunderbolt/domain.c +++ b/drivers/thunderbolt/domain.c @@ -1,12 +1,9 @@ +// SPDX-License-Identifier: GPL-2.0 /* * Thunderbolt bus support * * Copyright (C) 2017, Intel Corporation - * Author: Mika Westerberg <mika.westerberg@linux.intel.com> - * - * 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. + * Author: Mika Westerberg <mika.westerberg@linux.intel.com> */ #include <linux/device.h> diff --git a/drivers/thunderbolt/icm.c b/drivers/thunderbolt/icm.c index 28fc4ce75edb..e3fc920af682 100644 --- a/drivers/thunderbolt/icm.c +++ b/drivers/thunderbolt/icm.c @@ -1,3 +1,4 @@ +// SPDX-License-Identifier: GPL-2.0 /* * Internal Thunderbolt Connection Manager. This is a firmware running on * the Thunderbolt host controller performing most of the low-level @@ -6,10 +7,6 @@ * Copyright (C) 2017, Intel Corporation * Authors: Michael Jamet <michael.jamet@intel.com> * Mika Westerberg <mika.westerberg@linux.intel.com> - * - * 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/delay.h> diff --git a/drivers/thunderbolt/property.c b/drivers/thunderbolt/property.c index 8fe913a95b4a..b2f0d6386cee 100644 --- a/drivers/thunderbolt/property.c +++ b/drivers/thunderbolt/property.c @@ -1,13 +1,10 @@ +// SPDX-License-Identifier: GPL-2.0 /* * Thunderbolt XDomain property support * * Copyright (C) 2017, Intel Corporation * Authors: Michael Jamet <michael.jamet@intel.com> * Mika Westerberg <mika.westerberg@linux.intel.com> - * - * 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/err.h> diff --git a/drivers/thunderbolt/tb_msgs.h b/drivers/thunderbolt/tb_msgs.h index 2487e162c885..02c84aa3d018 100644 --- a/drivers/thunderbolt/tb_msgs.h +++ b/drivers/thunderbolt/tb_msgs.h @@ -1,12 +1,9 @@ +/* SPDX-License-Identifier: GPL-2.0 */ /* * Thunderbolt control channel messages * * Copyright (C) 2014 Andreas Noever <andreas.noever@gmail.com> * Copyright (C) 2017, Intel Corporation - * - * 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 _TB_MSGS diff --git a/drivers/thunderbolt/xdomain.c b/drivers/thunderbolt/xdomain.c index db8bece63327..e27dd8beb94b 100644 --- a/drivers/thunderbolt/xdomain.c +++ b/drivers/thunderbolt/xdomain.c @@ -1,13 +1,10 @@ +// SPDX-License-Identifier: GPL-2.0 /* * Thunderbolt XDomain discovery protocol support * * Copyright (C) 2017, Intel Corporation * Authors: Michael Jamet <michael.jamet@intel.com> * Mika Westerberg <mika.westerberg@linux.intel.com> - * - * 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> diff --git a/include/linux/thunderbolt.h b/include/linux/thunderbolt.h index a3ed26082bc1..bf6ec83e60ee 100644 --- a/include/linux/thunderbolt.h +++ b/include/linux/thunderbolt.h @@ -1,3 +1,4 @@ +/* SPDX-License-Identifier: GPL-2.0 */ /* * Thunderbolt service API * @@ -5,10 +6,6 @@ * Copyright (C) 2017, Intel Corporation * Authors: Michael Jamet <michael.jamet@intel.com> * Mika Westerberg <mika.westerberg@linux.intel.com> - * - * 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 THUNDERBOLT_H_ -- 2.18.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 3/3] thunderbolt: Add Intel as copyright holder 2018-09-03 13:33 [PATCH 0/3] thunderbolt: Miscellaneous cleanups Mika Westerberg 2018-09-03 13:33 ` [PATCH 1/3] thunderbolt: Make the driver less verbose Mika Westerberg 2018-09-03 13:33 ` [PATCH 2/3] thunderbolt: Convert rest of the driver files to use SPDX identifier Mika Westerberg @ 2018-09-03 13:33 ` Mika Westerberg 2018-09-03 20:18 ` Yehezkel Bernat 2 siblings, 1 reply; 10+ messages in thread From: Mika Westerberg @ 2018-09-03 13:33 UTC (permalink / raw) To: linux-kernel Cc: Andreas Noever, Michael Jamet, Mika Westerberg, Yehezkel Bernat, Lukas Wunner Intel has done pretty major changes to the driver and we continue to do so in the future as well. Add Intel as copyright holder of the files we have done changes. While there drop "Cactus Ridge" from the headers because this driver works also with other Thunderbolt controllers. No functional changes intended. Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> --- drivers/thunderbolt/cap.c | 3 ++- drivers/thunderbolt/ctl.c | 3 ++- drivers/thunderbolt/ctl.h | 3 ++- drivers/thunderbolt/eeprom.c | 3 ++- drivers/thunderbolt/nhi.c | 3 ++- drivers/thunderbolt/nhi.h | 3 ++- drivers/thunderbolt/nhi_regs.h | 1 + drivers/thunderbolt/switch.c | 3 ++- drivers/thunderbolt/tb.h | 3 ++- drivers/thunderbolt/tb_regs.h | 3 ++- 10 files changed, 19 insertions(+), 9 deletions(-) diff --git a/drivers/thunderbolt/cap.c b/drivers/thunderbolt/cap.c index c2277b8ee88d..9553305c63ea 100644 --- a/drivers/thunderbolt/cap.c +++ b/drivers/thunderbolt/cap.c @@ -1,8 +1,9 @@ // SPDX-License-Identifier: GPL-2.0 /* - * Thunderbolt Cactus Ridge driver - capabilities lookup + * Thunderbolt driver - capabilities lookup * * Copyright (c) 2014 Andreas Noever <andreas.noever@gmail.com> + * Copyright (C) 2018, Intel Corporation */ #include <linux/slab.h> diff --git a/drivers/thunderbolt/ctl.c b/drivers/thunderbolt/ctl.c index 2ab6033a0a3d..270fec96c603 100644 --- a/drivers/thunderbolt/ctl.c +++ b/drivers/thunderbolt/ctl.c @@ -1,8 +1,9 @@ // SPDX-License-Identifier: GPL-2.0 /* - * Thunderbolt Cactus Ridge driver - control channel and configuration commands + * Thunderbolt driver - control channel and configuration commands * * Copyright (c) 2014 Andreas Noever <andreas.noever@gmail.com> + * Copyright (C) 2018, Intel Corporation */ #include <linux/crc32.h> diff --git a/drivers/thunderbolt/ctl.h b/drivers/thunderbolt/ctl.h index 3062e0b5f71e..2f1a1e111110 100644 --- a/drivers/thunderbolt/ctl.h +++ b/drivers/thunderbolt/ctl.h @@ -1,8 +1,9 @@ /* SPDX-License-Identifier: GPL-2.0 */ /* - * Thunderbolt Cactus Ridge driver - control channel and configuration commands + * Thunderbolt driver - control channel and configuration commands * * Copyright (c) 2014 Andreas Noever <andreas.noever@gmail.com> + * Copyright (C) 2018, Intel Corporation */ #ifndef _TB_CFG diff --git a/drivers/thunderbolt/eeprom.c b/drivers/thunderbolt/eeprom.c index d2dd40390783..81e8ac4c5805 100644 --- a/drivers/thunderbolt/eeprom.c +++ b/drivers/thunderbolt/eeprom.c @@ -1,8 +1,9 @@ // SPDX-License-Identifier: GPL-2.0 /* - * Thunderbolt Cactus Ridge driver - eeprom access + * Thunderbolt driver - eeprom access * * Copyright (c) 2014 Andreas Noever <andreas.noever@gmail.com> + * Copyright (C) 2018, Intel Corporation */ #include <linux/crc32.h> diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c index 26d34b76a88f..9aa44f9762a3 100644 --- a/drivers/thunderbolt/nhi.c +++ b/drivers/thunderbolt/nhi.c @@ -1,10 +1,11 @@ /* - * Thunderbolt Cactus Ridge driver - NHI driver + * Thunderbolt driver - NHI driver * * The NHI (native host interface) is the pci device that allows us to send and * receive frames from the thunderbolt bus. * * Copyright (c) 2014 Andreas Noever <andreas.noever@gmail.com> + * Copyright (C) 2018, Intel Corporation */ #include <linux/pm_runtime.h> diff --git a/drivers/thunderbolt/nhi.h b/drivers/thunderbolt/nhi.h index 1696a4560948..1b5d47ecd3ed 100644 --- a/drivers/thunderbolt/nhi.h +++ b/drivers/thunderbolt/nhi.h @@ -1,8 +1,9 @@ /* SPDX-License-Identifier: GPL-2.0 */ /* - * Thunderbolt Cactus Ridge driver - NHI driver + * Thunderbolt driver - NHI driver * * Copyright (c) 2014 Andreas Noever <andreas.noever@gmail.com> + * Copyright (C) 2018, Intel Corporation */ #ifndef DSL3510_H_ diff --git a/drivers/thunderbolt/nhi_regs.h b/drivers/thunderbolt/nhi_regs.h index b3e49d19c01e..a60bd98c1d04 100644 --- a/drivers/thunderbolt/nhi_regs.h +++ b/drivers/thunderbolt/nhi_regs.h @@ -3,6 +3,7 @@ * Thunderbolt driver - NHI registers * * Copyright (c) 2014 Andreas Noever <andreas.noever@gmail.com> + * Copyright (C) 2018, Intel Corporation */ #ifndef NHI_REGS_H_ diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c index 81134ee29578..354ecc3b2791 100644 --- a/drivers/thunderbolt/switch.c +++ b/drivers/thunderbolt/switch.c @@ -1,8 +1,9 @@ // SPDX-License-Identifier: GPL-2.0 /* - * Thunderbolt Cactus Ridge driver - switch/port utility functions + * Thunderbolt driver - switch/port utility functions * * Copyright (c) 2014 Andreas Noever <andreas.noever@gmail.com> + * Copyright (C) 2018, Intel Corporation */ #include <linux/delay.h> diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h index 5a99245573c0..52584c4003e3 100644 --- a/drivers/thunderbolt/tb.h +++ b/drivers/thunderbolt/tb.h @@ -1,8 +1,9 @@ /* SPDX-License-Identifier: GPL-2.0 */ /* - * Thunderbolt Cactus Ridge driver - bus logic (NHI independent) + * Thunderbolt driver - bus logic (NHI independent) * * Copyright (c) 2014 Andreas Noever <andreas.noever@gmail.com> + * Copyright (C) 2018, Intel Corporation */ #ifndef TB_H_ diff --git a/drivers/thunderbolt/tb_regs.h b/drivers/thunderbolt/tb_regs.h index 693b0353c3fe..6f1ff04ee195 100644 --- a/drivers/thunderbolt/tb_regs.h +++ b/drivers/thunderbolt/tb_regs.h @@ -1,12 +1,13 @@ /* SPDX-License-Identifier: GPL-2.0 */ /* - * Thunderbolt Cactus Ridge driver - Port/Switch config area registers + * Thunderbolt driver - Port/Switch config area registers * * Every thunderbolt device consists (logically) of a switch with multiple * ports. Every port contains up to four config regions (HOPS, PORT, SWITCH, * COUNTERS) which are used to configure the device. * * Copyright (c) 2014 Andreas Noever <andreas.noever@gmail.com> + * Copyright (C) 2018, Intel Corporation */ #ifndef _TB_REGS -- 2.18.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] thunderbolt: Add Intel as copyright holder 2018-09-03 13:33 ` [PATCH 3/3] thunderbolt: Add Intel as copyright holder Mika Westerberg @ 2018-09-03 20:18 ` Yehezkel Bernat 2018-09-04 9:40 ` Mika Westerberg 0 siblings, 1 reply; 10+ messages in thread From: Yehezkel Bernat @ 2018-09-03 20:18 UTC (permalink / raw) To: Mika Westerberg; +Cc: LKML, Andreas Noever, michael.jamet, lukas > * Copyright (c) 2014 Andreas Noever <andreas.noever@gmail.com> > + * Copyright (C) 2018, Intel Corporation Nitpicking: (c) or (C)? I can't find anything in the documentation and both are found in various files. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] thunderbolt: Add Intel as copyright holder 2018-09-03 20:18 ` Yehezkel Bernat @ 2018-09-04 9:40 ` Mika Westerberg 0 siblings, 0 replies; 10+ messages in thread From: Mika Westerberg @ 2018-09-04 9:40 UTC (permalink / raw) To: Yehezkel Bernat; +Cc: LKML, Andreas Noever, michael.jamet, lukas On Mon, Sep 03, 2018 at 11:18:49PM +0300, Yehezkel Bernat wrote: > > * Copyright (c) 2014 Andreas Noever <andreas.noever@gmail.com> > > + * Copyright (C) 2018, Intel Corporation > > Nitpicking: (c) or (C)? > I can't find anything in the documentation and both are found in various files. I don't think there are any rules for it. I've been using (C) typically even if it is not "consistent" with other copyrights in the same file. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-09-06 10:48 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-09-03 13:33 [PATCH 0/3] thunderbolt: Miscellaneous cleanups Mika Westerberg 2018-09-03 13:33 ` [PATCH 1/3] thunderbolt: Make the driver less verbose Mika Westerberg 2018-09-05 9:05 ` Lukas Wunner 2018-09-05 9:54 ` Mika Westerberg 2018-09-06 8:41 ` Lukas Wunner 2018-09-06 10:47 ` Mika Westerberg 2018-09-03 13:33 ` [PATCH 2/3] thunderbolt: Convert rest of the driver files to use SPDX identifier Mika Westerberg 2018-09-03 13:33 ` [PATCH 3/3] thunderbolt: Add Intel as copyright holder Mika Westerberg 2018-09-03 20:18 ` Yehezkel Bernat 2018-09-04 9:40 ` Mika Westerberg
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).