linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] thunderbolt: Fixes for few reported issues
@ 2019-10-01 10:29 Mika Westerberg
  2019-10-01 10:29 ` [PATCH 1/3] thunderbolt: Read DP IN adapter first two dwords in one go Mika Westerberg
                   ` (3 more replies)
  0 siblings, 4 replies; 6+ messages in thread
From: Mika Westerberg @ 2019-10-01 10:29 UTC (permalink / raw)
  To: linux-kernel
  Cc: Andreas Noever, Michael Jamet, Yehezkel Bernat, Mika Westerberg,
	Lukas Wunner, Brad Campbell, Dominik Brodowski, Bjorn Helgaas,
	Nicholas Johnson, linux-pci

Hi all,

This series includes fixes for a couple of issues people have reported:

  - Discovering DP path fails on Light Ridge based iMac with two devices
    (monitors) connected.

  - There is a lockdep splat on Dominik's system when plugging in Thunderbolt
    dock.

  - Nicholas spotted that we do unnecessary PCI config space read when
    issuing LC mailbox command on ICL.

Mika Westerberg (3):
  thunderbolt: Read DP IN adapter first two dwords in one go
  thunderbolt: Fix lockdep circular locking depedency warning
  thunderbolt: Drop unnecessary read when writing LC command in Ice Lake

 drivers/thunderbolt/nhi_ops.c |  1 -
 drivers/thunderbolt/switch.c  | 28 +++++++++++-----------------
 2 files changed, 11 insertions(+), 18 deletions(-)

-- 
2.23.0


^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH 1/3] thunderbolt: Read DP IN adapter first two dwords in one go
  2019-10-01 10:29 [PATCH 0/3] thunderbolt: Fixes for few reported issues Mika Westerberg
@ 2019-10-01 10:29 ` Mika Westerberg
  2019-10-04  2:43   ` Brad Campbell
  2019-10-01 10:29 ` [PATCH 2/3] thunderbolt: Fix lockdep circular locking depedency warning Mika Westerberg
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 6+ messages in thread
From: Mika Westerberg @ 2019-10-01 10:29 UTC (permalink / raw)
  To: linux-kernel
  Cc: Andreas Noever, Michael Jamet, Yehezkel Bernat, Mika Westerberg,
	Lukas Wunner, Brad Campbell, Dominik Brodowski, Bjorn Helgaas,
	Nicholas Johnson, linux-pci

When we discover existing DP tunnels the code checks whether DP IN
adapter port is enabled by calling tb_dp_port_is_enabled() before it
continues the discovery process. On Light Ridge (gen 1) controller
reading only the first dword of the DP IN config space causes subsequent
access to the same DP IN port path config space to fail or return
invalid data as can be seen in the below splat:

  thunderbolt 0000:07:00.0: CFG_ERROR(0:d): Invalid config space or offset
  Call Trace:
   tb_cfg_read+0xb9/0xd0
   __tb_path_deactivate_hop+0x98/0x210
   tb_path_activate+0x228/0x7d0
   tb_tunnel_restart+0x95/0x200
   tb_handle_hotplug+0x30e/0x630
   process_one_work+0x1b4/0x340
   worker_thread+0x44/0x3d0
   kthread+0xeb/0x120
   ? process_one_work+0x340/0x340
   ? kthread_park+0xa0/0xa0
   ret_from_fork+0x1f/0x30

If both DP In adapter config dwords are read in one go the issue does
not reproduce. This is likely firmware bug but we can work it around by
always reading the two dwords in one go. There should be no harm for
other controllers either so can do it unconditionally.

Link: https://lkml.org/lkml/2019/8/28/160
Reported-by: Brad Campbell <lists2009@fnarfbargle.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/thunderbolt/switch.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
index 410bf1bceeee..8e712fbf8233 100644
--- a/drivers/thunderbolt/switch.c
+++ b/drivers/thunderbolt/switch.c
@@ -896,12 +896,13 @@ int tb_dp_port_set_hops(struct tb_port *port, unsigned int video,
  */
 bool tb_dp_port_is_enabled(struct tb_port *port)
 {
-	u32 data;
+	u32 data[2];
 
-	if (tb_port_read(port, &data, TB_CFG_PORT, port->cap_adap, 1))
+	if (tb_port_read(port, data, TB_CFG_PORT, port->cap_adap,
+			 ARRAY_SIZE(data)))
 		return false;
 
-	return !!(data & (TB_DP_VIDEO_EN | TB_DP_AUX_EN));
+	return !!(data[0] & (TB_DP_VIDEO_EN | TB_DP_AUX_EN));
 }
 
 /**
@@ -914,19 +915,21 @@ bool tb_dp_port_is_enabled(struct tb_port *port)
  */
 int tb_dp_port_enable(struct tb_port *port, bool enable)
 {
-	u32 data;
+	u32 data[2];
 	int ret;
 
-	ret = tb_port_read(port, &data, TB_CFG_PORT, port->cap_adap, 1);
+	ret = tb_port_read(port, data, TB_CFG_PORT, port->cap_adap,
+			   ARRAY_SIZE(data));
 	if (ret)
 		return ret;
 
 	if (enable)
-		data |= TB_DP_VIDEO_EN | TB_DP_AUX_EN;
+		data[0] |= TB_DP_VIDEO_EN | TB_DP_AUX_EN;
 	else
-		data &= ~(TB_DP_VIDEO_EN | TB_DP_AUX_EN);
+		data[0] &= ~(TB_DP_VIDEO_EN | TB_DP_AUX_EN);
 
-	return tb_port_write(port, &data, TB_CFG_PORT, port->cap_adap, 1);
+	return tb_port_write(port, data, TB_CFG_PORT, port->cap_adap,
+			     ARRAY_SIZE(data));
 }
 
 /* switch utility functions */
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH 2/3] thunderbolt: Fix lockdep circular locking depedency warning
  2019-10-01 10:29 [PATCH 0/3] thunderbolt: Fixes for few reported issues Mika Westerberg
  2019-10-01 10:29 ` [PATCH 1/3] thunderbolt: Read DP IN adapter first two dwords in one go Mika Westerberg
@ 2019-10-01 10:29 ` Mika Westerberg
  2019-10-01 10:29 ` [PATCH 3/3] thunderbolt: Drop unnecessary read when writing LC command in Ice Lake Mika Westerberg
  2019-10-08  9:15 ` [PATCH 0/3] thunderbolt: Fixes for few reported issues Mika Westerberg
  3 siblings, 0 replies; 6+ messages in thread
From: Mika Westerberg @ 2019-10-01 10:29 UTC (permalink / raw)
  To: linux-kernel
  Cc: Andreas Noever, Michael Jamet, Yehezkel Bernat, Mika Westerberg,
	Lukas Wunner, Brad Campbell, Dominik Brodowski, Bjorn Helgaas,
	Nicholas Johnson, linux-pci

When lockdep is enabled, plugging Thunderbolt dock on Dominik's laptop
triggers following splat:

  ======================================================
  WARNING: possible circular locking dependency detected
  5.3.0-rc6+ #1 Tainted: G                T
  ------------------------------------------------------
  pool-/usr/lib/b/1258 is trying to acquire lock:
  000000005ab0ad43 (pci_rescan_remove_lock){+.+.}, at: authorized_store+0xe8/0x210

  but task is already holding lock:
  00000000bfb796b5 (&tb->lock){+.+.}, at: authorized_store+0x7c/0x210

  which lock already depends on the new lock.

  the existing dependency chain (in reverse order) is:

  -> #1 (&tb->lock){+.+.}:
         __mutex_lock+0xac/0x9a0
         tb_domain_add+0x2d/0x130
         nhi_probe+0x1dd/0x330
         pci_device_probe+0xd2/0x150
         really_probe+0xee/0x280
         driver_probe_device+0x50/0xc0
         bus_for_each_drv+0x84/0xd0
         __device_attach+0xe4/0x150
         pci_bus_add_device+0x4e/0x70
         pci_bus_add_devices+0x2e/0x66
         pci_bus_add_devices+0x59/0x66
         pci_bus_add_devices+0x59/0x66
         enable_slot+0x344/0x450
         acpiphp_check_bridge.part.0+0x119/0x150
         acpiphp_hotplug_notify+0xaa/0x140
         acpi_device_hotplug+0xa2/0x3f0
         acpi_hotplug_work_fn+0x1a/0x30
         process_one_work+0x234/0x580
         worker_thread+0x50/0x3b0
         kthread+0x10a/0x140
         ret_from_fork+0x3a/0x50

  -> #0 (pci_rescan_remove_lock){+.+.}:
         __lock_acquire+0xe54/0x1ac0
         lock_acquire+0xb8/0x1b0
         __mutex_lock+0xac/0x9a0
         authorized_store+0xe8/0x210
         kernfs_fop_write+0x125/0x1b0
         vfs_write+0xc2/0x1d0
         ksys_write+0x6c/0xf0
         do_syscall_64+0x50/0x180
         entry_SYSCALL_64_after_hwframe+0x49/0xbe

  other info that might help us debug this:
   Possible unsafe locking scenario:
         CPU0                    CPU1
         ----                    ----
    lock(&tb->lock);
                                 lock(pci_rescan_remove_lock);
                                 lock(&tb->lock);
    lock(pci_rescan_remove_lock);

   *** DEADLOCK ***
  5 locks held by pool-/usr/lib/b/1258:
   #0: 000000003df1a1ad (&f->f_pos_lock){+.+.}, at: __fdget_pos+0x4d/0x60
   #1: 0000000095a40b02 (sb_writers#6){.+.+}, at: vfs_write+0x185/0x1d0
   #2: 0000000017a7d714 (&of->mutex){+.+.}, at: kernfs_fop_write+0xf2/0x1b0
   #3: 000000004f262981 (kn->count#208){.+.+}, at: kernfs_fop_write+0xfa/0x1b0
   #4: 00000000bfb796b5 (&tb->lock){+.+.}, at: authorized_store+0x7c/0x210

  stack backtrace:
  CPU: 0 PID: 1258 Comm: pool-/usr/lib/b Tainted: G                T 5.3.0-rc6+ #1

On an system using ACPI hotplug the host router gets hotplugged first and then
the firmware starts sending notifications about connected devices so the above
scenario should not happen in reality. However, after taking a second
look at commit a03e828915c0 ("thunderbolt: Serialize PCIe tunnel
creation with PCI rescan") that introduced the locking, I don't think it
is actually correct. It may have cured the symptom but probably the real
root cause was somewhere closer to PCI stack and possibly is already
fixed with recent kernels. I also tried to reproduce the original issue
with the commit reverted but could not.

So to keep lockdep happy and the code bit less complex drop calls to
pci_lock_rescan_remove()/pci_unlock_rescan_remove() in
tb_switch_set_authorized() effectively reverting a03e828915c0.

Link: https://lkml.org/lkml/2019/8/30/513
Fixes: a03e828915c0 ("thunderbolt: Serialize PCIe tunnel creation with PCI rescan")
Reported-by: Dominik Brodowski <linux@dominikbrodowski.net>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/thunderbolt/switch.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
index 8e712fbf8233..5ea8db667e83 100644
--- a/drivers/thunderbolt/switch.c
+++ b/drivers/thunderbolt/switch.c
@@ -1034,13 +1034,6 @@ static int tb_switch_set_authorized(struct tb_switch *sw, unsigned int val)
 	if (sw->authorized)
 		goto unlock;
 
-	/*
-	 * Make sure there is no PCIe rescan ongoing when a new PCIe
-	 * tunnel is created. Otherwise the PCIe rescan code might find
-	 * the new tunnel too early.
-	 */
-	pci_lock_rescan_remove();
-
 	switch (val) {
 	/* Approve switch */
 	case 1:
@@ -1060,8 +1053,6 @@ static int tb_switch_set_authorized(struct tb_switch *sw, unsigned int val)
 		break;
 	}
 
-	pci_unlock_rescan_remove();
-
 	if (!ret) {
 		sw->authorized = val;
 		/* Notify status change to the userspace */
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH 3/3] thunderbolt: Drop unnecessary read when writing LC command in Ice Lake
  2019-10-01 10:29 [PATCH 0/3] thunderbolt: Fixes for few reported issues Mika Westerberg
  2019-10-01 10:29 ` [PATCH 1/3] thunderbolt: Read DP IN adapter first two dwords in one go Mika Westerberg
  2019-10-01 10:29 ` [PATCH 2/3] thunderbolt: Fix lockdep circular locking depedency warning Mika Westerberg
@ 2019-10-01 10:29 ` Mika Westerberg
  2019-10-08  9:15 ` [PATCH 0/3] thunderbolt: Fixes for few reported issues Mika Westerberg
  3 siblings, 0 replies; 6+ messages in thread
From: Mika Westerberg @ 2019-10-01 10:29 UTC (permalink / raw)
  To: linux-kernel
  Cc: Andreas Noever, Michael Jamet, Yehezkel Bernat, Mika Westerberg,
	Lukas Wunner, Brad Campbell, Dominik Brodowski, Bjorn Helgaas,
	Nicholas Johnson, linux-pci

The read is not needed as we overwrite the returned value in the next
line anyway so drop it.

Fixes: 3cdb9446a117 ("thunderbolt: Add support for Intel Ice Lake")
Reported-by: Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/thunderbolt/nhi_ops.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/thunderbolt/nhi_ops.c b/drivers/thunderbolt/nhi_ops.c
index 61cd09cef943..6795851aac95 100644
--- a/drivers/thunderbolt/nhi_ops.c
+++ b/drivers/thunderbolt/nhi_ops.c
@@ -80,7 +80,6 @@ static void icl_nhi_lc_mailbox_cmd(struct tb_nhi *nhi, enum icl_lc_mailbox_cmd c
 {
 	u32 data;
 
-	pci_read_config_dword(nhi->pdev, VS_CAP_19, &data);
 	data = (cmd << VS_CAP_19_CMD_SHIFT) & VS_CAP_19_CMD_MASK;
 	pci_write_config_dword(nhi->pdev, VS_CAP_19, data | VS_CAP_19_VALID);
 }
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH 1/3] thunderbolt: Read DP IN adapter first two dwords in one go
  2019-10-01 10:29 ` [PATCH 1/3] thunderbolt: Read DP IN adapter first two dwords in one go Mika Westerberg
@ 2019-10-04  2:43   ` Brad Campbell
  0 siblings, 0 replies; 6+ messages in thread
From: Brad Campbell @ 2019-10-04  2:43 UTC (permalink / raw)
  To: Mika Westerberg, linux-kernel
  Cc: Andreas Noever, Michael Jamet, Yehezkel Bernat, Lukas Wunner,
	Dominik Brodowski, Bjorn Helgaas, Nicholas Johnson, linux-pci

On 1/10/19 6:29 pm, Mika Westerberg wrote:
> When we discover existing DP tunnels the code checks whether DP IN
> adapter port is enabled by calling tb_dp_port_is_enabled() before it
> continues the discovery process. On Light Ridge (gen 1) controller
> reading only the first dword of the DP IN config space causes subsequent
> access to the same DP IN port path config space to fail or return
> invalid data as can be seen in the below splat:
> 
>    thunderbolt 0000:07:00.0: CFG_ERROR(0:d): Invalid config space or offset
>    Call Trace:
>     tb_cfg_read+0xb9/0xd0
>     __tb_path_deactivate_hop+0x98/0x210
>     tb_path_activate+0x228/0x7d0
>     tb_tunnel_restart+0x95/0x200
>     tb_handle_hotplug+0x30e/0x630
>     process_one_work+0x1b4/0x340
>     worker_thread+0x44/0x3d0
>     kthread+0xeb/0x120
>     ? process_one_work+0x340/0x340
>     ? kthread_park+0xa0/0xa0
>     ret_from_fork+0x1f/0x30
> 
> If both DP In adapter config dwords are read in one go the issue does
> not reproduce. This is likely firmware bug but we can work it around by
> always reading the two dwords in one go. There should be no harm for
> other controllers either so can do it unconditionally.
> 
> Link: https://lkml.org/lkml/2019/8/28/160
> Reported-by: Brad Campbell <lists2009@fnarfbargle.com>

Tested-by: Brad Campbell <lists2009@fnarfbargle.com>

> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>   drivers/thunderbolt/switch.c | 19 +++++++++++--------
>   1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
> index 410bf1bceeee..8e712fbf8233 100644
> --- a/drivers/thunderbolt/switch.c
> +++ b/drivers/thunderbolt/switch.c
> @@ -896,12 +896,13 @@ int tb_dp_port_set_hops(struct tb_port *port, unsigned int video,
>    */
>   bool tb_dp_port_is_enabled(struct tb_port *port)
>   {
> -	u32 data;
> +	u32 data[2];
>   
> -	if (tb_port_read(port, &data, TB_CFG_PORT, port->cap_adap, 1))
> +	if (tb_port_read(port, data, TB_CFG_PORT, port->cap_adap,
> +			 ARRAY_SIZE(data)))
>   		return false;
>   
> -	return !!(data & (TB_DP_VIDEO_EN | TB_DP_AUX_EN));
> +	return !!(data[0] & (TB_DP_VIDEO_EN | TB_DP_AUX_EN));
>   }
>   
>   /**
> @@ -914,19 +915,21 @@ bool tb_dp_port_is_enabled(struct tb_port *port)
>    */
>   int tb_dp_port_enable(struct tb_port *port, bool enable)
>   {
> -	u32 data;
> +	u32 data[2];
>   	int ret;
>   
> -	ret = tb_port_read(port, &data, TB_CFG_PORT, port->cap_adap, 1);
> +	ret = tb_port_read(port, data, TB_CFG_PORT, port->cap_adap,
> +			   ARRAY_SIZE(data));
>   	if (ret)
>   		return ret;
>   
>   	if (enable)
> -		data |= TB_DP_VIDEO_EN | TB_DP_AUX_EN;
> +		data[0] |= TB_DP_VIDEO_EN | TB_DP_AUX_EN;
>   	else
> -		data &= ~(TB_DP_VIDEO_EN | TB_DP_AUX_EN);
> +		data[0] &= ~(TB_DP_VIDEO_EN | TB_DP_AUX_EN);
>   
> -	return tb_port_write(port, &data, TB_CFG_PORT, port->cap_adap, 1);
> +	return tb_port_write(port, data, TB_CFG_PORT, port->cap_adap,
> +			     ARRAY_SIZE(data));
>   }
>   
>   /* switch utility functions */
> 


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH 0/3] thunderbolt: Fixes for few reported issues
  2019-10-01 10:29 [PATCH 0/3] thunderbolt: Fixes for few reported issues Mika Westerberg
                   ` (2 preceding siblings ...)
  2019-10-01 10:29 ` [PATCH 3/3] thunderbolt: Drop unnecessary read when writing LC command in Ice Lake Mika Westerberg
@ 2019-10-08  9:15 ` Mika Westerberg
  3 siblings, 0 replies; 6+ messages in thread
From: Mika Westerberg @ 2019-10-08  9:15 UTC (permalink / raw)
  To: linux-kernel
  Cc: Andreas Noever, Michael Jamet, Yehezkel Bernat, Lukas Wunner,
	Brad Campbell, Dominik Brodowski, Bjorn Helgaas,
	Nicholas Johnson, linux-pci

On Tue, Oct 01, 2019 at 01:29:02PM +0300, Mika Westerberg wrote:
> Mika Westerberg (3):
>   thunderbolt: Read DP IN adapter first two dwords in one go
>   thunderbolt: Fix lockdep circular locking depedency warning
>   thunderbolt: Drop unnecessary read when writing LC command in Ice Lake

Applied to thunderbolt.git/fixes.

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-10-08  9:16 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-01 10:29 [PATCH 0/3] thunderbolt: Fixes for few reported issues Mika Westerberg
2019-10-01 10:29 ` [PATCH 1/3] thunderbolt: Read DP IN adapter first two dwords in one go Mika Westerberg
2019-10-04  2:43   ` Brad Campbell
2019-10-01 10:29 ` [PATCH 2/3] thunderbolt: Fix lockdep circular locking depedency warning Mika Westerberg
2019-10-01 10:29 ` [PATCH 3/3] thunderbolt: Drop unnecessary read when writing LC command in Ice Lake Mika Westerberg
2019-10-08  9:15 ` [PATCH 0/3] thunderbolt: Fixes for few reported issues 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).