[PATCH 1/2] thunderbolt: Disable ports that are not implemented

[PATCH 1/2] thunderbolt: Disable ports that are not implemented

[Mika Westerberg]

From: "Nikunj A. Dadhania" <nikunj.dadhania@linux.intel.com>

Commit 4caf2511ec49 ("thunderbolt: Add trivial .shutdown") exposes a bug
in the Thunderbolt driver, that frees an unallocated id, resulting in the
following spinlock bad magic bug.

[ 20.633803] BUG: spinlock bad magic on CPU#4, halt/3313
[ 20.640030] lock: 0xffff92e6ad5c97e0, .magic: 00000000, .owner: <none>/-1, .owner_cpu: 0
[ 20.672139] Call Trace:
[ 20.675032] dump_stack+0x97/0xdb
[ 20.678950] ? spin_bug+0xa5/0xb0
[ 20.682865] do_raw_spin_lock+0x68/0x98
[ 20.687397] _raw_spin_lock_irqsave+0x3f/0x5d
[ 20.692535] ida_destroy+0x4f/0x124
[ 20.696657] tb_switch_release+0x6d/0xfd
[ 20.701295] device_release+0x2c/0x7d
[ 20.705622] kobject_put+0x8e/0xac
[ 20.709637] tb_stop+0x55/0x66
[ 20.713243] tb_domain_remove+0x36/0x62
[ 20.717774] nhi_remove+0x4d/0x58

Fix the issue by disabling ports that are enabled as per the EEPROM, but
not implemented. While at it, update the kernel doc for the disabled
field, to reflect this.

Cc: stable@vger.kernel.org
Fixes: 4caf2511ec49 (thunderbolt: Add trivial .shutdown)
Reported-by: Srikanth Nandamuri <srikanth.nandamuri@intel.com>
Signed-off-by: Nikunj A. Dadhania <nikunj.dadhania@linux.intel.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/thunderbolt/switch.c | 1 +
 drivers/thunderbolt/tb.h     | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
index 712395f518b8..698c52775eec 100644
--- a/drivers/thunderbolt/switch.c
+++ b/drivers/thunderbolt/switch.c
@@ -684,6 +684,7 @@ static int tb_init_port(struct tb_port *port)
 		if (res == -ENODEV) {
 			tb_dbg(port->sw->tb, " Port %d: not implemented\n",
 			       port->port);
+			port->disabled = true;
 			return 0;
 		}
 		return res;
diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
index a413d55b5f8b..df08f6d7aaa0 100644
--- a/drivers/thunderbolt/tb.h
+++ b/drivers/thunderbolt/tb.h
@@ -186,7 +186,7 @@ struct tb_switch {
  * @cap_adap: Offset of the adapter specific capability (%0 if not present)
  * @cap_usb4: Offset to the USB4 port capability (%0 if not present)
  * @port: Port number on switch
- * @disabled: Disabled by eeprom
+ * @disabled: Disabled by eeprom or enabled, but not implemented
  * @bonded: true if the port is bonded (two lanes combined as one)
  * @dual_link_port: If the switch is connected using two ports, points
  *		    to the other port.
-- 
2.28.0

[PATCH 2/2] thunderbolt: Use maximum USB3 link rate when reclaiming if link is not up

[Mika Westerberg]

If the USB3 link is not up the actual link rate is 0 so when reclaiming
bandwidth we should look at maximum supported link rate instead.

Cc: stable@vger.kernel.org
Fixes: 0bd680cd900c ("thunderbolt: Add USB3 bandwidth management")
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/thunderbolt/tunnel.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/thunderbolt/tunnel.c b/drivers/thunderbolt/tunnel.c
index 2aae2c76d880..d50e5b93632a 100644
--- a/drivers/thunderbolt/tunnel.c
+++ b/drivers/thunderbolt/tunnel.c
@@ -951,10 +951,18 @@ static void tb_usb3_reclaim_available_bandwidth(struct tb_tunnel *tunnel,
 	int ret, max_rate, allocate_up, allocate_down;
 
 	ret = usb4_usb3_port_actual_link_rate(tunnel->src_port);
-	if (ret <= 0) {
-		tb_tunnel_warn(tunnel, "tunnel is not up\n");
+	if (ret < 0) {
+		tb_tunnel_warn(tunnel, "failed to read actual link rate\n");
 		return;
+	} else if (!ret) {
+		/* Use maximum link rate if the link valid is not set */
+		ret = usb4_usb3_port_max_link_rate(tunnel->src_port);
+		if (ret < 0) {
+			tb_tunnel_warn(tunnel, "failed to read maximum link rate\n");
+			return;
+		}
 	}
+
 	/*
 	 * 90% of the max rate can be allocated for isochronous
 	 * transfers.
-- 
2.28.0

Re: [PATCH 1/2] thunderbolt: Disable ports that are not implemented

[Yehezkel Bernat]

> - * @disabled: Disabled by eeprom
> + * @disabled: Disabled by eeprom or enabled, but not implemented

I'm not a native speaker, so I'm not sure about it, but maybe the comma here is
superfluous and just confuses the reader. To me it looks like it means
"(disabled
|| enabled) && !implemented" instead of "disabled || (enabled && !implemented)".
Any opinion?

Re: [PATCH 1/2] thunderbolt: Disable ports that are not implemented

[Mika Westerberg]

On Wed, Aug 19, 2020 at 02:54:39PM +0300, Yehezkel Bernat wrote:
> > - * @disabled: Disabled by eeprom
> > + * @disabled: Disabled by eeprom or enabled, but not implemented
> 
> I'm not a native speaker, so I'm not sure about it, but maybe the comma here is
> superfluous and just confuses the reader. To me it looks like it means
> "(disabled
> || enabled) && !implemented" instead of "disabled || (enabled && !implemented)".
> Any opinion?

For me (also non-native speaker) I don't see a difference but sure I can
remove it :)

Re: [PATCH 1/2] thunderbolt: Disable ports that are not implemented

[Nikunj A. Dadhania]

On 8/19/2020 6:15 PM, Mika Westerberg wrote:
> On Wed, Aug 19, 2020 at 02:54:39PM +0300, Yehezkel Bernat wrote:
>>> - * @disabled: Disabled by eeprom
>>> + * @disabled: Disabled by eeprom or enabled, but not implemented
>>
>> I'm not a native speaker, so I'm not sure about it, but maybe the comma here is
>> superfluous and just confuses the reader. To me it looks like it means
>> "(disabled
>> || enabled) && !implemented" instead of "disabled || (enabled && !implemented)". >> Any opinion?
> 
> For me (also non-native speaker) I don't see a difference but sure I can
> remove it :)
> 

I meant the second - "disabled || (enabled && !implemented)"
(also non-native speaker). If the comma confuses the reader please 
remove it.

Regards
Nikunj

Re: [PATCH 1/2] thunderbolt: Disable ports that are not implemented

[Mika Westerberg]

On Thu, Aug 20, 2020 at 05:31:08PM +0530, Nikunj A. Dadhania wrote:
> On 8/19/2020 6:15 PM, Mika Westerberg wrote:
> > On Wed, Aug 19, 2020 at 02:54:39PM +0300, Yehezkel Bernat wrote:
> > > > - * @disabled: Disabled by eeprom
> > > > + * @disabled: Disabled by eeprom or enabled, but not implemented
> > > 
> > > I'm not a native speaker, so I'm not sure about it, but maybe the comma here is
> > > superfluous and just confuses the reader. To me it looks like it means
> > > "(disabled
> > > || enabled) && !implemented" instead of "disabled || (enabled && !implemented)". >> Any opinion?
> > 
> > For me (also non-native speaker) I don't see a difference but sure I can
> > remove it :)
> > 
> 
> I meant the second - "disabled || (enabled && !implemented)"
> (also non-native speaker). If the comma confuses the reader please remove
> it.

Removed comma from the comment and applied to fixes, thanks!

Re: [PATCH 2/2] thunderbolt: Use maximum USB3 link rate when reclaiming if link is not up

[Mika Westerberg]

On Wed, Aug 19, 2020 at 02:27:16PM +0300, Mika Westerberg wrote:
> If the USB3 link is not up the actual link rate is 0 so when reclaiming
> bandwidth we should look at maximum supported link rate instead.
> 
> Cc: stable@vger.kernel.org
> Fixes: 0bd680cd900c ("thunderbolt: Add USB3 bandwidth management")
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Applied to fixes.