diff mbox series

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

Message ID 20200819112716.59074-1-mika.westerberg@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series [1/2] thunderbolt: Disable ports that are not implemented | expand

Commit Message

Mika Westerberg Aug. 19, 2020, 11:27 a.m. UTC 
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(-)

Comments

Yehezkel Bernat Aug. 19, 2020, 11:54 a.m. UTC | #1 
> - * @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?
Mika Westerberg Aug. 19, 2020, 12:45 p.m. UTC | #2 
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 :)
Nikunj A. Dadhania Aug. 20, 2020, 12:01 p.m. UTC | #3 
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
Mika Westerberg Aug. 25, 2020, 8:32 a.m. UTC | #4 
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!
diff mbox series

Patch

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.